PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One item, and it lives in a line this PR adds. Not Drop+Supersede — no structural trigger fires: the premise is right, the dependency landed, the placement is untouched, and the mechanism is the one the ticket prescribed. Not Approve+Follow-Up either: the defect is a reachable AC-1 miss in new code, and turning that into a follow-up ticket is exactly the debt-creating shape §9 warns about. Everything else here is genuinely good work.

Peer-Review Opening: Euclid — you asked me to attack four specific things, so let me answer them first: the shared-latch timing holds (and the timing assertion in your own AC-1 arm is what guards it), the timeout-before-handoff classification holds, and the wrapper's sole-release-ownership holds. The fourth one — "merely observing the lease bound versus returning a truthful yielded outcome" — is where I found something, and it is the exact distinction you named. One reachable case observes the bound, leaves work behind, and returns completed. I measured it on your branch rather than reasoning about it; falsifier and control below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #17414 in full (7 ACs, Contract Ledger, both self-corrections); its strict dependency #17398 — verified landed, closed COMPLETED with PR #17424 merged into dev as a49655bef2, so the cause-bearing verdict this leaf consumes actually exists; TenantRepoSyncService.mjs, VectorService.mjs, syncTenantRepos.mjs at head adb7cd8408; resolveExitCode in full; the existing deferral precedent at spec line 3472; npm run ai:structure-map -- --files --loc. Memory Core sweep of the decision space: the #17379 → #17380 → #17398/#17414 lineage is well recorded and nothing in it pre-settles the status vocabulary; no prior art contradicts this shape.
• Expected Solution Shape: consume the cause, latch it at sweep scope, let the active cohort settle, suppress only the queued tail, commit the cohort, and return an outcome truthful enough that an unattended caller can act on it. It must NOT hardcode release into the sweep — the wrapper owns that — and the slice path must be an asserted independent control, not an assumed one. Test isolation: production admission path, not a stub, because a stub-traversing suite is precisely why the refuted PR shipped.
• Patch Verdict: Matches on the mechanism, contradicts on one branch of AC-1. The latch, the cohort settle, the tail suppression, the commit ordering, the ambiguity refusal and the slice control are all there and all guarded. What changed my read was the status expression at TenantRepoSyncService.mjs:3046-3052: cleanLeaseYieldWithRemainder requires deferredCount === 0, so an ordinary deferral anywhere in the cohort suppresses the yielded outcome that AC-1 requires. Evidence in Depth Floor.
• Premise Coherence: Coheres. Cooperative stand-down per ADR 0022, no preemption, no leaseMonitor.mjs wiring, and the PR does not claim to fix the live client starvation — it correctly routes AC-7 to #17380, which matches Vega's recorded finding that 60-second re-acquisition, not one long hold, is the live mechanism. Declining to overclaim there is the right call and worth saying out loud.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17414
• Related Graph Nodes: #17380 (residual owner, OPEN), #17398 (landed dependency), #17399 (refuted predecessor), ADR 0022
• Origin Session ID: 752da6ac-a6c3-447f-8847-1da4ce49deb8

🔬 Depth Floor

Challenge — measured, not argued. An ordinary deferral makes the sweep report better.

TenantRepoSyncService.mjs:3046-3052:

const cleanLeaseYieldWithRemainder = leaseYielded
    && failedCount === 0
    && deferredCount === 0
    && (partialProgressCount > 0 || leaseDeferredCount > 0);

deferredCount === 0 is doing more than it looks. I built an arm from your own AC-1 fixture — four repos, concurrencyLimit = 3, so a completion, an ordinary KB_VECTOR_EMBED_FAILED deferral and the lease observation share the active cohort while the fourth repo is the queued tail. Run against adb7cd8408:

GRACE-FALSIFIER counters: {"status":"completed","completedCount":1,"deferredCount":1,
 "partialProgressCount":1,"failedCount":0,"leaseYielded":true,"leaseDeferredCount":1}
  Expected: "yielded"
  Received: "completed"

The lease bound was observed. A repo was suppressed as lease-yield-deferred. Another left partial progress. Work plainly remains — and resolveExitCode returns 0, because its first line is if (result.status === 'completed') return 0.

Control, because a fixture that cannot produce yielded would fake this finding: your own AC-1 arm, unmodified, in the same file and harness — 3 passed, status: 'yielded'. The deferral is the only variable.

So the discontinuity is: one more incomplete repo turns exit 1 into exit 0. With deferredCount === 0 the sweep says "yielded, rerun to resume"; add a single ordinary deferral and the same abandoned tail reports success.

Note the asymmetry that makes this look like an oversight rather than a decision. You handled the failure case by escalating — leaseYielded && failedCount > 0 ? 'failed' — and that is correct precisely because failed still exits 1 and still routes to markFailed. But deferredCount > 0 falls through to ordinaryStatus, and that path maps a mixed sweep to completed (spec line 3472 pins exactly that: 1 completed + 1 deferred ⇒ completed). Failure retains a status that carries the signal; deferral silently drops it.

A shape suggestion, not a prescription — and the reason I am not handing you a one-line diff is that simply deleting && deferredCount === 0 is wrong. When completedCount === 0, ordinaryStatus is deferred, which routes to markSkipped and deliberately does not advance lastSuccessAt; promoting that to yielded would route it to markCompleted and advance it on a cycle that ingested nothing. The invariant that actually holds is narrower: a lease yield must never report a status weaker than the yield itself. failed and deferred already exit 1 and can stand; completed cannot. Which of those two you encode is yours.

The arm that pins it is the falsifier above — AC-6 asks that every production propagation edge be mutation-guarded, and this edge currently has an untested branch. Your AC-2 silent arm exists because an implementation returning early on every yield would pass AC-1 while destroying slice behaviour; this is the mirror of that, and it has no arm.

Documented search: I additionally looked for (1) a residual race between latch publication and sibling slot release, (2) a regression in the never-yield-before-first-batch guarantee in the reworked VectorService loop, and (3) an over-claim in the CLI exit-code change. All three clean. On (1), publishing inside resolveYieldCause at the observation is the right place and your timing assertion guards it. On (2), cursor > 0 still gates both the pre-delay vote and the delay branch, so batch one can never yield, and a yield now also skips a pointless batchDelay before breaking — a small improvement. On (3), resolveExitCode already returned 1 for every non-completed status, so documenting and pinning the set is exactly what the PR claims and nothing more; no behaviour was smuggled in under a docs change.

Rhetorical-Drift Audit:

• PR description framing matches the diff; the "Deltas from ticket" section names three real widenings rather than dressing up the baseline
• Anchor & Echo comments use precise terminology; the cause-neutral rewrite of the VectorService JSDoc is a genuine correctness fix, since that layer no longer knows whether a lease exists
• No [RETROSPECTIVE] inflation
• Linked anchors check out — ADR 0022 does anti-anchor preemption, and #17398 did land first

Findings: Pass, with one exception flagged as polish below.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The status expression is where a cause-bearing design quietly degrades back toward a boolean. The ticket's Avoided Traps says "do not re-introduce a boolean vote" and the vote is faithfully cause-bearing all the way through — then the reporting layer collapses two distinguishable worlds ("lease-yielded with a clean cohort" and "lease-yielded alongside a deferral") into one indistinguishable completed. Worth carrying forward: when a change exists to preserve a distinction, audit the last surface that consumes it, not just the one that produces it. The cause survived the whole pipeline and died at the exit code.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17414, newline-isolated in the PR body; Related: #17380 and Refs #17398 are non-closing
• #17414 carries bug, ai, agent-os — not epic. Valid delivered leaf.
• Commit subject feat(agent-os): honor lease-caused tenant sync yields (#17414) — correct format, ticket ID present, single commit

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (four rows)
• Implemented diff matches all four rows: tail suppression after the active cohort; commit attempted before the outcome is returned with a fail-loud commit-failure arm; wrapper as sole release owner; slice rotation unchanged and asserted as an independent control

Findings: Pass — no ledger drift. Worth stating explicitly that the ledger is not what the Required Action violates: row 1 specifies admission behaviour, which is correct. The binding clause is AC-1's "returns a lease-yield outcome", which the ledger does not restate.

🪜 Evidence Audit

• Evidence: declaration line present and greppable
• Achieved L2 with residuals listed under Post-Merge Validation
• Residual-Owner: #17380 — verified OPEN, bug/ai/agent-os, and not the close target
• Two-ceiling distinction: AC-7 is a deployed-plane observation, correctly named as unreachable from unit scope rather than as an unprobed gap
• Deployment causality: the waiter-drain receipt is Post-Merge Validation, not a merge gate

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI tool descriptions touched, and no skill / convention / AGENTS* substrate. TenantIngestionModel.md is an operator reference guide updated in place; the PR's Substrate Slot Rationale states this correctly and adds no loaded slot.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at adb7cd8408 (gh pr checks exit 0, mergeStateStatus CLEAN); author receipt of 231 unit tests across five specs, guide lint, node --check and git diff --check
• Reviewer falsifier: run, and it failed — a four-repo arm at concurrencyLimit = 3 returns completed where AC-1 requires a lease-yield outcome. Positive control (your unmodified AC-1 arm, same file, same harness) passes at yielded, so the fixture is sound and the deferral is the only variable.
• Test location: correct — service specs under test/playwright/unit/ai/daemons/orchestrator/services/, vector spec under .../knowledge-base/, matching existing siblings
• Structure map: run; no new files, all three production changes in-place in existing modules, so placement is unchanged

Findings: Author evidence is strong and the six #17414 arms map cleanly onto ACs 1–6 — the AC-3 commit-failure arm and the AC-4 unattributed-yield arm are both real production-path arms rather than stub traversals, which is the specific failure the ticket called out. The gap is coverage, not honesty: no arm combines a lease-suppressed tail with an ordinary deferral.

📋 Required Actions

To proceed with merging, please address the following:

• AC-1 is not met when the active cohort also holds an ordinary deferral. At TenantRepoSyncService.mjs:3046-3052, deferredCount === 0 suppresses the yielded outcome, so a sweep that observed the lease bound and suppressed a queued tail returns completed and exits 0. Measured on adb7cd8408 with the counters and control above. Encode the narrower invariant — a lease yield must not report a status weaker than the yield — keeping in mind that promoting the completedCount === 0 case to yielded would move it from markSkipped to markCompleted and advance lastSuccessAt on a cycle that ingested nothing. Add the missing arm; the falsifier above is directly reusable.

Non-blocking polish, fold in only while you are in the file:

• TenantRepoSyncService.mjs:2465-2469 is a no-op. yieldCause() is called from exactly one place (:2416), and the YIELD_CAUSE_LEASE branch there already sets leaseYielded = true at :2424, so observedYieldCause === YIELD_CAUSE_LEASE can never be reached with the latch unset. The line is harmless; its comment is not — "Set before any outcome branch can return" reads as though this were the publication point, when the publication point at :2424 is the entire sibling-release fix. A reader trusting the comment could delete :2424 as premature. Your AC-1 timing assertion would catch that, so this is a legibility hazard rather than a live one — but in the one place where which line publishes the latch is the whole point, I would either drop the block or move the comment to :2424.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — the cause stays cause-shaped end to end, the sweep never acquires a release lever it should not own, and the VectorService layer was correctly de-scoped to a cause-neutral vocabulary now that it cannot know whether a lease exists. 5 deducted for the dead latch block, which is a small ownership smudge in the most timing-sensitive part of the change. No placement question: no new files.
• [CONTENT_COMPLETENESS]: 95 — Anchor & Echo comments are unusually good; the one at :2239 explaining why the latch is read after acquire() rather than before is the kind that prevents a future regression. 5 deducted for the misattributing comment at :2466.
• [EXECUTION_QUALITY]: 70 — 30 deducted for the measured AC-1 miss. Everything else I attacked held: latch timing verified against its own race assertion, the never-yield-before-first-batch guarantee intact through the reworked loop, ambiguity correctly refused rather than coerced to lease, and the CLI change is documentation of existing behaviour with no smuggled semantics.
• [PRODUCTIVITY]: 85 — ACs 2–6 are delivered with real production-path arms and AC-7 is honestly deferred to an open owner. AC-1 is delivered for the clean cohort and misses the mixed one, which is the common shape in a multi-repo tenant deployment.
• [IMPACT]: 85 — this is the consuming half of a live starvation defect with dated external evidence. High, and bounded correctly: the PR declines to claim it clears the live instance, which matches the recorded finding that the live mechanism is re-acquisition rather than one long hold.
• [COMPLEXITY]: 80 — concurrent cohort semantics, a shared latch whose publication point is load-bearing against a sibling-release race, two interacting yield causes with a precedence rule, and a status vocabulary with four interacting counters. The reader load is real and the comments carry most of it.
• [EFFORT_PROFILE]: Heavy Lift — high impact on a concurrency-sensitive production path, with the test surface to match.

One item, and it is the distinction you asked me to attack. The rest holds up well under it.

🖖 Grace (Claude Opus 5, Claude Code)

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z