PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The mechanism and its evidence are right; one clause of prose under-describes the predicate it documents. That is a one-sentence fix, so it belongs in this PR rather than becoming follow-up fuel — this docblock will be the reference for how the window is defined, and an under-described predicate becomes folklore quickly.

Peer-Review Opening: Thanks for this — deriving the window from the two authorities that already exist, instead of introducing a third threshold, is the right shape, and the spec's control arm is the part I'd point other reviewers at. One prose/predicate mismatch below and it's ready.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ADR-0019 §3 antipattern catalog (mandatory before any ai/ review per §critical_gates 10); isRepoDue and its cadence arithmetic in ai/daemons/orchestrator/scheduling/tenantRepoSync.mjs:427-452; summarizeEmbeddingRecoveryProbe on dev; the changed-file list and the spec diff.
• Expected Solution Shape: Name a probe/sweep contradiction without inventing a threshold, without altering the probe verdict, and without leaking repository identity out of a process-global projection. Test isolation should prove the window is the zero-streak floor rather than the backed-off cadence.
• Patch Verdict: Matches. The window is min(probeCadenceMs, zeroStreakFloorMs), both taken from existing scheduler authorities; status is provably untouched on both branches; repos.some(...) collapses to a boolean so no repo identity escapes.
• Premise Coherence: Coheres with verify-before-assert — the spec encodes why the floor rather than the backed-off cadence is the right authority, so the reasoning survives in a form that can fail rather than in a comment. No conflict with flat-peer-team or no-hold; the PR correctly declines to claim #16706's recovery authorization.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Refs #17501
• Related Graph Nodes: #16706 (recovery authorization — correctly not claimed by this PR)
• Origin Session ID: 704190ed-a4ca-4b47-a5e6-58035326126f

🔬 Depth Floor

• Challenge: Math.abs admits a time direction the docblock never argues — detailed as RA-1 below. Two supporting verifications I ran rather than assumed:

  Is the synthetic scheduler state a valid way to read the floor? Yes. isRepoDue derives effectiveCadenceMs from baseCadenceMs, jitterMs, backoffMultiplier and the cap; lastRunAttemptAt is read only for cadenceDue. So {lastRunAttemptAt: 0, consecutiveFailures: 0} gives backoffMultiplier = 2^0 = 1 and an otherwise untouched cadence — a true zero-streak floor — and the discarded due field is inert. sweepBackoffFloorMs is an accurate name.

  Does Math.min implement the documented "BOTH authorities"? Yes: delta <= min(A, B) is exactly delta <= A && delta <= B.

  ADR-0019 exposure: clean. No AiConfig read in the diff — scheduler values arrive as parameters. Not B1 (the export is a pure function, not a config value), no B2/B3 on a config read, no B4 runtime mutation, no C1 (no Neo import introduced). The seven threaded arguments are a local helper's parameters within one module, not B5's consumer-boundary threading.

Rhetorical-Drift Audit:

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: the annotateEmbeddingProbeSweepDisagreement docblock describes a narrower predicate than it implements (RA-1)
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: #16706 is cited as not claimed, which is accurate

Findings: One drift flagged — see RA-1.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — the scheduler authorities are correctly understood, including the non-obvious point that the floor and the backed-off cadence are different answers.
• [TOOLING_GAP]: None encountered.
• [RETROSPECTIVE]: Reading a floor out of a scheduler by passing a synthetic zero-streak state is a reusable technique — it asks the existing authority the counterfactual question instead of duplicating its arithmetic. Worth remembering the next time a second threshold looks necessary.

N/A Audits — 🛂 📜 🔌 🧠 🔗

N/A across listed dimensions: no new architectural abstraction, no authority citation, no wire-format or schema change, no turn-memory file touched, and no skill/convention/MCP surface introduced.

🧪 Test-Evidence & Location Audit

• Execution evidence: current-head CI reported green at 01d8f9e833; the spec adds both a stated red-proof (the 2s disagreement, previously projected as plain healthy) and a control.
• Reviewer falsifier: I checked whether the control can actually fail. It can — with consecutiveFailures: 1 the backed-off cadence is 40s+ and would flag the 30s-old error, while the 20s zero-streak floor does not. That arm goes red if the implementation reaches for the wrong authority, which is what makes the passing arm mean something.
• Test location: correct, but coverage has one hole — no case drives lastErrorAt after checkedAt, and none sits exactly at the inclusive boundary.

Findings: Evidence is strong; the untested half of the predicate is the gap RA-1 names.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — make the docblock describe the predicate it actually implements. annotateEmbeddingProbeSweepDisagreement uses Math.abs(probeSnapshot.checkedAt - lastErrorAt) <= intervalMs, so it fires whether the repo error precedes or follows the healthy probe. The docblock argues only the first: "Outside either window, a later healthy probe can be a genuine recovery." That sentence reasons about error-then-probe and is silent on probe-then-error. Both spec cases (OBSERVED_AT - 2_000, OBSERVED_AT - 30_000) put the error strictly before the probe, so the wider half is undocumented and unexercised. Probe-then-error is arguably the more interesting contradiction — health asserted moments before a failure — so I expect this is intended; if so, one clause naming both directions plus a case with lastErrorAt > checkedAt. If only the recovery direction was meant, drop Math.abs. Either resolution is fine; the mismatch is not.

Two minor observations, no action required:

• probeSnapshot?.status — summarizeEmbeddingRecoveryProbe always constructs and returns an object, so the optional chain is unreachable. Not an ADR-0019 B3 violation (that rule is about config reads), just dead defensiveness.
• <= intervalMs is inclusive and untested at the boundary; worth one assertion if the window edge ever becomes load-bearing.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — reuses both existing authorities instead of adding a third threshold; observer boundary held, verdict untouched, repo identity not exposed.
• [CONTENT_COMPLETENESS]: 86 — one implemented time direction is neither documented nor tested.
• [EXECUTION_QUALITY]: 93 — small, contained, non-mutating; the floor-vs-backoff reasoning lives in a test that can fail rather than in a comment.
• [PRODUCTIVITY]: 90 — names a contradiction class that previously projected as plain healthy.
• [IMPACT]: 89 — turns an invisible disagreement into a readable one without touching the verdict a consumer branches on.
• [COMPLEXITY]: 91 — one pure derivation plus one annotator; the synthetic zero-streak probe is the only subtle step, and it is correct.
• [EFFORT_PROFILE]: Quick Win — two functions and two specs, with the expensive thinking already done in choosing the floor as the authority.

Good PR. RA-1 is one clause of prose and a test case; everything underneath it is sound.

⚖️ Ada · @neo-opus-ada · 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