Custom commands and workflows: complete task-runner replacement for go-task - #2882
Custom commands and workflows: complete task-runner replacement for go-task#2882Erik Osterman (Cloud Posse) (osterman) wants to merge 16 commits into
Conversation
…unner replacement Closes the remaining gaps that kept teams running go-task alongside Atmos: - Named cross-unit dependencies: dependencies.commands/dependencies.workflows on custom commands and workflows, with automatic dedup of identical invocations, parameterized invocations as distinct graph nodes, and concurrent-by-default execution via the existing scheduler. - Freshness-based step skipping: inputs.sources/artifacts.paths skip a step when nothing has changed since its last successful run (implicit when: checksum.changed); precondition.tools skips a step when a required tool is already on PATH (implicit when: "!precondition.success"). Exposes checksum.changed/timestamp.changed/ precondition.success as when: CEL facts, plus structured per-file records for custom comparisons. - continue: always step field, mirroring GitHub Actions' continue-on-error: a step's own failure is forgiven, later steps still run, overall exit status unaffected. - Fixed type: parallel/type: matrix steps silently failing in custom commands (only workflows supported them) -- the exact recipe the go-task migration guide recommended for concurrent dependents. - platforms via when: CEL facts (os/arch/platform), native per-command aliases:/internal:, and values: constraint on flags/arguments with an interactive picker. Relocates cmd/custom_command_dependency_adapter.go and cmd/custom_command_values.go into pkg/taskgraph/adapters and pkg/flags respectively, so this logic is unit-testable in isolation instead of coupled to cmd's live command registry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCustom commands and workflows now support dependency graphs, freshness-aware execution, continuation conditions, parallel and matrix steps, platform facts, native aliases, internal visibility, and constrained values. Schemas, runtime execution, tests, fixtures, and documentation were updated. ChangesTask Runner Convergence
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…first-class-support # Conflicts: # cmd/cmd_utils.go # internal/exec/workflow_utils.go # pkg/condition/cel.go # pkg/condition/condition.go # pkg/datafetcher/schema/atmos/manifest/1.0.json # pkg/datafetcher/schema/config/global/1.0.json # pkg/runner/runner.go # pkg/workflow/executor.go # website/src/data/roadmap.js
GitHub Actions
Inactive
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Resource Changes Found for
|
…erflow Windows Acceptance Tests: unquoted backslash paths embedded in shell Command strings get corrupted by mvdan/sh (pkg/utils/shell_utils.go parses commands with bash syntax, which consumes unquoted backslashes as escapes). Apply filepath.ToSlash() to every path used inside a shell Command string across the freshness/dependency/precondition test suites; forward slashes are valid path separators on Windows too. Also give freshness state Save() a uniquely named temp file per write (os.CreateTemp) since pkg/cache.FileLock is a documented no-op on Windows, so a fixed temp filename let concurrent writers collide. CodeQL: pkg/taskgraph.RefsFromDependencies allocated with len(a)+len(b), which go/allocation-size-overflow flags as a potentially overflowing sum; size the capacity hint to a single len() instead.
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…ertions TestCustomCommandIntegration_ParallelStepWithNeeds failed on Windows CI with an exact-match assertion against shell-redirected file content: mvdan/sh's `echo`+`>>` produced "first \r\nsecond \r\n" there instead of "first\nsecond\n". Reproduced locally that the redirect itself correctly isolates fd1 from the live-display writer (no leaked/duplicated output), so this is a shell/OS text formatting difference Atmos doesn't control, not a functional bug. Strengthen the shared splitNonEmptyLines test helper to trim each line (handles \r and trailing whitespace) and switch this test's assertion to use it, matching how sibling dependency tests already tolerate line content.
GitHub Actions
Inactive
…encies and freshness checking Found via field-testing the task-runner dependency/freshness feature; each is fixed with a failing-first regression test: - pkg/taskgraph/adapters/cobra_command.go: same-name dependency dispatches (e.g. the same command depended on twice with different flags) resolve to one shared *cobra.Command and now serialize per-target instead of racing on its mutable flags/context, and reset every non-overridden flag to its declared default before each dispatch instead of silently inheriting a prior dispatch's leftover value. - cmd/cmd_utils.go + cobra_command.go: a step failure inside a dependency's own execution no longer hard-exits the whole process before taskgraph.Run's fail: mode handling (wait_all/fail_fast/best_effort) can see it -- failures now report through a dependency error sink instead. - cmd/cmd_utils.go + internal/exec/workflow_utils.go: a step's freshness-referencing `when:` (timestamp.changed, structured sources/artifacts) no longer gets silently evaluated against an empty pre-check context, which always read false and skipped the whole command/workflow. - internal/exec/workflow_dependency_adapter.go: workflow-depends-on-command subprocess dispatch now resolves its own binary path via os.Executable() instead of the bare "atmos" (PATH lookup), which could silently run an unrelated installed version instead of the active build. examples/task-runner-dependencies/ is a new, durable fixture exercising all of the above end to end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…burst writeLine locked/unlocked the shared writeMu per individual line rather than per flush. When one Write() resolved into multiple lines (e.g. a \r-separated progress update followed later by its completion), releasing the lock between them let a concurrently writing sibling node's entire output interleave in the gap. Write/Flush now hold writeMu across every line one call flushes. Fixes the CI failure in TestExecuteTerraformConcurrentHooksUseNodeWriters (pkg/scheduler/adapters/terraform_test.go), the only real failure in the macOS acceptance-test job's log. Also fixes a numbered-list indentation lint finding in the previous commit's fix doc. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (20)
cmd/list/aliases.go-28-35 (1)
28-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
AliasInfo.Typedocumentation.Line 28 adds
"custom", but the exported field comment at Line 48 lists only"built-in"and"configured". Include"custom"so the public contract matches the emitted values.🤖 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 `@cmd/list/aliases.go` around lines 28 - 35, The AliasInfo.Type documentation currently omits the newly supported "custom" value; update its exported field comment to list "custom" alongside "built-in" and "configured", without changing the type behavior.Source: Coding guidelines
pkg/flags/constrained.go-39-41 (1)
39-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap picker errors with field context.
PromptForValueerrors return unchanged. Wrap each error with the affected argument or flag name and the applicable static error so users can identify the failed prompt.Also applies to: 81-83
🤖 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 `@pkg/flags/constrained.go` around lines 39 - 41, Update the error handling around PromptForValue in the argument and flag prompt paths to wrap returned errors with the affected arg.Name or flag name and the applicable static error context. Preserve the existing early-return behavior while ensuring both occurrences provide field-specific context.Source: Coding guidelines
pkg/flags/constrained.go-46-46 (1)
46-46: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an argument-specific validation error.
ValidateValuereports invalid input asflag --<name>. This call validates a positional argument, so an invalidenvvalue is reported as an invalid flag. Return an argument-specific error that identifies the positional argument and its allowed values.🤖 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 `@pkg/flags/constrained.go` at line 46, Update the validation error handling in the constrained argument path around ValidateValue so positional-argument failures use an argument-specific error instead of the flag-oriented message. Identify the argument by arg.Name and include arg.Values in the error, while preserving the existing successful validation flow.pkg/datafetcher/schema/atmos/config/1.0.json-6138-6170 (1)
6138-6170: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winOne misplaced Go doc comment produced two wrong schema descriptions. The
Inputsdescription opens with"Task represents a unit of work that can be executed. This type unifies workflow steps and custom command steps...", and theTaskdefinition now has no description at all. Both follow from the same cause: inpkg/schema/task.gotheTaskdoc comment sits directly above theInputsdeclaration inside the groupedtype (...)block, so the generator attached it toInputs. Editors reading this schema show unrelated prose forinputs:and nothing for a task.
pkg/datafetcher/schema/atmos/config/1.0.json#L6138-L6170: no direct edit here. Move theTaskdoc comment inpkg/schema/task.goback above theTaskdeclaration, then regenerate so this description contains only theInputsprose starting at "Inputs declares a step's freshness inputs".pkg/datafetcher/schema/atmos/config/1.0.json#L10925-L10925: after the same source fix and regeneration, confirm theTaskdefinition regains its description.🤖 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 `@pkg/datafetcher/schema/atmos/config/1.0.json` around lines 6138 - 6170, Move the Task doc comment in the grouped type declaration in pkg/schema/task.go directly above Task rather than Inputs, then regenerate the schema. At pkg/datafetcher/schema/atmos/config/1.0.json lines 6138-6170, verify Inputs retains only its own description and requires no direct edit; at line 10925, verify the regenerated Task definition has the Task description restored.pkg/datafetcher/schema/atmos/manifest/1.0.json-2473-2475 (1)
2473-2475: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicate
dependencieskey inworkflow_manifest.
workflow_manifest.propertiesalready declaresdependencieswith the same$refat lines 2494-2496. Duplicate object members are not valid JSON hygiene, and most parsers keep only the last occurrence. Drop the new block and keep the existing one.🧹 Proposed fix
"stack": { "type": "string" }, - "dependencies": { - "$ref": "`#/definitions/dependencies`" - }, "steps": {🤖 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 `@pkg/datafetcher/schema/atmos/manifest/1.0.json` around lines 2473 - 2475, Remove the duplicate dependencies property block from workflow_manifest.properties, keeping the existing declaration that references `#/definitions/dependencies` unchanged.Source: Linters/SAST tools
pkg/datafetcher/schema/stacks/stack-config/1.0.json-2137-2139 (1)
2137-2139: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winConstrain
continueto the condition schema.The new property has no type or
$ref. The schema accepts invalid values such ascontinue: 42orcontinue: {unexpected: true}.Reference
#/definitions/conditionso editor validation and manifest validation match the runtime contract.Proposed fix
"continue": { - "description": "Condition that forgives this step's own failure so later steps still run and the overall status is unaffected (GitHub Actions' continue-on-error semantics). Evaluated after the step's own execution, against its own outcome, unlike 'when' which is evaluated before the step runs." + "description": "Condition that forgives this step's own failure so later steps still run and the overall status is unaffected (GitHub Actions' continue-on-error semantics). Evaluated after the step's own execution, against its own outcome, unlike 'when' which is evaluated before the step runs.", + "allOf": [ + { "$ref": "`#/definitions/condition`" } + ] }🤖 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 `@pkg/datafetcher/schema/stacks/stack-config/1.0.json` around lines 2137 - 2139, Update the continue property definition in the stack configuration schema to reference `#/definitions/condition`, replacing the unconstrained definition while preserving its existing description.pkg/schema/dependencies.go-171-179 (1)
171-179: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject non-string YAML scalar dependencies.
yaml.ScalarNodeincludes booleans, numbers, and null values. For example,commands: [true]becomes a dependency named"true"during direct workflow decoding. The Viper path rejects the same value becausedecodeUnitDependencyItemaccepts onlystring.Check
node.Tagbefore assigningnode.Value. Add tests for boolean, numeric, and null entries.Proposed fix
case yaml.ScalarNode: + if node.Tag != "!!str" { + return fmt.Errorf("%w at index %d: got %s scalar (expected string or mapping)", ErrTaskUnexpectedNodeKind, i, node.Tag) + } dep.Name = node.Value🤖 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 `@pkg/schema/dependencies.go` around lines 171 - 179, Update the ScalarNode branch in the dependency decoding logic to verify node.Tag denotes a YAML string before assigning node.Value to dep.Name; return the existing unexpected-node error for booleans, numbers, null, and other non-string scalars. Add coverage for boolean, numeric, and null dependency entries while preserving valid string and mapping decoding.internal/exec/workflow_utils.go-1034-1041 (1)
1034-1041: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAn
artifacts:-only step never skips.
RecordSuccessruns only whenstep.Inputs != nil. For a step that declaresartifacts:withoutinputs:, no record is ever saved.Checker.checksumChangedthen finds no record and returnstrueon every run, so the implicitchecksum.changedcondition always matches.
pkg/runner/freshness/checker.golines 136-139 document that declaringartifacts:alone is enough to skip a step whose work is already done. Please align the two: either record state for artifacts-only steps, or treat "artifacts all exist and no sources declared" as unchanged inchecksumChanged.🤖 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/exec/workflow_utils.go` around lines 1034 - 1041, The freshness state is recorded only when step.Inputs is non-nil, so artifacts-only steps never become skippable. Update the post-success logic around freshnessChecker.RecordSuccess to also persist freshness state for steps declaring artifacts without inputs, or update Checker.checksumChanged to treat existing artifacts with no sources as unchanged, preserving the documented artifacts-only skip behavior.internal/exec/custom_command_control_adapter.go-53-68 (1)
53-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate dry-run into custom command child steps.
ExecuteCustomCommandControlStephard-codesfalseforExecuteShellCommand[5], whileexecuteWorkflowControlStep[2]passescontrol.dryRun. AddDryRuntoCustomCommandControlContextand use it here if custom commands are expected to honor dry-run.🤖 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/exec/custom_command_control_adapter.go` around lines 53 - 68, Update CustomCommandControlContext to include a DryRun field, then pass that value instead of the hard-coded false argument in the RunCommand callback used by ExecuteCustomCommandControlStep. Ensure custom command child steps receive and honor the same dry-run state propagated by executeWorkflowControlStep.pkg/runner/freshness/checker.go-433-448 (1)
433-448: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
baseDiris documented as absolute, but callers can pass a relative path.The doc comment states
baseDiris absolute so state does not cross-contaminate between checkouts.internal/exec/workflow_utils.goline 564-567 falls back to"."whenCalculateWorkingDirectoryreturns an empty string. Two different worktrees then produce the same key. Please resolve the path before hashing, or relax the comment.🛡️ Proposed fix
func (c *Checker) stateKey(scope, stepName, baseDir string, sourcesPatterns []string) string { sorted := make([]string, len(sourcesPatterns)) copy(sorted, sourcesPatterns) sort.Strings(sorted) + if abs, err := filepath.Abs(baseDir); err == nil { + baseDir = abs + } + h := sha256.New()🤖 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 `@pkg/runner/freshness/checker.go` around lines 433 - 448, Update Checker.stateKey to resolve baseDir to an absolute, cleaned path before incorporating it into the hash, preserving the documented per-checkout isolation even when callers provide "." or another relative path. Keep the existing scope, stepName, and sorted sourcesPatterns hashing behavior unchanged; handle path-resolution errors according to the surrounding package’s established conventions.pkg/taskgraph/adapters/cobra_command.go-184-195 (1)
184-195: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck
ctxafter the lock is acquired sofail_fastcancellation stops queued dispatches.Same-name dispatches serialize on
targetLock. A dispatch can wait there while the scheduler cancelsctxbecause offail_fast. When the lock is released, this code still runs the command in full. Add a cancellation check before dispatch.🛡️ Proposed guard
targetLock := locks.lockFor(target) targetLock.Lock() defer targetLock.Unlock() + // The scheduler may have cancelled ctx (fail_fast) while this dispatch waited on the lock. + if err := ctx.Err(); err != nil { + return err + } + return dispatchCustomCommand(ctx, target, &ref)🤖 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 `@pkg/taskgraph/adapters/cobra_command.go` around lines 184 - 195, In the returned dispatch function, check ctx cancellation immediately after targetLock is acquired and before calling dispatchCustomCommand. Return the context cancellation error when ctx is done, while preserving the existing lock/unlock and command lookup behavior.cmd/custom_command_inputs_test.go-186-218 (1)
186-218: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that
gois on PATH before relying on it.The comment on lines 189-191 treats
goas guaranteed present. A prebuilt test binary can run without the Go toolchain on PATH. In that environment the precondition is unmet, the step runs,runLogis created, and line 241 fails with "step must be skipped when the precondition tool is already on PATH" — a message that points at the feature rather than at the environment. Add an explicitexec.LookPathcheck so the misconfiguration fails loudly and legibly.💚 Proposed addition
tmpDir := t.TempDir() atmosConfig.BasePath = tmpDir runLog := filepath.Join(tmpDir, "run.txt") + // This test's whole premise is that the declared tool resolves. Fail loudly, not with a + // misleading "step must be skipped" assertion failure, if the environment lacks it. + _, lookErr := exec.LookPath("go") + require.NoError(t, lookErr, "this test requires the 'go' binary on PATH") +Add
"os/exec"to the imports.As per coding guidelines: "Safety precondition and fixture-count checks must fail loudly with
require.Positiveor an equivalent assertion; do not silently skip on misconfiguration."🤖 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 `@cmd/custom_command_inputs_test.go` around lines 186 - 218, In TestCustomCommandIntegration_PreconditionSkipsWhenToolAlreadyOnPath, import os/exec and explicitly verify that exec.LookPath("go") succeeds before configuring the scenario. Use a require assertion with a clear environment-focused failure message rather than skipping, then retain the existing precondition test flow.Source: Coding guidelines
pkg/taskgraph/taskgraph_test.go-114-128 (1)
114-128: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the specific cycle error.
Line 127 accepts any error. A future regression that makes
Runfail earlier, for example withErrMissingRunnerorErrUnknownDependency, would keep this test green while the cycle check silently stops working.buildGraphdocuments thatdependency.GraphBuilder.Buildreturnsdependency.ErrCircularDependency, so assert it.💚 Proposed fix
require.Error(t, err) + assert.ErrorIs(t, err, dependency.ErrCircularDependency, "a -> b -> a must be reported as a circular dependency") }Add
"github.com/cloudposse/atmos/pkg/dependency"to the imports.As per coding guidelines: "avoid tautological, stub, always-skipped, or coverage-only tests".
🤖 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 `@pkg/taskgraph/taskgraph_test.go` around lines 114 - 128, Update TestRun_CycleErrors to assert that Run returns dependency.ErrCircularDependency, importing the dependency package for the expected sentinel error. Replace the broad require.Error assertion while preserving the existing cyclic dependency setup.Source: Coding guidelines
pkg/taskgraph/adapters/cobra_command.go-231-240 (1)
231-240: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSet workflow dependency execution state from the custom command context.
CustomCommandDependencyOptionsalways passesdryRun=falseandcommandLineIdentity="". Later, custom commands still read the inherited--identityvalue at step execution. If a caller can invoke the custom tree with--dry-runor--identity, workflow dependencies can run in production and lose the caller-selected identity. Thread both values into this constructor and pass them through.🤖 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 `@pkg/taskgraph/adapters/cobra_command.go` around lines 231 - 240, Update CustomCommandDependencyOptions to accept the custom command context’s dry-run and command-line identity values, then pass both through to e.WorkflowRunner instead of hardcoding false and an empty identity. Update all callers to supply the inherited --dry-run and --identity values so workflow dependencies preserve the caller’s execution state.pkg/taskgraph/taskgraph.go-100-116 (1)
100-116: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
best_effortalso swallows configuration errors, not just task failures.Lines 112-115 discard
aggregate.Errentirely. The aggregate can carry non-task errors produced bynewDispatcher, for exampleErrMissingRunner(Line 239) or the "no ref metadata" error (Line 232). Underfail: best_effort, a misconfigured graph then reports success with no signal at all.Consider logging the swallowed aggregate at warn/debug level so operators still see the cause.
♻️ Suggested adjustment
aggregate := scheduler.New(graph, dispatcher, schedOpts...).Run(ctx) if failMode == FailBestEffort { + if aggregate.Err != nil { + log.Debug("dependency failures ignored due to fail: best_effort", "error", aggregate.Err) + } return nil } return aggregate.ErrAdd the
log "github.com/charmbracelet/log"import alias already used across the repo.🤖 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 `@pkg/taskgraph/taskgraph.go` around lines 100 - 116, Update the FailBestEffort branch in the taskgraph execution flow to log aggregate.Err at warn or debug level before returning nil, preserving successful best-effort task handling while surfacing configuration errors such as missing runners or ref metadata. Add the repository’s existing charmbracelet/log import alias and use it for this diagnostic.docs/fixes/2026-08-06-line-prefix-writer-multiline-atomicity.md-19-19 (1)
19-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language identifier to the CI-log fence.
The opening fence at Line 19 has no language identifier. Add
textafter the backticks to satisfy markdownlint MD040.The supplied markdownlint result identifies MD040 at Line 19.
🤖 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 `@docs/fixes/2026-08-06-line-prefix-writer-multiline-atomicity.md` at line 19, Update the opening fenced code block at the indicated location in the markdown document to use the text language identifier, changing the fence from an untyped fence to a text fence while preserving its contents and closing fence.Source: Linters/SAST tools
pkg/hashfile/hashfile_test.go-65-68 (1)
65-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a platform-neutral missing-file path.
"/no/such/file"is a Unix-style absolute path. Build the missing path belowt.TempDir()withfilepath.Join.Suggested fix
- _, err := HashFiles([]string{"/no/such/file"}) + missing := filepath.Join(t.TempDir(), "missing.txt") + _, err := HashFiles([]string{missing})🤖 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 `@pkg/hashfile/hashfile_test.go` around lines 65 - 68, Update TestHashFiles_MissingFileErrors to construct the nonexistent path beneath t.TempDir() using filepath.Join instead of the hard-coded Unix absolute path, while preserving the existing error assertion.Source: Coding guidelines
docs/fixes/2026-08-05-custom-command-dependency-shared-cobra-state.md-1-1 (1)
1-1: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the fix-document summaries with their stated scope.
Both fix notes use broader headlines than their explicit scope statements. Readers may assume that all dependency failures are recoverable and that freshness checks can skip an entire workflow.
docs/fixes/2026-08-05-custom-command-dependency-shared-cobra-state.md#L1-L1: Qualify the “hard-exit” claim to the covered step-failure paths. The document states that earlier flag, working-directory, and identity-resolution failures still hard-exit.docs/fixes/2026-08-05-custom-command-freshness-when-precheck.md#L1-L16: Separate the custom-command whole-run fix from the workflowneedsAuthfix. The document states that the workflow change only controls auth-manager setup.🤖 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 `@docs/fixes/2026-08-05-custom-command-dependency-shared-cobra-state.md` at line 1, Update the summaries in docs/fixes/2026-08-05-custom-command-dependency-shared-cobra-state.md:1 to qualify the “hard-exit” claim as applying only to the covered step-failure paths, while acknowledging that flag, working-directory, and identity-resolution failures still hard-exit. Update docs/fixes/2026-08-05-custom-command-freshness-when-precheck.md:1-16 to distinguish the custom-command whole-run behavior from the workflow needsAuth change, which only controls auth-manager setup.website/docs/workflows/workflows/workflow/steps/continue.mdx-29-37 (1)
29-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe the default failure behavior precisely.
Line 31 says a failed step “stops the workflow.” Later structured steps still evaluate after a failure. This permits
when: failureandwhen: alwayssteps to run. State that an omittedcontinuekeeps the workflow failed and skips success-only steps.🤖 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 `@website/docs/workflows/workflows/workflow/steps/continue.mdx` around lines 29 - 37, Update the “Omitted” entry in the continue behavior documentation to state that a step failure keeps the workflow failed, skips success-only steps, and still allows subsequent steps using when: failure or when: always to evaluate and run.website/docs/workflows/workflows/workflow/steps/continue.mdx-29-40 (1)
29-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the YAML
!celtag in the quoted example.When
!celis inside quotes, YAML passes it as a literal scalar to CEL instead of applying it as a tag. Use the explicit tag for a copy-pasteable CEL example.Proposed documentation fix
- <dd>Any CEL expression that evaluates to a boolean, for finer control — for example, `continue: "!cel env.CI == \'true\'"` to tolerate a failure only in CI.</dd> + <dd>Any CEL expression that evaluates to a boolean, for finer control — for example, `continue: !cel 'env.CI == "true"'` to tolerate a failure only in CI.</dd>🤖 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 `@website/docs/workflows/workflows/workflow/steps/continue.mdx` around lines 29 - 40, Update the CEL expression example in the workflow step documentation to use YAML’s explicit !cel tag rather than placing !cel inside the quoted scalar. Keep the example’s CI-based boolean expression and ensure it remains copy-pasteable YAML.
🧹 Nitpick comments (19)
pkg/taskgraph/adapters/cobra_command_test.go (1)
176-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert what the options wire, not just how many there are.
assert.Len(t, opts, 4, ...)passes even if one option is duplicated and another is dropped, which is exactly the regression the message claims to guard against. Apply the options to ataskgraph.Optionsvalue and assert that the command runner, command lookup, workflow runner, and workflow lookup are each non-nil.As per coding guidelines: "for slice results assert element values rather than only length".
🤖 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 `@pkg/taskgraph/adapters/cobra_command_test.go` around lines 176 - 180, Update TestCustomCommandDependencyOptions_ReturnsAllFourOptions to apply opts to a taskgraph.Options value, then assert that the command runner, command lookup, workflow runner, and workflow lookup fields are each non-nil. Replace the length-only assertion while preserving the test’s coverage of all four dependencies.Source: Coding guidelines
pkg/condition/condition_test.go (1)
300-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
EvaluateEfor the negative assertions.
Evaluateswallows evaluation errors and returnsfalse(seepkg/condition/evaluate.golines 20-23). Soassert.False(t, mismatchOS.Evaluate(ctx))andassert.False(t, notStale.Evaluate(ctx))pass both when the expression correctly evaluates to false and when it fails at runtime.EvaluateEwithrequire.NoErrorseparates the two outcomes.♻️ Proposed change for the negative cases
mismatchOS, err := New("!cel os == 'not-a-real-os'") require.NoError(t, err) - assert.False(t, mismatchOS.Evaluate(ctx)) + got, evalErr := mismatchOS.EvaluateE(ctx) + require.NoError(t, evalErr) + assert.False(t, got)notStale, err := New("!cel sources.exists(s, artifacts.all(a, s.mtime < a.mtime))") require.NoError(t, err) - assert.False(t, notStale.Evaluate(ctx)) + got, evalErr := notStale.EvaluateE(ctx) + require.NoError(t, evalErr) + assert.False(t, got)Also applies to: 337-358
🤖 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 `@pkg/condition/condition_test.go` around lines 300 - 323, Update the negative assertions in TestConditionEvaluate_PlatformFacts and the additional notStale cases to call EvaluateE instead of Evaluate. Require no evaluation error, then assert the returned boolean is false so runtime failures cannot satisfy the negative checks.pkg/schema/dependencies_test.go (1)
14-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
argsandfailin the round-trip assertions.The tests exercise
name,flags, andfile, but notargsorfail. Both fields carry real behavior: per theUnitDependencydoc inpkg/schema/dependencies.go,argsparticipates in the DAG dedup key, andfailselects the failure-propagation mode. Adding them here locks the full decode contract.♻️ Proposed addition
commands: - build - name: test flags: env: dev + args: [--verbose] + fail: fail_fast workflows:assert.Equal(t, map[string]string{"env": "dev"}, deps.Commands[1].Flags) + assert.Equal(t, []string{"--verbose"}, deps.Commands[1].Args) + assert.Equal(t, "fail_fast", deps.Commands[1].Fail)🤖 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 `@pkg/schema/dependencies_test.go` around lines 14 - 38, Extend TestUnitDependencies_UnmarshalYAML to include args and fail values in the YAML input for relevant UnitDependency entries, then assert the decoded Args and Fail fields. Preserve the existing name, flags, file, and collection assertions while covering both fields’ unmarshalling behavior.pkg/datafetcher/schema/atmos/config/1.0.json (1)
12616-12626: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider constraining
failto its three valid values.The description names
wait_all,fail_fast, andbest_effort, but the schema accepts any string. A typo such asfailfastpasses schema validation and only surfaces at runtime.ParallelFailConfig.modeinpkg/datafetcher/schema/atmos/manifest/1.0.jsonalready uses anenumfor the same vocabulary, so this would align the two. The schema is generated, so the change belongs on the Go field's jsonschema tag inpkg/schema/dependencies.go.🤖 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 `@pkg/datafetcher/schema/atmos/config/1.0.json` around lines 12616 - 12626, The fail field schema is too permissive because it accepts arbitrary strings despite documenting three valid values. Update the Go field in dependencies.go that generates this schema, adding an enum constraint for wait_all, fail_fast, and best_effort while preserving its nullable behavior and regenerating the affected schema.examples/task-runner-dependencies/atmos.yaml (2)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGNU-only shell utilities in the cross-platform fixtures. Both fixtures build log lines with
date +%s%N.%Nis a GNU coreutils extension, so macOS prints a literalNand Windows has nodatebinary. The shared root cause is the use of GNU-specific utilities in example fixtures that must run on Linux, macOS, and Windows.
examples/task-runner-dependencies/atmos.yaml#L19-L23: replacedate +%s%Nwith a portable marker, and replacesleep 2in thestep-c-slowcommand with a portable delay.examples/task-runner-dependencies/workflows/task-runner.yaml#L8-L11: replacedate +%s%Nin thesmokestep with the same portable marker.🤖 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 `@examples/task-runner-dependencies/atmos.yaml` around lines 19 - 23, Replace the GNU-specific date expressions with one portable marker in examples/task-runner-dependencies/atmos.yaml lines 19-23 and examples/task-runner-dependencies/workflows/task-runner.yaml lines 8-11, preserving the log format and using the same marker in both fixtures. In atmos.yaml, also replace the step-c-slow command’s sleep 2 with a delay mechanism that works on Linux, macOS, and Windows.
102-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese two steps write into
logs/without creating it.Every other step runs
mkdir -p logsfirst.release-failfastandrelease-besteffortdepend on that side effect from their dependencies. If a dependency is skipped or fails early, the redirect fails.♻️ Suggested fix
steps: - type: shell - command: echo "release-failfast ran" >> logs/order.log + command: | + mkdir -p logs + echo "release-failfast ran" >> logs/order.logAlso applies to: 113-115
🤖 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 `@examples/task-runner-dependencies/atmos.yaml` around lines 102 - 103, Update the shell commands for the release-failfast and release-besteffort steps to create the logs directory with mkdir -p before appending to logs/order.log, so each step works independently without relying on dependency side effects.internal/exec/workflow_utils_test.go (1)
1541-1569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test depends on the
gobinary being onPATH.
go testruns a compiled binary. The toolchain directory is usually onPATH, but that is not guaranteed in every CI image or when the test binary runs standalone. Ifexec.LookPath("go")fails, the step runs and the assertion fails for the wrong reason.Create a temporary executable and prepend its directory to
PATHwitht.Setenv, then declare that name inPrecondition.Tools. That keeps the test self-contained and cross-platform.🤖 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/exec/workflow_utils_test.go` around lines 1541 - 1569, Update TestExecuteWorkflow_PreconditionSkipsWhenToolAlreadyOnPath to create a temporary executable in a temporary directory, prepend that directory to PATH with t.Setenv, and use the executable’s name in Precondition.Tools instead of relying on “go”. Ensure the fixture is executable across supported platforms so the precondition is deterministically satisfied and the workflow step remains skipped.pkg/runner/freshness/checker_test.go (1)
499-507: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the matched path, not only the slice length.
The test proves one match exists but not that it is the created file.
💚 Proposed assertion
matches, err := g.Glob(tmpDir, "*.go") require.NoError(t, err) - assert.Len(t, matches, 1) + require.Len(t, matches, 1) + assert.Equal(t, filepath.Join(tmpDir, "main.go"), matches[0])As per coding guidelines: "for slice results assert element values rather than only length".
🤖 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 `@pkg/runner/freshness/checker_test.go` around lines 499 - 507, Update TestDefaultGlobber_ResolvesRealFiles to assert that the matched path equals the expected path for the created main.go file, while retaining the existing error check and match-count assertion.Source: Coding guidelines
pkg/runner/freshness/checker.go (1)
228-250: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winArtifact matches are not deduplicated, unlike source matches.
globAlldeduplicates source matches. The artifact branch appends raw matches per pattern, so two overlappingartifacts.pathspatterns produce duplicate entries. Duplicates then reachbuildFileFacts, so theartifactsCEL list contains the same file twice, and each duplicate is hashed again.♻️ Suggested dedup for artifact matches
if needs.artifactGlob() && len(artifactPatterns) > 0 { + seen := make(map[string]struct{}) for _, pattern := range artifactPatterns { matches, globErr := c.globber.Glob(baseDir, pattern) if globErr != nil { return globResult{}, globErr } if len(matches) == 0 { result.artifactsAllExist = false } - result.artifactMatches = append(result.artifactMatches, matches...) + for _, m := range matches { + if _, ok := seen[m]; ok { + continue + } + seen[m] = struct{}{} + result.artifactMatches = append(result.artifactMatches, m) + } } }The per-pattern
artifactsAllExistcheck must stay inside the loop, as shown.🤖 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 `@pkg/runner/freshness/checker.go` around lines 228 - 250, Update Checker.globSourcesAndArtifacts to deduplicate artifactMatches across overlapping artifactPatterns, matching globAll’s source-match behavior. Preserve the per-pattern artifactsAllExist check inside the loop, and ensure each artifact is appended only once before results reach buildFileFacts.pkg/runner/freshness/errors.go (1)
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
ErrGlobInvalidsentinel.
ErrGlobInvalidis only declared and documentation saysdefaultGlobber.Globreturns errors fromfilesystem.GetGlobMatches. Wire the sentinel into the glob path or remove this unused error to keep the exported surface clean.🤖 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 `@pkg/runner/freshness/errors.go` around lines 9 - 11, Remove the unused exported ErrGlobInvalid sentinel and its associated comment from the freshness errors definitions, since defaultGlobber.Glob continues to propagate filesystem.GetGlobMatches errors directly.cmd/custom_command_dependency_test.go (1)
362-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCapture the exit code, not only the fact of exiting.
The stub at line 366 discards
code. Recording it lets the test pin the propagated failure status, so a regression that exits with 0 on a failed dependency is caught.💚 Proposed addition
var mu sync.Mutex exited := false + exitCode := 0 originalOsExit := errUtils.OsExit t.Cleanup(func() { errUtils.OsExit = originalOsExit }) errUtils.OsExit = func(code int) { mu.Lock() exited = true + exitCode = code mu.Unlock() } @@ assert.True(t, exited, "the default (wait_all) fail mode must still surface a dependency's failure via errUtils.OsExit") + assert.NotZero(t, exitCode, "a failed dependency must produce a non-zero exit code")🤖 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 `@cmd/custom_command_dependency_test.go` around lines 362 - 378, Update the errUtils.OsExit stub in the parentCmd.Run test to capture the provided code in a protected variable, then assert that the propagated exit status matches the expected failure code in addition to asserting exited is true. Keep the existing synchronization and ownStepLog assertion unchanged.internal/exec/workflow.go (1)
206-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the manifest path to the read and parse errors.
Lines 208 and 213 return the raw
os.ReadFileand YAML errors.LoadWorkflowConfignow serves three call sites:ExecuteWorkflowCmd,WorkflowLookup, andWorkflowRunner. A bareyaml: line 7: did not find expected keyno longer tells the user which manifest failed, and dependency resolution can load several manifests in one run.♻️ Proposed change
fileContent, err := os.ReadFile(workflowPath) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to read workflow manifest %q: %w", filepath.ToSlash(workflowPath), err) } workflowManifest, err := u.UnmarshalYAML[schema.WorkflowManifest](string(fileContent)) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse workflow manifest %q: %w", filepath.ToSlash(workflowPath), err) }As per coding guidelines: "Provide clear error messages to users, include troubleshooting hints when appropriate" and wrap errors "with context using
fmt.Errorf(\"context: %w\", err)".🤖 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/exec/workflow.go` around lines 206 - 214, Update LoadWorkflowConfig around os.ReadFile and UnmarshalYAML to wrap both errors with workflowPath context using fmt.Errorf and %w, preserving the original errors for unwrapping while identifying which manifest failed.Source: Coding guidelines
pkg/taskgraph/taskgraph.go (1)
229-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReusing
ErrUnknownDependencyKindfor missing node metadata is misleading.Line 232 reports an internal invariant violation ("no ref metadata") with the sentinel that means "unsupported Kind". A caller that uses
errors.Is(err, ErrUnknownDependencyKind)cannot tell the two cases apart. Add a dedicated sentinel inpkg/taskgraph/errors.go.♻️ Suggested change
- return scheduler.Result{}, fmt.Errorf("%w: node %q has no ref metadata", ErrUnknownDependencyKind, node.ID) + return scheduler.Result{}, fmt.Errorf("%w: node %q", ErrMissingRefMetadata, node.ID)Add to
pkg/taskgraph/errors.go:// ErrMissingRefMetadata is returned when a graph node lacks its "ref" metadata entry, which // indicates an internal graph-construction bug rather than a user configuration error. var ErrMissingRefMetadata = errors.New("graph node has no ref metadata")🤖 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 `@pkg/taskgraph/taskgraph.go` around lines 229 - 233, Add the dedicated ErrMissingRefMetadata sentinel in errors.go and update the metadata validation in the DispatcherFunc callback to wrap it instead of ErrUnknownDependencyKind when node.Metadata lacks a valid "ref". Preserve the existing node ID context and keep ErrUnknownDependencyKind for unsupported dependency kinds.internal/exec/workflow_dependency_adapter.go (1)
109-122: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
ctxis accepted but never reaches the subprocess.
ExecuteShellCommandtakes nocontext.Context, so afail_fastcancellation or a Ctrl-C leaves this dependency subprocess running until it exits on its own. The parent graph waits on it. Threading a context throughExecuteShellCommandis a cross-cutting change and belongs in its own PR, so record the gap here and track it.I can open an issue for a context-aware
ExecuteShellCommandvariant if that helps.🤖 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/exec/workflow_dependency_adapter.go` around lines 109 - 122, Document the missing context propagation in commandRunnerViaSubprocess, noting that ExecuteShellCommand cannot currently receive ctx and that subprocess cancellation remains unhandled. Add a TODO or issue-tracking reference at the call site without attempting to modify ExecuteShellCommand or introduce broader context changes.pkg/taskgraph/taskgraph_test.go (1)
130-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
fail_fastand forWithMaxConcurrency.The tests cover
wait_all(Line 143) andbest_effort(Line 159). TheFailFastbranch ofeffectiveFailModeand thescheduler.WithFailFastwiring inRunhave no test, andWithMaxConcurrencyis never exercised. Afail_fastcase with two independent dependencies, one failing, would pin both the mode derivation and the sibling-cancellation behavior.🤖 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 `@pkg/taskgraph/taskgraph_test.go` around lines 130 - 160, Extend the taskgraph tests with a fail_fast case containing two independent dependencies, making one fail and asserting the sibling is cancelled or not completed, to cover effectiveFailMode and Run’s scheduler.WithFailFast wiring. Add a separate test that invokes Run with WithMaxConcurrency and verifies execution is bounded by the configured concurrency limit.cmd/custom_command_control_test.go (1)
146-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
errUtils.OsExitis not reached.The doc comment on lines 89-91 states that the command "does not exit with an error", but nothing here checks that.
customCmd.Runreturns nothing, so a hard exit througherrUtils.OsExitwould leave these three assertions passing.cmd/custom_command_dependency_test.goalready establishes the mutex-guarded override pattern for exactly this check; reuse it.💚 Proposed addition
+ var mu sync.Mutex + exited := false + originalOsExit := errUtils.OsExit + t.Cleanup(func() { errUtils.OsExit = originalOsExit }) + errUtils.OsExit = func(int) { + mu.Lock() + exited = true + mu.Unlock() + } + customCmd.Run(customCmd, []string{}) + mu.Lock() + defer mu.Unlock() + assert.False(t, exited, "continue: always must not exit the process") assert.FileExists(t, failFile)Add the
syncanderrUtils "github.com/cloudposse/atmos/errors"imports.🤖 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 `@cmd/custom_command_control_test.go` around lines 146 - 151, Update the test around customCmd.Run to use the mutex-guarded errUtils.OsExit override pattern established in custom_command_dependency_test.go, adding the required sync and errUtils imports. Capture whether OsExit is invoked, restore the override safely, and assert it was not reached while preserving the existing file assertions.pkg/taskgraph/adapters/cobra_command.go (1)
65-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the error sink type or hide
WithDependencyErrorSink.
WithDependencyErrorSinkis exported, but it returns the unexported*errorSink, so external packages cannot name the value in variable declarations, fields, or signatures. Rename it toErrorSinkwith an exportedErr()accessor, or make the constructor unexported if this API stays package-local.🤖 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 `@pkg/taskgraph/adapters/cobra_command.go` around lines 65 - 78, Resolve the exported API mismatch around WithDependencyErrorSink: either export errorSink as ErrorSink and add an exported Err() accessor for reading the recorded error, or make WithDependencyErrorSink unexported if it is strictly package-local. Update all references consistently while preserving the sink’s existing behavior.Source: Coding guidelines
pkg/hashfile/hashfile.go (1)
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the repository error-wrapping policy.
HashFilesreturns file and digest-write errors directly at Line 30, Line 33, and Line 36. Add operation and path context with%w, and route failures through the static errors defined inerrors/errors.go.As per coding guidelines, wrap all errors with static errors from
errors/errors.goand use%wfor string context.🤖 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 `@pkg/hashfile/hashfile.go` around lines 28 - 36, Update HashFiles to wrap each os.ReadFile and hash.Write failure with the appropriate static error from errors/errors.go, adding operation and path context via %w. Preserve the existing immediate returns while ensuring errors at all three failure points identify the relevant file path and operation.Source: Coding guidelines
website/blog/2026-08-05-taskfile-convergence.mdx (1)
153-158: 📐 Maintainability & Code Quality | 🔵 TrivialVerify the documentation routes and build the website.
The blog links to
/cli/configuration/commands/dependencies, while the supplied repository path iswebsite/docs/cli/configuration/commands/command/dependencies.mdx. Confirm that the page frontmatter publishes the flattened route. Then runcd website && npm run buildto validate the new MDX and links.Based on learnings, Docusaurus routes in this repository must use explicit frontmatter
idorslugvalues, not inferred file paths. As per coding guidelines, documentation changes require website build and link/rendering verification.🤖 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 `@website/blog/2026-08-05-taskfile-convergence.mdx` around lines 153 - 158, Verify the frontmatter in the dependencies documentation page resolves to the linked flattened route /cli/configuration/commands/dependencies, adding or correcting its explicit id or slug as needed. Then run the website build with cd website && npm run build to validate the MDX and links.Sources: Coding guidelines, Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 457c5637-575b-43e0-85f8-3e99da465109
📒 Files selected for processing (74)
cmd/cmd_utils.gocmd/custom_command_aliases_test.gocmd/custom_command_control_test.gocmd/custom_command_dependency_test.gocmd/custom_command_inputs_test.gocmd/custom_command_values_test.gocmd/list/aliases.gocmd/list/aliases_test.godocs/fixes/2026-08-05-custom-command-dependency-shared-cobra-state.mddocs/fixes/2026-08-05-custom-command-freshness-when-precheck.mddocs/fixes/2026-08-05-workflow-command-dependency-wrong-atmos-binary.mddocs/fixes/2026-08-06-line-prefix-writer-multiline-atomicity.mderrors/errors.goexamples/task-runner-dependencies/atmos.yamlexamples/task-runner-dependencies/src/example.txtexamples/task-runner-dependencies/workflows/task-runner.yamlinternal/exec/custom_command_control_adapter.gointernal/exec/workflow.gointernal/exec/workflow_dependency_adapter.gointernal/exec/workflow_dependency_adapter_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gopkg/condition/cel.gopkg/condition/condition.gopkg/condition/condition_test.gopkg/condition/evaluate.gopkg/config/load.gopkg/datafetcher/schema/atmos/config/1.0.jsonpkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/flags/constrained.gopkg/flags/constrained_test.gopkg/flags/standard.gopkg/flags/standard_test.gopkg/hashfile/hashfile.gopkg/hashfile/hashfile_test.gopkg/io/line_prefix_writer.gopkg/process/exec_replace_windows.gopkg/process/shell_command_unix.gopkg/process/shell_command_windows.gopkg/process/shell_session.gopkg/runner/freshness/checker.gopkg/runner/freshness/checker_test.gopkg/runner/freshness/errors.gopkg/runner/freshness/globber.gopkg/runner/freshness/state.gopkg/schema/command.gopkg/schema/dependencies.gopkg/schema/dependencies_test.gopkg/schema/task.gopkg/schema/task_test.gopkg/schema/workflow.gopkg/taskgraph/adapters/cobra_command.gopkg/taskgraph/adapters/cobra_command_test.gopkg/taskgraph/errors.gopkg/taskgraph/ref.gopkg/taskgraph/schema.gopkg/taskgraph/taskgraph.gopkg/taskgraph/taskgraph_test.gopkg/workflow/condition_context.gowebsite/blog/2026-08-05-taskfile-convergence.mdxwebsite/docs/cli/configuration/aliases.mdxwebsite/docs/cli/configuration/commands/command/arguments.mdxwebsite/docs/cli/configuration/commands/command/dependencies.mdxwebsite/docs/cli/configuration/commands/command/flags.mdxwebsite/docs/cli/configuration/commands/command/index.mdxwebsite/docs/cli/configuration/commands/command/steps.mdxwebsite/docs/workflows/workflows/workflow/dependencies.mdxwebsite/docs/workflows/workflows/workflow/steps/artifacts.mdxwebsite/docs/workflows/workflows/workflow/steps/continue.mdxwebsite/docs/workflows/workflows/workflow/steps/index.mdxwebsite/docs/workflows/workflows/workflow/steps/inputs.mdxwebsite/docs/workflows/workflows/workflow/steps/precondition.mdxwebsite/src/data/roadmap.js
- cmd/cmd_utils.go: propagate cmd.Context() to taskgraph.Run so Cobra cancellation reaches dependency execution instead of using context.Background(); generalize the dependency-error-sink helper and route every error path in executeCustomCommand (~28 sites: argument processing, dependency/tool resolution, working-directory resolution, validation, component_config, ENV var resolution, per-step auth) through it, not just step-execution failures. - internal/exec/workflow_utils.go + workflow_dependency_adapter.go: fix workflow-depends-on-workflow redundantly re-resolving and re-running its own dependency graph (a diamond dependency shared by two parents ran 3x instead of once) by adding a dependencies-resolved marker for nested ExecuteWorkflow calls, mirroring the existing command-side mechanism. - pkg/hashfile/hashfile.go: fix two real hash collisions (path/content concatenation ambiguity, and losing directory identity by hashing only the basename) with length-prefixed records and full-path hashing; stream file reads via os.Open + io.Copy instead of loading whole files into memory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…first-class-support
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub Actions
Inactive
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/workflow_utils.go (1)
531-1059: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit
ExecuteWorkflowinto named execution phases.
ExecuteWorkflownow extends past line 1070. It combines dependency scheduling, authentication discovery, freshness evaluation, condition evaluation, step execution, and error aggregation. Extract these phases into helpers and keepExecuteWorkflowas a flat orchestration pipeline.As per coding guidelines, “Keep files under 600 lines” and refactor functions that exceed the stated size and complexity limits.
🤖 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/exec/workflow_utils.go` around lines 531 - 1059, Split ExecuteWorkflow into focused named helpers for dependency resolution, environment/authentication setup, freshness and condition evaluation, step execution, and error aggregation, leaving ExecuteWorkflow as a short linear orchestration pipeline. Preserve existing ordering, shared state, and error semantics while moving the corresponding logic into helpers, including the step loop currently containing executeStep and workflowErr handling. Ensure the resulting functions comply with the file and complexity limits.Source: Coding guidelines
🟡 Minor comments (12)
cmd/cmd_utils.go-964-1000 (1)
964-1000: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFreshness state key ignores flag and argument values.
freshnessScopeis"command:" + commandConfig.NameandStepNameis the step name or index. Two invocations of the same command with different flags (the parameterized-dependency case exercised incmd/custom_command_dependency_test.go) map to the same state key. If such a step declaresinputs:, the first invocation records state and the second one skips, even though it produces a different artifact.Consider including the resolved flag/argument values in the scope, or document that
inputs:freshness is per-command, not per-parameterization.🤖 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 `@cmd/cmd_utils.go` around lines 964 - 1000, Update the freshness identity built in the custom-command step loop around freshnessScope and freshness.StepIdentity so resolved flag and argument values distinguish parameterized invocations of the same command. Incorporate the resolved invocation values into the scope or another identity component while preserving stable keys for identical invocations, ensuring inputs/artifacts freshness does not incorrectly reuse state across different parameterizations.pkg/taskgraph/taskgraph.go-229-233 (1)
229-233: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWrong sentinel for the missing-metadata case.
Line 232 wraps
ErrUnknownDependencyKindfor a node that has norefmetadata. That is an internal invariant violation, not an unknown kind. Any caller doingerrors.Is(err, ErrUnknownDependencyKind)to report a badkind:in config would misattribute this.Add a dedicated sentinel in
pkg/taskgraph/errors.go, for exampleErrMissingRefMetadata, and wrap that instead.🤖 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 `@pkg/taskgraph/taskgraph.go` around lines 229 - 233, Add a dedicated ErrMissingRefMetadata sentinel in errors.go, then update the missing-ref branch in the scheduler.DispatcherFunc callback to wrap it instead of ErrUnknownDependencyKind. Preserve the existing node ID context and leave ErrUnknownDependencyKind reserved for unknown dependency kinds.pkg/datafetcher/schema/atmos/config/1.0.json-6168-6169 (1)
6168-6169: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the doc comment assignment for
TaskandInputs.The comment above
Inputscurrently saysTaskrepresents a unit of work and describes howInputsreplacessources:/generates:/status:. Move theTaskdescription onto theTasktype declaration and attach theInputsdescription toInputs, then regenerate the schema.🤖 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 `@pkg/datafetcher/schema/atmos/config/1.0.json` around lines 6168 - 6169, The schema documentation is attached to Inputs instead of describing the correct declarations. Move the Task unit-of-work description to the Task type declaration, give Inputs its own description covering freshness inputs and the replacement of sources:/generates:/status:, then regenerate the schema.pkg/flags/constrained.go-86-87 (1)
86-87: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap the persistent-flag error with a static error.
Line 87 creates a dynamic error without a repository sentinel. Add an appropriate static error from
errors/errors.goand preservesetErras the cause. This keepserrors.Ischecks and error classification consistent.As per coding guidelines, “Wrap all errors with static errors from
errors/errors.go.”🤖 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 `@pkg/flags/constrained.go` around lines 86 - 87, Update the error handling around PersistentFlags().Set in the prompting flow to wrap a suitable sentinel from errors/errors.go while retaining setErr as the underlying cause. Replace the dynamic-only fmt.Errorf classification with the repository’s static error and preserve the flag name in the contextual message.Source: Coding guidelines
pkg/flags/standard.go-805-823 (1)
805-823: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not label positional arguments as flags.
pkg/flags/constrained.gocallsValidateValueforCommandArgument.Values. Line 821 then reports an invalid positional argument asfor flag --<name>. Keep the membership check shared, but let callers select argument or flag wording.🤖 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 `@pkg/flags/standard.go` around lines 805 - 823, The shared ValidateValue membership check must support caller-selected wording so positional arguments are not reported as flags. Update ValidateValue and its callers, including constrained.go’s CommandArgument.Values path, to distinguish argument versus flag context while preserving the existing invalid-value details and shared validation behavior.docs/fixes/2026-08-06-line-prefix-writer-multiline-atomicity.md-19-23 (1)
19-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced log block.
Markdownlint reports MD040 for this block. Use
textorconsoleafter the opening fence.🤖 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 `@docs/fixes/2026-08-06-line-prefix-writer-multiline-atomicity.md` around lines 19 - 23, Add a language identifier, such as text or console, to the opening fenced code block containing the failing test output in the documentation. Keep the log contents unchanged.Source: Linters/SAST tools
pkg/hashfile/hashfile_test.go-65-68 (1)
65-68: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a portable missing-file path.
/no/such/fileis Unix-specific. Build a missing path undert.TempDir()withfilepath.Join()so this test has the same contract on Windows.🤖 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 `@pkg/hashfile/hashfile_test.go` around lines 65 - 68, Update TestHashFiles_MissingFileErrors to create a nonexistent path beneath t.TempDir() using filepath.Join instead of the Unix-specific literal, while preserving the existing error assertion.Source: Coding guidelines
pkg/hashfile/hashfile.go-32-36 (1)
32-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrap file-operation failures in the project error contract.
These paths return raw
os.Open,Stat, andio.Copyerrors. Add or use a static error fromerrors/errors.go, then wrap each failure with the path and operation context. This keeps freshness failures classifiable witherrors.Is().Also applies to: 45-62
🤖 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 `@pkg/hashfile/hashfile.go` around lines 32 - 36, Update the file-operation error handling used by writeRecord and hashFileContent, including the os.Open, Stat, and io.Copy failure paths, to use a static project error from errors/errors.go. Wrap each returned error with the relevant path and operation context while preserving the static error for errors.Is classification.Source: Coding guidelines
internal/exec/workflow_dependency_adapter_test.go-65-66 (1)
65-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert dependency ordering, not only execution.
Both tests pass if the dependent workflow runs before its dependency. Record both events in one log and assert that the dependency entry precedes the dependent entry.
internal/exec/workflow_dependency_adapter_test.go#L65-L66: assert thatbuildprecedesdeployfor same-file resolution.internal/exec/workflow_dependency_adapter_test.go#L119-L120: assert thatbuildprecedesdeployfor cross-file resolution.As per coding guidelines, tests must be behavior-focused and comprehensive for new features.
🤖 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/exec/workflow_dependency_adapter_test.go` around lines 65 - 66, Update the same-file dependency test at internal/exec/workflow_dependency_adapter_test.go:65-66 and the cross-file dependency test at internal/exec/workflow_dependency_adapter_test.go:119-120 to record build and deploy events in a shared log, then assert that the build entry precedes the deploy entry. Retain the existing assertions that both workflows execute.Source: Coding guidelines
internal/exec/workflow_utils_test.go-1537-1568 (1)
1537-1568: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the current test executable for the satisfied precondition.
go testdoes not guarantee thatgois available on PATH when the compiled test binary runs. Useos.Executable()as theprecondition.toolsvalue. This keeps the test independent of the host toolchain PATH.As per coding guidelines, tests must avoid platform-specific binaries and use Go-native helpers such as
os.Executable().🤖 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/exec/workflow_utils_test.go` around lines 1537 - 1568, Update TestExecuteWorkflow_PreconditionSkipsWhenToolAlreadyOnPath to obtain the current test executable with os.Executable(), assert that lookup succeeds, and use its returned path as the Precondition.Tools value instead of the "go" binary. Preserve the existing skipped-step assertion and workflow setup.Source: Coding guidelines
internal/exec/workflow.go-206-213 (1)
206-213: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWrap new helper failures with static Atmos errors.
The new helpers return errors without the required static Atmos error classification. Preserve the underlying cause with
%w, but add the appropriate sentinel so callers can useerrors.Is().
internal/exec/workflow.go#L206-L213: classify workflow file read and YAML parse failures with a static workflow-file or invalid-manifest error.internal/exec/workflow_dependency_adapter.go#L99-L103: classify executable-path resolution failure with a static execution error before adding string context.As per coding guidelines, “Wrap all errors with static errors from errors/errors.go” and use
%wfor string context.🤖 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/exec/workflow.go` around lines 206 - 213, In internal/exec/workflow.go:206-213, wrap workflow file read failures with the static workflow-file error and YAML parsing failures with the static invalid-manifest error, preserving each underlying cause with %w. In internal/exec/workflow_dependency_adapter.go:99-103, wrap executable-path resolution failures with the static execution error before adding string context, also using %w so errors.Is() can classify them.Source: Coding guidelines
website/docs/workflows/workflows/workflow/steps/inputs.mdx-77-77 (1)
77-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the CI cache instruction with the schema field.
ci.cache.includesis the documented schema field in the freshness state comment; keep.atmos/cache/freshnessunder that same field in the workflow inputs docs so users do not get zero CI cache hits.🤖 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 `@website/docs/workflows/workflows/workflow/steps/inputs.mdx` at line 77, Update the CI cache guidance in the workflow inputs documentation to place `.atmos/cache/freshness` under the documented `ci.cache.includes` field, replacing the mismatched `ci.cache.paths` reference while preserving the project-relative cache location and persistence behavior.
🧹 Nitpick comments (10)
cmd/custom_command_dependency_test.go (1)
54-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffShell
echo/>>marker writes remain in two test files after the Go-native helper landed.cmd/custom_command_values_test.goalready usescustomCommandWriteHelperCommand(t, path, content)in this same package, but these two files still drive step execution through shell built-ins and redirection, which is why they carryfilepath.ToSlashworkarounds and\r-trimming logic.
cmd/custom_command_dependency_test.go#L54-L69: replace theecho ... >> buildLogArg/releaseLogArgstep commands with the helper. These tests need append semantics across several dependency invocations, so add an append mode to the helper or give each invocation its own marker file and count files instead of lines.cmd/custom_command_inputs_test.go#L55-L57: replace"echo ran >> " + filepath.ToSlash(runLog)with the helper. The same substitution applies at lines 158, 217, and 271 in that file. The run-count assertions need append or per-run files as above.Once both files are migrated,
splitNonEmptyLinesno longer needs its cross-platform trimming rationale.As per coding guidelines: "Never use platform-specific binaries or shell commands in tests; use Go-native helpers,
os.Executable(), dependency injection, andfilepath/os/ioAPIs instead."🤖 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 `@cmd/custom_command_dependency_test.go` around lines 54 - 69, Replace shell-based echo/redirection commands in cmd/custom_command_dependency_test.go lines 54-69 and cmd/custom_command_inputs_test.go lines 55-57, 158, 217, and 271 with customCommandWriteHelperCommand. Preserve repeated-invocation counts by adding append support to the helper or using separate marker files per invocation, then update assertions accordingly; remove splitNonEmptyLines’ platform-specific trimming rationale once migration is complete.Source: Coding guidelines
pkg/process/shell_command_unix.go (1)
12-17: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueExport is fine; the SAST
sh -chit is pre-existing and in-scope by design.
NewShellCommandintentionally runs a shell for user-authored step commands, so the command-injection rule fires on every call. The trust boundary has not moved: the command string comes from the sameatmos.yamlthe CLI already executes.The one thing the export changes is reach. Any package can now build a
sh -cinvocation throughpkg/process. A short doc note stating that callers must only pass config-authored command strings would keep the contract explicit for future callers.🤖 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 `@pkg/process/shell_command_unix.go` around lines 12 - 17, Add a concise Go doc note to NewShellCommand stating that callers must pass only command strings authored in trusted configuration, such as atmos.yaml. Keep the existing shell invocation and function behavior unchanged.Source: Linters/SAST tools
cmd/cmd_utils.go (1)
1041-1047: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConstrained-value validation and prompting repeat for every step.
flagsDatais rebuilt fromcmd.Flag(...)on each loop iteration, andpromptForSemanticValues/ValidateConstrainedFieldsonly write into the local maps. For a command with several runnable steps and a missing requiredvalues:flag, the picker appears once per step.Hoisting argument/flag data construction and validation above the step loop would validate once and prompt once. The per-step template data can still reference the same maps.
🤖 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 `@cmd/cmd_utils.go` around lines 1041 - 1047, The command setup currently rebuilds flagsData and validates constrained fields inside the per-step loop, causing required interactive prompts to repeat. Move construction of argumentsData/flagsData and the promptForSemanticValues/ValidateConstrainedFields flow above the runnable-step loop, then reuse the populated maps when building each step’s template data.pkg/flags/constrained_test.go (1)
21-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven test for validation scenarios.
These tests exercise variants of
ValidateConstrainedFields. Consolidate the non-interactive valid, invalid, optional, and type cases into a behavior-focused table. Keep focused tests for interactive prompting and persistent-flag updates.As per coding guidelines, “Use table-driven tests for testing multiple scenarios in Go.”
🤖 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 `@pkg/flags/constrained_test.go` around lines 21 - 131, Consolidate the non-interactive ValidateConstrainedFields scenarios into one behavior-focused table-driven test, covering valid and invalid arguments/flags, missing optional or required fields, wrong flag types, and combined argument/flag validation. Keep separate focused tests for interactive prompting and persistent-flag updates, and use each table case to configure inputs and assert the expected error outcome.Source: Coding guidelines
pkg/runner/freshness/state.go (2)
98-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp files survive a killed process.
If Atmos is killed between
os.CreateTempat Line 98 andos.Renameat Line 113, a<key>.<random>.tmpfile stays in the freshness state directory forever. The directory is project-relative and users are told to add it to their CI cache, so the strays get cached and shipped around.Low priority. A
defer os.Remove(tmp)after a successful rename is a no-op, so the cheap version is a sweep of stale*.tmpfiles onSave, or documenting that the directory is disposable.🤖 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 `@pkg/runner/freshness/state.go` around lines 98 - 116, Update the Save flow around the temporary file created by os.CreateTemp to clean up stale *.tmp files in the freshness state directory before or during each save. Ensure cleanup covers files left by killed processes while preserving the existing atomic write-and-rename behavior and current error handling.
121-123: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate
keybefore joining it into a path.
StateStoreandNewStateStoreare exported, sokeyis part of the public contract rather than an internal detail.recordPathjoins it directly. Today the only producer isChecker.stateKey, which returns a 64-character hex digest, so nothing is broken. A future caller that passes a raw step name gets two surprises: a key containing..escapesstateDir, and a key containing a path separator makesos.CreateTempat Line 98 fail outright.A one-line guard documents the contract and removes both:
🛡️ Proposed fix
+// recordPath maps a state key to its JSON file. The key must be a single path element with no +// separators; Checker.stateKey satisfies this by returning a hex digest. func recordPath(stateDir, key string) string { - return filepath.Join(stateDir, key+".json") + return filepath.Join(stateDir, filepath.Base(key)+".json") }An explicit rejection in
Load/Savewould be stricter, if you prefer failing loudly over normalizing.Based on learnings:
filepath.Joindoes not discard a prefix when given an absolute or traversing component, so a relative-subpath contract must be enforced explicitly rather than assumed.🤖 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 `@pkg/runner/freshness/state.go` around lines 121 - 123, Validate key in recordPath before joining it with stateDir, rejecting absolute paths, path separators, and traversal components so the result remains within stateDir and remains safe for temporary-file creation. Preserve the existing stateKey digest behavior and use the established error-handling contract for invalid keys.Source: Learnings
pkg/runner/freshness/checker.go (1)
360-374: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
buildFileFactshashes one file per call.
c.hasher([]string{p})runs once per matched path. For asources: ["**/*.go"]pattern in a large repository, that is one full read plus one hash setup per file. This path only runs whenwhen:references the baresources/artifactsidentifiers, so the blast radius is limited, and the lazy gating already keeps it off the common path.No change needed now. Consider a batched hasher signature if per-file records become a common pattern.
🤖 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 `@pkg/runner/freshness/checker.go` around lines 360 - 374, No code change is required for buildFileFacts; retain the current per-file c.hasher invocation and lazy gating. Consider batching paths in a future change only if per-file fact generation becomes common.pkg/runner/freshness/checker_test.go (2)
499-570: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the globber and state-store tests into co-located files.
This file is 609 lines, which crosses the 600-line limit. It also holds tests for three units:
Checker,defaultGlobber/globAll(Lines 499-529), andfileStateStore(Lines 531-540, plus 246-294). Moving the globber tests toglobber_test.goand the state-store tests tostate_test.gofixes both the length and the co-location rule in one step.As per coding guidelines: "Keep files under 600 lines, use one command implementation per file, co-locate tests, and never disable the file-length linter."
🤖 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 `@pkg/runner/freshness/checker_test.go` around lines 499 - 570, Move the defaultGlobber and globAll tests, including TestDefaultGlobber_ResolvesRealFiles, TestDefaultGlobber_MissingBaseDirIsNotAnError, TestGlobAll_PropagatesError, and TestGlobAll_DeduplicatesAcrossPatterns, into globber_test.go. Move all fileStateStore tests, including TestFileStateStore_SaveThenLoadRoundTrips and the related tests currently elsewhere in this file, into state_test.go; leave Checker tests in checker_test.go and do not disable the file-length linter.Source: Coding guidelines
146-165: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the
artifacts:-only path.
TestChecker_ArtifactsMissingForcesRerunEvenIfSourcesUnchangedcovers artifacts plus inputs. No test covers a step that declaresartifacts:and noinputs:, which is the documented standalone usage. That gap hides the rerun-forever behavior flagged inpkg/runner/freshness/checker.go.Add a case that declares artifacts only, calls
RecordSuccess, then assertsChecksumChangedis false on the secondCompute.🤖 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 `@pkg/runner/freshness/checker_test.go` around lines 146 - 165, Add a test alongside TestChecker_ArtifactsMissingForcesRerunEvenIfSourcesUnchanged for an artifacts-only step with no inputs. Configure artifacts, call RecordSuccess, run the second Compute, and assert facts.ChecksumChanged is false to verify the documented standalone path does not rerun forever.pkg/runner/freshness/globber.go (1)
32-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ErrGlobInvalidis declared but never returned. One root cause: the only failure path that should produce it returns the underlying error unwrapped, so the static error has no users and glob failures reach the caller without the pattern or base directory.
pkg/runner/freshness/globber.go#L32-L49: replacereturn nil, errat Line 46 with a wrap that carriesErrGlobInvalid, the pattern, andbaseDirusing%w.pkg/runner/freshness/errors.go#L9-L11: keep the declaration once it has a caller, and confirm whether it belongs here or in the centralerrors/errors.gothat this PR also modifies.As per coding guidelines: "Wrap all errors with static errors from
errors/errors.go; useerrors.Joinfor multiple errors,%wfor string context, the error builder for complex errors, anderrors.Is()for checks; never use dynamic errors directly."🤖 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 `@pkg/runner/freshness/globber.go` around lines 32 - 49, The Glob method in pkg/runner/freshness/globber.go#L32-L49 must wrap non-missing-directory failures with ErrGlobInvalid using %w and include the pattern and baseDir context; retain the existing ErrFailedToFindImport handling. In pkg/runner/freshness/errors.go#L9-L11, keep ErrGlobInvalid declared exactly once after it is used, or relocate it to the central errors/errors.go if that is the established location.Source: Coding guidelines
🤖 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 `@cmd/cmd_utils.go`:
- Around line 1428-1448: Restore fail-stop behavior in the custom command step
loop: after the unforgiven failure branch records the error in commandErr and
sets conditionStatus to schema.ConditionPredicateFailure, immediately break out
of the loop. Preserve the forgiven continue path and successful freshness
recording behavior.
In `@cmd/custom_command_control_test.go`:
- Around line 51-57: Replace the shell command fixtures in the command-order and
related tests with a Go subprocess helper invoked via os.Executable(). Have the
helper write the requested marker file using os and filepath APIs, and return
the configured exit code, then update the test command data to invoke that
helper without echo, redirection, or exit shell syntax while preserving the
existing command-flow assertions.
In `@internal/exec/custom_command_control_adapter.go`:
- Around line 70-77: The TemplateData callback in ExecuteControlStep currently
ignores its matrix argument; update it to copy the base data from
control.Executor.Variables().TemplateData(), merge the per-child matrix values
using the same shape as the workflow control adapter, and return the merged map.
Add a rendering test covering matrix-specific values in a custom command’s
command or environment.
In `@internal/exec/workflow_utils.go`:
- Around line 531-546: Update the workflow dependency execution around
ExecuteWorkflow and taskgraph.Run so each concurrently running workflow receives
its own stepPkg.StepExecutor and associated Variables state. Remove reliance on
resetting or globally reading the shared stepExecutorState.Variables(), and pass
the per-invocation executor through workflow step execution so template results
cannot be mixed between dependencies.
In `@pkg/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 2473-2475: Update pkg/datafetcher/schema/atmos/manifest/1.0.json
at lines 2473-2475 to retain a single dependencies property and reference the
workflow dependency schema containing commands and workflows, removing the
duplicate declaration. Update
pkg/datafetcher/schema/stacks/stack-config/1.0.json at lines 2076-2078 to
reference that same workflow dependency schema so both configuration surfaces
validate the same contract.
In `@pkg/runner/freshness/checker.go`:
- Around line 325-341: Update Checker.RecordSuccess in
pkg/runner/freshness/checker.go:325-341 to persist state for artifacts-only
steps by removing the inputs == nil early return and hashing an empty source
list; retain the empty-inputs early return only where appropriate. Add the
artifacts-without-inputs regression case in
pkg/runner/freshness/checker_test.go:146-165, asserting ChecksumChanged is false
on the second Compute after RecordSuccess. In
website/docs/workflows/workflows/workflow/steps/artifacts.mdx:41, retain the
standalone-artifacts documentation because the checker fix supports it.
In `@pkg/schema/command.go`:
- Around line 13-28: Update FindCommandByName and command validation so
duplicate command names across nested commands and atmos.d-imported global
definitions cannot be resolved ambiguously. Prefer rejecting duplicate global
names during validation with a clear error; otherwise require
dependencies.commands references to use an unambiguous path-qualified form,
while preserving valid unique-name lookups.
In `@pkg/taskgraph/adapters/cobra_command.go`:
- Around line 215-224: In the command dispatch flow around
WithDependencyErrorSink and target.SetContext, save the target’s original
context and defer restoring it while targetLock remains held, ensuring
dependenciesResolved and the error sink do not persist across invocations.
Update TestCustomCommandRunner_SetsFlagsAndInvokesRun to verify context
restoration, and add a regression test covering a subsequent top-level
invocation.
In `@pkg/taskgraph/taskgraph.go`:
- Around line 101-132: In the scheduler run flow, preserve the current run-wide
fail-mode behavior but log aggregate.Err at warn level before returning nil for
FailBestEffort, including useful error context. Update the UnitDependency.Fail
schema description to explicitly document that fail mode is applied run-wide, or
otherwise align effectiveFailMode with per-entry sibling scoping; ensure the
chosen behavior matches the documented contract.
---
Outside diff comments:
In `@internal/exec/workflow_utils.go`:
- Around line 531-1059: Split ExecuteWorkflow into focused named helpers for
dependency resolution, environment/authentication setup, freshness and condition
evaluation, step execution, and error aggregation, leaving ExecuteWorkflow as a
short linear orchestration pipeline. Preserve existing ordering, shared state,
and error semantics while moving the corresponding logic into helpers, including
the step loop currently containing executeStep and workflowErr handling. Ensure
the resulting functions comply with the file and complexity limits.
---
Minor comments:
In `@cmd/cmd_utils.go`:
- Around line 964-1000: Update the freshness identity built in the
custom-command step loop around freshnessScope and freshness.StepIdentity so
resolved flag and argument values distinguish parameterized invocations of the
same command. Incorporate the resolved invocation values into the scope or
another identity component while preserving stable keys for identical
invocations, ensuring inputs/artifacts freshness does not incorrectly reuse
state across different parameterizations.
In `@docs/fixes/2026-08-06-line-prefix-writer-multiline-atomicity.md`:
- Around line 19-23: Add a language identifier, such as text or console, to the
opening fenced code block containing the failing test output in the
documentation. Keep the log contents unchanged.
In `@internal/exec/workflow_dependency_adapter_test.go`:
- Around line 65-66: Update the same-file dependency test at
internal/exec/workflow_dependency_adapter_test.go:65-66 and the cross-file
dependency test at internal/exec/workflow_dependency_adapter_test.go:119-120 to
record build and deploy events in a shared log, then assert that the build entry
precedes the deploy entry. Retain the existing assertions that both workflows
execute.
In `@internal/exec/workflow_utils_test.go`:
- Around line 1537-1568: Update
TestExecuteWorkflow_PreconditionSkipsWhenToolAlreadyOnPath to obtain the current
test executable with os.Executable(), assert that lookup succeeds, and use its
returned path as the Precondition.Tools value instead of the "go" binary.
Preserve the existing skipped-step assertion and workflow setup.
In `@internal/exec/workflow.go`:
- Around line 206-213: In internal/exec/workflow.go:206-213, wrap workflow file
read failures with the static workflow-file error and YAML parsing failures with
the static invalid-manifest error, preserving each underlying cause with %w. In
internal/exec/workflow_dependency_adapter.go:99-103, wrap executable-path
resolution failures with the static execution error before adding string
context, also using %w so errors.Is() can classify them.
In `@pkg/datafetcher/schema/atmos/config/1.0.json`:
- Around line 6168-6169: The schema documentation is attached to Inputs instead
of describing the correct declarations. Move the Task unit-of-work description
to the Task type declaration, give Inputs its own description covering freshness
inputs and the replacement of sources:/generates:/status:, then regenerate the
schema.
In `@pkg/flags/constrained.go`:
- Around line 86-87: Update the error handling around PersistentFlags().Set in
the prompting flow to wrap a suitable sentinel from errors/errors.go while
retaining setErr as the underlying cause. Replace the dynamic-only fmt.Errorf
classification with the repository’s static error and preserve the flag name in
the contextual message.
In `@pkg/flags/standard.go`:
- Around line 805-823: The shared ValidateValue membership check must support
caller-selected wording so positional arguments are not reported as flags.
Update ValidateValue and its callers, including constrained.go’s
CommandArgument.Values path, to distinguish argument versus flag context while
preserving the existing invalid-value details and shared validation behavior.
In `@pkg/hashfile/hashfile_test.go`:
- Around line 65-68: Update TestHashFiles_MissingFileErrors to create a
nonexistent path beneath t.TempDir() using filepath.Join instead of the
Unix-specific literal, while preserving the existing error assertion.
In `@pkg/hashfile/hashfile.go`:
- Around line 32-36: Update the file-operation error handling used by
writeRecord and hashFileContent, including the os.Open, Stat, and io.Copy
failure paths, to use a static project error from errors/errors.go. Wrap each
returned error with the relevant path and operation context while preserving the
static error for errors.Is classification.
In `@pkg/taskgraph/taskgraph.go`:
- Around line 229-233: Add a dedicated ErrMissingRefMetadata sentinel in
errors.go, then update the missing-ref branch in the scheduler.DispatcherFunc
callback to wrap it instead of ErrUnknownDependencyKind. Preserve the existing
node ID context and leave ErrUnknownDependencyKind reserved for unknown
dependency kinds.
In `@website/docs/workflows/workflows/workflow/steps/inputs.mdx`:
- Line 77: Update the CI cache guidance in the workflow inputs documentation to
place `.atmos/cache/freshness` under the documented `ci.cache.includes` field,
replacing the mismatched `ci.cache.paths` reference while preserving the
project-relative cache location and persistence behavior.
---
Nitpick comments:
In `@cmd/cmd_utils.go`:
- Around line 1041-1047: The command setup currently rebuilds flagsData and
validates constrained fields inside the per-step loop, causing required
interactive prompts to repeat. Move construction of argumentsData/flagsData and
the promptForSemanticValues/ValidateConstrainedFields flow above the
runnable-step loop, then reuse the populated maps when building each step’s
template data.
In `@cmd/custom_command_dependency_test.go`:
- Around line 54-69: Replace shell-based echo/redirection commands in
cmd/custom_command_dependency_test.go lines 54-69 and
cmd/custom_command_inputs_test.go lines 55-57, 158, 217, and 271 with
customCommandWriteHelperCommand. Preserve repeated-invocation counts by adding
append support to the helper or using separate marker files per invocation, then
update assertions accordingly; remove splitNonEmptyLines’ platform-specific
trimming rationale once migration is complete.
In `@pkg/flags/constrained_test.go`:
- Around line 21-131: Consolidate the non-interactive ValidateConstrainedFields
scenarios into one behavior-focused table-driven test, covering valid and
invalid arguments/flags, missing optional or required fields, wrong flag types,
and combined argument/flag validation. Keep separate focused tests for
interactive prompting and persistent-flag updates, and use each table case to
configure inputs and assert the expected error outcome.
In `@pkg/process/shell_command_unix.go`:
- Around line 12-17: Add a concise Go doc note to NewShellCommand stating that
callers must pass only command strings authored in trusted configuration, such
as atmos.yaml. Keep the existing shell invocation and function behavior
unchanged.
In `@pkg/runner/freshness/checker_test.go`:
- Around line 499-570: Move the defaultGlobber and globAll tests, including
TestDefaultGlobber_ResolvesRealFiles,
TestDefaultGlobber_MissingBaseDirIsNotAnError, TestGlobAll_PropagatesError, and
TestGlobAll_DeduplicatesAcrossPatterns, into globber_test.go. Move all
fileStateStore tests, including TestFileStateStore_SaveThenLoadRoundTrips and
the related tests currently elsewhere in this file, into state_test.go; leave
Checker tests in checker_test.go and do not disable the file-length linter.
- Around line 146-165: Add a test alongside
TestChecker_ArtifactsMissingForcesRerunEvenIfSourcesUnchanged for an
artifacts-only step with no inputs. Configure artifacts, call RecordSuccess, run
the second Compute, and assert facts.ChecksumChanged is false to verify the
documented standalone path does not rerun forever.
In `@pkg/runner/freshness/checker.go`:
- Around line 360-374: No code change is required for buildFileFacts; retain the
current per-file c.hasher invocation and lazy gating. Consider batching paths in
a future change only if per-file fact generation becomes common.
In `@pkg/runner/freshness/globber.go`:
- Around line 32-49: The Glob method in pkg/runner/freshness/globber.go#L32-L49
must wrap non-missing-directory failures with ErrGlobInvalid using %w and
include the pattern and baseDir context; retain the existing
ErrFailedToFindImport handling. In pkg/runner/freshness/errors.go#L9-L11, keep
ErrGlobInvalid declared exactly once after it is used, or relocate it to the
central errors/errors.go if that is the established location.
In `@pkg/runner/freshness/state.go`:
- Around line 98-116: Update the Save flow around the temporary file created by
os.CreateTemp to clean up stale *.tmp files in the freshness state directory
before or during each save. Ensure cleanup covers files left by killed processes
while preserving the existing atomic write-and-rename behavior and current error
handling.
- Around line 121-123: Validate key in recordPath before joining it with
stateDir, rejecting absolute paths, path separators, and traversal components so
the result remains within stateDir and remains safe for temporary-file creation.
Preserve the existing stateKey digest behavior and use the established
error-handling contract for invalid keys.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a3a6221-7763-4e01-ad2d-4c2ab1e0dd86
📒 Files selected for processing (79)
cmd/cmd_utils.gocmd/custom_command_aliases_test.gocmd/custom_command_control_test.gocmd/custom_command_dependency_test.gocmd/custom_command_inputs_test.gocmd/custom_command_values_test.gocmd/list/aliases.gocmd/list/aliases_test.godocs/fixes/2026-08-05-custom-command-dependency-shared-cobra-state.mddocs/fixes/2026-08-05-custom-command-freshness-when-precheck.mddocs/fixes/2026-08-05-workflow-command-dependency-wrong-atmos-binary.mddocs/fixes/2026-08-06-coderabbit-dependency-error-routing-and-context.mddocs/fixes/2026-08-06-hashfile-collision-and-streaming.mddocs/fixes/2026-08-06-line-prefix-writer-multiline-atomicity.mddocs/fixes/2026-08-06-schema-unitdependencies-string-shorthand.mddocs/fixes/2026-08-06-workflow-dependency-diamond-dedup.mderrors/errors.goexamples/task-runner-dependencies/atmos.yamlexamples/task-runner-dependencies/src/example.txtexamples/task-runner-dependencies/workflows/task-runner.yamlinternal/exec/custom_command_control_adapter.gointernal/exec/workflow.gointernal/exec/workflow_dependency_adapter.gointernal/exec/workflow_dependency_adapter_test.gointernal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gopkg/condition/cel.gopkg/condition/condition.gopkg/condition/condition_test.gopkg/condition/evaluate.gopkg/config/load.gopkg/config/schema/overrides.gopkg/datafetcher/schema/atmos/config/1.0.jsonpkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/flags/constrained.gopkg/flags/constrained_test.gopkg/flags/standard.gopkg/flags/standard_test.gopkg/hashfile/hashfile.gopkg/hashfile/hashfile_test.gopkg/io/line_prefix_writer.gopkg/process/exec_replace_windows.gopkg/process/shell_command_unix.gopkg/process/shell_command_windows.gopkg/process/shell_session.gopkg/runner/freshness/checker.gopkg/runner/freshness/checker_test.gopkg/runner/freshness/errors.gopkg/runner/freshness/globber.gopkg/runner/freshness/state.gopkg/schema/command.gopkg/schema/dependencies.gopkg/schema/dependencies_test.gopkg/schema/task.gopkg/schema/task_test.gopkg/schema/workflow.gopkg/taskgraph/adapters/cobra_command.gopkg/taskgraph/adapters/cobra_command_test.gopkg/taskgraph/errors.gopkg/taskgraph/ref.gopkg/taskgraph/schema.gopkg/taskgraph/taskgraph.gopkg/taskgraph/taskgraph_test.gopkg/workflow/condition_context.gowebsite/blog/2026-08-05-taskfile-convergence.mdxwebsite/docs/cli/configuration/aliases.mdxwebsite/docs/cli/configuration/commands/command/arguments.mdxwebsite/docs/cli/configuration/commands/command/dependencies.mdxwebsite/docs/cli/configuration/commands/command/flags.mdxwebsite/docs/cli/configuration/commands/command/index.mdxwebsite/docs/cli/configuration/commands/command/steps.mdxwebsite/docs/workflows/workflows/workflow/dependencies.mdxwebsite/docs/workflows/workflows/workflow/steps/artifacts.mdxwebsite/docs/workflows/workflows/workflow/steps/continue.mdxwebsite/docs/workflows/workflows/workflow/steps/index.mdxwebsite/docs/workflows/workflows/workflow/steps/inputs.mdxwebsite/docs/workflows/workflows/workflow/steps/precondition.mdxwebsite/src/data/roadmap.js
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (79.12%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2882 +/- ##
==========================================
- Coverage 82.76% 82.75% -0.02%
==========================================
Files 1861 1872 +11
Lines 180478 181439 +961
==========================================
+ Hits 149380 150144 +764
- Misses 23311 23448 +137
- Partials 7787 7847 +60
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Fixes: - pkg/runner/freshness/checker.go: RecordSuccess early-returned when inputs == nil, so an artifacts-only step never persisted state and reran forever even once its artifact existed and was unchanged. Widen the caller gates and hash an empty source list instead of skipping the record entirely. - pkg/taskgraph/adapters/cobra_command.go: a dependency dispatch permanently left its dependencies-resolved marker and drained error sink on the target *cobra.Command's context. Restore the original context via defer so a later top-level invocation of the same command object (long-lived processes, test suites reusing RootCmd) doesn't inherit stale dispatch state. - pkg/taskgraph/taskgraph.go: a `fail: best_effort` entry silences the whole run's failures, including siblings that declared no fail: at all -- documented as explicitly run-wide (not per-entry) on UnitDependency.Fail, and the swallowed error is now logged at warn level so it isn't invisible. - pkg/datafetcher/schema/atmos/manifest/1.0.json + pkg/datafetcher/schema/stacks/stack-config/1.0.json: the shared "dependencies" definition only modeled tools/components/files/folders, so a documented dependencies.commands/dependencies.workflows declaration was rejected by the manifest schema (additionalProperties: false) and had no typed shape in the stack-config schema. Also removes a duplicate "dependencies" key in the manifest's workflow_manifest object (Biome noDuplicateObjectKeys). - pkg/schema/command.go: FindCommandByName resolved a bare name to the first depth-first match, so a config with duplicate global/nested command names could route a dependencies.commands reference to the wrong target. Now reports ambiguous=true instead of guessing; CommandLookup surfaces this as a clear error during graph-building, before any dispatch happens. Investigated, not applied: - internal/exec/workflow_utils.go's stepExecutorState global can race when taskgraph.Run dispatches multiple sibling dependencies.workflows entries concurrently. Documented the failure mode and why a quick mutex is unsafe here (deadlocks on multi-level dependency chains, or leaves a stale-pointer race window) -- a real fix means threading a *stepPkg.StepExecutor through ExecuteWorkflow instead of reaching for the package-level var, which is a larger, separately-scoped refactor. Verified as correct behavior, not bugs (added regression tests either way): - cmd/cmd_utils.go's unforgiven-failure step loop deliberately doesn't break -- it matches the pre-existing workflow executor's identical pattern and GHA's own if:success()/if:failure() semantics, so a when:failure handler step still runs after an unforgiven failure. - internal/exec/custom_command_control_adapter.go's TemplateData callback ignoring its matrix argument matches the workflow control adapter's identical, already-correct pattern -- pkg/workflow/control.go's controlTemplateData injects matrix independently of the callback. Replaces the remaining shell echo/redirect/exit test fixtures in cmd/custom_command_control_test.go with Go-native os.Executable() helpers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/datafetcher/schema/atmos/manifest/1.0.json`:
- Around line 1627-1661: The duplicated unit_dependency definitions allow
dependencies without a name; add required ["name"] to
pkg/datafetcher/schema/atmos/manifest/1.0.json lines 1627-1661 and
pkg/datafetcher/schema/stacks/stack-config/1.0.json lines 1272-1306. Keep both
schema surfaces aligned so entries such as {} or flag-only objects are rejected.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a641a72f-b7f9-4298-8155-9f110969a739
📒 Files selected for processing (20)
cmd/cmd_utils.gocmd/custom_command_control_test.gocmd/custom_command_integration_test.goerrors/errors.gointernal/exec/workflow_dependency_adapter.gointernal/exec/workflow_dependency_adapter_test.gointernal/exec/workflow_utils.gopkg/datafetcher/schema/atmos/config/1.0.jsonpkg/datafetcher/schema/atmos/manifest/1.0.jsonpkg/datafetcher/schema/stacks/stack-config/1.0.jsonpkg/datafetcher/schema_workflow_validation_test.gopkg/runner/freshness/checker.gopkg/runner/freshness/checker_test.gopkg/schema/command.gopkg/schema/command_find_test.gopkg/schema/dependencies.gopkg/taskgraph/adapters/cobra_command.gopkg/taskgraph/adapters/cobra_command_test.gopkg/taskgraph/taskgraph.gopkg/taskgraph/taskgraph_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- errors/errors.go
- internal/exec/workflow_dependency_adapter.go
- pkg/taskgraph/adapters/cobra_command_test.go
- pkg/schema/dependencies.go
- pkg/taskgraph/adapters/cobra_command.go
- internal/exec/workflow_utils.go
- pkg/taskgraph/taskgraph.go
- pkg/datafetcher/schema/atmos/config/1.0.json
- cmd/cmd_utils.go
- pkg/runner/freshness/checker_test.go
- pkg/runner/freshness/checker.go
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…first-class-support # Conflicts: # pkg/io/line_prefix_writer.go
GitHub Actions
Inactive
Bump the pnpm overrides pinning js-yaml and mermaid to their patched versions -- all within the same major version, so no dependabot.yml ignore-policy exception is needed: - js-yaml@^3: 3.15.0 -> 3.15.1 (GHSA-5p4m-2wfm-xmqj, alert #269) - js-yaml@^4: 4.2.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, alert #268) - mermaid@^11: 11.15.0 -> 11.16.1 (GHSA-rhh3-jpg6-66xh #267, GHSA-c4c3-pg64-4m4v #266, GHSA-6x64-9x62-f2gx #265, GHSA-3rrr-jr9j-h3q3 #264, GHSA-2v8p-3f2j-5mp7 #263) Verified: pnpm install regenerates the lockfile at the patched versions, `npm run build` succeeds, and NOTICE is unchanged (no license drift). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub Actions
Inactive
…oward 85%
Schema fix (CodeRabbit):
- pkg/datafetcher/schema/atmos/manifest/1.0.json + stacks/stack-config/1.0.json:
unit_dependency accepted an entry with no `name` (e.g. `{}` or
`{flags: {env: dev}}`), producing a reference with an empty name that only
failed later during graph construction instead of failing fast at schema
validation. Add "required": ["name"] to both copies.
Coverage (patch coverage was 79.12% against an 85% target; 224 lines missing):
adds real, behavior-asserting tests across cmd, internal/exec, pkg/condition,
pkg/datafetcher, pkg/flags, pkg/hashfile, pkg/runner/freshness, pkg/schema,
pkg/taskgraph, and pkg/workflow -- error paths, edge cases, and previously
uncovered branches added by this PR's dependencies/freshness/continue/matrix
work. Notably: pkg/flags/constrained.go gained isInteractiveFn/promptForValueFn
DI seams (mirroring the existing pattern in cmd/secret/deps.go) so the
interactive-prompt branches of ValidateConstrainedFields are testable without
a real TTY. Genuinely untestable lines (defensive/unreachable code, no
injection seam, TTY-only, or requiring real network/toolchain access) are
left uncovered with the reasoning documented at each call site rather than
padded with tautological tests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub Actions
In progress
|
Warning This PR exceeds the recommended limit of 10,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
2 participants
what
dependencies.commands/dependencies.workflowsto custom commands and workflows: named, parameterized, concurrent-by-default dependency ordering across units, with automatic dedup of identical invocations.inputs/artifactsstep fields: skip a step when its declared sources haven't changed since the last successful run (implicitwhen: checksum.changed), exposingchecksum.changed/timestamp.changed/sources/artifactsaswhen:CEL facts.preconditionstep field: skip a step when a required tool is already onPATH(implicitwhen: "!precondition.success"), resolved viaexec.LookPath— no shell involved.continue: alwaysstep field, mirroring GitHub Actions'continue-on-error: a step's own failure is forgiven, later steps still run, overall exit status unaffected.type: parallel/type: matrixsteps silently failing in custom commands (only workflows supported them before).platformsviawhen:CEL facts (os/arch/platform), native per-commandaliases:/internal:, and avalues:constraint on flags/arguments with an interactive picker.cmd/custom_command_dependency_adapter.goandcmd/custom_command_values.gointopkg/taskgraph/adaptersandpkg/flagsrespectively, so this logic is unit-testable in isolation instead of coupled tocmd's live command registry.atmos/manifest,config/global,stacks/stack-config) accordingly.why
Atmos workflows and custom commands already covered most of what a task runner needs, but a handful of real gaps kept teams running go-task alongside Atmos: no dependency ordering between named commands/workflows, no up-to-date checking, no continue-on-error, no precondition shortcut, and custom commands couldn't even use
parallel/matrixsteps — the exact recipe the project's own go-task migration guide recommends for concurrent dependents. This closes those gaps using the existingwhen:/CEL condition engine and scheduler rather than inventing a second mechanism.references
website/blog/2026-08-05-taskfile-convergence.mdxSummary by CodeRabbit
continueconditions to allow selected step failures without stopping subsequent execution.