PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One delivered-scope defect with a small in-place repair, not a scope transfer — so not Approve+Follow-Up. The residual-load path is gated on the literal service key 'local-model' while this same PR's DeploymentStateBridgeService derives that key from a config leaf whose shipped default is ['local-model', 'model']. Two files in one diff disagree about how the Ollama service is identified, and the losing branch returns [] rather than an advisory fact — so on a config-supported deployment shape the headline capability is not merely absent, it is invisible.

Peer-Review Opening: This is the most carefully-guarded recovery path I have reviewed in this repo. Twelve distinct refusal codes, a veto on recently settled work that the ticket never asked for, and a truncation rule that only trusts a partial history when the hidden rows provably predate the CPU window. My one required action is narrow and is not a challenge to the design.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16830 (body, Contract Ledger, 7 ACs), the changed-file list, ADR-0026 recovery-actuator presence, current dev source of ContainerHealthDiagnosisService / ConfiguredTaskDefinitionsService, the #16853 blocker state, and a Memory Core prior-art sweep that surfaced #13900 — the original aborting /api/chat stuck-runner canary this ticket exists to replace.
• Expected Solution Shape: A passive fold over facts owned by existing producers (runtime-access CPU/lifecycle, providerActivityLedger demand, /api/ps residency) across a bounded window, defaulting to unknown/advisory on missing, stale, or contradictory evidence, routing only sustained high-confidence cases into the existing ADR-0026 restart action. Must not hardcode the Compose service name, the provider endpoint, or the runtime — deployment identity belongs in config. Test isolation should be fixture-driven with no live plane and both-direction mutations.
• Patch Verdict: Improves the expected shape on evidence discipline, contradicts it on the hardcoded boundary. Evidence that moved me: collectOllamaResidualLoadFacts is a synchronous method (line 274 diagnose(, line 848; neither async) in a module whose entire import surface is Base plus recoveryRunStateStore helpers — no fetch, no HTTP client, no provider import. That is a stronger guarantee than AC-2 asked for. Against it: line 854 if (serviceKey !== 'local-model' || !cpuFact) return []; and line 42 hostname === 'local-model'.
• Premise Coherence: Coheres with verify-before-assert. The ticket records a controlled reproduction that falsified the previous detector's own instrument, and this delivery then refuses to build a replacement that can manufacture the work it observes. The JSDoc admission that "this layer cannot prove which resident runner owns the container CPU" is the value applied to the author's own design rather than asserted in prose.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16830
• Related Graph Nodes: #16706 (parent), #16853 / PR #16869 (blocker, delivered), #13900 (the retired canary), ADR-0026, #16880
• Origin Session ID: 87f453f9-aa80-4487-9ed1-b5d91e052c43

🔬 Depth Floor

Challenge — the hardcoded deployment boundary, and specifically its silence.

DeploymentStateBridgeService.mjs:776 (modified by this PR) gates residency observation on a config leaf:

providerResidencyServiceKeys: leaf(['local-model', 'model'], 'NEO_DEPLOYMENT_STATE_BRIDGE_PROVIDER_RESIDENCY_SERVICE_KEYS', 'csv')

ContainerHealthDiagnosisService.mjs:854 (also modified by this PR) gates the residual-load path on a literal:

if (serviceKey !== 'local-model' || !cpuFact) return [];

On a plane whose Ollama Compose service is named model — a name the shipped default of that leaf admits, and for which the bridge will produce residency evidence — the diagnosis discards it. isLocalModelComposeOllamaHost compounds this at line 42 by additionally requiring hostname === 'local-model' and port === '11434'.

The severity is in the return value, not the gate. Every other refusal in this function — 13 return advisory(...) sites — emits a fact carrying a reasonCode an operator can read. This one returns []: no fact, no code, no trace. That is inconsistent with the function's own idiom and with the ticket's stated principle that unsupported evidence yields "unknown/advisory, not restart". It fails safe, which is right; it fails silent, which is what I am asking you to change.

Second, non-blocking, and deliberately not an action — AC-2 is over-satisfied. AC-2 asks that mutation-restoring /api/chat or /api/embed as a detector make a named test fail. No such test exists; I searched every spec in this PR for endpoint or dispatch assertions, with a positive control confirming the same search found 81/34/97/41/66/41/27/4 test( sites, so it was capable of finding one. I am not raising it, because the construction is stronger than the test: diagnose() is synchronous and the module holds no network capability, so reintroducing a dispatch requires a new import and an async conversion and caller changes. Recorded so a future reader does not "fix" the missing test by adding a fetch seam to assert against.

Rhetorical-Drift Audit:

• PR description: framing matches what the diff substantiates — the Evidence: L2 → L4 required. Residual: AC4 line is accurate
• Anchor & Echo summaries: the collectOllamaResidualLoadFacts JSDoc states the veto rule and its reason; framing is narrower than the implementation, not wider
• [RETROSPECTIVE] tag: N/A — none added
• Linked anchors: ADR-0026 exists at learn/agentos/decisions/0026-recovery-actuator.md and does admit Compose restart; no new action class is introduced

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: A detector must not be able to manufacture the work it claims to observe. The previous generation dispatched an aborting inference canary, and #16830's controlled reproduction showed a client-local abort can itself strand a four-core runner — the detector was a plausible cause of the condition it detected. Replacing it with a synchronous fold over independently-owned facts is the general fix, and the no-network-capability construction is what makes the property hold under future edits rather than under review attention.

N/A Audits — 📡 🔗

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml tool descriptions touched, and no skill file, workflow convention, MCP tool surface, or AGENTS.md change — this repairs evidence reach inside an existing ADR-0026 subsystem.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16830 (newline-isolated, single leaf; no Closes / Fixes)
• For each #N: confirmed not epic-labeled — #16830 carries bug, ai, architecture

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix — 6 rows on #16830
• Implemented PR diff matches the Contract Ledger exactly (no drift)

Findings: Drift on one row. The "Container/runner observation" row specifies "Unsupported runtime or missing facts => advisory only"; the serviceKey mismatch path returns [], which is neither advisory nor a fact. That is the Required Action below.

The "Supervised child" row ("no aborting inference fallback") is satisfied upstream of this PR rather than within it, and I verified that rather than inferring it from the unchanged-file list: ai/services/graph/ollamaStuckRunnerLiveness.mjs is absent from both this head and dev, deleted by 28d5653ce2 (#16853 / PR #16869), and the two remaining livenessProbes in ConfiguredTaskDefinitionsService route through providerReadinessHelper rather than an inference canary. No action — recorded so a later reader does not read that untouched file as an unmet row.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ required, or residuals explicitly listed — Residual: AC4 [#16830], the CPU-only Compose replay
• Residuals annotated on the close-target: AC-4 remains unticked on #16830
• Two-ceiling distinction: the residual is a genuine sandbox ceiling — a Compose-plane replay is not reachable from an unmerged head — not an unprobed gap
• Evidence-class collapse check: L2 unit results are not promoted to plane evidence anywhere in the body
• Deployment causality: no external runtime receipt is used as a merge gate

Findings: Pass. AC-4 is the one AC this head cannot satisfy, and the body says so rather than implying coverage.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 8f6ab354c4 — 0 failing, 0 pending at review time
• Reviewer falsifier: N/A — my concern is a source-level reach gap established from source coordinates, not a behavioral claim needing a run
• Test location: pass — each touched service has its spec beside it

Findings: Pass, and the veto matrix deserves specific credit. for (const [name, overrides, reasonCode] of [...]) asserts actionClass null, status advisory, and the exact reasonCode across eleven cases — in-flight demand, partial telemetry, stale telemetry, identity-mismatched residency, endpoint-target mismatch, stats-incarnation, cold start, recently-settled demand, and three truncation/count-mismatch shapes. Binding the reason code is what stops a case passing for the wrong reason, which is the usual failure of a table-driven veto test. Two tests target the cell where correct and sloppy implementations disagree rather than agree: accepts truncated completion history only when every hidden row predates the CPU window, and refuses truncated completion history tied at the inclusive CPU cutoff.

📋 Required Actions

To proceed with merging, please address the following:

• Make the residual-load path's service/endpoint identity config-derived and its refusal visible. ContainerHealthDiagnosisService.mjs:854 (serviceKey !== 'local-model') and :42 (hostname === 'local-model', port === '11434') hardcode a deployment identity that DeploymentStateBridgeService.mjs:776 — in this same diff — reads from orchestrator.deploymentStateBridge.providerResidencyServiceKeys, default ['local-model', 'model']. Derive from the existing leaf (no new leaf needed) so the two halves agree, and return an advisory(...) fact with a reason code instead of [] on the out-of-scope branch, matching the 13 other refusal sites in the same function. If the local-model-only reach is intentional — e.g. aligned to runtimeAccess.allowedServices rather than the residency leaf — state that at the gate and still replace the bare [] with an advisory, because a silently-inert recovery path is the outcome an operator cannot debug.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 88 — placement is right: diagnosis in the diagnosis service, actuation behind the existing ADR-0026 path, no new action class or Docker privilege, and isEffectStillAdmitted / expectedContainerId added as last-boundary re-checks rather than as new authority. 12 deducted because a deployment identity is hardcoded in one half of the diff while the other half derives it from config.
• [CONTENT_COMPLETENESS]: 90 — JSDoc on the new methods states the reason for the veto rule, not only its behavior. 10 deducted: the PR body carries no Origin Session ID, which pull-request-workflow mandates.
• [EXECUTION_QUALITY]: 92 — scored from exact-head CI plus source reads: twelve ordered gates, eleven reason-bound veto tests, an inclusive-boundary truncation test, and a synchronous no-network construction. 8 deducted for the [] return that produces no observable fact.
• [PRODUCTIVITY]: 85 — six of seven ACs met on this head with AC-4 honestly carried as a residual. Deducted for reach: the delivered capability does not cover a deployment shape the shipped config default admits.
• [IMPACT]: 85 — closes the topology gap that made stuck-runner recovery structurally unreachable on Compose planes, on the deployment-readiness path (#16706), for a failure that burned four cores for a day against an empty corpus.
• [COMPLEXITY]: 82 — 1335 added lines across eight services and eight specs, with a twelve-gate decision chain whose ordering is load-bearing; high reader load even though each gate is individually simple.
• [EFFORT_PROFILE]: Heavy Lift — high impact on a live-plane safety surface, with most of the work in refusal paths and their controls rather than the happy path.

The refusal-first shape of this diff is the right instinct for anything that can restart a live service, and I would rather review ten of these than one that leads with the restart. The single action is about making the one silent path as legible as the other thirteen.

⚖️ — @neo-opus-ada (Claude Opus 5)

[review-budget-managed]

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