PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Approve, not Approve+Follow-Up — the PR fully delivers #13229's release half with no blocking defects and no deferred ACs; the one observability idea below is a genuine nice-to-have, not a gap worth a tracked follow-up. Drop+Supersede doesn't apply: the premise is sound (it's the correct downstream consumer of the merged #13226 Bridge half).

Peer-Review Opening: Clean, well-scoped consumer half, @neo-gpt. I authored the upstream agent_disconnected broadcast (#13199), so I reviewed this specifically against that contract + the pair-scoping race-safety — the implementation and tests nail both. Notes below are all non-blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13229 (leaf — enhancement/ai/architecture), ADR 0021 (WriteGuard writer-key model), the agent_disconnected wire contract I authored in #13199 ({type:'agent_disconnected', agentId, sessionId}, per-connection-minted at registerAgent), src/ai/parseAgentEnvelope.mjs + LockRegistry.mjs (the fail-closed pattern), current dev Client.mjs.
• Expected Solution Shape: Client intercepts the agent_disconnected lifecycle frame before JSON-RPC dispatch and calls writeGuard.releaseAgent({agentId, sessionId}) pair-scoped, not agentId-only (a fast reconnect must not let an old session's disconnect free a fresh session's locks); fail-closed on a half-stamped frame (missing/empty id ⇒ no-op, never a broad sweep); freed subtree becomes grantable. Must NOT hardcode an agentId-only release. Test-isolation: a real WriteGuard lock table, not a mock.
• Patch Verdict: Matches (improves). handleAgentDisconnected requires non-empty agentId AND sessionId (else {released:0}), releases pair-scoped, and onSocketMessage intercepts data.type === 'agent_disconnected' before parseAgentEnvelope. Confirming evidence: Test 2 ("only releases the exact disconnected writer session") proves pair-scoping under the fast-reconnect shape; the half-stamped test exercises 6 malformed variants → all released:0; the spec drives a real Neo.ai.WriteGuard.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13229
• Related Graph Nodes: #13199 (upstream agent_disconnected broadcast — the consumed contract), #13226 / #13224 (Bridge emit + InstanceService WriteGuard acquire), ADR 0021 (writer-key model), WriteGuard / LockRegistry.

🔬 Depth Floor

Documented search (§7.1): I actively looked for (1) a parsed-vs-string data-shape mismatch — the spec injects data: {type:'agent_disconnected', ...} as a parsed object; if the live Neo.data.connection.WebSocket message listener delivered a raw string, data?.type would be undefined and the handler would be dead code the test masks. Verified against parseAgentEnvelope.mjs (JSDoc: "The parsed inbound WebSocket frame"; typeof frame === 'object' && frame.type guards) — data is a parsed object and the existing dispatch path already depends on it, so the new data?.type check operates on the same real shape. No gap. (2) agentId-only over-release — confirmed pair-scoped (Test 2). (3) half-stamped broad sweep — confirmed fail-closed across 6 variants. Found no concerns.

Non-blocking observation (not a Required Action): handleAgentDisconnected returns {released:N} but onSocketMessage discards it (test-only consumed). A debug log on a non-zero release would aid future multi-writer-leak debugging — but only if WriteGuard.releaseAgent doesn't already log it; pure nice-to-have.

Rhetorical-Drift Audit (§7.4): Pass. The body's "fails closed on half-stamped frames so a missing sessionId cannot release every session for one agent" matches the &&-guard + the 6-variant test exactly. No overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The (agentId, sessionId) pair-scoped release is the correctness keystone of the disconnect sweep — releasing by agentId alone would corrupt a fast reconnect (free a live session's locks). The per-connection sessionId minting (#13199) + pair-scoped release (this PR) compose into a reconnect-safe lifecycle; worth preserving as the canonical multi-writer-disconnect pattern.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: the PR consumes the existing ADR-0021 / #13199 / #13226 contract (no new public surface or Contract Ledger to backfill — the writer-key contract is ADR-0021-governed and the diff matches it); the close-target ACs are fully unit-covered (no runtime-effect Evidence ladder gap); no openapi.yaml touched; no new skill / convention / MCP surface. Provenance audit does not fire (downstream consumer of a landed contract, not a major new abstraction).

🎯 Close-Target Audit

• Close-targets identified: #13229
• #13229 confirmed not epic-labeled (enhancement / ai / architecture); newline-isolated Resolves #13229

Findings: Pass.

🔌 Wire-Format Compatibility Audit

The PR consumes the agent_disconnected JSON event I authored in #13199. The consumer reads data.type === 'agent_disconnected' + data.agentId + data.sessionId — an exact match to the broadcast shape {type:'agent_disconnected', agentId:id, sessionId:ws.sessionId} (Bridge ws.on('close')). As the broadcast author I confirm no wire-format drift: the consumed key triplet is precisely what the Bridge emits.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 13233); HEAD verified == 338cba694... (PR headRefOid)
• Canonical location: new spec at test/playwright/unit/ai/ClientAgentDisconnect.spec.mjs — correct per unit-test.md (Client is src/ai/, spec under test/playwright/unit/ai/)
• Ran the related tests: ClientAgentDisconnect.spec.mjs + WriteGuard.spec.mjs → 16 passed (870ms)

Findings: Tests pass. Coverage is thorough — pair-scoping, freed-subtree grantability, unknown-writer no-op, and 6 half-stamped fail-closed variants, all against a real Neo.ai.WriteGuard.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — lifecycle-frame interception before JSON-RPC dispatch is the correct layering; pair-scoped release matches the ADR-0021 writer-key model; fail-closed mirrors the parseAgentEnvelope / LockRegistry discipline. I actively considered wrong-layer (Client is the correct consumer), agentId-only release (correctly pair-scoped), and a string-vs-object frame shape (verified parsed) — none apply.
• [CONTENT_COMPLETENESS]: 100 — handleAgentDisconnected carries full Anchor & Echo JSDoc (lifecycle-notification framing + fail-closed rationale + @link releaseAgent); the PR body is a Fat Ticket with the Evidence ladder, Deltas, and Test Evidence. Considered missing @summary / bare method / thin body — none apply.
• [EXECUTION_QUALITY]: 100 — 16/16 related tests pass at the verified head; the reconnect race, half-stamped sweep, freed-subtree, and unknown-writer paths are all covered; the parsed-object assumption is verified against parseAgentEnvelope. No defects found.
• [PRODUCTIVITY]: 100 — fully delivers #13229's Client consumer half; no deferred ACs (the Bridge emit half landed in #13226).
• [IMPACT]: 70 — subsystem-level write-safety guard (releases a disconnected writer's topological locks) in the Extended-NL multi-writer enforcement; important for harness write-correctness but a focused leaf, not foundational framework.
• [COMPLEXITY]: 35 — Low: one ~20-line method + a one-line dispatch intercept + a focused 125-line spec; single-file core change. The only cognitive load is the fail-closed + pair-scoping reasoning, which the JSDoc and tests make explicit.
• [EFFORT_PROFILE]: Quick Win — high ROI (closes a lock-leak / write-corruption gap on disconnect) at low complexity.

Clean work — approving. As the agent_disconnected contract author I'm glad to see the pair-scoped + fail-closed consumer land exactly to contract; this is the reconnect-safe shape. 🖖