feat(toolchain): add update command, fix version-pinning bugs - #2894
Open
Erik Osterman (Cloud Posse) (osterman) wants to merge 28 commits into
Open
feat(toolchain): add update command, fix version-pinning bugs#2894Erik Osterman (Cloud Posse) (osterman) wants to merge 28 commits into
Erik Osterman (Cloud Posse) (osterman) wants to merge 28 commits into
Conversation
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>
…dmap 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>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
August 6, 2026 18:59
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
minor
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds ChangesToolchain modernization
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: ⚪ Minimal · up to The PR is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/toolchain/install_validation_helpers.go (1)
49-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the validation comment.
ValidateVersionSpecaccepts literal vendor tags such asjq-1.7.1thatParseVersionSpecdoes not recognize. DescribeValidateVersionSpecin this comment.Proposed fix
-// version is a format ParseVersionSpec recognizes, before any network call is made. +// version is accepted by ValidateVersionSpec before any network call is made.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/install_validation_helpers.go` around lines 49 - 61, Update the comment above validateToolAndVersion to state that the version format is validated by ValidateVersionSpec, replacing the inaccurate reference to ParseVersionSpec while preserving the existing description of non-empty tool and version validation before network calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/toolchain/add.go`:
- Around line 46-48: Wrap the errors returned by addParser.BindFlagsToViper in
cmd/toolchain/add.go:46-48 and cleanParser.BindFlagsToViper in
cmd/toolchain/clean.go:29-33 with their applicable static errors from
errors/errors.go, preserving each original error using %w before returning it.
In `@cmd/toolchain/update.go`:
- Around line 35-40: Before reading v.GetBool("dry-run") in the update and exec
command flows, use StandardParser.IsBoolFlagExplicitlySet to determine whether
the command’s own CLI flag or registered environment variable was explicitly
provided. Apply this change in cmd/toolchain/update.go lines 35-40 and
cmd/toolchain/exec.go lines 35-40, ensuring each command does not consume the
other command’s dry-run environment variable while preserving the existing
dry-run behavior.
In `@errors/errors.go`:
- Around line 353-376: Add the static ErrToolchainCleanConfirmation sentinel in
errors/errors.go, wrap the confirmation prompt failure in pkg/toolchain/clean.go
using that sentinel and %w, and wrap the tool-versions load failure in
pkg/toolchain/update.go with ErrToolVersionsFileOperation and %w. Apply these
changes at errors/errors.go lines 353-376, pkg/toolchain/clean.go lines 138-143,
and pkg/toolchain/update.go lines 55-59 respectively, preserving the underlying
errors for errors.Is checks.
In `@pkg/toolchain/exec_test.go`:
- Around line 205-210: Update the test around RunExecCommandWithOptions to cover
a missing-binary dry-run case: do not pre-install terraform, invoke the command
with dry-run enabled, and assert the tool installation occurs while the injected
execFunc is never called. Prefer adding this as a behavior-focused table-driven
case using the existing fake tool setup.
In `@pkg/toolchain/list.go`:
- Around line 231-242: The empty-configuration path in RunListWithOptions must
emit valid JSON when format is "json". Before returning after the existing UI
message, write ListToolsOutput with an empty Tools slice via data.WriteJSON for
JSON output, while preserving the current table-output message; add a test
covering empty .tool-versions JSON output.
In `@pkg/toolchain/set.go`:
- Around line 343-347: In SetToolVersion, validate the selected version with
ValidateVersionSpec after interactive selection and before calling
AddToolToVersionsAsDefault. Prevent persistence when validation fails,
preserving the existing default-version replacement behavior for valid versions.
In `@pkg/toolchain/update_test.go`:
- Around line 160-171: The TestRunUpdate_ConcurrencyPreservesOrder test
currently verifies only the error result; capture RunUpdate’s reported output
and assert owner/a appears before owner/b and owner/c, with owner/b before
owner/c. Use the existing test I/O setup and keep the test focused on output
order under MaxConcurrency: 4.
In `@pkg/toolchain/update.go`:
- Around line 237-249: Reorder the update flow around
installSingleToolWithInstaller and AddToolToVersionsAsDefault so newest is
installed successfully before .tool-versions is changed. Preserve the existing
failure result for installation errors, and add a failure-path test verifying
the previous default remains configured when installation fails.
In `@pkg/version/manager/crud.go`:
- Around line 182-183: Update marshalJSONNoEscape to wrap enc.Encode failures
with the existing ErrEncode sentinel before returning them, so SetEntryFields
and AddEntry expose errors.Is-compatible encoding errors. Add a negative test
using an unsupported field value and assert that the returned error matches
ErrEncode via errors.Is.
In `@website/docs/cli/commands/toolchain/toolchain-info.mdx`:
- Around line 212-215: Update the Tool Aliases link in the toolchain-info
documentation to target the new toolchain aliases page, matching the route
defined by that page rather than the legacy aliases route. Remove the duplicate
legacy aliases page if it is no longer referenced or needed.
In `@website/docs/cli/commands/toolchain/toolchain-update.mdx`:
- Around line 36-38: Update the documentation bullet describing pr:, sha:, and
ref: pins to remove the claim that they are immutable and clarify that updates
skip all three to preserve the explicitly selected source; retain the existing
guidance to use atmos toolchain add to change them.
- Around line 44-47: Update the dependencies.tools link in the toolchain update
documentation to point to /stacks/dependencies, matching the configuration
reference used by toolchain usage documentation; leave the surrounding
version-tracking guidance unchanged.
In `@website/static/casts/screengrabs/atmos-toolchain-exec--help.cast`:
- Line 8: Fix the toolchain exec help rendering so examples contain only the
executable version reference, not an injected mailto link, after terraform@1.5.0
and kubectl@1.28.0. Update the source/renderer responsible for the exec
examples, then regenerate atmos-toolchain-exec--help.cast and verify copied
commands contain no mailto arguments.
In `@website/static/casts/screengrabs/atmos-toolchain-uninstall--help.cast`:
- Line 8: Update the uninstall command help text to mention tools installed via
component, workflow, and command dependencies, matching the existing dependency
wording. Then regenerate the atmos-toolchain-uninstall help cast so it reflects
the updated output.
---
Outside diff comments:
In `@pkg/toolchain/install_validation_helpers.go`:
- Around line 49-61: Update the comment above validateToolAndVersion to state
that the version format is validated by ValidateVersionSpec, replacing the
inaccurate reference to ParseVersionSpec while preserving the existing
description of non-empty tool and version validation before network calls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 243991f8-cabb-465b-97ec-cf333d4b27b1
📒 Files selected for processing (74)
agent-skills/skills/atmos-toolchain/SKILL.mdcmd/root.gocmd/root_help_routing_test.gocmd/toolchain/add.gocmd/toolchain/add_test.gocmd/toolchain/clean.gocmd/toolchain/clean_test.gocmd/toolchain/command_provider_test.gocmd/toolchain/exec.gocmd/toolchain/exec_test.gocmd/toolchain/list.gocmd/toolchain/list_test.gocmd/toolchain/provider_test.gocmd/toolchain/toolchain.gocmd/toolchain/update.gocmd/toolchain/update_test.godemo/casts/atmos.d/screengrabs/cli.yamlerrors/errors.gopkg/ai/tools/atmos/toolchain_add.gopkg/toolchain/add.gopkg/toolchain/add_test.gopkg/toolchain/clean.gopkg/toolchain/clean_test.gopkg/toolchain/exec.gopkg/toolchain/exec_test.gopkg/toolchain/install.gopkg/toolchain/install_validation_helpers.gopkg/toolchain/list.gopkg/toolchain/list_test.gopkg/toolchain/lockfile/lockfile.gopkg/toolchain/set.gopkg/toolchain/set_test.gopkg/toolchain/update.gopkg/toolchain/update_test.gopkg/toolchain/version_spec.gopkg/toolchain/version_spec_test.gopkg/toolchain/which.gopkg/toolchain/which_test.gopkg/version/manager/crud.gopkg/version/manager/crud_test.gotests/snapshots/TestCLICommands_atmos_toolchain_--help.stdout.goldenwebsite/blog/2026-08-06-toolchain-update-command.mdxwebsite/docs/cli/commands/toolchain/toolchain-aliases.mdxwebsite/docs/cli/commands/toolchain/toolchain-get.mdxwebsite/docs/cli/commands/toolchain/toolchain-info.mdxwebsite/docs/cli/commands/toolchain/toolchain-remove.mdxwebsite/docs/cli/commands/toolchain/toolchain-set.mdxwebsite/docs/cli/commands/toolchain/toolchain-uninstall.mdxwebsite/docs/cli/commands/toolchain/toolchain-update.mdxwebsite/docs/cli/commands/toolchain/toolchain-versions.mdxwebsite/docs/cli/commands/toolchain/usage.mdxwebsite/src/data/roadmap.jswebsite/static/casts/screengrabs/atmos-toolchain--help.castwebsite/static/casts/screengrabs/atmos-toolchain-add--help.castwebsite/static/casts/screengrabs/atmos-toolchain-aliases--help.castwebsite/static/casts/screengrabs/atmos-toolchain-clean--help.castwebsite/static/casts/screengrabs/atmos-toolchain-du--help.castwebsite/static/casts/screengrabs/atmos-toolchain-env--help.castwebsite/static/casts/screengrabs/atmos-toolchain-exec--help.castwebsite/static/casts/screengrabs/atmos-toolchain-get--help.castwebsite/static/casts/screengrabs/atmos-toolchain-info--help.castwebsite/static/casts/screengrabs/atmos-toolchain-install--help.castwebsite/static/casts/screengrabs/atmos-toolchain-list--help.castwebsite/static/casts/screengrabs/atmos-toolchain-path--help.castwebsite/static/casts/screengrabs/atmos-toolchain-registry--help.castwebsite/static/casts/screengrabs/atmos-toolchain-registry-list--help.castwebsite/static/casts/screengrabs/atmos-toolchain-registry-search--help.castwebsite/static/casts/screengrabs/atmos-toolchain-remove--help.castwebsite/static/casts/screengrabs/atmos-toolchain-search--help.castwebsite/static/casts/screengrabs/atmos-toolchain-set--help.castwebsite/static/casts/screengrabs/atmos-toolchain-uninstall--help.castwebsite/static/casts/screengrabs/atmos-toolchain-update--help.castwebsite/static/casts/screengrabs/atmos-toolchain-versions--help.castwebsite/static/casts/screengrabs/atmos-toolchain-which--help.cast
💤 Files with no reviewable changes (5)
- website/static/casts/screengrabs/atmos-toolchain-versions--help.cast
- website/static/casts/screengrabs/atmos-toolchain-aliases--help.cast
- website/docs/cli/commands/toolchain/toolchain-get.mdx
- website/docs/cli/commands/toolchain/toolchain-aliases.mdx
- website/docs/cli/commands/toolchain/toolchain-versions.mdx
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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/toolchain/update_test.go`:
- Around line 114-149: Make
TestUpdateOneTool_ExactPin_InstallFailureLeavesToolVersionsUnchanged
deterministic by mocking or injecting the installation step to fail locally
instead of relying on the real installer and unavailable upstream release.
Retain the mocked GitHub release list, exercise updateOneTool with the injected
failure, and keep asserting that the original tool version remains unchanged.
In `@pkg/ui/markdown/extensions/linkify.go`:
- Around line 86-125: Replace the global searchFrom-based lookup in
replacementTextNode and its caller with the current ast.AutoLink’s source
position, deriving the ast.Text segment directly from that node’s label/source
span so repeated labels bind to the correct occurrence. Update the affected
linkify logic in pkg/ui/markdown/extensions/linkify.go (lines 86-125) and adjust
the corresponding coverage in pkg/ui/markdown/extensions/extensions_test.go
(lines 627-650) to validate repeated identical links preserve their source
order.
- Around line 144-153: Update stringNodeRenderer.renderString to capture and
return the error from w.Write(str.Value) instead of discarding it, while
preserving the existing walk status and early-return behavior for non-entering
nodes and non-string AST nodes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 03f95944-a417-42a3-81ad-09e8b4fc7740
📒 Files selected for processing (25)
cmd/toolchain/add.gocmd/toolchain/clean.gocmd/toolchain/exec.gocmd/toolchain/uninstall.gocmd/toolchain/update.goerrors/errors.gopkg/toolchain/clean.gopkg/toolchain/exec_test.gopkg/toolchain/list.gopkg/toolchain/list_test.gopkg/toolchain/set.gopkg/toolchain/set_test.gopkg/toolchain/update.gopkg/toolchain/update_test.gopkg/ui/formatter.gopkg/ui/formatter_test.gopkg/ui/markdown/custom_renderer.gopkg/ui/markdown/extensions/extensions_test.gopkg/ui/markdown/extensions/linkify.gopkg/version/manager/crud.gopkg/version/manager/crud_test.gowebsite/docs/cli/commands/toolchain/toolchain-uninstall.mdxwebsite/docs/cli/commands/toolchain/toolchain-update.mdxwebsite/static/casts/screengrabs/atmos-toolchain-exec--help.castwebsite/static/casts/screengrabs/atmos-toolchain-uninstall--help.cast
🚧 Files skipped from review as they are similar to previous changes (15)
- website/static/casts/screengrabs/atmos-toolchain-uninstall--help.cast
- errors/errors.go
- website/static/casts/screengrabs/atmos-toolchain-exec--help.cast
- cmd/toolchain/clean.go
- cmd/toolchain/update.go
- pkg/toolchain/set.go
- website/docs/cli/commands/toolchain/toolchain-update.mdx
- website/docs/cli/commands/toolchain/toolchain-uninstall.mdx
- cmd/toolchain/add.go
- pkg/toolchain/update.go
- pkg/version/manager/crud.go
- cmd/toolchain/exec.go
- pkg/toolchain/clean.go
- pkg/toolchain/list_test.go
- pkg/toolchain/list.go
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
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>
Resource Changes Found for
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agent-skills/skills/atmos-toolchain/SKILL.md`:
- Around line 206-216: Update the common command list in the toolchain skill
documentation to include atmos toolchain update [tool...], noting its --dry-run
and --max-concurrency options or linking to the full update reference. Keep the
existing command entries unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a9979ff-fe9b-4adf-8d73-fda963a84b40
📒 Files selected for processing (1)
agent-skills/skills/atmos-toolchain/SKILL.md
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>
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>
…l 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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
pkg/toolchain/installer/lock_tool_test.go (2)
73-113: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winVerify the full no-op contract when lock files are disabled.
Installer.LockToolstill builds the asset URL, downloads the asset, and verifies it beforeupdateLockFileinpkg/toolchain/installer/installer.goLines 451-481. This test only checks that no lock file exists, so a regression that performs network I/O whileuseLockFileis false would still pass. Count requests and assert zero, or rename the test and comment to promise only that no lock file is written.As per coding guidelines, behavior-focused tests must assert the behavior named by the test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/installer/lock_tool_test.go` around lines 73 - 113, Strengthen TestLockTool_NoOpsWhenLockFileDisabled by counting HTTP requests handled by the test server and asserting the count remains zero after LockTool returns. Keep the existing no-error and no-lock-file assertions, ensuring the test verifies the complete no-op behavior when useLockFile is false.Source: Coding guidelines
39-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the existing-binary path.
The test starts with an empty
binDir, soassert.Emptyonly proves thatLockTooldid not create a new entry. It does not prove that an existing binary remains unchanged. SeedbinDirwith sentinel contents and assert that the contents remain unchanged after locking.As per coding guidelines, new features need comprehensive behavior-focused tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/installer/lock_tool_test.go` around lines 39 - 70, Update the LockTool test around the existing binDir setup to create a sentinel binary file with known contents before calling installer.LockTool. After locking, read that same file and assert its contents are unchanged, replacing the empty-directory-only assertion while preserving the lock-file verification.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/toolchain/lock.go`:
- Around line 63-84: Add GoDoc comments to the exported methods on
LockCommandProvider: GetCommand, GetName, GetGroup, GetFlagsBuilder,
GetPositionalArgsBuilder, and GetCompatibilityFlags. Start each comment with the
corresponding method name and briefly describe its returned command, name,
group, flags builder, positional-argument builder, or compatibility flags.
- Around line 55-57: Update cmd/toolchain/lock.go lines 55-57 by introducing an
injectable lock-runner interface or dependency and have runLock invoke it while
preserving argument and MaxConcurrency forwarding. In cmd/toolchain/lock_test.go
lines 19-48, add table-driven mock-runner tests covering default and explicit
concurrency, invalid concurrency values, and multiple tool arguments; both sites
require changes.
- Around line 25-27: Replace the inline Example text in the toolchain lock
command with a dedicated cmd/markdown/*_usage.md usage file, embed that file
using //go:embed, and render the embedded content through
utils.PrintfMarkdown().
- Around line 46-47: Update the error return in lockCmd after
lockParser.BindFlagsToViper fails to wrap the underlying error with fmt.Errorf
using errors.ErrWrapFormat and errors.ErrFlagBinding, preserving the original
error while adding command-context classification.
In `@pkg/toolchain/install_test.go`:
- Line 334: Update the comment near the tool registration test to end with a
period after “resolution,” preserving the existing wording and meaning.
In `@pkg/toolchain/lock_test.go`:
- Around line 73-96: Add a success-path test for RunLock using a valid tool
fixture and LockOptions with UseLockFile: false. Assert the configured lock file
is written with the expected contents and verify that no binary installation
occurs, covering the command’s force-write behavior without relying only on
helper or failure-path tests.
In `@pkg/toolchain/lock.go`:
- Around line 52-55: Update the error returned by the tool-versions loading flow
around GetToolVersionsFilePath and LoadToolVersions to report the configured
filePath value instead of hardcoding “.tool-versions”, while preserving the
existing wrapped errors and operation context.
- Around line 120-123: Update the error return after installer.ParseToolSpec in
the tool resolution flow to wrap errUtils.ErrInvalidToolSpec alongside the
underlying error, preserving the existing tool name context and enabling
errors.Is classification for malformed specifications.
In
`@tests/snapshots/TestCLICommands_atmos_toolchain_install_--help.stdout.golden`:
- Around line 10-11: Separate the two install examples in the source help text
for the toolchain install command so they render as distinct commands, then
regenerate the affected golden snapshot using -regenerate-snapshots; do not edit
the snapshot directly.
---
Nitpick comments:
In `@pkg/toolchain/installer/lock_tool_test.go`:
- Around line 73-113: Strengthen TestLockTool_NoOpsWhenLockFileDisabled by
counting HTTP requests handled by the test server and asserting the count
remains zero after LockTool returns. Keep the existing no-error and no-lock-file
assertions, ensuring the test verifies the complete no-op behavior when
useLockFile is false.
- Around line 39-70: Update the LockTool test around the existing binDir setup
to create a sentinel binary file with known contents before calling
installer.LockTool. After locking, read that same file and assert its contents
are unchanged, replacing the empty-directory-only assertion while preserving the
lock-file verification.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: de714dcd-b404-42ce-aa1c-2c0338605a3e
📒 Files selected for processing (36)
agent-skills/skills/atmos-toolchain/SKILL.mdagent-skills/skills/atmos-toolchain/references/commands-reference.mdcmd/toolchain/lock.gocmd/toolchain/lock_test.gocmd/toolchain/provider_test.gocmd/toolchain/toolchain.godemo/casts/atmos.d/screengrabs/cli.yamlpkg/toolchain/install_test.gopkg/toolchain/installer/installer.gopkg/toolchain/installer/lock_tool_test.gopkg/toolchain/lock.gopkg/toolchain/lock_test.gopkg/toolchain/types.gotests/snapshots/TestCLICommands_atmos_toolchain_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_toolchain_install_--help.stdout.goldenwebsite/docs/cli/commands/toolchain/toolchain-lock.mdxwebsite/static/casts/screengrabs/atmos-toolchain--help.castwebsite/static/casts/screengrabs/atmos-toolchain-add--help.castwebsite/static/casts/screengrabs/atmos-toolchain-clean--help.castwebsite/static/casts/screengrabs/atmos-toolchain-du--help.castwebsite/static/casts/screengrabs/atmos-toolchain-env--help.castwebsite/static/casts/screengrabs/atmos-toolchain-exec--help.castwebsite/static/casts/screengrabs/atmos-toolchain-get--help.castwebsite/static/casts/screengrabs/atmos-toolchain-install--help.castwebsite/static/casts/screengrabs/atmos-toolchain-list--help.castwebsite/static/casts/screengrabs/atmos-toolchain-lock--help.castwebsite/static/casts/screengrabs/atmos-toolchain-path--help.castwebsite/static/casts/screengrabs/atmos-toolchain-registry--help.castwebsite/static/casts/screengrabs/atmos-toolchain-registry-list--help.castwebsite/static/casts/screengrabs/atmos-toolchain-registry-search--help.castwebsite/static/casts/screengrabs/atmos-toolchain-remove--help.castwebsite/static/casts/screengrabs/atmos-toolchain-search--help.castwebsite/static/casts/screengrabs/atmos-toolchain-set--help.castwebsite/static/casts/screengrabs/atmos-toolchain-uninstall--help.castwebsite/static/casts/screengrabs/atmos-toolchain-update--help.castwebsite/static/casts/screengrabs/atmos-toolchain-which--help.cast
🚧 Files skipped from review as they are similar to previous changes (23)
- website/static/casts/screengrabs/atmos-toolchain-which--help.cast
- website/static/casts/screengrabs/atmos-toolchain-env--help.cast
- website/static/casts/screengrabs/atmos-toolchain-registry-search--help.cast
- cmd/toolchain/provider_test.go
- tests/snapshots/TestCLICommands_atmos_toolchain_--help.stdout.golden
- website/static/casts/screengrabs/atmos-toolchain-list--help.cast
- website/static/casts/screengrabs/atmos-toolchain-set--help.cast
- website/static/casts/screengrabs/atmos-toolchain-registry-list--help.cast
- website/static/casts/screengrabs/atmos-toolchain-add--help.cast
- website/static/casts/screengrabs/atmos-toolchain-du--help.cast
- website/static/casts/screengrabs/atmos-toolchain-get--help.cast
- website/static/casts/screengrabs/atmos-toolchain-update--help.cast
- website/static/casts/screengrabs/atmos-toolchain-uninstall--help.cast
- demo/casts/atmos.d/screengrabs/cli.yaml
- website/static/casts/screengrabs/atmos-toolchain--help.cast
- website/static/casts/screengrabs/atmos-toolchain-registry--help.cast
- website/static/casts/screengrabs/atmos-toolchain-install--help.cast
- website/static/casts/screengrabs/atmos-toolchain-clean--help.cast
- website/static/casts/screengrabs/atmos-toolchain-path--help.cast
- website/static/casts/screengrabs/atmos-toolchain-remove--help.cast
- agent-skills/skills/atmos-toolchain/SKILL.md
- website/static/casts/screengrabs/atmos-toolchain-exec--help.cast
- website/static/casts/screengrabs/atmos-toolchain-search--help.cast
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2894 +/- ##
==========================================
+ Coverage 82.98% 83.04% +0.05%
==========================================
Files 1881 1886 +5
Lines 183067 183909 +842
==========================================
+ Hits 151925 152728 +803
- Misses 23317 23340 +23
- Partials 7825 7841 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Fixes from PR #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>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/toolchain/lock_test.go (1)
114-126: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftKeep the default unit-test path offline.
Lines 114-118 state that this test performs a real registry lookup and download. This makes the unit-test result depend on network availability, registry behavior, and remote artifacts.
Use a deterministic package-level hook or mock for this test. Put live registry coverage in an opt-in integration test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/lock_test.go` around lines 114 - 126, Update the test setup around RunLock to replace the real NewInstaller registry lookup and download with a deterministic package-level hook or mock, ensuring the default unit-test path remains offline while preserving the existing temporary InstallPath isolation. Move live registry/download coverage into a separately opt-in integration test.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/toolchain/lock_test.go`:
- Around line 131-135: Strengthen the lock-file assertions in the test around
lockFilePath by decoding lockData with the existing lock-file model, then locate
the hashicorp/terraform@1.11.4 entry and assert its tool, version,
checksum_algorithm, and non-empty checksum value instead of checking only raw
text containment.
---
Nitpick comments:
In `@pkg/toolchain/lock_test.go`:
- Around line 114-126: Update the test setup around RunLock to replace the real
NewInstaller registry lookup and download with a deterministic package-level
hook or mock, ensuring the default unit-test path remains offline while
preserving the existing temporary InstallPath isolation. Move live
registry/download coverage into a separately opt-in integration test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 387ea6ab-baa8-4705-84a9-930ed0c5b092
📒 Files selected for processing (7)
cmd/toolchain/install.gocmd/toolchain/lock.gopkg/toolchain/install_test.gopkg/toolchain/lock.gopkg/toolchain/lock_test.gotests/snapshots/TestCLICommands_atmos_toolchain_install_--help.stdout.goldenwebsite/static/casts/screengrabs/atmos-toolchain-info--help.cast
💤 Files with no reviewable changes (1)
- tests/snapshots/TestCLICommands_atmos_toolchain_install_--help.stdout.golden
🚧 Files skipped from review as they are similar to previous changes (4)
- website/static/casts/screengrabs/atmos-toolchain-info--help.cast
- pkg/toolchain/install_test.go
- pkg/toolchain/lock.go
- cmd/toolchain/lock.go
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>
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>
… 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>
…tput 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>
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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>
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>
…date-pinning-field-test
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (10)
website/docs/cli/commands/toolchain/toolchain-lock.mdx (1)
13-13: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the platform scope of lock entries.
State that
toolchain.lock.yamlpins the resolved artifact and checksum for the current operating system and architecture. The same tool version can resolve to a different artifact on another platform.Based on learnings: “lockfiles pin resolved artifacts per operating system and architecture; scope identical-artifact claims to repeated installs on the same platform.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/docs/cli/commands/toolchain/toolchain-lock.mdx` at line 13, Update the toolchain-lock command description to state that toolchain.lock.yaml records the resolved artifact and checksum for the current operating system and architecture. Clarify that the same tool version may resolve to a different artifact on another platform, and limit any identical-artifact guarantee to repeated installs on the same platform.Source: Learnings
pkg/toolchain/add.go (1)
23-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNice ordering fix. Consider a static sentinel for the registry-lookup wrap.
Moving
ValidateVersionSpecahead ofinstaller.FindToolis the right call — bad input now fails without a registry round trip.One follow-up on line 29:
fmt.Errorf("tool '%s' not found in registry: %w", tool, err)builds a dynamic error. The repo convention is to wrap with a static sentinel fromerrors/errors.goso callers can useerrors.Is()against a stable target. AErrToolNotFoundInRegistry-style sentinel pluserrUtils.Build(...).WithCause(err)also carries the hint/context thatcmd/toolchain/add.gogained in this same PR.As per coding guidelines: "Wrap all errors with static errors from
errors/errors.go; ... never use dynamic errors directly."♻️ Sketch of the sentinel-based wrap
// Ensure the tool exists in the registry. if _, err := installer.FindTool(owner, repo, version); err != nil { - return fmt.Errorf("tool '%s' not found in registry: %w", tool, err) + return errUtils.Build(errUtils.ErrToolNotFoundInRegistry). + WithCause(err). + WithExplanationf("Tool '%s' was not found in the registry", tool). + WithContext("tool", tool). + WithContext("version", version). + Err() }This needs a matching
ErrToolNotFoundInRegistrydeclaration inerrors/errors.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/add.go` around lines 23 - 30, Replace the dynamic fmt.Errorf in the installer.FindTool failure path with a static ErrToolNotFoundInRegistry sentinel declared in errors/errors.go. Wrap it using the repository’s errUtils.Build(...).WithCause(err) pattern, preserving the tool context or hint added by the add flow so callers can reliably use errors.Is().Source: Coding guidelines
pkg/toolchain/filemanager/lockfile_test.go (1)
125-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnd the new comments with periods.
Add periods to
// Lock first versionand// Lock a second version.As per coding guidelines, "All comments must end with periods." Based on learnings, this rule applies to each single-line comment; only the final line of a multi-line comment block needs a period.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/filemanager/lockfile_test.go` around lines 125 - 133, Update the single-line comments in the lockfile test around AddTool so both “Lock first version” and “Lock a second version” end with periods.Sources: Coding guidelines, Learnings
pkg/toolchain/installer/verification_integration_test.go (2)
64-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the tense of the doc comment.
The comment states in present tense that
install"doesn't" verify against the lock file. The test now asserts that it does fail on mismatch, so the two disagree. Describe the old behavior in past tense, or drop that clause and keep the "what this guards" sentence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/installer/verification_integration_test.go` around lines 64 - 71, Update the doc comment for TestInstallFromTool_DetectsTamperedLockFileChecksum to describe the previous install behavior in past tense, or remove the outdated “doesn't” verification clause while preserving the explanation of what the test protects against.
106-119: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the specific mismatch error.
require.Errorpasses for any failure. A download error, a lock-file parse error, or an unrelated verification failure would satisfy it, so the test can go green while the tamper detection is broken. Assert the sentinel that the lock verification path returns.🛡️ Suggested tightening
- require.Error(t, err, "install must fail when the freshly downloaded checksum doesn't match the one already recorded in toolchain.lock.yaml -- silently overwriting a tampered lock entry defeats the point of use_lock_file") + require.Error(t, err, "install must fail when the freshly downloaded checksum doesn't match the one already recorded in toolchain.lock.yaml -- silently overwriting a tampered lock entry defeats the point of use_lock_file") + assert.ErrorIs(t, err, verification.ErrChecksumMismatch)Please confirm the sentinel returned by the
verifyAgainstLockpath before applying;errors/errors.goorpkg/toolchain/installer/errors.gomay define a lock-specific error instead.#!/bin/bash # Find the lock-verification path and the sentinel error it returns. rg -nP -C5 '\bverifyAgainstLock\b' --type=go rg -nP -C3 'Err[A-Za-z]*Lock[A-Za-z]*\s*=' --type=go pkg/toolchain errors🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/installer/verification_integration_test.go` around lines 106 - 119, Replace the broad require.Error assertion in the installFromTool test with an assertion for the lock-verification sentinel returned by verifyAgainstLock. First identify the lock-specific error symbol defined in the relevant errors package, then assert that the returned error matches it while preserving the existing tampered-checksum scenario.pkg/toolchain/update_test.go (2)
496-510: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThis test depends on the moving
latestrelease.
TestUpdateOneTool_LatestPin_InstallSucceedsresolves and downloads whateverhashicorp/terraformpublishes as newest at run time, so the test payload changes without any code change here. Combined with the other real-install tests in this file, that is a meaningful CI runtime and flakiness cost. Atesting.Short()skip keeps the fast path deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/update_test.go` around lines 496 - 510, Update TestUpdateOneTool_LatestPin_InstallSucceeds to skip when testing.Short() is enabled, before performing setup or the real installation; retain the existing assertions and behavior for non-short runs.
234-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStacked stale doc headers on two renamed tests. Both tests were renamed when reporting moved from target order to live completion order, and in both files the old header was left sitting above the new one. Each function now carries two doc comments that state opposite guarantees.
pkg/toolchain/update_test.go#L234-L244: delete theTestRunUpdate_ConcurrencyPreservesOrderheader at lines 234-236 and keep theTestRunUpdate_ConcurrentAllSkippedReportsEveryTargetblock.pkg/toolchain/lock_test.go#L76-L87: delete theTestRunLock_ReportsInTargetOrderheader at lines 76-80 and keep theTestRunLock_ReportsAllTargetsblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/update_test.go` around lines 234 - 244, Remove the stale TestRunUpdate_ConcurrencyPreservesOrder header from pkg/toolchain/update_test.go:234-244, keeping the TestRunUpdate_ConcurrentAllSkippedReportsEveryTarget documentation. Also remove the obsolete TestRunLock_ReportsInTargetOrder header from pkg/toolchain/lock_test.go:76-87, keeping the TestRunLock_ReportsAllTargets documentation so each test has only an accurate comment.pkg/toolchain/lockfile/lockfile_test.go (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep a literal for the schema version.
Comparing
lf.Metadata.LockFileVersiontocurrentLockFileVersioncompares the production constant with itself. The assertion cannot fail, so an accidental bump to 3 ships unnoticed. A literal2acts as the sentinel that forces a deliberate test update whenever the schema changes.🔒 Proposed change
- assert.Equal(t, currentLockFileVersion, lf.Metadata.LockFileVersion) + // Literal on purpose: a schema bump must be a deliberate, reviewed test change. + assert.Equal(t, 2, lf.Metadata.LockFileVersion)As per coding guidelines, "avoid tautological, stub, always-skipped, or coverage-only tests" and "add compile-time schema-field sentinels."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/lockfile/lockfile_test.go` at line 20, Update the LockFileVersion assertion in the lockfile test to compare against the literal schema version 2 instead of currentLockFileVersion, preserving the test as a sentinel that requires an intentional update when the schema changes.Source: Coding guidelines
pkg/toolchain/lock_test.go (1)
118-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider guarding the network-dependent tests.
TestRunLock_ForceWritesLockFileWithoutInstallingdownloads a realhashicorp/terraformrelease, andTestLockOneTool_ResolveLatestVersionError/TestLockOneTool_LockToolErrorboth reach GitHub. These pass today but fail on rate limits, offline runners, or upstream release removal. Atesting.Short()skip or a build tag keeps the default unit run deterministic while keeping the coverage available.The repo already follows this real-install convention in
install_test.go, so this is a suggestion, not a blocker.As per coding guidelines, "Prefer behavior-focused, table-driven unit tests with mocks; avoid tautological, stub, always-skipped, or coverage-only tests."
Also applies to: 225-251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/lock_test.go` around lines 118 - 138, Guard the network-dependent tests TestRunLock_ForceWritesLockFileWithoutInstalling, TestLockOneTool_ResolveLatestVersionError, and TestLockOneTool_LockToolError with the repository’s established testing.Short() skip convention, while leaving them runnable in non-short runs and preserving their existing assertions.Source: Coding guidelines
pkg/ui/markdown/custom_renderer_test.go (1)
649-661: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the negative control.
ApplyStrictLinkifyreturns silently whengetGlamourGoldmarkyields nil (seepkg/ui/markdown/custom_renderer.golines 205-213). If glamour changes its internals and that reflection breaks, this test still passes as long as the default renderer happens not to emitmailto:. Render the same input on a renderer withoutApplyStrictLinkifyand assertmailto:is present. That pins the fix to an observable difference.🧪 Proposed addition
stripped := stripANSIForTest(output) assert.NotContains(t, stripped, "mailto:", "tool@version spec must not be auto-linked as an email") assert.Contains(t, stripped, "terraform@1.5.0", "tool@version spec must still appear in the rendered output") + + // Negative control: without the fix, glamour's default GFM Linkify produces a mailto: + // link. If this stops holding, the assertions above no longer prove anything. + plain, err := glamour.NewTermRenderer() + require.NoError(t, err) + plainOutput, err := plain.Render("atmos toolchain exec terraform@1.5.0 -- version") + require.NoError(t, err) + require.Contains(t, stripANSIForTest(plainOutput), "mailto:", + "precondition: unpatched glamour must auto-link tool@version, otherwise this test proves nothing") +}As per coding guidelines, "Safety precondition and fixture-count checks must fail loudly ... do not silently skip on misconfiguration."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/markdown/custom_renderer_test.go` around lines 649 - 661, Extend TestApplyStrictLinkify with a baseline renderer that renders the same input without calling ApplyStrictLinkify, and assert its stripped output contains “mailto:”. Keep the existing strict-renderer assertions, so the test fails loudly if ApplyStrictLinkify cannot access or modify the Glamour Goldmark renderer.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/field-test/SKILL.md:
- Around line 115-127: The field-test guidance must distinguish live-progress
validation from ANSI styling validation: instruct users to run live-rendering
checks in a pseudo-TTY without piping, while using a separate piped check with
--force-color and cat -v or equivalent solely to inspect escape sequences.
Update the relevant verification instructions in the field-test guidance and
retain the Phase 4 alignment.
In `@cmd/markdown/atmos_toolchain_lock.md`:
- Around line 3-14: Update the command examples in the toolchain lock
documentation by marking every fenced block as shell code and removing the
leading `$` prompt from each command, while preserving the commands and their
descriptions.
In `@cmd/toolchain/list_test.go`:
- Around line 69-75: Preserve the shared toolchain configuration across tests by
capturing toolchain.GetAtmosConfig() before setup and restoring that value in
cleanup instead of clearing it. Apply this to the setup blocks in
cmd/toolchain/list_test.go (69-75), cmd/toolchain/lock_test.go (48-54), and
cmd/toolchain/update_test.go (69-75), while retaining each test’s temporary
configuration.
In `@docs/fixes/2026-08-07-toolchain-update-live-usage-bugs.md`:
- Line 114: Update the formatting validation entry in the toolchain
documentation to require gofumpt for every touched Go file instead of gofmt, and
state that gofumpt must be run to verify formatting.
In `@pkg/sbom/sbom.go`:
- Around line 338-343: Update appendToolchain before traversing lock.Tools to
validate the loaded lockfile with toolchainlock.Verify, or guard nil
versionEntry values before accessing versionEntry.Platforms. Ensure null nested
lockfile entries are skipped or reported without panicking during SBOM
generation.
In `@pkg/toolchain/clean_test.go`:
- Around line 644-724: Add a root-user guard for the chmod-based permission
tests, using os.Geteuid() to skip when the effective UID is 0 while preserving
the existing Windows skips. Apply the guard to
TestPreviewClean_ToolsDirPermissionError_PropagatesError,
TestPreviewClean_CacheDirPermissionError_PropagatesError, and
TestCleanDir_FatalRemoveAllFailure_ReturnsError, ideally through a shared
helper.
In `@pkg/toolchain/filemanager/lockfile.go`:
- Around line 112-127: Sort the collected lockedVersions slice before passing it
to strings.Join in the lockfile version mismatch error path. Update the code
around the existing version-map loop, reusing the same sort.Strings behavior
already used by GetTools for deterministic error output.
- Around line 76-83: Reject an empty version at the AddTool boundary, before
GetOrCreateVersion is called, using the appropriate existing sentinel from
errors/errors.go or adding one if necessary. Apply the same validation to
SetDefault before it forwards the version, so neither path can create an empty
version key while preserving existing behavior for non-empty versions.
In `@pkg/toolchain/installer/lockfile_update.go`:
- Around line 69-85: Move the existing lockfile checksum comparison from
updateLockFile into installFromTool immediately after download verification and
before extractAndInstall, using the existing versionEntry and platform symbols.
On mismatch, remove the downloaded cached asset before returning
ErrLockfileChecksumMismatch; retain updateLockFile only for persisting the
checksum after a successful installation.
In `@pkg/toolchain/lockfile/lockfile.go`:
- Around line 61-70: Update Load and its lock-file parsing flow to inspect
Metadata.LockFileVersion and handle v1 files explicitly: migrate each tool’s
legacy version and platforms fields into the corresponding Tool.Versions entry,
or reject unsupported versions with a clear message directing the user to rerun
“atmos toolchain lock.” Preserve all recorded checksums so Verify and
installation continue using the existing lock data.
---
Nitpick comments:
In `@pkg/toolchain/add.go`:
- Around line 23-30: Replace the dynamic fmt.Errorf in the installer.FindTool
failure path with a static ErrToolNotFoundInRegistry sentinel declared in
errors/errors.go. Wrap it using the repository’s
errUtils.Build(...).WithCause(err) pattern, preserving the tool context or hint
added by the add flow so callers can reliably use errors.Is().
In `@pkg/toolchain/filemanager/lockfile_test.go`:
- Around line 125-133: Update the single-line comments in the lockfile test
around AddTool so both “Lock first version” and “Lock a second version” end with
periods.
In `@pkg/toolchain/installer/verification_integration_test.go`:
- Around line 64-71: Update the doc comment for
TestInstallFromTool_DetectsTamperedLockFileChecksum to describe the previous
install behavior in past tense, or remove the outdated “doesn't” verification
clause while preserving the explanation of what the test protects against.
- Around line 106-119: Replace the broad require.Error assertion in the
installFromTool test with an assertion for the lock-verification sentinel
returned by verifyAgainstLock. First identify the lock-specific error symbol
defined in the relevant errors package, then assert that the returned error
matches it while preserving the existing tampered-checksum scenario.
In `@pkg/toolchain/lock_test.go`:
- Around line 118-138: Guard the network-dependent tests
TestRunLock_ForceWritesLockFileWithoutInstalling,
TestLockOneTool_ResolveLatestVersionError, and TestLockOneTool_LockToolError
with the repository’s established testing.Short() skip convention, while leaving
them runnable in non-short runs and preserving their existing assertions.
In `@pkg/toolchain/lockfile/lockfile_test.go`:
- Line 20: Update the LockFileVersion assertion in the lockfile test to compare
against the literal schema version 2 instead of currentLockFileVersion,
preserving the test as a sentinel that requires an intentional update when the
schema changes.
In `@pkg/toolchain/update_test.go`:
- Around line 496-510: Update TestUpdateOneTool_LatestPin_InstallSucceeds to
skip when testing.Short() is enabled, before performing setup or the real
installation; retain the existing assertions and behavior for non-short runs.
- Around line 234-244: Remove the stale TestRunUpdate_ConcurrencyPreservesOrder
header from pkg/toolchain/update_test.go:234-244, keeping the
TestRunUpdate_ConcurrentAllSkippedReportsEveryTarget documentation. Also remove
the obsolete TestRunLock_ReportsInTargetOrder header from
pkg/toolchain/lock_test.go:76-87, keeping the TestRunLock_ReportsAllTargets
documentation so each test has only an accurate comment.
In `@pkg/ui/markdown/custom_renderer_test.go`:
- Around line 649-661: Extend TestApplyStrictLinkify with a baseline renderer
that renders the same input without calling ApplyStrictLinkify, and assert its
stripped output contains “mailto:”. Keep the existing strict-renderer
assertions, so the test fails loudly if ApplyStrictLinkify cannot access or
modify the Glamour Goldmark renderer.
In `@website/docs/cli/commands/toolchain/toolchain-lock.mdx`:
- Line 13: Update the toolchain-lock command description to state that
toolchain.lock.yaml records the resolved artifact and checksum for the current
operating system and architecture. Clarify that the same tool version may
resolve to a different artifact on another platform, and limit any
identical-artifact guarantee to repeated installs on the same platform.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e243402-2552-48ac-a092-7b3854f30c49
📒 Files selected for processing (118)
.claude/skills/field-test/SKILL.md.tool-versionsagent-skills/skills/atmos-toolchain/SKILL.mdagent-skills/skills/atmos-toolchain/references/commands-reference.mdcmd/markdown/atmos_toolchain_lock.mdcmd/root.gocmd/root_help_routing_test.gocmd/toolchain/add.gocmd/toolchain/add_test.gocmd/toolchain/clean.gocmd/toolchain/clean_test.gocmd/toolchain/command_provider_test.gocmd/toolchain/exec.gocmd/toolchain/exec_test.gocmd/toolchain/install.gocmd/toolchain/list.gocmd/toolchain/list_test.gocmd/toolchain/lock.gocmd/toolchain/lock_test.gocmd/toolchain/provider_test.gocmd/toolchain/toolchain.gocmd/toolchain/uninstall.gocmd/toolchain/update.gocmd/toolchain/update_test.godemo/casts/atmos.d/screengrabs/cli.yamldocs/fixes/2026-08-07-toolchain-update-live-usage-bugs.mddocs/fixes/2026-08-07-toolchain-update-lock-version-pinning-fixes.mddocs/fixes/2026-08-08-toolchain-live-renderer-windows-ci-deadlock.mddocs/fixes/2026-08-10-website-pnpm-lockfile-duplicate-nanoid-override.mddocs/fixes/2026-08-11-batch-progress-test-ansi-color-flake.mderrors/errors.golychee.tomlpkg/ai/tools/atmos/toolchain_add.gopkg/sbom/sbom.gopkg/sbom/sbom_test.gopkg/toolchain/add.gopkg/toolchain/add_test.gopkg/toolchain/batch_progress.gopkg/toolchain/batch_progress_test.gopkg/toolchain/clean.gopkg/toolchain/clean_test.gopkg/toolchain/exec.gopkg/toolchain/exec_test.gopkg/toolchain/filemanager/lockfile.gopkg/toolchain/filemanager/lockfile_test.gopkg/toolchain/filemanager/toolversions_test.gopkg/toolchain/install.gopkg/toolchain/install_test.gopkg/toolchain/install_validation_helpers.gopkg/toolchain/installer/errors.gopkg/toolchain/installer/installer.gopkg/toolchain/installer/installer_test.gopkg/toolchain/installer/lock_tool_test.gopkg/toolchain/installer/lockfile_update.gopkg/toolchain/installer/lockfile_update_test.gopkg/toolchain/installer/verification_integration_test.gopkg/toolchain/list.gopkg/toolchain/list_test.gopkg/toolchain/lock.gopkg/toolchain/lock_test.gopkg/toolchain/lockfile/lockfile.gopkg/toolchain/lockfile/lockfile_test.gopkg/toolchain/set.gopkg/toolchain/set_test.gopkg/toolchain/tool_versions.gopkg/toolchain/tool_versions_test.gopkg/toolchain/types.gopkg/toolchain/update.gopkg/toolchain/update_test.gopkg/toolchain/version_spec.gopkg/toolchain/version_spec_test.gopkg/toolchain/which.gopkg/toolchain/which_test.gopkg/ui/formatter.gopkg/ui/formatter_test.gopkg/ui/markdown/custom_renderer.gopkg/ui/markdown/custom_renderer_test.gopkg/ui/markdown/extensions/extensions_test.gopkg/ui/markdown/extensions/linkify.gopkg/version/manager/crud.gopkg/version/manager/crud_test.gotests/snapshots/TestCLICommands_atmos_toolchain_--help.stdout.goldentests/snapshots/TestCLICommands_atmos_toolchain_install_--help.stdout.goldenwebsite/blog/2026-08-06-toolchain-update-command.mdxwebsite/docs/cli/commands/toolchain/toolchain-aliases.mdxwebsite/docs/cli/commands/toolchain/toolchain-get.mdxwebsite/docs/cli/commands/toolchain/toolchain-info.mdxwebsite/docs/cli/commands/toolchain/toolchain-lock.mdxwebsite/docs/cli/commands/toolchain/toolchain-remove.mdxwebsite/docs/cli/commands/toolchain/toolchain-set.mdxwebsite/docs/cli/commands/toolchain/toolchain-uninstall.mdxwebsite/docs/cli/commands/toolchain/toolchain-update.mdxwebsite/docs/cli/commands/toolchain/toolchain-versions.mdxwebsite/docs/cli/commands/toolchain/usage.mdxwebsite/src/data/roadmap.jswebsite/static/casts/screengrabs/atmos-toolchain--help.castwebsite/static/casts/screengrabs/atmos-toolchain-add--help.castwebsite/static/casts/screengrabs/atmos-toolchain-aliases--help.castwebsite/static/casts/screengrabs/atmos-toolchain-clean--help.castwebsite/static/casts/screengrabs/atmos-toolchain-du--help.castwebsite/static/casts/screengrabs/atmos-toolchain-env--help.castwebsite/static/casts/screengrabs/atmos-toolchain-exec--help.castwebsite/static/casts/screengrabs/atmos-toolchain-get--help.castwebsite/static/casts/screengrabs/atmos-toolchain-info--help.castwebsite/static/casts/screengrabs/atmos-toolchain-install--help.castwebsite/static/casts/screengrabs/atmos-toolchain-list--help.castwebsite/static/casts/screengrabs/atmos-toolchain-lock--help.castwebsite/static/casts/screengrabs/atmos-toolchain-path--help.castwebsite/static/casts/screengrabs/atmos-toolchain-registry--help.castwebsite/static/casts/screengrabs/atmos-toolchain-registry-list--help.castwebsite/static/casts/screengrabs/atmos-toolchain-registry-search--help.castwebsite/static/casts/screengrabs/atmos-toolchain-remove--help.castwebsite/static/casts/screengrabs/atmos-toolchain-search--help.castwebsite/static/casts/screengrabs/atmos-toolchain-set--help.castwebsite/static/casts/screengrabs/atmos-toolchain-uninstall--help.castwebsite/static/casts/screengrabs/atmos-toolchain-update--help.castwebsite/static/casts/screengrabs/atmos-toolchain-versions--help.castwebsite/static/casts/screengrabs/atmos-toolchain-which--help.cast
💤 Files with no reviewable changes (6)
- website/static/casts/screengrabs/atmos-toolchain-versions--help.cast
- website/docs/cli/commands/toolchain/toolchain-versions.mdx
- website/docs/cli/commands/toolchain/toolchain-get.mdx
- website/static/casts/screengrabs/atmos-toolchain-aliases--help.cast
- website/docs/cli/commands/toolchain/toolchain-aliases.mdx
- tests/snapshots/TestCLICommands_atmos_toolchain_install_--help.stdout.golden
🚧 Files skipped from review as they are similar to previous changes (66)
- website/static/casts/screengrabs/atmos-toolchain-du--help.cast
- agent-skills/skills/atmos-toolchain/references/commands-reference.md
- website/static/casts/screengrabs/atmos-toolchain-env--help.cast
- website/static/casts/screengrabs/atmos-toolchain-which--help.cast
- website/static/casts/screengrabs/atmos-toolchain-registry--help.cast
- website/static/casts/screengrabs/atmos-toolchain-install--help.cast
- cmd/toolchain/toolchain.go
- cmd/toolchain/install.go
- tests/snapshots/TestCLICommands_atmos_toolchain_--help.stdout.golden
- pkg/ui/markdown/custom_renderer.go
- website/static/casts/screengrabs/atmos-toolchain--help.cast
- website/static/casts/screengrabs/atmos-toolchain-path--help.cast
- pkg/toolchain/which_test.go
- website/static/casts/screengrabs/atmos-toolchain-lock--help.cast
- pkg/ai/tools/atmos/toolchain_add.go
- pkg/toolchain/set_test.go
- pkg/toolchain/install.go
- pkg/toolchain/which.go
- cmd/toolchain/command_provider_test.go
- website/static/casts/screengrabs/atmos-toolchain-list--help.cast
- cmd/toolchain/uninstall.go
- website/static/casts/screengrabs/atmos-toolchain-set--help.cast
- website/static/casts/screengrabs/atmos-toolchain-get--help.cast
- cmd/toolchain/list.go
- website/static/casts/screengrabs/atmos-toolchain-clean--help.cast
- cmd/toolchain/clean_test.go
- website/static/casts/screengrabs/atmos-toolchain-search--help.cast
- cmd/toolchain/update.go
- website/static/casts/screengrabs/atmos-toolchain-uninstall--help.cast
- pkg/toolchain/exec.go
- pkg/toolchain/install_test.go
- demo/casts/atmos.d/screengrabs/cli.yaml
- pkg/toolchain/version_spec_test.go
- pkg/toolchain/set.go
- website/static/casts/screengrabs/atmos-toolchain-registry-search--help.cast
- website/static/casts/screengrabs/atmos-toolchain-info--help.cast
- pkg/version/manager/crud.go
- cmd/toolchain/lock.go
- website/static/casts/screengrabs/atmos-toolchain-add--help.cast
- cmd/toolchain/exec.go
- pkg/toolchain/version_spec.go
- website/static/casts/screengrabs/atmos-toolchain-registry-list--help.cast
- website/docs/cli/commands/toolchain/toolchain-uninstall.mdx
- website/docs/cli/commands/toolchain/usage.mdx
- website/static/casts/screengrabs/atmos-toolchain-remove--help.cast
- pkg/ui/formatter_test.go
- cmd/toolchain/add.go
- errors/errors.go
- website/src/data/roadmap.js
- cmd/root.go
- pkg/toolchain/add_test.go
- pkg/version/manager/crud_test.go
- pkg/ui/markdown/extensions/linkify.go
- cmd/toolchain/exec_test.go
- website/static/casts/screengrabs/atmos-toolchain-update--help.cast
- cmd/toolchain/clean.go
- cmd/toolchain/provider_test.go
- pkg/ui/formatter.go
- website/static/casts/screengrabs/atmos-toolchain-exec--help.cast
- pkg/toolchain/exec_test.go
- pkg/toolchain/install_validation_helpers.go
- pkg/toolchain/update.go
- pkg/toolchain/types.go
- pkg/toolchain/list.go
- pkg/toolchain/clean.go
- pkg/toolchain/list_test.go
…gerousPath 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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/toolchain/filemanager/lockfile.go (1)
121-121: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrevent a nil
Tooldereference.
Loadaccepts a null tool entry. The lookup at Line 113 can then returnexistingTool == nil. Line 121 dereferencesexistingTool.Versions, so removing a specific version panics on a malformed lockfile.Check for a nil tool entry before accessing
Versions. Return a structured lockfile error. Add a regression test for a null tool entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/toolchain/filemanager/lockfile.go` at line 121, Update Load to validate that the tool returned by the lookup before accessing existingTool.Versions is non-nil; for a null tool entry, return the established structured lockfile error instead of dereferencing it. Add a regression test covering removal of a specific version from a lockfile containing a null tool entry..claude/skills/field-test/SKILL.md (1)
137-140: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse only supported version equivalences.
normalizeVersionremoves only a leading lowercasev. It does not normalize casing or build metadata. Usev1.2.3versus1.2.3as the example, and require any other form to be supported bynormalizeVersion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/field-test/SKILL.md around lines 137 - 140, Update the field-test guidance around diff-style “N -> M” reports to use only equivalences supported by normalizeVersion: retain v1.2.3 versus 1.2.3, and remove claims involving casing or trailing metadata unless normalizeVersion explicitly supports them. Keep the requirement to verify that equivalent values are not reported as changes and that summary counts match the individual lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/fixes/2026-08-11-coderabbit-review-toolchain-lockfile-findings.md`:
- Line 123: Update the formatter validation entry in the review findings
document to use gofumpt instead of gofmt, and ensure it explicitly verifies that
every touched Go file passes the required gofumpt check.
In `@docs/fixes/2026-08-11-isdangerouspath-windows-clean-behavior.md`:
- Around line 43-46: Revise the validation statement in the
`TestIsDangerousPath` results section to separate the Windows diagnosis from
macOS validation: say the Windows-specific behavior was diagnosed by tracing
Go’s standard-library source, and that the platform-independent test suite
passed on macOS. Remove the claim that the Windows failure was reproduced on
macOS, reserving “reproduced” for an actual Windows run.
In `@pkg/toolchain/lockfile/lockfile.go`:
- Around line 145-147: Update the migration failure handling around
migrateLegacyTools to wrap the underlying error with the appropriate static
migration error defined in errors/errors.go, while retaining %w so errors.Is can
match it. Replace the dynamic fmt.Errorf message’s base error and reuse the
existing static error symbol rather than defining a new one.
---
Outside diff comments:
In @.claude/skills/field-test/SKILL.md:
- Around line 137-140: Update the field-test guidance around diff-style “N -> M”
reports to use only equivalences supported by normalizeVersion: retain v1.2.3
versus 1.2.3, and remove claims involving casing or trailing metadata unless
normalizeVersion explicitly supports them. Keep the requirement to verify that
equivalent values are not reported as changes and that summary counts match the
individual lines.
In `@pkg/toolchain/filemanager/lockfile.go`:
- Line 121: Update Load to validate that the tool returned by the lookup before
accessing existingTool.Versions is non-nil; for a null tool entry, return the
established structured lockfile error instead of dereferencing it. Add a
regression test covering removal of a specific version from a lockfile
containing a null tool entry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88983266-4620-48f0-8432-43c57588879f
📒 Files selected for processing (19)
.claude/skills/field-test/SKILL.mdcmd/toolchain/list_test.gocmd/toolchain/lock_test.gocmd/toolchain/update_test.godocs/fixes/2026-08-07-toolchain-update-live-usage-bugs.mddocs/fixes/2026-08-11-coderabbit-review-toolchain-lockfile-findings.mddocs/fixes/2026-08-11-isdangerouspath-windows-clean-behavior.mderrors/errors.gopkg/sbom/sbom.gopkg/sbom/sbom_test.gopkg/toolchain/clean.gopkg/toolchain/clean_test.gopkg/toolchain/filemanager/lockfile.gopkg/toolchain/filemanager/lockfile_test.gopkg/toolchain/installer/installer.gopkg/toolchain/installer/lockfile_update.gopkg/toolchain/installer/verification_integration_test.gopkg/toolchain/lockfile/lockfile.gopkg/toolchain/lockfile/lockfile_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/sbom/sbom.go
- docs/fixes/2026-08-07-toolchain-update-live-usage-bugs.md
- errors/errors.go
- pkg/toolchain/installer/verification_integration_test.go
- pkg/toolchain/clean.go
- cmd/toolchain/list_test.go
- cmd/toolchain/lock_test.go
- pkg/toolchain/installer/lockfile_update.go
- cmd/toolchain/update_test.go
- pkg/toolchain/clean_test.go
- pkg/toolchain/installer/installer.go
- 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>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/fixes/2026-08-11-coderabbit-review-toolchain-lockfile-findings.md (1)
117-127: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRecord the required full validation.
If this is the complete validation record, it lists only targeted tests and omits
atmos testand the required full test run. Run and record the complete validation, or document why it was not required.As per coding guidelines: “Compile after changes with
go build ./... && atmos test; run full tests before PRs or when slow tests are affected.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/fixes/2026-08-11-coderabbit-review-toolchain-lockfile-findings.md` around lines 117 - 127, Update the Validation section to include the required full validation commands, specifically go build ./... && atmos test and the complete test suite. Record their results, or explicitly document why either validation was not required; retain the existing targeted checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/fixes/2026-08-11-coderabbit-review-toolchain-lockfile-findings.md`:
- Around line 117-127: Update the Validation section to include the required
full validation commands, specifically go build ./... && atmos test and the
complete test suite. Record their results, or explicitly document why either
validation was not required; retain the existing targeted checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ae93d6e-09df-42dd-8509-8f0ad23c9eaa
📒 Files selected for processing (3)
docs/fixes/2026-08-11-coderabbit-review-toolchain-lockfile-findings.mddocs/fixes/2026-08-11-isdangerouspath-windows-clean-behavior.mdpkg/toolchain/lockfile/lockfile.go
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/fixes/2026-08-11-isdangerouspath-windows-clean-behavior.md
- pkg/toolchain/lockfile/lockfile.go
…date-pinning-field-test
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
atmos toolchain update [tool...]to move a pinned tool to its newest available version and reinstall it, with--dry-runand bounded--max-concurrency. Tools pinned topr:/sha:/ref:are skipped with an explanation instead of silently left alone.which/execresolving the wrong version (last token instead of the default first token) on a multi-version.tool-versionsline, which caused false "not installed" errors.setappending instead of replacing the default version, contradicting its documented behavior.add/installsilently accepting SemVer range syntax (^1.2.0,~>1.0.0) and only failing later with a raw HTTP 404; now rejected immediately with a hint towarddependencies.tools/atmos version track.atmos version track add/setcorrupting any value containing<,>, or&(ajson.MarshalHTML-escaping bug), which broke the exact~>/>=constraint syntax the toolchain docs recommend.atmos toolchain versions --helpsilently rendering the wrong command's help and exiting 0 instead of erroring; fixed globally in root help routing (atmos <cmd> <bogus-subcommand> --helpnow errors for every command tree). Removes the staletoolchain-versionsandtoolchain-aliasesdocs/casts for commands that were never implemented.list --format/--installed-only/--pending-only,clean --dry-run/--cache-only/--force,exec --dry-run.updateToolVersionsFilewriting to the hardcoded default.tool-versionspath instead of the configured one.updatecommand.why
atmos toolchainsurfaced that there was no way to update a pinned tool to a newer version, and no clear signal for why range/constraint syntax (^1.2.0,~>1.0.0) didn't work when the docs implied it should.add <tool>@latest+install --reinstall) reproduced a real crash inwhich/exec, which led to finding the rest of the bugs above along the way — a documentedsetbehavior that didn't match reality, a JSON-escaping bug corrupting exactly the constraint syntax the toolchain skill doc recommends, and a help-routing bug that letatmos toolchain versions --helpsilently succeed for a command that doesn't exist (which is also why its docs page and cast looked legitimate despite documenting nothing real).atmos toolchain's docs promised and what the CLI actually did, and give users a real, safe way to move a pinned tool forward.references
osterman/toolchain-update-pinning-field-test)Summary by CodeRabbit
New Features
toolchain updatefor selected or all tools, with dry-run and concurrency controls.toolchain lockto verify artifacts and create lock entries without installation.toolchain uninstall --all.Bug Fixes
Documentation