Repository navigation
fix(install): write the root workspace lockfile under sharedWorkspaceLockfile=false - #882
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughwrite_per_project_lockfiles now processes the workspace root importer ( ChangesRoot importer inclusion with format preservation
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. Comment |
Greptile SummaryUnder
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to the The fix correctly handles all three cases (root project, config-only root, filtered install), the ordering of the two new guards is intentional and correct, and the new code introduces no unwraps at public API boundaries. Both the unit tests and the updated e2e bats tests give good behavioral coverage for the changed path. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/root-worksp..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/aube/src/commands/install/workspace.rs (2)
482-518: ⚡ Quick winConsider covering the root-importer case in
test/workspace.batstoo.This unit test proves
write_per_project_lockfiles, but the regression was in the fullsharedWorkspaceLockfile=falseinstall flow and the existing bats coverage in the provided context still only exercises member-only layouts. One CLI-level root-importer test would lock in the fallback-kind plumbing end to end.🤖 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 `@crates/aube/src/commands/install/workspace.rs` around lines 482 - 518, Add a CLI-level bats test that exercises the sharedWorkspaceLockfile=false install flow for a workspace that has a root importer (mirroring the unit test writes_root_importer_lockfile_alongside_members). The test should invoke the real install command, create a workspace with a root package plus a member package, ensure the root already has a pnpm-lock.yaml (so the runner preserves its format), run the install, and assert that the root pnpm-lock.yaml is written with the root importer present and that the member receives an aube-lock.yaml; this verifies the end-to-end behavior of write_per_project_lockfiles and the fallback LockfileKind::Aube in the CLI path.
296-304: ⚡ Quick winReuse
importer_project_dir()here.This reimplements the same
"."/member mapping but drops the helper's lexical normalization for parent-relative importer keys (for example../sibling). Calling the existing helper keeps root handling and off-root workspace members consistent in one place.♻️ Proposed change
- let pkg_dir = if importer_path == "." { - workspace_root.to_path_buf() - } else { - workspace_root.join(importer_path) - }; + let pkg_dir = importer_project_dir(workspace_root, importer_path);🤖 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 `@crates/aube/src/commands/install/workspace.rs` around lines 296 - 304, The code computes pkg_dir manually from workspace_root and importer_path; replace that block with a call to the existing helper importer_project_dir(...) so importer_project_dir(workspace_root, importer_path) is assigned to pkg_dir, preserving the helper's lexical normalization for parent-relative importer keys (e.g., "../sibling") and keeping root (".") handling consistent.
🤖 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 `@crates/aube/src/commands/install/workspace.rs`:
- Around line 482-518: Add a CLI-level bats test that exercises the
sharedWorkspaceLockfile=false install flow for a workspace that has a root
importer (mirroring the unit test
writes_root_importer_lockfile_alongside_members). The test should invoke the
real install command, create a workspace with a root package plus a member
package, ensure the root already has a pnpm-lock.yaml (so the runner preserves
its format), run the install, and assert that the root pnpm-lock.yaml is written
with the root importer present and that the member receives an aube-lock.yaml;
this verifies the end-to-end behavior of write_per_project_lockfiles and the
fallback LockfileKind::Aube in the CLI path.
- Around line 296-304: The code computes pkg_dir manually from workspace_root
and importer_path; replace that block with a call to the existing helper
importer_project_dir(...) so importer_project_dir(workspace_root, importer_path)
is assigned to pkg_dir, preserving the helper's lexical normalization for
parent-relative importer keys (e.g., "../sibling") and keeping root (".")
handling consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19b7c0d1-bdf7-47d0-b6a3-e7a14d0c850c
📒 Files selected for processing (1)
crates/aube/src/commands/install/workspace.rs
91e6291 to
6bb7882
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aube/src/commands/install/workspace.rs (1)
311-317: ⚡ Quick winPrefer the existing
importer_project_dirhelper for path computation.The inline directory computation duplicates logic from the
importer_project_dirhelper (line 153) but omitsnormalize_lexicalfor non-root importers. When workspace packages use parent-relative glob patterns (../**), importer paths can contain..segments that should be collapsed for correct path operations. The helper already handles this case (line 167).♻️ Proposed refactor
- // The root importer (`.`) writes its lockfile at the workspace - // root itself; members nest under their relative path. - let pkg_dir = if importer_path == "." { - workspace_root.to_path_buf() - } else { - workspace_root.join(importer_path) - }; + // The root importer (`.`) writes its lockfile at the workspace + // root itself; members nest under their relative path. + let pkg_dir = importer_project_dir(workspace_root, importer_path);🤖 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 `@crates/aube/src/commands/install/workspace.rs` around lines 311 - 317, The inline computation of pkg_dir duplicates logic and misses normalize_lexical for non-root importers; replace the conditional that derives pkg_dir (which checks importer_path == ".") with a call to the existing helper importer_project_dir passing workspace_root and importer_path (or the helper's expected parameter order), so the path normalization (including normalize_lexical) is applied for both root and non-root importers; ensure you remove the old if/else block and use the helper wherever pkg_dir is computed.
🤖 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 `@crates/aube/src/commands/install/workspace.rs`:
- Around line 311-317: The inline computation of pkg_dir duplicates logic and
misses normalize_lexical for non-root importers; replace the conditional that
derives pkg_dir (which checks importer_path == ".") with a call to the existing
helper importer_project_dir passing workspace_root and importer_path (or the
helper's expected parameter order), so the path normalization (including
normalize_lexical) is applied for both root and non-root importers; ensure you
remove the old if/else block and use the helper wherever pkg_dir is computed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4c3ffda-6cd0-4c53-81a3-d78a571d7900
📒 Files selected for processing (4)
crates/aube/src/commands/install/workspace.rstest/pnpm_savecatalog.batstest/pnpm_update.batstest/workspace.bats
✅ Files skipped from review due to trivial changes (1)
- test/pnpm_update.bats
|
I checked this against real pnpm behavior with Corepack pnpm v11.6.0, and the main behavior here does appear to match pnpm:
So the existing test expectation changes from “no root lockfile” to “root lockfile exists” look correct for fixtures where the workspace root is itself a project. One edge I think is worth covering before merge: filtered workspace installs. In a quick pnpm check, I also agree with the existing review note to use This comment was generated by Codex. |
…kfile=false A scoped `aube install --filter <pkg>` rewrote the root and every unrelated member's per-project lockfile, diverging from pnpm which touches only the selected project(s). Derive the write selection from the same selector the link pass uses, and reuse `importer_project_dir` for pkg_dir so parent-relative importer keys stay normalized. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
In a workspace with
sharedWorkspaceLockfile: false, aube wrote a per-projectpnpm-lock.yamlfor every member except the workspace root — so the root's ownpnpm-lock.yaml(the one holding the root package's own dependencies) silently disappeared after install. pnpm, under the same setting, writes a lockfile for every project including the root. This restores that behavior.Root cause
write_per_project_lockfilesexplicitly skipped the.importer:so the root project never got its lockfile written under the per-project layout.
Fix
crates/aube/src/commands/install/workspace.rs:.-skip; the root importer is now written like any other project..→workspace_root); members continue to nest under their relative path.subset_to_importer(...) -> Noneguard.pnpm-lock.yamlkeeps gettingpnpm-lock.yaml(no surpriseaube-lock.yaml); a root with no lockfile uses the workspace fallback kind.Verification
writes_root_importer_lockfile_alongside_membersasserts the rootpnpm-lock.yamlis written (with its own importer, format preserved) alongside members. Existing per-project tests still pass.sharedWorkspaceLockfile: false: both./pnpm-lock.yaml(root importer., root's own deps) and./packages/member/pnpm-lock.yamlare written, format preserved, with noaube-lock.yamlproduced anywhere.Test plan
cargo test -p aube per_project_lockfile_testscargo clippy --tests -p aubeclean,cargo fmt --checkcleansharedWorkspaceLockfile: falseworkspace, runaube installand confirm the root
pnpm-lock.yamlplus each member's lockfile are allpresent and in the correct format
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation