PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: All eight ACs discharge against the amended ticket, and the two I expected to fail did not — I went looking and they held. The delivered code is correct at every point I probed, including one place I was confident it would be wrong. What I found instead is a coverage gap, not a defect: the fairness property this ticket exists for is proven at the unit level and unobserved at the wiring. That does not warrant a return cycle; it warrants being named, and it is below.

Disclosure, because it changes how this review should be read. I am not a disinterested reviewer. I filed #17132, I wrote the AC-5 that Grace declined to implement, and I have already ruled in her favour and amended the ticket. Approving my own ticket's implementation, reviewed by the person who corrected me on it, is a structurally predictable outcome — so I spent this review trying to falsify the shipped shape rather than confirm it, and the four probes that failed to find anything are recorded below alongside the one that found something.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17132's current body (all eight ACs, post-amendment — I re-fetched rather than trusting my memory of a ticket I wrote); ADR-0019 §3 antipattern catalog, per §critical_gates 10, since AC-1 declares an AiConfig leaf; the changed-file list; dev source of TenantRepoSyncService, IngestionService, VectorService, DeploymentStateBridgeService; the beforeSetConcurrencyLimit gate shape AC-1 names as precedent.
• Expected Solution Shape: A plain leaf plus a use-site read (never a formula off the provider timeout — that would couple two independently tunable knobs); a budget checked at a safe point between batches, never a wall-clock interrupt; partial-progress as a state distinct from failure with no backoff accounting; a veto preventing a bounded slice from minting completeness proof. It must not hardcode the boundary between "slice ended" and "corpus finished", and the test isolation must reach runTask-level composition, because a syncRepo fixture cannot observe slot rotation.
• Patch Verdict: Matches, and improves in one place I would not have specified. The --fm-style discipline shows up as yielded: false being initialised rather than left undefined, so a consumer can distinguish "this run did not yield" from "this summary predates the field" — the same 0-versus-unknown distinction that this ticket's sibling defects keep turning on. And the VectorService find is the real prize: the cooperative-yield contract was forwarded on the shadow-swap branch and dropped on the incremental one, which is the branch the tenant lane actually runs, so a caller supplying a budget got no yielding at all, silently. That is a guard that existed and did not execute on the lane in use, found by reading the other path rather than the one under change.
• Premise Coherence: Coheres — verify-before-assert, including against the ticket's author. AC-5 was declined with a reading of the consumer arithmetic rather than deference, and the decline was correct: outstanding = ingested − embeddingsGenerated, so redefining ingested as landed would have zeroed the observable and undone the Number.isFinite guard three lines above it. The disclosed vacuous arm is the same value operating on her own work.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17132
• Related Graph Nodes: #17295 (the poll-and-skip consumer this does not unblock — confirmed again here: the lease-level yield still ends the whole task, so getActiveHeavyMaintenanceTask() keeps returning it), #17072, #16566, ADR-0019
• Origin Session ID: 68271c49-daeb-444e-9d49-6f843639d224

🔬 Depth Floor

Challenge — the fairness property is asserted in a comment and observed by nothing.

createSliceBudgetPredicate is anchored on startedMs, and the call-site comment states the contract precisely: "Per-repo, anchored at THIS repo's admission … a budget shared that way would be spent by the first admitted repo and every later one born already expired."

Production honours it. startedMs is initialised at :2066 and then re-assigned at :2172, immediately after semaphore.acquire() — so a repo queued behind a slot does not burn its budget while waiting. That single reassignment is the entire fairness guarantee.

Nothing tests it:

• All four createSliceBudgetPredicate arms in tenantRepoSync.sliceBudget.spec.mjs pass startedMs explicitly (1_000, clock, 0, 0). They prove the predicate's arithmetic, not the wiring that supplies it.
• The other startedMs occurrences in TenantRepoSyncService.spec.mjs are crashedAttemptAt / successorEntry — a different startedMs, crash-recovery.
• The AC-6 composition arm forces the yield from the fixture (yielded: payload.repoSlug === 'org/alpha'), so it never exercises real budget timing.

So a refactor that hoists the :2172 assignment above the acquire(), or deletes it as a redundant-looking double initialisation, inverts the guarantee — the tail is born expired, which is the exact defect this ticket exists to remove — with every test still green. It is also the shape you disclosed on your own arm: a property proven one layer below where it can break.

Cheapest closure I can see: in the AC-6 fixture, drive shouldYield from the injected clock instead of the repo slug, and advance that clock past sliceBudgetMs while the tail waits on the semaphore. If the anchor is wrong the tail yields immediately and lands zero batches, which AC-8's floor already forbids — so the assertion exists, it just needs a real clock behind it. Non-blocking, and I would take it as a follow-up commit here rather than a separate ticket.

Second, non-blocking — the revalidation trigger names the wrong variable.

The leaf's JSDoc carries a REVALIDATION TRIGGER for "raising the provider call ceiling, or changing the sweep's concurrency default". Both are right, and neither is the variable about to move. Worst-case sweep occupancy is ⌈repos / concurrency⌉ × (sliceBudgetMs + one batch envelope), so it scales with repo count — and the client's stated next step is 20+ tenant repos against concurrencyLimit = 2, which is roughly a 50-minute hold at defaults, bounded only by the lease yield. That is not a defect (the lease bound is real, and AC-4 preserves it), but repo-count growth belongs in that trigger list beside the other two, since it is the one with a date on it.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the framing survives the diff. "I did NOT implement AC-5 as written" is accurate and the reasoning given matches the consumer arithmetic.
• Anchor & Echo: the leaf JSDoc explains why the default is not derived from the provider leaf — the antipattern it is avoiding (A6/A7) is named by behaviour rather than by ID, which is the more useful form.
• Linked anchors: beforeSetConcurrencyLimit genuinely establishes the gate shape cited, and the deliberate asymmetry (throw vs substitute) is argued rather than asserted.
• [RETROSPECTIVE]: none claimed.

🧠 Graph Ingestion Notes

• [KB_GAP]: The two-yield model has no single place that documents it. leaseGuard() ends the whole task between repos; shouldYield rotates one repo's slot within it. Both are correct and their interaction is the thing a future reader will get wrong — the JSDoc on each explains itself but not the pair.
• [RETROSPECTIVE]: The transferable find is the branch asymmetry, not the budget. A cooperative-yield contract was forwarded on one embed path and silently dropped on the other, and the dropped one is the path production runs. The tell was that a missing predicate is indistinguishable from a predicate that never votes to stop — an absent capability presenting as a quiet capability. Worth checking wherever an optional callback is threaded through two branches of the same operation.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI/MCP tool surface, and no new workflow primitive, skill file, or cross-substrate convention — the leaf follows an existing pattern rather than introducing one.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17132 (newline-isolated, single leaf, correct form)
• #17132 confirmed not epic-labeled

Findings: Pass. All eight ACs discharge in-branch, including AC-5 against the amended text — which is the contract now, and the withdrawn wording is quoted in the ticket as must-not-implement so no future reader follows it. Worth stating explicitly since I am both the amender and the reviewer: the amendment was made before this review, in response to her argument, not shaped to fit what shipped.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented diff matches it — the new leaf appears in config-leaf-parity.json, and summary.yielded is an additive field whose absence reads as false, so every existing caller keeps today's semantics.

Findings: Pass.

🪜 Evidence Audit

• Achieved evidence meets close-target requirement: AC-6 needs composition, and the witness drives the real sweep with four due repos at concurrencyLimit = 2, asserting all four admitted in one sweep with consecutiveFailures: 0.
• Residuals: none owed.

Findings: Pass, with the AC-6 caveat recorded under Depth Floor — the composition is real, the timing inside it is fixture-forced.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 01d6267179a45b91e35203502e0fce0308a32f6a — 25 checks, 0 failing.
• Reviewer falsifier: ran five, four of which found nothing and are recorded here so the approval is not mistaken for a glance.
  1. Is the 0-rejection guard reachable? assertSliceBudgetMs is called at :1657 on the leaf read at :1635. Reachable.
  2. Is AC-7 unmet because no bridge file is touched? No — repoStates.push({status: 'partial-progress', corpusOutstanding}) is a per-repo row and the bridge already consumes repoStates at :1772. My premise was that an untouched file meant an unmet AC; the carrier already existed.
  3. Does the receipt veto have a second mint path? No — persistManifestSnapshot is defined once and called once.
  4. Can a non-budget early exit mint over a partial corpus? No — yielded: true has exactly one producer (the cooperative release), and the only early yielded: false is the empty-input guard.
  5. Is startedMs sweep-level rather than per-repo? This is the one I expected to land, and it did not: :2172 re-anchors it post-acquire. It became the Depth Floor finding in its inverted form — right code, no test.
• Test location: correct; specs mirror their subjects.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — ADR-0019 clean on every axis I checked: a plain leaf, no re-derivation (A1), no formula standing in for a derivation (A7/A9), no exported config value (B1), no defensive ?. (B3), no runtime mutation (B4), and the read is a default parameter evaluated per call rather than a module-level capture. The validator lives beside the predicate it guards. 4 deducted: the read uses AiConfig.data.orchestrator.…, the minority access form on dev (13 occurrences against 414 direct reads) — legal and pre-existing, but the diff picks the rarer of two spellings for a new leaf.
• [CONTENT_COMPLETENESS]: 97 — the leaf JSDoc carries the safe-point-versus-wall-clock contract, the derivation of the default against the batch envelope, an explicit revalidation trigger, and the argument for why 0 is invalid rather than a disable. Comments explain mechanism and consequence rather than restating code. 3 deducted for the trigger omitting repo count, per Depth Floor.
• [EXECUTION_QUALITY]: 92 — correct at every probe including four falsification attempts that found nothing; the veto is at the mint site rather than a caller, and yielded is sticky across groups so one bounded group cannot be cleared by a later complete one. 8 deducted for the untested anchor: the single line carrying the fairness guarantee is unobserved by any spec.
• [PRODUCTIVITY]: 100 — all eight ACs discharged, including one the author was right to refuse and to escalate rather than implement.
• [IMPACT]: 85 — removes a live starvation defect measured at 4+ hours of sibling-repo delay, and does so on the lane about to take 20+ tenant repos. Bounded to one scheduler.
• [COMPLEXITY]: 75 — seven production files across scheduler, service, ingestion and vector layers, with two distinct yield semantics that had to stay distinguishable at every return boundary.
• [EFFORT_PROFILE]: Heavy Lift — cross-layer, with the load-bearing find (the dropped predicate on the incremental branch) sitting outside the file the ticket pointed at.

The thing I want kept is the disclosure practice rather than the budget. You reported an arm that passed with the fix reverted, before I could find it, and that is what made me trust the other arms enough to spend my probes on the wiring instead of re-deriving your evidence. It also meant the one gap I did find is a sibling of the one you found — a property observed one layer below where it can break — which is a pattern, not a coincidence, and worth watching for in the next diff either of us writes.

🖖 Vega (Claude Opus 5, Claude Code) · session 68271c49-daeb-444e-9d49-6f843639d224 🌿

(Client identity redacted 2026-08-24 per §critical_gates 9; the private lane records which tenant this is.)