Fix setup-atmos Windows installs#111
Merged
Erik Osterman (Cloud Posse) (osterman) merged 8 commits intoJul 3, 2026
Merged
Conversation
Erik Osterman (Cloud Posse) (osterman)
marked this pull request as ready for review
Erik Osterman (Cloud Posse) (osterman)
requested review from
a team
as code owners
Erik Osterman (Cloud Posse) (osterman)
requested review from
RoseSecurity and
PePe Amengual (jamengual)
Member
Author
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds SHA256 checksum validation to the atmos installer (with ChangesChecksum Validation and Installer Feature
CI Workflow Restructuring
Estimated code review effort: 4 (Complex) | ~60 minutes Related PRs: None identified in the provided information. Suggested reviewers: None specified. Poem: A rabbit checks each hash with care, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/main.yml (1)
9-9: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the PR-head checkout if this job must validate the contributor’s branch —
actions/checkoutonpull_requestuses the merge ref, so fork PRs are linted/tested/built against merged code instead of the head commit.🤖 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 @.github/workflows/main.yml at line 9, The checkout step in the workflow is using the default pull_request merge ref, so this job may validate merged code instead of the contributor’s branch. Update the `actions/checkout` usage in the workflow to explicitly check out the PR head when this job is meant to lint/test/build the contributor’s changes, and keep the existing workflow/job structure intact while adjusting the checkout configuration.src/installer.ts (1)
305-317: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up the downloaded binary when checksum verification fails.
downloadPathis only removed afterio.cp; any error fromverifyDownloadedToolleaves the downloaded asset behind.Proposed cleanup
core.info(`Acquiring ${info.resolvedVersion} from ${info.downloadUrl}`); const downloadPath = await tc.downloadTool(info.downloadUrl, undefined, auth); const toolPath = path.join(atmosInstallPath, atmosBinName); - await verifyDownloadedTool(info, downloadPath, auth, checksumValidation); - - core.info("Installing downloaded file..."); - // Ensure the destination directory exists - await io.mkdirP(atmosInstallPath); - // Use copy + delete instead of mv/rename to handle cross-device installations - // This fixes EXDEV errors in Docker-in-Docker and other containerized environments - await io.cp(downloadPath, toolPath); - await io.rmRF(downloadPath); + try { + await verifyDownloadedTool(info, downloadPath, auth, checksumValidation); + + core.info("Installing downloaded file..."); + await io.mkdirP(atmosInstallPath); + await io.cp(downloadPath, toolPath); + } finally { + await io.rmRF(downloadPath); + }🤖 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 `@src/installer.ts` around lines 305 - 317, The cleanup around the download/install flow in installer.ts leaves the file returned by tc.downloadTool behind when verifyDownloadedTool fails. Update the download handling in the install path near verifyDownloadedTool and io.cp so the temporary downloadPath is removed on checksum verification errors as well, using a try/finally or equivalent cleanup around the acquisition/verification block while preserving the existing copy-and-delete install behavior.
🧹 Nitpick comments (1)
src/wrapper.ts (1)
65-69: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFloating promise on module-level
runWrapper()call.The invocation isn't awaited or has an explicit
.catch(). SincerunWrapper's internal try/catch always routes failures tocore.setFailed, this is unlikely to cause an unhandled rejection in practice, but adding a.catch()would be a small defensive improvement.🛡️ Optional defensive tweak
if (require.main === module) { - runWrapper(); + void runWrapper(); }🤖 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 `@src/wrapper.ts` around lines 65 - 69, The module-level runWrapper() call is a floating promise in the require.main === module branch, so add an explicit .catch() handler (or equivalent top-level await pattern) to the runWrapper invocation in src/wrapper.ts. Use the runWrapper function and the module entrypoint guard as the anchor points, and make the fallback handler minimally defensive by forwarding any unexpected rejection to core.setFailed.
🤖 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 @.github/workflows/main.yml:
- Line 48: The checkout step in the workflow is persisting git credentials
unnecessarily. Update the actions/checkout usage in the main job to set
persist-credentials to false so the workflow does not keep push-capable
credentials after checkout. Keep the change scoped to the checkout step and
ensure the job still only checks out code without storing auth.
- Around line 34-47: The new integration job is missing an explicit
least-privilege permissions block, so it currently inherits broad default
GITHUB_TOKEN access. Add a permissions setting on the integration job in
main.yml and scope it down to only what the job needs, such as contents: read,
while keeping the existing matrix and steps intact.
- Around line 56-65: The “Verify outputs are available” step is interpolating
atmos outputs directly into the run script, which creates a template-injection
risk. Move steps.atmos.outputs.stdout, stderr, and exitcode into env variables
on that step instead of embedding them in run, and reference those variables in
the shell command. Also ensure the syntax is cross-shell safe in the workflow,
either by setting shell: bash for the step or by using the appropriate
PowerShell env variable form when running on windows-latest.
In `@README.yaml`:
- Around line 88-92: Correct the typo in the README wording by updating the
phrase in the GitHub action description from “wrapper scipt” to “wrapper
script.” Keep the change limited to the documentation text near the
checksum-validation explanation and the note about capturing stdout, stderr, and
exitcode.
In `@src/__tests__/setup-atmos.test.ts`:
- Around line 249-255: The fs.readFileSync spy in setup-atmos.test.ts is hitting
a TypeScript overload mismatch because the mock callback returns string |
Buffer. Update the mock around jest.spyOn(fs, "readFileSync") to use the
overloaded function type, such as casting the spy to jest.MockedFunction<typeof
fs.readFileSync> or giving mockImplementation an explicit compatible signature,
and apply the same typing approach consistently to all four readFileSync mocks
in this test file.
In `@src/installer.ts`:
- Around line 347-371: The cache lookup in installer.ts can return legacy
wrapper-based atmos entries when installWrapper is false, causing the wrong
shimmed binary to be reused. Update the install flow around getToolCacheName and
tc.find to detect or bypass old namespace/cache entries for the plain binary,
using either a cache-version bump or a content/metadata check before accepting
the cached toolPath, and ensure configureInstalledPath only runs on the correct
binary.
---
Outside diff comments:
In @.github/workflows/main.yml:
- Line 9: The checkout step in the workflow is using the default pull_request
merge ref, so this job may validate merged code instead of the contributor’s
branch. Update the `actions/checkout` usage in the workflow to explicitly check
out the PR head when this job is meant to lint/test/build the contributor’s
changes, and keep the existing workflow/job structure intact while adjusting the
checkout configuration.
In `@src/installer.ts`:
- Around line 305-317: The cleanup around the download/install flow in
installer.ts leaves the file returned by tc.downloadTool behind when
verifyDownloadedTool fails. Update the download handling in the install path
near verifyDownloadedTool and io.cp so the temporary downloadPath is removed on
checksum verification errors as well, using a try/finally or equivalent cleanup
around the acquisition/verification block while preserving the existing
copy-and-delete install behavior.
---
Nitpick comments:
In `@src/wrapper.ts`:
- Around line 65-69: The module-level runWrapper() call is a floating promise in
the require.main === module branch, so add an explicit .catch() handler (or
equivalent top-level await pattern) to the runWrapper invocation in
src/wrapper.ts. Use the runWrapper function and the module entrypoint guard as
the anchor points, and make the fallback handler minimally defensive by
forwarding any unexpected rejection to core.setFailed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7f61ec2-327c-430b-ac78-45174b0cf411
⛔ Files ignored due to path filters (5)
README.mdis excluded by!README.mddist/index.jsis excluded by!**/dist/**dist/wrapper/index.jsis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.github/workflows/main.yml.github/workflows/validate-codeowners.ymlREADME.yamlaction.ymljest.config.jspackage.jsonsrc/__tests__/setup-atmos.test.tssrc/__tests__/wrapper.test.tssrc/installer.tssrc/interfaces.tssrc/main.tssrc/wrapper.ts
💤 Files with no reviewable changes (1)
- .github/workflows/validate-codeowners.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/test-install.yaml (1)
21-22: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon checkout.This workflow only checks out the repo to run local actions and doesn't push anything, so the persisted
GITHUB_TOKENcredential in git config is unnecessary and widens the attack surface for subsequent steps.🔒 Proposed fix
- name: Checkout uses: actions/checkout@v7 + with: + persist-credentials: false🤖 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 @.github/workflows/test-install.yaml around lines 21 - 22, The Checkout step in the workflow currently leaves the default GITHUB_TOKEN credentials persisted, which is unnecessary for this read-only job. Update the actions/checkout usage in the Checkout step to set persist-credentials to false, keeping the checkout behavior the same while preventing git credential persistence for later steps.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/test-install.yaml:
- Around line 21-22: The Checkout step in the workflow currently leaves the
default GITHUB_TOKEN credentials persisted, which is unnecessary for this
read-only job. Update the actions/checkout usage in the Checkout step to set
persist-credentials to false, keeping the checkout behavior the same while
preventing git credential persistence for later steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73d2e63a-db8e-4287-9622-b57ef2ff58f4
⛔ Files ignored due to path filters (3)
README.mdis excluded by!README.mddist/index.jsis excluded by!**/dist/**dist/wrapper/index.jsis excluded by!**/dist/**
📒 Files selected for processing (5)
.github/workflows/main.yml.github/workflows/test-install.yamlREADME.yamlsrc/__tests__/setup-atmos.test.tssrc/installer.ts
✅ Files skipped from review due to trivial changes (1)
- README.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/main.yml
- src/tests/setup-atmos.test.ts
Member
Author
|
CodeRabbit (@coderabbitai) full review |
✅ Action performedFull review finished. |
Andriy Knysh (aknysh)
approved these changes
Jul 3, 2026
Erik Osterman (Cloud Posse) (osterman)
deleted the
osterman/fix-setup-atmos-windows-ci
branch
|
These changes were released in v3.1.0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
.exeassets and install runnableatmos.exe/wrapper shims across OSes.warn, 80% coverage enforcement, expanded Jest tests, and OS/mode CI matrix.dist/artifacts.why
atmosimmediately after setup for wrapper and no-wrapper modes.references
npm run format:check,npm run lint:check,npm run test:coverage -- --runInBand,npm run clean && npm run build && npm run build:wrapper.