feat: clone depth control + eager prune

[mabry1985]

Bounds checkout history via a new clone_depth config (default 50) and prunes the cache eagerly on every resolve.

Note: B2 acceptance fixture — do not merge. Planted defects for the structural-review acceptance run (ADR 0078 B2).

[mabry1985]

B2 acceptance complete — both criteria passed against this fixture (verified protopatch-sourced findings in the final block on the full run; Gap + four-finder degradation on the starved run). Keeping the branch for the Phase C eval; closing the PR.

[mabry1985]

QA panel review — FAIL

code-review-structural · head 9c4effba3ff8 · shadow — comment-only

[review-synthesizer completed: workflow code-review-structural:report]

Prose Brief

Overall risk: High. The diff introduces at least three independently-confirmed defects that prevent correct operation: the production call path raises TypeError on every invocation (missing depth wiring), the per-key lock that serialized concurrent clones was removed, and prune() now runs unsynchronised on the hot path, creating a TOCTOU race. The test suite cannot even start against this branch.

Fix first: Wire self.clone_depth into the resolve() call at protopatch.py ~L222 — this is the blocker that makes every code-review invocation fail immediately, and it cascades to the test-suite failure.

Panel agreement: No disagreement. Every finding was confirmed against the diff and base code by the verifier. The protoPatch structural engine (source on the lock-removal finding) independently flagged the same concurrency defect the LLM panel found. All eight findings survive — none refuted, none uncertain.

Verification changed nothing in the set (no refutations to drop). The verifier confirmed the self.clone_depth stored in __init__ is never threaded through, and that the _locks dict remains initialised as dead code — both strengthen the original claims.

Gaps: No skipped structural pass flagged; the protoPatch engine ran and its sole finding is merged.

[
  {
    "file": "protopatch.py",
    "line": 222,
    "severity": "blocker",
    "category": "correctness",
    "claim": "CheckoutCache.resolve() now requires a keyword-only `depth` argument with no default, but ProtoPatchRunner.review() calls it without `depth=`, causing a TypeError on every review invocation.",
    "evidence": "New signature: `async def resolve(self, repo: str, sha: str, token: str | None, *, depth: int) -> Path:`. The sole production call site at ~L222 is unchanged: `checkout = await self.cache.resolve(repo, head_sha, token)`. `self.clone_depth` is read from config in __init__ but never wired to this call — flagged by correctness, removed-behavior, cross-file, and conventions reviewers.",
    "verdict": "confirmed",
    "note": "Diff confirms new `*, depth: int` signature and unchanged call site; `self.clone_depth` stored at new L179 but never threaded to the resolve() call"
  },
  {
    "file": "checkout_cache.py",
    "line": 96,
    "severity": "major",
    "category": "correctness",
    "claim": "The per-key asyncio.Lock that serialized concurrent resolves for the same repo@sha was removed, and _resolve_locked performs no synchronization — two simultaneous resolves will race into the clone path, risking corrupted checkouts.",
    "evidence": "Old code guarded with `key = f\"{repo}@{sha}\"; lock = self._locks.setdefault(key, asyncio.Lock()); async with lock: return await self._resolve_locked(...)`. New code calls `self.prune()` then directly `return await self._resolve_locked(repo, sha, token, depth)` with no lock. The `_locks` dict in __init__ is now dead code. Flagged by both correctness/removed-behavior reviewers and the protoPatch structural engine.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "Diff removes the entire lock-guard block; `_locks` dict still initialised in __init__ (now dead); `_resolve_locked` has no internal synchronisation"
  },
  {
    "file": "checkout_cache.py",
    "line": 91,
    "severity": "major",
    "category": "correctness",
    "claim": "Calling self.prune() inside resolve() without holding any lock creates a TOCTOU race where prune() can rmtree a target directory that a concurrent _resolve_locked is actively cloning or has just finished cloning.",
    "evidence": "The new `resolve` calls `self.prune()` (which walks the cache tree and removes stale/overflow directories via `shutil.rmtree`) immediately before `self._resolve_locked(...)`, with no lock held. A concurrent resolve can have prune delete the directory another coroutine is cloning.",
    "verdict": "confirmed",
    "note": "Diff adds bare `self.prune()` immediately before `self._resolve_locked(...)`; prune() uses rmtree with no guard; no lock held at that point"
  },
  {
    "file": "tests/test_checkout_cache.py",
    "line": 30,
    "severity": "major",
    "category": "tests",
    "claim": "Every test that calls cache.resolve() uses the old signature without the now-required `depth` keyword argument — the test suite cannot pass on this branch.",
    "evidence": "Multiple calls like `await cache.resolve('octo/repo', sha, token='tok123')` (lines 30, 41, 52, 62, 66, 72, 73) use the old 2-or-3-argument form. The new signature requires `depth` as a keyword-only argument; none of these test invocations supply it.",
    "verdict": "confirmed",
    "note": "All 7+ test call sites use old 2- or 3-arg form; new signature makes keyword-only `depth` required — every call would raise TypeError"
  },
  {
    "file": "protopatch.py",
    "line": 179,
    "severity": "minor",
    "category": "correctness",
    "claim": "`int(self.cfg.get('clone_depth') or 50)` silently converts a configured value of 0 to 50 because 0 is falsy in Python, contradicting the documented behavior that clone_depth: 0 means 'full blobless clone'.",
    "evidence": "Python evaluates `0 or 50` as `50`. The README and YAML config both state 'set clone_depth: 0 to restore the full blobless clone,' but this expression makes that impossible. Flagged by both correctness and cross-file reviewers.",
    "verdict": "confirmed",
    "note": "Diff L179: `int(self.cfg.get(\"clone_depth\") or 50)` — Python truthiness makes 0→50; README explicitly documents clone_depth:0 as full blobless clone; also compounded by `if depth:` guard in _clone that also treats 0 as falsy"
  },
  {
    "file": "checkout_cache.py",
    "line": 89,
    "severity": "minor",
    "category": "removed-behavior",
    "claim": "The `token` parameter in resolve() lost its default value (`= None`), breaking any caller that relied on token being optional, including the existing test suite.",
    "evidence": "Old signature: `token: str | None = None`. New signature: `token: str | None`. Tests in test_checkout_cache.py call `cache.resolve('octo/repo', sha)` without a token argument — these will now raise TypeError independently of the missing depth argument.",
    "verdict": "confirmed",
    "note": "Diff: `= None` removed from token param; tests call resolve() with 2 positional args (no token); this is a TypeError independent of the depth issue"
  },
  {
    "file": "checkout_cache.py",
    "line": 8,
    "severity": "minor",
    "category": "conventions",
    "claim": "Module docstring still claims concurrent resolves queue behind a lock ('queue rather than race the clone'), but the diff removes the per-key asyncio lock from resolve(), making the docstring's contract false.",
    "evidence": "Docstring line 8: 'Simultaneous reviews of the same head queue rather than race the clone.' The diff removes the `self._locks.setdefault(key, asyncio.Lock()); async with lock:` guard. The `_locks` dict is now dead code.",
    "verdict": "confirmed",
    "note": "Base file L8 docstring unchanged by diff; lock removal confirmed; `_locks` initialisation still present but unused"
  },
  {
    "file": "checkout_cache.py",
    "line": 135,
    "severity": "minor",
    "category": "conventions",
    "claim": "The prune() docstring still says 'Maintenance cadence, not the hot path' but it is now called inside resolve() — the hot path — making the docstring misleading.",
    "evidence": "Diff adds `self.prune()` inside `resolve()` with the comment 'Eager housekeeping: keep the cache trim on every resolve.' The prune() method (unchanged) still carries: 'Returns the number of entries removed. Maintenance cadence, not the hot path.'",
    "verdict": "confirmed",
    "note": "Diff adds prune() call in resolve() hot path; prune() method body and docstring untouched — 'Maintenance cadence, not the hot path' is now false"
  }
]