fix(steps): resolve relative paths against step.WorkingDirectory - #2880
fix(steps): resolve relative paths against step.WorkingDirectory#2880Erik Osterman (Cloud Posse) (osterman) wants to merge 13 commits into
Conversation
The archive, file, workdir, junit, and container build step handlers resolved relative source/destination/path/glob/context fields via template substitution only, then let filesystem calls resolve them against the Atmos process's own cwd instead of step.WorkingDirectory. This surfaced most visibly for `type: archive` hooks, since the hooks engine correctly defaults working_directory to the component path but the handler never read it back. Add a shared BaseHandler.ResolveInWorkingDirectory helper that anchors a relative resolved value to step.WorkingDirectory (falling back to process cwd when unset, matching prior behavior), and apply it across the five affected handlers. container_build.go additionally anchors Dockerfile to the resolved Context rather than WorkingDirectory directly, matching Docker's own convention. 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:
📝 WalkthroughWalkthroughStep handlers now resolve relative filesystem paths against ChangesWorking-directory path resolution
Estimated code review effort: 4 (Complex) | ~45 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 |
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resource Changes Found for
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2880 +/- ##
=======================================
Coverage 82.76% 82.76%
=======================================
Files 1861 1861
Lines 180478 180551 +73
=======================================
+ Hits 149380 149442 +62
- Misses 23311 23321 +10
- Partials 7787 7788 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…nd loadReport Codecov flagged handler_base.go and junit.go below the 85% patch-coverage threshold. Add a case that exercises the relative (non-template) WorkingDirectory branch in resolveWorkingDirectory, and a junit test that triggers a WorkingDirectory template-resolution error from inside loadReport's per-pattern loop, distinct from the already-covered `files` template error path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 `@docs/fixes/2026-08-05-step-handlers-ignore-working-directory.md`:
- Around line 7-10: Update the sentence in the documentation to add “that” after
“type: archive steps,” making the subject grammatically complete while
preserving the rest of the explanation.
🪄 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: 1e889ec1-4f4c-4cc3-aeaa-28caf4c91da5
📒 Files selected for processing (16)
docs/fixes/2026-08-05-step-handlers-ignore-working-directory.mdpkg/hooks/step_engine_test.gopkg/runner/step/archive.gopkg/runner/step/archive_test.gopkg/runner/step/container_actions_extra_test.gopkg/runner/step/container_build.gopkg/runner/step/container_runtime_fake_test.gopkg/runner/step/container_test.gopkg/runner/step/file.gopkg/runner/step/file_test.gopkg/runner/step/handler_base.gopkg/runner/step/handler_base_test.gopkg/runner/step/junit.gopkg/runner/step/junit_test.gopkg/runner/step/workdir.gopkg/runner/step/workdir_test.go
…rror context A field test of the WorkingDirectory fix found the container_build.go handler was only partially fixed: build.bake.file/bake.files and cache.from/cache.to type: local src/dest still resolved against the Atmos process's own cwd instead of step.WorkingDirectory, reproduced live as a silent build against the wrong bake file. Both now route through ResolveInWorkingDirectory like context/dockerfile already do. Also root-caused and fixed a separate defect surfaced while verifying error output: buildWorkflowStepError dual-wrapped step errors with fmt.Errorf before building the final error, and cockroachdb/errors treats that Go 1.20 multi-error shape as an opaque leaf node, silently dropping any hints/context a handler attached deeper in the chain. Switched to WithCause, which extracts them eagerly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/runner/step/container_build.go (1)
98-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve Buildx remote inputs before filesystem anchoring.
build.context,build.dockerfile, andbuild.bake.filecan be remote references such ashttps://github.com/org/repo.git, whilebuild.bake.filescan be multiple remote definitions. These values are not absolute filesystem paths, butfilepath.IsAbstreats them as relative and anchors them underContextDir/WorkingDirectory, changing supported Docker Buildx inputs into invalid paths. Classify supported remote forms after template resolution and bypass working-directory anchoring for them; add regression cases covering remote context, Dockerfile, and Bake definitions.🤖 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/step/container_build.go` around lines 98 - 108, Update the path-resolution flow in the container build handling around ResolveInWorkingDirectory, resolveOptional, and the build.bake file resolution so supported remote references such as HTTPS Git URLs remain unchanged after template resolution instead of being anchored to ContextDir or WorkingDirectory. Apply the same remote classification to build.context, build.dockerfile, build.bake.file, and each entry in build.bake.files, while preserving filesystem anchoring for local paths; add regression coverage for remote context, Dockerfile, and Bake definitions.
🧹 Nitpick comments (1)
pkg/runner/step/container_actions_extra_test.go (1)
209-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse tables for the scenario variants.
Both tests enumerate multiple input and expected-output scenarios in separate
t.Runblocks. Convert each set to a table and loop over the cases. This keeps future path variants consistent.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/runner/step/container_actions_extra_test.go` around lines 209 - 284, The tests covering bake path resolution and build-cache path resolution should use table-driven scenarios instead of separate t.Run blocks. Refactor the cases in the relevant test functions around buildBuildConfig and resolveBuildCache into case tables containing inputs and expected outputs, then iterate over each table entry with t.Run while preserving the existing assertions and distinct behavior for relative, absolute, local, and non-local paths.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 `@docs/fixes/2026-08-05-step-handlers-ignore-working-directory.md`:
- Line 138: Remove the host-specific `/tmp/atmos-field-test/` reference from the
field-test repro note, replacing it with a relative fixture identifier or a
worktree-local reference while preserving the `container-bake-print` context.
In `@internal/exec/workflow_utils_test.go`:
- Around line 661-701: Move TestBuildWorkflowStepError and
TestBuildWorkflowStepErrorPreservesInnerHintsAndContext from
workflow_utils_test.go into a focused workflow-step-error test file, preserving
their setup and assertions. Keep related workflow-step-error tests together and
ensure workflow_utils_test.go remains below 600 lines.
---
Outside diff comments:
In `@pkg/runner/step/container_build.go`:
- Around line 98-108: Update the path-resolution flow in the container build
handling around ResolveInWorkingDirectory, resolveOptional, and the build.bake
file resolution so supported remote references such as HTTPS Git URLs remain
unchanged after template resolution instead of being anchored to ContextDir or
WorkingDirectory. Apply the same remote classification to build.context,
build.dockerfile, build.bake.file, and each entry in build.bake.files, while
preserving filesystem anchoring for local paths; add regression coverage for
remote context, Dockerfile, and Bake definitions.
---
Nitpick comments:
In `@pkg/runner/step/container_actions_extra_test.go`:
- Around line 209-284: The tests covering bake path resolution and build-cache
path resolution should use table-driven scenarios instead of separate t.Run
blocks. Refactor the cases in the relevant test functions around
buildBuildConfig and resolveBuildCache into case tables containing inputs and
expected outputs, then iterate over each table entry with t.Run while preserving
the existing assertions and distinct behavior for relative, absolute, local, and
non-local paths.
🪄 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: f96657a1-4197-4134-9bba-e825b0523697
📒 Files selected for processing (6)
docs/fixes/2026-08-05-step-handlers-ignore-working-directory.mdinternal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gopkg/runner/step/container_actions_extra_test.gopkg/runner/step/container_build.gopkg/runner/step/container_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/runner/step/container_test.go
…rror context Address CodeRabbit findings on PR #2880: - Remove the host-specific /tmp/atmos-field-test/ path from the fix doc, replacing it with a worktree-relative fixture reference. - Split TestBuildWorkflowStepError and TestBuildWorkflowStepErrorPreservesInnerHintsAndContext out of workflow_utils_test.go into a focused workflow_step_error_test.go, keeping step-error tests co-located and out of the oversized (pre-existing) workflow_utils_test.go file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ache anchoring Codecov flagged container_build.go's patch coverage below 85% after the bake/cache anchoring follow-up. Add regression tests for the three previously-uncovered error branches: resolveBakeFiles propagating a per-entry template failure, anchorCacheLocalPaths' own resolve failure, and that failure propagating out through resolveBuildCache. handler_base.go's remaining gap (os.Getwd/filepath.Abs failing inside resolveWorkingDirectory) is left uncovered deliberately -- it requires corrupting the process's own working directory to reach, has no DI seam, and matches this repo's existing untested pattern for the identical os.Getwd/filepath.Abs fallback in container_run.go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 `@internal/exec/workflow_step_error_test.go`:
- Around line 27-28: Update all table cases in the workflow path tests to create
an absolute root with t.TempDir(), then construct WorkflowPath and
WorkflowBasePath using filepath.Join instead of hard-coded Unix paths. Normalize
any expected nested-path suffix used in assertions with filepath.ToSlash,
applying the same construction consistently across every listed case.
🪄 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: fa40af1a-ee6d-4358-ae85-02a453f6f776
📒 Files selected for processing (4)
docs/fixes/2026-08-05-step-handlers-ignore-working-directory.mdinternal/exec/workflow_step_error_test.gointernal/exec/workflow_utils_test.gopkg/runner/step/container_actions_extra_test.go
💤 Files with no reviewable changes (1)
- internal/exec/workflow_utils_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/fixes/2026-08-05-step-handlers-ignore-working-directory.md
- pkg/runner/step/container_actions_extra_test.go
…nection refused) CI's Check Markdown Links job failed on docs/prd/archive-step.md's citation of the SOURCE_DATE_EPOCH origin -- the CI runner's outbound request was refused while the page returns 200 OK outside CI. Follows the repo's existing precedent for excluding CI-hostile hosts (docs.docker.com, otelic.com, taskfile.dev, etc.) in lychee.toml. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
An explicit working_directory: on a kind: step/kind: steps hook was always resolved against the Atmos process's own cwd, regardless of shape. Following docs/prd/base-path-resolution-semantics.md's existing Dot/Bare convention: a dot-prefixed value (., .., ./x, ../x) keeps resolving against the process cwd; a bare relative value (x, x/y) now resolves against the component's own working directory instead -- the same directory ComponentPath(ctx) already computes for the unset-default case, so this stays compatible with provisioned working directories and metadata.component aliasing for free. Workflows and custom commands are unaffected -- they have no "current component" concept, so bare-relative values there still resolve against the process cwd exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/fixes/2026-08-07-lychee-reproducible-builds-org-connection-refused.md (1)
28-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the Lychee exclusion to the failing host.
reproducible-builds\.orghas no^https?://or end anchor, so Lychee can exclude any URL containing that string—including subdomains and mirrored URLs. Use an exact URL or a host-bounded pattern if this CI failure affects only this link.🤖 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-07-lychee-reproducible-builds-org-connection-refused.md` around lines 28 - 30, Update the Lychee exclude regex entry for reproducible-builds.org to match only the failing URL or exact host, using protocol anchoring and host boundaries as appropriate; avoid an unbounded substring pattern that also excludes subdomains or mirrored URLs, while preserving the existing comment and placement.
🤖 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 `@docs/fixes/2026-08-07-hook-working-directory-bare-vs-dot-anchoring.md`:
- Around line 19-20: Fix the markdownlint MD018 warning in the paragraph
referencing PR `#2880` by escaping the leading hash or moving the PR reference to
the preceding line, while preserving the existing prose and meaning.
In `@website/docs/cli/configuration/commands/command/working-directory.mdx`:
- Around line 17-19: Update the relative-path documentation in the
working-directory section to state that paths are resolved against the process
working directory when no component anchor exists, matching
BaseHandler.resolveWorkingDirectory and the workflow/hook documentation; remove
the claim that every relative value uses the Atmos base_path.
---
Nitpick comments:
In `@docs/fixes/2026-08-07-lychee-reproducible-builds-org-connection-refused.md`:
- Around line 28-30: Update the Lychee exclude regex entry for
reproducible-builds.org to match only the failing URL or exact host, using
protocol anchoring and host boundaries as appropriate; avoid an unbounded
substring pattern that also excludes subdomains or mirrored URLs, while
preserving the existing comment and placement.
🪄 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: 4a45e742-24da-42a8-bd89-2746a0bf002c
📒 Files selected for processing (13)
agent-skills/skills/atmos-hooks/SKILL.mdagent-skills/skills/atmos-steps/SKILL.mddocs/fixes/2026-08-07-hook-working-directory-bare-vs-dot-anchoring.mddocs/fixes/2026-08-07-lychee-reproducible-builds-org-connection-refused.mdlychee.tomlpkg/hooks/step_engine.gopkg/hooks/step_engine_test.gopkg/runner/step/handler_base.gopkg/runner/step/handler_base_test.gopkg/runner/step/variables.gowebsite/docs/cli/configuration/commands/command/working-directory.mdxwebsite/docs/stacks/hooks.mdxwebsite/docs/workflows/workflows/workflow/steps/working-directory.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/runner/step/handler_base_test.go
Bump pnpm.overrides floors for transitive npm dependencies flagged by Dependabot (all patch/minor bumps within the same major, not blocked by dependabot.yml's major-version ignore policy): - js-yaml 3.15.0 -> 3.15.1 (#269, GHSA-5p4m-2wfm-xmqj: quadratic CPU consumption in !!omap resolution, high) - js-yaml 4.3.0 -> 4.3.1 (#268, same advisory, 4.x line, high) - mermaid 11.16.0 -> 11.16.1 (#267, #266, #265, #264, #263: five advisories ranging low-medium) NOTICE unchanged (no license changes). Verified: pnpm install regenerated website/pnpm-lock.yaml with the bumped versions resolved, atmos fix lint (patch-scoped, no Go files touched) is a no-op, and cd website && npm run build succeeds with no new broken links. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Address CodeRabbit finding on PR #2880: a paragraph wrap left "#2880):" at the start of a line, which markdownlint's atx-heading rule (MD018) flags as a heading missing a space after the hash. Rewrap so the line doesn't start with a bare hash-prefixed token. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found by field-testing the working_directory fix against real hooks, workflows, and a live docker build instead of just unit tests: - workdir step: `source` (local relative path) now anchors to `working_directory` via a new `sourceprov.WithBaseDir` option, matching `path`'s existing anchoring instead of silently falling back to the process cwd. - workflow-level `working_directory:` default now reaches extended step types (archive/file/junit/workdir/container), not just shell/exec/atmos steps, which silently ignored it before. - step-level `working_directory` now expands a leading `~`, matching the tilde expansion --chdir already gets. - Corrected agent-skills/atmos-steps and the workflow/hooks docs, which claimed a single blanket CWD-vs-base_path rule that doesn't match actual per-surface behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@agent-skills/skills/atmos-steps/SKILL.md`:
- Around line 142-156: Document in agent-skills/skills/atmos-steps/SKILL.md
lines 142-156 and
website/docs/workflows/workflows/workflow/steps/working-directory.mdx lines
30-48 that relative handler fields source, destination, path, files, and context
resolve against the resolved step working directory; state separately that a
container Dockerfile resolves relative to the resolved context. Preserve the
existing workflow-default fallback wording in both locations.
In `@website/docs/workflows/workflows/workflow/steps/working-directory.mdx`:
- Around line 44-48: Update the working-directory resolution documentation in
the referenced workflow step section to explicitly state that an unset hook
working_directory defaults to the component’s working directory. Preserve the
existing rules for bare and dot-prefixed values, ensuring the documented
behavior matches the atmos-steps guidance.
🪄 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: a7e21712-aafb-4677-aae7-75ddf55c9070
⛔ Files ignored due to path filters (1)
website/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
agent-skills/skills/atmos-steps/SKILL.mddocs/fixes/2026-08-07-hook-working-directory-bare-vs-dot-anchoring.mdinternal/exec/workflow_utils.gointernal/exec/workflow_utils_test.gopkg/hooks/step_engine_test.gopkg/provisioner/source/vendor.gopkg/runner/step/handler_base.gopkg/runner/step/handler_base_test.gopkg/runner/step/workdir.gopkg/runner/step/workdir_test.gowebsite/docs/stacks/hooks.mdxwebsite/docs/workflows/workflows/workflow/steps/working-directory.mdxwebsite/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- website/docs/stacks/hooks.mdx
- docs/fixes/2026-08-07-hook-working-directory-bare-vs-dot-anchoring.md
- pkg/runner/step/workdir_test.go
- pkg/runner/step/handler_base.go
- pkg/hooks/step_engine_test.go
…contracts Addresses CodeRabbit feedback on PR #2880: state that relative handler fields (source, destination, path, files, context) resolve against the already-resolved working_directory, that a container Dockerfile resolves relative to context rather than working_directory directly, and that an unset kind: step hook working_directory (not just a bare value) anchors to the component's own working directory. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
what
type: archive,file,workdir,junit, and containerbuildstep handlers to resolve relativesource/destination/path/files/context/dockerfilefields againststep.WorkingDirectoryinstead of the Atmos process's own cwd.BaseHandler.ResolveInWorkingDirectoryhelper (pkg/runner/step/handler_base.go) used by all five handlers; containerbuildadditionally anchorsDockerfileto the resolvedContext, matching Docker's own convention.TestStepEngineRunsArchiveTypeWithRelativeWorkingDirectory) reproducing the original bug end-to-end.why
type: archivesteps run as component lifecycle hooks ignoredstep.WorkingDirectory, even though the hooks engine (pkg/hooks/step_engine.go) already correctly computes and sets it to the resolved component path before dispatch — the field was just never read back out by the handler.file,workdir,junit, containerbuild) with the identical bug: relative paths resolved via template substitution only, then silently anchored to process cwd instead of the step's configured working directory.references
Summary by CodeRabbit
Bug Fixes
Documentation
Tests