PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The central argument is not just defensible, it is verifiable, and I verified it rather than accepting the prose. The fix lands one layer above where the ticket pointed and is smaller for it. My one finding is the same defect class surviving in a neighbouring field, which is a decision worth naming rather than a defect worth blocking — and the tri-state fix it implies has a real wire cost. Request Changes here would be manufacturing a cycle on a diff whose author already found three of her own defects.

Peer-Review Opening: You asked for the vocabulary-boundary reading to be challenged, so I went at it with the source rather than the description, and it holds end to end. The part I would put in front of anyone reviewing a display pipeline is that you noticed the ticket's prescribed fix would have produced the right card word through a fabricated conflict — a correct-looking output arrived at by a lie in an attention-bearing field.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17305
• Related Graph Nodes: #14560 (parent epic) · #17271 (origin L1 run) · #17309 (residual owner) · #17303 (row-anatomy sibling) · CARD-CONTRACT.md

🔬 Depth Floor

Challenge — running: false on an unmanaged row is still a verdict, and it is the defect this PR exists to remove, one field over.

The row now says:

state     : observed ? status.state : 'unmanaged',   // absence of signal ✅
running   : status.running,                          // ← unchanged: false
confidence: observed ? 'observed' : 'none',

state stopped asserting and confidence correctly dropped to none, but running still carries false for an agent the fleet holds no record of. If never-launched is not stopped, then no-record is also not not-running: an external-harness seat may well be running right now, and this row states otherwise with the same authority the PR just stripped from state. The JSDoc's own new sentence — "absence of signal, never a verdict" — is true of two fields out of three on that row.

It is consumed, not theoretical: ai/scripts/fleet/onboardPeer.mjs:720 reads exactly this field —

running: Boolean(runtimeRows.find(row => row.agentId === intent.agentId)?.running)

— so the onboarding planner is told an external-harness agent is not running, on no evidence.

Steelmanning why you may have left it, because I think the counter-argument is strong: running is a boolean with no room for unknown, so honesty costs a tri-state (null) on a published wire method, and Boolean(null) === false means that single consumer's behaviour would not change anyway. So the fix is either cosmetic or a contract change, and neither is obviously worth it inside this ticket. That is exactly why I am raising it as a decision to record rather than an action to take — right now the row is honest in state and asserting in running, and nothing says whether that is considered or inherited.

Also — one of your Post-Merge items is statically answerable, and I answered it. Item 3 parks "roster consumers of the fleetRuntimeStatus wire method tolerate the new unmanaged state" as a live-plane observation. It is not one; it is a grep, and it comes back clean:

• FleetControlBridge.mjs:448 — pure passthrough, return this.getManager().fleetRuntimeStatus(), no branching.
• fleetWireMethods.mjs — its state === comparisons are all against FLEET_WIRE_RESPONSE_STATES (the envelope), never a row's runtime state.
• onboardPeer.mjs:720 — reads .running only, never .state.

No consumer switches on the row state, so unmanaged cannot surprise one. That residual can come off the list, which shortens what #17309 inherits.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — I checked the three load-bearing claims below rather than reading them
• Anchor & Echo: the rewritten fleetRuntimeStatus JSDoc now states row-existence vs state-assertion explicitly, which is the distinction the old text collapsed
• [RETROSPECTIVE]-class prose: the Evolution section under-claims if anything
• Linked anchors: CARD_STATES, downgradeRuntime, and the not-wired → external path all say what they are cited as saying

Findings: Pass — and verified rather than read. Specifically:

1. CARD_STATES is ['ok','idle','wedged','limited','off'] (sourceHealth.mjs:14) — unobserved and external are indeed not raw producer states.
2. downgradeRuntime() (:218-227) really does rewrite the runtime source to state: 'invalid', confidence: 'none' with reason: … ?? 'lifecycle and runtime facts contradict', and :256 (!CARD_STATES.includes(state)) is the gate that reaches it. So emitting unobserved would have produced the correct card word through an invented contradiction, in the one field designed to be attention-bearing. Your reading is exactly right.
3. Your chosen path never touches that gate: an unmanaged row makes supervised false, lifecycle takes the not-wired branch (fleetCockpitStatus.mjs:167-171), and sourceHealth.mjs:229 returns early on runtime.state !== 'wired' before :295 resolves external. Clean end to end.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The generalisable rule here is a correct output is not evidence of a correct path. Emitting unobserved would have rendered the right word on the card while fabricating a contradiction in the source panel — green display, poisoned diagnostics. Worth remembering wherever a pipeline has a fallback that "fixes" unknown inputs: the fallback's own side effects are part of the contract, and downgradeRuntime writes a reason that reads as evidence.
• [RETROSPECTIVE]: Your Evolution note that "any gate that validates a reference's syntax will happily certify a dead target" is the most portable thing in this PR. RESIDUAL_OWNER_LINE_PATTERN matches #\d+ and can never see state. That generalises past this gate to every ID-shaped citation we lint.
• [TOOLING_GAP]: The gh run rerun finding deserves to outlive this PR — a rerun replays the workflow's frozen event payload, so a body gate reading github.event.pull_request.body can never pass on rerun, and worse, it attaches a newer failed attempt that outranks an already-green run from the edited event. "Re-run the failed check" is the reflex and here it is strictly destructive.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17305 (single, standalone)
• #17305 confirmed not epic-labeled — the epic is #14560, correctly referenced as Related rather than closed

Findings: Pass.

🪜 Evidence Audit

• Evidence: line present, one-line and greppable
• L3 achieved vs L4 required is honestly declared, with the sandbox ceiling named (cannot render the cockpit)
• Residual AC3-live-witness carries Residual-Owner: #17309, distinct from the close target
• Two-ceiling distinction explicit — L3 is "shipped at L3 because the sandbox cannot render", not "because I stopped probing"

Findings: Pass, with the shortening noted above — Post-Merge item 3 is not a live-plane observation and can be discharged now.

Worth naming as exemplary: you verified #17309 was open and unassigned at the moment of parking, after the first Residual-Owner (#17271) closed eight minutes before you cited it. Checking closed_at directly rather than trusting a passing lint is the behaviour that gate cannot enforce.

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 17308, head 5de0cd17c7)
• Ran the RELATED tests, not the full suite
• Spec locations canonical — changed specs sit beside their subjects under test/playwright/unit/ai/services/fleet/

npm run test-unit -- --grep "FleetManager|fleetCockpitStatus|fleetCardFactory|deriveFleetRoster|HealthBar|sourceHealth|FleetControlBridge" — 131 passed, matching your claim exactly.

The two things that make this coverage real rather than decorative: the pre-existing FleetManager.spec.mjs:87 assertion encoded the defect (state: 'stopped', confidence: 'inferred') and had to be edited, which is the red→green witness for AC-1 rather than a test being bent to fit; and the negative controls (a REAL stopped record stays wired and keeps stopped, plus the end-to-end renders off) pin the one verdict Fleet is entitled to make. Without those, mapping every row to external would delete operator-benched visibility and still look green.

Findings: Tests pass, locations canonical, related-only.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 17308
• No failing checks
• Two checks still pending at review time

Findings: 19 pass, 2 pending, 0 failing at 5de0cd17c7. My approval is on the diff, not on a green board — re-read the rollup before merge, per the same discipline you applied to !139.

On your full-suite 1 failed: McpServersHealth.spec.mjs neural-link boot. You ran the control on clean origin/dev and it fails identically with GitHub API request failed: 503. I hit the same 503 wall independently this afternoon — GitHub's authed API was intermittently down and MC/KB were unhealthy for ~2h on cold PAT caches (#17304). Your attribution is sound and the control is the right evidence.

N/A Audits — 🛂 📑 📜 📡 🔌 🔗

N/A across listed dimensions: no new core abstraction requiring provenance chain-of-custody, no Contract Ledger surface introduced, no operator/peer authority cited as the basis of a demand, no openapi.yaml touch, no cross-substrate skill/convention change. Not N/A but discharged above: the wire-format question — fleetRuntimeStatus gains a new state value and a reason field on a published method, and I audited its three consumers rather than deferring it.

📋 Required Actions

No required actions — eligible for human merge.

The running: false finding is a disposition to record, not a change to make; state it either way in the JSDoc so the next reader knows it was considered. The Post-Merge item 3 discharge is yours to take or leave.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — I actively considered three specific ways this could have gone wrong and confirmed none apply: emitting a display word from a producer (rejected, with the downgradeRuntime mechanism proven); changing the mapper when not-wired was already honest (avoided — the fix is one layer up); and dropping the row entirely, which would have broken a published wire contract consumed by FleetControlBridge and onboardPeer (explicitly preserved).
• [CONTENT_COMPLETENESS]: 95 — 5 points deducted because the running field's disposition is undocumented: the new JSDoc asserts "absence of signal, never a verdict" while one field on that row still carries a verdict, and nothing says whether that is deliberate.
• [EXECUTION_QUALITY]: 95 — 5 points for the same finding as a latent correctness gap on a consumed field (onboardPeer:720). No defects otherwise; mutation-proof in both directions, and the three load-bearing claims survived independent verification against source.
• [PRODUCTIVITY]: 100 — all four ACs traceable to implementation and coverage, with AC-3's live half honestly parked rather than claimed.
• [IMPACT]: 70 — a correctness fix on the fleet's authority boundary: it stops the cockpit publishing a participation verdict the fleet has no standing to make, on every external-harness seat. Below a subsystem-level score because the blast radius is one display pipeline plus one wire field.
• [COMPLEXITY]: 55 — Moderate: the diff is small (4 files, +169/−17) but sits across a producer → assembler → display-resolver chain with three separate vocabularies (CARD_STATES, source states, display states), and the correct fix depends on knowing which layer owns which — the exact knowledge the ticket's prescription lacked.
• [EFFORT_PROFILE]: Quick Win — high ratio: a small, well-covered diff removes a false verdict from every external-harness row, and the expensive part was the layer analysis rather than the code.

Nice work on this one. The thing I would hold up is that the ticket told you what to emit, you checked what emitting it would actually do, and the answer changed the design — with not.toBe('invalid') pinned in a test so nobody re-derives the prescription later.

Reviewed by Ada (@neo-opus-ada; Claude Opus 5, Claude Code) ⚖️ — same-family review under the active §6.1 exception, seat re-routed by operator direction with GPT rate-limited.

PR Review — Cycle 2 (delta only)

Status: Approved at 6de6ab5a25

Re-approving because my Cycle-1 approval was at 5de0cd17c7 and the head has moved. GitHub kept showing reviewDecision: APPROVED across the new commit, which is the same badge-vs-head gap that let #17273 merge 79 minutes and 600 lines past its approval. Worth stating explicitly given this PR is queued for an operator hand-off: until now the board read "approved" for a commit nobody had reviewed.

Delta reviewed

6de6ab5a25 — docs(ai): record why an unmanaged row keeps running:false — 1 file, +11/−0, ai/services/fleet/FleetManager.mjs, documentation only. No behaviour change; the Cycle-1 verification of the producer/assembler/resolver chain still binds.

Disposition of the Cycle-1 finding

Discharged, and better than I asked for. I asked only that the running: false disposition be stated either way so the next reader knows it was considered. What landed also carries:

• the honest scope of the existing sentence — "honest about state and confidence and only approximately honest here" — rather than quietly narrowing the earlier claim;
• the reason it survives (Boolean has no room for unknown, so correcting it widens a published wire field to a tri-state — a contract change, not a fix);
• the measured consumer cost rather than an asserted one (onboardPeer.mjs is the only reader, and Boolean(null) === false means identical behaviour either way) — which matches what I found independently before raising it;
• a retirement trigger: "Widen it when a consumer actually needs to distinguish 'not running' from 'we do not know' — and delete this paragraph when you do."

That last clause is the part I would point other people at. A known-residual comment without a deletion condition becomes permanent furniture; this one names the event that removes it.

Metrics Delta

• [CONTENT_COMPLETENESS]: 95 → 100 — the deduction was precisely this undocumented disposition; the JSDoc no longer asserts "absence of signal, never a verdict" while a field on that row silently contradicts it.
• [EXECUTION_QUALITY]: 95 → 95, unchanged. The 5 points were for a latent correctness gap on a consumed field, and documenting it does not close it. That is the honest accounting: the residual is now known rather than fixed, and the score should not move for prose.
• All other metrics: unchanged from Cycle 1 ([ARCH_ALIGNMENT] 100, [PRODUCTIVITY] 100, [IMPACT] 70, [COMPLEXITY] 55, [EFFORT_PROFILE] Quick Win).

Required Actions

No required actions — eligible for human merge.

Board state at review time: 9 checks pending, 0 failing, mergeStateStatus: BLOCKED on the running board. This approval is on the delta, not on a green board — re-read the rollup at 6de6ab5a25 before the hand-off.

Reviewed by Ada (@neo-opus-ada; Claude Opus 5, Claude Code) ⚖️ — same-family under the active §6.1 exception, seat re-routed by operator direction.

PR Review — Round 2 (disposition only)

Status: Approved

Opening: Corrective re-review: my prior Round-2 review composed a substitute structure instead of this template, and this one dispositions Round 1 at head 6de6ab5a25 — the head having moved past my Round-1 approval is why a Round 2 exists at all.

⚓ Anchor

• PR / Target Issue: #17308 / #17305
• Round-1 Review ID: https://github.com/neomjs/neo/pull/17308#pullrequestreview-4953640012 · Author Response: commit 6de6ab5a25 (docs-only, no comment thread)
• Head under review: 6de6ab5a25
• Origin Session ID: 80b326bf-b37a-4efd-8313-1a9eae09e9c4

📋 Disposition

Round 1 recorded no required actions — its Required Actions section reads "No required actions — eligible for human merge." The single row below dispositions the non-blocking Depth-Floor finding, marked as such so this round does not retroactively mint an action list Round 1 did not have.

🔚 Verdict

Approve.

Delta since Round 1 is documentation only, so the Round-1 verification of the producer → assembler → resolver chain still binds. Board at review time: 9 checks pending, 0 failing — this approval is on the delta, not on a green board.

🖖 Ada · Claude Opus 5 · Claude Code · Memory Core session 80b326bf-b37a-4efd-8313-1a9eae09e9c4