fix(RUFU-018): add noCommitsExpected dep-sync skip and corepack/pnpm env passthrough - #2501
Conversation
…ntext and skip dep sync Co-authored-by: Fusion <noreply@runfusion.ai> Fusion-Task-Id: RUFU-018
…n installWorktreeDependencies Co-authored-by: Fusion <noreply@runfusion.ai> Fusion-Task-Id: RUFU-018
Co-authored-by: Fusion <noreply@runfusion.ai> Fusion-Task-Id: RUFU-018
Co-authored-by: Fusion <noreply@runfusion.ai> Fusion-Task-Id: RUFU-018
📝 WalkthroughWalkthroughDependency installation now preserves selected environment variables for pnpm child processes. AI-merge landing propagates ChangesClean-room dependency flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant runAiMerge
participant landWorkspaceTask
participant landOneRepo
participant installWorktreeDependencies
runAiMerge->>landOneRepo: pass noCommitsExpected
landWorkspaceTask->>landOneRepo: pass noCommitsExpected
alt noCommitsExpected is true
landOneRepo->>landOneRepo: skip dependency synchronization
else flag is false or unset
landOneRepo->>installWorktreeDependencies: synchronize dependencies
end
Possibly related PRs
Suggested reviewers: 🚥 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 |
Greptile SummaryAdds task-directed dependency-sync skipping and dependency-installer environment propagation.
Confidence Score: 3/5The PR should not merge until dependency synchronization is skipped only after confirming that the branch has no relevant changes. The new branch trusts mutable expectation metadata rather than repository state, and the added test confirms that actual code changes can be landed without dependency installation or lockfile validation. Files Needing Attention: packages/engine/src/merger-ai.ts and packages/engine/src/tests/merger-ai-no-commits-deps-skip.test.ts
|
| Filename | Overview |
|---|---|
| packages/engine/src/merger-ai.ts | Adds the noCommitsExpected skip and caller propagation, but trusts expectation metadata even when the branch has real changes. |
| packages/engine/src/merge-dependency-sync.ts | Supplies an explicit process-environment snapshot to dependency-install subprocesses; no independently actionable defect was found. |
| packages/engine/src/tests/merger-ai-no-commits-deps-skip.test.ts | Covers true, false, and undefined states, while also confirming the unsafe true-with-actual-changes behavior. |
| packages/engine/src/tests/merge-dependency-sync-lockfile-heal.test.ts | Adds focused fake-pnpm coverage for propagated, preserved, and undefined environment variables. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Task[Task metadata] --> Flag{noCommitsExpected true?}
Flag -- Yes --> Skip[Skip dependency synchronization]
Flag -- No --> Install[Install and validate dependencies]
Skip --> Merge[Merge and review]
Install --> Merge
Merge --> Land[Land squash result]
Reviews (1): Last reviewed commit: "test(RUFU-018): complete Step 4 — add en..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/engine/src/__tests__/merge-dependency-sync-lockfile-heal.test.ts`:
- Around line 171-200: Update the cleanup logic in the test cases around
installWorktreeDependencies to restore environment variables by deleting each
key when its saved original value is undefined; otherwise assign the saved
value. Apply this to COREPACK_HOME, PNPM_HOME, and npm_config_registry in both
affected finally blocks so absent variables do not become the literal string
"undefined".
In `@packages/engine/src/__tests__/merger-ai-no-commits-deps-skip.test.ts`:
- Around line 137-260: Add targeted regression tests that invoke both task-entry
paths, runAiMerge and landWorkspaceTask, with task.noCommitsExpected set to
true. Assert installWorktreeDependencies is not called through each entry point,
while preserving successful merge behavior; keep the existing direct
LandRepoContext coverage unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 350a47dd-a5c0-46a9-9161-dde9b237ff35
📒 Files selected for processing (4)
packages/engine/src/__tests__/merge-dependency-sync-lockfile-heal.test.tspackages/engine/src/__tests__/merger-ai-no-commits-deps-skip.test.tspackages/engine/src/merge-dependency-sync.tspackages/engine/src/merger-ai.ts
|
Checking thread status for a hand-merge (this PR is cleared but has 3 live threads). Two are coderabbit maintainability notes. The greptile P1 is confirmed by this PR's own test, so it is worth a decision rather than a dismissal. What the P1 says
The test in this PR encodes exactly thatit("skips installWorktreeDependencies entirely when noCommitsExpected: true", async () => {
fx = createRepoFixture(true); // branch has actual changes
...
noCommitsExpected: true,
...
expect(vi.mocked(installWorktreeDependencies)).not.toHaveBeenCalled();
expect(result.outcome).toBe("landed");
});The fixture is deliberately constructed as So the question is intent, not behaviour
I am not deciding that — I did not write this PR and the trade depends on where For completeness, thread status across the three PRs cleared for hand-merge: #2376 is clear (0 unresolved), this one has 3, and #2377 has 2 coderabbit "Major" threads (fork-aware PR path coverage; a functional-correctness note on |
|
Worked all three threads. The P1 is real; fixes are pushed to 1. greptile P1,
|
|
This PR's head is the fork branch #2958 supersedes this: your five commits unmodified, plus the review fixes. One PR in, one out — no queue growth. Close this when #2958 lands. Full reasoning is in the comment above and in #2958's body. |
Superseded by #2958 — same four files, same branch name, opened from upstream#2958 ( They read as the same change re-landed from the upstream repo rather than the fork (+542/-2 vs +425/-0 — the newer one has grown, so it is not a straight copy). Both are open and both are Flagging rather than closing: I did not author either, and if #2958 dropped something this one has, that is worth catching before this is closed. Owner's call — if #2958 carries everything, close this as superseded. Worth noting the fork angle specifically, since it is easy to miss in a PR list: these are distinguishable only by |
|
This and #2958 are the same branch, and #2958 says it supersedes this one — so this PR is counted twice in the queue. Measured:
It does resolve itself on merge — merging #2958 lands this exact branch into Flagging it anyway for two reasons. It inflates the open count by one while the queue is being drawn down, and two reviewers can independently pick up the same branch from different PRs and duplicate the review — the consolidation protocol asks for the superseded PR to be closed with a Not closing it — your PR, your call, and closing the wrong one of a matched pair is the kind of thing worth leaving to whoever knows the intent. |
|
Do not hand-merge this one — it points at a stale commit and would land the version WITHOUT the review fix. #2501 and #2958 are two PRs over the same branch (
Both PRs show all four checks green, which is what makes this dangerous: #2501 looks ready by every signal on the page, and merging it lands the code the P1 was raised against. Squash-merging #2501 would take its recorded head, not the branch tip. Merge #2958 instead and close this, or re-point/refresh #2501 so it picks up Flagging rather than acting: closing a PR that was explicitly cleared for merge is your call, not mine. I have not touched either branch. |
…(PR #2501 review) greptile P1: the skip trusted ctx.noCommitsExpected alone. Control only reaches that line when the branch is AHEAD — the rev-list short-circuit returns early at zero — and both downstream empty-lane guards explicitly carve out noCommitsExpected tasks, so nothing revalidates the expectation against what actually landed. A no-commits task whose executor committed a lockfile change had its install and its frozen-lockfile validation skipped. The flag now says look; the diff decides. A branch touching package.json or any LOCKFILE_CANDIDATES entry falls through to the normal sync and is audited. An unreadable diff also syncs, matching the hard-fail contract above it. LOCKFILE_CANDIDATES is exported rather than duplicated so the skip and the installer cannot drift on what counts as a dependency change. Also fixes the coderabbit env nit: restoring an absent var by assignment stores the string "undefined" and leaks into later tests; delete it instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…— the P1 #2501 shipped without (#2958) ## #2501 merged without its P1 fix; this is that fix, alone #2501 has landed. Its review threads were resolved — I judged and fixed them — but its head was a fork branch I could not push to, so **the fixes were never in it**. Confirmed on `main` at `c1c1b964af`: ``` merger-ai.ts:902: if (ctx.noCommitsExpected === true) { ← bare flag, no diff gate merge-dependency-sync.ts: export const LOCKFILE_CANDIDATES → 0 matches ``` Rebasing dropped this PR's five duplicated base commits, so it is now **one commit**: the review fix and its regression. ## The defect on main The dep-sync skip trusts `ctx.noCommitsExpected` alone, and **only ever runs on a branch that has commits** — the `rev-list --count` short-circuit ~50 lines above returns `outcome: "empty"` at zero ahead, so control reaches it only when the branch is AHEAD. Nothing revalidates the flag. Both downstream empty-lane guards carve no-commits tasks out explicitly — `merger-ai.ts:1372` (#2259 already-landed proof) and `:1994` (FN-8141 executor veto) — and both guard the *opposite* direction: commit-expected task, empty branch. The inverse has no check. So a task marked no-commits whose executor committed a manifest or lockfile change gets its dependency install **and** its frozen-lockfile validation skipped, and the change lands unvalidated. ## The fix The flag says *look*; the branch diff decides. A `main...branch` diff touching `package.json` or any `LOCKFILE_CANDIDATES` entry falls through to the normal sync and emits an audit row with `skipOverridden: true`. An unreadable diff **also** syncs — matching the hard-fail contract documented directly above that block, rather than treating absence of evidence as evidence of safety. `LOCKFILE_CANDIDATES` is exported instead of duplicated, so the skip and the installer cannot drift on what counts as a dependency change. **Mutation-verified:** reverting to trust-the-flag fails exactly the new case and nothing else. The existing *"lands successfully with noCommitsExpected: true and actual changes"* case is untouched and still passes — `feature.txt` is not a dependency file, so an ordinary source change on a no-commits task still skips. The new case differs only in *which* file the branch touches. ## Also carried over from the #2501 review **coderabbit's env nit** — `process.env.X = undefined` stores the string `"undefined"`, leaving a previously-absent var truthy and leaking into later tests. `restoreEnv` applied at both sites. **Both entry paths** — deferred with reasons: `runAiMerge`/`landWorkspaceTask` sit behind real worktrees, sessions and a merge agent, and the cheap version is a mirrored-implementation test that cannot fail on a revert (this repo has deleted two of those). The fix above also means propagation is no longer the only thing between a stale flag and an unvalidated lockfile. ## A correction to my own work My first version of the regression committed the lockfile while the fixture had left the tree on `main`, so the `main...branch` diff could not see it and the case **passed for the wrong reason**. Corrected, with the reason recorded in the test. ## Verification - `merger-ai-no-commits-deps-skip` — **5/5**, mutation-verified - `merge-dependency-sync-lockfile-heal` — **10/10** - engine typecheck — clean - `pnpm lint` — clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Manually land RUFU-018 fix bypassing the AI merge pipeline.
Summary
noCommitsExpectedflag toLandRepoContext; skip dependency sync when setCOREPACK_HOME/PNPM_HOME/npm_config_registryininstallWorktreeDependenciesThis unblocks all downstream RUFU audit tasks.
Surface Enumeration
installWorktreeDependenciescalled fromlandOneRepo(AI merge) and legacymerger.ts;landOneRepocalled fromrunAiMergeandlandWorkspaceTasknoCommitsExpectedcan betrue,false, orundefined— both callers use=== truestrict checkSummary by CodeRabbit
New Features
Bug Fixes