Repository navigation
feat(workflows): add native archive step type for zip/tar packaging - #2730
Merged
Merged
Conversation
Packaging build artifacts (e.g. a Lambda handler.zip) had no native Atmos primitive; the only option was shelling out to zip/tar, which breaks on Windows and gives no typed validation. The new `type: archive` step packs a directory/file into a zip/tar/tgz archive using the Go standard library only. It reuses the existing `kind: step` hook bridge instead of adding a dedicated hook kind, so it's usable as a lifecycle hook for free. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
July 11, 2026 20:48
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
lychee's exclude list only whitelists /cli/ and /functions/ root-relative website paths (the only forms it can't resolve locally); /stacks/generate isn't covered, so the link checker treated it as a literal repo-relative file path and failed. Match the convention every other PRD doc uses for paths outside those two prefixes: a full https://atmos.tools/... URL. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resource Changes Found for
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a native ChangesArchive step contract and integration
Archive implementation
Documentation and repository support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ArchiveHandler
participant VariableResolver
participant ArchiveRun
participant DetectFormat
participant CollectEntries
participant AtomicWriter
ArchiveHandler->>VariableResolver: Resolve archive fields
ArchiveHandler->>ArchiveRun: Pass action and PackOptions
ArchiveRun->>DetectFormat: Detect format
ArchiveRun->>CollectEntries: Collect filtered entries
CollectEntries-->>ArchiveRun: Return pack entries
ArchiveRun->>AtomicWriter: Write or update archive
AtomicWriter-->>ArchiveHandler: Return success or typed error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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/prd/archive-step.md`:
- Line 267: Update the “Generate Terraform Files” link in the archive-step
documentation to reference an existing documentation path or valid website route
instead of /stacks/generate, ensuring link checking resolves the target
successfully.
In `@pkg/archive/archive.go`:
- Around line 140-146: Update validatePackOptions to check for a nil opts before
accessing Source or Destination, and return the established typed validation
error for invalid pack options. Preserve the existing source and destination
validation for non-nil PackOptions.
- Around line 78-80: Update the format detection call in the archive flow around
DetectFormat to infer from opts.Destination first and fall back to the source
archive path when Format is omitted, preserving explicit format handling. Add a
regression test covering a file source such as bundle.tar with a destination
such as out.bin and asserting tar inference.
In `@pkg/archive/tar.go`:
- Around line 17-49: Update writeTar to create and write the archive through a
temporary file in the destination directory, preserving the existing tar/gzip
construction and error handling. Close and remove the temporary file on every
failure, including addTarEntry, tar.Close, and gzip.Close; rename the completed
temporary file to destination only after all writes succeed, matching the atomic
replacement pattern used by updateTar.
- Around line 65-85: Update the archive update flow around the temporary file
creation and os.Rename to preserve the existing destination file mode when
replacing an existing archive. Capture the destination’s permissions before
creating the temp file, apply that mode to the temp file before renaming, and
retain the current behavior for destinations that do not already exist.
In `@pkg/archive/walk.go`:
- Around line 76-80: Update the walkErr handling in the archive walking flow to
classify only genuine missing-source failures as ErrArchiveSourceNotFound.
Preserve already-classified filter errors and return other filesystem or
callback failures distinctly, while retaining the existing source context where
appropriate.
- Around line 36-38: Update the single-file branch in the archive-walking
function around the !info.IsDir() check to evaluate the source basename against
the configured Include and Exclude filters before returning a packEntry. Reuse
the existing filter-matching logic used for directory entries, returning no
entries when the file is excluded or not included; otherwise preserve the
current archivePath construction and return behavior.
- Around line 37-38: Validate the subpath before the archive-entry returns and
joins in the walk logic around archivePath, including the paths used at the
referenced entry ranges. Reject absolute paths and any "." or ".." components
when split on both forward and backslashes, then only call archiveJoin for valid
relative subpaths.
In `@pkg/archive/zip.go`:
- Around line 13-33: Update writeZip to create a temporary file in the
destination directory instead of calling os.Create on destination, write and
finalize the ZIP there, and propagate errors from both file and ZIP close
operations. Rename the completed temporary file to destination only after all
writes and closes succeed, and remove the temporary file on any failure so the
existing archive remains intact.
In `@website/blog/2026-07-11-archive-step-type.mdx`:
- Around line 76-78: Update the format option documentation near the format
description to state that the archive format is inferred from the destination
extension, falling back to the source extension when the destination has no
extension. Preserve the existing list of supported formats and the note about
unsupported tar.bz2/tar.xz writing.
In `@website/docs/workflows/workflows/workflow/steps/type.mdx`:
- Line 30: Update the archive step entry in the workflow steps table so its
purpose text mentions all supported formats, including tgz, using “zip/tar/tgz”
or a link to the complete format list. Leave the existing configuration keys
unchanged.
In `@website/docs/workflows/workflows/workflow/steps/type/archive.mdx`:
- Around line 49-50: Update the format field description near the archive step
documentation to state that format inference falls back to the source extension
when it cannot be inferred from destination. Preserve the existing supported
formats, destination-extension mappings, and unsupported tar.bz2/tar.xz
limitation.
- Line 10: Update the opening description of the archive step type to mention
tgz alongside zip and tar as a supported archive format, keeping the existing
implementation and platform details unchanged.
🪄 Autofix (Beta)
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
Run ID: bba68c8b-9612-4d46-a05a-560e29ce1b5e
📒 Files selected for processing (20)
docs/prd/archive-step.mddocs/prd/custom-hooks.mderrors/errors.gopkg/archive/archive.gopkg/archive/archive_test.gopkg/archive/format.gopkg/archive/tar.gopkg/archive/walk.gopkg/archive/zip.gopkg/hooks/step_engine_test.gopkg/runner/step/archive.gopkg/runner/step/archive_test.gopkg/schema/task.gopkg/schema/workflow.gowebsite/blog/2026-07-11-archive-step-type.mdxwebsite/docs/stacks/hooks.mdxwebsite/docs/workflows/_partials/_step-types.mdxwebsite/docs/workflows/workflows/workflow/steps/type.mdxwebsite/docs/workflows/workflows/workflow/steps/type/archive.mdxwebsite/src/data/roadmap.js
Fixes several correctness/security issues from PR review: writeZip/writeTar now write through a temp file and rename atomically, so a failure partway through (e.g. a source file vanishing mid-run) never truncates or corrupts the previous archive; action: update no longer downgrades an existing archive's file permissions to the temp file's default mode; a single-file source now respects include/exclude filters instead of always being archived; Run(action, nil) returns a typed validation error instead of panicking; invalid glob-pattern errors from directory walking are no longer misclassified as a missing source; and subpath is validated to reject absolute paths and "."/".." segments, closing a path-traversal vector where an extracted archive could write outside its target directory. Also fixes two doc gaps: the step-types summary table and the archive step's intro paragraph both undersold supported formats as "zip/tar" when tgz is also supported. Three review comments proposing a destination-then-source format-inference fallback were not applied — the documented contract in docs/prd/archive-step.md is destination-only for pack actions and source-only for extract, not a fallback chain, so the "bug" was based on a misreading of ambiguous PR description wording, not an actual gap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TestSchemaCoversWorkflowStepFields was failing in CI: the archive step's format/destination/subpath/include/exclude fields were missing from the published workflow_step schema, which would reject valid workflow files annotated with the $schema. action/source were already covered since the archive step reuses those existing fields. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json (1)
2403-2406: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
actionfield description doesn't mention archive verbs.The
actionfield is reused by the archive step type forcreate,extract,update, andreplace(perpkg/runner/step/archive.go:39-48), but the description only covers container verbs (build,push,run,inspect). This could confuse users authoringtype: archivesteps.📝 Updated description
"action": { "type": "string", - "description": "Container verb for the container step type: build, push, run, or inspect." + "description": "Container verb for the container step type (build, push, run, inspect), or archive verb for the archive step type (create, extract, update, replace)." },🤖 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/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json` around lines 2403 - 2406, Update the action field description in the atmos manifest schema to document archive verbs create, extract, update, and replace in addition to the existing container verbs build, push, run, and inspect.
🧹 Nitpick comments (1)
website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json (1)
2436-2461: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSchema fields match the Go contract — consider constraining
formatwith an enum.The five new properties (
format,destination,subpath,include,exclude) correctly mirrorpkg/schema/workflow.go:375-382and map toarchive.PackOptionsfields consumed inpkg/archive/archive.go:25-99. Types and descriptions are accurate.The
formatfield is a free-formstringwhose description lists valid values (zip,tar,tgz,tar.bz2,tar.xz). Adding anenumwould let schema validators catch typos before runtime, matching the pattern already used forkubernetes_provider(line 506) andbackend_type(line 1281).♻️ Suggested enum for `format`
"format": { "type": "string", + "enum": ["zip", "tar", "tgz", "tar.bz2", "tar.xz"], "description": "Archive format for the archive step type: zip, tar, tgz, tar.bz2, or tar.xz. Inferred from destination's extension when omitted." },🤖 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/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json` around lines 2436 - 2461, Constrain the archive step’s format property to the documented values by adding an enum containing zip, tar, tgz, tar.bz2, and tar.xz, while preserving its existing string type and description.
🤖 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.
Outside diff comments:
In `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 2403-2406: Update the action field description in the atmos
manifest schema to document archive verbs create, extract, update, and replace
in addition to the existing container verbs build, push, run, and inspect.
---
Nitpick comments:
In `@website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json`:
- Around line 2436-2461: Constrain the archive step’s format property to the
documented values by adding an enum containing zip, tar, tgz, tar.bz2, and
tar.xz, while preserving its existing string type and description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 290604ef-2a89-4058-a074-0170a4d3703e
📒 Files selected for processing (10)
errors/errors.gopkg/archive/archive.gopkg/archive/archive_test.gopkg/archive/tar.gopkg/archive/walk.gopkg/archive/zip.gowebsite/docs/workflows/_partials/_step-types.mdxwebsite/docs/workflows/workflows/workflow/steps/type.mdxwebsite/docs/workflows/workflows/workflow/steps/type/archive.mdxwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
✅ Files skipped from review due to trivial changes (3)
- website/docs/workflows/_partials/_step-types.mdx
- website/docs/workflows/workflows/workflow/steps/type.mdx
- website/docs/workflows/workflows/workflow/steps/type/archive.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/archive/tar.go
- errors/errors.go
- pkg/archive/zip.go
- pkg/archive/archive.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2730 +/- ##
==========================================
+ Coverage 81.60% 81.65% +0.05%
==========================================
Files 1641 1648 +7
Lines 154902 155530 +628
==========================================
+ Hits 126408 127000 +592
- Misses 21564 21571 +7
- Partials 6930 6959 +29
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Codecov flagged the archive-step PR's patch coverage at 74.4% (target 85%). Adds targeted tests for previously-uncovered branches: format inference across all tar.bz2/tar.xz/tar extension aliases, Run() with an unknown action, update()'s own validation/MkdirAll error paths, destinationMode and atomicRewrite's direct error paths (stat failures, CreateTemp failure, a failing write callback, a rename-over-non-empty-directory failure), missing source entries fed straight into updateZip/updateTar, corrupt existing archives on update, single-file sources with an invalid glob pattern, a generic (non-glob) directory-walk failure, and every template-resolution error path in the archive step handler (source/destination/format/subpath/ include/exclude) plus a combined include+exclude success case. Raises pkg/archive from 83.7% to 93.0% and pkg/runner/step/archive.go to 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/archive/archive_test.go (1)
568-593: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting the specific error type for MkdirAll failures.
TestRun_Replace_MkdirAllFailureandTestRun_Update_MkdirAllFailureonly assertrequire.Error(t, err), whereas the siblingTestAtomicRewrite_DestinationModeError(line 623) exercises the same "destination nested under a regular file" scenario and assertserrors.Is(err, errUtils.ErrArchiveWriteFailed). Tightening these two tests to check the same sentinel would catch regressions where the error type changes silently.♻️ Proposed tightening
func TestRun_Replace_MkdirAllFailure(t *testing.T) { dir := t.TempDir() src := filepath.Join(dir, "src") writeFixture(t, src) err := Run(ActionReplace, &PackOptions{Source: src, Destination: notADirDestination(t, dir)}) require.Error(t, err) + assert.True(t, errors.Is(err, errUtils.ErrArchiveWriteFailed)) } func TestRun_Update_MkdirAllFailure(t *testing.T) { dir := t.TempDir() src := filepath.Join(dir, "src") writeFixture(t, src) err := Run(ActionUpdate, &PackOptions{Source: src, Destination: notADirDestination(t, dir)}) require.Error(t, err) + assert.True(t, errors.Is(err, errUtils.ErrArchiveWriteFailed)) }Please confirm
replace()/update()actually surfaceErrArchiveWriteFailedfor this failure path before applying (not fully visible in the provided 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/archive/archive_test.go` around lines 568 - 593, Update TestRun_Replace_MkdirAllFailure and TestRun_Update_MkdirAllFailure to assert that the returned error matches errUtils.ErrArchiveWriteFailed using errors.Is, while retaining the existing error assertion if appropriate. First verify that replace() and update() propagate this sentinel for the notADirDestination MkdirAll failure path, and adjust only the error propagation if necessary to preserve that contract.
🤖 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.
Nitpick comments:
In `@pkg/archive/archive_test.go`:
- Around line 568-593: Update TestRun_Replace_MkdirAllFailure and
TestRun_Update_MkdirAllFailure to assert that the returned error matches
errUtils.ErrArchiveWriteFailed using errors.Is, while retaining the existing
error assertion if appropriate. First verify that replace() and update()
propagate this sentinel for the notADirDestination MkdirAll failure path, and
adjust only the error propagation if necessary to preserve that contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85393001-f737-4c6d-b048-2a2b9e44d49b
📒 Files selected for processing (4)
errors/errors.gopkg/archive/archive_test.gopkg/runner/step/archive_test.gowebsite/src/data/roadmap.js
🚧 Files skipped from review as they are similar to previous changes (2)
- website/src/data/roadmap.js
- errors/errors.go
Shell/stdlib zip and tar both bake in real file mtime and permission bits, so identical source content produces different archive bytes on every rebuild — the same non-determinism long reported against Terraform's archive_file provider. Add reproducible: epoch|git, backed by go-git commit history (no shelling out to git log), plus permission-bit normalization. Opt-in and off by default so existing workflows are unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/archive/tar.go (1)
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tgz to the reproducibility byte-identical test.
The comment claims tgz output needs no extra reproducibility handling, but
TestRun_Replace_Reproducible_ByteIdenticalRegardlessOfSourceMetadataonly testszipandtar. Adding{"tgz", ".tgz"}would verify the gzip header (timestamp, OS field) doesn't break determinism. As per coding guidelines, every new feature must include comprehensive unit tests targeting >80% code coverage.🤖 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/archive/tar.go` around lines 14 - 20, Extend TestRun_Replace_Reproducible_ByteIdenticalRegardlessOfSourceMetadata to include the tgz format with its .tgz extension alongside zip and tar. Ensure the test compares tgz outputs byte-for-byte across differing source metadata, covering gzip-header determinism and the reproducibility behavior described by the writeTar documentation.Source: Coding guidelines
pkg/archive/reproducible_test.go (1)
61-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test for source = repo root.
No test covers
newReproducibleTimestampswithsourceset to the repository root. This is the edge case wherefilepath.Relreturns".", which currently causeslastCommitForPrefixto fail silently and fall back to 1980-01-01. As per coding guidelines, every new feature must include comprehensive unit tests targeting >80% code coverage.🧪 Proposed test
func TestNewReproducibleTimestamps_EpochMode_SourceIsRepoRoot(t *testing.T) { root, _, _, t3 := gitFixture(t) rt := newReproducibleTimestamps(ReproducibleEpoch, root) // When source is the repo root, the epoch should be the latest commit // in the repo (t3, "add readme"), not the fallback 1980-01-01. assert.Equal(t, t3, rt.epoch) assert.Equal(t, t3, rt.modTimeFor(filepath.Join(root, "src", "a.txt"))) }🤖 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/archive/reproducible_test.go` around lines 61 - 73, Add a unit test covering newReproducibleTimestamps with source set to the repository root, verifying the epoch and a file timestamp use the repository’s latest commit rather than the fallback timestamp. Create the test alongside TestNewReproducibleTimestamps_EpochMode_UsesLastCommitTouchingSubtree and reuse gitFixture and the existing timestamp assertions.Source: Coding guidelines
pkg/runner/step/archive.go (1)
90-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated resolve-and-wrap pattern.
source,destination,format,subpath,include,exclude, and nowreproducibleeach repeat the samevars.Resolve(...)+if err != nil { return ..., fmt.Errorf("step '%s': failed to resolve %s: %w", ...) }shape. Withreproducibleas the sixth near-identical block, a small helper would cut the boilerplate without changing behavior.♻️ Sketch of a helper to reduce duplication
func resolveStepField(stepName, fieldName, value string, vars *Variables) (string, error) { resolved, err := vars.Resolve(value) if err != nil { return "", fmt.Errorf("step '%s': failed to resolve %s: %w", stepName, fieldName, err) } return resolved, nil }Each call site collapses to
source, err := resolveStepField(step.Name, "source", source, vars), etc.🤖 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/archive.go` around lines 90 - 139, Extract the repeated variable-resolution and error-wrapping logic from resolveArchiveOptions into a helper such as resolveStepField, accepting the step name, field name, value, and Variables. Use it for source, destination, format, subpath, and reproducible, while preserving the existing resolveArchiveGlobs handling for include and exclude and all current error messages.
🤖 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/archive/reproducible.go`:
- Around line 206-225: Update lastCommitForPrefix to treat relPrefix "." as the
repository-root prefix: make its PathFilter match every Git path while
preserving the existing exact and nested-prefix matching for non-root prefixes.
Ensure root-source calls return the latest commit instead of io.EOF and
triggering reproducibleFallbackEpoch.
In `@pkg/runner/step/archive.go`:
- Around line 57-63: The reproducible-mode check in Validate currently validates
the raw templated value instead of the resolved option. Move this validation to
the resolved archive options used by resolveArchiveOptions and archive.Run, or
explicitly enforce literal-only values consistently; preserve acceptance of
templates resolving to epoch or git.
In `@website/blog/2026-07-11-archive-step-type.mdx`:
- Around line 114-143: Add a caveat to the “Reproducible Output” section
clarifying that reproducible timestamp and permission normalization applies only
to the archive’s replace path. State that update-mode copied-forward entries
retain the mtime and mode from their prior write, so update archives are not
fully normalized.
---
Nitpick comments:
In `@pkg/archive/reproducible_test.go`:
- Around line 61-73: Add a unit test covering newReproducibleTimestamps with
source set to the repository root, verifying the epoch and a file timestamp use
the repository’s latest commit rather than the fallback timestamp. Create the
test alongside
TestNewReproducibleTimestamps_EpochMode_UsesLastCommitTouchingSubtree and reuse
gitFixture and the existing timestamp assertions.
In `@pkg/archive/tar.go`:
- Around line 14-20: Extend
TestRun_Replace_Reproducible_ByteIdenticalRegardlessOfSourceMetadata to include
the tgz format with its .tgz extension alongside zip and tar. Ensure the test
compares tgz outputs byte-for-byte across differing source metadata, covering
gzip-header determinism and the reproducibility behavior described by the
writeTar documentation.
In `@pkg/runner/step/archive.go`:
- Around line 90-139: Extract the repeated variable-resolution and
error-wrapping logic from resolveArchiveOptions into a helper such as
resolveStepField, accepting the step name, field name, value, and Variables. Use
it for source, destination, format, subpath, and reproducible, while preserving
the existing resolveArchiveGlobs handling for include and exclude and all
current error messages.
🪄 Autofix (Beta)
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
Run ID: bd88d7a7-0b56-4ad9-928e-3a024afbb868
📒 Files selected for processing (15)
docs/prd/archive-step.mderrors/errors.gopkg/archive/archive.gopkg/archive/archive_test.gopkg/archive/reproducible.gopkg/archive/reproducible_test.gopkg/archive/tar.gopkg/archive/zip.gopkg/runner/step/archive.gopkg/runner/step/archive_test.gopkg/schema/task.gopkg/schema/workflow.gowebsite/blog/2026-07-11-archive-step-type.mdxwebsite/docs/workflows/workflows/workflow/steps/type/archive.mdxwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
🚧 Files skipped from review as they are similar to previous changes (5)
- errors/errors.go
- website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
- pkg/schema/task.go
- pkg/schema/workflow.go
- pkg/archive/zip.go
- lastCommitForPrefix silently fell back to the fixed epoch when source was the repository root (filepath.Rel returns "." there, which never matches a real git path); match every path in that case. - Validate rejected a templated reproducible field before resolution, so a template resolving to a valid mode still failed early. Move the check to after vars.Resolve, in resolveArchiveOptions. - Note in the blog post that reproducible only normalizes the replace path, matching the caveat already in the step reference doc. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TestDestinationMode's "stat error other than not-exist" case and TestCopyUnchangedTarEntries_OpenFailure both fail on Windows: they use a file-as-directory-component trick to force a stat/open error, but Windows reports that as ERROR_PATH_NOT_FOUND (which os.IsNotExist treats as true), not POSIX's distinct ENOTDIR — so the code under test correctly treats it as "doesn't exist" and returns no error. Skip on Windows, matching the existing TestAtomicRewrite_RenameFailure precedent for the same class of platform difference. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/runner/step/archive.go (1)
57-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment references non-existent function name
validateResolvedReproducible.The comment says "See validateResolvedReproducible" but the actual function is
resolveArchiveReproducible(line 141). Update the reference to match.📝 Proposed fix for comment reference
// reproducible is not validated here: it supports Go templates and // resolveArchiveOptions resolves it before archive.Run sees it, so // checking the raw (possibly templated) value here would reject a - // template that resolves to a valid mode. See validateResolvedReproducible. + // template that resolves to a valid mode. See resolveArchiveReproducible.🤖 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/archive.go` around lines 57 - 60, Update the comment near the reproducible validation note to reference the existing resolveArchiveReproducible function instead of the non-existent validateResolvedReproducible name; leave the surrounding explanation unchanged.
🤖 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/runner/step/archive_test.go`:
- Around line 280-302: Update TestArchiveHandler_Execute_Reproducible to
explicitly call os.Chmod on handler.js after os.WriteFile, setting its
permissions to 0o664 and checking the error. Keep the existing archive assertion
unchanged so it verifies normalization from 0o664 to 0o644.
---
Nitpick comments:
In `@pkg/runner/step/archive.go`:
- Around line 57-60: Update the comment near the reproducible validation note to
reference the existing resolveArchiveReproducible function instead of the
non-existent validateResolvedReproducible name; leave the surrounding
explanation unchanged.
🪄 Autofix (Beta)
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
Run ID: 6ac7a388-00e0-45de-bb12-abeab9fb3159
📒 Files selected for processing (15)
docs/prd/archive-step.mderrors/errors.gopkg/archive/archive.gopkg/archive/archive_test.gopkg/archive/reproducible.gopkg/archive/reproducible_test.gopkg/archive/tar.gopkg/archive/zip.gopkg/runner/step/archive.gopkg/runner/step/archive_test.gopkg/schema/task.gopkg/schema/workflow.gowebsite/blog/2026-07-11-archive-step-type.mdxwebsite/docs/workflows/workflows/workflow/steps/type/archive.mdxwebsite/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/archive/tar.go
- pkg/archive/archive.go
- pkg/schema/workflow.go
- website/static/schemas/atmos/atmos-manifest/1.0/atmos-manifest.json
- pkg/schema/task.go
- errors/errors.go
- pkg/archive/reproducible.go
- pkg/archive/zip.go
- pkg/archive/archive_test.go
Renames the archive step's opt-in reproducible: epoch|git field to mtime: filesystem|epoch|git, naming it after the mechanism it controls (the per-entry timestamp stamped into the archive) rather than an opinionated outcome word, and making the default an explicit value instead of an implicit empty string. Also tightens the PRD and blog post: the archive_file dependency-ordering claim now cites the actual upstream Terraform issues and explains the missing-reference root cause instead of reading as an unqualified strawman, and both docs now state explicitly that action: replace is idempotent while action: update is not, rather than implying it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- pkg/cache: bump the exclusive-lock retry budget from 500ms to 2s (filelock_unix.go). TestFileCache_ConcurrentAccess was intermittently failing on Linux CI with "cache file is locked by another process" - 10 goroutines contending for the cache directory's single lock file could exceed the old 50-retry/10ms budget under normal CI load. Ran the test 50x locally with -race after the fix with no failures. - CI workflow: bump the Windows/macOS "Acceptance tests" step timeout from 45m to 60m. Compared successful vs. failed run timings for this step: 37m43s vs. 45m06s (timed out) - only ~7 minutes of headroom on a step with that much run-to-run variance. 60m matches the Linux coverage step's existing budget. - docs/prd/archive-step.md, website/blog: drop the "still-open" framing on the cited archive_file GitHub issues - verified via `gh issue view` that all three (hashicorp/terraform#30042, terraform-provider-archive #218 and #34) are actually closed. Reworded to cite them as documented evidence of the failure mode without asserting current issue-tracker status. - website/docs archive.mdx: scope the byte-identical-output claim to mtime: epoch/git only - mtime: filesystem preserves real source mtime and permission bits and is not deterministic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
… meaningful os.WriteFile's requested mode is subject to the process umask, so under the common 0o022 umask the source file already landed at 0o644 on disk - the same value the test asserts on the archived entry. That made the assertion pass trivially without proving normalization actually ran. os.Chmod bypasses umask, guaranteeing the source is genuinely 0o664 before archiving. Addresses a CodeRabbit finding on PR #2730; verified against current code that the other four open findings on that review pass no longer apply (format inference from source is by design reserved for the not-yet-implemented extract action per the PRD, and the tar.go permission-stripping concern is already handled by atomicRewrite's destinationMode() chmod). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (Cloud Posse) (@osterman)? 🙏 |
…type-prd # Conflicts: # .github/workflows/test.yml
Contributor
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 `@errors/errors.go`:
- Line 845: In the error sentinel declarations near ErrLayerExtraction, restore
the deprecated ErrTarballExtraction alias by assigning it to ErrLayerExtraction.
Add a deprecation comment documenting that callers should use
ErrLayerExtraction, preserving the existing sentinel identity for external
users.
🪄 Autofix (Beta)
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
Run ID: dc9e34ca-6ac9-4aa9-b61f-3a45bc3a1656
📒 Files selected for processing (2)
errors/errors.gowebsite/src/data/roadmap.js
🚧 Files skipped from review as they are similar to previous changes (1)
- website/src/data/roadmap.js
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@errors/errors.go`:
- Line 845: In the error sentinel declarations near ErrLayerExtraction, restore
the deprecated ErrTarballExtraction alias by assigning it to ErrLayerExtraction.
Add a deprecation comment documenting that callers should use
ErrLayerExtraction, preserving the existing sentinel identity for external
users.
🪄 Autofix (Beta)
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
Run ID: dc9e34ca-6ac9-4aa9-b61f-3a45bc3a1656
📒 Files selected for processing (2)
errors/errors.gowebsite/src/data/roadmap.js
🚧 Files skipped from review as they are similar to previous changes (1)
- website/src/data/roadmap.js
🛑 Comments failed to post (1)
errors/errors.go (1)
845-845: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n '\bErrTarballExtraction\b|\bErrLayerExtraction\b' --glob '*.go'Repository: cloudposse/atmos
Length of output: 154
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the relevant section of errors/errors.go. file="errors/errors.go" wc -l "$file" sed -n '830,860p' "$file" # Search for the old and new sentinel names across Go files. rg -n '\bErrTarballExtraction\b|\bErrLayerExtraction\b' . --glob '*.go' || trueRepository: cloudposse/atmos
Length of output: 2068
Preserve the deprecated alias for the old sentinel. Keep
ErrTarballExtraction = ErrLayerExtractionwith a deprecation comment so external callers don’t break.🤖 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 `@errors/errors.go` at line 845, In the error sentinel declarations near ErrLayerExtraction, restore the deprecated ErrTarballExtraction alias by assigning it to ErrLayerExtraction. Add a deprecation comment documenting that callers should use ErrLayerExtraction, preserving the existing sentinel identity for external users.
Andriy Knysh (aknysh)
approved these changes
Jul 14, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
Warning Release Documentation RequiredThis PR is labeled
|
|
These changes were released in v1.223.0-rc.11. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
type: archivestep for workflows and custom commands that packs a directory or file into azip,tar, ortgzarchive using the Go standard library only (archive/zip,archive/tar,compress/gzip) — no externalzip/tarbinary required.action: replace(always rebuild fresh) on every format, andaction: update(incremental add/refresh) forzipand uncompressedtar;action: create/action: extractandtar.bz2/tar.xzwriters are reserved in the schema for a later phase and return a typed "not yet implemented" error.subpathnesting,include/excludeglob filtering (reusing the existingpkg/utils.PathMatchmatcher), and format inference from the destination/source extension.kind: stephook bridge instead of adding a dedicated hook kind, sokind: step+type: archiveworks as a component lifecycle hook with zero hook-side code.docs/prd/archive-step.mdPRD, step-type reference docs, a changelog blog post, and a roadmap milestone.why
handler.zipbeforeterraform plan/apply) had no native Atmos primitive — the only option was shelling out tozip/tarvia akind: commandhook ortype: shellstep, which breaks on Windows CI and gives no typed validation.before_hookwrapping thezipbinary; adata "archive_file"Terraform data source was tried first but doesn't reliably run before packaging is needed.emulator/http/container, which reach hooks purely through thekind: stepbridge — this avoids duplicating schema and hook-engine code for a capability the step registry already provides.references
docs/prd/archive-step.md