fix(merger-ai): gate the no-commits dep-sync skip on the branch diff — the P1 #2501 shipped without

[gsxdsm]

#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

[coderabbitai]

Warning

Review limit reached

@gsxdsm, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 31 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5e6c6c0a-082b-49a2-aa81-1633597c4735

📥 Commits

Reviewing files that changed from the base of the PR and between c1c1b96 and 5323671.

📒 Files selected for processing (4)

• packages/engine/src/__tests__/merge-dependency-sync-lockfile-heal.test.ts
• packages/engine/src/__tests__/merger-ai-no-commits-deps-skip.test.ts
• packages/engine/src/merge-dependency-sync.ts
• packages/engine/src/merger-ai.ts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[gsxdsm]

Merge-blocking correction before anyone lands this by hand: this PR and #2501 point at the same commit (2972472dbb). git diff pr2501 pr2958 is empty — identical trees, 18 files / +584 / −313 against main either way.

So the rationale in the description above no longer holds. It says #2501 "cannot be pushed to" and "the fixes those threads asked for are not in it — merging #2501 as it stands ships the P1 unfixed." Neither is true of the current head: the fix is present (the restoreEnv helper, 7 references in merge-dependency-sync-lockfile-heal.test.ts, deleting absent keys instead of assigning undefined back as the string "undefined"). Whatever the state was when this PR was opened, the fork head has since caught up.

Consequence: these are duplicates, not a fixed-vs-unfixed pair. Merge one, close the other — there is no version-selection decision to make, and merging both would be a no-op second merge of the same tree.

I checked because the description asserted a difference and a hand-merge was about to be made on that basis. Verified by SHA and diff, not by reading either body.

#2501's last open thread asked for exactly this env-restore fix; I have replied there pointing at the code, so both PRs are now thread-clear.