fix(engine): capture baseCommitSha against local main, not origin/main#1376
Conversation
In-review tasks showed other tasks' files in their "files changed" list. Task branches fork from local main, but the base capture measured merge-base(HEAD, origin/main) — when local main carried merged-but-unpushed task commits, the recorded base rewound past them, and after the post-merge rebase-and-push rewrote those SHAs, baseCommitSha..HEAD permanently swept the predecessors' files into the new task's diff. Extract the capture into base-commit-capture.ts, measure local main first (origin/main fallback) to match the contamination-base sites, and add a real-git regression suite covering local-ahead-of-origin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 7 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extracts inline git merge-base logic into a dedicated ChangesBase Commit SHA Resolution Refactoring
Sequence DiagramsequenceDiagram
participant Executor as Executor<br/>(captureBaseCommitSha)
participant Resolver as resolveCapturedBaseCommitSha
participant Git as git command
Executor->>Resolver: resolveCapturedBaseCommitSha(worktreePath, logger)
Resolver->>Git: git merge-base HEAD main
alt merge-base main succeeds
Git-->>Resolver: base SHA
else merge-base main fails
Resolver->>Git: git merge-base HEAD origin/main
alt merge-base origin/main succeeds
Git-->>Resolver: base SHA
else merge-base origin/main fails
Resolver->>Git: git rev-parse HEAD
alt rev-parse succeeds
Git-->>Resolver: HEAD SHA
else all fail
Resolver-->>Resolver: return undefined
end
end
end
Resolver-->>Executor: resolved SHA or undefined
alt SHA resolved
Executor->>Executor: update task.baseCommitSha<br/>emit audit event
else no SHA
Executor->>Executor: throw error<br/>catch as non-fatal
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 fixes a diff-contamination bug where in-review tasks displayed 20–31 "files changed" when they only touched 2–12, because
Confidence Score: 5/5Safe to merge — the change is a targeted single-command swap backed by real-git integration tests covering every branch of the fallback chain. The fix correctly aligns baseCommitSha capture with the two existing contamination-base sites that were already local-main-first. The shell command swap is minimal and the fallback ladder (local main to origin/main to HEAD) is exercised end-to-end by the new real-git test suite. The executor refactor is a straight delegation to the new helper with equivalent error semantics. No logic regressions are visible. No files require special attention — the new module and its tests are self-contained and the executor change is small. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[captureBaseCommitSha called after worktree acquisition] --> B{isResume and task.baseCommitSha?}
B -- Yes --> C[git merge-base --is-ancestor stored SHA HEAD]
C -- passes --> D[Preserve stored baseCommitSha]
C -- stale or fails --> E[Recapture]
B -- No --> E
E --> F[resolveCapturedBaseCommitSha]
F --> G[git merge-base HEAD main or git merge-base HEAD origin/main]
G -- succeeds --> H[baseCommitSha = SHA]
G -- both fail --> I[log warn]
I --> J[git rev-parse HEAD]
J -- succeeds --> H
J -- fails --> K[return undefined]
H --> L{baseCommitSha defined?}
K --> L
L -- undefined --> M[throw Error - caught by outer try-catch]
L -- defined --> N[store.updateTask baseCommitSha]
style G fill:#d4edda,stroke:#28a745
style M fill:#fff3cd,stroke:#ffc107
Reviews (3): Last reviewed commit: "Merge branch 'main' into gsxdsm/filescha..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * Returns `undefined` only when every git invocation fails (caller treats a | ||
| * missing base as non-fatal). |
There was a problem hiding this comment.
The comment says "caller treats a missing base as non-fatal", but the only real caller — executor.ts — immediately throw new Error("could not resolve base commit SHA") when the return value is undefined. Non-fatal treatment only applies one level further out, in the outer captureBaseCommitSha try-catch. The mismatch could mislead a future caller into expecting the function to be safe to ignore on undefined, when the established usage pattern is to throw.
![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/base-commit-capture.ts (2)
1-4: ⚡ Quick winConsider adding timeout to exec calls for defensive coding.
While the coding guideline allows
execSyncfor "short deterministic git plumbing" (implying timeouts aren't strictly required), git operations can occasionally hang on corrupted repositories, network-mounted paths, or slow disks. Adding an explicit timeout would prevent indefinite hangs.⏱️ Example: Add timeout to execAsync wrapper
-const execAsync = promisify(exec); +const execAsync = promisify(exec); +const GIT_PLUMBING_TIMEOUT_MS = 10_000; // 10 seconds for git plumbingThen use
{ cwd: worktreePath, encoding: "utf-8", timeout: GIT_PLUMBING_TIMEOUT_MS }in the exec options.🤖 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/base-commit-capture.ts` around lines 1 - 4, Add a default timeout for git plumbing exec calls by introducing a GIT_PLUMBING_TIMEOUT_MS constant and replacing plain promisify(exec) usage with a small wrapper (e.g., execWithTimeout or change execAsync to accept default options) that merges provided options with { encoding: "utf-8", timeout: GIT_PLUMBING_TIMEOUT_MS } before calling exec; update callers in this module that use execAsync to call the new wrapper so all git exec calls (used in base-commit-capture.ts) will time out instead of hanging indefinitely.
32-36: ⚡ Quick winConsider splitting shell OR into JavaScript fallback for better portability and clarity.
The current implementation uses shell-specific syntax (
2>/dev/nulland||) that relies on Unix shell behavior. On native Windows (cmd.exe), the2>/dev/nullredirect would fail. While Git Bash typically provides Unix shell semantics, splitting this into separate JavaScript try-catch blocks would be more explicit, portable, and easier to follow.♻️ Proposed refactor using JavaScript fallback chain
let baseCommitSha: string | undefined; try { - const { stdout } = await execAsync( - "git merge-base HEAD main 2>/dev/null || git merge-base HEAD origin/main", - { cwd: worktreePath, encoding: "utf-8" }, - ); - baseCommitSha = stdout.trim() || undefined; + // Try local main first + try { + const { stdout } = await execAsync("git merge-base HEAD main", { + cwd: worktreePath, + encoding: "utf-8", + }); + baseCommitSha = stdout.trim() || undefined; + } catch { + // Fall back to origin/main + const { stdout } = await execAsync("git merge-base HEAD origin/main", { + cwd: worktreePath, + encoding: "utf-8", + }); + baseCommitSha = stdout.trim() || undefined; + } } catch (err: unknown) { const errorMessage = err instanceof Error ? err.message : String(err); logger?.warn(`merge-base failed, falling back to HEAD: ${errorMessage}`); }This approach makes the fallback chain explicit in JavaScript and works identically across platforms.
🤖 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/base-commit-capture.ts` around lines 32 - 36, The current execAsync call uses shell-specific redirects and || chaining which breaks on non-Unix shells; change the logic in base-commit-capture.ts where execAsync is called (the call that sets baseCommitSha) to run two explicit commands in JavaScript: first await execAsync("git merge-base HEAD main", { cwd: worktreePath, encoding: "utf-8" }) in a try/catch, and if that fails or yields empty stdout, call await execAsync("git merge-base HEAD origin/main", { cwd: worktreePath, encoding: "utf-8" }); trim the stdout and assign to baseCommitSha (or undefined) — remove the "2>/dev/null ||" combined-shell usage so behavior is portable across platforms.
🤖 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/base-commit-capture.ts`:
- Around line 1-4: Add a default timeout for git plumbing exec calls by
introducing a GIT_PLUMBING_TIMEOUT_MS constant and replacing plain
promisify(exec) usage with a small wrapper (e.g., execWithTimeout or change
execAsync to accept default options) that merges provided options with {
encoding: "utf-8", timeout: GIT_PLUMBING_TIMEOUT_MS } before calling exec;
update callers in this module that use execAsync to call the new wrapper so all
git exec calls (used in base-commit-capture.ts) will time out instead of hanging
indefinitely.
- Around line 32-36: The current execAsync call uses shell-specific redirects
and || chaining which breaks on non-Unix shells; change the logic in
base-commit-capture.ts where execAsync is called (the call that sets
baseCommitSha) to run two explicit commands in JavaScript: first await
execAsync("git merge-base HEAD main", { cwd: worktreePath, encoding: "utf-8" })
in a try/catch, and if that fails or yields empty stdout, call await
execAsync("git merge-base HEAD origin/main", { cwd: worktreePath, encoding:
"utf-8" }); trim the stdout and assign to baseCommitSha (or undefined) — remove
the "2>/dev/null ||" combined-shell usage so behavior is portable across
platforms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a0243ba-4144-4809-a723-d6549e85d795
📒 Files selected for processing (4)
.changeset/fix-base-commit-sha-local-main.mdpackages/engine/src/__tests__/base-commit-capture.real-git.test.tspackages/engine/src/base-commit-capture.tspackages/engine/src/executor.ts
…lary Document the FN-5937 files-changed inflation root cause in docs/solutions/ and add the Branching & diff attribution cluster (Integration branch, Fork point, Rebase-and-push, Contamination) to CONCEPTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
In-review tasks on the board could show 20–31 "files changed" when they actually touched 2–12 — the extra files belonged to other tasks that had already merged. Task branches fork from local
main, butbaseCommitShawas captured asmerge-base(HEAD, origin/main): whenever local main carried merged-but-unpushed task commits, the recorded base rewound past them, and once the post-merge rebase-and-push rewrote those SHAs,baseCommitSha..HEADpermanently swept the predecessors' files into the new task's diff (display recovery can't tighten because the orphaned SHAs are no longer in main).The capture now measures against local main first with
origin/mainas fallback — matching the two contamination-base sites (worktree-acquisition.ts,auto-recovery-handlers/branch-worktree.ts) that were already local-first; this was the only origin-first site. The logic is extracted intobase-commit-capture.tswith a real-git regression suite, including the local-ahead-of-origin scenario that fails against the old ordering.The five affected live in-review tasks had their stored bases repaired separately (board counts dropped from 21–24 files to their true 2–7). Verified with the full engine suite (6,256 tests) plus the new 4-test real-git suite.
Summary by CodeRabbit
Bug Fixes
Tests