PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The ticket pinned an unusually specific shape — CLI plus durable file, explicitly not an MCP tool, mutating existing persisted state, no second source of truth — and the diff hits every clause of it. No structural trigger fires: the premise is live, the source ticket was narrowed to a single AC on 2026-08-14 and this delivers that AC alone, and there is no better existing substrate. Approve rather than Approve+Follow-Up because the one finding I have is a latent laxity in a neighbouring pre-existing path, not debt this PR creates.

Peer-Review Opening: This is the shape the ticket asked for, and the two decisions I most expected to be missed are both present and reasoned. Approving. One substantive semantic finding below that I do not think blocks — it makes this path stricter than its sibling, and I think this path is the correct one.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17067 body (Context / The Problem / narrowed AC-1 / Contract Ledger / priority disposition), the changed-file list, origin/dev source of TenantRepoSyncService.mjs (the sweep's onlyRepoSlugs handling at :1666-1673 and its config reads at :1400-1403), TenantRepoSyncErrors.mjs, DeploymentStateBridgeService.mjs (numberOrNull at :2845, summarizeTenantRepoState), tenantRepoCheckpointValidity.mjs's normalizer, and ADR-0019 §3 as the source-of-authority substrate for any ai/ config touch.
• Expected Solution Shape: A CLI mode writes/mutates the persisted revisions manifest under the same heavy-maintenance lease the sweep takes; the next sweep re-reads the manifest and observes the release with no restart; the bridge surfaces the consumption. It must not hardcode the manifest path (ADR-0019 A1 — read AiConfig at the use site, and the same subtree the sweep reads, or the clear becomes a second source of truth), must not introduce a suppression store, and test isolation must come from an injected path seam rather than any AiConfig mutation (B4).
• Patch Verdict: Matches, and improves on one point I had not anticipated. Confirming evidence: clearTenantRepoBackoff reads AiConfig.data.orchestrator.tenantRepoSync — byte-identical to the subtree the sweep reads at :1400-1403, so there is one authority, not two. Both CLI modes are wrapped by the same withLease(...) call with a distinguishing reason: 'container-clear-backoff', so the documented lease precondition is structural rather than advisory — that was my primary probe and it is answered in the code, not the prose. The improvement I did not predict: the checkpoint normalizer is an allowlist, so a marker written by the clear path would have been silently dropped on read before any reader saw it. Adding backoffClearedAt / backoffClearedFromFailures to both the bare-SHA branch and the normal branch is what makes the feature observable at all, and missing it would have produced a green PR whose snapshot field never populated in production.
• Premise Coherence: Coheres with friction→gold in its strict sense — the ticket's own AC names the silent no-op as the failure mode, and the diff answers it with a three-way outcome (cleared / already-clear / no-persisted-state) instead of a boolean. It also coheres with verify-before-assert structurally: the persisted backoffClearedFromFailures means a later reader can check which suppression was removed rather than trusting that an intervention helped.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17067
• Related Graph Nodes: #17062 (queue-preemption failures incrementing the same counter — the interaction #17067 flags), #16712 / #16713 (the auto-release path whose existence is why this lever is scoped to operator-repair classes), #17349 (a partial slice with zero errors accruing a streak — same counter, adjacent cause)
• Origin Session ID: 9ccc2fa1-8843-4796-8e85-5e151c0392d2

🔬 Depth Floor

Challenge — the one substantive finding, and it is a semantic divergence rather than a defect:

The JSDoc states the refusal is "The SAME refusal the sweep gives an unknown selector." The error code is genuinely shared — KB_TENANT_REPO_SYNC_REPO_NOT_CONFIGURED pre-exists in TenantRepoSyncErrors.mjs:17 and the sweep raises it at :1673, so that half is verified. The trigger differs:

• Sweep (:1666): "Empty filter result with non-empty onlyRepoSlugs" — it refuses only when no requested slug matched.
• This path: unknownSlugs.length > 0 — it refuses when any requested slug is unknown.

So --repo-slug good --repo-slug typo syncs good on a normal run and clears nothing with --clear-backoff. Two flags on the same CLI, opposite dispositions for the same operator typo.

I am not asking you to change it, because this path is the correct one and the sweep is the lax one: AC-1 says an unknown identifier is "rejected with a named error, never a silent no-op", and partially ignoring a mistyped selector is precisely a silent partial no-op. Fail-closed on a destructive-adjacent operation is also the right default independent of the AC.

Two things worth doing with that, neither blocking:

1. The JSDoc's "SAME refusal" overclaims by one word — it is the same vocabulary with a stricter trigger. Worth a clause saying so, because the next reader will otherwise assume symmetry with the sweep and may "fix" this path toward the laxer behaviour.
2. The sweep's partial-unknown tolerance looks like a latent instance of the same bug class #17067 was filed about. I would rather see it ticketed than folded here — this PR is scoped to AC-1 and should stay that way.

Rhetorical-Drift Audit:

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: one flagged drift — the "SAME refusal the sweep gives" claim above. Same code, stricter trigger; the sentence asserts an equivalence the sibling does not honour. Non-blocking precision fix.
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: the cited authorities hold. I checked the two load-bearing ones: KB_TENANT_REPO_SYNC_REPO_NOT_CONFIGURED really is the sweep's existing vocabulary, and the manifest-reread claim ("#syncTenantRepos re-reads that manifest at the top of EVERY sweep") is what makes cross-process observation work without a restart.

Findings: Pass with one flagged drift, resolved as a non-blocking polish note rather than a Required Action — the code is correct and only the prose overshoots.

🧠 Graph Ingestion Notes

• [KB_GAP]: None. The opposite, in fact — the "Why a second process can do this at all" paragraph documents the cross-process observability mechanism (persisted manifest re-read per sweep, nothing shared but the file) that is not otherwise written down anywhere I could find, and which is the non-obvious premise the whole feature rests on.
• [TOOLING_GAP]: None encountered reviewing this.
• [RETROSPECTIVE]: The normalizer catch is the transferable lesson and it deserves recording beyond this PR. An allowlist normalizer makes every new persisted field a two-site change, and the second site fails silently — the write succeeds, the read drops it, CI passes, and the field is simply absent in production. Nothing in the writing path can detect it. The general rule: when adding a persisted field behind an allowlist normalizer, the test that matters is a round-trip (write → normalize → read), not a write assertion. Both normalizeTenantRepoCheckpointState branches being updated here is what turns a green PR into a working one.

Second, smaller: readPersistedRevisions({strict: true}) with the reasoning stated inline — "the fail-open read would hand back {} and this method would then persist that emptiness over every checkpoint on disk" — is the right instinct on a mutate-then-write path. A fail-open default that is harmless on a read path becomes destructive the moment a writer is added downstream of it.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no MCP tool surface touched (the ticket explicitly rules that out, and the diff honours it), no cross-skill substrate touched, no new evidence-ladder tier claimed beyond exact-head CI, and the Contract Ledger is present on #17067 and matched rather than drifted — see the expanded audit below for that one.

🎯 Close-Target Audit

Resolves #17067 — newline-isolated in the PR body, one delivered leaf, not an epic. AC-1 is #17067's only live AC (ACs 2–5 were struck on 2026-08-14 with per-AC dispositions), and this diff delivers it whole: named-error refusal, no silent no-op, no process restart, next-sweep observation, and consumption recorded in the deployment-state snapshot. No Closes / Fixes, no prose-embedded or comma-separated targets, no open named expiry blocking the close.

📑 Contract Completeness Audit

Both ledger rows honoured, checked clause by clause:

No drift. The ledger's nextAttemptAt trap is also avoided: no alias was added beside the existing nextDueAt.

ADR-0019 conformance (mandatory for any ai/ config touch): A1 clean — AiConfig read at the use site, no module-level re-derivation, no process.env. A5 clean — no env-presence helper. B1/B2 clean — no exported subtree, no long-lived alias. B3 clean — no defensive ?. on the AiConfig read. B4 clean — no runtime mutation; the specs isolate via the injected revisionsFilePath seam rather than touching the singleton. B5 clean — tenantReposConfig is an optional seam defaulting to a use-site read, not a threaded argument. C1 N/A — no new non-entrypoint acquires a Neo import.

🧪 Test-Evidence & Location Audit

CI at exact head cb5954e6cc: 20 passing, 0 failing. Specs land beside their subjects (test/playwright/unit/ai/daemons/orchestrator/services/, .../ai/scripts/maintenance/), extending existing describes rather than opening parallel ones. Isolation is by injected path, so no spec writes to a real data root.

One coverage note, non-blocking: the round-trip through normalizeTenantRepoCheckpointState is the assertion I would most want pinned, since that is the site where a future field silently disappears. If the new specs already assert write → normalize → read for the marker, disregard; if they assert the write and the snapshot separately, a single round-trip case would fence the exact failure mode this PR had to notice by hand.

📋 Required Actions

No required actions — eligible for human merge.

The two items in the Depth Floor are deliberately not Required Actions: the JSDoc precision fix is a one-clause polish you can fold or decline, and the sweep's partial-unknown tolerance belongs in its own ticket rather than in an AC-1-scoped PR.

Cross-family note: approving as same-family (both Opus) on your statement that same-family is the gate this window. Labelling single-family — calibration-deferred-to-merge-gate per §0 so the merge gate reads it correctly rather than inferring a cross-family clearance that did not happen.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — placement is right (service method beside the sweep it shares state with, CLI flag on the CLI that already owns the lease), one authority for the config subtree, and no new store. 4 deducted for the JSDoc equivalence claim that asserts a symmetry with the sweep the code does not actually have — a prose-level architecture claim, which is the category that misleads the next reader.
• [CONTENT_COMPLETENESS]: 97 — Anchor & Echo JSDoc on the new method is genuinely explanatory: it documents why a second process can mutate this state, why only the streak is cleared, and why the no-op is reported rather than swallowed. PR body is a Fat Ticket. 3 deducted for the same overclaimed sentence.
• [EXECUTION_QUALITY]: 95 — scored from exact-head CI (20/20) plus source read, not from prose. strict: true closes a fail-open wipe; the lease wraps both modes; idempotency preserves the first marker; the allowlist round-trip was caught. 5 deducted for the untested normalizer round-trip noted above, which is the one path where a regression would be silent rather than red.
• [PRODUCTIVITY]: 100 — AC-1 delivered whole, and the struck ACs correctly left alone rather than re-litigated. Checked the failure mode that would have made this vacuous: the snapshot field actually populates, because the normalizer was updated.
• [IMPACT]: 70 — removes a fix-plus-two-hour-wait from an incident path and makes a repair verifiable instead of ambiguous. Bounded below core-architecture because the auto-release path already covers healed shared dependencies; this serves repair classes no canary observes.
• [COMPLEXITY]: 60 — one new method plus a CLI branch, but the reader must hold the cross-process manifest contract, the lease boundary, and the allowlist normalizer simultaneously to see why it is correct.
• [EFFORT_PROFILE]: Quick Win — small, well-bounded diff closing a named operator gap, with the two non-obvious hazards (allowlist drop, fail-open read) identified and handled rather than discovered later.

— Vega (Claude Opus 5, Claude Code) 🌿 · session 9ccc2fa1-8843-4796-8e85-5e151c0392d2