[AI-1731] A branch can no longer refuse every borrowed review by exceeding the review-context cap

[realtonyyoung]

Linear: AI-1731. Pre-existing in the merged #443 review-context server, not a regression from #445 — filed separately for exactly that reason.

The defect

ExtractReviewContextEntriesAsync admitted reserved-path blobs into the review-context manifest under a hard 256 KiB aggregate cap and threw borrowed_snapshot_review_context_capacity_exceeded past it. The throw was not scoped to the review context: it propagated out of CreateReviewContextGenerationAsync → BuildIndependentSnapshotOnceAsync → BuildIndependentSnapshotAsync → CreateBorrowedSnapshotAsync, and the whole borrowed snapshot build failed (the retry loop catches only SourceChangedException).

The content is branch-authored and the cap is small. One tracked vendor MCP config padded past 256 KiB made every borrowed review of that repository impossible — the adversary this surface defends against (someone adding a hostile vendor config to a branch) could contain the payload and deny the review that would have found it. Worse mid-flow: TryRefreshBorrowedSnapshotAsync treats refresh failure as fatal by design, so a config crossing the cap between rounds terminated a reviewer already in progress.

The fix

Containment and review-context capacity are separate properties, and only containment needs to hold for a launch to be safe. The config never enters the executable tree regardless of size (unchanged); the manifest is now bounded and honest rather than fail-closed:

• A blob that does not fit the remaining capacity is declared, not shipped: a new omittedForCapacity list carries path, index mode, blob object id, byte count, and sha256. Admission continues — an omitted blob consumes no capacity needed by later configs.
• The hash is computed streaming from cat-file (HashBlobSha256Async), never buffered — an arbitrarily large branch-authored blob cannot cost the daemon its own size in memory the way reusing RunGitCaptureBytes would.
• The MCP server's instructions and tool description tell the reviewer to report omitted configs as unverifiable, never as absent; an empty entries array is affirmative only when omittedForCapacity is empty too. Silently dropping the entry would recreate the false-clean failure ([AI-1680] Quarantine branch-authored MCP config so a reviewer can still read it #437 class) this surface exists to prevent.
• Read-back validation: each path at most once across both lists, membership in the classifier-matched set for both, and shape checks (regular index mode, positive size, valid object id, lowercase sha256) on every declaration. Integrity failures (encoding, unmerged index, non-blob, collisions, oversized serialized manifest) still fail closed.
• Declarations cannot re-create the refusal one level up: their count and path bytes are bounded by the exclusion plan's own caps (MaxCwdDepth, MaxVendorPathAggregateBytes), well inside MaxReviewContextManifestBytes headroom — noted in its doc comment.

Mid-flow refresh needs no orchestrator change: with capacity no longer throwing, SyncBorrowedSnapshotFromSourceAsync publishes a generation that declares the omission, and the fail-closed catch stays for genuine failures.

Tests

TDD (each watched failing on the old behaviour first):

• Aggregate_capacity_accepts_exact_limit_and_declares_one_extra_byte_as_omitted — updated, not deleted: exact 256 KiB still fully admitted with an affirmative empty omission list; one extra byte now launches, keeps .mcp.json out of the executable tree, and declares path/mode/oid/size/sha256 with no base64/text shipped.
• Omitted_oversized_config_does_not_consume_capacity_needed_by_later_configs — the oversized blob sorts first and a later small config is still admitted in full.
• Refresh_after_config_grows_past_capacity_succeeds_and_declares_omission — the between-rounds path that used to kill a live reviewer.
• Manifest_validation_rejects_malformed_or_out_of_set_omissions — out-of-set path, zero size, symlink mode, all-zero oid, uppercase/truncated sha, and a path appearing in both lists.

Verification: BorrowedReviewContextTests 19/19; neighbours (BorrowedSnapshotExclusionScopeTests 31, WorktreeManagerTests 20+1 platform skip, WorkspaceMcpNeutralizationTests 25, McpReviewContextServerTests 6, integration McpReviewContextServerIntegrationTests 1) all green; KCAP_WORKSPACE_MCP_CERT=1 live certification passed 2/2 with real kiro-cli (positive control reproduced the exploit in a raw worktree; production path stayed contained) — nothing about relaxing the cap puts the config back in the executable tree. NativeAOT publish: daemon clean; CLI shows 4 pre-existing IL3050/IL2026 in untouched McpWorkItemsServer.cs (flagged separately). Full suites delegated to CI.

🤖 Generated with Claude Code

[linear-code]

AI-1731

[qodo-code-review]

PR Summary by Qodo

Declare over-capacity review-context configs instead of failing borrowed reviews

🐞 Bug fix ✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Fixes a security-relevant defect: one oversized (>256 KiB) branch-authored MCP config could abort
 borrowed-review snapshot creation entirely, denying review of the very repo containing it.
• Replaces the fail-closed capacity throw with a bounded omittedForCapacity declaration list
 (path, index mode, blob id, size, sha256) so oversized blobs are declared rather than shipped or
 silently dropped.
• Hashes omitted blobs by streaming git cat-file output instead of buffering, preventing an
 arbitrarily large branch blob from costing daemon memory.
• Strengthens manifest read-back validation: each path must appear in exactly one of
 entries/omissions, and omission shape (mode, size, object id, sha256) is checked.
• Updates the MCP review-context server's instructions/tool description so reviewers treat omitted
 configs as unverifiable, not absent, and documents the change in README.
• Adds unit tests covering omission-on-exceeding-cap, non-consumption of capacity by later configs,
 mid-review refresh success, and manifest validation of malformed omissions.

High-Level Assessment

The PR's approach — declare-not-ship for over-capacity blobs, with a separate bounded list, streaming hash, and stricter read-back validation — is the correct fix given the constraint that containment (never entering the executable tree) and reviewability (never silently absent) must both hold without letting size become a launch-refusal vector. Alternatives like raising the cap or buffering hashes were considered and rejected in the PR description itself (raising the cap doesn't eliminate the attack surface, buffering costs daemon memory).

Files changed (4) +247 / -26

Bug fix (1) +103 / -15

WorktreeManager.ReviewContext.csReplace fail-closed capacity throw with bounded omittedForCapacity declarations +103/-15

Replace fail-closed capacity throw with bounded omittedForCapacity declarations

• Adds a BorrowedReviewContextOmission record and OmittedForCapacity field to the manifest; when a blob exceeds the remaining 256 KiB aggregate cap it is now declared (path, index mode, blob id, byte count, streamed sha256) instead of throwing and aborting the whole snapshot. Adds a new streaming HashBlobSha256Async helper that hashes cat-file output without buffering the blob, and extends ValidateReviewContextManifest to enforce exclusive, well-formed membership across both entries and omissions lists.

src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs

Tests (1) +133 / -7

BorrowedReviewContextTests.csAdd and update tests for capacity-omission behavior and manifest validation +133/-7

Add and update tests for capacity-omission behavior and manifest validation

• Reworks the exact-limit capacity test to assert declaration instead of a thrown exception, and adds new tests for capacity not being consumed by later configs, successful mid-review refresh after growth past the cap, and manifest validation rejecting malformed or duplicated omission records.

test/Capacitor.Cli.Tests.Unit/BorrowedReviewContextTests.cs

Documentation (2) +11 / -4

McpReviewContextServer.csDocument omittedForCapacity semantics in MCP instructions and tool description +7/-2

Document omittedForCapacity semantics in MCP instructions and tool description

• Updates the server Instructions string and the tool description returned in tools/list to tell reviewers that omittedForCapacity entries represent configs that exist but were too large to ship, and must be reported as unverifiable rather than absent or clean.

src/Capacitor.Cli/Commands/McpReviewContextServer.cs

README.mdDocument oversized-config declaration behavior for borrowed reviews +4/-2

Document oversized-config declaration behavior for borrowed reviews

• Updates the borrowed-review-snapshot documentation to explain that an oversized config is now declared to the reviewer by path, size and hash instead of failing the launch.

README.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Cancellation reported as timeout ✗ Dismissed 🐞 Bug ☼ Reliability

Description

HashBlobSha256Async catches OperationCanceledException from a linked CTS and always throws a timeout
InvalidOperationException, so caller-requested cancellation is misreported and cannot propagate
correctly. This can break cancellation/shutdown behavior for borrowed snapshot builds/refreshes that
encounter an omitted blob hashing operation.

Code

src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[R290-293]

+        } catch (OperationCanceledException) {
+            try { process.Kill(entireProcessTree: true); } catch { }
+            throw new InvalidOperationException(
+                $"git cat-file blob {objectId} timed out after {GitTimeout.TotalSeconds:F0}s");

Evidence

The new method links the caller token into timeoutCts and then unconditionally converts
OperationCanceledException into a timeout exception, without checking whether ct was canceled.

src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[272-304]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`HashBlobSha256Async` converts *all* `OperationCanceledException`s into a timeout `InvalidOperationException`, even when the caller’s `CancellationToken ct` was explicitly canceled. This breaks cancellation propagation and misreports the cause.

### Issue Context
The method links `ct` into `timeoutCts` and then catches `OperationCanceledException` without distinguishing whether cancellation came from the caller or the timeout.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[272-304]

### Implementation notes
- Use exception filters to rethrow caller cancellation:
 - `catch (OperationCanceledException) when (ct.IsCancellationRequested) { ... rethrow ... }`
 - Only translate timeout-triggered cancellation into the timeout `InvalidOperationException`.
- Ensure any needed process termination/drain still happens before rethrowing cancellation (see separate cleanup finding).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Git process not drained ✓ Resolved 🐞 Bug ☼ Reliability

Description

HashBlobSha256Async starts a git process and a stderr pump task but has no finally-based
termination/drain; on non-timeout exceptions it may leave the child process running and on timeout
it may leave stderrTask unobserved. This risks leaking git processes and producing unobserved task
exceptions in the long-running daemon.

Code

src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[R274-279]

+        var psi = NewGitPsi(source, ["cat-file", "blob", objectId], sourceReadOnly: true);
+        using var process = Process.Start(psi)!;
+        using var timeoutCts = CancellationTokenSource.CreateLinkedTokenSource(ct);
+        timeoutCts.CancelAfter(GitTimeout);
+        var stderrTask = ReadAllDecodedAsync(process.StandardError.BaseStream, timeoutCts.Token);
+        using var hash = IncrementalHash.CreateHash(HashAlgorithmName.SHA256);

Evidence

HashBlobSha256Async starts a process and stderr pump but has no finally to terminate/reap and
observe pump tasks; the repo already has TerminateAndDrainAsync and uses it for similar patterns.

src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[272-304]
src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs[215-290]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`HashBlobSha256Async` creates a `Process` and starts `stderrTask`, but lacks a `finally` that reliably (1) kills/reaps the git process and (2) observes/drains pump tasks on *all* exit paths. Other parts of the codebase already implement a safe pattern (`TerminateAndDrainAsync`).

### Issue Context
`using var process = ...` does not guarantee the OS child process is terminated if still running. The cancellation path kills, but does not wait/reap; other exception paths don’t kill at all, and the stderr pump can fault unobserved.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[272-304]
- src/Capacitor.Cli.Daemon/Services/WorktreeManager.ExclusionPlan.cs[215-290]

### Implementation notes
- Wrap the stdout-read + `WaitForExitAsync` block in `try/finally`.
- In `finally`, call the existing helper:
 - `await TerminateAndDrainAsync(process, stderrTask);`
- Preserve the original exception semantics (timeout vs caller-cancel vs other failure) after cleanup.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Oversized hash can still fail ✗ Dismissed 🐞 Bug ☼ Reliability

Description

For omitted blobs, the code hashes the entire blob via git cat-file under the fixed 60s GitTimeout;
sufficiently large/slow-to-read blobs can still time out and fail snapshot generation. This
reintroduces a launch-refusal vector via time (even though memory is now bounded).

Code

src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[R242-249]

+            if (objectSize > MaxReviewContextBytes - totalBytes) {
+                // Capacity bounds what the manifest SHIPS, never whether the launch happens — failing
+                // here would let one branch-authored oversized config refuse every borrowed review of
+                // the repository. The blob is declared by path, size and hash instead (streamed, so its
+                // size cannot cost memory), and it never enters the executable tree regardless.
+                omitted.Add(new BorrowedReviewContextOmission(
+                    path, fields[0], objectId, objectSize,
+                    await HashBlobSha256Async(source, objectId, objectSize, path, ct)));

Evidence

Oversized blobs are always hashed in the omission path, and HashBlobSha256Async enforces
CancelAfter(GitTimeout). GitTimeout is defined as 60 seconds, so hashing time can become the new
failure mode for very large/slow blobs.

src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[242-250]
src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[268-304]
src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs[1246-1248]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When an oversized reserved-path blob is omitted, the code always computes a full SHA-256 by streaming the entire blob with `git cat-file blob`. This operation uses the global `GitTimeout` (60s), so very large/slow-to-read blobs can still trigger a timeout and fail the borrowed snapshot build/refresh.

### Issue Context
The PR’s goal is to prevent oversized branch-authored configs from blocking borrowed review launches. Bounding memory helps, but the current hashing step can still block by exceeding a fixed wall-clock timeout.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/WorktreeManager.ReviewContext.cs[242-304]
- src/Capacitor.Cli.Daemon/Services/WorktreeManager.cs[1246-1248]

### Possible remediation directions
- Use a separate, larger timeout budget for hashing omitted blobs (possibly size-scaled).
- Or make omission hashing non-fatal: if hashing times out, still emit an omission record but with an explicit "hash unavailable" representation (would require schema + validator updates).
- Ensure whatever approach you choose cannot be used to refuse the launch via timeouts.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}