Compare task coverage with the base branch

[LarryHu0217]

Summary

• load coverage from the task worktree and an existing worktree on the configured base branch without switching or modifying either checkout
• show base/task aggregate coverage and signed per-file line-coverage deltas, including explicit new, deleted, renamed, missing-report, missing-file, and no-executable-lines states
• report materially impacted unchanged files and add the comparison to merge-readiness details while preserving the existing task-only fallback

Validation

• npm test (1,634 passed, 23 skipped)
• npm run check
• npm run lint:dead
• npm run lint:arch
• npm run build:frontend

Closes #236

[johannesjo]

Summary

The read-only base-worktree approach and explicit coverage states are solid, but I reproduced three correctness gaps against the current head (62ee9fc6).

PR Discussion Context

This implements #236. There are no existing review threads, and both the quality and GitGuardian checks pass; the cases below are not covered by the added tests.

Issues

Important

• src/lib/coverage-comparison.ts:116 — impactedUnchangedFiles only iterates task-report entries and skips zero-line counterparts. If an unchanged file covered in the base report disappears from the task report, or becomes no-executable-lines, it is omitted. Removing a low-covered file can raise the aggregate, so merge readiness reports Coverage pass while the regression is invisible. Iterate the union of base/task paths and retain the unavailable states for unchanged files, with base-only and available→no-lines tests.

• src/components/ChangedFilesList.tsx:312, line 839 — Badge eligibility is being used as the complete changed-path/global-display scope. A changed test/declaration file present in coverage can therefore be labeled “unchanged,” and a test-only change hides the aggregate delta and impacted-file indicator from the Changed Files panel entirely. Pass all Git-changed paths into comparison bookkeeping, keep candidate filtering only for per-row badges, and gate the global footer on report/comparison availability rather than candidate count.

• electron/ipc/git.ts:541 — Any ordinary filename containing literal => is parsed as rename shorthand. With a modified ordinary => modified.txt, Git emits that exact path in both raw and numstat output, but this parser produces a phantom modified.txt row with the stats plus the real path with zero stats, breaking counts, navigation, and coverage matching. Prefer the exact raw-status path before parsing rename shorthand, or switch these calls to -z; add a literal-arrow filename regression test.

Strengths

• Baseline discovery and report loading remain read-only.
• The IPC channel wiring is consistent, and the ordinary numeric/new/deleted/renamed cases have focused tests.

Verdict

Needs changes. The first issue can produce a false-green readiness result, while the other two make the displayed comparison inaccurate for valid inputs.

[LarryHu0217]

Addressed the review findings in 4436846:

• compares the union of base/task coverage paths and preserves unavailable states for unchanged files
• uses every Git-changed path for comparison bookkeeping while keeping per-row badges source-only
• keeps the global coverage footer visible for report-backed test/declaration-only changes
• preserves literal => filenames by preferring the exact raw-status path

Added regression coverage for base-only and available-to-no-lines transitions, non-source changed paths, test-only footer visibility, and literal-arrow filenames. Local check:static, security-rule tests, and the full suite pass (1640 passed, 23 skipped).

[johannesjo]

Summary

I rechecked the current head (4436846) against #236 and the follow-up fixes. The original reported cases are addressed, but two PR-introduced correctness gaps remain. The overall design is proportionate to the feature; I found no KISS/YAGNI premise issue or reason to add a broader abstraction or dependency.

PR Discussion Context

There are no inline review threads, and both quality and GitGuardian pass. The cases below are not covered by the current tests.

Issues

Important

• src/components/ChangedFilesList.tsx:323 — files() is selection-scoped UI state, not a reliable base-relative comparison inventory. Reports can resolve while it is still empty, briefly classifying changed paths as impactedUnchangedFiles in merge readiness; if inventory loading fails, that state persists. In uncommitted-only mode it permanently contains HEAD-to-working-tree rows while the reports remain whole-task versus base, so committed branch changes are labeled “unchanged” and branch-added/renamed files edited locally lose their base-relative kind and rename delta. Keep a nullable base-relative GetChangedFiles inventory for comparison, share the all-changes result when possible, retain files() for displayed rows, and suppress comparison output until the canonical inventory loads. Add delayed/failed-load and uncommitted-selection regression tests.

• src/lib/coverage-comparison.ts:62 — Treat report paths as own dictionary entries. Because this head now compares every Git-changed path, a valid root file named toString, constructor, or __proto__ that is absent from a loaded report resolves through Object.prototype; line 64 then throws on file.lines.total, breaking the Changed Files and merge-dialog comparison. Use Object.hasOwn for the lookup and a null-prototype result record, with a parameterized regression test for these names.

Strengths

• Base-worktree discovery and report loading remain read-only.
• The explicit coverage-state model and pure comparison helper are justified by the acceptance criteria, and the follow-up tests correctly cover the original narrow regressions.

Verdict

Needs changes. Both fixes can remain small and local, but the current behavior can misclassify common uncommitted views and throw for valid repository paths.

[LarryHu0217]

Addressed both follow-up findings in 1e2ae56:

• Coverage comparison now waits for a nullable, base-relative GetChangedFiles inventory. Uncommitted-only views retain their displayed rows while merge-readiness uses the complete task inventory; failed loads suppress comparison output.
• Coverage report lookups now use own entries and the result map has a null prototype, including regressions for toString, constructor, and __proto__.

Validation: 1,647 tests passed; typecheck, ESLint, Knip, dependency-cruiser, security-rule tests, frontend/remote builds, Electron compilation, and MCP bundling passed.

[johannesjo]

Summary

Re-reviewed at head 1e2ae56 with a KISS/YAGNI lens. All five items from the previous two rounds are genuinely fixed — I verified each against the code, not just the changelog:

• union of base/task report paths, with available → no-lines and base-only states retained (coverage-comparison.ts:117-135)
• all Git-changed paths feed comparison bookkeeping; isCoverageCandidate is now only a per-row badge filter (ChangedFilesList.tsx:54-74)
• literal => filenames resolve via the exact raw-status path first (git.ts:596-599)
• nullable base-relative inventory backs the comparison (ChangedFilesList.tsx:553-562)
• Object.hasOwn + null-prototype result map, with the parameterized toString/constructor/__proto__ test

I also confirmed the no-baseline fallback is preserved exactly: with baseCoverage === null, badges resolve to the old plain 82% + coverageColor, and deleted files still suppress no cov. That was the acceptance criterion easiest to break.

I agree with the earlier assessment that there is no premise-level KISS/YAGNI objection to the feature — #236 is a specific request and the shape of the implementation matches it. The concerns below are one design gap and several local simplifications.

Fundamental Question

Is "whatever worktree is currently on the base branch" a baseline? getBranchWorktreePath finds a worktree checked out on the base branch, and readCoverageSummary reads whatever coverage file happens to sit in it. Nothing anchors that report to a commit: it may have been generated weeks ago, at a commit far from the task's merge-base, or with that worktree dirty. The task report has the same freedom. So base 82% → task 78% (-4pp) may be entirely attributable to elapsed time rather than to the task — and it now drives a merge-readiness check.

Both reports already carry generatedAt (file mtime, coverage.ts:161), so the cheap fix is available: surface both timestamps, and hold the Coverage check at neutral when the base report predates the task's merge-base. That keeps the read-only design while making the number honest about what it is.

Issues

Important

• merge-readiness.ts:193 — The aggregate warning has no materiality threshold: aggregate.delta < 0 alone flips the check to warning and overall readiness to attention. Per-file impacts are gated on MATERIAL_COVERAGE_DELTA = 1, so the two halves of the same check disagree on what "regression" means. roundPercentage keeps two decimals, so -0.01pp of drift between two independently-generated whole-repo reports produces a standing "attention" on every task. Apply the same threshold (or a smaller, explicitly named one) to the aggregate.

• ChangedFilesList.tsx:374-402 — Neither the footer label, aggregateCoverageTitle (which lists only reportPath), nor the merge-readiness detail exposes either report's generatedAt. And because the new aggregate span replaces the old one, this removes the only place generatedAt was surfaced (coverageFooterTitle, line 121). Enabling the feature strictly loses the freshness signal that existed before it. Add both timestamps to aggregateCoverageTitle.

• ChangedFilesList.tsx:920 — <Show when={!aggregateCoverageLabel() && touchedCoveragePct() !== null}> hides the changed-files coverage percentage (◔ 82%) whenever a base report is found, substituting the whole-repo aggregate. #236 asks to "prioritize changed/new files, while still reporting materially impacted unchanged files" — this inverts it. For a three-file task the whole-repo aggregate barely moves, so the headline number becomes the least informative one while the actionable one disappears. Render both rather than substituting.

• ChangedFilesList.tsx:553-562 and :672 — In uncommitted-only mode every 5s poll now issues a second full GetChangedFiles (pinHead + detectDiffBase + merge-base git diff --raw --numstat, ~5 extra processes) on top of GetUncommittedChangedFiles, and resolveBaseCoverageRoot runs git worktree list --porcelain on every coverage poll. None of it is gated on a coverage report existing, so projects that never run coverage pay the full cost and discard the result. Gate both on coverage() !== null || baseCoverage() !== null.

  Related: setCoverage/setBaseCoverage/setComparisonFiles receive fresh objects each poll, so coverageComparison rebuilds the whole comparison — Set construction over every report path, plus the sort — every 5 seconds even when nothing changed, and re-fires onCoverageComparisonChange into buildMergeReadiness.

KISS / YAGNI

• coverage-comparison.ts:155-163 + ChangedFilesList.tsx:76-84 — Two exported wrapper layers exist to perform two null checks. buildCoverageComparisonIfReady only converts changedFiles === null into null; buildCoverageComparisonForSelection wraps that to add the commit-hash check. Both are exported solely so tests can reach them. Inlining into the memo deletes ~15 lines, one exported symbol, and a test block:

  const coverageComparison = createMemo(() => {
    const inventory = comparisonFiles();
    if (!inventory || isCommitHashSelection(props.selectedCommit)) return null;
    return buildCoverageComparison(coverage(), baseCoverage(), inventory);
  });

• ChangedFilesList.tsx:64-74 — shouldShowCoverageFooter(selectedCommit, count, hasCoverageArtifact, hasBaseCoverageArtifact) takes four positional params, the last two both booleans and adjacent — transposing them at the call site is silent. The test reads shouldShowCoverageFooter(undefined, 0, true, true), which conveys nothing. The body is !isCommitHashSelection(sel) && (count > 0 || hasTask || hasBase); either inline it or take an options object, matching changedFilesFromMaps in this same PR.

• git.ts:539-561 — parseNumstatPath returns previousPath, but that value is already set from the --raw R/C lines at git.ts:582-585, which is the authoritative source and always precedes the numstat block in combined output. Line 602 just rewrites the same value. The function only needs to return the destination path, which drops the previousPath construction from both branches.

  Broader (follow-up, not a blocker): -z remains the simpler route. It removes rename shorthand, quoting, and escaping from the wire format entirely — deleting parseNumstatPath, the statusMap.has(normalizedPath) ambiguity heuristic, and the quote/backslash handling in normalizeStatusPath. It would also fix a pre-existing bug this PR makes more consequential: normalizeStatusPath unescapes \\(.) without decoding octal, so with the default core.quotePath=true a path like "src/caf\303\251.ts" becomes src/caf303251.ts — which now silently reads as "not present" in coverage matching. Honest accounting: -z touches the shared parser and all four callers, so it is a real refactor, not one flag.

Minor

• ChangedFilesList.tsx:613 — The branch fallback lost its uncommittedOnly filter (was setFiles(uncommittedOnly ? result.filter((f) => !f.committed) : result)). Since GetChangedFilesFromBranch marks everything committed: true, that path previously yielded [] in uncommitted view and would now render the whole branch diff as "uncommitted". Reaching it needs an empty worktreePath plus an uncommitted selection, which I could not construct from current call sites — but it is an unexplained behavior change with a free fix: keep the filter on setFiles and pass the unfiltered result to setComparisonFiles.

• ChangedFilesList.test.ts:85-90 — it.each(['pending', 'failed']) ignores its parameter and asserts on the identical input twice, so it reports two passing cases while covering one. This is the test standing in for the requested delayed/failed-load coverage; the failed-load path (the .catch at line 560) is not distinguished from the initial-null path.

• MergeDialog.tsx:452, TaskChangedFilesSection.tsx:155 — branchName is added at both call sites, but the coverage code only reads projectRoot. Since the fallback is gated on projectRoot && branchName, adding it is precisely what enables branch-based fallback — and its permanent poll shutdown plus canOpenFilesInEditor: false — in these two surfaces for the first time. Possibly desirable, but it is an unrelated behavior change riding along; drop it or note it in the description.

• Smaller: base 80% → task 78% (-2pp) is prose in a footer whose other items are compact glyphs (△ 3, ∅ 1, ▤ 7), so it dominates the row and wraps on narrow panels; the ↕ N other badge reuses aggregateCoverageTitle(), which truncates to three files with no way to see the rest; and getBranchWorktreePath filters on !entry.detached but only the missing-branch case is tested.

Strengths

• The read-only constraint from #236 holds end-to-end — no checkout, no worktree creation, no test execution.
• The four-state CoverageValueState model is the right shape for the acceptance criteria and keeps the UI branches flat and readable.
• The previous_path plumbing is careful, handles both {a => b} and bare-arrow numstat forms, and correctly prefers the unambiguous raw-status path.

Verdict

Needs changes. The architecture is sound and the prior rounds' findings are resolved. What remains is that a check gating merge readiness rests on two unanchored snapshots with no materiality threshold and no freshness signal. The blocking pieces are small and local: the aggregate threshold (merge-readiness.ts:193), generatedAt in the tooltip, and a one-line Show condition at ChangedFilesList.tsx:920. The KISS items are cleanup that can land alongside.

[LarryHu0217]

Addressed the latest review in b887d66. Coverage now records the task merge-base timestamp and keeps a stale base report neutral; aggregate warnings use the existing 1pp materiality threshold; both changed-file and aggregate coverage remain visible with task/base generation times; no-report projects skip the extra comparison/base-worktree Git work; unchanged polling snapshots preserve object identity; the wrapper exports and redundant numstat previous-path parsing are removed; and branch fallback retains the uncommitted filter. Verification: 96 focused tests, full suite 1,647 passed / 23 skipped, npm run check, Knip, and dependency-cruiser.

[johannesjo]

Summary

Re-reviewed at head b887d66. All ten items from round 3 are genuinely fixed — I verified each against the code rather than the changelog:

Round 3 item	Resolution at b887d66
Aggregate warning had no materiality threshold	aggregate.delta <= -MATERIAL_COVERAGE_DELTA (merge-readiness.ts:205)
Feature removed the only generatedAt signal	Both reports' timestamps in aggregateCoverageTitle (ChangedFilesList.tsx:387-389)
Aggregate substituted for ◔ 82%	!aggregateCoverageLabel() && dropped from both <Show>s (:976, :995); they co-render
No-coverage projects paid full poll cost	comparisonEnabled (:544) + if (taskResult) (:716)
Comparison rebuilt every 5s	sameChangedFiles / sameCoverageSummary preserve identity across no-op polls
Two wrapper layers for two null checks	Inlined into the memo (:344-348); all three exports gone repo-wide
shouldShowCoverageFooter's 4 positional params	Replaced by the zero-arg showCoverageFooter memo (:337-343)
Redundant previousPath from numstat	parseNumstatPath returns a plain string; only --raw R/C lines write previousPathMap (git.ts:575-577)
Branch fallback lost uncommittedOnly	Filter on setFiles, unfiltered result to setComparisonFiles (:643-654)
Freshness design gap	GetMergeBaseTimestamp + baseline.stale → neutral, with tests

CI is green on this head. The remaining items are one incomplete half of the freshness fix and a set of local cleanups.

Issues

Important

• ChangedFilesList.tsx:960-975 — The staleness gate landed in merge readiness but not in the display it exists to qualify. grep -n 'stale\|baseline' over this file returns only lines 395-397, inside aggregateCoverageTitle. The footer span colors purely on deltaColor(aggregate.delta), so a stale base report still paints base 82% → task 78% (-4pp) in theme.error at font-weight: 600. Per-file badges have the same gap (:187-192).

  This is reachable exactly where it hurts: merge-readiness.ts:185 is guarded by the returns at 159-184, so it only fires when task and base are both available and delta !== null — the same state that paints the red delta.

  Scope, to be fair to the fix: in the merge dialog MergeReadinessPanel renders {check.detail} as visible text (MergeReadinessPanel.tsx:93), so there the user sees a contradiction rather than a silent lie. But ChangedFilesList also renders in TaskChangedFilesSection.tsx:152, DiffViewerDialog.tsx:436, arena/BattleScreen.tsx:138 and arena/ResultsScreen.tsx:233, none of which show a readiness panel — there the title tooltip is the only cue. Fix is local: branch the color on coverageComparison()?.baseline?.stale to theme.fgMuted, matching the neutral status the check already assigns.

Minor

• ChangedFilesList.tsx:699-735 — cachedMergeBase is keyed on taskResult.generatedAt, which is the coverage file's mtime (coverage.ts:161). The merge-base is a function of git history and has no relationship to it. Concrete failure: the merge dialog's own "Rebase onto <base>" button (MergeDialog.tsx:265-309) rebases in place and refetches merge status, branch log and worktree status — but changes no ChangedFilesList prop, so the effect doesn't restart and the closure keeps serving the pre-rebase merge-base. The rebase moves the merge-base forward, so baseGeneratedTime < mergeBaseTime under-reports, and a now-genuinely-stale base report reads as fresh. Rebase-then-check-readiness is the exact workflow this check was added for. (It self-heals if coverage is re-run afterwards, which changes the key.) Caching here is right — getMergeBaseTimestamp fans out to 6-8 git subprocesses — but cache it for the effect lifetime, not on an unrelated value. Related: resolveBaseCoverageRoot → git worktree list --porcelain is the one call that genuinely repeats every 5s and is uncached at every layer; and GetMainBranch moved from hoisted-outside-refresh to inside it (cheap, since detectMainBranch has a 60s TTL cache, but it's an unnecessary per-poll IPC round trip).

• git.ts:1598-1610 — getMergeBaseTimestamp conflates "couldn't determine" with a value, in both directions:

  • Fail-closed: line 1602 is const mergeBase = picked?.sha ?? head. pickMergeBase returns null at git.ts:305 (branch missing locally and as origin/<branch>) and at :321 (merge-base fails for both). Substituting the task's own HEAD yields a timestamp newer than essentially any base report, so stale is permanently true and the Coverage check sits at neutral forever, blaming report freshness for something else. The :305 route mostly coincides with having no base report anyway (no worktree can be checked out on a branch that doesn't exist), so the live case is :321 — unrelated histories or a merge-base failure. Returning null when picked is null would be both simpler and honest.
  • Fail-open: the catch returns null, coverage-comparison.ts:149-157 then leaves baseline undefined, and coverage.baseline?.stale at merge-readiness.ts:185 is falsy — so the delta drives pass/warning completely unguarded, exactly as before the fix, and aggregateCoverageTitle guards on if (comparison.baseline) so nothing tells the user freshness couldn't be established. Narrow (needs a resolved base report plus a failed timestamp lookup), but silent.

• src/components/ChangedFilesList.test.ts — This commit deletes the buildCoverageComparisonForSelection and shouldShowCoverageFooter describe blocks along with the exports, leaving the file byte-identical to main. Fair framing: those tests were added by this PR in 1e2ae56, so this isn't a regression against main — but 'uses the canonical whole-task inventory for an uncommitted-only display selection' was the guard for round 2's Important finding, and it's now gone. The behavior survives the inlining (:344-348 for the suppression guards, :574-607 for the separate base-relative fetch in uncommitted mode), but I searched all 103 *.test.* files: nothing else covers it. coverage-comparison.test.ts exercises the pure builder given an inventory and cannot observe which inventory the component passes, and no test file references ChangedFilesList at all. Agreed the deleted it.each(['pending','failed']) was low-value — it ignored its parameter — but the uncommitted-selection case was not.

• ChangedFilesList.tsx:177-183 — Dead branch. comparisonBadge's 'no report' case needs task.state === 'no-report' && base.state !== 'no-report'. baseResult is declared null at :714 and assigned only inside if (taskResult) (:716) → if (baseRoot) (:737), and fileValue returns 'no-report' only when the whole summary is null (a file absent from a loaded report gives 'file-not-present'). So the condition is exactly coverage() === null && baseCoverage() !== null — unreachable. Same root cause makes || hasBaseCoverageArtifact() redundant in showCoverageFooter (:342) and 'Task: no coverage report.' unreachable in aggregateCoverageTitle (:390). Worth noting the batch() at :745 is load-bearing for this: it writes setCoverage before setBaseCoverage, so without the batch Solid's per-write flush would briefly expose that exact state.

• ChangedFilesList.tsx:544 — comparisonEnabled is a tracked read in the effect body, so the whole polling effect tears down and restarts when either boolean flips. Because both are boolean memos with default === equality, this is only the null ↔ report transition, not every regeneration — so it costs one extra teardown/refetch on task open, with setCanOpenFilesInEditor(false) (:548) in between. The buttons are opacity: 0 until row hover (styles.css:1386-1403), so it's mostly invisible DOM churn. Flagging because the fix is one untrack, and because the same restart is why setComparisonFiles(null) had to be moved out into its own effect at :529-534 — setCanOpenFilesInEditor(false) was left behind.

• Carried over from round 3, still open — the footer now shows two unlabeled percentages side by side (whole-repo base 80% → task 78% (-2pp) as prose, next to changed-files ◔ 91% as a glyph), distinguishable only by hovering; ↕ N other (:1026-1033) still reuses aggregateCoverageTitle(), which truncates the impacted list at 3; and branchName is still passed at MergeDialog.tsx:452 / TaskChangedFilesSection.tsx:155 — the one prop here unrelated to coverage, since it newly enables the GetChangedFilesFromBranch fallback at those two call sites — with no mention in the PR description.

Strengths

• Every round-3 item is genuinely fixed, and the fixes are the small local ones proposed rather than rewrites.
• Declining the -z refactor while keeping the narrow literal-=> fix was the right scope call.
• GetMergeBaseTimestamp is wired completely and consistently — manifest, register.ts, preload allowlist — with a focused test asserting both ISO-UTC normalization and the exact argv.
• The read-only constraint from #236 still holds end-to-end.

Verdict

Needs changes — one blocking item. The freshness fix landed in merge-readiness.ts but not in the footer and badges that display the number, so outside the merge dialog the panel shows a red regression the readiness check has already decided to ignore. That's a color condition. The ?? head fallback at git.ts:1602 is a second one-liner worth taking with it; the rest is cleanup that can land alongside.

[LarryHu0217]

Addressed the coverage freshness feedback in 4bd754d:

• stale base-report deltas now render neutral in both per-file badges and the aggregate footer, with tooltip context that they are informational
• merge-base timestamp lookup returns unknown when no merge-base can be determined, instead of using the task HEAD as a false baseline; added an IPC regression test

Validation: focused git IPC tests, static checks, and the full suite pass (1,649 passed, 23 skipped).

[johannesjo]

Summary

Re-reviewed at head 4bd754d. Both round-4 items are fixed, verified against the code rather than the changelog:

Round 4 item	Resolution at 4bd754d
Stale baseline still painted as a red regression	comparisonBadge takes baselineStale → theme.fgMuted + tooltip clause (ChangedFilesList.tsx:191-192), threaded from :907; aggregate footer greys on baseline?.stale (:969-975)
?? head fallback in getMergeBaseTimestamp	if (!picked) return null (git.ts:1602), with a test asserting git show never fires

CI is green. What follows is one leftover span from the first fix, one consequence of the second that I got wrong in round 4 and want to state correctly, and a design question about what the freshness check actually measures.

Issues

Important

• ChangedFilesList.tsx:1032-1039 — The staleness gate reached two of the three coverage indicators in this footer. ↕ N other is still hardcoded theme.warning:

  <Show when={(coverageComparison()?.impactedUnchangedFiles.length ?? 0) > 0}>
    <span title={aggregateCoverageTitle()} style={{ color: theme.warning, 'font-weight': '600' }}>

  It is the indicator merge readiness discards most completely when stale: the neutral return at merge-readiness.ts:185 strictly precedes the only read of impactedUnchangedFiles at :193, and that is the sole non-test consumer of the field. So with a stale base the readiness panel says "regenerate it before comparing" while the footer shows ↕ 4 other in warning orange next to a now-grey aggregate.

  Not cosmetic: set membership is itself delta-derived (Math.abs(delta) < MATERIAL_COVERAGE_DELTA → continue, coverage-comparison.ts:132), so a stale base both invents and hides entries. This badge is exactly as untrustworthy as the deltas this commit just muted, and it is now the only warning-colored base-derived signal left. One line: color: coverageComparison()?.baseline?.stale ? theme.fgMuted : theme.warning. (The tooltip is already correct — it reuses aggregateCoverageTitle, which carries the stale sentence.)

• Unknown freshness now renders as a confident pass. git.ts:1602-1611 + ChangedFilesList.tsx:730-739 + coverage-comparison.ts:149-157 — I flagged this direction as narrow in round 4; having traced it properly, I had the harm model wrong and it is wider than I said. Correcting both:

  null → baseline: undefined → coverage.baseline?.stale is falsy at merge-readiness.ts:185 → the delta drives pass/warning with no freshness guard, and aggregateCoverageTitle's if (comparison.baseline) guard at :399 means the tooltip says nothing either. buildCoverageComparison cannot distinguish "no base report" from "base report present, merge-base unknown" — both are undefined.

  Where I was wrong in round 4: I described the ?? head removal as trading a false-neutral for an unguarded verdict, implying the old behavior was safer. It wasn't uniformly. neutral and pass both roll up to overall: 'ready' (merge-readiness.ts:219-225), so for the delta <= -1 subset this change is strictly stricter now (warning → attention). The real regression is the delta > -1 subset: b887d66 showed a muted check with "regenerate it before comparing"; 4bd754d shows a green ✓ and Base X% → task Y% (+Npp) computed against a report of unknown provenance. Also, my "head is newer than essentially any base report" was an overstatement — any base report regenerated after the task's last commit already yielded stale: false and was already unguarded.

  Where it is wider than I said: I scoped reachability to pickMergeBase returning null (git.ts:305/:321, and :324 is unreachable TS narrowing). But mergeBaseAt === null has three further sources independent of that: an unparseable %cI (git.ts:1608), the bare catch swallowing any exec failure (:1609-1611), and .catch(() => null) on the IPC invoke (ChangedFilesList.tsx:736). And cachedMergeBase is keyed on taskResult.generatedAt, so a single transient failure is cached and replayed on every 5s poll until the coverage report is regenerated — one flaky exec pins the confidently-green verdict for the rest of the session. A third baseline state ("present, unanchored") costs about what stale did and would let both the check and the tooltip say so.

• The freshness check is one-directional, and its vocabulary doesn't match its mechanism. stale = baseGeneratedTime < mergeBaseTime compares a report file's mtime (coverage.ts:161/:272, stat.mtime.toISOString()) against the merge-base commit time. But nothing anchors the base report to any commit: resolveBaseCoverageRoot → getBranchWorktreePath matches !entry.detached && entry.branchName === branchName and returns a path, and the report is read straight off that working tree. That worktree sits at the base branch's current HEAD.

  In this app that drift is the mainline, not an edge case — mergeTask merges task branches into the base branch in projectRoot (git.ts:1724-1777), i.e. into exactly the worktree that later serves as the baseline. So for any task older than the last merge, a base report regenerated today has mtime > mergeBaseTime → stale: false → the delta ships as clean while measuring a tree containing everything merged since the task branched. Other people's work can manufacture or mask the delta <= -1 warning.

  The sharpest form: following the check's own instruction — "regenerate it before comparing" (merge-readiness.ts:189) — necessarily produces a report measuring base HEAD, the tree furthest from the merge-base. The action guaranteed to clear the warning maximizes the unmeasured drift. Meanwhile the tooltip prints Task merge-base: <ts> beside the base number, which reads as anchoring that never happens.

  I don't think #236's read-only constraint excuses this, because the issue is detection, not measurement. Both values needed are already computed and thrown away: parseWorktreeList (git.ts:697-733) parses the HEAD <sha> line from git worktree list --porcelain and discards it (your own fixture feeds it — git.test.ts:1487), and getMergeBaseTimestamp holds picked.sha at :1603. Comparing them costs zero extra git invocations.

  Fair counterpoint, and I'd accept either resolution: #236 cites GitHub Code Quality, which compares a branch against the default branch's head — so base-HEAD semantics are defensible prior art, and the PR body ("an existing worktree on the configured base branch") reads that way. If that's the intent, then the wording is the defect: drop "merge-base" from the user-facing strings, say the baseline is "<base> as currently checked out," and compare the report mtime against the base worktree's HEAD commit time rather than the merge-base's — the current reference is wrong under both readings, since mergeBaseTime <= baseHeadCommitTime always.

Minor

• ChangedFilesList.tsx:730-739 (carried from round 4, unchanged) — beyond the sticky-failure case above, keying the merge-base cache on a coverage-file mtime still breaks the rebase workflow. I re-verified the mechanism: the Rebase onto <base> handler (MergeDialog.tsx:273-280) calls RebaseTask then refetches mergeStatus/branchLog/worktreeStatus — none of which feed any of the eight props at MergeDialog.tsx:449-458, and RebaseTask emits nothing to the renderer. Tasks live in a fine-grained createStore, so unchanged strings can't notify. The effect never restarts and the closure serves the pre-rebase merge-base. Cache for the effect lifetime instead.

• Dead code, all from the same invariant (carried, unchanged) — baseResult is declared null at :718 and assigned only inside if (taskResult) (:720-747), and the three writes are wrapped in batch() at :749, so non-null base ⟹ non-null task with no observable transient. Therefore: comparisonBadge's 'no report' branch (:178-184) can never fire, || hasBaseCoverageArtifact() is redundant in showCoverageFooter (:346), and 'Task: no coverage report.' is unreachable in aggregateCoverageTitle (:394, guarded by if (!baseReport …) return '' at :389).

• ChangedFilesList.tsx:548 (carried, unchanged) — comparisonEnabled reads two createMemo accessors inside the effect body; untrack appears nowhere in the file, so the whole polling effect (interval + cleanup) tears down and restarts on the null ↔ report flip.

• Test coverage for the component layer. ChangedFilesList.test.ts is byte-identical to the base (same blob OID fef489b8), and while it does import from ./ChangedFilesList, it pulls only the five pure helpers — the component is never rendered, and there is no MergeDialog.test.ts. So this commit's baselineStale work has no coverage at all: a repo-wide grep for baselineStale|comparisonBadge hits only ChangedFilesList.tsx. It also can't have any as written — comparisonBadge (:152) and FileCoverageBadge (:205) are module-private. coverage-comparison.test.ts:195-219 covers stale in the pure builder and MergeReadinessPanel.test.ts:282 covers the neutral check with impactedUnchangedFiles: [], so nothing exercises the stale + non-empty-impacted combination that the Important item above turns on. This is why a fix could land on two of three indicators with a green suite.

• Carried, unchanged — aggregateCoverageTitle truncates the impacted list at .slice(0, 3) (:409) while printing the full count (:417), and that same string is the only tooltip on ↕ N other (:1034); the footer still shows two unlabeled percentages side by side; and branchName at MergeDialog.tsx:452 / TaskChangedFilesSection.tsx:155 — added by this PR, and the thing that newly enables the GetChangedFilesFromBranch fallback at those two surfaces — is still absent from the PR description.

Strengths

• Both round-4 items are genuinely fixed, with the small local changes proposed rather than rewrites — baselineStale is one boolean threaded through, not a refactor.
• if (!picked) return null is the honest version of the fallback, and the added test asserting the absence of the git show call is the right shape.
• I checked the branches I flagged as at-risk and they're fine: comparisonBadge's remaining non-neutral color (:200) is reachable only when delta === null, so it encodes the task report's own absolute coverage with zero base input — staleness cannot make it wrong. Same for △/∅ and the ◔ footer.
• The read-only constraint from #236 holds end-to-end across all five commits, and rounds 3–4's KISS deletions stayed deleted.

Verdict

Needs changes. Two items, both small: the ↕ N other color (one line, same fix already applied twice in this commit) and the unguarded-null path — which after this round presents unknown freshness as a green pass rather than a muted neutral, and sticks for the session once a transient failure is cached.

The third item is a genuine question rather than a defect I want fixed a particular way: decide whether the baseline is the merge-base or the base branch's current head, then make the code and the strings agree. If it's base-HEAD, this is a wording change plus swapping which timestamp you compare against. My apologies for the round-4 mis-scoping on the null path — the direction of the fix was right, the consequences were not what I described.

[LarryHu0217]

Addressed the two concrete follow-ups in b1837bf:

• ↕ N other now uses the muted informational color whenever the base comparison is stale or cannot be anchored.
• A present base report with an unavailable merge-base is now represented explicitly as unanchored, keeping merge readiness, aggregate coverage, and per-file deltas neutral/informational instead of pass or warning. Added regressions for the builder and readiness check.

Validation: npm run check and npm test (1,650 passed, 23 skipped).

I left the merge-base versus base-HEAD baseline semantics unchanged because the review frames that as a policy choice; I can align the wording and timestamp reference once the preferred semantics are confirmed.

[johannesjo]

Summary

Re-reviewed at head b1837bf, checked out locally: npm run typecheck is clean and the affected suites are green (87 tests across coverage-comparison, MergeReadinessPanel, git).

Both round-5 items are genuinely fixed — verified against the code, not the changelog:

Round 5 item	Resolution at b1837bf
↕ N other hardcoded theme.warning while the aggregate greys	ChangedFilesList.tsx:1044-1049 — mutes on stale || unanchored
Unknown freshness rendered as a confident green pass	A third baseline state: coverage-comparison.ts:152-162 now distinguishes "no base report" (undefined) from "present, unanchored", merge-readiness.ts:192-199 returns neutral, and it is threaded to the badge (:158-163), the tooltip (:403-406), and the aggregate footer (:979-981)

That is the right shape and the right cost: the distinction is made once in the builder rather than as a null check scattered across consumers.

The deferred question — decision: the baseline is the base branch's current HEAD

Thanks for flagging it rather than guessing. #236 cites GitHub Code Quality, which compares against the default branch's head, and base-HEAD is what the code already reads — resolveBaseCoverageRoot → getBranchWorktreePath returns the path of whatever worktree is on the base branch, and the report is read straight off that working tree. So the code is right and the vocabulary is the defect. Three changes:

1. Drop "merge-base" from the user-facing strings — ChangedFilesList.tsx:160, :162, :405, :408, :411, and merge-readiness.ts:189, :197. Say the baseline is "<base> as currently checked out".

2. Compare the base report's mtime against the base worktree's HEAD commit time, not the merge-base's. Today's reference is systematically lenient: the merge-base sits behind what the base worktree actually contains, so a base report regenerated after any merge into projectRoot (which is where mergeTask merges, i.e. exactly the worktree that later serves as the baseline) yields stale: false while measuring a tree that includes everything merged since the task branched. Comparing against base HEAD closes that.

3. It is cheaper, not more expensive. getMergeBaseTimestamp currently costs 5–8 git invocations — pinHead, localBranchExists + remoteTrackingRefExists, one or two merge-base, up to two merge-base --is-ancestor, then git show. Under base-HEAD semantics the sha is free: resolveBaseCoverageRoot already runs git worktree list --porcelain on every coverage poll, and its output already contains the HEAD <sha> line — parseWorktreeList simply ignores it today (your own fixture feeds it — git.test.ts:1484). Capture it into ListedWorktree, return it alongside the path, and one git show -s --format=%cI <sha> yields the timestamp.

Worth documenting in the tooltip that the delta may include base-branch work merged after the task branched — that is inherent to base-HEAD semantics and fine, as long as it is stated rather than implied away.

Note the knock-on: because the sha rides along with a call that already happens every poll, the whole cachedMergeBase mechanism at ChangedFilesList.tsx:711-747 can go — which subsumes the item below and the carried rebase-cache item from round 4.

Issues

Important

• ChangedFilesList.tsx:738-747 — The merge-base cache stores null results. getMergeBaseTimestamp returns null on any exec failure or unparseable %cI (git.ts:1608-1611), and .catch(() => null) at :744 swallows IPC failures too. Since the cache is keyed on taskResult.generatedAt, one transient failure is replayed on every 5s poll until the coverage report is regenerated or the panel/dialog is deactivated and reactivated.

  This round changed what that costs. At 4bd754d the sticky null produced a green pass; now it pins the entire comparison — per-file badges, aggregate footer, ↕ N other, and the readiness check — to muted "informational only", with no self-healing on the poll that is otherwise running fine. Safer direction, but still sticky. One line: only assign cachedMergeBase when the value is non-null, so the next tick retries. (Moot if you take the base-HEAD route above, which removes the cache.)

Minor

• stale || unanchored is now duplicated across four consumers — ChangedFilesList.tsx:158, :979-981, :1046-1047, and merge-readiness.ts:185 + :192 as two sequential ifs. They are mutually exclusive by construction (the unanchored branch sets stale: false), so every consumer wants the same disjunction, and missing one of them is exactly what round 5 caught. Export a predicate from coverage-comparison.ts — isBaselineInformational(baseline) — and use it at all four; that also collapses the two merge-readiness branches into one.

• Carried from rounds 4–5, unchanged. All three dead branches still follow from the same invariant: baseResult is declared null at :726 and assigned only inside if (taskResult), and all three writes are batched at :757, so non-null base ⟹ non-null task with no observable transient. Therefore comparisonBadge's 'no report' branch (:184-190) cannot fire, || hasBaseCoverageArtifact() is redundant in showCoverageFooter (:350), and 'Task: no coverage report.' is unreachable at :398 behind the :393 guard. Also unchanged: comparisonEnabled (:556) reads two createMemo accessors in the effect body and untrack appears nowhere in the file (0 occurrences), so the whole GetChangedFiles polling effect tears down and restarts on the null ↔ report flip; aggregateCoverageTitle truncates at .slice(0, 3) (:417) while printing the full count (:425), and that string is the only tooltip on ↕ N other (:1043); and branchName at MergeDialog.tsx:452 / TaskChangedFilesSection.tsx:155 — which newly enables the GetChangedFilesFromBranch fallback (ChangedFilesList.tsx:647) at both surfaces, a behavior change beyond coverage — is still absent from the PR description.

• Component-layer test gap, unchanged. ChangedFilesList.test.ts is still byte-identical to main (same blob hash), there is no MergeDialog.test.ts, and comparisonBadge/FileCoverageBadge remain module-private. This round's two new tests cover the pure builder and the readiness check; all four UI call-sites the commit touched have no coverage. That is the same structure that let round 5's fix land on two of three indicators with a green suite.

Strengths

• The third baseline state is a builder-level distinction rather than a null check smeared across consumers, so the round-5 harm model — unknown provenance rendering as a confident pass — is closed at the source.
• Both fixes are the small local changes proposed, not rewrites.
• The read-only constraint from #236 holds end-to-end across all six commits, and the earlier KISS deletions stayed deleted.

Verdict

Needs changes, and the remaining work is small: realign the baseline to base-HEAD semantics per the decision above (wording plus one swapped timestamp reference, net fewer git invocations), which also lets the merge-base cache and its sticky-null behavior go.

[LarryHu0217]

Updated in fc81faa.

• Coverage now uses the base branch as currently checked out: git worktree list --porcelain provides its HEAD, and one git show obtains the timestamp used to classify the base report.
• Removed the merge-base timestamp IPC and its sticky cache.
• Centralized the informational-baseline predicate across the badge, aggregate UI, and readiness check.
• Updated the tooltip to state that base-branch changes merged after the task branched may be included.

Validation: npm run check and npm test (1,648 passed, 23 skipped).

[johannesjo]

Summary

Re-reviewed at head fc81faa. CI is green, and all six round-6 items are implemented — verified against the code rather than the changelog:

Round 6 item	Resolution at fc81faa
Drop "merge-base" from user-facing strings	All 7 sites reworded to "as currently checked out" (ChangedFilesList.tsx:162, :164, :417, :421, :428; merge-readiness.ts:192, :194)
Compare base mtime against base-worktree HEAD, not merge-base	stale: baseGeneratedTime < baseHeadTime (coverage-comparison.ts:164)
Get the sha free from the existing worktree list call	head captured in ListedWorktree (git.ts:691, :722-725); one git show -s --format=%cI replaces pinHead + 2 ref checks + up to 2 merge-base + 2 --is-ancestor + git show
Remove cachedMergeBase and its sticky null	Gone, along with the whole GetMergeBaseTimestamp channel, handler, and preload entry
Document that the delta may include post-branch base work	ChangedFilesList.tsx:423-425
Centralize stale || unanchored	isBaselineInformational used at all four consumers (:159, :982, :1046, merge-readiness.ts:186)

One correction to my own round-6 cost claim: steady state is now 2 uncached git invocations per 5s poll (worktree list + git show) versus 1 plus a cached merge-base. Cheaper on a cache miss, marginally worse in steady state. Not a defect — just accurate accounting.

Issues

Important

• merge-readiness.ts:197-201 — Base-branch drift is now attributed to the task, and it flips overall readiness to attention. The file.task.state !== 'available' arm of regressedUnchanged fires when a file is available in the base report and absent from the task report. Under the base-HEAD semantics we just adopted, that combination has exactly one common cause: someone else added a covered source file to the base branch after this task branched. It is present in the base worktree (so normalizeCoveragePath's existsSync gate at coverage.ts:112-114 keeps it) and cannot appear in the task's report at all.

  It survives every filter. coverage-comparison.ts:138 doesn't continue ('file-not-present' !== 'available'), :139 doesn't either (delta === null), so it lands in impactedUnchangedFiles; then regressedUnchanged.length > 0 at :208-210 is an independent OR beside the aggregate threshold, so MATERIAL_COVERAGE_DELTA never applies to it. Result: Base 82% → task 82% (0pp). 3 unchanged files also regressed. → warning → overall: 'attention', for a task that changed none of those files. In an app whose whole shape is many task worktrees off one base branch, that's the mainline case, not an edge case.

  The task-attributable case this arm was added for in round 1 is already handled elsewhere: if the task deleted a covered file, that path is in the Git inventory, so changedPaths.has(filePath) at coverage-comparison.ts:134 skips it and it renders as the del 82% badge instead. By construction, nothing reaching impactedUnchangedFiles with base available / task not-present can be the task's doing. So restrict the readiness gate to deltas that are actually attributable — file.task.state === 'available' && file.delta !== null && file.delta <= -MATERIAL_COVERAGE_DELTA — and keep the wider set for the ↕ N other display. The freshness check can't rescue this: it detects "base report older than base HEAD", never "task report older than base HEAD", which is normal and permanent for any task that isn't freshly branched.

• coverage-comparison.ts:166-169 — baseBranch is spread into the anchored baseline (:162) but not the unanchored one, so the branch name is dropped in one of the two states that reads it. Every consumer falls back to its placeholder, and two of the three placeholders are missing an article, producing broken user-facing text: merge-readiness.ts:194 → "Base coverage report cannot be anchored to base branch as currently checked out", rendered as visible text at MergeReadinessPanel.tsx:93; comparisonBadge :164 → "cannot be anchored to base branch". (ChangedFilesList.tsx:417 uses 'the base branch', so the three defaults also disagree with each other.)

  The test that should have caught it asserts the opposite. MergeReadinessPanel.test.ts:318-337 hand-builds baseline: { baseBranch: 'main', stale: false, unanchored: true } and expects "...anchored to main as currently checked out" — a shape buildCoverageComparison cannot emit — while the builder's own unanchored test (coverage-comparison.test.ts:227-234) passes no baseBranch and asserts its absence. Fix is the same spread on both branches, plus one builder assertion that baseBranch survives an unknown head.

Minor

• git.ts:1598-1600 — git show runs with cwd: match.path. The sha is reachable from projectRoot's object database (same repository), so using the worktree path only adds a failure mode: a prunable entry whose directory is gone still appears in worktree list --porcelain, and exec then fails on the missing cwd. cwd: projectRoot is strictly safer.

• git.ts:1596 — if (!match?.path || !match.head) return null collapses "a worktree is on the base branch but its HEAD line is unreadable" into "no base worktree at all", which drops the base report and the entire comparison. Returning { path, head, headCommittedAt: null } would degrade to unanchored — the state this PR added specifically for unknown provenance. Rare, but it's the wrong direction and it's untested.

• Test coverage for what this commit changed. git.test.ts:1494-1512 covers only the success path: git show always returns a valid timestamp. Nothing covers headCommittedAt: null (the sole producer of unanchored), the !match.head guard, or an unparseable %cI. That is exactly the path broken by the baseBranch item above. Also dead now: buildWorktreeMockHandler's mergeBaseTimestamp option (git.test.ts:446, handler at :597) has no callers after the getMergeBaseTimestamp block was deleted.

• ChangedFilesList.tsx:292 — baseWorktree.path === taskRoot is a raw string compare on a path straight out of git worktree list, while listImportableWorktrees does the same comparison through safeRealpath on both sides (git.ts:1540, :1548). On a mismatch the guard fails and the task's own report is compared against itself: base 82% → task 82% (+0pp), empty impacted set, permanent pass. Reachable when the user works directly on the base branch through a symlinked project path.

• Freshness residual (inherent — worth a sentence in the tooltip, not a fix). stale compares a file mtime against HEAD's committer date. A git pull --ff-only that fast-forwards the base worktree onto commits authored days ago leaves a report generated before the pull reading as fresh (mtime > commit time), even though it measures the pre-pull tree. Genuinely closing it needs the report's own sha, which is out of scope for a read-only feature — but the tooltip currently warns only about the base being too new.

• MergeReadinessPanel.tsx:26-28 — The Coverage help text still says "task and base-branch coverage reports" without the "as currently checked out" qualification the rest of the strings gained this round. It's the one place that explains the check.

• Carried, unchanged. All three dead branches still follow from the same invariant (baseResult is null at :737, assigned only inside if (taskResult) at :740, all writes batched at :758, so non-null base ⟹ non-null task with no observable transient): comparisonBadge's 'no report' case (:186-192), || hasBaseCoverageArtifact() in showCoverageFooter (:356), and 'Task: no coverage report.' (:410) behind the :405 guard. Also unchanged: comparisonEnabled (:573) reads two memo accessors in the effect body with no untrack anywhere in the file, so the polling effect tears down and restarts on the null ↔ report flip; aggregateCoverageTitle truncates at .slice(0, 3) (:434) while printing the full count (:442), and that string is the only tooltip on ↕ N other (:1044); the footer still shows two unlabeled percentages side by side (:975-991 prose vs :992-1010 glyph); branchName at MergeDialog.tsx:452 / TaskChangedFilesSection.tsx:155 — the one added prop unrelated to coverage, and the thing that newly enables the GetChangedFilesFromBranch fallback at both surfaces — is still absent from the PR description; and ChangedFilesList.test.ts is still byte-identical to main (blob fef489b8), so none of the four UI sites this round touched has component-level coverage.

• Tiny: aggregate.delta === null at merge-readiness.ts:179 is unreachable — the preceding guards leave task.state === 'available', so delta === null implies base isn't available, which the same condition's first clause already covers.

Strengths

• Round 6's decision landed completely and in the shape proposed: the sha rides along with a call that already happens every poll, and the merge-base IPC is gone end-to-end — channel enum, manifest, handler, preload allowlist, and tests.
• isBaselineInformational is exported once rather than re-derived at four call sites, which is what let this round's rewording touch every consumer without missing one.
• The tooltip now states the base-HEAD limitation instead of implying anchoring that never happens — the wording defect from round 5 is closed at the source.
• parseWorktreeList handles HEAD before branch and treats a blank line as a record terminator, so the new field doesn't perturb listImportableWorktrees.
• The read-only constraint from #236 holds end-to-end across all seven commits, and rounds 3–6's KISS deletions stayed deleted.

Verdict

Needs changes — one design item, one one-liner.

The baseBranch spread is a one-line fix plus a test correction. The readiness item is the substantive one: locking in base-HEAD semantics was the right call, but it means impactedUnchangedFiles now routinely contains other people's merged work, and regressedUnchanged converts that into attention with a detail line that reads as the task's fault. The disambiguator is already present — anything in that set provably wasn't touched by the task — so the fix is narrowing one filter, not reworking the model.

[LarryHu0217]

Addressed both round-7 blockers in 7a8443d:

• Merge readiness now counts only material coverage deltas where both the base and task entries are available, so files added on the base branch after the task branched remain visible in the comparison without being attributed to the task.
• Unanchored baselines now preserve baseBranch, with the builder regression updated to verify the real emitted shape.

Validation: targeted coverage/readiness suites (32 passed), npm run check, npm run check:static, and the full suite (1,649 passed, 23 skipped).

[johannesjo]

Summary

Re-reviewed current head (7a8443d) against #236 and all seven prior review rounds. The round-7 fixes are present, and quality plus GitGuardian are green, but four correctness gaps remain. There are no inline review threads.

PR Discussion Context

The selected base-HEAD semantics remain reasonable for displaying an informational comparison. They do not make two divergent branch snapshots attributable to the task, and the task-side report still needs the same freshness treatment already added for the base.

Issues

Important

• src/components/merge-readiness.ts:197-214 — When the base branch is ahead, its post-fork changes still drive the Coverage warning. A fresh base-only commit can raise aggregate coverage, making the unchanged task snapshot look like a regression; base-only tests can also improve an existing source file, leaving both entries available so regressedUnchanged counts it. Merge safety already reports that the base is ahead, so this does not independently change the overall state, but the Coverage line still falsely attributes base drift to the task and leaves the round-7 fix incomplete. While main_ahead_count > 0, keep Coverage neutral/informational until rebase and regenerated task coverage, while retaining the footer comparison for reference.

• src/components/ChangedFilesList.tsx:733-766 — Only the base report is checked against a HEAD timestamp. Run coverage, then commit or rebase the task without regenerating it: the unchanged task report remains authoritative and can produce either a false pass or a false warning. Obtain task-HEAD freshness alongside the base, and keep the comparison informational whenever either report predates or cannot be anchored to its corresponding HEAD. Add a commit/rebase-after-report regression test.

• src/components/ChangedFilesList.tsx:224-258, lines 358-360 — A delayed or failed canonical inventory removes the existing task-only per-file badge. With a valid task summary but no comparison, badge() is null and the only fallback requires !props.summary, so neither branch renders. The same null is sent to merge readiness and displayed as “No task coverage report,” even though a report exists. This is transient during delayed loads and persists while GetChangedFiles fails, especially in uncommitted mode. Restore the absolute-summary badge whenever comparison data is unavailable, and represent inventory loading/failure separately from a genuinely missing report. Add a mounted-component regression; the current tests cover only pure helpers.

• electron/ipc/git.ts:539-590 — The rename parser still relies on Git’s ambiguous human-formatted numstat shorthand. In a real repository, renaming dir/old => literal.ts to dir/new.ts emits dir/{old => literal.ts => new.ts}; the parser assigns the stats to a phantom dir/literal.ts => new.ts row and leaves the real rename at zero. Literal braces reproduce the same failure. The base already mishandled rename shorthand generally, but this PR adds previous_path and explicitly promises defined renamed-file coverage behavior, so the case is materially exposed in this feature. Use NUL-delimited --raw --numstat -z parsing rather than heuristic brace/arrow parsing, and add real Git-shaped regressions for literal-arrow and brace renames.

Strengths

• Base-worktree discovery and report loading remain read-only.
• The round-7 unavailable-state and unanchored-branch fixes are correct, and IPC wiring remains consistent.

Verdict

Needs changes. The first three items can make coverage readiness or fallback UI report a state unsupported by the available data; the rename parser still fails a defined acceptance case for legal paths.

[LarryHu0217]

Addressed the four current-head findings in 347108e:

• coverage stays informational while the base branch is ahead, and task-report freshness is now anchored to task HEAD
• task-only per-file coverage remains visible while canonical inventory loads or fails, with inventory state kept separate from report availability
• all combined raw/numstat diffs now use NUL-delimited -z parsing, preserving exact literal-arrow and brace rename paths
• regressions cover base-ahead readiness, stale/unanchored task reports, mounted badge fallback, inventory states, and real Git-shaped rename records

Validation on the rebased branch: npm run check, npm run check:static, npm test (1,660 passed, 23 skipped), security-rule tests, frontend/remote builds, and MCP bundling. Hosted quality and GitGuardian checks are green.

[johannesjo]

Summary

Re-reviewed current head 347108e against #236, the complete PR diff, all prior review discussion, and the latest regression tests. The four findings reported at 7a8443d are implemented: base-ahead readiness is neutral, task-report freshness is anchored to task HEAD, task-only badges survive unavailable inventory, and combined raw/numstat parsing is NUL-delimited.

One fallback-path correctness gap remains.

PR Discussion Context

There are no inline review threads. Hosted quality and GitGuardian checks are green. On an isolated export of this exact head, npm test passed (1,660 passed, 25 skipped), as did frontend typechecking, Electron compilation, ESLint, Knip, and dependency-cruiser.

Issues

Important

• src/components/ChangedFilesList.tsx:400 — When the canonical changed-file inventory is loading or has failed, this passes [] to buildCoverageComparison. The builder cannot distinguish “inventory unavailable” from “there are no changed files,” so every materially different report path is classified as an impacted unchanged file. The footer then renders those entries as ↕ N other and describes them as “materially impacted unchanged files” at lines 1140–1151. In an uncommitted-only view this is visible during a delayed inventory request and persists after GetChangedFiles fails, falsely relabeling actual changed paths as unchanged. The new readiness guard correctly keeps the merge verdict neutral, but it does not prevent this incorrect Changed Files output.

  I reproduced the state directly with differing task/base reports and the component's empty-inventory substitution: a known changed path is emitted in impactedUnchangedFiles with a -20pp delta. Keep the valid aggregate comparison and task-only badges, but suppress per-file/impactedUnchangedFiles comparison output until inventoryState === 'available'. A mounted-component regression should cover both loading and failed inventory with a report-differing changed path and assert that ↕ N other is absent.

Strengths

• Task/base freshness and base-ahead handling now degrade safely to informational merge-readiness output.
• The NUL parser is applied consistently to all combined raw/numstat consumers and preserves exact rename paths.

Verdict

Needs changes. The remaining fix is small, but the current fallback can present materially incorrect file classification.

[LarryHu0217]

Addressed the remaining fallback-path issue in 087bd8f:

• Per-file coverage comparisons and impactedUnchangedFiles are now suppressed until the canonical inventory is available, while aggregate comparison and task-only badges remain visible.
• Added mounted happy-dom regressions for both loading and failed inventory states; each verifies the aggregate and task badge remain while ↕ N other is absent.

Validation: npm test (1,660 unit tests plus 2 mounted client tests), npm run check, npm run check:static, and npm run build:frontend pass.