Repository navigation
feat(install): add dry-run resolution preview - #927
Conversation
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge — dry-run exits before any disk write, so worst-case a user sees an unexpected message or count but nothing is mutated. The orchestration changes are narrow and well-guarded: every mutating step in the main pipeline is individually gated on No files require special attention. Important Files Changed
Reviews (6): Last reviewed commit: "fix(install): let CI dry-run resolve dri..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/aube/src/commands/install/mod.rs`:
- Around line 168-173: The dry-run flag check happens too late in the execution
flow, allowing filesystem mutations through crate::runtime::ensure before the
dry-run no-write path is reached. Move the opts.dry_run check earlier in the
command execution flow, before calling crate::runtime::ensure and other
functions that can download and pin runtimes. This ensures that when dry-run is
enabled, all operations that would mutate filesystem state are skipped entirely,
with the lockfile-only flow at line 239 becoming the only path taken for dry-run
mode.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 171646e9-e6d6-46a3-8ebf-248712533a2f
📒 Files selected for processing (6)
crates/aube/src/commands/ci.rscrates/aube/src/commands/deploy/mod.rscrates/aube/src/commands/install/args.rscrates/aube/src/commands/install/mod.rscrates/aube/src/commands/install/resolve.rstest/install.bats
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c59ae55. Configure here.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/install.bats (2)
947-960: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider verifying lockfile preservation in this drift scenario.
The test modifies
package.jsonafter_setup_basic_fixturecreates a lockfile, then runs dry-run. Since dry-run explicitly disables lockfile writes (perwrite_lockfile: !opts.dry_run), consider adding an assertion to verify the existing lockfile wasn't modified.For example, capture the lockfile timestamp or content hash before dry-run and verify it remains unchanged, or at minimum add:
assert [ -e aube-lock.yaml ]to confirm the lockfile still exists and wasn't removed/recreated.
🤖 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 `@test/install.bats` around lines 947 - 960, The test "aube install --dry-run in CI resolves drifted lockfile" does not verify that the existing lockfile is preserved after running the dry-run command with a drifted package.json. Add an assertion to confirm the lockfile (aube-lock.yaml) still exists and was not modified by the dry-run operation. You can do this by adding an assert statement checking that the lockfile file exists after the run command completes. Optionally, capture the lockfile content or timestamp before running the dry-run command and verify it matches afterwards to ensure the file was not rewritten.
962-971: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding lockfile preservation check for the fresh scenario.
This test verifies the "up-to-date lockfile" dry-run path correctly skips fetch/link and reports appropriately. Since
_setup_basic_fixturecreates a lockfile and this scenario explicitly tests that resolution is skipped when the lockfile is fresh, consider adding:assert [ -e aube-lock.yaml ]after line 970 to confirm the lockfile still exists and dry-run didn't remove or modify it.
🤖 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 `@test/install.bats` around lines 962 - 971, The test function "aube install --dry-run with fresh lockfile still reports skipped fetch and link" does not verify that the lockfile is preserved after the dry-run operation. Add an assertion after the assert_output checks to confirm the aube-lock.yaml file still exists following the dry-run, ensuring the lockfile was neither removed nor modified during the operation.
🤖 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 `@test/install.bats`:
- Around line 947-960: The test "aube install --dry-run in CI resolves drifted
lockfile" does not verify that the existing lockfile is preserved after running
the dry-run command with a drifted package.json. Add an assertion to confirm the
lockfile (aube-lock.yaml) still exists and was not modified by the dry-run
operation. You can do this by adding an assert statement checking that the
lockfile file exists after the run command completes. Optionally, capture the
lockfile content or timestamp before running the dry-run command and verify it
matches afterwards to ensure the file was not rewritten.
- Around line 962-971: The test function "aube install --dry-run with fresh
lockfile still reports skipped fetch and link" does not verify that the lockfile
is preserved after the dry-run operation. Add an assertion after the
assert_output checks to confirm the aube-lock.yaml file still exists following
the dry-run, ensuring the lockfile was neither removed nor modified during the
operation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e58612b-bfce-48de-b7d7-74cf715f1c1f
📒 Files selected for processing (6)
aube.usage.kdlcrates/aube/src/commands/install/mod.rscrates/aube/src/commands/install/resolve.rsdocs/cli/commands.jsondocs/cli/install.mdtest/install.bats
✅ Files skipped from review due to trivial changes (1)
- docs/cli/install.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/aube/src/commands/install/mod.rs
- crates/aube/src/commands/install/resolve.rs
Summary
aube install --dry-runas a no-write resolution previewValidation
cargo buildcargo fmt --checkmise run test:bats test/install.batsNote
Medium Risk
Changes the central install command path and frozen/CI interaction semantics, but dry-run is explicitly no-write and is covered by new integration tests.
Overview
Adds
aube install --dry-runto resolve dependencies and print what would happen without writing the lockfile, linkingnode_modules, or running install side effects. The flag conflicts with--lockfile-onlyand is documented in CLI usage and install docs.Dry-run threads through
InstallOptions.dry_runand reuses the existing lockfile-only resolve path withwrite_lockfile: false. The main install flow skips force/state reset, the install fast path, Node runtime ensure, branch-lockfile merge, and rootpreinstallhooks when dry-run is set.Behavior differs from
--lockfile-onlyin a few ways: explicit--frozen-lockfilestill fails when no lockfile exists (and validates drift before resolve), while CI-style dry-run can resolve without a lockfile or when the lockfile is drifted. When the lockfile is already fresh, output uses dry-run-specific messages instead of the lockfile-only “resolution step is skipped” line.cianddeploycontinue to passdry_run: false. Bats coverage asserts noaube-lock.yaml/node_modulesand checks frozen/CI/fresh-lockfile cases, includinglockfile=falseprojects that can dry-run without the lockfile-only conflict.Reviewed by Cursor Bugbot for commit 9427906. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
--dry-runtoaube installto resolve and report planned changes without writing the lockfile, creatingnode_modules, or running lifecycle scripts.--dry-runis incompatible with--lockfile-only.installdocs for--dry-run.aube-lock.yamlornode_modulesare created.