feat: add support for PR artifacts in --use-version flag#2040
feat: add support for PR artifacts in --use-version flag#2040Andriy Knysh (aknysh) merged 16 commits intomainfrom
Conversation
Erik Osterman (CEO @ Cloud Posse) (osterman)
requested a review
from a team
as a code owner
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds PR- and SHA-based artifact installation: GitHub artifact-fetching APIs and token retrieval, version-spec parsing and re-exec routing, local TTL caching, artifact download/extract/install with Zip Slip protections, extensive tests and generated mocks, CLI wiring for --use-version pr:NNNN / sha:XXXX flows, and related docs/roadmap updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Root as cmd/root.go
participant Reexec as pkg/version/reexec
participant Toolchain as pkg/toolchain
participant GitHub as GitHub API
participant Cache as Local Cache
User->>Root: atmos --use-version pr:2040 ...
Root->>Reexec: CheckAndReexec()
Reexec->>Reexec: ParseVersionSpec("pr:2040")
Reexec->>Toolchain: findOrInstallPRVersion(2040)
Toolchain->>Cache: CheckPRCacheStatus(2040)
alt Cache valid
Cache-->>Toolchain: PRCacheValid + path
else Cache missing/expired
Toolchain->>GitHub: GetGitHubToken()
GitHub-->>Toolchain: token
Toolchain->>GitHub: GetPRArtifactInfo(owner, repo, 2040)
GitHub-->>Toolchain: artifact metadata
Toolchain->>GitHub: GetArtifactDownloadURL(artifactID)
GitHub-->>Toolchain: download URL
Toolchain->>Toolchain: download -> extract -> install
Toolchain->>Cache: SavePRCacheMetadataAfterInstall(2040)
end
Toolchain-->>Reexec: installed path
Reexec->>Root: re-exec with ATMOS_VERSION_USE
sequenceDiagram
participant Toolchain as pkg/toolchain
participant Token as pkg/github/token
participant Env as Environment
participant CLI as gh CLI
Toolchain->>Token: GetGitHubToken()
Token->>Env: check ATMOS_GITHUB_TOKEN
alt env token
Env-->>Token: token
else
Token->>Env: check GITHUB_TOKEN
alt env token
Env-->>Token: token
else
Token->>CLI: run 'gh auth token'
alt gh available & authed
CLI-->>Token: token
else
CLI-->>Token: (no token)
end
end
end
Token-->>Toolchain: token or error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2040 +/- ##
==========================================
- Coverage 76.21% 76.15% -0.06%
==========================================
Files 824 829 +5
Lines 77622 78464 +842
==========================================
+ Hits 59161 59756 +595
- Misses 14759 14975 +216
- Partials 3702 3733 +31
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@toolchain/pr_artifact.go`:
- Around line 95-98: The slice of artifactInfo.HeadSHA using shortSHALength can
panic if HeadSHA is shorter than shortSHALength; update the code around
artifactInfo.HeadSHA and shortSHALength to compute a safe shortSHA (e.g., take
the minimum of len(artifactInfo.HeadSHA) and shortSHALength or fall back to
artifactInfo.HeadSHA) before calling ui.Successf, then use that safe shortSHA in
the ui.Successf call so no slicing can panic.
- Around line 288-310: The loop over r.File uses f.Name directly to build
destPath which allows Zip Slip; validate and sanitize the joined path before
creating directories or extracting. After computing destPath via
filepath.Join(destDir, f.Name) (the current variable), call filepath.Clean and
then verify the path is inside destDir (e.g., use filepath.Rel(destDir,
cleanedPath) and ensure the result does not start with ".." and is not equal to
".."), returning ErrPRArtifactExtractFailed on failure; only then proceed with
os.MkdirAll and call extractZipEntry for that validated path. Ensure the same
check applies to directory entries and remove any unchecked trust of f.Name.
🧹 Nitpick comments (1)
toolchain/pr_artifact.go (1)
176-185: Consider adding a timeout to the HTTP client.The client has no timeout configured. A stalled download could hang indefinitely. Adding a reasonable timeout improves reliability.
♻️ Proposed fix
client := &http.Client{ + Timeout: 5 * time.Minute, // Reasonable timeout for artifact downloads. // Follow redirects but preserve auth header for GitHub domain only. CheckRedirect: func(req *http.Request, via []*http.Request) error { // Don't add auth header when redirected to S3 (pre-signed URL). if !strings.Contains(req.URL.Host, "github") { req.Header.Del("Authorization") } return nil }, }You'll need to add
"time"to imports.
Erik Osterman (CEO @ Cloud Posse) (osterman)
added
the
minor
|
Warning Release Documentation RequiredThis PR is labeled
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@cmd/root.go`:
- Around line 445-466: Comments in the shown block end with commas or no
punctuation and must end with periods to satisfy the godot linter; update each
comment line in the block (the three comment blocks above
parseUseVersionFromArgs()/explicitVersionRequested, above the os.Setenv call,
and above shouldReexec and the CheckAndReexec call) to end with periods. Ensure
references to parseUseVersionFromArgs, explicitVersionRequested,
explicitVersion, pkgversion.VersionUseEnvVar, isVersionManagementCommand,
pkgversion.CheckAndReexec, and tmpConfig remain unchanged while only fixing
punctuation at the end of each comment sentence.
In `@pkg/version/reexec.go`:
- Around line 265-268: The bulleted comment describing TTL caching ("If binary
exists and cache is within TTL (1 min) -> use cached binary, no API call", "If
binary exists but TTL expired -> check API for new commits", "If no binary
exists -> fresh install") must end each sentence with a period to satisfy the
godot linter; update that comment block so each bullet line and the initial
comment sentence end with periods (i.e., append a trailing period to each bullet
and the explanatory line).
- Around line 172-188: The code currently lets invalid PR specifiers (e.g.
"pr:abc" or "pr:0") fall through and be treated as PR `#0`; to fix, call
toolchain.ParseVersionSpec(requestedVersion) and inspect its returned kind/value
before proceeding: if ParseVersionSpec indicates a PR but the numeric PR
identifier is missing/invalid or zero, treat that as a parse error and take the
hard-fail path (same behavior as the existing PR hard-fail), and only call
findOrInstallPRVersion or treat it as a PR when ParseVersionSpec actually yields
a valid PR id; apply the same change to both places (the current block around
findOrInstallVersionWithConfig and the similar block at lines 223-238) so
invalid PR specifiers are rejected instead of being coerced to 0.
In `@toolchain/pr_artifact.go`:
- Around line 303-327: The HTTP client created for downloading artifacts has no
timeout and can hang indefinitely; set a sensible request timeout (e.g.,
client.Timeout = 30*time.Second) on the http.Client or wrap req with a context
with deadline before calling client.Do so the download fails fast on stalls;
update the client initialization where client := &http.Client{ CheckRedirect:
func(req *http.Request, via []*http.Request) error { ... } } to include the
timeout (and ensure any context-based change still removes tempPath and returns
ErrPRArtifactDownloadFailed on failure, referencing req, client, CheckRedirect,
tempPath, and ErrPRArtifactDownloadFailed).
In `@toolchain/version_spec.go`:
- Around line 29-33: The comment block above ParseVersionSpec must end each
sentence with a period to satisfy the godot linter; update the doc comment lines
for ParseVersionSpec (including the description line and each bullet) so every
line terminates with a period, e.g., "Supports explicit prefix (pr:)." and each
bullet like "All digits -> PR number." and "Valid semver pattern (X.Y.Z or
vX.Y.Z) -> semver.".
- Around line 44-52: ParseVersionSpec currently accepts any string with prPrefix
(pr:) even if the suffix is non-numeric; change the prPrefix branch to validate
the trimmed suffix with isAllDigits before returning VersionTypePR. In the
strings.HasPrefix(version, prPrefix) branch (and when calling
strings.TrimPrefix(version, prPrefix)), check isAllDigits(trimmed) and if true
return VersionTypePR, trimmed, nil; otherwise return an appropriate error (e.g.,
formatted error indicating invalid PR id). Keep the existing fallback that
treats all-digits as PRs via isAllDigits(version).
🧹 Nitpick comments (1)
toolchain/pr_cache_test.go (1)
66-140: Consider a table‑driven structure for the CheckPRCacheStatus variants. It would reduce duplication and make edge cases easier to extend.As per coding guidelines: Use table-driven tests for testing multiple scenarios in Go.
|
💥 This pull request now has conflicts. Could you fix it Erik Osterman (CEO @ Cloud Posse) (@osterman)? 🙏 |
Allow users to test Atmos features from PRs without installing Go or manually downloading artifacts from GitHub Actions UI. Extend --use-version to support PR numbers using the pr:NNNN format (e.g., --use-version pr:2038). Key features: - Smart GitHub token detection (ATMOS_GITHUB_TOKEN, GITHUB_TOKEN, or gh CLI) - Automatic platform detection for Linux/macOS/Windows - Download and extract artifacts from successful PR CI runs - Caching of downloaded binaries for reuse - Clear error messages for missing tokens, failed CI, unsupported platforms New packages: - pkg/github/: GitHub API integration for PR artifact retrieval - toolchain/pr_artifact.go: PR artifact download and installation logic Modified: - pkg/version/reexec.go: Added PR version detection in version switching - toolchain/install.go: Added PR version detection in tool installation Implements #2038. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Add SHA and timestamp to "Found workflow run" message - Use spinner pattern for download+install (replaces separate messages) - Format timestamp in user-friendly format (e.g., "Jan 29, 2025 at 3:04 PM") - Show short SHA (7 chars) with markdown backticks for styling - Extract downloadAndInstallArtifact helper for cleaner code New output format: ✓ Found workflow run #21489525938 (sha: `ceb7526`) from Jan 29, 2026 at 12:11 PM ✓ Installed to ~/.local/share/atmos/toolchain/bin/cloudposse/atmos/pr-2040/atmos Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove SHA version support (sha:XXXXXXX and auto-detected SHAs)
- Add semver validation - reject invalid inputs like 'abc', 'abc123'
- Delete toolchain/sha_artifact.go
- Update ParseVersionSpec to return error for invalid formats
- Add isValidSemver() function for version validation
- Remove GetSHAArtifactInfo from pkg/github/artifacts.go
- Update blog post and PR description to not mention SHA
Invalid version formats now show helpful error:
✗ Error: invalid version format 'abc'
💡 Version must be a PR number, pr:NNNN, or semver (e.g., 1.2.3)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add proper path sanitization to prevent Zip Slip attacks when extracting downloaded PR artifacts. The `sanitizeZipPath()` function now: - Rejects absolute paths (Unix and Windows style) - Rejects paths containing backslash (Windows-style traversal on Unix) - Rejects paths containing ".." traversal components - Verifies the final path stays within the destination directory Added comprehensive tests for all attack patterns. Fixes CodeQL alert: "Arbitrary file access during archive extraction" Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add length check before slicing HeadSHA to prevent potential panic (CodeRabbit suggestion) - Add PR number (2040) to roadmap milestone entry Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The `pr:` prefix is optional - just use the PR number directly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add support for installing Atmos from a specific commit SHA's build artifact, similar to PR-based installation. Changes: - Add SHAArtifactInfo type and GetSHAArtifactInfo function to github pkg - Add error checking helper functions (IsNotFoundError, etc.) - Add toolchain/sha_artifact.go for SHA-based installation - Update version_spec to support SHA version parsing - Fix Windows test failures: use platform-specific binary name in tests The SHA installation allows users to install a specific commit's build: atmos --use-version sha:ceb7526 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update documentation to reflect the SHA-based installation support and fix any remaining content in the PR artifact installation blog post. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Erik Osterman (CEO @ Cloud Posse) (osterman)
force-pushed
the
osterman/version-from-pr
branch
from
f8f8baf to
5c95a83
Compare
…dling - Add tests for IsNotFoundError, artifact URL construction, and platform support - Add tests for PR artifact installation flow, zip extraction, and Zip Slip protection - Add SHA artifact test suite with installation, cache status, and error paths - Improve error assertions to use errors.Is with sentinel errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/github/artifacts_test.go`:
- Around line 208-210: The trailing comment near the GetPRArtifactInfo tests
contains a sentence fragment across two lines; rewrite it into one or two
complete sentences that end with periods so it satisfies the godot linter.
Locate the comment referencing "Full integration tests for GetPRArtifactInfo
require a real GitHub token" and "and network access" and combine or rephrase
into a grammatically complete sentence (for example: "Full integration tests for
GetPRArtifactInfo require a real GitHub token and network access.") ensuring the
final comment line ends with a period.
- Around line 1-210: The tests call network-backed functions GetPRArtifactInfo,
GetArtifactDownloadURL, and GetPRHeadSHA—make them unit-testable by introducing
a GitHubClient interface in artifacts.go (e.g., methods GetPRArtifactInfo(ctx,
owner, repo, pr) / GetArtifactDownloadURL(ctx, owner, repo, artifactID) /
GetPRHeadSHA(ctx, owner, repo, pr) or the smaller set of underlying GitHub SDK
calls those functions need), refactor the top-level functions to accept or be
methods on that interface (dependency-inject the client instead of using
global/new clients), add a mock generated via mockgen (follow the pattern used
for ratelimit.go: create mock_githubclient.go in the package), and update tests
to use the generated mock to assert behavior for success, wrapped errors, and
platform cases so the unit tests run deterministically without network access.
🧹 Nitpick comments (4)
pkg/toolchain/pr_artifact_test.go (2)
104-110: This test is fully subsumed byTestHandlePRArtifactError_AllCases.The "generic error" case on line 106 is an exact duplicate of the last entry in the table-driven test at lines 143–148. Consider removing this standalone test to avoid redundancy.
231-267: Use the existingtestBinaryName()helper instead of duplicating the logic.Both
setupTestInstallPathandtestBinaryNameexist inpkg/toolchain/pr_cache_test.go. The test correctly usessetupTestInstallPath, but duplicates the platform-specific binary naming logic (lines 9–12) instead of callingtestBinaryName(). Replace the inline if-block with a call to the existing helper to avoid duplication.pkg/toolchain/sha_artifact_test.go (1)
239-278: RemovejoinStringsfunction and its test — usestrings.Joininstead.The
joinStringsfunction is a direct reimplementation ofstrings.Joinfrom the standard library with identical behavior. Per coding guidelines, avoid wrapper functions without added value. Replace the single production usage at line 246 ofsha_artifact.gowithstrings.Join(platforms, ", ")and remove this test, which duplicates stdlib behavior testing.pkg/github/artifacts_test.go (1)
12-47: Platform-dependent test only exercises one branch per CI run.
getArtifactNameForPlatform()readsruntime.GOOS/runtime.GOARCHdirectly, so on linux/amd64 CI only that branch is validated. The other OS branches are dead paths. Consider refactoring the underlying function to acceptgoos/goarchparameters (or a struct) so all mappings can be tested deterministically via a table-driven approach.Not blocking, but worth tracking. As per coding guidelines, "Use table-driven tests for testing multiple scenarios in Go."
Introduce PullRequestService and ActionsService interfaces following the established ratelimit.go pattern, enabling deterministic unit tests without network calls. Export ArtifactFetcher struct with NewArtifactFetcher constructor for test injection. Refactor private helpers to accept interfaces and generate mocks via mockgen. Adds 21 new unit tests covering getPRHeadSHA, findSuccessfulWorkflowRun, findArtifactByName, GetArtifactDownloadURL, GetPRArtifactInfo, and GetSHAArtifactInfo with success, error propagation, and sentinel error cases. Also fixes a minor grammar issue in a trailing comment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pkg/github/artifacts_test.go`:
- Line 213: Update the section-divider comments that start with "// ---" to end
with a period so they satisfy the godot linter; for example change "// ---
Mock-based unit tests for getPRHeadSHA ---" to "// --- Mock-based unit tests for
getPRHeadSHA ---.", and do the same for the other divider comments noted (the
similar "// ---" headers at the other locations).
In `@pkg/github/artifacts.go`:
- Around line 294-307: The call to actions.ListRepositoryWorkflowRuns can return
a nil 'runs' with no error, so before iterating you must add a nil-safety check:
after the call to ListRepositoryWorkflowRuns (the variables 'runs, resp, err'),
verify that 'runs' is not nil and that 'runs.WorkflowRuns' is non-nil/has length
> 0; if it's nil/empty, return a sensible nil result (e.g., nil, nil or the same
error-handling flow) rather than iterating. Update the block that searches for a
matching run (which returns &workflowRunInfo{...}) to only loop when runs != nil
and len(runs.WorkflowRuns) > 0 to avoid a panic.
- Around line 322-331: In findArtifactByName, guard against a nil artifacts
response from ListWorkflowRunArtifacts: after calling
actions.ListWorkflowRunArtifacts(ctx, owner, repo, runID, opts) and checking
err, add a nil-check for artifacts and for artifacts.Artifacts (e.g., if
artifacts == nil || artifacts.Artifacts == nil) and return nil, nil (or a clear
error) to avoid dereferencing a nil pointer when iterating; update references to
artifacts.Artifacts in the loop accordingly so the function safely handles a nil
response.
🧹 Nitpick comments (4)
pkg/github/artifacts.go (4)
17-31: DuplicateErrUnsupportedPlatformsentinel.
ErrUnsupportedPlatformis already defined inerrors/errors.go(line 633). Sinceerrors.Newproduces distinct values,errors.Is(err, atmosErrors.ErrUnsupportedPlatform)won't match this local sentinel. Any caller checking against the canonical one will get a false negative.Per project learnings, this can be cleaned up in a separate refactor PR, but worth tracking.
Based on learnings: "cloudposse/atmos: For toolchain work, duplicate/unused error sentinels in errors/errors.go should be cleaned up in a separate refactor PR and not block feature PRs."
282-310: Single-page pagination may miss workflow runs.
findSuccessfulWorkflowRunfetches only one page of 100 results. If the target "Tests" workflow run isn't in the first page (e.g., repo has many concurrent workflows), it'll incorrectly reportErrNoWorkflowRunFound. The same applies tofindArtifactByName(line 315-341).For a PR-specific SHA, the result set is typically small, so this is low risk in practice. Still worth a brief comment in the code or a follow-up for pagination support.
112-118:perf.Trackon trivial factories and predicates adds overhead with no value.
NewArtifactFetcheris a simple factory,SupportedPRPlatformsis a trivial getter, and theIs*Errorpredicates are single-lineerrors.Iscalls. The coding guidelines explicitly exclude "trivial getters/setters, command constructors, simple factories" from theperf.Trackrequirement.Not blocking, but removing them would reduce noise in perf traces.
As per coding guidelines: "Add
defer perf.Track(...)()to all public functions, except trivial getters/setters, command constructors, simple factories, and functions that only delegate to another tracked function."Also applies to: 344-352, 427-453
34-36: Comment/const name mismatch.Line 35 says
// WorkflowName is the name...(capitalized, implying exported), but the const isworkflowName(unexported).Proposed fix
- // WorkflowName is the name of the CI workflow that produces build artifacts. + // workflowName is the name of the CI workflow that produces build artifacts.
Guard against nil responses from ListRepositoryWorkflowRuns and ListWorkflowRunArtifacts to prevent panics. Add trailing periods to section-divider comments to satisfy the godot linter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/github/artifacts.go`:
- Around line 220-228: The getArtifactDownloadURL function must guard against
artifact being nil after calling f.actions.GetArtifact; if artifact is nil (even
when err==nil) return a clear error (use handleGitHubAPIError or create a
descriptive error) instead of dereferencing it. Update getArtifactDownloadURL to
check artifact == nil and return an appropriate error (including resp/context)
before calling artifact.GetArchiveDownloadURL(), referencing the GetArtifact
call and keeping the existing handleGitHubAPIError usage for consistency.
- Around line 16-43: Remove the local ErrUnsupportedPlatform sentinel and use
the centralized sentinel from the shared errors package via the existing
errUtils alias; specifically, delete the local ErrUnsupportedPlatform variable
in the artifacts package and replace any references to ErrUnsupportedPlatform
with errUtils.ErrUnsupportedPlatform so errors.Is checks match the canonical
sentinel.
🧹 Nitpick comments (2)
pkg/github/artifacts.go (1)
438-464: Error predicate functions are straightforward.One note:
perf.Trackon trivial one-liner predicates likeIsNotFoundErroradds overhead for negligible insight. These are essentially getters/setters. Per coding guidelines, trivial getters/setters are exempt fromperf.Track. Not a blocker, just something to consider trimming.pkg/github/artifacts_test.go (1)
553-592: Consider adding a nil-artifact test forGetArtifactDownloadURL.If
GetArtifactreturns(nil, resp, nil), the production code currently panics (see my comment onartifacts.goline 220-228). A test exercising that path would catch the regression and validate the fix.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/root.go (1)
112-140: Clean implementation, consistent with the existingparseChdirFromArgspattern.One minor note: neither this nor
parseChdirFromArgsInternalstops scanning after a bare--(end-of-flags delimiter). Something likeatmos terraform plan -- --use-version 1.0would still match. Low risk since--use-versionafter--is unlikely in practice, and both parsers share the same limitation. Worth a follow-up if it ever matters.🔧 Optional: stop scanning after `--`
func parseUseVersionFromArgsInternal(args []string) string { for i := 0; i < len(args); i++ { arg := args[i] + + // Stop scanning after bare "--" (end-of-flags delimiter). + if arg == "--" { + return "" + } // Check for --use-version=value format. if strings.HasPrefix(arg, "--use-version=") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 112 - 140, The parser parseUseVersionFromArgsInternal should stop scanning when it encounters the end-of-flags delimiter "--" to avoid matching flags that appear after it; update parseUseVersionFromArgsInternal to break/return when arg == "--" before checking for "--use-version" forms, and make the analogous change to parseChdirFromArgsInternal so both follow the same behavior (still supporting both "--use-version=value" and "--use-version value" forms).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/root.go`:
- Around line 112-140: The parser parseUseVersionFromArgsInternal should stop
scanning when it encounters the end-of-flags delimiter "--" to avoid matching
flags that appear after it; update parseUseVersionFromArgsInternal to
break/return when arg == "--" before checking for "--use-version" forms, and
make the analogous change to parseChdirFromArgsInternal so both follow the same
behavior (still supporting both "--use-version=value" and "--use-version value"
forms).
…artifact installation Add comprehensive mock-based unit tests for PR and SHA artifact installation paths across pkg/version, pkg/toolchain, and pkg/github packages. Fix end-of-flags delimiter (--) handling in root command for --use-version and --chdir flags. Address CodeRabbit review feedback including interface-based DI for GitHub API clients, improved error messages, and token detection chain fixes. Update PRD and blog post to reflect fully-implemented status with current architecture details. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Andriy Knysh (aknysh)
dismissed
coderabbitai[bot]’s stale review
via
d6d36a0
|
These changes were released in v1.206.3-rc.1. |
|
These changes were released in v1.207.0. |
|
These changes were released in v1.208.0-rc.0. |
|
These changes were released in v1.208.0-test.15. |
3 participants
what
--use-versionflag to support PR numbers using thepr:NNNNformat (e.g.,--use-version pr:2038) or auto-detected all-digit format (--use-version 2038)--use-versionflag to provide clear error messages for invalid inputswhy
go install(which has proxy issues), or manually downloading artifacts from GitHub Actions UI--use-versionmakes testing as simple asatmos --use-version 2038 terraform planUsage
Test plan
./build/atmos --use-version 2040 version./build/atmos --use-version 999999 version(should error)./build/atmos --use-version abc version(should error)./build/atmos --use-version abc123 version(should error)./build/atmos --use-version 1.175.0 version(should work)references
pkg/github/artifacts.go- GitHub API integration for PR artifact retrievalpkg/github/token.go- Smart token detection with gh CLI fallbacktoolchain/pr_artifact.go- PR artifact download and installation logictoolchain/version_spec.go- Version format parsing and validationpkg/version/reexec.go- PR version detection in version switching🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests