Repository navigation
Use self-hosted github runners - #269
Closed
Max Lobur (max-lobur) wants to merge 1 commit into
Closed
Max Lobur (max-lobur) wants to merge 1 commit into
Max Lobur (max-lobur) wants to merge 1 commit into
Conversation
Max Lobur (max-lobur)
requested review from
Andy Roth (RothAndrew) and
Cody Moore (dotCipher)
December 1, 2022 16:29
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
Bumps pnpm overrides for two transitive website dependencies to their patched versions: - js-yaml 3.15.0 -> 3.15.1, 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, quadratic CPU consumption in !!omap resolution, high severity, alerts #268/#269) - mermaid 11.16.0 -> 11.16.1 (GHSA-rhh3-jpg6-66xh, GHSA-c4c3-pg64-4m4v, GHSA-6x64-9x62-f2gx, GHSA-3rrr-jr9j-h3q3, GHSA-2v8p-3f2j-5mp7, alerts #263-#267) Both are within-major-version patches (not blocked by dependabot.yml's major-bump ignore policy). No CodeQL alerts were open. Verified with a full `pnpm run build` in website/ — succeeds, no new broken links. NOTICE is unchanged (no license-set change from these bumps). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
Bump pnpm.overrides for transitively-pulled packages to their patched versions, all within the semver-major bump the dependabot.yml ignore policy blocks: - js-yaml 3.15.0 -> 3.15.1, 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, GHSA for the 4.x line): quadratic CPU consumption in !!omap resolution, fixes #269, #268. - mermaid 11.16.0 -> 11.16.1: fixes #267 (radar diagram DoS), #266 (config API prototype pollution), #265 (CSS injection), #264 (Architecture diagram prototype pollution), #263 (XY Chart infinite-loop DoS). No open CodeQL alerts. Verified with `atmos lint --changed` (0 issues) and `npm run build` in website/ (succeeds, same pre-existing unrelated broken-anchor warning as before this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
Bumps js-yaml (3.15.0->3.15.1, 4.3.0->4.3.1) and mermaid (11.16.0->11.16.1) pnpm overrides to patched versions. All are patch-level bumps within the allowed (non-major) range per .github/dependabot.yml's ignore policy. Fixes GHSA-5p4m-2wfm-xmqj (js-yaml quadratic CPU in !!omap resolution, high severity, alerts #268/#269) and GHSA-rhh3-jpg6-66xh/GHSA-c4c3-pg64-4m4v/ GHSA-6x64-9x62-f2gx/GHSA-3rrr-jr9j-h3q3/GHSA-2v8p-3f2j-5mp7 (mermaid, medium/low severity, alerts #263-#267). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
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>
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
Bumps transitive js-yaml (>=3.15.1, >=4.3.1) and mermaid (>=11.16.1) via pnpm.overrides to their patched versions - quadratic-CPU YAML parsing and mermaid XSS/ReDoS advisories. All fixes are minor/patch bumps within the same major version, so none are blocked by dependabot.yml's major-version ignore policy. Fixes GHSA-5p4m-2wfm-xmqj (js-yaml, alerts #268/#269), GHSA-rhh3-jpg6-66xh, GHSA-c4c3-pg64-4m4v, GHSA-6x64-9x62-f2gx, GHSA-3rrr-jr9j-h3q3, GHSA-2v8p-3f2j-5mp7 (mermaid, alerts #263-267). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
Bump the existing pnpm.overrides pins for js-yaml and mermaid to their patched versions — all within-major patch/minor bumps, none blocked by dependabot.yml's major-version ignore policy: - js-yaml@^3 -> ^3.15.1 (GHSA-5p4m-2wfm-xmqj, alert #269) - js-yaml@^4 -> ^4.3.1 (GHSA-5p4m-2wfm-xmqj, alert #268) - mermaid@^11 -> ^11.16.1 (alerts #263-267: GHSA-2v8p-3f2j-5mp7, GHSA-3rrr-jr9j-h3q3, GHSA-6x64-9x62-f2gx, GHSA-c4c3-pg64-4m4v, GHSA-rhh3-jpg6-66xh) Verified with `pnpm install --lockfile-only` (patched versions land) and a full `website` build (succeeds, no new warnings). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
Bump the pnpm overrides pinning js-yaml and mermaid to their patched versions -- all within the same major version, so no dependabot.yml ignore-policy exception is needed: - js-yaml@^3: 3.15.0 -> 3.15.1 (GHSA-5p4m-2wfm-xmqj, alert #269) - js-yaml@^4: 4.2.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, alert #268) - mermaid@^11: 11.15.0 -> 11.16.1 (GHSA-rhh3-jpg6-66xh #267, GHSA-c4c3-pg64-4m4v #266, GHSA-6x64-9x62-f2gx #265, GHSA-3rrr-jr9j-h3q3 #264, GHSA-2v8p-3f2j-5mp7 #263) Verified: pnpm install regenerates the lockfile at the patched versions, `npm run build` succeeds, and NOTICE is unchanged (no license drift). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
…onfig indentation Security (Dependabot #263-#269, all transitive npm deps in website/, pinned via pnpm.overrides, none requiring a major bump so none blocked by dependabot.yml's semver-major ignore policy): - js-yaml 3.15.0 -> 3.15.1, 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, high): quadratic CPU consumption in !!omap resolution - mermaid 11.16.0 -> 11.16.1 (5 alerts) No open CodeQL alerts at time of remediation. Verified with a full `pnpm run build` in website/ -- succeeds with only pre-existing, unrelated warnings. NOTICE is unaffected (tracks Go module dependencies only). EditorConfig (CI failure fix): CI's "Validation (affected)" and "Run pre-commit hooks" jobs validate every file touched on the branch in full, not just the changed lines. Editing docs/prd/atmos-profiles.md and docs/prd/base-path-resolution-semantics.md in an earlier commit surfaced 150 pre-existing "Wrong amount of left-padding spaces (want multiple of 2)" violations elsewhere in those files -- long-standing 3/5/7-space list/pseudocode indentation never previously caught because neither file had been touched by a PR before. Normalized every odd-indented line to the next even width (add 1 space), preserving nesting structure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
Bump pnpm overrides to patched versions, all within the major-version ignore policy in .github/dependabot.yml: - js-yaml 3.15.0 -> 3.15.1, 4.3.0 -> 4.3.1: quadratic CPU consumption in !!omap resolution (GHSA-5p4m-2wfm-xmqj / GHSA advisories, alerts #269, #268, high severity) - mermaid 11.16.0 -> 11.16.1: radar-diagram DoS, prototype pollution (config APIs and Architecture diagrams), CSS injection, XY-chart infinite-loop DoS (alerts #267, #266, #265, #264, #263, medium/low) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added a commit
that referenced
this pull request
Aug 7, 2026
Bumps three pnpm.overrides pins to their patched releases, all within the major-version line dependabot.yml's ignore policy allows: - js-yaml 3.15.0 -> 3.15.1 (GHSA-5p4m-2wfm-xmqj, alert #269) - js-yaml 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, alert #268) - mermaid 11.16.0 -> 11.16.1 (GHSA-rhh3-jpg6-66xh #267, GHSA-c4c3-pg64-4m4v #266, GHSA-6x64-9x62-f2gx #265, GHSA-3rrr-jr9j-h3q3 #264, GHSA-2v8p-3f2j-5mp7 #263) Verified via `pnpm run build` in website/; NOTICE unchanged (no license changes from these patch bumps). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Andriy Knysh (aknysh)
added a commit
that referenced
this pull request
Aug 8, 2026
* feat(ai): add browsable /ai/skills directory and skill categories Adds a searchable, category-grouped /ai/skills page (reusing the existing file-browser plugin behind /examples and /gists) with a per-skill detail page for each SKILL.md, replacing the hand-maintained "Available Skills" list in the docs that had drifted to half the real count (25 vs 52). Adds `metadata.category` to every bundled SKILL.md, threads it through `pkg/ai/skills/marketplace`, and exposes it via a new `--format` flag (table/json/yaml/csv/tsv) on `atmos ai skill list`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(ai): polish agent skills directory nav, homepage count, and changelog Adds the changelog post and roadmap entry for the browsable Agent Skills Directory shipped in the prior commit, plus follow-up polish: a SkillCount component that renders the live skill count at build time (used on the homepage AI section instead of a hardcoded number), file-browser plugin card icon/CTA options, sidebar nav restructuring under Atmos AI, and doc updates for the new `--format` flag. Also fixes pre-existing EditorConfig indentation violations (3-space markdown list continuations) in several SKILL.md files, surfaced by the affected-file validator once those files were touched by this branch's earlier commit. * feat(ai): add copy-as-markdown button to skill pages Adds a "Copy as Markdown" button to a skill's root page that concatenates its SKILL.md and every nested reference file into one clipboard-ready Markdown document, so the full context can be grabbed without installing the skill. Ships as an opt-in `enableCopyMarkdown` file-browser plugin option (enabled for the /ai/skills instance only; /examples and /gists render unchanged). * fix: address CodeRabbit review feedback on skills directory browser PR Fixes six issues flagged on PR #2881: - cmd/ai/skill/list.go: dispatch structured --format output (json/yaml/csv/tsv) before the "No skills installed" empty-message check, so --installed with zero results stays machine-readable instead of returning prose. - cmd/ai/skill/list.go: validate the Viper-resolved --format value (covers ATMOS_AI_SKILL_FORMAT env/config, not just the CLI flag) before it reaches the renderer. - pkg/ai/skills/marketplace/catalog_test.go: add the missing Category field to the AvailableSkill compile-time sentinel so a future field rename or drop is caught at compile time. - cmd/ai/skill/markdown/atmos_ai_skill_list_usage.md: fix MD040/MD014 lint on the new --format=json example (shell fence, no unshown $ prompt). - agent-skills/skills/atmos-templates/SKILL.md: add missing trailing period. - website/src/components/FileBrowser/styles.module.css: move the viewport max-height constraint from .sidebar onto .sidebarInner so a long file tree scrolls inside the sidebar instead of growing it (shared by /examples, /gists, and /ai/skills). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(ai): use natural link text for the Agent Skills Directory Two spots linked to /ai/skills using the raw path as the visible link text ("browsable at /ai/skills") instead of natural language, which read poorly next to the "Agent Skills Directory" phrasing used everywhere else this page is linked. Match the established convention. * feat(ai): per-page skill markdown, code-styled titles, fix AWS compliance category - file-browser plugin: new enablePerPageMarkdown option writes a raw <name>.md per item at build time (SKILL.md + nested reference files concatenated), extending the sitewide "append .md for raw Markdown" convention to skill pages, which docusaurus-plugin-llms-txt can't see since they're custom routes, not docs/blog content. - new titleAsCode option renders each item's title as a code-formatted `/name` on the index cards and sidebar header, signaling how a skill is invoked. Enabled for the skills instance only. - atmos-aws-compliance: recategorize from "security" to "aws" so it groups with the other AWS integrations instead of Auth/Secrets. * feat(ai): hover copy button on skill cards, drop code-title background - IndexPage cards get a "Copy as Markdown" icon button in the corner, revealed on hover/focus, so a skill's full context (including nested reference files) can be copied straight from the grid without opening it. Gated by enableCopyMarkdown, so /examples and /gists are unaffected. CopyMarkdownButton gained an iconOnly mode. - Code-formatted titles (titleAsCode) now drop Infima's default inline-code background chip/border, keeping just the monospace font. * fix(ai): match card copy-button style to the site's pill buttons The icon-only copy button used a solid circular background with a border and drop shadow, inconsistent with the translucent rounded- rect pill language used everywhere else (.copyMarkdownButton, .githubButton, .filterButton). Same rgba background/hover, 6px radius, no shadow. * docs(ci): note that atmos CLI replaces the old github-action-* Actions * fix: JSON empty-array bug in list renderer, backtick-fence collision - pkg/list/renderer: formatJSON built its result with a nil slice var, so json.MarshalIndent emitted "null" instead of "[]" for zero rows. Every --format=json list command was affected, not just skills. Strengthened the regression test (cmd/ai/skill/list_test.go) to require a non-nil slice, which is what caught this. - file-browser plugin + client utils: collectMarkdownContext wrapped non-Markdown file content in a fixed ``` fence. A nested file whose own content contains a ``` run would close the fence early and corrupt the generated/copied Markdown. Both twins now pick a fence longer than any backtick run already in the content. * fix(ai): disable pointer-events on hidden card copy button opacity: 0 doesn't remove an element from hit testing. The invisible button sat in front of the card link (top-right), so an early click or touch there hit the button instead of navigating - its handler stops propagation, silently swallowing the click. * ci: give windows acceptance-test job timeout real headroom A run hit the 75-minute job-level ceiling to the exact second while every individual step, including the full go test suite, had already completed successfully per the logs - the "Acceptance tests" step itself finished in 54.6m, under its own 60m budget, but the job-level timeout left almost no slack once summed with setup/toolchain steps. Same CI-to-CI variance already documented on the step-level timeout below; widen the job-level ceiling to match. * docs(website): reorganize Atmos AI sidebar and consolidate agent skills docs Nest MCP under the Atmos AI category alongside a flat "Agent Skills" link (previously a top-level sibling behind a nested "Skills" category), and consolidate the redundant agent-skills.mdx and skill-marketplace.mdx pages into the atmos ai skill CLI command reference, updating all cross-references and adding redirects for the removed URLs. Also fixes a duplicate "scaffold" sidebar entry: scaffold.mdx now lives inside the scaffold/ folder as usage.mdx with a _category_.json, matching every other multi-subcommand command (auth, pro, validate, ai, toolchain). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(website): correct EditorConfig indentation in scaffold blog post CI's EditorConfig validation requires left-padding in multiples of 2; these list items under a numbered entry used 3 spaces. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(website): collapse multi-line PrimaryCTA/SecondaryCTA to single line MDX wraps a JSX component's text in a <p> when it sits on its own line inside a block-level context, which overrides the button's centered white/bold styling with muted paragraph styling. Single-line usage (the pattern already used on working pages like auth/usage.mdx) keeps the label as plain inline text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(ci): clarify actions/checkout + atmos is only a baseline setup The intro note and workflow overview implied actions/checkout plus an atmos command was universally sufficient. Status checks, check runs, PR comments, OIDC, SBOM uploads, and github/artifacts planfile storage need additional permissions or the github-runtime action, already documented in the Permissions section below - cross-reference it instead of overclaiming. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ai): keep copy confirmation visible for its full duration Repeated clicks scheduled overlapping setTimeout calls to reset the "Copied!" state; an earlier click's timeout could fire and hide the confirmation before the latest click's 2s window elapsed. Track the timeout in a ref, clear it before scheduling a new one, and clean it up on unmount. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(blog): fix inaccurate claim and rewrite agent-skills-directory post The post claimed atmos ai skill list was a hand-maintained doc that drifted out of sync with the real catalog. It never was -- the listing is generated at runtime from the embedded skill catalog. The actual gap was the missing full-content browse/search/copy/fetch experience, which is what the post now leads with. Also rewritten in short, active, single-idea sentences (ASD-STE100 style). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: give the acceptance-test job real timeout headroom on Linux timeout-minutes: 90 matched Linux's timed-step sum exactly (25m registry cache + 60m coverage tests + 5m coverage upload), leaving zero slack for checkout, Go/Atmos setup, and toolchain installs before the job gets force-cancelled. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(blog): use a shell-safe curl example in the skills directory post The unquoted <skill-name> placeholder is parsed as shell redirection syntax, so a reader who copies the command as written fails before curl runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(website): remove invalid font-family quotes in CommandBox Stylelint's font-family-name-quotes rule rejects quotes on single- word font names like Monaco and Menlo; Courier New keeps its quotes since it contains a space. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(website): handle clipboard write failures in CommandBox navigator.clipboard.writeText can reject when the browser denies clipboard access, and the rejection was left unhandled. Catch it and surface a "Copy failed" state instead of letting the click silently do nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(website): keep CommandBox copy result states mutually exclusive Independent copied/failed booleans let a fast retry leave the button showing a checkmark with a "Copy failed" tooltip (or vice versa) until the stale timeout caught up. Replace them with a single CopyStatus value so the two states can't coexist, and extract the clipboard-write outcome into a pure performCopy() function (following the CastPlayer convention: sibling .mjs logic module + node:test .test.mjs) so the success/failure/reset behavior has regression coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 7 Dependabot alerts (js-yaml, mermaid) Bump pnpm overrides to patched versions, all within the major-version ignore policy in .github/dependabot.yml: - js-yaml 3.15.0 -> 3.15.1, 4.3.0 -> 4.3.1: quadratic CPU consumption in !!omap resolution (GHSA-5p4m-2wfm-xmqj / GHSA advisories, alerts #269, #268, high severity) - mermaid 11.16.0 -> 11.16.1: radar-diagram DoS, prototype pollution (config APIs and Architecture diagrams), CSS injection, XY-chart infinite-loop DoS (alerts #267, #266, #265, #264, #263, medium/low) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
Andriy Knysh (aknysh)
pushed a commit
to zack-is-cool/atmos
that referenced
this pull request
Aug 10, 2026
* fix(ai): close DX gaps found in atmos ai field test Fixes 16 findings from a hands-on field test of `atmos ai` commands: skill install/uninstall: - Enforce the compatibility.atmos version gate for bundled and multi-skill Git package installs, not just single-skill Git clones - Reject unrecognized --client values instead of silently no-op'ing (extends pkg/flags WithValidValues to string-slice flags generically) - Warn when --path is combined with --client/--scope/--global, since --path skips auto-distribution and those flags are otherwise ignored - Give the registry-corruption error an actionable hint via the error-builder pattern - Show a skill's minimum required Atmos version in `skill list --detailed` and flag when an installed skill has a newer catalog version available - Fix `agent-skills/skills/atmos-ai/SKILL.md` doc drift (never documented `skill install`) and remove a phantom `info` subcommand from --help - Add local-path/file:// support to skill source parsing and the downloader - Document --scope/--global precedence ask/exec/sessions/MCP: - Make `exec`/`ask --session` actually persist and resume conversations (previously a complete no-op despite being a documented flag) - Resolve a session's Model from the constructed AI client instead of a raw config lookup, fixing sessions export/import for the default zero-config claude-code provider path - Apply --mcp server filtering for CLI providers too (was silently ignored, all configured servers were always passed through) - Reject invalid --format values instead of silently falling back to text Two intentional behavior changes: - `ai exec` can now return exit code 2 for a genuine infrastructure-level tool failure (e.g. an unregistered tool) without waiting on the 25-iteration tool-call loop to exhaust - `sessions clean --older-than 0d` now deletes all sessions immediately, distinguished from the flag not being passed at all (which still defaults to 30 days); negative durations are now a hard parse error instead of silently falling back to the default Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 7 Dependabot alerts in website deps Bumps pnpm overrides for two transitive website dependencies to their patched versions: - js-yaml 3.15.0 -> 3.15.1, 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, quadratic CPU consumption in !!omap resolution, high severity, alerts cloudposse#268/cloudposse#269) - mermaid 11.16.0 -> 11.16.1 (GHSA-rhh3-jpg6-66xh, GHSA-c4c3-pg64-4m4v, GHSA-6x64-9x62-f2gx, GHSA-3rrr-jr9j-h3q3, GHSA-2v8p-3f2j-5mp7, alerts cloudposse#263-cloudposse#267) Both are within-major-version patches (not blocked by dependabot.yml's major-bump ignore policy). No CodeQL alerts were open. Verified with a full `pnpm run build` in website/ — succeeds, no new broken links. NOTICE is unchanged (no license-set change from these bumps). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ai): fix flaky CI assertion in session export warning test TestManager_ExportSession_WarnsOnUnimportableCheckpoint asserted on the raw ui.Warning() output, including ANSI styling. Under CI=true (the real GitHub Actions env, confirmed by reproducing locally with CI=true) the formatter emits the message across two adjacent styled runs, which split the literal substring "not be re-importable" the test was checking for -- the visible text was identical, only the styling boundary differed from a local run. Passed locally, failed in CI. Strip ANSI codes and collapse whitespace before asserting on captured UI output, so the test checks content, not rendering byte-layout. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(ai): add `atmos ai skill update` to refresh outdated bundled skills Closes the gap from finding cloudposse#11 of the field-test fix pass: bundled skills are static copies made at install time, so upgrading the atmos binary alone never refreshed a skill you'd already installed, even when the new release shipped improved skill content. `atmos ai skill list --detailed` already surfaced an "update available" hint; nothing acted on it. `atmos ai skill update [name]` compares each installed bundled skill's recorded version against the catalog embedded in the running binary and reinstalls only the ones that are actually outdated: - With no <name>, updates every installed bundled skill that has a newer version available (a single confirmation, not one per skill). Skills already current are left untouched and reported separately, so it's safe to run repeatedly. - An outdated skill is reinstalled the same way `install <name> --force` would install it, reusing all the same client-distribution/scope flags and logic (--client, --all-clients, --scope, --global, --path). - Skills installed from GitHub aren't covered yet (no cheap way to check their upstream version without a fetch); update returns a clear error pointing at `install <source> --force` as the manual path for those. Both `atmos ai skill list`'s "update available" indicator and the new `update` command now share one `marketplace.SkillVersionOutdated` helper instead of duplicating the comparison, so they can never disagree. Docs, blog post, and roadmap updated; the `atmos ai skill --help` screengrab regenerated (--filter'd to just that one cast). Relabeling this PR patch -> minor since this is new user-visible functionality, which requires both per this repo's release-doc policy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): add fix records for this session's four PR changes Per the fix-log skill: one record each for the 16-finding atmos ai field-test fix pass, the website Dependabot remediation, the flaky CI test-assertion fix, and the new atmos ai skill update command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(flags): validate env-sourced flag values, not just CLI-set ones ValidateFlagValues only checked cmd.Flags().Changed(), so an invalid value supplied purely via a bound environment variable (e.g. ATMOS_AI_SKILL_CLIENT=bogus) silently bypassed validation -- a skill install/update would report success while distributing to zero clients. Now validates whenever Viper reports the value as actually set (CLI or env), not just CLI-changed. Found via a field-test pass on the atmos ai skill update / pkg/flags changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ai): surface batch failures in `atmos ai skill update`/install UpdateAllBundled/InstallAllBundled's batch path logged a warning per failed skill but always returned nil, so a real per-skill failure (e.g. a reinstall that can't remove the existing install) was reported as overall success with exit code 0 -- the only signal was an easily-missed log line, invisible entirely at logs.level: Error. Adds a distinct outcomeRejected for expected compatibility/structure skips (already covered by an existing test), keeping it separate from genuine outcomeFailed so only real failures propagate an error. Found via a field-test pass on the atmos ai skill update changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ai): resolve anonymous chat session Model via client.GetModel() The anonymous-session branch (no --session given) still called getModelFromConfig -- a raw config lookup with no default fallback -- instead of client.GetModel(), unlike the named-session branch fixed in a prior commit. A zero-config claude-code chat with no --session got a blank Model on its session record, breaking re-import (which requires a non-empty Model). Found via a field-test pass on the atmos ai session DX fixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ai): warn when --session is ignored because sessions are disabled prepareSession returned (nil, nil) identically for "no --session given" and "--session given but ai.sessions.enabled is false", so a user could run `exec --session foo` across multiple invocations believing conversation context was being carried over, with nothing persisted and no indication why -- only discoverable indirectly via `sessions list` erroring "sessions are not enabled". Found via a field-test pass on the atmos ai session DX fixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ai): fix concurrent session storage access failing with SQLITE_BUSY Two processes opening the same new session database at once (e.g. concurrent `atmos ai exec --session <same-name>` invocations) could crash with SQLITE_BUSY: PRAGMA journal_mode = WAL briefly needs exclusive access to switch modes, and busy_timeout was set after it in the pragma list, so a connection that lost the race had no busy timeout in effect yet. Reorders busy_timeout first and adds a bounded retry loop around pragma execution, since the pure-Go sqlite driver (modernc.org/sqlite) doesn't consistently honor busy_timeout for that specific one-time mode switch. Found via a field-test pass on the atmos ai session DX fixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 5 Dependabot alerts (go-git, dompurify, nanoid) - github.com/go-git/go-git/v5: 5.19.1 -> 5.19.2, fixing a symlink traversal in worktree operations (GHSA-hc8v-wwc9-vgxm, high) and a path traversal in reference name handling (GHSA-qgq7-7hm3-q39j, medium). Alerts cloudposse#270, cloudposse#271. - dompurify (website, transitive via pnpm override): 3.4.12 -> 3.4.13, fixing an IN_PLACE sanitization XSS via detached subtree hook removal (GHSA-55q2-fjhq-7xh7, medium). Alert cloudposse#272. - nanoid (website, transitive via pnpm override, both the direct override target and postcss's own dependency): 3.3.15/3.3.16 -> 3.3.18, fixing two infinite-loop DoS issues with negative/zero size (GHSA-28wg-ghj8-5hjv, GHSA-2v37-7h3g-55p8, high). Alerts cloudposse#273, cloudposse#274. Not fixed: cloudposse#275/cloudposse#276 (npm image-size <= 2.0.2, high) -- no patched version exists yet upstream (first_patched_version is null on both advisories); nothing to bump to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): fix flaky merge-queue failure in describe-affected base test TestSetDescribeAffectedFlagValueInCliArgs_BaseResolution's "CI auto-detect when enabled and no explicit base" subtest cleared GITHUB_ACTIONS/CI but not GITHUB_EVENT_PATH. When this test suite runs locally or under a regular pull_request CI trigger, no merge_group event file is present, so resolveMergeGroupBase falls back to the simulated GITHUB_BASE_REF the subtest sets, and the test passes. But when this test actually runs inside a real merge_group-triggered workflow (i.e. a PR going through the GitHub merge queue), the runner's real GITHUB_EVENT_PATH points at a genuine merge_group event payload with a real base_sha -- and resolveMergeGroupBase correctly prefers that real payload data over GITHUB_BASE_REF, per its documented fallback order. The subtest never accounted for this, so it fails with an empty Ref field specifically -- and only -- in real merge-queue runs, not locally or in regular PR CI. Reproduced locally by pointing GITHUB_EVENT_PATH at a synthetic merge_group payload; fixed by explicitly clearing GITHUB_EVENT_PATH so the subtest is hermetic regardless of the ambient CI context it happens to execute in. This was pre-existing on main (from PR cloudposse#2395) and surfaced when PR cloudposse#2903 hit the merge queue for the first time; not a regression from this branch's own changes, but fixed here directly per repo convention for CI-blocking issues found on an open PR's branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zack-is-cool
pushed a commit
to zack-is-cool/atmos
that referenced
this pull request
Aug 10, 2026
…udposse#2880) * fix(steps): resolve relative paths against step.WorkingDirectory 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> * docs(fixes): add fix record for step WorkingDirectory bug Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(runner/step): close coverage gaps in ResolveInWorkingDirectory and 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> * fix(steps): anchor container bake/cache paths and preserve workflow error 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> * fix(steps): anchor container bake/cache paths and preserve workflow error context Address CodeRabbit findings on PR cloudposse#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> * test(runner/step): close container_build.go coverage gaps from bake/cache 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> * fix(docs): exclude reproducible-builds.org from link checking (CI connection 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> * feat(hooks): anchor bare-relative working_directory to the component 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> * fix(security): remediate 7 open Dependabot alerts 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 (cloudposse#269, GHSA-5p4m-2wfm-xmqj: quadratic CPU consumption in !!omap resolution, high) - js-yaml 4.3.0 -> 4.3.1 (cloudposse#268, same advisory, 4.x line, high) - mermaid 11.16.0 -> 11.16.1 (cloudposse#267, cloudposse#266, cloudposse#265, cloudposse#264, cloudposse#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> * docs(fixes): fix markdownlint MD018 in working-directory fix record Address CodeRabbit finding on PR cloudposse#2880: a paragraph wrap left "cloudposse#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> * fix(runner/step): close remaining working_directory anchoring gaps 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> * docs(steps): document handler-field and unset-hook working_directory contracts Addresses CodeRabbit feedback on PR cloudposse#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> * fix(runner/step): isolate CI base-resolution test from ambient GITHUB_EVENT_PATH TestSetDescribeAffectedFlagValueInCliArgs_BaseResolution's merge_group subtest only overrode GITHUB_ACTIONS/GITHUB_EVENT_NAME/GITHUB_BASE_REF, so it depended on $GITHUB_EVENT_PATH being unset in the ambient environment. That's true for local/regular CI runs, but not when the test itself runs inside a real GitHub Actions merge_group job (i.e. the merge queue): the real event payload's merge_group.base_sha then takes precedence over the simulated GITHUB_BASE_REF, resolving to a SHA instead of the "refs/remotes/origin/main" ref the test asserts. This was failing the merge queue for this PR and, independently, for cloudposse#2900 and cloudposse#2903 as well -- confirmed pre-existing and unrelated to any of their changes. Fixed by explicitly forcing GITHUB_EVENT_PATH="" so the subtest deterministically exercises the fallback path it's meant to test, regardless of the real job's event context. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 5 of 7 open Dependabot alerts - github.com/go-git/go-git/v5: v5.19.1 -> v5.19.2, fixing GHSA-hc8v-wwc9-vgxm (symlink traversal in worktree operations) and the companion malicious reference-name advisory (cloudposse#270, cloudposse#271). Patch-level bump, no API changes. - website: dompurify pnpm override ^3.4.12 -> ^3.4.13, fixing an IN_PLACE hook removal XSS (cloudposse#272). - website: nanoid pnpm overrides bumped to ^3.3.17 (added a second selector, nanoid@^3.3.16, to also catch postcss's own nanoid range, which the existing nanoid@3.3.3 selector didn't match), fixing two indefinite-loop DoS advisories (cloudposse#273, cloudposse#274). The nanoid 5.x line was already patched by the existing override. - NOTICE regenerated for the go-git/dompurify/nanoid version bumps. Not fixed: image-size (cloudposse#275, cloudposse#276, both DoS via infinite loop) has no patched release yet as of this commit -- GitHub's advisories list "<= 2.0.2" as vulnerable with no first_patched_version. Left as-is pending an upstream fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
zack-is-cool
pushed a commit
to zack-is-cool/atmos
that referenced
this pull request
Aug 10, 2026
…rges (cloudposse#2875) * fix(config): honor --config across internal reloads and multi-file array merges Internal call sites that re-invoke InitCliConfig(schema.ConfigAndStacksInfo{}, false) mid-command no longer silently discard --config/--config-path/--base-path, fixing `atmos --config <file> terraform plan/test` falling back to plain auto-discovery (closes cloudposse#2868). A second --config file with a conflicting array-typed value (e.g. stacks.included_paths) no longer aborts stack discovery for entries that still match, and `atmos config get` now reports the effective, fully-merged configuration instead of a stale single-file value (closes cloudposse#2867). Also precomputes VendorDirAbsolutePath/WorkflowsDirAbsolutePath (same base_path resolution fix as cloudposse#2864) so vendor/workflow path joins don't re-derive a possibly still-relative BasePath. * fix: stop leaking absolute paths in workflow/vendor messages, split CliConfigPath for profiles CI (linux/macos/windows) failed because getWorkflowsDirToUse/getVendorDirToUse made the "Vendoring from" log message and the invalid/missing workflow manifest error messages show a full, environment-length-dependent absolute path instead of the previous cwd-relative one. That broke word-wrapped golden snapshots and literal-pattern test assertions differently on every runner. Reuse the existing displayPath() helper (validate_schema.go) so these messages stay short and machine-independent again, while the underlying file resolution stays absolute and correct. Also addresses CodeRabbit findings on PR cloudposse#2875: - discoverProfileLocations treated CliConfigPath's ";"-joined multi-directory form (from connectPaths, reachable now that --config flows into profile loading) as one directory, producing paths like "dirA;dirB;/.atmos/profiles" that could never exist. Split and search each contributor directory. - Wrap the new Vendor/Workflows filepath.Abs failures with the existing absPathOrError/ErrPathResolution helper instead of returning a raw error. - getWorkflowsDirToUse/getVendorDirToUse's fallback join now uses the absolute-aware u.JoinPath instead of filepath.Join. - Reset viper and restore ATMOS_PROFILE around two tests that mutated global state without cleanup; fixed a doc comment that named the wrong test function. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(config): fix 8 DX bugs found field-testing the --config/--profile precedence changes A field-test pass against this branch's config-loading changes (multi-file --config merging, profile precedence, config get/set) found 8 real, live-reproduced issues and fixes them all, each with a failing test committed first: - config set/delete/format silently edited only the FIRST --config file while config get reported the fully-merged value -- a false-success bug whenever a later file also set the same key. Now refuses ambiguous multi-file --config with a clear error (pkg/config/config_edit.go: new ErrAmbiguousConfigFile/ResolveConfigOverride, shared by cmd/config/operations.go and the duplicate in pkg/mcp/config/config.go). - ATMOS_CONFIG/ATMOS_CONFIG_PATH with multiple comma-separated values worked for some commands (via pkg/config's own os.Args/env fallback) but broke ~40 others reading these flags through pkg/flags' Viper-based ParseGlobalFlags, which splits env-sourced values on whitespace, not commas. Fixed once at that shared choke point by exporting the existing --profile fix (parseViperProfilesFromEnv -> cfg.FixViperEnvStringSliceQuirk) and applying it there too. - profiles.base_path declared in a non-first --config file resolved against the FIRST file's directory regardless of which file actually declared it, silently failing to find profiles that exist. Added per-file directory tracking (mirroring the existing base_path tracking in mergeFiles) threaded through to discoverProfileLocations. - Vendor/workflow error messages (ErrEmptySources, ErrMissingVendorConfigDefinition, ErrDuplicateComponents, ErrComponentNotDefined, ErrNoComponentsWithTags, and others) still leaked absolute paths right next to the "Vendoring from" line already fixed in the prior commit -- a half-fixed pattern. Wrapped 13 sites in displayPath(), plus fixed a copy/paste bug in one workflow directory-read error that showed the raw unresolved config value instead of the path actually searched. - displayPath() itself was silently defeated whenever the working directory was reached through a symlink (e.g. macOS's /tmp), because os.Getwd() preserves the logical $PWD path while git-root-discovery-resolved config paths are physical. Fixed with a two-attempt comparison (raw first, then both sides resolved via the directory, since the target file often doesn't exist yet). - --config-path always wins over --config regardless of CLI argument order (undocumented, now documented, not code-changed -- effort didn't justify a fix for this ordering nuance). - Documented the previously-undocumented ATMOS_CONFIG/ATMOS_CONFIG_PATH env vars and fixed --config's flag-type description on config-set/delete/format.mdx (all three incorrectly said "string" instead of "string slice"). Also updates the field-test skill to default to testing the current branch's diff against its base branch when no explicit target is given, instead of asking. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review + close patch coverage gap on PR cloudposse#2875 CodeRabbit findings on the previous commit, verified and fixed: - displayPath()'s relPath() treated any path whose relative form merely started with the substring ".." as escaping cwd, so an in-directory file literally named e.g. "..vendor.yaml" would incorrectly leak its absolute path. Now checks for ".." as a complete path segment, with a regression test. - field-test skill: fixed the branch-targeting algorithm to resolve the actual PR base (gh pr view --json baseRefName) instead of the upstream tracking branch, which for a pushed feature branch produces an empty diff against itself; also now inspects staged/unstaged/untracked changes, not just committed history. Fixed the Phase 1 guidance that could be read as "skip internal/exec/" when the branch's diff actually touches it. - configuration.mdx: split the flags/env-var precedence entries and stated explicitly that a flag always wins over its env var equivalent. - vendor_utils_test.go: the "ErrEmptySources" test case actually exercised ErrMissingVendorConfigDefinition (same empty Sources+Imports triggers the earlier guard first) -- verified ErrEmptySources is structurally unreachable via ExecuteAtmosVendorInternal's public path given processVendorImports' per-level non-empty-content invariant, documented why, and removed the duplicate/misleading case rather than leave it mislabeled. - The load_profile_test.go missing-period finding was already resolved (the comment reads as one grammatically complete, period-terminated sentence spanning two lines) and the patch-scoped lint gate already passes at 0 issues, so left as-is. Also closes the Codecov patch-coverage gate (83.33% -> ~94%, threshold 85%) with real tests for the newly-added code, not coverage theater: - cmd/config/operations.go: config get's InitCliConfig failure path (a malformed --config file). - pkg/config/config_edit.go: ResolveConfigOverride's three branches directly. - pkg/config/load.go / load_config_args.go: declaresProfilesBasePath's branches (malformed YAML, non-mapping profiles, mapping without base_path) via a direct table test. - pkg/config/profiles.go: splitCliConfigPath's separators-only-no-content edge. - pkg/config/utils.go: a genuine (non-ErrFailedToFindImport) glob syntax error in FindAllStackConfigsInPaths[ForStack], previously untested because the only existing test used the "matched nothing" tolerated case. - internal/exec/vendor_utils.go: getVendorDirToUse/resolveVendorConfigFilePath directly, plus ErrDuplicateImport in processVendorImports. Remaining gaps were investigated and left deliberately uncovered, each for a stated reason rather than silently: two filepath.Abs error branches in AtmosConfigAbsolutePaths (config.go) are unreachable because atmosBasePathAbs is already guaranteed absolute by that point in the function (same as six untested sibling branches above them predating this PR); a handful of load.go/load_config_args.go lines are pre-existing code that only appear as "added" because wrapping the old flow in a new if/else shifted their indentation; and two permission-denied-style branches (vendor_utils.go, workflow_utils.go) are the same class of cross-platform-fragile os.Chmod scenario this repo already accepts as untested elsewhere (TestReadWorkDirConfig_GetwdError). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit follow-up review on PR cloudposse#2875 Verified against current code, two real findings fixed and one skipped as mechanically inapplicable: - field-test skill: the branch-diff inspection was still incomplete -- `--stat` alone never shows diff content, and untracked files never appear in `git diff HEAD` or a `--stat` summary at all, only their bare paths via `git status --porcelain`. Now reads the full `git diff <base>...HEAD` content and explicitly enumerates + reads untracked files via `git ls-files --others --exclude-standard`, so a brand-new implementation file can't go completely unread. - vendor_utils_test.go: TestResolveVendorConfigFilePath_CheckGlobalConfig's "absolute Vendor.BasePath" subtest used a hardcoded "/abs/vendor" string literal, which filepath.IsAbs only treats as absolute on POSIX -- on Windows it would take the wrong branch (no drive letter/UNC prefix) and fail. Switched to an OS-native absolute path built from t.TempDir(). - Skipped: "create cmd.NewTestKit(t) before invoking configGetCmd.RunE" in cmd/config/operations_test.go. Verified this is not applicable: NewTestKit is declared in cmd/testkit_test.go (package cmd, a _test.go file, so not importable cross-package at all), and cmd/config could not import package cmd regardless -- cmd/root.go already imports cmd/config, so the reverse import would be a circular dependency and fail to build. The test already follows this same file's established isolation pattern (viper.Reset() + os.Args save/restore via t.Cleanup) for the state it actually depends on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: verify base ref resolves before diffing in field-test skill CodeRabbit review on PR cloudposse#2875: the branch-name resolved via `gh pr view --json baseRefName` / `gh repo view --json defaultBranchRef` was never checked against what's actually available in the current checkout before being handed to `git diff <base>...HEAD`. A shallow clone, detached HEAD, or worktree with a narrow fetch refspec can know a branch's NAME without having its commits, so the skill could silently attempt (and fail) a diff against an unresolvable ref instead of falling back or asking. Added an explicit verification step: try `origin/<resolved-name>`, `<resolved-name>`, `origin/main`, `main` in order via `git rev-parse --verify --quiet <candidate>^{commit}`, take the first that resolves, and stop to ask the user only if none do -- rather than running the diff against an unverified ref and surfacing a raw git error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: trigger CI re-run Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(config): resolve base_path/profiles.base_path correctly with multi-source --config/--config-path A field-test pass on the --config/--profile precedence branch found that connectPaths joins every --config file/--config-path directory into a ";"-delimited CliConfigPath string once 2+ sources are given. Only profile discovery was taught to split that string; base_path resolution still joined a relative value directly against it, producing a nonexistent path -- silently breaking stack/component/vendor/workflow discovery ("No stacks found" with no error, or a misleading "not inside a git repository" error even though the config loaded correctly). - Add AtmosConfiguration.BasePathConfigDir, tracking whichever --config/--config-path source declared base_path (falling back to the first source), and anchor resolveAbsolutePath against it instead of the raw multi-directory CliConfigPath string. - Extend mergeConfigFromDirectories to track profiles.base_path declarations the same way mergeFiles already does, so it's honored for --config-path directories, not just --config files. - Backtick-wrap the glob path list in ErrNoStackManifestsFound so the markdown error renderer doesn't misinterpret "**" wildcards as emphasis and corrupt the rendered output. - A bare (non-dot-prefixed) hook working_directory now anchors to the component directory instead of silently falling through to CWD-relative exec.Cmd.Dir behavior, consistent with the existing component-anchored default for an empty working_directory. Updates docs/prd/base-path-resolution-semantics.md, atmos-profiles.md, and custom-hooks.md to document the multi-source anchoring rule and the hook working_directory anchoring rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: bump js-yaml/mermaid (security) + normalize pre-existing EditorConfig indentation Security (Dependabot cloudposse#263-cloudposse#269, all transitive npm deps in website/, pinned via pnpm.overrides, none requiring a major bump so none blocked by dependabot.yml's semver-major ignore policy): - js-yaml 3.15.0 -> 3.15.1, 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, high): quadratic CPU consumption in !!omap resolution - mermaid 11.16.0 -> 11.16.1 (5 alerts) No open CodeQL alerts at time of remediation. Verified with a full `pnpm run build` in website/ -- succeeds with only pre-existing, unrelated warnings. NOTICE is unaffected (tracks Go module dependencies only). EditorConfig (CI failure fix): CI's "Validation (affected)" and "Run pre-commit hooks" jobs validate every file touched on the branch in full, not just the changed lines. Editing docs/prd/atmos-profiles.md and docs/prd/base-path-resolution-semantics.md in an earlier commit surfaced 150 pre-existing "Wrong amount of left-padding spaces (want multiple of 2)" violations elsewhere in those files -- long-standing 3/5/7-space list/pseudocode indentation never previously caught because neither file had been touched by a PR before. Normalized every odd-indented line to the next even width (add 1 space), preserving nesting structure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review on PR cloudposse#2875 - getWorkflowsDirToUse/getVendorDirToUse: use getBasePathToUse (not raw atmosConfig.BasePath) in their fallback branches, consistent with every other BasePath-anchored resolution in internal/exec. - Dedupe configGetTestStreams/configSchemaTestStreams into one shared configTestStreams/initConfigTestWriter helper in cmd/config. - Expand AtmosConfigAbsolutePaths test coverage: absolute nested Vendor/Workflows.BasePath pass through unchanged, empty nested paths default to the base path itself. - Assert stage 2/3 intermediate values in the array-field-merge test instead of only logging them, so a regression at those stages fails loudly. - Extract resolveStackGlobMatches to share glob-pattern resolution and error handling between FindAllStackConfigsInPathsForStack and FindAllStackConfigsInPaths (previously duplicated verbatim). - field-test skill: stop falling back to origin/main/main when a PR's actual base (e.g. develop) is known but unresolved locally -- that silently diffed against the wrong history. main is now a legitimate fallback only in the genuine no-PR case; a known non-default base that doesn't resolve now stops and asks instead. - atmos-profiles.md: replace a duplicate, conflicting "Configuration loading chain" description with a reference to FR3.1 (the authoritative order), which also states that --config/--config-path bypass later discovery. Verified the rest of the review (ErrEmptySources unreachability, several already-applied fixes, and one nitpick -- retiring loadConfigFromCLIArgs -- deferred as disproportionate to its stated cleanup value) and left them unchanged; see prior conversation turn for the itemized reasoning. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(config): preserve genuine glob errors from every pattern, not just patterns[0] resolveStackGlobMatches discarded every generated pattern's own error inside the loop and, once no matches were found across all of them, only ever retried patterns[0] to decide whether a genuine error had occurred. Since GetGlobMatches always errors (wrapping ErrFailedToFindImport) on zero matches, a genuine error (bad glob syntax, permission denied) from any pattern after the first was silently discarded -- discovery could return no matches or ErrNoStackManifestsFound instead of the real error. Check each pattern's error inline: skip ErrFailedToFindImport (expected, valid-but-empty), return every other error immediately with that pattern (not a hardcoded patterns[0]) in its context. Removes the now-unnecessary post-loop retry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
Marko Petrovic (gitbluf)
pushed a commit
to gitbluf/atmos
that referenced
this pull request
Aug 11, 2026
…custom commands and workflows (cloudposse#2882) * feat(workflows): make custom commands and workflows a complete task-runner 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> * docs(roadmap): link taskfile-convergence milestones to PR cloudposse#2882 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): fix Windows shell-escape path corruption and CodeQL alloc overflow 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. * fix(ci): tolerate mvdan/sh CRLF+trailing-space on Windows in test assertions 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. * fix(workflows): fix 4 concurrency/control-flow bugs in command dependencies 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> * fix(ci): make LinePrefixWriter flush multi-line writes as one atomic 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> * fix(workflows): address CodeRabbit review findings on PR cloudposse#2882 - 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> * fix(ci): regenerate JSON schema for UnitDependencies string shorthand The generated atmos.yaml schema rejected the plain-string shorthand form of dependencies.commands/.workflows entries, even though schema.UnitDependencies' UnmarshalYAML has always accepted it. Add the missing applyPolymorphicOverrides entry (mirroring the existing Tasks entry) and regenerate the schema, fixing TestGeneratedSchemaAcceptsRealConfigs failures on the Linux and macOS acceptance-test CI jobs. Also fix an EditorConfig list-continuation indentation issue in a previously committed fix doc, caught by the pre-commit hook while committing this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(cmd): use Go-native marker writer instead of shell echo/redirect Replace the echo/>> shell redirect in the values: constraint tests with the package's existing customCommandWriteHelperCommand os.Executable() helper, matching the pattern already used throughout custom_command_integration_test.go for marker-file writes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: retrigger CI Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workflows): address CodeRabbit review findings on PR cloudposse#2882 (round 2) Fixes: - pkg/runner/freshness/checker.go: RecordSuccess early-returned when inputs == nil, so an artifacts-only step never persisted state and reran forever even once its artifact existed and was unchanged. Widen the caller gates and hash an empty source list instead of skipping the record entirely. - pkg/taskgraph/adapters/cobra_command.go: a dependency dispatch permanently left its dependencies-resolved marker and drained error sink on the target *cobra.Command's context. Restore the original context via defer so a later top-level invocation of the same command object (long-lived processes, test suites reusing RootCmd) doesn't inherit stale dispatch state. - pkg/taskgraph/taskgraph.go: a `fail: best_effort` entry silences the whole run's failures, including siblings that declared no fail: at all -- documented as explicitly run-wide (not per-entry) on UnitDependency.Fail, and the swallowed error is now logged at warn level so it isn't invisible. - pkg/datafetcher/schema/atmos/manifest/1.0.json + pkg/datafetcher/schema/stacks/stack-config/1.0.json: the shared "dependencies" definition only modeled tools/components/files/folders, so a documented dependencies.commands/dependencies.workflows declaration was rejected by the manifest schema (additionalProperties: false) and had no typed shape in the stack-config schema. Also removes a duplicate "dependencies" key in the manifest's workflow_manifest object (Biome noDuplicateObjectKeys). - pkg/schema/command.go: FindCommandByName resolved a bare name to the first depth-first match, so a config with duplicate global/nested command names could route a dependencies.commands reference to the wrong target. Now reports ambiguous=true instead of guessing; CommandLookup surfaces this as a clear error during graph-building, before any dispatch happens. Investigated, not applied: - internal/exec/workflow_utils.go's stepExecutorState global can race when taskgraph.Run dispatches multiple sibling dependencies.workflows entries concurrently. Documented the failure mode and why a quick mutex is unsafe here (deadlocks on multi-level dependency chains, or leaves a stale-pointer race window) -- a real fix means threading a *stepPkg.StepExecutor through ExecuteWorkflow instead of reaching for the package-level var, which is a larger, separately-scoped refactor. Verified as correct behavior, not bugs (added regression tests either way): - cmd/cmd_utils.go's unforgiven-failure step loop deliberately doesn't break -- it matches the pre-existing workflow executor's identical pattern and GHA's own if:success()/if:failure() semantics, so a when:failure handler step still runs after an unforgiven failure. - internal/exec/custom_command_control_adapter.go's TemplateData callback ignoring its matrix argument matches the workflow control adapter's identical, already-correct pattern -- pkg/workflow/control.go's controlTemplateData injects matrix independently of the callback. Replaces the remaining shell echo/redirect/exit test fixtures in cmd/custom_command_control_test.go with Go-native os.Executable() helpers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 7 Dependabot alerts (js-yaml, mermaid) Bump the pnpm overrides pinning js-yaml and mermaid to their patched versions -- all within the same major version, so no dependabot.yml ignore-policy exception is needed: - js-yaml@^3: 3.15.0 -> 3.15.1 (GHSA-5p4m-2wfm-xmqj, alert cloudposse#269) - js-yaml@^4: 4.2.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, alert cloudposse#268) - mermaid@^11: 11.15.0 -> 11.16.1 (GHSA-rhh3-jpg6-66xh cloudposse#267, GHSA-c4c3-pg64-4m4v cloudposse#266, GHSA-6x64-9x62-f2gx cloudposse#265, GHSA-3rrr-jr9j-h3q3 cloudposse#264, GHSA-2v8p-3f2j-5mp7 cloudposse#263) Verified: pnpm install regenerates the lockfile at the patched versions, `npm run build` succeeds, and NOTICE is unchanged (no license drift). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workflows): unit_dependency requires name, raise patch coverage toward 85% Schema fix (CodeRabbit): - pkg/datafetcher/schema/atmos/manifest/1.0.json + stacks/stack-config/1.0.json: unit_dependency accepted an entry with no `name` (e.g. `{}` or `{flags: {env: dev}}`), producing a reference with an empty name that only failed later during graph construction instead of failing fast at schema validation. Add "required": ["name"] to both copies. Coverage (patch coverage was 79.12% against an 85% target; 224 lines missing): adds real, behavior-asserting tests across cmd, internal/exec, pkg/condition, pkg/datafetcher, pkg/flags, pkg/hashfile, pkg/runner/freshness, pkg/schema, pkg/taskgraph, and pkg/workflow -- error paths, edge cases, and previously uncovered branches added by this PR's dependencies/freshness/continue/matrix work. Notably: pkg/flags/constrained.go gained isInteractiveFn/promptForValueFn DI seams (mirroring the existing pattern in cmd/secret/deps.go) so the interactive-prompt branches of ValidateConstrainedFields are testable without a real TTY. Genuinely untestable lines (defensive/unreachable code, no injection seam, TTY-only, or requiring real network/toolchain access) are left uncovered with the reasoning documented at each call site rather than padded with tautological tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workflows): quote test-helper exe paths for real host shell on Windows customCommandWriteHelperCommand and its siblings used Go's %q to quote os.Executable()'s path in the shell command string. %q applies Go-syntax escaping, doubling every backslash in a Windows path. Steps nested inside a `type: parallel`/`type: matrix` group run through pkg/workflow/control_executor.go's controlShellInvocationForOS, which (absent a wired ShellRunner) shells out to the real host shell -- cmd.exe /C on Windows -- instead of the in-process mvdan/sh interpreter used by top-level shell steps. Native cmd.exe doesn't collapse a doubled backslash back to one, so %q corrupted the executable path there, failing TestCustomCommandIntegration_ParallelStepWithNeeds on the Acceptance Tests (windows) CI job. Switched to a plain double-quote wrap, valid unescaped syntax in both POSIX shell and cmd.exe for a path with no embedded quotes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(process): use verbatim CmdLine for cmd.exe /C on Windows The previous fix (3dec2e9) stopped double-quote-formatting from doubling backslashes in a Windows executable path, but TestCustomCommandIntegration_ ParallelStepWithNeeds still failed CI: a `type: shell` step nested in a `type: parallel`/`type: matrix` group routes through pkg/workflow/control_executor.go's controlShellInvocationForOS, which -- absent a wired ShellRunner -- runs the raw command through the real host shell (`cmd.exe /C <command>` on Windows) via process.DefaultRunner's plain os/exec.CommandContext(Command, Args...). Go's default Windows arg escaping re-quotes and backslash-escapes that whole command string (it contains spaces), corrupting the quote characters already wrapping the executable path before cmd.exe ever parses it -- the same class of bug pkg/process/shell_command_windows.go's NewShellCommand already solved for session-attached commands, by bypassing Args-based escaping entirely with a verbatim SysProcAttr.CmdLine ("<shell>" /S /C "<command>"). Extend that same fix to process.DefaultRunner.Run: applyWindowsCmdExeQuoting detects the `cmd.exe /C <command>` shape controlShellInvocationForOS produces and rewrites the *exec.Cmd to use the verbatim CmdLine, exactly like NewShellCommand, instead of the default per-argument escaping. No-op everywhere else (POSIX exec.Cmd passes Args to execve verbatim; other Program/Args shapes on Windows keep normal escaping). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): record windows parallel/matrix shell child quoting fix Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor(workflows): rename step field precondition to preconditions Matches the pluralization convention already used by inputs/artifacts/ dependencies for a single YAML block that can hold multiple check kinds (currently tools; more may follow). Renames the Go type/field, the CEL fact (precondition.success -> preconditions.success), the hand-maintained manifest/stack-config JSON schemas, and all docs/examples. Safe now since this field hasn't shipped yet (PR cloudposse#2882 is still open). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate nanoid infinite-loop DoS Dependabot alert Bumps the pnpm.overrides target for nanoid's 3.x line from ^3.3.15 to ^3.3.17 (patched: GHSA-2v37-7h3g-55p8 / CVE-2026-67213), and adds an override for postcss's own `^3.3.16` request range so it resolves to the same patched version. The 4.x/5.x override already resolved to 5.1.16, past the patched 5.1.6 cutoff, so it needed no change. image-size's two DoS alerts (GHSA-w3rx-r6r6-pgpr, GHSA-5p2g-fcmc-qvqq) are not fixed here: both report first_patched_version=null and npm has no image-size release past 2.0.2 yet, so there is nothing to bump to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(workflows): note GOBIN/PATH requirement in preconditions example Addresses CodeRabbit review comment on PR cloudposse#2882: `go install` writes to GOBIN (or GOPATH/bin) rather than updating PATH directly, so the install-stringer example needs to say that directory must already be on PATH for preconditions.tools to find the binary on a later run. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workflows): address CodeRabbit review findings on PR cloudposse#2882 (round 3) 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> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
Marko Petrovic (gitbluf)
pushed a commit
to gitbluf/atmos
that referenced
this pull request
Aug 12, 2026
…posse#2897) * feat(config): auto type inference for config/stack set, fix provenance and merge bugs Adds --type=auto (the new default) to `atmos config set` and `atmos stack set`: it infers from the Atmos config schema, then from the type of the value already at the path, warning instead of silently stringifying when neither source has an answer. `atmos stack set` previously never inferred at all. Also fixes several bugs found during a field-test pass of the config/stack commands: - MCPSettings.Enabled (bool + omitempty) could vanish entirely when merging atmos.yaml with an atmos.d fragment that both set it; now a *bool. - PickProvenanceFile picked a phantom Line:0 provenance entry over the real defining file for values that live only in an imported catalog manifest, breaking `stack set/delete/get/list` for the standard catalog+import pattern. This also fixed an independent copy of the same bug in the AI/MCP tools layer. - Error hints containing a raw <placeholder> were silently stripped by the markdown renderer (parsed as inline HTML); now HTML-entity-escaped outside backtick spans. - `--config a.yaml,b.yaml` on edit commands silently targeted only the first file; now warns. - `config get` on a key defined only in an atmos.d fragment returned a bare "not found" with no indication it might be defined elsewhere. - `unset` alias was missing from `--help` output for config/stack delete. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(fixtures): add config/stack field-test fixture Adds the durable Atmos project fixture used to manually verify the config/stack fixes in this PR live: multi-level imports, base-component inheritance, YAML anchors/aliases, and an atmos.d fragment -- coverage no existing fixture provided for this command surface. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review on PR cloudposse#2897 - GetType now correctly distinguishes an explicit YAML null (real, present value) from a missing path, returning (TypeNull, true) instead of ok=false. The auto-inference callers (config/stack set) deliberately still treat an existing null as unresolved, since buildRHS's TypeNull case ignores the value argument and would otherwise silently discard the new value being set. - looksNonStringPattern now recognizes scientific notation (1e3), explicit plus signs (+5), and leading/trailing-dot decimals (.5, 5.), so the fallback-to-string warning fires for these shapes too. - escapeHintAngleBrackets now uses a CommonMark-compliant delimiter-aware code-span scanner instead of a single-backtick regex, so double-backtick-delimited hints are handled correctly. - Fixed two doc examples (blog post + config-set.mdx) that claimed to demonstrate the fallback warning but used commands that couldn't actually produce it (explicit --type, or a value that doesn't look non-string); also added the required -s/-c flags to two stack set examples. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(claude): block direct golangci-lint invocation The repo's real PR lint gate is ./custom-gcl (the custom golangci-lint binary with the lintroller plugin, per pre-commit), not plain golangci-lint. Denying the raw binary steers future sessions toward the already-allowed ./custom-gcl instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(vendor): resolve component versions across import chain A manual field test of `atmos vendor config` and the `atmos vendor get/set` aliases found the alias commands couldn't see components declared only in an imported vendor manifest, even though `vendor config list`/`format` already walked the import chain correctly. - ComponentVersionPath/SetComponentVersion now search vendor.yaml and every manifest it imports, writing to the file that actually declares the component instead of always the root file. - vendor config set now warns when a numeric/bool-looking value is stored as a string with no --type passed, matching config set/stack set. - vendor config get/set/delete now attach actionable hints to missing-file and not-found-in-import-chain errors instead of leaking a raw "failed to read file" message. - The shared YAML anchor guard's error now names the actual workaround (an explicit override key) instead of just "restructure". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review on PR cloudposse#2897 - Move the vendor config set "looks non-string" warning to fire only after a successful write, so a failed write (e.g. a missing file) no longer falsely claims a value was stored. - Fix GetType/pathIsExplicitlyPresent to distinguish an explicit YAML null from a missing value for raw yq paths (leading "."), not just ordinary dot-paths -- a raw path is decomposed into segments first when possible, falling back to the prior collapsed check only for genuinely non-decomposable expressions. - Strengthen the vendor get/set import-chain regression test to assert the actual returned version, not just command success, so it would fail if get read the root file instead of the declaring file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address hands-on field-test findings for config/stack/vendor set Live-driving the CLI (not just unit tests) against this branch's own config-field-test fixture surfaced 6 real gaps left by the auto type-inference and provenance fixes: - pkg/yaml/anchors.go: the anchor alias-flattening rejection had no actionable hint, and reusing the sibling content-diff branch's hint verbatim would have been actively misleading (it tells the user to do exactly what triggers this rejection). - cmd/stack/operations.go: `stack set --file` silently dropped type inference for values inherited from an abstract/base component, storing them as strings with an inaccurate "nothing to infer from" warning even though the merged value's type was already computed and simply discarded. - pkg/yaml/typed.go: LooksNonString now warns for underscore-separated numbers and "nan" (forms strconv already accepts), while deliberately leaving hex/octal/.inf unwarned since warning about those would send users into a parser dead end. - pkg/yaml/errors.go: value/type validation failures (bad --type input, unknown --type) were headlined "invalid YAML path or expression" even though the path was fine -- split into a new ErrInvalidTypedValue sentinel. vendor config set --type=auto now gets an explicit, hinted rejection instead of falling through to a bare internal error. - cmd/vendor/config.go: the non-string warning printed after the success message instead of before, unlike config set/stack set. - tests/fixtures/scenarios/config-field-test: fixed a comment that had the atmos.d-vs-root precedence direction backwards. Each fix has a regression test confirmed to fail before the fix and pass after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 7 npm Dependabot alerts (js-yaml, mermaid) Bump the existing pnpm.overrides pins for js-yaml and mermaid to their patched versions — all within-major patch/minor bumps, none blocked by dependabot.yml's major-version ignore policy: - js-yaml@^3 -> ^3.15.1 (GHSA-5p4m-2wfm-xmqj, alert cloudposse#269) - js-yaml@^4 -> ^4.3.1 (GHSA-5p4m-2wfm-xmqj, alert cloudposse#268) - mermaid@^11 -> ^11.16.1 (alerts cloudposse#263-267: GHSA-2v8p-3f2j-5mp7, GHSA-3rrr-jr9j-h3q3, GHSA-6x64-9x62-f2gx, GHSA-c4c3-pg64-4m4v, GHSA-rhh3-jpg6-66xh) Verified with `pnpm install --lockfile-only` (patched versions land) and a full `website` build (succeeds, no new warnings). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review on PR cloudposse#2897 - pkg/yaml/typed.go: looksNonStringPattern's fractional branch accepted underscores strconv.ParseFloat rejects -- a trailing underscore right after the last fraction digit ("1.2_") or one immediately after the decimal point ("1._2"). Narrow the fractional group so underscores are only valid strictly between two digits, matching the integer branch's existing discipline. - cmd/stack/operations_test.go: the new inherited-value regression test used a slash-literal fixture path and re-literalized it a second time for os.ReadFile. Build it with filepath.Join and reuse flagFile. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address second field-test round on config/stack set type inference Live-driving the CLI (not just unit tests) again against this branch's config-field-test fixture surfaced 3 new high-severity gaps beyond the 6 already fixed in d751acb: - pkg/yaml/edit.go: GetType fell through to (TypeString, true) for a !!seq/!!map existing value, which config set/stack set's --type=auto read as a confidently inferred string -- silently overwriting an existing list or map with a plain string, no warning. GetType now reports TypeYAML for non-scalar values, and the callers refuse with an actionable error instead of coercing. - pkg/yaml/typed.go: --type=int wrote the raw literal verbatim, so a leading-zero value like "010" (valid decimal 10 per strconv) would round-trip as octal 8 on the next YAML read -- confirmed through the full describe component pipeline. Now writes the canonical decimal form. Also distinguishes int overflow (strconv.ErrRange) from a syntax error for a clearer message, and rejects NaN/Infinity for --type=float explicitly: an initial attempt to write the YAML 1.1 ".nan"/".inf" spelling silently wrote null instead, since the leading dot collides with yq's path-navigation operator in the raw SetRaw expression. - cmd/stack/operations.go: stack set/delete resolving via provenance to a shared imported catalog file silently mutated every other stack/component importing it. Now warns before the mutation when the resolved file isn't one of the stack's own top-level manifests. Also fixes the config-field-test fixture's settings.fragment_only_setting comment, which claimed the key was visible via the merged `atmos describe config` view -- it's actually silently dropped everywhere, since atmos.yaml's root settings: decodes into the strongly-typed AtmosSettings struct with no catch-all field. Replaced with the real settings.list_merge_strategy field, which actually demonstrates the atmos.d-vs-root visibility gap the fixture was built to test. Each fix has a regression test confirmed to fail before the fix and pass after, including the fixture's mycomponent-anchor-owner/ mycomponent-anchor-user components exercised through the real CLI command layer for the first time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review on PR cloudposse#2897 Use the correct command name (`stack set`) instead of `stack config set` in the vendor config set --type flag doc, matching the naming already used consistently elsewhere in this codebase (e.g. cmd/vendor/config.go's own comments). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat: --type=auto infers instead of nagging on config/stack set config set and stack set --type=auto used to fall back to a plain string for any brand-new key and merely warn that the value "looked like" a bool/int/float, without ever acting on that judgment -- `atmos stack set vars.replicas 5` on a brand-new key wrote the string "5" and warned, instead of just writing the integer 5. --type=auto now actually infers the type in that case: - stack set draws on the target Terraform component's declared variable type for vars.* paths (already parsed via terraform-config-inspect as a side effect of resolving the component, so this costs nothing extra) -- ranked above even an already-stored value, so a declared type retypes a value that disagrees with it (e.g. `variable "replicas" { type = number }` but the manifest has replicas: "5" quoted). A real retype prints an informational notice rather than happening silently. - Both config set and stack set fall back to guessing the type from the new value's own shape (pkg/yaml.GuessScalarType) when nothing else resolves it -- "5" infers int, "true" infers bool -- before finally falling back to a warned string for values that don't look like anything but a string. Regression tests were written first (against stub implementations, confirmed to fail for the predicted reasons) per this repo's test-first bug-fixing workflow, then the real implementation added. Along the way, an apparent second bug (yqlib stripping quotes from values like "nan"/"yes"/"no" even under explicit --type=string) was investigated and found to be a false alarm: yaml.v3 (Atmos's actual downstream config/stack loader) and pkg/yaml's own GetType/Get both already resolve those unquoted words back to plain strings. An initial fix attempt (forcing double-quote style on every string value) broke existing tests expecting unquoted plain strings and was reverted once the false alarm was confirmed empirically against yaml.v3's actual resolver table. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate nanoid infinite-loop DoS (GHSA-2v37-7h3g-55p8) Broadened the existing nanoid pnpm override from an exact-version key (nanoid@3.3.3) to a range key (nanoid@^3), since postcss's own nanoid dependency wasn't covered by the exact-pin form and still resolved to the vulnerable 3.3.16. Bumped the override target to ^3.3.17, the first patched 3.x release for CVE-2026-67213 (a size=0 argument spins customAlphabet/customRandom in an infinite loop). The 4.x lineage's override (nanoid@4.0.2 -> ^5.1.16) was already on a patched release. Two other open Dependabot alerts (image-size, GHSA-w3rx-r6r6-pgpr and GHSA-5p2g-fcmc-qvqq) have no first_patched_version upstream yet and are left unfixed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: treat GitHub API transport failures as transient in live tests TestIsArchived_LiveNetwork failed on Windows Acceptance Tests CI with a TLS certificate verification error reaching api.github.com -- a CI-runner-side trust-store issue, not a real GitHub API error or a bug in Atmos. isGitHubTransientError (the helper live-network tests in pkg/github already use to skip on conditions outside the test's control) only recognized errors where an HTTP response was actually received (rate limits, 5xx). A pure transport failure -- DNS, connection, TLS -- never gets a response at all: handleGitHubAPIError's 401/rate-limit branches both require resp != nil, so a transport error falls through unwrapped as the raw *url.Error net/http constructs. isGitHubTransientError now also recognizes *url.Error via errors.As, checked after the existing *github.ErrorResponse case so a genuine API error is never misclassified. Regression test written first (confirmed failing pre-fix) covering both the observed TLS case and a DNS timeout, plus a non-regression case for a real 404 response. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(vendor): update complete example's vendor.lock.yaml Regenerated lock entries for infra/vpc-flow-logs-bucket and infra/account-map, picked up from running vendor-related tests locally against this fixture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(blog): align type-inference blog post with shipped contract The blog's "The Fix" section and vars.new_flag example still described the pre-"auto nag" fix contract (new unmodeled value falls back to a string with a warning). The shipped implementation, tests, and command docs all now infer from the new value's own shape instead, and also draw on the target Terraform component's declared variable type for stack set's vars.* paths (retyping an existing value that disagrees). Updates the post to describe that final contract, per PR review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: correct misleading nan/Infinity fallback warning hint warnIfSilentlyStoredAsString's hint told users to pass --type to store a value "as bool, int, float, or yaml" -- but this warning is now reachable only for the bare "nan" literal (GuessScalarType handles every other bool/int/float shape directly), and --type=float explicitly rejects NaN/Infinity, so the hint sent users straight into a parser error. CodeRabbit correctly flagged the --type=float suggestion as broken, but its own proposed replacement (--type=yaml with a .nan literal) is also broken -- confirmed live: the leading "." in ".nan" collides with yq's path-navigation operator in the raw assignment expression, so it silently writes null instead of NaN. That's worse than the original message, since it "succeeds" while corrupting the value instead of failing loudly. There's no currently-safe way to write NaN/Infinity as a typed float through this command, so the new message says so plainly instead of pointing at a workaround that doesn't work. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
added a commit
to jorrite/atmos
that referenced
this pull request
Aug 14, 2026
…udposse#2888) (cloudposse#2892) * test: add regression tests proving cloudposse#2888 deferred-merge data loss Strengthens the existing "deep merges with yaml functions" assertion (it only checked the key existed, never the merged value) and adds a matching !labels/!tags case. Both fail today: !template, !labels, and !tags all silently lose data when a concrete override collides with an unresolved deferred function, because every ApplyDeferredMerges call site in stack_processor_merge.go passes processor=nil. Also updates the deferred-yaml-functions-evaluation-in-merge PRD to correct its stale "implemented and tested" status and document the completion plan (staged as plumbing-only PR 1 + behavior-change PR 2) for wiring real post-merge resolution, tracked by cloudposse#2888. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: add fix-log record for cloudposse#2888; fix pre-existing editorconfig violations Adds the fix-log record for the cloudposse#2888 regression tests and completion plan. Also fixes ~230 pre-existing editorconfig violations in the PRD (tab-indented Go snippets, 3-space list indentation, misaligned fenced-block content) that were surfaced once the file entered a diff for the first time since it was written — unrelated to cloudposse#2888 itself, but blocking any commit that touches the file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: trigger CI re-run Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(merge): resolve deferred YAML functions and deep-merge with concrete overrides (cloudposse#2888) Wires up the previously-unconnected "resolve, then deep-merge" half of the deferred-merge design: every production call site of ApplyDeferredMerges was passing processor=nil, so !template/!terraform.output/!terraform.state/!store/ !exec/!env silently lost data whenever a concrete value at another config layer collided with them, and !labels/!tags weren't even deferred at all (the literal cloudposse#2888 report). Adds a real Stage 3 resolution pass (per-invocation, auth- and template-context-aware) that resolves deferred functions and deep-merges the result against any concrete override at the same path — including the mirror-precedence direction (a concrete value at a *lower*-precedence layer than the function), which the original design didn't handle and which a new regression test caught during implementation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: raise PR cloudposse#2892 patch coverage from 70.64% to 86.20% Codecov flagged patch coverage below the 85% gate on the deferred-merge fix. Adds targeted error-injection and regression tests for the previously-uncovered branches in stack_processor_merge.go, stack_processor_process_stacks.go, deferred_contexts.go, generate_adapter_funcs.go, and completions.go — no production code changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): exclude flaky reproducible-builds.org link from markdown link check CI's Check Markdown Links job failed on docs/prd/archive-step.md:175 with "Connection refused" against reproducible-builds.org/docs/source-date-epoch/. The link returns 200 outside CI (verified via curl) — same CI-runner-specific connection-refusal pattern as the ~30 other domains already excluded in lychee.toml. * fix: address CodeRabbit review feedback on PR cloudposse#2892 Fixes two real correctness bugs and updates stale/incomplete test and documentation coverage flagged by CodeRabbit on this PR: - stack_processor_process_stacks.go: add cfg.HelmComponentType to builtInTypes so components.helm is no longer reprocessed (and its merged data clobbered) by the custom-component-type passthrough loop, which would otherwise make Stage 3 resolve deferred YAML functions against mismatched component data. - terraform_generate_backends.go / terraform_generate_varfiles.go: both batch generators now retain FindStacksMap's deferred-merge contexts and call resolveDeferredYamlFunctions after ProcessCustomYamlTags, closing the same cloudposse#2888 data-loss gap in these two call sites that the main describe/plan path already fixed. - yaml_processor.go: the deferred-string template fast path now resolves the configured left template delimiter instead of hardcoding "{{", so custom delimiters (e.g. "[[ ]]") aren't silently skipped. - cmd/emulator/completions_test.go: assert the full expected sorted stack list instead of just Contains("local"). - tests/yaml_functions_integration_test.go: initialize the CLI config in TestYAMLFunctionsDeferredMergeCacheCorrectness so it doesn't depend on state primed by an earlier test in the same file. - docs/prd and docs/fixes: correct stale file/symbol references and pre-fix status claims now that the cloudposse#2888 fix has shipped in this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: close remaining patch-coverage gap in fillMissingLayerValues The last uncovered branch from the cloudposse#2892 patch-coverage fix (the other 7 flagged files were already addressed): the defensive len(values)==0 guard, unreachable via the public AddDeferred API, so seeded directly via the unexported field per this file's existing pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 7 Dependabot alerts in website pnpm overrides Bumps transitive js-yaml (>=3.15.1, >=4.3.1) and mermaid (>=11.16.1) via pnpm.overrides to their patched versions - quadratic-CPU YAML parsing and mermaid XSS/ReDoS advisories. All fixes are minor/patch bumps within the same major version, so none are blocked by dependabot.yml's major-version ignore policy. Fixes GHSA-5p4m-2wfm-xmqj (js-yaml, alerts cloudposse#268/cloudposse#269), GHSA-rhh3-jpg6-66xh, GHSA-c4c3-pg64-4m4v, GHSA-6x64-9x62-f2gx, GHSA-3rrr-jr9j-h3q3, GHSA-2v8p-3f2j-5mp7 (mermaid, alerts cloudposse#263-267). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: close remaining PR cloudposse#2892 patch-coverage gaps in stack_processor_merge.go and stack_processor_process_stacks.go stack_processor_merge.go (now fully covered): each nil-processor ApplyDeferredMerges write-back error branch (auth, providers, required_providers, hooks, generate, test, plus processAuthConfig's own second pass) forced via a deferred value at a nested path whose parent segment is then overridden by a higher-precedence scalar, so SetValueAtPath can no longer navigate to it (ErrCannotNavigatePath). Also closes the remote-state-backend error path via a non-map value at the backend-type key. stack_processor_process_stacks.go: closes the processComponentsInParallel error-propagation branch for all 5 non-terraform component types (same technique as above, routed through the public ProcessStackConfig entry point), the custom-component metadata.inherits type-validation branch, and the per-component "value is not a map" branch in buildComponentWork. The remaining gap in this file (~37 lines) is the mechanical `if err != nil { return nil, nil, err }` sweep after plain m.Merge calls between two map[string]any layers: traced deepMergeNative's only real error source (ErrMergeNilDst) and confirmed it's unreachable through the public Merge entry point, which always filters nil/empty inputs first — the native merge engine's override-always-wins design makes these lines defensive, not reachable, code. Same conclusion applies to yaml_processor.go's remaining 3 lines/3 partials (verified via 11 probe inputs against its deliberately robust YAML-quoting logic) and the equivalent 1-line gaps in terraform_generate_backends.go/terraform_generate_varfiles.go (the error would need to survive an earlier eager-resolution pass but fail on the later deferred-resolution pass for the same value). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review feedback on PR cloudposse#2892 (round 2) - docs/prd/deferred-yaml-functions-evaluation-in-merge.md: the historical-design-pseudocode note pointed readers to "Implementation Status" for the as-shipped design, but that section records the Version 2.0 pre-fix state (processor = nil, never wired up). Point to the top Status and Completion Plan sections instead, and label Implementation Status as historical. - lychee.toml: anchor the reproducible-builds.org exclusion to the documented /docs/source-date-epoch/ URL (optional trailing slash) so it doesn't also swallow future paths under that prefix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(skills): field-test respects plan mode and always ties fixes to fix-log Add a Plan Mode section so a field-test invoked under plan mode does read-only research/hypothesis phases, writes a plan, and requests approval via ExitPlanMode before building fixtures or executing. Also tighten the existing fix-log guidance so any plan to fix findings — not just its implementation — closes with the fix-log skill. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: order deferred YAML function paths ancestor-before-descendant ApplyDeferredMerges ranged over dctx.GetDeferredValues(), a plain Go map, so a parent path (e.g. vars.combo) and a child path (e.g. vars.combo.nested) occupied by different deferred functions in different merge layers could resolve in either order. Each path's resolution ends in an unconditional SetValueAtPath call, so when the ancestor was processed after the descendant, its wholesale replace of the shared parent map silently discarded the descendant's already- resolved value — live reproduction showed ~40% of runs corrupting output with no error. Fixed by sorting path keys by ascending path-segment length before processing, so descendant leaf writes always happen last. Found via a field-test pass on PR cloudposse#2892 (cloudposse#2888); adds a 200-iteration regression test plus the field-test fixtures that confirmed 7 other scenarios already behaved correctly. See docs/fixes/2026-08-07-deferred-merge- nested-function-collision.md for full validation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: split oversized merge_deferred_test.go into focused files merge_deferred_test.go had grown to 1598 lines, well past this repo's 600-line file-size convention. Split by the function/feature each test group exercises: YAML-function detection/walking, map/slice/path primitives, merge strategies, MergeWithDeferred (kept in the original filename), ApplyDeferredMerges (including the parent/child-collision regression test), and the process/fill helpers. All 16 test functions moved verbatim; no behavior changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: resolve return-signature mismatch from merging origin/main Merging origin/main (which landed cloudposse#2874, a Kubernetes validate/GitOps fix) into this branch combined cleanly at the text level but broke compilation: this branch had already changed mergeComponentConfigurations to return (map[string]any, ComponentDeferredContexts, error), but cloudposse#2874 added a new finalComponentValidate error path and 7 test call sites still using the old 2-value (map[string]any, error) signature — a silent semantic merge conflict CI's PR-preview build caught (all 5 failing jobs shared this one root cause). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: add fix-log record for the CI build failure fix Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(website): dedupe nanoid pnpm override entry, breaks lockfile parsing Merging origin/main introduced a second, identical "nanoid@^3.3.16" key into website/package.json's pnpm.overrides (both branches added it independently at different points, so git's textual merge combined them without a conflict). The duplicate JSON key propagated into pnpm-lock.yaml as a duplicate YAML mapping key, which pnpm rejects outright (ERR_PNPM_BROKEN_LOCKFILE), blocking `pnpm install` and the website build. Removed the duplicate override entry and regenerated the lockfile via `pnpm install --lockfile-only` rather than hand- editing it. Verified `pnpm run build` succeeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): retry the GitHub releases fetch behind `toolchain info` A single transient network/TLS hiccup on api.github.com silently degraded `atmos toolchain info` to "no available versions" instead of retrying, since getAvailableVersions already treats a fetch failure as non-fatal by design. This surfaced as a flaky CI golden-snapshot mismatch on TestCLICommands/atmos_toolchain_info_shows_atmos-inline_registry. makeGitHubRequest now retries transient failures (429/5xx/transport errors) with bounded exponential backoff via pkg/retry, following the same pattern already used in pkg/oci/pull.go, while still failing fast on deterministic client errors (404, 403, ...). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review feedback on PR cloudposse#2892 Wraps request/response errors in makeGitHubRequest with static errors from errors/errors.go instead of dynamic fmt.Errorf roots. Makes isRetryableGitHubStatus header-aware: distinguishes a rate-limited 403 (Retry-After / X-RateLimit-Remaining: 0) from a terminal one, and honors Retry-After / X-RateLimit-Reset when the wait fits the existing retry budget, failing fast rather than blocking the CLI for GitHub's full mandated cooldown on this best-effort auxiliary fetch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): retry the GitHub releases fetch behind `toolchain info` (round 2) Round 2 of CodeRabbit review feedback on PR cloudposse#2892, plus a matching gap found while investigating the attached CI failure logs: - pkg/toolchain/set.go: isRetryableGitHubStatus only decided *whether* to retry a rate-limited response; retry.Do still applied its own fixed ~300ms backoff regardless of what Retry-After specified, so a Retry-After: 1 response could be retried too soon and hit the same rate limit again. makeGitHubRequest is now a self-contained loop that sleeps the exact Retry-After/X-RateLimit-Reset duration when present and within budget, falling back to fixed exponential backoff only when no header supplies a wait. Also fixed a missing trailing period on a doc comment. - pkg/toolchain/registry/aqua: GetLatestVersion and GetAvailableVersionsContext had no retry at all for transient network failures, unlike the sibling getBytes helper in the same file, causing a separate CI golden-snapshot flake (atmos_toolchain_info_yaml_output resolving "latest" instead of a concrete version). Wired them through the same already-established, already-tested retry.TransientRetryConfig()/IsTransientNetworkError pattern via a new getBytesWithLinkHeader helper. Both come with regression tests: timing-based assertions for the Retry-After fix (which fail against the prior buggy behavior), and a mock transport simulating a connection reset for the aqua-registry fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): stop running TestTerraformRegistryCache twice on macOS/Windows It already runs once in its own dedicated step; the subsequent "Acceptance tests" step never excluded it (unlike Linux's coverage step), so it ran a second time concurrently with dozens of other tests that make real TLS calls. On macOS this races the System keychain trust-store mutations the test performs, destabilizing cert verification for the rest of the job and causing spurious "certificate signed by unknown authority" failures on unrelated tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): retry rate-limit and server-error statuses in Aqua registry fetches CodeRabbit review on PR cloudposse#2892 flagged that aqua.go's getBytes/ getBytesWithLinkHeader retry predicate only accepted IsTransientNetworkError, so a 429 or 5xx response was wrapped as a plain error the predicate doesn't recognize and returned after a single attempt — exactly the failure class the retry was added to fix. Extracts the GitHub rate-limit classification already written for set.go into pkg/toolchain/registry (IsRetryableGitHubStatus, GitHubSignalsRateLimit, GitHubRetryAfter, HTTPStatusError) so aqua.go reuses it instead of duplicating it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address CodeRabbit review feedback on PR cloudposse#2892 - Document the exported HTTPStatusError.Error and HTTPStatusError.Unwrap methods per Go documentation conventions. - Widen the X-RateLimit-Reset fallback test's interval from 3 seconds to 1 minute: the Unix-second truncation could zero out the wait if scheduling delayed the test past the 3-second boundary, flaking assert.Positive. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(merge): resolve deferred YAML functions once, not twice, in Stage 3. Add fix doc * fix(merge): resolve deferred YAML functions once, not twice, in Stage 3. Add fix doc * fix(merge): resolve remaining deferred-context gaps flagged in PR cloudposse#2892 review CodeRabbit's re-review of the deferred-YAML-function merge fix found two real gaps left over from the original PR: - describe_stacks.go has its own component processor that never went through processStacks (utils.go), so it never ran Stage 3 (resolveDeferredYamlFunctions). `atmos describe stacks` (bulk) still had the original cloudposse#2888 data-loss bug this PR claims to fix, even though `atmos describe component` (single) was already correct. Threads per-component deferred contexts from FindStacksMap into processComponentEntry and resolves them there. - terraform_generate_backends.go/terraform_generate_varfiles.go built ConfigAndStacksInfo.ComponentSection from a hand-picked subset of sections for Go-template rendering and Stage 3, silently omitting auth (and, for backends, required_providers/generate). A template referencing an omitted section rendered empty. Both now snapshot the complete merged section via a new cloneComponentSectionWithOverrides helper. Also corrects FindStacksMapForGenerate's doc comment, which inaccurately claimed "varfile/backend generation" as its rationale — that flow calls FindStacksMap directly; this wrapper's only caller is the bulk `generate:`-section preview, which never resolves YAML functions at all. Each fix ships with a regression test verified to fail without the fix and pass with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: address remaining CodeRabbit review feedback on PR cloudposse#2892 - cmd/emulator/completions_test.go: exampleProjectPath now fails loudly (require.NoError) instead of t.Skipf when the checked-in emulator-aws example is missing, so a moved/deleted fixture can't masquerade as a passing test. - internal/exec/deferred_contexts_test.go: build the fixture path with filepath.Join instead of a slash-literal string. - pkg/merge/deferred_test.go: Clone isolation tests now verify both mutation directions (clone->original was already covered; added original->clone for both DeferredValue fields and Path elements). - pkg/merge/merge_deferred_apply_test.go: fixed a mock comment that claimed each invocation yields a distinct value — it returns a constant; the `calls` counter, not the value, is what detects a spurious re-invoke. - pkg/merge/merge_deferred_walk_test.go: added !labels/!labels.keys/ !labels.values/!tags cases to the allowlist table (previously untested directly, only via integration tests). - tests/yaml_functions_integration_test.go: assert exact append order (assert.Equal) instead of assert.ElementsMatch for !tags precedence, and add real assertions (not just "loads without error") for 7 previously unexercised deferred-merge fixtures: nested-in-list, 3-layer type flip, scalar-overrides-map, default (replace) list_merge_strategy, nested parent/child function collision, an untracked (non-allowlisted) function still being clobbered by design, and a deferred function inside the backend section. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address remaining CodeRabbit review feedback, revert unsound double-execution fix attempt - internal/exec/describe_stacks_component_processor.go: wrap the mapstructure.Decode error in the Stage 3 settings-decode step with component/stack manifest context (%w), matching the existing error pattern in this file. Extract the Stage 3 block from processComponentEntry into a named resolveDeferredForComponent helper for readability (takes the settings map directly rather than the full componentSections struct, avoiding a gocritic hugeParam finding). - internal/exec/describe_stacks_test.go: use checked type assertions (require.True) for the intermediate map lookups instead of unchecked ones that would panic on failure. - tests/yaml_functions_integration_test.go: fix a comment referencing pkg/merge's internal postMergeFunctions variable and a hardcoded count instead of the canonical constants in pkg/utils/yaml_utils.go; explain why the expected map in one assertion is built via json.Unmarshal (float64 from !template's JSON decoding); rename a subtest that implied a deferred-merge collision it doesn't actually exercise. Also attempted and reverted a generalization of the Stage 3 double-execution fix (docs/fixes/2026-08-13-deferred-merge-double-execution.md) to cover collision paths where the higher-precedence layer is itself a function. The approach — a speculative pre-check via MergeDeferredValues on still-unresolved values — is unsound: a raw function string is always scalar-typed before execution, so the check cannot predict whether resolving it would produce a map needing to merge with a concrete sibling layer. Caught by tests/yaml_functions_integration_test.go's "deep merges with yaml function at higher precedence" case, which regressed silently past pkg/merge's own unit tests (they only exercised a scalar base, not the map-base case that actually broke). Full pkg/merge/merge_yaml_functions.go and merge_deferred_apply_test.go changes reverted to the last-known-good state; the collision-path double-execution remains open, documented in the fix log's Follow-ups with the failed approach recorded to save a future attempt from repeating it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com> Co-authored-by: aknysh <andriy.knysh@gmail.com>
Michael Pursifull (arcaven)
pushed a commit
to arcaven/atmos
that referenced
this pull request
Aug 15, 2026
…osse#2894) * feat(toolchain): add update command, fix version-pinning DX bugs Adds `atmos toolchain update` to move a pinned tool to its newest available version safely, and fixes several bugs found while field testing the update/pinning workflow: - which/exec resolved the wrong version on a multi-version .tool-versions line (last token instead of the default first token), causing false "not installed" errors. - set appended instead of replacing the default version, contradicting its own documented behavior. - add/install silently accepted SemVer range syntax (^1.2.0, ~>1.0.0) and only failed later with a raw HTTP 404; now rejected immediately with a hint toward dependencies.tools/atmos version track. - atmos version track add/set corrupted any value containing <, >, or & (a json.Marshal HTML-escaping bug), breaking exactly the ~>/>= constraint syntax the toolchain docs recommend. - atmos toolchain versions --help silently rendered the wrong command's help and exited 0 instead of erroring; fixed globally in root help routing. Removed the stale toolchain-versions and toolchain-aliases docs for commands that were never implemented. - Implemented six previously documented-but-missing flags: list --format/--installed-only/--pending-only, clean --dry-run/--cache-only/--force, exec --dry-run. - updateToolVersionsFile wrote to the hardcoded default .tool-versions path instead of the configured one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(toolchain): announce atmos toolchain update in changelog and roadmap Adds the changelog post for the new atmos toolchain update command and links it from the toolchain milestone list in the roadmap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): address CodeRabbit review on PR cloudposse#2894 Addresses 13 of 14 findings from CodeRabbit's review (the 14th, a doc link, was already correct): - Wrap addParser/cleanParser BindFlagsToViper errors with a new ErrFlagBinding sentinel instead of returning them raw. - Fix update/exec --dry-run flags reading the wrong command's env var (both register a Viper key literally named "dry-run", and BindEnv on a shared key lets the second parser's registration silently override the first's env var binding). Use IsBoolFlagExplicitlySet instead of v.GetBool so each command only reads its own flag/env var. - Add ErrToolchainCleanConfirmation sentinel for the clean confirmation prompt failure path; wrap update's .tool-versions load failure with ErrToolVersionsFileOperation. - Add a dry-run test that starts with no binary installed, confirming exec auto-installs before reporting instead of always assuming the tool is already present. - Fix `list --format=json` on an empty .tool-versions to emit a valid empty JSON document instead of only a human-readable message. - Fix `set` never validating the version before persisting it -- it could write invalid range syntax into .tool-versions the same way `add`/`install` could before this PR's earlier fix. - Fix update's exact-version path writing the new version into .tool-versions as the default *before* installing it; if install then failed, the configured default pointed at an uninstalled version. Install first, then persist. Add a failure-path test. - Rewrite the concurrency-order test to actually assert output order, and fix a real bug it caught: `atmos toolchain update` with no arguments iterated a Go map (randomized order) instead of a sorted one, so tools were reported in a different order on every run. - Wrap marshalJSONNoEscape's encode failures with the existing ErrEncode sentinel. - Fix an unrelated pre-existing bug the exec cast surfaced: command help/usage text rendered through pkg/ui/formatter.go's bare glamour renderer had no strict-linkify protection, so package/tool references like foo/bar@1.0.0 rendered with a stray mailto: link auto-attached. Export ApplyStrictLinkify for that renderer to use, and fix a latent ast.String Pos() issue in the linkify extension that could reorder or drop the replacement text once wired in. - Correct the toolchain-update.mdx doc's claim that ref: pins are immutable (a named ref can move; it's skipped to preserve the user's explicit source selection, not because it can't change), fix its dependencies.tools link to point at /stacks/dependencies, and widen uninstall's --all help text to match its actual dependency scope. Regenerate the affected casts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: trigger CI Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(agent-skills): trim atmos-toolchain SKILL.md under 500-line CI limit Removed content duplicated in references/commands-reference.md and the Custom Registries/Configuration sections; the CI "Validate agent skills structure and size" job flagged the file at 502 lines. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(agent-skills): document atmos toolchain update command Addresses CodeRabbit review comment: the toolchain skill's command list and reference doc omitted the update command introduced in this PR, including its --dry-run and --max-concurrency flags. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(toolchain): add `atmos toolchain lock` command Generates or refreshes toolchain.lock.yaml (checksum/provenance records) without reinstalling tools, for the use_lock_file workflow where a lockfile is wanted but a full reinstall isn't. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): read back the configured tool-versions path in install tests install_test.go read updated tool-versions state from the hardcoded DefaultToolVersionsFilePath constant instead of the tempDir-isolated path the test actually configured. This was masked by a matching bug in updateToolVersionsFile (already fixed) that made writes land on the same wrong path; once the write path was corrected, these tests started reading a stale/shared file and picking up unrelated content from CI's own toolchain install step, causing intermittent failures. Also regenerate the install --help golden snapshot, stale since the mailto-linkify fix landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): address CodeRabbit review on lock/install commands Fixes from PR cloudposse#2894 review: wrap lock command's flag-binding error with the static ErrFlagBinding sentinel, interpolate the actual configured tool-versions path (instead of a hardcoded ".tool-versions" literal) into RunLock's load-failure message, classify ParseToolSpec failures in resolveLockTargets under ErrInvalidToolSpec so callers can errors.Is() them, add a period to a dangling comment, and split `toolchain install`'s Long description away from its Example field so the two usage lines no longer render concatenated on one line in --help output. Also adds TestRunLock_ForceWritesLockFileWithoutInstalling, covering the force-write contract `atmos toolchain lock` exists to provide (lock file written and no binary installed even with toolchain.use_lock_file: false). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(casts): re-record atmos-toolchain-info--help screengrab Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 7 Dependabot alerts in website/pnpm-lock.yaml Bumps js-yaml (3.15.0->3.15.1, 4.3.0->4.3.1) and mermaid (11.16.0->11.16.1) pnpm overrides to patched versions. All are patch-level bumps within the allowed (non-major) range per .github/dependabot.yml's ignore policy. Fixes GHSA-5p4m-2wfm-xmqj (js-yaml quadratic CPU in !!omap resolution, high severity, alerts cloudposse#268/cloudposse#269) and GHSA-rhh3-jpg6-66xh/GHSA-c4c3-pg64-4m4v/ GHSA-6x64-9x62-f2gx/GHSA-3rrr-jr9j-h3q3/GHSA-2v8p-3f2j-5mp7 (mermaid, medium/low severity, alerts cloudposse#263-cloudposse#267). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(toolchain): assert the actual lock entry, not raw substring containment TestRunLock_ForceWritesLockFileWithoutInstalling only checked that "checksum_algorithm" and "terraform" appeared somewhere in the raw lock file bytes, which would still pass if the requested hashicorp/terraform@1.11.4 entry itself had no checksum. Decode with lockfile.Load and assert the specific entry's version, checksum algorithm, and non-empty checksum for the current platform. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): fix version-pinning replace semantics and lock-file multi-version bugs Found via a field-test pass of `atmos toolchain update`/`atmos toolchain lock`: AddVersionToTool(asDefault=true) only prepended a new version instead of replacing the line, so `set`, `add --default`, and `update` all left stale versions permanently pinned alongside the new one (existing tests encoded this as correct behavior, checking versions[0] but never the full slice). Fixed to fully replace the line, matching asdf's own `set` convention (its docs describe `asdf set <tool> <version>` as equivalent to `echo "<tool> <version>" > .tool-versions`). toolchain.lock.yaml's schema keyed tools by owner/repo with a single flat Version field, so locking a second version of an already-locked tool silently discarded the first's checksum -- a real risk since .tool-versions can legitimately pin multiple versions of one tool (e.g. examples/toolchain's "yq 4.45.1 4.50.1"). Restructured Tool to hold a nested Versions map so each locked version's data survives independently, and bumped lock_file_version to 2. `install` never verified a freshly downloaded checksum against what was already recorded in the lock file, despite `lock`'s own warning implying it would once toolchain.use_lock_file is enabled -- a tampered or corrupted lock entry was silently overwritten instead of failing. Added a verifyAgainstLock installer field (true for config-driven installs, false for `lock`'s own force-write/refresh path) and a checksum-mismatch check before any lock entry is overwritten. Also: resolveLockFilePath's default fallback now matches GetInstallPath()'s XDG-cache-first chain instead of a hardcoded relative ".tools" (they previously resolved to different directories by default); `add`'s error wrapping used a double-%w fmt.Errorf that silently discarded cockroachdb/errors hints attached via errUtils.Build, now uses WithCause so the range/constraint-syntax rejection hint actually reaches users; and `update`'s --help text no longer claims ref:-pinned tools are "immutable by design" (a named ref can move; it's skipped by choice). Nine regression tests were written and confirmed failing before each fix landed. Fixing the shared replace-semantics helper and the lock schema surfaced two more affected consumers not caught by the original field test: pkg/toolchain/filemanager's ToolVersionsFileManager/LockFileManager (unwired, but still compiled against and tested), whose own tests also encoded the same bugs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): fix version-compare bug, add live batch progress to update/lock A tool pinned with a literal "v" prefix (e.g. "peteretelej/tree v1.3.0") was always reported as "updated" even when nothing changed, because pkg/github.GetReleaseVersions strips the "v" from fetched release tags but updateExactPinnedTool's newest == current check never normalized the pin the same way. This silently triggered a real, unnecessary reinstall and rewrote .tool-versions, and miscounted the summary line. Fixed by normalizing both sides with the existing normalizeVersion() helper before comparing. update/lock printed zero output while a concurrent batch ran (network- bound, can take tens of seconds), then dumped every result at once when the last worker finished -- indistinguishable from a hang. Added pkg/toolchain/batch_progress.go: a generic, reusable runConcurrentBatchWithLiveProgress, modeled directly on install.go's existing batchEvent/batchRenderer (a worker pool + event channel + single collecting goroutine as the sole terminal writer -- the only place in this codebase that already solves "N concurrent items, one live-updating display" safely; lock.go's own spinnerControl is explicitly documented and avoided as unsafe for concurrent use). update/lock now show the same live spinner-per-tool + N/M progress bar batch UI atmos toolchain install already has, with each tool's line printed live as it completes instead of buffered to the end. This trades away the old target-order-printing guarantee for completion-order live output, matching install's own convention -- a deliberate choice, covered by new tests asserting the narrower guarantee that survives (per-item results never misattributed regardless of completion order). Also: stripped the hand-rolled checkmark/cross glyphs from status messages now that they're never re-printed via the plain ui.Writef; the summary line now omits zero-count categories instead of always printing "0 skipped, 0 failed" noise. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(toolchain): bump pinned terraform/opentofu versions Reflects a real `atmos toolchain update` run: hashicorp/terraform 1.15.6 -> 1.15.8, opentofu/opentofu 1.12.2 -> 1.12.5. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): exclude goo.gle and playwright.dev from link check Both URLs (docs/prd/atmos-ai-local-providers.md, docs/prd/saml-browser-driver-integration.md) intermittently reset connections from GitHub-hosted runners ("Connection reset by peer"), causing the Check Markdown Links job to fail with 2 errors. Both verified to return 200 outside CI via curl -- same class of CI-runner-hostile host already documented for taskfile.dev, geminicli.com, concourse-ci.org, etc. in this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): fix Windows CI deadlock in live-renderer batch tests captureCleanTestOutput redirects stderr to an os.Pipe that's only drained after the tested function returns; combined with force-tty activating the new ticker-driven live batch renderer, repeated writes filled the pipe's bounded buffer and blocked forever once a batch ran more than a few seconds, hanging TestRunLock_ReportsAllTargets and TestRunUpdate_ConcurrentAllSkippedReportsEveryTarget for the full 40-minute CI timeout on Windows. Added captureUITestOutput, a buffer-backed helper that never blocks on write, and switched both tests to use it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(website): remove duplicate nanoid pnpm override breaking lockfile Two independent commits merged from main each added the same nanoid@^3.3.16 override to package.json; pnpm's lockfile writer emitted it twice as sibling YAML keys, which is invalid YAML and made pnpm install --frozen-lockfile fail with ERR_PNPM_BROKEN_LOCKFILE in the website-deploy-preview CI job. Removed the duplicate and regenerated pnpm-lock.yaml. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(toolchain): close patch-coverage gaps flagged by Codecov (72% -> 93.6%) Adds real, behavior-asserting tests for uncovered branches across pkg/toolchain (batch_progress.go, update.go, clean.go, lock.go), cmd/toolchain (add.go, list.go, lock.go, update.go), pkg/toolchain/installer, pkg/ui/markdown/extensions (linkify.go), and several smaller touched packages -- no coverage theater, genuinely untestable defensive branches are left uncovered and documented inline. Also adds an askCleanConfirmationFunc seam in pkg/toolchain/clean.go (mirroring the existing isTTYForStdoutFunc seam) so confirmClean's post-TTY-check dispatch is testable without a live /dev/tty, and fixes a latent test-hygiene bug where pflag.Flag.Set() always marks Changed regardless of value, leaking flag state between subtests in the new cmd/toolchain RunE tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): strip ANSI before asserting on batch-progress test output TestLiveBatchRenderer_StartTickCompleteRenderAndClear and TestLiveBatchDisplay_WithRenderer_DelegatesEveryMethod failed on CI (both linux and macos) because ui.Success styles a completed line's label and trailing message as separate color runs, splitting a literal "tool-a done"/"tool done" substring match whenever CI's color profile differs from local. Ported the existing ansiEscapeRE/stripANSI pattern from cmd/secret/handler_helpers_test.go so the assertions are robust to the ambient color profile. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(toolchain): add dedicated usage markdown for toolchain lock --help install.go already has cmd/markdown/atmos_toolchain_install.md, which overrides its inline Example field at --help render time via the generic embed+override mechanism in cmd/markdown_help.go and cmd/root.go. lock.go had no equivalent file, so its help text still fell back to the inline Example text. Added the missing markdown file to match the established convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): fix left-padding in toolchain lock --help usage markdown The Validate EditorConfig pre-commit hook (and CI's Validation (affected) job, which runs the identical atmos validate --affected command) failed on cmd/markdown/atmos_toolchain_lock.md: its code blocks used a single leading space before "$", which .editorconfig's indent_size=2 for *.md requires to be a multiple of 2. Every sibling atmos_toolchain_*.md file has this same single-space pattern but was never touched in this diff, so the affected-only check never flagged them. Removed the leading space in this file (matching atmos_toolchain_exec.md's zero-space convention) rather than touching every other file out of scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): address CodeRabbit review findings + Windows CI isDangerousPath bug - installFromTool now rejects a lock-file checksum mismatch before extraction, not after: the old ordering could install a tampered binary before returning the error, defeating use_lock_file's supply-chain guarantee. - lockfile.Load now migrates v1-shaped tool entries (every released version through v1.226.0-rc.4) into the current nested Versions shape, instead of silently dropping every recorded checksum on upgrade. - appendToolchain (SBOM generation) now guards against nil tool/ version/platform entries instead of panicking on a hand-edited or corrupted toolchain.lock.yaml. - filemanager.LockFileManager.AddTool rejects an empty version instead of writing a bogus "" lock entry; RemoveTool's version-mismatch error sorts the locked-versions list for deterministic output. - cmd/toolchain's list/lock/update RunE tests restore the prior global Atmos config in cleanup instead of clearing it to nil. - pkg/toolchain/clean_test.go's chmod-based permission tests now also skip when running as root (not just on Windows). - Fixed isDangerousPath (pkg/toolchain/clean.go): filepath.Clean is OS-native, so the old Unix-flavored checks silently failed to recognize Windows root/drive-root paths as dangerous, failing TestIsDangerousPath on the Windows Acceptance Tests CI leg. - Reworded field-test skill's live-progress verification guidance to separate the pseudo-TTY check from the piped ANSI-styling check. See docs/fixes/2026-08-11-coderabbit-review-toolchain-lockfile-findings.md and docs/fixes/2026-08-11-isdangerouspath-windows-clean-behavior.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): address second CodeRabbit review pass - lockfile.Load's migration-failure error now wraps a new static sentinel (ErrLegacyLockFileMigrationFailed) instead of a bare fmt.Errorf, so callers can errors.Is() it, per CLAUDE.md's error handling mandate. Message text is unchanged. - Corrected two more gofmt -> gofumpt wording slips in fix-log validation entries. - Reworded the isDangerousPath fix-log's validation note to distinguish "diagnosed via Go source tracing" from "reproduced" -- the Windows-specific failure was never actually reproduced on macOS, only its root cause traced and the fix validated there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(toolchain): guard nil lockfile entries --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Michael Pursifull (arcaven)
pushed a commit
to arcaven/atmos
that referenced
this pull request
Aug 19, 2026
…cloudposse#2879) * test(container): cover combined buildx driver/cache/tags/context args Strengthens pkg/container's pure arg-building test with a case combining engine, driver, cache, custom dockerfile/context, and tags in a single config, closing the one remaining gap versus per-field-only coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(git): tolerate config errors for CI git-clone bootstrap pre-Cobra atmos git clone in a fresh CI workspace (no atmos.yaml yet, e.g. a profile referenced by CI config) failed with "profile not found" before ever attempting the clone, and ATMOS_CI=true had no effect. Execute() runs an initial cfg.InitCliConfig before Cobra resolves any command; only the second, PersistentPreRun-scoped InitCliConfig call knew how to tolerate the CI bootstrap clone's expected missing config (applyCIGitCloneBootstrap), so the first call's error aborted the process before that check could run. Add isCIGitCloneBootstrapArgs, an os.Args-based equivalent of the existing cmd-aware bootstrap check, so the pre-Cobra handler recognizes the same no-argument `atmos git clone` shape and defers to the same ATMOS_CI/CI-provider resolution (via the new exported CIGitCloneModeRequestedFromEnv) before Cobra ever parses the command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): record CI git-clone bootstrap profile fix Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(git): parse CI bootstrap flags with real pflag arity, not a heuristic The pre-Cobra CI git-clone bootstrap check (added in the prior commit) disqualified the bootstrap on any bare, non-"-"-prefixed token, including a space-separated flag value like the "0" in `--depth 0`. That misread a value-taking flag's argument as a positional repo name/URI, so the exact reported reproduction (`atmos git clone --ci --depth 0` in a fresh CI workspace) still failed on "profile not found". Replace the heuristic with CIGitCloneBootstrapRequestedFromRawArgs, which parses the clone-specific args against a throwaway command carrying the real clone flag set (a fresh newCloneParser() instance, never the shared singleton) via actual pflag parsing, then defers to the existing CICloneBootstrapRequested. This also lets an explicit --ci/--ci=false in the raw args be honored before Cobra resolves the command, which the removed env-only CIGitCloneModeRequestedFromEnv could not do. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): update CI git-clone bootstrap fix record for pflag rewrite Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(schema): decode with: into Build/Run/Push/Inspect for custom commands Fixes cloudposse#2876. A custom command's `type: container` step with a `with:` block (engine, driver, cache, tags, etc.) silently dropped everything, falling back to a bare `docker build -f Dockerfile .`, when loaded from a commands.yaml merged into atmos.yaml's Viper config tree. Root cause: `with:` is polymorphic -- decoded into Build/Run/Push/Inspect for `type: container` steps, or the generic With map otherwise -- but that promotion lives entirely in Task.UnmarshalYAML/WorkflowStep.UnmarshalYAML (go-yaml's yaml.Unmarshaler interface), invoked only when something calls yaml.Node.Decode directly (e.g. standalone workflows/*.yaml files via pkg/utils.UnmarshalYAMLFromFile). Custom commands merged into atmos.yaml decode via Viper's mapstructure pipeline (TasksDecodeHook -> decodeTaskFromMap), which never invokes yaml.Unmarshaler and had no equivalent promotion, so `with:` only ever reached the raw generic map. decodeTaskFromMap now pulls `with:` out before the mapstructure decode and replays the same polymorphic decode via decodeStepWith, round-tripping the value through YAML so both code paths share one implementation and can't drift apart. Reproduced through the real production paths per the bug report's request: config loaded via InitCliConfig (pkg/config), and the full custom command executed via RootCmd through a fake logging docker executable (cmd/) -- not by manually constructing schema.Task/WorkflowStep/ContainerBuildStep literals, which would have bypassed the actual decode bug. Added a complementary test proving workflow-file and custom-command steps decode with: identically, per the report's public-contract requirement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): sanitize nested component names in BuildPath A component name containing "/" (e.g. a nested layout like ecs/cluster) made workdir.BuildPath produce a real extra subdirectory instead of a single path segment, since the name was interpolated into "<stack>-<name>" without escaping and then filepath.Join'd. That put the nested component's workdir one level deeper than a flat component's at the same stack. Any path computed relative to the workdir -- most visibly a relative `backend.local.path` template like `../../../.context/tfstate/...` -- therefore climbed to a different real ancestor for the nested component than for the flat one, silently writing state under a different root (<repo>/.workdir/.context/... instead of <repo>/.context/...) even though both components used the identical backend config. Sanitize the component name the same way internal/exec/terraform_generate_ backends.go already does for backend template context: replace "/" with "-" before building the workdir directory name. BuildPath is the single formula reused by the source provisioner and by internal/terraform_backend's JIT-workdir state lookup, so both pick up the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): record custom-command with: and workdir path-depth fixes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(terraform/output): retarget containment-guard test at stack traversal; surface cached output lookups BuildPath now sanitizes "/" out of component names, so the containment guard test's traversal-via-component vector no longer escapes BasePath. Retarget it at the stack argument, which isn't sanitized the same way and still needs the guard. Also make cache-hit output lookups emit the same visible "Fetching ..." notification a real fetch would, instead of only a Debug-level log, so a second output lookup on an already-cached component isn't silently invisible. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(terraform/output): strip ANSI before asserting cache-hit visibility; correct fix-log formatter name CI forces color output (CI=true), which makes the markdown-based UI renderer split "Fetching vpc_id ..." into multiple ANSI-styled runs right at the literal underscore, without dropping or reordering any visible characters. Strip ANSI before the assert.Contains checks, matching the ansi.Strip convention already used elsewhere in the test suite. Also correct the fix-log's "gofmt" validation bullet to "gofumpt", the formatter this repo actually mandates and runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(claude): deny gofmt in Claude Code permissions Repo mandates gofumpt, not gofmt (CLAUDE.md, .golangci.yml). Denying the raw command prevents Claude Code from running gofmt directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: close Codecov patch-coverage gaps on PR cloudposse#2879 Adds behavior-focused tests for the 5 lines Codecov flagged as uncovered on this branch's added code: isCIGitCloneBootstrapArgs's len(args) < 1 guard, decodeTaskFromMap/decodeStepWithFromMapValue's three error-wrap branches (invalid container action, yaml.Marshal failure via a yaml.Marshaler that errors, yaml.Unmarshal failure via a dangling YAML alias), and resolveOutputFromCache's cache-miss and getOutputVariable- error branches. No production code changes; no assertions weakened. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): record terraform/output CI fixes; correct gofmt->gofumpt typo Two fixes from this branch (cache-hit output lookups now visible; containment-guard test retargeted at the still-open stack-traversal vector after the workdir fix closed the component-name one) had no docs/fixes/ record. Also corrects a stale "gofmt" mention in the git-clone-ci-bootstrap doc to "gofumpt", matching the correction already applied to the container with-block doc. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(registry): widen timing margin in provider-mirror concurrency test Fixes a flaky Windows Acceptance Tests failure: resolving 10 platforms took 784ms against a 750ms threshold, even though that's nowhere near the 1.5s serial floor the test guards against. Raises the bound to 4/5 of the serial floor (1200ms) for headroom against normal CI timing variance, Windows runners especially. No production code changed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): sanitize nested component names in createWorkdirDirectory createWorkdirDirectory duplicated the unsanitized stack-componentName formula that BuildPath was already fixed to sanitize, so a local (non-source) component with provision.workdir.enabled: true and a nested name still got a workdir one level deeper than a flat sibling, silently shifting where relative backend.local.path state resolves. Now delegates to BuildPath so both formulas can't drift apart again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner): guard path traversal in source-vendoring fallback DetermineTargetDirectory's non-workdir fallback (the default vendoring path when provision.workdir.enabled is unset) joined the component base path with the raw component name with no containment check, so a component named with ../ segments could vendor outside components/terraform/. Adds the same absolutize-and-prefix containment guard already used by the two other BuildPath-derived callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cli): decode and execute custom-command step-level container: overrides Sibling gap to the with: block fix (docs/fixes/2026-08-05-custom- command-container-with-block-dropped.md): a custom command step's container: override went through three independent failures. The bare boolean opt-out (container: false) broke InitCliConfig for the whole atmos.yaml because decodeTaskFromMap never round-tripped container: through YAML the way with: now does. The mapping form decoded fine but was never consulted at execution time -- the custom-command step loop always ran type: shell steps on the host. And once both of those were fixed, container: false still ran the step inside a container because cloneCommand's JSON round-trip silently dropped WorkflowContainer.Enabled (json:"-"), inverting the opt-out. Fixes all three: decodeTaskContainerFromMapValue mirrors the with: fix's round-trip for container:, cmd/cmd_utils.go's step loop now reuses the same pkg/workflow/container.go session logic the workflow-file path already uses, and WorkflowContainer gained MarshalJSON/UnmarshalJSON so Enabled survives a JSON round-trip. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(container): pass restart/healthcheck through to ephemeral run steps ContainerRunStep.Restart/.HealthCheck decoded fine but EphemeralConfig (the runtime config for type: container, action: run steps) had no such fields, and buildRunConfig never populated them -- unlike the persistent-component path, which already wires the same settings. Adds the fields to EphemeralConfig and populates them via the existing (previously unused for this path) RestartPolicyFromStep/ HealthCheckFromStep helpers, so --restart/--health-* flags now reach the real docker/podman invocation for step-based container runs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(config): surface swallowed import errors and hide-nothing validation Two DX gaps where already-computed diagnostic detail never reached the default log level: a YAML syntax error in an import:-loaded commands file was silently swallowed (Debug-only), leaving only a generic "Unknown command" with no hint a config file failed to parse; and container-step validation (missing required field, invalid pull: value) already computed the field/step/type and the bad value but only exposed them via --verbose or dropped them entirely. LocalAdapter now pairs its existing log.Debug with a ui.Warning naming the file and parse error. ValidateRequired's default message now names the field; invalidContainerField now echoes the actual invalid value typed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 7 Dependabot alerts in website dependencies Bumps three pnpm.overrides pins to their patched releases, all within the major-version line dependabot.yml's ignore policy allows: - js-yaml 3.15.0 -> 3.15.1 (GHSA-5p4m-2wfm-xmqj, alert cloudposse#269) - js-yaml 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, alert cloudposse#268) - mermaid 11.16.0 -> 11.16.1 (GHSA-rhh3-jpg6-66xh cloudposse#267, GHSA-c4c3-pg64-4m4v cloudposse#266, GHSA-6x64-9x62-f2gx cloudposse#265, GHSA-3rrr-jr9j-h3q3 cloudposse#264, GHSA-2v8p-3f2j-5mp7 cloudposse#263) Verified via `pnpm run build` in website/; NOTICE unchanged (no license changes from these patch bumps). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(schema): reject unknown fields in container step with:/driver: blocks The JSON Schema for a container step's with: block already documented it as "validated by the step handler at run time," but nothing fulfilled that promise for unknown keys -- yaml.Node.Decode (used by both the workflow-file and custom-command loading paths) has no strict/KnownFields mode, so a typo'd field like `platforms:` was silently dropped with no error. This masked a real pre-existing test bug: TestWorkflowStep_DecodeWith's push-action case used `tag: v1` instead of the actual `tags: []string` field and passed anyway. decodeYAMLInto and ContainerDriverConfig.UnmarshalYAML now decode through a stream-level yaml.Decoder with KnownFields(true) (the only place go-yaml exposes strict decoding) instead of plain node.Decode, scoped narrowly to the typed container structs -- the generic with: map[string]any fallback other step types use is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 5 Dependabot alerts, 2 unpatched and deferred - github.com/go-git/go-git/v5 5.19.1 -> 5.19.2 (GHSA-hc8v-wwc9-vgxm cloudposse#270, GHSA-qgq7-7hm3-q39j cloudposse#271), pulling in patch bumps to golang.org/x/{mod,net,text,tools} via go mod tidy - nanoid pnpm override widened from a pinned 3.3.3->^3.3.15 mapping to a ^3->^3.3.17 range so every 3.x requester resolves past both vulnerable versions (GHSA-28wg-ghj8-5hjv cloudposse#274, GHSA-2v37-7h3g-55p8 cloudposse#273); previously two different 3.x versions were resolving simultaneously because the override only matched exact-version requests - dompurify pnpm override 3.4.12 -> 3.4.13 (GHSA-55q2-fjhq-7xh7 cloudposse#272) image-size alerts cloudposse#275/cloudposse#276 (GHSA-5p2g-fcmc-qvqq, GHSA-w3rx-r6r6-pgpr) have no first_patched_version yet in any release line -- left open, nothing to bump to. Verified via `go build ./...`, targeted go-git-consumer package tests, and `pnpm run build`; NOTICE regenerated (version-string changes only, no license changes). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd): update stale path assertions in container build argv test TestCustomCommandContainerBuildPassesWithBlockToDocker asserted the buildx argv contained bare "Dockerfile" and "app" strings. Since main's cloudposse#2880 (resolve relative paths against step.WorkingDirectory), context: and dockerfile: values in a with: block are correctly resolved to absolute paths before reaching docker, so the argv now contains the full resolved paths instead of the bare relative strings. Docker still receives the same file -- update the assertions to check for the resolved absolute paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner): pass OutputWriters in nested-workdir Provision test Merging origin/main's fix(terraform): prevent concurrent output corruption (cloudposse#2898) added a fourth provisioner.OutputWriters parameter to (*Service).Provision. Every call site main's own history knew about was updated by that commit, but this branch's own TestServiceProvision_NestedComponentName_SanitizesLikeBuildPath (added by an earlier, unrelated fix on this branch) didn't exist in main's history, so git's auto-merge had nothing to reconcile it against and left the stale three-argument call in place -- a compile failure only visible once this branch actually merges main, which is exactly what GitHub Actions' implicit PR-merge checkout does on every CI run regardless of whether this branch has locally merged yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(cmd): assert cache/driver flag values, not just presence TestCustomCommandContainerBuildPassesWithBlockToDocker only checked that --cache-from/--cache-to/--builder appeared somewhere in the argv, not that they carried the configured registry ref, mode=max, or driver. The driver: block also provisions a real Buildx builder via a separate `docker buildx create` invocation (pkg/container/docker.go's ensureBuilder) that the fake runtime already recorded but the test never inspected. Adds table-driven flag-value assertions covering both the create and build invocations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd): normalize marker path for cross-platform shell command markerPath comes from t.TempDir(), which contains backslashes on Windows. The step command string goes through Atmos's mvdan/sh interpreter, which treats \ as an escape character, so the redirection target would resolve to a mangled path. Convert to slashes and quote the path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner): sanitize backslash in workdir component names BuildPath sanitized "/" in a component name to prevent it from adding a real directory level, but not "\", which is Windows' actual path separator. A crafted or copy-pasted component name containing "\" (e.g. "..\\..\\evil") would let filepath.Join/Clean treat it as real ".."-traversal segments on that platform, escaping the intended workdir root. Sanitize "\" identically and unconditionally on every platform, matching the existing "/" handling, so a given component name's workdir path also stays identical across OSes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(runner): make step-name assertion independent, cover empty type The default-message test used step name "run" and field "run.image", so the step-name assertion could pass even if ValidateRequired dropped the step name entirely, since "run.image" also contains "run". Converted to a table with a distinct step name/field per case and no shared substrings, and added a step.Type == "" case to cover the previously-untested branch that omits the "(type ...)" clause. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(schema): reject unknown fields in container: override blocks WorkflowContainer.UnmarshalYAML's mapping branch used plain value.Decode, so a typo'd field (e.g. `imgae` instead of `image`) in a workflow-level or step-level container: block was silently discarded rather than rejected -- the same class of gap already fixed for with: blocks in decodeYAMLInto. Use the existing decodeYAMLKnownFields helper instead, and add regression coverage for both the workflow-file (yaml.Unmarshal) and custom-command (mapstructure + TasksDecodeHook) decode paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixtures): sync workdir-nested fixture with path-safety fixes The README still documented the pre-fix behavior for the app/local-nested "known bug" scenario and the ../escape-test-nowd path-traversal probe, which are both now fixed on this branch. Re-verified both scenarios for real (atmos terraform apply/source pull against the fixture) and updated the manual-testing steps and expected output to match: the nested local component now sanitizes to a sibling workdir, and the unguarded probe now fails with ErrPathTraversal instead of vendoring outside components/terraform/. Updated the .gitignore comment on the now-defensive-only escape-test-nowd entry accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): correct formatter name, markdown syntax, typos, spelling - gofmt -> gofumpt (the repository-required formatter) - double-backtick delimiter for a code span containing a literal backtick, which single backticks can't escape in Markdown - add `text` language identifiers to unlabeled fenced output blocks - "on a already-parsed" -> "on an already-parsed" - "macos" -> "macOS" in a CI platform list Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): correct macOS spelling in CI platform list Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner): use filepath.Rel for component base path containment The naive absBase+separator prefix check breaks when componentBasePath resolves to a filesystem root ("/" on Unix, "C:\" on Windows): absBase already ends in the separator there, so the literal absBase+sep prefix ("//" or "C:\\") never matches any real descendant, rejecting every valid target with ErrPathTraversal. filepath.Rel doesn't have this edge case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(schema): wrap WorkflowContainer JSON decode error UnmarshalJSON returned the raw json.Unmarshal error directly instead of wrapping it with a static error from errors/errors.go, so callers couldn't classify a WorkflowContainer JSON decode failure the way they already can for its YAML counterpart. Wrap with the existing ErrInvalidWorkflowContainer sentinel, matching UnmarshalYAML. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixtures): sync stack-manifest comments with path-safety fixes The comments on the three path-traversal probes in this fixture's dev.yaml still described pre-fix behavior (determineSourceTargetDirectory having no containment guard, DetermineTargetDirectory's non-workdir fallback having no sanitization, createWorkdirDirectory reimplementing an unsanitized formula) even though all three are now fixed on this branch. Updated to describe the current, correct expected behavior, matching the README sync in the prior commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(provisioner,cmd): close symlink containment gap and route script steps through container overrides - pkg/provisioner/source: validateWithinComponentBasePath now resolves symlinks in the existing portion of target/base paths before checking containment, closing a bypass where a symlink under componentBasePath pointing outside it passed the old lexical-only check. - cmd/cmd_utils: type: script custom-command steps now route through StepContainerOverride/RunStepContainerOverride like type: shell steps already do, instead of silently ignoring a step-level container: override. - tests/fixtures: drop the POSIX-only /dev/stderr log destination from the source-provisioner-workdir-nested fixture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd): propagate Cobra cancellation to custom-command step execution executor.Execute and both RunStepContainerOverride call sites in executeCustomCommand used context.Background() instead of cmd.Context(), so a Ctrl-C on the top-level invocation couldn't cancel an extended step handler or a container runtime operation. Derive one executionCtx from cmd.Context() (falling back to context.Background() for direct-test invocations), mirroring the existing depCtx pattern used for dependency graph execution in the same function. Addresses CodeRabbit review comment on PR cloudposse#2879. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): add missing comma after "e.g." in fix-log doc Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd,schema): address CodeRabbit findings on PR cloudposse#2879 Completes Cobra-cancellation propagation to the remaining context.Background() call sites in executeCustomCommand, wraps with:/container: decode failures with their static sentinel errors so errors.Is works regardless of which decode step fails, and extends cmd.NewTestKit(t) to restore RootCmd.Commands() between tests so custom commands registered by one test no longer leak into the next. Also fixes comment/doc-accuracy nits (godot periods, exported test-double doc comments, and two docs/fixes/*.md wording corrections). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(workdir): document BuildPath's separator sanitization in its doc comment CodeRabbit nitpick on PR cloudposse#2879: the doc comment didn't mention that both "/" and "\" are replaced with "-", only the inline comment did. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(cmd): add regression test for RootCmd.Commands() restoration Addresses CodeRabbit findings on PR cloudposse#2879: a dedicated table-driven test was missing for restoreRootCmdCommands (confirmed failing pre-fix, passing post-fix), plus a comment period and an inconsistent path-traversal probe description in a test fixture README. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir,schema): close BuildPath collision/traversal gaps, stop mutating caller task maps Addresses CodeRabbit findings on PR cloudposse#2879: - workdir.BuildPath encoded "/" and "-" identically, so components named e.g. "app/local" and "app-local" collided on the same workdir, sharing files, metadata, and Terraform state. Encode "/" and "\" as "--" instead. - BuildPath validated the component name but never the stack name, both of which come from user-controlled YAML; a crafted stack value could escape BasePath. Move a shared containment check into BuildPath itself so all six call sites get it, instead of the two ad hoc copies that existed before (and the four call sites that had none). - decodeTaskFromMap deleted "with"/"container" keys in place, which could mutate the caller's own map (e.g. Viper's live config tree) when earlier normalization steps returned it unchanged instead of a copy. Also converts a hard-coded JIT-workdir test case to table-driven per a separate nitpick on the same PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): fix EditorConfig indentation in workdir fix-log doc The numbered list's continuation lines used 3-space indentation (aligned under the "1. " marker), which fails the repo's indent_size=2 EditorConfig rule (want multiple of 2). CI's "Run pre-commit hooks" job caught this. Switched to the 2-space continuation indentation already used elsewhere in docs/fixes/. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir,cmd): make BuildPath's encoding fully injective, restore removed RootCmd commands Addresses a second CodeRabbit review pass on PR cloudposse#2879: - The prior "/" -> "--" fix for workdir.BuildPath's component-name collision was still not injective: a component literally named "app--local" collided with "app/local" (both encode to "app--local"). Replaced it with a fully injective scheme -- escape the literal hyphen ("-" -> "-h") before encoding separators ("/" or "\" -> "-s") -- so "-" never appears unescaped in the output and no two distinct component names can produce the same encoded path segment. This changes the on-disk workdir directory name for existing hyphenated components; updated every hardcoded expected-path assertion this touched across six packages, several switched to compute the expected path via the real BuildPath instead of a hand-rolled formula so they can't go stale the same way again. - restoreRootCmdCommands (added in an earlier pass on this PR) only removed commands added to RootCmd after a NewTestKit snapshot; a command present in the snapshot but removed mid-test (via RootCmd.RemoveCommand) stayed gone for every later test. It now also re-adds any snapshot command whose Parent() is no longer RootCmd. Also fixes a godot comment-period nit and stale sanitized-name comments in a fixture referencing the earlier "--" encoding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: update hardcoded workdir names for BuildPath's injective encoding Fixes CI failures on all three platforms (linux/windows/macos) from the previous commit: several tests hardcoded the pre-encoding workdir directory name (e.g. "dev-vpc-remote-workdir", "dev-null-label-exports", "test-producer-from-source") instead of the new "-h"/"-s" escaped form BuildPath now produces for hyphenated component names. These packages weren't covered by the test sweep before that commit landed: pkg/ci/plugins/terraform and the tests/ acceptance suite's JIT-source and source-provisioner-workdir tests. The pkg/git TestDefaultBranchAndGitHubRepository failure on the Windows run is unrelated -- a transient runner permission error on C:/Users/runneradmin/.gitconfig, not a code issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): give backslash its own escape token, route CleanWorkdir through BuildPath Addresses a third CodeRabbit review pass on PR cloudposse#2879: - escapeComponentNameForPath aliased "/" and "\" to the same "-s" token, so "ecs/cluster" and `ecs\cluster` still collided. The aliasing was meant to keep a component name's encoding OS-independent, but that's already guaranteed by the encoding being pure Go string processing (never delegated to path/filepath) -- so backslash now gets its own token ("-b") at no cost to that property. - CleanWorkdir had its own separate, unsanitized stack+"-"+component formula, never routed through BuildPath. It already couldn't find workdirs for "/"-containing components; the encoding fixes on this PR widened that to ordinary hyphenated components too. Now delegates to BuildPath like every other consumer. - TestCustomCommandStepContainerFalseOptOutRunsOnHost only proved the host command ran, not that docker was never invoked. Now installs the fake container runtime and asserts its argument log was never created. - Two other fix-log docs and this PR's own fix-log doc still described earlier, now-superseded encoding examples; updated for consistency. - Replaced a slash-delimited path literal with filepath.Join in a test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): handle filesystem-root basePath, de-tautologize workdir_path_test oracles containWithinBase's absBase+separator prefix check rejected every legitimate workdir path when basePath resolved to a filesystem root (absBase already ends in the separator there, so absBase+sep doubled up). Switch to filepath.Rel, mirroring pkg/provisioner/source/source.go's isWithinBase. Also replace pkg/component/workdir_path_test.go's BuildPath-derived expected paths with independent hand-computed literals: BuildAndResolveWorkdirPath calls the same BuildPath internally, so a setup+assertion pair that both called BuildPath would silently agree on a wrong path if the encoding ever regressed again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test: cover BuildPath's error-propagation branches across workdir consumers Adds behavioral tests for the (string, error) BuildPath signature change: stack-name path-traversal now returns errUtils.ErrPathTraversal instead of silently resolving, and every caller's new `if err != nil` branch needs its own test to prove it actually forwards/wraps that error rather than swallowing it. Also covers resolveExistingSymlinks's non-ENOENT propagation and buildWorkdirPath's empty-BasePath default, both left untested by the original patch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir,cmd,tests): contain stack traversal to component-type root, fix bootstrap flag inheritance, fix mock fixture marker pkg/provisioner/workdir/types.go: BuildPath validated the derived path against basePath only, but stack (unlike component) was never escaped before being folded into workdirName -- a stack like "../../components" resolves inside basePath while still escaping .workdir/<componentType>. Now also validates against the canonical per-component-type workdir root. cmd/git/bootstrap.go: CIGitCloneBootstrapRequestedFromRawArgs's throwaway Cobra tree only registered clone-specific flags, so an inherited global flag like --config made clone.ParseFlags reject the args and silently report no CI-bootstrap request. Now registers the real global persistent flags before parsing. tests/fixtures/.../components/terraform/mock/main.tf: was byte-identical to the vendored source-modules/mock/main.tf, including its "vendored" header -- the local component is never vendored, so its component_type marker couldn't distinguish which module actually produced a given state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir,cmd/git): address PR cloudposse#2879 CodeRabbit round and fix local-backend state loss on re-provision CodeRabbit review round on PR cloudposse#2879 (verified against current code, not just the diff it saw): - pkg/provisioner/source/source.go: attach underlying filesystem errors to symlink-resolution failures instead of discarding them. - pkg/provisioner/workdir/types.go: BuildPath rejects a stack name containing "/" or "\" instead of only checking containment after the fact, closing a workdir-collision gap (e.g. stack "team/../prod" aliasing stack "prod"). - pkg/provisioner/workdir/clean.go, cmd/terraform/workdir/workdir_helpers.go: CleanWorkdir/GetWorkdirInfo/DescribeWorkdir now honor atmos_component overrides via BuildPath. The CLI-wired DefaultWorkdirManager had its own separate, never-updated path formula that couldn't find any hyphenated component's real workdir at all -- fixed too. - pkg/provisioner/workdir/workdir.go: best-effort migration of a workdir found at the pre-escaping path onto the new encoded one, so upgrading doesn't orphan existing local state. - cmd/git/bootstrap.go: a malformed `atmos git clone --depth not-a-number` no longer gets masked by an unrelated config/profile error; Cobra's own flag-parsing error now surfaces as intended. Also fixes a real, separate bug found while testing the above: workdir sync was deleting local-backend Terraform state (terraform.tfstate) on every re-provision, since only provider lock files and the workspace-specific terraform.tfstate.d/ were protected from the sync's delete-orphaned-files pass. A local-backend component's state was silently gone after the second run. shouldSkipSyncFile now also protects terraform.tfstate, terraform.tfstate.backup, and .terraform.tfstate.lock.info. See docs/fixes/2026-08-17-pr2879-coderabbit-round-workdir-and-bootstrap-fixes.md and docs/fixes/2026-08-17-workdir-sync-deletes-local-backend-state.md for full details, and website/blog/2026-08-17-container-config-validation-and-workdir-path-encoding.mdx for the user-facing changelog. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(workdir): reject only '.'/'..' segments in stack, not every '/' CI caught a real regression from the previous commit's validateStackForPath: rejecting any stack value containing "/" broke cmd/terraform/migrate's own test fixtures (stack "deploy/test") and, transitively, three tests/cli_workdir_test.go fixtures for hyphenated component names. Only a literal "." or ".." path segment is an actual collision/traversal risk (filepath.Join's implicit Clean() can fold it away, aliasing e.g. stack "team/../prod" onto stack "prod"). A plain "/" without such a segment, like "deploy/test", is a real, already-supported nesting convention with no traversal risk -- it just becomes a real subdirectory, exactly as it always has. Narrowed the check accordingly. Also fixed tests/cli_workdir_test.go's testWorkdirShow/testWorkdirDescribe/ testWorkdirCleanSpecific fixtures, which hand-rolled a pre-escaping workdir path for a hyphenated component name instead of computing it via BuildPath -- the same class of drift the prior commit's CleanWorkdir fix addressed. testWorkdirShow/testWorkdirDescribe had been silently masking their own breakage via a weak assert.Contains check that passed on error output too; tightened to require.NoError. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor(cmd): extract shared container-override step helper The "shell" and "script" custom-command step cases each built an identical workflowPkg.ContainerStepParams and called RunStepContainerOverride, differing only in the workflowStep and the display command. Extracted into a shared runContainerOverrideStep closure. No behavior change: TestCustomCommandStepContainerOverrideRunsInsideContainer (shell), its _ScriptType variant, and TestCustomCommandStepContainerFalseOptOutRunsOnHost all still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd,workdir,terraform): address PR cloudposse#2879 CodeRabbit round 3 findings Propagate executionCtx to non-TTY shell steps and the atmos step type so Ctrl-C/prompt cancellation actually stops an in-flight custom-command step instead of letting it run to completion. Make migrateLegacyWorkdir fail closed on a rename error instead of silently creating a fresh empty workdir over an orphaned legacy directory that may hold real Terraform state. Make ExtractComponentPath propagate a BuildPath rejection instead of falling back to the source component directory, which could point Terraform at the wrong workdir on a rejected (traversal/invalid) stack. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd): make workdir clean/describe/show honor atmos_component overrides resolveComponentConfig passed the caller's already-loaded (processStacks=false) AtmosConfiguration into ExecuteDescribeComponent, which only does its own full stack-processing init when passed nil. That branch never ran, so component resolution always failed silently and every clean/describe/show call fell back to treating the component as its own instance name -- the exact failure mode atmos_component-override support exists to prevent. It now builds its own fully-processed config from the same CLI flag overrides (base-path, config, config-path, profile) the caller already derived, so overrides actually resolve. Adds three regression tests that execute the real command path against a real stack fixture and assert the manager receives the resolved atmos_component, replacing gomock.Any() assertions CodeRabbit flagged as too permissive to catch this. Also fixes two stale doc/comment references from the same review round (obsolete BuildPath encoding description; a stale fixture path in a test comment). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(cmd,provisioner): address PR cloudposse#2879 CodeRabbit round 4 findings Fixes four independent path-identity gaps in the vendoring/workdir subsystem: DetermineTargetDirectory's default vendoring target permitted resolving to the shared component-type directory itself (component name "." or "child/.."); shouldSkipSyncFile protected local-backend state files by basename, over-broadly excluding nested source files with the same name; migrateLegacyWorkdir could rename the wrong identity's directory since its legacy-name formula isn't injective across stack/component; and validateStackForPath's segment split silently dropped empty segments from a leading or repeated "/", letting a stack name alias another's workdir path. Also fixes a test helper that suppressed all panics instead of only the expected one, and corrects three stale doc references from earlier rounds. Skipped one invalid finding (adding perf.Track to pkg/schema/workflow.go would create an import cycle with pkg/perf). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zack-is-cool
pushed a commit
to zack-is-cool/atmos
that referenced
this pull request
Sep 8, 2026
…posse#2896) * feat(migration): add Makefile, Justfile, and Taskfile migration guides Extend the atmos-migration skill and docs to cover moving task-runner orchestration (Make, Just, Task) to Atmos custom commands and workflows, alongside the existing native-Terraform/Terraform-Workspaces coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(config): stop directory's own commands: inheriting unrelated .atmos.d subcommands A directory's own inline atmos.yaml commands: entry named the same as a command discovered from git-root .atmos.d (e.g. an unrelated outer project's dev tooling) silently inherited that command's subcommand tree and other subcommand-referencing fields such as default:. Treat a leaf command with no commands: key as fully authoritative instead of merging it field-by-field against the discovered default. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(validate): stacks validation no longer requires name_template/name_pattern createComponentStackMap derived a component's logical stack name via a stricter, older code path that predated zero-config filename-based stack naming (cloudposse#1934), so atmos validate stacks hard-failed on any repo that terraform plan, list stacks, and describe component already resolved stacks for fine, including this repo's own examples/native-terraform. Reuse resolveStackName's precedence (manifest name > name_template > name_pattern > filename) instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(migration): correct field-tested gaps in task-runner migration guides Field-testing the new Makefile/Justfile/Taskfile migration references against real fixtures and the real atmos binary surfaced several gaps: from-native-terraform.md's Shape B recipe used a component name that never resolved (component names must match the physical directory); workflows.base_path has no default and needs to be called out; an orphaned [private] Justfile recipe and Just's command-echo behavior weren't addressed; from-taskfile.md overstated the need for `import:` when atmos.d/.atmos.d is auto-discovered; and the migration docs sidebar order contradicted the pages' own sidebar_position values. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(migration): address CodeRabbit review on PR cloudposse#2896 - Use the terraform component name (matching the physical terraform/ directory) instead of the never-resolving infra in every single-directory Makefile/Justfile/Taskfile example, consistent with the from-native-terraform.md Shape B fix. - Reserve type: atmos for native Atmos verbs only in from-taskfile.md's Shape A guidance; calling another custom command still needs type: shell. - Fix from-justfile.md's Common Problems link fragment (verified against the actual github-slugger algorithm). - Stop telling readers import: is required for auto-discovered atmos.d/.atmos.d files in from-makefile.md and website/docs/migration/taskfile.mdx. - Document that workflows.base_path has no default in website/docs/migration/taskfile.mdx, matching the equivalent fix already applied to the agent-skill references. - Normalize from-native-terraform.md's odd-space (3/5/7) list-continuation and nested-YAML indentation to even, matching the EditorConfig multiple-of-2 rule applied to the other reference files earlier in this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * feat(commands): add hidden custom commands and --help=hidden topic Custom commands can now set `hidden: true` to stay runnable (directly, as a `default:` target, or from another command's steps) while dropping out of `--help` listings, completions, and the AI `atmos_list_commands` tool. This closes the gap the Just/Task/Make migration guides used to call "no match", where a `[private]`/`internal: true` recipe or task needed to be reusable across callers or invoked directly for debugging rather than folded into a single caller's step. Add a matching `--help=hidden` topic to reveal a command's hidden subcommands on demand; the default-help hint only mentions it when a command actually has one, to avoid cluttering the common case. Refresh the affected migration guides (website + agent-skills mirrors) to point at `hidden: true` instead of the old "no match" guidance, and add previously-missing coverage for Task's `internal: true` flag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(links): exclude reproducible-builds.org from link check The Check Markdown Links workflow failed on the SOURCE_DATE_EPOCH citation in docs/prd/archive-step.md with "Connection refused". The domain refuses connections from every network tested (CI, curl, and WebFetch), not just this path or CI specifically — an upstream outage, not a broken/moved link — so exclude it the same way other known-flaky external docs are already handled in this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(migration): address CodeRabbit review on PR cloudposse#2896 - lychee.toml: narrow the reproducible-builds.org exclude to the exact SOURCE_DATE_EPOCH path instead of the whole domain, so other links on that domain stay covered by the link check. - justfile.mdx/makefile.mdx/taskfile.mdx (+ agent-skills mirrors): the "after" Terraform-apply examples ran `terraform apply terraform`, confusing the atmos verb with a component literally named "terraform" that didn't match the shown legacy `terraform/` directory layout. Rename the placeholder component to `infra` and add a one-line note on where it maps to under `components.terraform.base_path`. - cmd_utils_test.go: document why the hidden-command tests' printf/ redirection is cross-platform (Atmos's TaskTypeShell runs through the in-process mvdan/sh interpreter, not the host shell) rather than replacing it — flagged as a platform-specific-binary risk, but it isn't one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(io): make LinePrefixWriter's cross-node line batches atomic TestExecuteTerraformConcurrentHooksUseNodeWriters was failing in CI (reproduced locally under `go test -race -count=200`, ~30% failure rate): concurrent nodes' hook output was interleaving mid-record instead of staying grouped per node. writeLine() acquired the shared writeMu once per line, but a single Write() call can flush multiple buffered lines at once (e.g. a \r-terminated segment held back by a prior Write, completed by the next). Between the two per-line lock acquisitions for one node's burst, another node's own burst could interleave into the shared writer. Fixed by collecting a burst's complete lines up front and writing them under one writeMu acquisition (writeLinesLocked), so a whole burst lands as one contiguous block. Preserves the existing partial-write-error retry behavior: a failed line and everything after it, plus any trailing partial content, are restored to the buffer for the next Write/Flush to retry. Verified with `go test ./pkg/scheduler/adapters/... -race -count=500` (0 failures, was reproducibly failing before) and the full pkg/io suite, race detector, 5x. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): remediate 7 npm Dependabot alerts in website/ Bump pnpm.overrides for transitively-pulled packages to their patched versions, all within the semver-major bump the dependabot.yml ignore policy blocks: - js-yaml 3.15.0 -> 3.15.1, 4.3.0 -> 4.3.1 (GHSA-5p4m-2wfm-xmqj, GHSA for the 4.x line): quadratic CPU consumption in !!omap resolution, fixes cloudposse#269, cloudposse#268. - mermaid 11.16.0 -> 11.16.1: fixes cloudposse#267 (radar diagram DoS), cloudposse#266 (config API prototype pollution), cloudposse#265 (CSS injection), cloudposse#264 (Architecture diagram prototype pollution), cloudposse#263 (XY Chart infinite-loop DoS). No open CodeQL alerts. Verified with `atmos lint --changed` (0 issues) and `npm run build` in website/ (succeeds, same pre-existing unrelated broken-anchor warning as before this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(io): preserve unwritten suffix on partial LinePrefixWriter writes Write and stdio.WriteString can return n > 0 with an error; the prior code ignored n and restored the entire raw line on retry, so bytes the underlying writer already accepted (including the prefix) could be resent. A nil-error short write (n < len(payload)) also silently dropped the unwritten tail. Track the encoded pending payload and retry only its unwritten suffix, converting a nil-error short write into io.ErrShortWrite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(website): repair broken pnpm lockfile and migration doc links website-deploy-preview failed on pnpm install --frozen-lockfile because a duplicate nanoid@^3.3.16 override (added independently by two commits and merged from main) produced a duplicate YAML key in pnpm-lock.yaml. Also fix 5 new migration docs linking to the nonexistent /ai/agent-skills route instead of /ai/skills, which broke the docusaurus build once the lockfile issue was resolved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(commands): qualify that internal commands don't appear in help Addresses CodeRabbit review comment: the `name` field description said names unconditionally appear in `atmos help`, contradicting the `internal` field's documented exclusion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(migration): fix stale no-parity claims for deps/freshness now shipped cloudposse#2882 (already merged into this branch) added dependencies.commands/ dependencies.workflows and step-level inputs/artifacts, giving Atmos direct parity with Task's deps:/sources:/generates: and Make's dependency ordering and file-timestamp caching. This branch's own migration guides -- the subject of this PR -- still declared those exact features unsupported gaps, written before cloudposse#2882 landed. - taskfile.mdx / from-taskfile.md: rewrite "parallel-by-default" and "sources/generates gap" sections to document dependencies.commands and inputs/artifacts as the direct matches, including the automatic dedup behavior a hand-built parallel step doesn't provide. - makefile.mdx / from-makefile.md: document dependencies.commands for target chains with a shared prerequisite, and inputs/artifacts (with timestamp.changed for make's exact mtime semantics) for file-timestamp targets. - SKILL.md: fix the same false claims in the top-level "Common Problems" summary agents read before the per-tool reference files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(migration): fix reviewer feedback on dependencies/freshness guidance Local review comments: - Recommend `atmos --help` (scriptable, direct) over the interactive `atmos help` (pages the same listing) for command discovery, across all three migration guides and their skill mirrors. - Reframe makefile.mdx around "Atmos is the front door either way": a custom command can call `make <target>` as its one step, permanently if desired, rather than treating full migration into native `steps:` as the only end state. CodeRabbit findings, verified against actual behavior before fixing: - GNU Make's default is to build a target's prerequisites one at a time, in listed order -- `-j` is required for concurrency. `dependencies.commands` runs concurrently by default, so presenting it as Make's/Just's "direct match" changes behavior and can race prerequisites that were only ever sequential by accident. Ordered steps are now the default-preserving match; `dependencies.commands` is reserved for a shared prerequisite (dedup, independent of concurrency), genuine independence, or an explicit `-j` source. Fixed in SKILL.md, makefile.mdx, and from-makefile.md, including a corrected Shape B example showing how to keep `build` ordered ahead of `test` even under the concurrent scheduler. - Confirmed in cmd/cmd_utils.go/internal/exec/workflow_utils.go that a skipped step just `continue`s the loop: inputs/artifacts freshness is evaluated and recorded per step, unlike Task's/Make's whole-recipe/task scope. Documented this across taskfile.mdx, from-makefile.md, and SKILL.md, with guidance to combine multiple commands into one step when a single freshness decision must gate all of them. - Removed the stale "target chains become workflows" claim, which contradicted the dependencies.commands guidance it now sits next to; workflows are reserved for fixed, multi-step orchestration across more than one component. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(gitignore): ignore cached tools/gomodcheck binary Mirrors the existing tools/lintroller/.lintroller entry for the mage lint helper's cached build output. * fix(docs): address CodeRabbit findings on atmos-migration skill docs Corrects eight documentation-accuracy issues flagged by CodeRabbit on the atmos-migration skill: prefer `type: atmos` over `type: shell` for calling another custom command (preserves stack context and structured output); map Make's `@` prefix to `show: { command: false }` instead of `output: none` (which discards stdout/stderr entirely); describe `build-all`'s `-j4` loop as sequential, not parallel; define a proper per-service `build-service` command for the Shape C matrix example; document the `metadata.component` no-move option for mapping a stack component onto an existing directory; scope "target chains become workflows" guidance to ordered custom-command steps instead; and fix the justfile.mdx examples to carry environment variables and per-environment Terraform vars across build/test/deploy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(docs): address second round of CodeRabbit findings on migration docs Makes the terraform-directory move optional (not required) in from-justfile.md, makefile.mdx, and taskfile.mdx; corrects the mischaracterization of Just's `{{ }}` interpolation as Go templates; clarifies that Task's `deps:` maps directly to the concurrent-by-default `dependencies.commands` rather than ordered steps; fixes a broken no-move `.tfvars` path example in justfile.mdx; stops mapping a single `$(MAKE) -C dir` invocation to `matrix` (reserving it for genuine `$(SUBDIRS)`-style loops); and documents that Atmos's `internal: true` (Cobra Hidden) does not block direct invocation the way Task's `internal: true` does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(docs): preserve per-environment tfvars contract in migration guides Both the Justfile source (-var-file=envs/{{env}}.tfvars) and Makefile source (-var-file=envs/$(ENV).tfvars) load per-environment Terraform variables, but the migrated `atmos terraform apply infra -s <env>` examples in from-justfile.md and makefile.mdx dropped that behavior -- `-s` only selects the stack, it doesn't load vars. Document the per-stack `vars: !include` mapping for both the moved-directory and no-move component layouts, matching the fix already applied to the sibling justfile.mdx doc. * fix(security): remediate 3 npm Dependabot alerts in website deps Pin transitive browserslist and postcss-selector-parser via pnpm.overrides to patched versions: - browserslist <= 4.28.6 (GHSA alerts cloudposse#281, cloudposse#282, high) -> ^4.28.7 - postcss-selector-parser >=6.1.0 <6.1.3 (alert cloudposse#280, low) -> ^6.1.3 Both are patch-level bumps within dependabot.yml's semver-major ignore policy. The postcss-selector-parser override is scoped to the ^6 line only (via postcss-calc's ^6.0.11 request) so it doesn't touch the separate, already-unaffected ^7.0.0 line used elsewhere. * fix(docs): add terminal periods to from-mise/from-aqua resource bullets --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
Use self-hosted runners everywhere.\n\nExcluded chages:\n- Readmes\n- Examples\n- Templates\n- Reusable actions from the catalog repo