Greptile Summary

This PR introduces a CompactionCircuitBreaker class to prevent token waste when compaction repeatedly fails, and wires it into summarizeChunks() as an optional backward-compatible parameter. The class itself is well-implemented with correct state transitions, deterministic time-based tests, and sensible defaults (3 failures, 60s cooldown).

However, there are two correctness defects that leave the feature non-functional:

• Circuit breaker is unreachable (P1): summarizeChunks is a private unexported function. Neither public entry point (summarizeWithFallback, summarizeInStages) accepts or forwards a circuitBreaker parameter, so circuitBreaker is always undefined inside summarizeChunks. The entire circuit-breaking path (canAttempt, recordSuccess, recordFailure) is dead code at runtime. The CompactionCircuitBreaker type needs to be added to summarizeWithFallback's (and summarizeInStages's) params and threaded through, and compaction-safeguard.ts needs to own an instance and pass it in.
• AbortError incorrectly trips the circuit (P1): The catch block in summarizeChunks calls recordFailure() for any exception, including user-initiated AbortError cancellations. This would allow normal user cancellations to advance the failure counter and potentially open the circuit, even though shouldRetry already correctly excludes AbortError from retry logic. The failure recording should mirror the same guard.

Confidence Score: 4/5

Not safe to merge as-is: the circuit breaker feature is entirely dead code and AbortError would incorrectly trip it once wired up.

Two P1 defects are present. The first (dead code / missing public API threading) means the PR's stated goal — preventing token waste from retry loops — is not actually achieved. The second (AbortError tripping the circuit) would cause incorrect behavior once the first is fixed. No production code paths are broken by the current state, but the feature does not work at all and the fix requires non-trivial threading changes across multiple functions and their callers.

src/agents/compaction.ts needs the circuitBreaker parameter threaded through summarizeWithFallback and summarizeInStages, and the catch block needs an AbortError guard. src/agents/pi-hooks/compaction-safeguard.ts (unchanged in this PR) will need to instantiate and own the CompactionCircuitBreaker.

This is a comment left during a code review.
Path: src/agents/compaction.ts
Line: 250

Comment:
**Circuit breaker is dead code — never reachable through the public API**

`summarizeChunks` is a private (unexported) function. Its `circuitBreaker` parameter can only be populated by callers within `compaction.ts`, but both callers — `summarizeWithFallback` (line 362, 389) and the spread in `summarizeInStages` (via `summarizeWithFallback`) — never pass a `circuitBreaker`. Neither public function accepts this parameter in their own `params` type.

The actual production call path is:

```
compaction-safeguard.ts
  → summarizeInStages()
  → summarizeWithFallback()
  → summarizeChunks(params)   ← params never contains circuitBreaker
```

As a result, `circuitBreaker` is always `undefined` inside `summarizeChunks`, making every line of circuit-breaker logic in that function (lines 256–259, 295, 297–298) unreachable. The import of `CompactionCircuitBreaker` in `compaction.ts` is also unused at runtime.

To actually activate this feature, `circuitBreaker?` needs to be added to the params type of `summarizeWithFallback` (and `summarizeInStages`), and then forwarded when `summarizeChunks` is called:

```ts
// In summarizeWithFallback's params type:
circuitBreaker?: CompactionCircuitBreaker;

// When calling summarizeChunks:
return await summarizeChunks(params);  // params now carries circuitBreaker
```

`compaction-safeguard.ts` would then need to create and own a `CompactionCircuitBreaker` instance and pass it through.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/agents/compaction.ts
Line: 296-298

Comment:
**AbortError incorrectly trips the circuit breaker**

The `catch` block unconditionally calls `circuitBreaker?.recordFailure()` for every thrown error — including `AbortError`. When the user cancels compaction via `AbortSignal`, `retryAsync` propagates the error without retrying (because `shouldRetry` returns false for `AbortError`), and this catch block would then count the cancellation as a compaction failure and advance toward opening the circuit.

The circuit breaker is intended to guard against model errors, timeouts, and malformed output — not against user-initiated cancellation. A cancellation should leave the failure counter unchanged:

```ts
} catch (err) {
    if (!(err instanceof Error && err.name === "AbortError")) {
        circuitBreaker?.recordFailure();
    }
    throw err;
}
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(compaction): add circuit breaker for..." | Re-trigger Greptile}

Closing — after reviewing how recent fixes have been structured on main, I think this approach (separate file + class) doesn't match the codebase style well. Will revisit with a more targeted approach if compaction retry issues come up again.