Repository navigation
Add task-runner dependencies, freshness checks, and preconditions to custom commands and workflows - #2882
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 DAGs, freshness-aware conditions, failure continuation, parallel and matrix steps, aliases, internal visibility, platform facts, constrained values, and expanded validation. ChangesTask-runner contracts and execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CustomCommand
participant Taskgraph
participant DependencyRunner
participant WorkflowExecutor
CustomCommand->>Taskgraph: Resolve command and workflow dependencies
Taskgraph->>DependencyRunner: Execute deduplicated dependency nodes
DependencyRunner->>WorkflowExecutor: Run workflow dependency steps
WorkflowExecutor-->>Taskgraph: Return dependency result
Taskgraph-->>CustomCommand: Apply failure mode
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 FilesNone |
![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>
GitHub Actions
Inactive
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…first-class-support # Conflicts: # pkg/flags/standard.go # pkg/flags/standard_test.go
…first-class-support # Conflicts: # website/package.json # website/pnpm-lock.yaml
…first-class-support # Conflicts: # internal/exec/workflow.go
GitHub Actions
Inactive
Erik Osterman (Cloud Posse) (osterman)
changed the title
Verified each finding against current code, fixing what was still valid and skipping what was already resolved or working as intended (documented inline where relevant). Fixed: - cmd_utils.go: hoist argumentsData/flagsData construction and values: validation above the per-step loop, so interactive prompts fire once per command invocation instead of once per step. - pkg/taskgraph: new ErrMissingRefMetadata sentinel instead of reusing ErrUnknownDependencyKind for a missing-graph-metadata condition. - internal/exec: wrap raw file-read/YAML-parse/executable-resolution errors with static sentinels (ErrReadFile, ErrInvalidWorkflowManifest, new ErrResolveExecutablePath) for errors.Is() classification. - pkg/flags: wrap PersistentFlags().Set failures with errUtils.ErrSetFlag; add a ValueKind (flag vs argument) parameter to ValidateValue so a values:-constrained positional argument is never reported as an invalid flag. - pkg/hashfile: wrap file-operation errors with static sentinels; replace a hardcoded Unix path in a test with a cross-platform t.TempDir() path. - pkg/runner/freshness: wire the previously-dead ErrGlobInvalid sentinel into Glob()'s real error path; harden recordPath against a path-traversing state key; clean up stale Save() *.tmp files left by a killed process. - pkg/process: document that NewShellCommand requires trusted config input. - pkg/schema/task.go: fix a doc-comment merge bug where Task's doc comment had been swallowed into Inputs' (no blank line between adjacent type declarations), leaving Task undocumented and Inputs mis-described in the generated JSON schema. Regenerated pkg/datafetcher/schema/atmos/config/1.0.json. - Docs: add a language tag to a fenced code block in a fix log. - Tests: doc comments on 6 exported test functions; migrate 3 test files' echo-based shell fixtures to existing Go-native helpers; strengthen the same-file/cross-file dependency tests to assert execution ORDER via a shared log, not just that both steps ran; switch the preconditions.tools regression test off the "go" binary onto the running test binary's own os.Executable() path. Skipped as already fixed/working as intended (verified against current code): custom-command fail-stop behavior (contradicted by an existing test documenting the no-break loop as deliberate), matrix TemplateData's ignored matrix arg (by design -- pkg/workflow/control.go injects .matrix after the callback), FindCommandByName ambiguous-name handling, cobra_command.go context restoration, taskgraph FailBestEffort logging + run-wide Fail docs, RecordSuccess artifacts-only handling, schema unit_dependency required: ["name"], preconditions.mdx GOBIN/PATH note, and a claimed duplicate `dependencies` schema property (schema has since regenerated/drifted). The ci.cache.includes doc finding was factually wrong (the real field is ci.cache.paths per pkg/schema/schema.go) and was left unchanged. Skipped as real but out of scope for a minimal pass: the stepExecutorState global-state race across concurrently-dispatched sibling workflows and splitting the ~700-line ExecuteWorkflow into helpers (both already documented in-code as known/deferred); two pure test-organization nitpicks (table-driven consolidation, splitting checker_test.go into separate files); one shell-fixture migration that couldn't be confidently line-matched after drift. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.226.0-rc.5. |
This branch was successfully deployed
3 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.preconditionsstep field: skip a step when a required tool is already onPATH(implicitwhen: "!preconditions.success"), resolved viaexec.LookPath— no shell involved. Pluralized (precondition→preconditions) to match the block-of-checks convention already used byinputs/artifacts/dependencies.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.parallel/matrix, and verbatimCmdLineconstruction forcmd.exe /C.UnitDependenciesstring-shorthand schema gap.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.mdx