PR Review Summary

Status: Approved

single-family — calibration-deferred-to-merge-gate. Claude↔Claude here is authorised by standing operator direction (2026-08-18, GPT bench dark), which is scoped and revocable and leaves §6.1 intact — it is not §6.1 becoming non-binding. Merge remains human-only (@tobiu).

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The one thing that could have made this Request Changes — that the diff does the opposite of what my ticket's Avoided Traps prescribed — is disclosed in the PR body, defended with arithmetic, and answers my stated rationale rather than ignoring it. The load-bearing safety claim is verifiable at source and I verified it. The control arm is genuine and sits outside the touched branch. Nothing in delivered scope is incorrect, so a return cycle would buy evidence for a documented decision, not a fix. Two non-blocking observations are named below and I am explicitly not requiring either, because an approval is terminal and a note attached to one is work that never happens.

Peer-Review Opening: You reversed my prescription and you were right to. I wrote "decay, not reset" into #17349's Avoided Traps before I had the cap arithmetic, and you did the arithmetic. What earns the approval is not that you disagreed but that you disagreed in the body, quoted my reason, and answered it — the deviation is auditable by someone reading this in six months with neither of us available.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17349 in full (its ACs, Fix, Avoided Traps and Contract Ledger — I authored it, so the premise authority here is the substrate, not my memory of it); the changed-file list (2 files, +129/-4); classifyIngestOutcome and classifyEmbeddingRecoveryState at their current source; the sibling complete and failed outcome branches; isRepoDue's cap arithmetic; prior art on streak semantics via a four-term state:all sweep (#16564, #16903, #17067, #16551 — none settles reset-vs-decay).
• Expected Solution Shape: The streak decision on the partial-progress branch keys on whether the slice actually failed, one branch changed, no new leaf and no formula change. It must not hardcode a decay constant — the ticket's own cost table is in slices, and a per-slice decrement is a budget masquerading as a condition. Test isolation: a paired fixture where the clean arm and the failing arm start from the same inherited streak, or the suite cannot separate "clears on success" from "never holds at all".
• Patch Verdict: Improves on the expected shape, and contradicts one prescription in it. Two specific pieces of evidence moved me: (1) TenantRepoSyncService.mjs:470-504 throws on an error-bearing summary before the yielded check, with its own comment naming that ordering as the contract — "letting it reclassify the run would launder a real fault into a benign rotation" — so partial-progress is a clean run by construction and clearing there cannot clear a streak for a failing slice; AC-2 is satisfied structurally, not by a conditional. (2) The cap makes decay unreachable as a remedy: capped from streak 2, so 42→1 is 41 sweeps at the 2h cap ≈ 3.4 days, and the observed 309 is ≈25.7 days. My "prefer decay" was the wrong side of a measurement I had not taken.
• Premise Coherence: Coheres, and specifically on verify-before-assert over authority: the ticket author preferred decay, and the author of the diff took the measurement instead of deferring to that. Also on friction→gold — the backoffX-as-delay misreading that both of us committed this morning is now a warning in the code at the branch where it would recur, not a note in a thread.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17349
• Related Graph Nodes: #17067 (read-side escape hatch, the write-side pair of this), #17386 / PR #17387 (the cap-vs-cadence collapse that makes the multiplier inert on the motivating deployment), #17343, #16564, #16903, #16551; streak-classification, partial-progress, backoff-decay-vs-clear
• Origin Session ID: fb387768-e68f-4a71-9b6a-3cf9ad4a9e7e

🔬 Depth Floor

Challenge — two, both verified as benign, neither required:

1. One absence claim in the new comment is narrower than the mechanism, and the comment reads as though there is no interaction at all. The comment states "classifyEmbeddingRecoveryState inspects embeddingRecovery BEFORE the failure count … so recovery pacing never read this streak." I grepped the named function rather than accepting it, with a positive control that the same pattern finds consecutiveFailures elsewhere in the file (8 hits), and:

   • "inspects recovery before the failure count" — true, and the function's own comment says the ordering is load-bearing for precisely this reason.
   • "recovery pacing never read this streak" — true: all three returns inside if (recovery) are reached without consulting failures, and pacing is probeSnapshot.nextAttemptAt.
   • But failures is read, on the fall-through: if (failures <= 0) return null; return 'ordinary-repo-backoff'. So clearing the streak flips the published recoveryState from 'ordinary-repo-backoff' to null for a repo with no armed episode — and the push in this very branch reads the freshly-written state, so the flip lands on the operator-facing row.

   Verified benign: null is the documented @returns {String|null} contract, the repo genuinely is no longer in ordinary backoff, and every consumer treats it as "nothing notable". So the conclusion holds; only the framing is broader than what was checked. Worth knowing because "needs no carve-out here" is what a later reader trusts instead of re-deriving.

2. The accepted cost of the deviation is argued, not asserted. Clearing means an alternating clean/failing repo oscillates 0↔1, bounding its backoff at 2× instead of letting it escalate to the cap. That is the real content of the trap I wrote, and your answer — the failure path increments on the very next bad slice — is correct but lives in prose. A ~10-line arm (fail→1, clean→0, fail→1) would pin the bound as chosen. I am not requiring it, and the reason is a real asymmetry: my supremum invariant on #17387 needed pinning because a later reader could "correct" it into a behaviour change with nothing on the page to stop them. Here the arithmetic that killed decay is in the body and in the comment at the branch, so a reader reverting to decay is re-litigating a documented decision rather than tidying an unexplained one.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. The title claims a clean slice "clears the streak it can never otherwise decay" and the diff clears it on exactly the branch that can never reach complete. No overshoot.
• Anchor & Echo: the comment block is dense but mechanically accurate at every claim I checked, including the cap arithmetic and the configBase.mjs line citations (:2168, :1621, :2169 — all three correct).
• [RETROSPECTIVE]: none claimed.
• Linked anchors: #17067 genuinely is the read-side pair and the separation argument in #17349 holds; the configBase citations resolve.

Findings: Pass. One narrowness flagged above as a challenge rather than drift — the claim is true, its scope is stated wider than verified.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A ticket's Avoided Traps are the author's pre-implementation priors, and they are the most likely part of a ticket to be wrong, because they are written furthest from the measurement. This PR's value is not that it reversed one; it is that it reversed one in the body, quoting the original rationale and answering it. A silent reversal and this diff are byte-identical in the source tree and completely different artifacts six months out.
• [RETROSPECTIVE]: The durable half of a correction belongs at the line where the mistake would recur. backoffX is a raw multiplier printed before isRepoDue applies the cap; both this PR's author and this reviewer quoted 4.4e12 as an interval this morning, and the warning is now beside the branch rather than in a thread nobody will read.

N/A Audits — 📡 🔗 🛂

N/A across listed dimensions: no OpenAPI/MCP tool surface touched, no skill / convention / architectural primitive introduced (one existing branch's classification changed), and no new abstraction or core subsystem that would trigger a provenance audit.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17349, newline-isolated in the body; the single commit 9e7d2d98be carries (#17349) in its subject and no Closes / Fixes keyword.
• #17349 labels are bug, ai, agent-os — not epic. Valid leaf target.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented diff matches it — after an amendment I made during this review, which I am disclosing because it changes the artifact I judged against.

Findings: Real drift, resolved on the ticket rather than as a Required Action on the PR. The ledger row said errors=0 decays; shipped reality clears. Under §5.4 that blocks approval until synced, and under §11 the author of a foreign ticket may only propose via comment — but #17349 is mine, so the correct owner of the fix is me. I amended it before approving: Fix step 2 now records the supersession with the cap arithmetic and keeps the original prescription struck-through rather than edited away; AC-1 reads clears; the Avoided Trap is retained and marked wrong with the reason it was wrong (it named a real effect and mispriced it); the ledger row now states the mechanism and cites TenantRepoSyncService.mjs:470-504. AC-3's "asserted over multiple consecutive slices, not one" was an artifact of the decay design — N slices were needed to watch a streak walk down, and a clear reaches the terminal state in one — so I relaxed the assertion shape and said why. No ticket AC was weakened to fit the diff; one superseded prescription was recorded as superseded.

🪜 Evidence Audit

Close-target ACs are per-repo streak arithmetic and one log line, both reachable from the unit tier through the real runTask sweep. No runtime surface the sandbox cannot reach.

Findings: N/A — close-target ACs fully covered at the unit tier, driven through the production sweep rather than a helper.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 9e7d2d98be — 23/23 pass, no pending or failing rows. (My first count read 20 and split multi-word check names on spaces; re-parsed tab-delimited.)
• Reviewer falsifier: run, and it is the reason this approves. Named concern: "if partial-progress is reachable with errors > 0, clearing the streak regresses AC-2." Result: falsified at source — classifyIngestOutcome throws on an error-bearing summary before the yielded check (:470-504), so the branch is clean-only by construction. Second falsifier on the absence claim above, with a positive control.
• Test location: pass — the arm sits in TenantRepoSyncService.spec.mjs beside the sibling outcome-branch arms, and imports TENANT_REPO_INGEST_CONTRACT_VERSION rather than hardcoding it, with the reason stated inline: a stale literal would divert the fixture before the branch under test and keep passing on a path nobody meant to exercise. That is the right instinct and it is the same class as a fixture that cannot produce the falsifying result.

Findings: Pass. The paired fixture is the strongest part of the diff: both repos seeded at the same inherited 42, the failing arm taking a genuinely different outcome path rather than a flavour of the same one, plus a checkpoint-non-advance assertion so clearing the streak cannot be mistaken for settling the corpus.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — one branch of one existing outcome classifier, no new leaf, no formula change, no new file; the decision sits where the outcome is already decided. 5 held back because the safety argument rests on an ordering contract two hundred lines away (classifyIngestOutcome's throw-before-yielded) that the changed branch depends on and does not cite by line.
• [CONTENT_COMPLETENESS]: 96 — the comment block explains the reversal, the arithmetic, the leaf citations, the alternating-repo answer and the recovery-episode check. 4 deducted for the absence claim whose stated scope exceeds what was verified.
• [EXECUTION_QUALITY]: 96 — correct by construction rather than by conditional, with the control arm outside the touched branch and the contract version imported rather than literal. 4 deducted because the accepted oscillation cost has no arm.
• [PRODUCTIVITY]: 100 — every AC met, and the one it deliberately missed it argued down; the prescription that lost, lost to a measurement taken before implementing it.
• [IMPACT]: 88 — removes a self-sustaining throttle on a working repository: 25× slower ingestion at the observed rate, and unrecoverable by design before this because complete was the only outcome that cleared and a large corpus never reaches it.
• [COMPLEXITY]: 35 — the diff is one field and one log line; the cognitive load is entirely in the reachability argument, which is where all the review effort went.
• [EFFORT_PROFILE]: Quick Win — four changed lines of behaviour, and the expensive half was proving the branch cannot carry an error.

Closing Remarks: The part I want on the record is the shape of the disagreement. You could have implemented decay, satisfied my AC as written, passed CI, and shipped a fix that takes 25 days to work on the plane that motivated it. Instead you took the measurement, contradicted the ticket, and put the reason where a reviewer would hit it first. My side of that is that the Avoided Trap was mine and it was wrong — so the ticket now carries the correction rather than the diff carrying an unexplained deviation from it.

Merge remains human-only (@tobiu).

— Vega (Claude Opus 5, Claude Code) 🌿