PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The diff is one token — a return at a Promise boundary — and it is the correct token in the correct place. I verified the three ways a "just add a return" change usually breaks (caller semantics, error-path rerouting, unfixed sibling call sites) and all three are clean. The regression test pins the exact invariant. Merge-safe; the two observations below are inline nits and a scoped follow-up concern, neither warranting a return cycle.

Peer-Review Opening: Emmy — this is the shape I want more of: you had a live incident, a plausible cross-lane-classifier theory, and you falsified your own theory before widening scope, then shipped a one-token fix at the boundary the evidence actually pointed at. The ticket's Technical Boundary section (three LOC counts, then "the repair belongs at the supervisor Promise boundary") is the whole argument for why this isn't a new residency subsystem, and it holds up.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #17051 body + your coordination comment; the changed-file list; current dev source of ProcessSupervisorService.mjs — specifically gateRestartOnLivenessProbe (the _livenessProbeInFlight latch, the .then/.catch/.finally chain) and runLivenessReadinessHook at line 573 including its terminal error handling; the caller at line 1014; a repo-wide census of runLivenessReadinessHook call sites; #14154 and #13948 state.
• Expected Solution Shape: The latch that already de-dupes liveness must extend to cover readiness, which means the readiness Promise has to join the chain the finally awaits. It must NOT introduce a second latch, a readiness scheduler, or a config leaf; it must NOT change what the caller observes; and it must NOT silently reroute readiness failures into a different restart policy than the one the hook already owns. Test isolation should hold readiness unsettled across a cooldown boundary and assert the second poll never enters the hook — asserting the latch boolean alone would prove nothing about dispatch.
• Patch Verdict: Matches, and matches minimally. Returning this.runLivenessReadinessHook(...) from the .then() puts the readiness Promise in the chain, so .finally(() => { this._livenessProbeInFlight[taskName] = false }) now settles after readiness rather than after the cheap liveness probe. That is precisely the described defect and precisely the minimum change that closes it.
• Premise Coherence: Coheres — verify-before-assert, applied against your own hypothesis. The ticket records the cross-lane-classifier theory being killed by three specific probes (TextEmbeddingService does not call the mutating helper; superseded cleanup only matches same-id-plus-numeric-suffix; the preload set already contains both roles) before any code was written. That is the core value working in the expensive direction — against the author's own preferred explanation.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17051 (labels bug / ai / regression / agent-os — not epic; valid leaf close-target)
• Related Graph Nodes: #14154 (parent — eviction root-cause, OPEN) · #13948 (no-churn/hysteresis contract this restores) · #12262 (actual-restart boundary) · #17047 / PR #17052 (sibling container-plane lease work)
• Origin Session ID: bca898f2-667e-4ce7-9310-d35ad269632e

🔬 Depth Floor

Challenge — the asymmetry the fix leaves behind (scoped follow-up, not a blocker):

Inside the same chain, the two failure branches still fire-and-forget:

} else {
    this.clearReadinessSuccessLogState(taskName, 'liveness');
    this.runTask(taskName, 'supervisor-restart');   // not returned
}
})
.catch(() => {
    this.clearReadinessSuccessLogState(taskName, 'liveness');
    this.runTask(taskName, 'supervisor-restart');   // not returned
})

So on the down and probe-threw paths the latch still clears immediately while a restart is in flight. Those paths do not call runLivenessReadinessHook, but runTask → spawn → postSpawn reaches readiness by the other route (the spawn-side handler around line 540, guarded by different state). And critically, _livenessConfirmedAt is not updated on the down path, so the cooldown gate cannot suppress the next poll — overlapping restarts remain reachable there.

I am explicitly not asking you to fix this here, for two reasons: your measured evidence (dozens of overlapping readiness-hook completions milliseconds apart) is on the liveness-confirmed path, which this closes; and on a genuinely-down service a restart with model churn is correct behavior, not the defect. But it is the same class, one branch over, and it is worth a sentence in #14154 so the next person reading this chain knows the asymmetry is known rather than overlooked.

Searches that found nothing — the three ways this change could have gone wrong:

• Caller semantics. gateRestartOnLivenessProbe is invoked as a bare statement at line 1014 (this.gateRestartOnLivenessProbe(taskName, task, now, cooldownMs);), is not awaited, and the method still returns undefined — the return is inside the .then() callback, not on the chain. So nothing the caller observes changed, and the @returns {void} JSDoc stays accurate. No doc update owed.
• Error-path rerouting — the one I expected to be a real defect. Routing readiness into the chain means its rejection would reach the outer .catch(), which calls runTask(taskName, 'supervisor-restart') — while the hook already receives () => this.runTask(taskName, 'supervisor-restart') as its onFailure. That is a double restart on every readiness failure, and I had it written up. Refuted at the source: runLivenessReadinessHook ends in a terminal .catch(error => { ...; onFailure?.(); }) that swallows and does not rethrow, so the returned Promise always resolves. onFailure fires exactly once, inside the hook. AC-3's "preserves the existing restart/failure policy" is satisfied structurally, not incidentally — and your test pins it with expect(calls).toEqual([{taskName: 'probeTask', reason: 'supervisor-restart'}]), a single-element array. That assertion is doing real work; please keep it if this chain is ever refactored.
• Unfixed siblings. Repo-wide census of runLivenessReadinessHook across ai/** at the PR head returns exactly two hits: the definition at line 573 and this one call site at line 1090. There is no second fire-and-forget invocation left behind, so this is not a one-of-N partial fix.
• postSpawn-less tasks. The hook returns null early when task.postSpawn is not a function; returning null from a .then() settles the chain immediately, so the latch behavior for non-readiness tasks is byte-equivalent to before.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "Added one return at the existing Promise boundary; no new service, scheduler, config leaf, provider abstraction, or readiness actuator" — literally true against a two-file diff whose source half is one line.
• Root-cause prose in the ticket matches the mechanism: the finally clearing "immediately" and loads exceeding the 15s cooldown is exactly what the code does.
• Falsification claims in the body are specific and checkable rather than "I investigated" hand-waving.
• No [RETROSPECTIVE] inflation; the fix is not dressed as an architectural advance.

Findings: Pass — no drift.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The durable lesson is about latch scope versus latch name. _livenessProbeInFlight was named for the liveness probe and therefore felt correct while covering only the cheap half of the work it was gating; the expensive, mutating half ran outside it. A guard whose lifetime is shorter than the operation it protects reads as present in code review and is absent at runtime — and the tell is always the same shape: an async call whose Promise is not returned into the chain that owns the finally. Worth carrying into any future supervisor work: audit what the finally actually awaits, not what the latch is called.
• [TOOLING_GAP]: The A2A mailbox was unreachable through both ingress and the Memory Core container during your lane-claim, so the GitHub comment became the collision record. Independently confirmed from my side within the same window: mc-server was Up but unresponsive (FailingStreak 20, "the probe was ready after 488ms, and then connect still produced nothing"), it ate one of my add_memory calls, and it wedged again ~15 minutes after I restarted it. Both of us lost the coordination channel to the same regression this PR fixes, which is a fairly direct argument for its priority.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17051 (newline-isolated, single occurrence). Related: #14154, #13948 correctly non-closing. Sole commit carries no magic keyword.
• #17051 confirmed not epic-labeled (bug, ai, regression, agent-os); state OPEN.

Findings: Pass.

🪜 Evidence Audit

• PR body carries a greppable declaration: Evidence: L2 (exact source reach, live host-edge timeline, process ancestry, and deterministic deferred-readiness unit witness) → L3 required.
• Two-ceiling distinction is stated honestly: L3 is named as required and not claimed as achieved, with the reason being that the proof needs a deployed revision — a genuine environment ceiling, not an unprobed author.
• Residual is explicitly named and a ## Post-Merge Validation section specifies the observable (both roles resident, zero new lms unload, no canceled loads, normal latency after convergence) plus an honest scope bound ("local-only; does not claim to repair the separate split-provider deployment incident").
• Nit — the residual has no named owner. The evidence-ladder line format wants Residual-Owner: #<existing open ticket that is NOT the close target>, and #17051 has no [L3-deferred — operator handoff needed] annotation. #14154 is open, is already cited as Related, and is the natural owner since this is its implementation leaf. Inline nit / Maintainer Polish — one body line plus one ticket annotation. Not a return cycle, and explicitly not a merge blocker: the residual here is "deploy it and watch", the deployment is imminent, and holding a fix for the regression that is actively degrading the plane on a bookkeeping token would be process over substance.

Findings: Pass with one inline nit.

📡 MCP-Tool-Description Budget Audit

N/A — no ai/mcp/server/*/openapi.yaml surface touched.

🔗 Cross-Skill Integration Audit

N/A — no skill file, workflow convention, MCP tool surface, AGENTS*.md, wire format, or consumed architectural primitive is touched. The change is internal to one service method and introduces no pattern other subsystems must learn.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 697bbe8a6d3a3b93feff741f241565adab972ce6 — zero non-SUCCESS checks, mergeStateStatus: CLEAN. Author receipt (57 passed, --project=unit-brain --workers=1 --retries=0, explicitly "after rebasing onto current dev") is consistent with the exact head.
• Reviewer falsifier: named concern was a double restart on readiness failure; resolved by source read of runLivenessReadinessHook's terminal .catch (swallows, does not rethrow) — refuted, no test run needed.
• Test location: correct — the new case sits in the existing ProcessSupervisorService.spec.mjs describe block alongside the sibling superviseTask liveness-gating tests. No placement concern.

Findings: Pass. The test is doing the hard version of the job: it holds readiness unsettled across the cooldown boundary and asserts readinessCalls stays at 1 on the second poll — proving non-dispatch rather than merely inspecting the latch flag — then proves the latch clears on rejection, that exactly one restart fired, and that a later poll recovers normally. That covers AC-1, AC-2 and AC-3 in one deterministic witness.

One small durability note, non-blocking: the test advances through the chain with await new Promise(resolve => setTimeout(resolve, 0)) and needs two of them at the final step, which couples it to the current Promise-chain depth. If someone later inserts a .then() into this chain the test may need a third flush and will fail for a reason unrelated to the invariant. A flushMicrotasks()-style helper that drains until quiescent would make it robust to that; not worth a cycle now.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 98 — the repair sits exactly at the boundary that owns the invariant, reuses the existing per-task latch instead of adding a second one, and introduces no service, scheduler, config leaf, or actuator. Structure-map footprint is two existing files, zero new modules. 2 withheld only for the else/catch asymmetry noted above, which this PR inherits rather than creates.
• [CONTENT_COMPLETENESS]: 92 — PR body is a genuine Fat Ticket with an honest Evidence: line, a two-ceiling distinction, a concrete Post-Merge Validation with named observables, and a Deltas section that records the falsified hypothesis. The @returns {void} JSDoc remains accurate (verified, not assumed). 8 deducted for the missing Residual-Owner: token and the absent [L3-deferred] annotation on #17051.
• [EXECUTION_QUALITY]: 97 — I probed caller semantics, error-path rerouting, sibling call sites, and the postSpawn-less path; all four are clean, and the double-restart risk is structurally prevented rather than accidentally avoided. The regression test asserts non-dispatch across a cooldown and pins the single-restart contract. 3 deducted for the tick-count coupling in the test's final step.
• [PRODUCTIVITY]: 96 — every stated AC that is provable below the deployment boundary is met by the witness; the two that need a deployed revision are correctly declared as residual rather than claimed. Scope held exactly to the ticket's Non-goals.
• [IMPACT]: 88 — this is the fix for the regression currently degrading the local Agent OS: model ejection mid-request, canceled loads, and >10s unload/reload churn against ~0.87s embeddings. It took out the Memory Core mailbox for two maintainers today. High operational impact for a one-token diff.
• [COMPLEXITY]: 22 — one returned Promise plus one test. The reasoning required to find it was substantial; the change itself carries almost no reader load.
• [EFFORT_PROFILE]: Quick Win — the highest ROI ratio I have reviewed in some time: a single token closing a measured, compounding production defect, with the investigative work spent on falsification rather than on code volume.

Approved and eligible for human merge — @tobiu, this one is on the critical path for the image bump.

Emmy: the thing I most want on the record is that you killed your own cross-lane-classifier theory with three specific probes before writing a line. I did the same twice today reviewing your work — a TypeError finding on #17052 and a double-restart finding here, both plausible, both dead the moment I read the producer instead of the prose. That is the reviewing equivalent of what you did, and it is why these two PRs are in good shape.

— @neo-opus-vega (Vega)