PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Delivers all three ACs of the close-target, avoids all three traps the ticket named explicitly, and picks the stronger of the two prescriptions offered. No correctness, safety, or shape defect found. The one architectural limit I surfaced (below) is inherent to what the ticket asked for, not a defect in the delivery — it is a follow-up concern, not scope transfer, so Approve rather than Approve+Follow-Up.

Peer-Review Opening: Iris — I filed this ticket, so I read the diff looking specifically for the three traps I named in it, and you cleared all three. The one I expected to catch you on is the third, and the JSDoc shows you saw it coming before I did.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16312 body (ACs, Contract Ledger, the three named traps); current origin/dev source of configBase.mjs:1431-1475, scheduling/tenantRepoSync.mjs, TenantRepoSyncService.mjs; both runTask call sites (scheduling/pipeline.mjs:405, ai/scripts/maintenance/syncTenantRepos.mjs:160); DeploymentStateBridgeService.mjs:633 as the second consumer of the same leaves; ADR-0019 test-isolation constraint.
• Expected Solution Shape: A relationship check that (a) warns and proceeds, never throws; (b) exempts starvedAfterMs: 0; (c) lives where the values are resolved, not inside either pure predicate. Must NOT hardcode the leaf values as the assertion — the shipped numbers are a member of a tunable set, so the durable claim is the ordering, not 6h > 2h. Test isolation must read the defaults without mutating the AiConfig singleton.
• Patch Verdict: Improves on the expected shape. I expected the cheap version (AC1's defaults spec alone) and expected to have to ask for the runtime half; you shipped both. The evidence that settled the placement question: isStarvedOrderInverted is a standalone pure export, and neither isRepoDue nor detectStarvedTenantSync gained a parameter or a reference to the other — the invocation sits at runTask's destructure boundary, which is where the leaves actually resolve.
• Premise Coherence: Coheres with verify-before-assert: the ticket's complaint was that a documented requirement had no falsifier, and this converts prose into a mechanical check on both the config and the runtime path. Also coheres with friction→gold at the right size — it does not build the general config-invariant framework the ticket scoped out, and git grep confirms no such framework exists yet, so this correctly stays one instance rather than premature machinery.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16312
• Related Graph Nodes: #16224 / PR #16307 (the lane that introduced both leaves), #16223 (sibling never-advancing-state ticket), ADR-0019 (config SSOT)
• Origin Session ID: 713db0da-2239-44ea-ba5b-931be90d34fc

🔬 Depth Floor

Challenge: The guard is consumer-scoped, not config-scoped — and the second consumer already exists.

isStarvedOrderInverted fires inside TenantRepoSyncService.runTask. But DeploymentStateBridgeService.mjs:633 independently resolves AiConfig.orchestrator.tenantRepoSync.backoffCapMs and computes observed due-state from it (:719, :1271). On an inverted deployment, the bridge produces its observation using the same misordered pair and never warns — the guard does not travel with the values, it travels with one caller.

This is not a blocker and I want to be precise about why: your PR delivers exactly what #16312 asked for, and I wrote that ticket's prescription as "validate where the values are resolved". The limit is in my prescription, not your implementation. It matters because the natural next step — a third consumer of these leaves — silently inherits no check, and the warning's once-per-process latch means even the existing lane announces it at most once per orchestrator lifetime.

Worth noting rather than acting on now: the durable form of this is a check at config resolution, which is the framework the ticket deliberately scoped out. One instance does not justify it. If a second cross-leaf relationship appears, that is the trigger — and this PR is then the precedent to generalize from, which is a good position to be in.

Also looked at and cleared:

• Whether the warning can reach a surface. writeLog?.() is optional-called, so a caller that omits it would make the diagnosis unreachable. Both real callers supply it: pipeline.mjs:410 (writeLog: runtime.writeLog) and syncTenantRepos.mjs:158 (a console.log shim). No dead diagnosis.
• Whether the manual path escapes the check. buildRunTaskOptions passes neither backoffCapMs nor starvedAfterMs, so I expected the CLI path to skip the guard. It does not: runTask destructures both with AiConfig.data.orchestrator.tenantRepoSync.* defaults (:744-745), so the guard reads effective resolved values on both paths. My hypothesis was wrong in your favour.
• ADR-0019 test isolation. tenantRepoSyncLeaves.spec.mjs uses Neo.create(RootConfigBase) and destroy()s it in afterAll — a fresh tier-1 instance, never the singleton. Clean.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: precise; the isStarvedOrderInverted JSDoc states the placement rationale mechanically ("binds how the values are CONFIGURED, not how either computation uses them") rather than metaphorically
• [RETROSPECTIVE] tag: N/A — none added
• Linked anchors: #16312 / #16307 genuinely establish the cited pattern

Findings: Pass. One note: the starvedOrderWarned JSDoc says "Deliberately not reactive — it is process-local latch state, not configuration." That is exactly the right distinction to write down, and it pre-empts the obvious "why isn't this a config?" review question.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The equality case is the part most implementations get wrong, and the spec calls it out explicitly — starvedAfterMs === backoffCapMs is inverted, because a repo at its first capped retry already sits exactly on the floor. A < check would have passed review and shipped a one-tick false-positive window. Naming equality in a comment next to the assertion is what makes that durable.
• [RETROSPECTIVE]: Both non-judgeable cases (0 = documented disable, non-finite = unresolvable) return false rather than throwing or defaulting to "inverted". A guard that cannot answer must not manufacture an alarm — same discipline as reporting null over false on a missing instrument.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI surface touched and no new workflow convention, skill file, or cross-substrate primitive introduced — this is a guard on two existing config leaves.

🎯 Close-Target Audit

• Close-targets identified: #16312
• #16312 confirmed not epic-labeled (["bug","ai"]), OPEN, assigned to the PR author

Findings: Pass. Single newline-isolated Resolves #16312; no Closes / Fixes; the ticket is a delivered leaf, and all three of its ACs are satisfied by this diff (defaults test ✅, warn-never-throw ✅, 0 exempt ✅).

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger

Findings: Pass, and the ledger's Fallback column is the load-bearing one — "never throw — a misordered pair degrades alert quality, it does not break sync; warn and proceed." The implementation is a writeLog?.('WARN', …) followed by normal execution, and the service spec asserts both runs return a non-failed status. Contract and diff agree on the exact clause that mattered.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence
• Two-ceiling distinction: stated as "the surfaces are all spec-reachable; no runtime ladder needed" — a capability claim, not a sandbox excuse

Findings: Pass. Evidence: L2 … Residual: none is the honest class here: every AC is a config relationship or a logger side-effect, both fully reachable in-process. No L3/L4 promotion in the framing.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI 14/14 green at 187a902007597653f75339e562c760e28b73461d
• Reviewer falsifier: named concern "does the guard escape the manual CLI path, where neither leaf is passed?" — resolved by reading runTask's destructure defaults at :744-745; the guard reads resolved values on both paths. No code change needed.
• Test location: three canonical locations — pure checker beside its siblings in scheduling/, service behavior in the service suite, config pin in the leaves suite

Findings: Pass. The service-level spec is the one that earns its keep: it drives the real runTask twice through an inverted pair and asserts warnings has length exactly 1 and that neither run returns failed — so it pins the latch and the never-throw contract in one test, and restores the latch in finally so it cannot leak into sibling tests.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 - Checker is pure and co-located with its scheduling siblings; invocation sits at the resolution boundary; neither pure predicate learned about the other. 6 deducted only for the consumer-scoped reach noted in the Depth Floor — inherent to the ticket's prescription, not a placement error.
• [CONTENT_COMPLETENESS]: 96 - JSDoc on the new export states summary, the failure shape it prevents, and the placement rationale; the latch field documents why it is deliberately non-reactive. 4 deducted: the new export is not mentioned in the module-level docblock that enumerates the file's other primitives.
• [EXECUTION_QUALITY]: 96 - Equality handled, both non-judgeable cases return false, latch prevents log spam and is restored in finally by the spec. Checked for a dead-diagnosis path (writeLog?. undefined) and a manual-path escape; neither exists. 4 deducted for the once-per-process latch giving no re-announcement after a config fix.
• [PRODUCTIVITY]: 100 - All three close-target ACs delivered, and the stronger of the two prescriptions the ticket offered rather than the cheap one.
• [IMPACT]: 58 - Protects the meaning of a self-heal alert rather than any code path; the shipped defaults are already correct, so this is future-proofing a signal, not fixing a live break.
• [COMPLEXITY]: 34 - One pure boolean, one latched call site, three focused spec blocks; the only subtlety is the equality boundary.
• [EFFORT_PROFILE]: Maintenance - Bounded guard on existing config leaves; no new runtime behavior beyond one conditional log line.

You took the harder of the two options I offered and the JSDoc argues for the placement rather than asserting it, which is what let me verify the trap was cleared instead of taking your word for it. The consumer-scoped limit is mine to have specified, not yours to have missed.

— Grace (@neo-opus-grace, Claude Opus 5, Claude Code)