Repository navigation
refactor(spec): address code-quality audit findings for internal/core/spec - #2513
Merged
Merged
Conversation
Collapse the duplicated public configuration model: LoadOption now configures the single build-options struct directly, removing the field-by-field copy between LoadOptions and BuildOpts and the drift risk that came with it. Unexport the build context, build options, and build flags, which carried only package-private state and had no callers outside the package. Drop LoadYAMLWithOpts, LoadBaseConfig, BuiltinActionNames, StepTypeNames, the unreachable ErrInvalidJSONFile sentinel, and the unused BuildFlagNone. Preserve error causes that were previously discarded: DAG env resolution now wraps the resolver error instead of restating the raw expression, and base-config reads wrap the underlying I/O error so callers can classify it. expandHomeDir reports a home-lookup failure rather than silently resolving a home-relative path against another directory, and the working-directory fallback drops its unreachable branch. Copy Tools and WebSearch when building DAG-level LLM configuration. Both builders decode the same type, but only the step builder copied these two fields, so a root llm.tools or llm.web_search was accepted and then lost before a chat step inherited the configuration. Scope builtin artifact-action detection to executable roots and skip parameter payloads, so free-form data carrying an "action" key no longer counts as an artifact action and no longer fails validation when artifacts are explicitly disabled.
…gnment The DAG and step build pipelines selected output fields by name through reflect.Value.FieldByName, so a renamed or misspelled field silently dropped its value and an incompatible builder result could panic inside reflect.Set. Neither failure was visible to the compiler. Each stage entry now pairs a builder with a typed assignment function, so the field and the built value are checked at compile time. The worker selector, previously the one multi-field case justifying the reflective escape hatch, becomes a plain typed function, and the exported Transformer interface is gone with it. Parse each shared step declaration once. stdout, stderr, and shell each fed several single-field builders that reparsed the same declaration, so one malformed value produced several equivalent errors that error deduplication could not collapse. One entry per declaration now assigns every field derived from it and reports a single error.
Three artifact detectors each implemented recursive reflection over the same configuration graph: pointer and interface unwrapping plus map, slice, array, and exported-struct traversal, repeated across roughly two hundred lines that had to stay synchronized. A single visitor now owns the traversal and supplies map-key and struct-field context to per-search predicates, which also unifies the map-key and struct-field spellings each detector previously matched separately. Value() any expansion stays specific to run-artifacts-dir references, and each detector keeps its own root scope. Three recursive clone helpers recognized different composite types, so the same decoded shape was deep-copied on one path and left aliased on another. The shared helper now covers the full map and slice union and backs the harness path; the Kubernetes helper is reduced to a wrapper that preserves its named config type. Add regression coverage for DAG-level LLM inheritance and for artifact action detection over parameter payloads.
…test Extract the post-build rules every step must satisfy into one helper, and give each steps syntax its own builder. Flat arrays, nested parallel arrays, and map form previously repeated the same validation sequence, so a new rule had to be added in three branches and validity could drift by syntax. Array entries now normalize into ordered groups that share one build, validation, and dependency loop; map form keeps its forced names and deterministic sort in a small adapter; router transformation runs once on the final list. Unexport the typed union decode hook, which recognizes unexported manifest types and had no external construction path. Remove the base-config file loader and its read helper. Base config is loaded through loadBaseDefinition; this path had no remaining callers. Split the step build test, which had grown to roughly 860 lines dispatching unrelated command, executor, continue-on, retry, repeat, signal, ID, and precondition cases, into focused suites. The blocks move unchanged, keeping their tables, parallelism, and assertions.
📝 WalkthroughWalkthroughThe PR makes builder and loader context types private, migrates loading to functional options, replaces reflective DAG and step transformations with typed callbacks, improves artifact and schema error handling, and expands validation coverage. ChangesSpec build pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant LoadYAML
participant buildContext
participant buildDAG
participant buildStep
participant coreDAG
Caller->>LoadYAML: Provide YAML and LoadOption values
LoadYAML->>buildContext: Create internal build context
buildContext->>buildDAG: Build DAG fields and metadata
buildDAG->>buildStep: Build typed step transformations
buildStep->>coreDAG: Assign validated steps
coreDAG-->>Caller: Return built DAG
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/core/spec/dag.go (1)
3207-3246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the redundant
ctx buildContextparameter.
stepBuildContextembedsbuildContext, sobuildCtx.buildContextsupplies the same value. Two context parameters allow the caller to pass mismatched contexts.♻️ Proposed signature change
func buildStepsFromArray( buildCtx stepBuildContext, - ctx buildContext, entries []any, result *core.DAG, names map[string]struct{}, defs *defaults, ) ([]core.Step, error) {Then use
buildCtx.buildContextfornormalizeStepDataandstepGroupFromRaw, and update the call site at Line 3168.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/spec/dag.go` around lines 3207 - 3246, Remove the redundant ctx buildContext parameter from buildStepsFromArray and update its caller accordingly. Within the function, use buildCtx.buildContext for normalizeStepData and stepGroupFromRaw so all operations share the embedded context and callers cannot provide mismatched values.internal/core/spec/step_types.go (1)
912-942: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHandle
map[any]anyinsidecloneAny.
cloneAnycan clone custom step fields copied intostepConfig, but a nestedmap[any]anyvalue is returned unchanged. That leaves shared nested state between the cloned config andresult.ExecutorConfig.Config, so later builder reads/writes to that value can mutate step state. Convertmap[any]anytomap[string]anyand recurse into values before returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/spec/step_types.go` around lines 912 - 942, Update cloneAny to handle map[any]any values by creating a map[string]any, converting each key to a string, and recursively cloning each value with cloneAny before returning the new map. Preserve existing handling for all other supported composite types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/spec/builder_test.go`:
- Around line 4348-4352: Extend the web-search inheritance assertions in this
test to verify that the chat step’s LLM WebSearch configuration preserves the
DAG-level max_uses value of 3. Keep the existing Enabled assertion and assert
the corresponding max-uses field on dag.Steps[0].LLM.WebSearch.
In `@internal/core/spec/loader.go`:
- Around line 371-378: Update applyWorkingDirFallback to recurse through every
LocalDAG attached to dag, applying the manifest file directory only when a DAG’s
WorkingDir is empty while preserving explicit child values; add a Load
regression test covering a multi-document manifest with a child DAG that omits
working_dir.
---
Nitpick comments:
In `@internal/core/spec/dag.go`:
- Around line 3207-3246: Remove the redundant ctx buildContext parameter from
buildStepsFromArray and update its caller accordingly. Within the function, use
buildCtx.buildContext for normalizeStepData and stepGroupFromRaw so all
operations share the embedded context and callers cannot provide mismatched
values.
In `@internal/core/spec/step_types.go`:
- Around line 912-942: Update cloneAny to handle map[any]any values by creating
a map[string]any, converting each key to a string, and recursively cloning each
value with cloneAny before returning the new map. Preserve existing handling for
all other supported composite types.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cc549db3-ba15-4193-87d9-c49a0f9b8350
📒 Files selected for processing (28)
internal/core/spec/builder.gointernal/core/spec/builder_test.gointernal/core/spec/consts.gointernal/core/spec/controller.gointernal/core/spec/dag.gointernal/core/spec/dag_test.gointernal/core/spec/defaults.gointernal/core/spec/dparams.gointernal/core/spec/dparams_runtime.gointernal/core/spec/human_task.gointernal/core/spec/kubernetes.gointernal/core/spec/llm_test.gointernal/core/spec/loader.gointernal/core/spec/loader_internal_test.gointernal/core/spec/loader_test.gointernal/core/spec/manifest_decoder.gointernal/core/spec/params.gointernal/core/spec/params_test.gointernal/core/spec/runtime_env_external_test.gointernal/core/spec/schema.gointernal/core/spec/schema_test.gointernal/core/spec/step.gointernal/core/spec/step_outputs_external_test.gointernal/core/spec/step_test.gointernal/core/spec/step_types.gointernal/core/spec/step_v2.gointernal/core/spec/variables.gointernal/core/spec/variables_test.go
💤 Files with no reviewable changes (1)
- internal/core/spec/step_v2.go
Comment on lines
+4348
to
+4352
| require.Len(t, dag.Steps, 1) | ||
| require.NotNil(t, dag.Steps[0].LLM) | ||
| assert.Equal(t, []string{"helper-dag"}, dag.Steps[0].LLM.Tools) | ||
| require.NotNil(t, dag.Steps[0].LLM.WebSearch) | ||
| assert.True(t, dag.Steps[0].LLM.WebSearch.Enabled) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test web_search.max_uses inheritance.
The test sets max_uses: 3 at DAG level but does not verify that the chat step receives it. A builder that copies only Enabled passes this test.
Proposed test update
require.NotNil(t, dag.Steps[0].LLM.WebSearch)
assert.True(t, dag.Steps[0].LLM.WebSearch.Enabled)
+ assert.Equal(t, 3, dag.Steps[0].LLM.WebSearch.MaxUses)As per coding guidelines, “Add or update tests appropriate to the changed code.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| require.Len(t, dag.Steps, 1) | |
| require.NotNil(t, dag.Steps[0].LLM) | |
| assert.Equal(t, []string{"helper-dag"}, dag.Steps[0].LLM.Tools) | |
| require.NotNil(t, dag.Steps[0].LLM.WebSearch) | |
| assert.True(t, dag.Steps[0].LLM.WebSearch.Enabled) | |
| require.Len(t, dag.Steps, 1) | |
| require.NotNil(t, dag.Steps[0].LLM) | |
| assert.Equal(t, []string{"helper-dag"}, dag.Steps[0].LLM.Tools) | |
| require.NotNil(t, dag.Steps[0].LLM.WebSearch) | |
| assert.True(t, dag.Steps[0].LLM.WebSearch.Enabled) | |
| assert.Equal(t, 3, dag.Steps[0].LLM.WebSearch.MaxUses) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/spec/builder_test.go` around lines 4348 - 4352, Extend the
web-search inheritance assertions in this test to verify that the chat step’s
LLM WebSearch configuration preserves the DAG-level max_uses value of 3. Keep
the existing Enabled assertion and assert the corresponding max-uses field on
dag.Steps[0].LLM.WebSearch.
Source: Coding guidelines
…ance Enabled alone did not distinguish a builder that copies the whole web-search configuration from one that copies only that flag.
Skipping every map entry named params also skipped a map-form step whose name is params, because that syntax uses the map key as the step name. A DAG declaring an artifact action under that name loaded without enabling artifacts, and the run then failed with no artifact directory. Scoping detection to executable roots is what fixes the reported case, in which a DAG-level params payload carrying an action key was treated as an artifact action; the name-based skip was not needed for it. Remove the skip, and with it the unused suppression path in the visitor. A parameter payload passed to a child DAG through with.params still counts as an artifact action. That predates this change and is unchanged here.
A params entry is a step name only inside a steps container; anywhere else it names a field. Detection now walks step declarations in those two modes rather than matching names without regard to position: container entries are steps whose keys carry no meaning for the search, and within a step a params entry is a payload handed to a child DAG while a steps entry opens a nested container. This drops the false positives where a payload passed through with.params counted as an artifact action, in steps, handlers, and custom step and action templates alike, while still detecting an action declared under any step name and in nested foreach steps.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses all 18 findings from a code-quality audit of
internal/core/spec. Thepackage was already clean under
go vetandgolangci-lint; every finding is acorrectness or maintainability issue those tools do not catch.
Net -85 lines. No behavior change except the four defects fixed below.
Defects fixed
DAG-level
llm.toolsandllm.web_searchwere silently dropped. The DAG andstep LLM builders decode the same type, but only the step builder copied these
two fields. A chat step that inherits DAG-level LLM configuration therefore
received no tools and no web search, despite both being part of the documented
root contract —
chat/executor.goreadscfg.Toolsfrom exactly that inheritedconfig.
Artifact-action detection scanned arbitrary data as step syntax. Detection
recursed over the whole intermediate DAG and treated any map key
actionwhosevalue began with
artifact.as an artifact action. A DAG carryingparams: [action: artifact.write]failed to load withartifact actions require artifacts.enabled to be truewhen artifacts wereexplicitly disabled. Detection is now scoped to executable roots — mirroring the
existing artifact-output detector — and skips parameter payloads.
Schema lookup reported every failure as "file not found." Permission and
resolution failures were indistinguishable from absence and lost their
errors.Is/errors.Aschains. Candidate order and fallback are unchanged; acandidate that failed for a reason other than absence is now reported as a load
failure carrying only those causes, so absence of earlier candidates cannot mask
it.
Two error causes were erased. DAG env resolution replaced the resolver error
with a sentinel plus the raw expression.
expandHomeDirswallowed aUserHomeDirfailure and returned the unexpanded path, which was later resolvedagainst an unrelated directory — so the eventual read error named a path the
caller never requested.
API surface
LoadOptionnow configures a single build-options struct.LoadOptionsandBuildOptsheld the same eleven fields and were copied field by field on everyload, so each new setting had to be added twice.
Unexported or removed, none of which had callers outside the package:
BuildContext,StepBuildContext,BuildOpts,BuildFlag,TypedUnionDecodeHook,LoadYAMLWithOpts,LoadBaseConfig,BuiltinActionNames,StepTypeNames,BuildFlagNone, and the unreachableErrInvalidJSONFilesentinel. The base-config file loader and its read helperare deleted outright — base config loads through
loadBaseDefinition.Structure
Typed field pipeline. DAG and step building selected output fields by name
through
reflect.Value.FieldByName, so a renamed field silently dropped itsvalue and an incompatible builder result could panic in
reflect.Set— neithervisible to the compiler. Each stage entry now pairs a builder with a typed
assignment function. The worker selector, the one multi-field case that
justified the reflective escape hatch, is a plain typed function, and the
exported
Transformerinterface is gone.Shared declarations parse once.
stdout,stderr, andshelleach fedseveral single-field builders that reparsed the same declaration, so one
malformed value produced several equivalent errors that deduplication could not
collapse.
One reflection visitor. Three artifact detectors each implemented the same
recursive traversal across roughly 200 lines that had to stay synchronized. Each
detector keeps its own root scope and predicates.
One clone helper.
cloneAny,cloneHarnessSpecValue, andcloneKubernetesValuerecognized different composite types, so the same decodedshape was deep-copied on one path and left aliased on another.
Shared step validation. Flat arrays, nested parallel arrays, and map form
repeated the same post-build validation, so a new rule had to land in three
branches and validity could drift by syntax.
Tests
TestBuildStephad grown to ~860 lines dispatching unrelated command, executor,continue-on, retry, repeat, signal, ID, and precondition cases. Split into six
focused suites; blocks move unchanged, all 71 subtests preserved.
Added regression coverage for the two defects with observable behavior:
DAG-level LLM inheritance, and artifact-action detection over parameter
payloads.
Verification
go build ./...,go test ./internal/core/... ./internal/runtime/..., andgolangci-lintunder bothGOOS=darwinandGOOS=windowsare clean.Provenance
Findings came from an LLM-orchestrated audit whose adversarial verification pass
confirmed 21 of 21 submitted findings. A 100% confirmation rate means that stage
was not discriminating, so each finding was re-validated against the code before
any change — the two behavioral defects were reproduced first. Two findings were
narrowed as a result: artifact-action scoping follows the existing detector's
root scope rather than a new per-field exclusion list, and
LoadBaseConfigisdeleted rather than privatized, since removing its only caller left it dead.
Summary by cubic
Refactors
internal/core/specto replace reflective builders with typed assignments, consolidate load options, and fix four correctness issues found by the audit. No behavior change beyond the listed bug fixes.Bug Fixes
toolsandweb_searchnow inherit correctly into chat steps, includingweb_search.max_uses.paramsas a payload only where it names a field; no false positives forwith.paramsin steps, handlers, or templates, while steps namedparamsand nestedforeachstill detect actions correctly.expandHomeDirpreserve underlying errors and avoid resolving to the wrong path.Migration
LoadYAMLWithOptswithLoadYAML(ctx, data, ...LoadOption);LoadOptionsis removed in favor of functionalLoadOptions.WithParams("..."),WithoutEval(), andSkipSchemaValidation()instead of flags/BuildOpts.WithBaseConfig(path);LoadBaseConfigis removed.BuildContext,StepBuildContext,BuildOpts,BuildFlag,TypedUnionDecodeHook,BuiltinActionNames,StepTypeNames,Transformer,ErrInvalidJSONFile.Written for commit 2f0f42e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Breaking Changes