fix(engine): self-heal failed in-review cards whose PR merged on the remote#1922
fix(engine): self-heal failed in-review cards whose PR merged on the remote#1922flexi767 wants to merge 1 commit into
Conversation
…remote A transient error at merge time can flag an in-review card `failed` even when its PR actually squash-merged on the remote. `recoverAlreadyMergedReviewTasks` only ran the already-merged evidence detector against the LOCAL base ref, so when this process never fetched the merge, the owned commit was absent locally, the detector returned null, and the card held its file-scope lease forever. Fetch-then-prove: when a failed candidate has a recorded PR and the local base yields no owned commit, best-effort `git fetch origin <base>` and re-run the SAME evidence detector against `origin/<base>`. The owned-commit proof and every foreign-ownership guard inside the detector remain the sole finalize gate, so this only un-wedges a genuinely-merged task — it never phantom-finalizes on unproven state. Gated on a recorded PR (no PR ⇒ nothing merged remotely ⇒ no fetch); fail-closed on fetch error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a ChangesStale base ref refresh for already-merged detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SelfHealingManager
participant findAlreadyMergedTaskCommit
participant refreshRemoteBaseRef
participant Git
SelfHealingManager->>findAlreadyMergedTaskCommit: check baseBranch for landed commit
findAlreadyMergedTaskCommit-->>SelfHealingManager: landed = null
SelfHealingManager->>refreshRemoteBaseRef: refreshRemoteBaseRef(baseBranch)
refreshRemoteBaseRef->>Git: git fetch origin baseBranch
Git-->>refreshRemoteBaseRef: fetch result (best-effort)
refreshRemoteBaseRef->>Git: git rev-parse --verify origin/baseBranch
Git-->>refreshRemoteBaseRef: verified ref or null
refreshRemoteBaseRef-->>SelfHealingManager: origin/baseBranch or null
SelfHealingManager->>findAlreadyMergedTaskCommit: retry with origin/baseBranch
findAlreadyMergedTaskCommit-->>SelfHealingManager: landed commit or null
🚥 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 SummaryThis PR teaches self-healing to recover failed review cards whose PR landed remotely. The main changes are:
Confidence Score: 4/5The changed recovery path needs a fix before merging.
packages/engine/src/self-healing.ts Important Files Changed
Reviews (1): Last reviewed commit: "fix(engine): self-heal failed in-review ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| await execAsync(`git fetch origin ${shellQuote(baseBranch)}`, { | ||
| cwd: this.options.rootDir, | ||
| timeout: 60_000, | ||
| }); | ||
| } catch { | ||
| // Swallow: fall through to the existing remote-tracking ref if present. | ||
| } | ||
| try { |
There was a problem hiding this comment.
When git fetch fails, this helper still returns an existing origin/<base> ref. That lets the new recovery path run the destructive finalize detector against a remote-tracking ref that was not refreshed, so an offline/auth failure can still move a failed in-review task to done, remove its worktree, and unblock dependents based on stale evidence.
| try { | |
| await execAsync(`git fetch origin ${shellQuote(baseBranch)}`, { | |
| cwd: this.options.rootDir, | |
| timeout: 60_000, | |
| }); | |
| } catch { | |
| // Swallow: fall through to the existing remote-tracking ref if present. | |
| } | |
| try { | |
| try { | |
| await execAsync(`git fetch origin ${shellQuote(baseBranch)}`, { | |
| cwd: this.options.rootDir, | |
| timeout: 60_000, | |
| }); | |
| } catch { | |
| return null; | |
| } | |
| try { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/engine/src/self-healing.ts (1)
8671-8701: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider throttling the new remote fetch to avoid repeated network calls on persistently-unmerged candidates.
refreshRemoteBaseRefruns unconditionally whenever a retry-exhausted failed in-review task has a PR and no local evidence. Since these candidates persist across sweeps until resolved, a genuinely-unmerged task will trigger agit fetch origin <base>(60s timeout) on every maintenance tick and every startup recovery run. Other reconcilers in this file (e.g.deadlockRecoveryCooldown) use a per-task cooldown map to avoid this kind of repeated work; the same pattern (or a per-sweep cache keyed bybaseBranchto dedupe fetches across candidates sharing the same base) would reduce redundant I/O without weakening the fail-closed guarantee.♻️ Example: per-sweep fetch dedup
async recoverAlreadyMergedReviewTasks(): Promise<number> { try { const settings = await this.store.getSettings(); if (settings.globalPause || settings.enginePaused) return 0; + const refreshedBaseRefsThisSweep = new Map<string, string | null>(); const maxAutoMergeRetries = resolveMaxAutoMergeRetries(settings);if (!landed && getPrimaryPrInfo(task)) { - const refreshedBaseRef = await this.refreshRemoteBaseRef(baseBranch); + let refreshedBaseRef: string | null; + if (refreshedBaseRefsThisSweep.has(baseBranch)) { + refreshedBaseRef = refreshedBaseRefsThisSweep.get(baseBranch)!; + } else { + refreshedBaseRef = await this.refreshRemoteBaseRef(baseBranch); + refreshedBaseRefsThisSweep.set(baseBranch, refreshedBaseRef); + }🤖 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 `@packages/engine/src/self-healing.ts` around lines 8671 - 8701, The new remote fetch in the landed-task check can repeat on every sweep for persistently unmerged candidates, causing unnecessary network work. Update the logic around `refreshRemoteBaseRef` in `self-healing.ts` to throttle or dedupe fetches, using the same cooldown-style approach as `deadlockRecoveryCooldown` or a per-sweep cache keyed by `baseBranch`. Ensure `findAlreadyMergedTaskCommit` still runs after a successful refresh and the fail-closed behavior remains unchanged.packages/engine/src/__tests__/self-healing-already-merged.real-git.test.ts (1)
480-480: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueStatic analysis flags dynamic
execSynccommands as a command-injection pattern.Both calls build the command via string interpolation of
${JSON.stringify(...)}-quoted paths. Sinceremote/cloneoriginate frommkdtempSyncunder test control (not external/untrusted input), the actual injection risk here is negligible — butJSON.stringifyis not a hardened shell-escaping mechanism in general. ConsiderexecFileSyncwith an argument array for consistency with the tool's recommendation, though this is optional given the existing file already uses this same pattern elsewhere for test-local paths.Also applies to: 494-494
🤖 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 `@packages/engine/src/__tests__/self-healing-already-merged.real-git.test.ts` at line 480, Static analysis is flagging the `execSync` shell command construction in the git setup calls, so update the `self-healing-already-merged.real-git.test.ts` test helpers to avoid interpolating paths into a shell string. Use `execFileSync` (or the same non-shell pattern used elsewhere in the test file) for the `git init --bare -b main` and related command near `remote`/`clone`, passing arguments separately while keeping the temp-path behavior unchanged.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 `@packages/engine/src/__tests__/self-healing-already-merged.real-git.test.ts`:
- Line 480: Static analysis is flagging the `execSync` shell command
construction in the git setup calls, so update the
`self-healing-already-merged.real-git.test.ts` test helpers to avoid
interpolating paths into a shell string. Use `execFileSync` (or the same
non-shell pattern used elsewhere in the test file) for the `git init --bare -b
main` and related command near `remote`/`clone`, passing arguments separately
while keeping the temp-path behavior unchanged.
In `@packages/engine/src/self-healing.ts`:
- Around line 8671-8701: The new remote fetch in the landed-task check can
repeat on every sweep for persistently unmerged candidates, causing unnecessary
network work. Update the logic around `refreshRemoteBaseRef` in
`self-healing.ts` to throttle or dedupe fetches, using the same cooldown-style
approach as `deadlockRecoveryCooldown` or a per-sweep cache keyed by
`baseBranch`. Ensure `findAlreadyMergedTaskCommit` still runs after a successful
refresh and the fail-closed behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa1a9859-cbfc-4db3-a8cb-b99dc0fa8101
📒 Files selected for processing (2)
packages/engine/src/__tests__/self-healing-already-merged.real-git.test.tspackages/engine/src/self-healing.ts
Problem
A transient error at merge time can flag an in-review card
failedeven when its PR actually squash-merged on the remote (human merge, merge-train, etc.).recoverAlreadyMergedReviewTasksruns the already-merged evidence detector only against the local base ref. If this process never fetched the merge, the owned commit is absent locally → the detector returnsnull→landedis null → the card never finalizes and holds its file-scope lease forever, wedging every other task that touches the same files.Fix — fetch-then-prove
When a
failedin-review candidate has a recorded PR (getPrimaryPrInfo) and the local base yields no owned commit:git fetch origin <base>(newrefreshRemoteBaseRefhelper), thenorigin/<base>.The detector's owned-commit proof and every foreign-ownership guard inside it remain the sole finalize gate, so this only un-wedges a genuinely-merged task — it never phantom-finalizes on unproven state.
Safety:
origin/<base>can't be resolved the card is left untouched.Tests
Two real-git tests in
self-healing-already-merged.real-git.test.ts, both verified to fail without the prod change:origin/main, and finalizes the card todone(mergeConfirmed: true, worktree removed).failed/in-review, never healed.Full self-heal suite: 594 passed. Engine typecheck clean.
🤖 Generated with Claude Code
Summary by CodeRabbit