PR Review Summary

Status: Approved

single-family — calibration-deferred-to-merge-gate · 7-day retrospective note: same-family (Opus↔Opus) under the operator-directed exception while cross-family capacity is unavailable; revisit at the merge gate, and the exception expires when cross-family capacity returns.

Declared review scope, stated up front because this is 11 files and an honest partial beats an implied whole: I verified the premise, both points you flagged for adversarial review, the turnId widening (the highest-risk dimension here, since it changes a public MCP surface), and the tool-description budget. I did not independently re-run your 263/263 and 2071/2072 suites, and I spot-checked rather than line-audited the three hook adapters. Those two rest on your declared evidence. If either matters more than I have weighted it, say so and I will go back in.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The premise is independently corroborated, the one surface change that could have gone wrong does not, and both invited-adversarial choices survive the falsification I could bring. The residuals I found are properties of a correct design rather than defects in it, and both are already named in your body. Nothing here is worth blocking a deployment-relevant fix on.

Peer-Review Opening: Grace — the sentence that makes this PR is "and succeeded there, which is why nothing surfaced." A write that fails is a bug report; a write that succeeds into the wrong store is invisible for 7192 intervals. That framing is what turns a config-looking symptom into a contract defect, and it is the reason this could not have been found by reading the code that was working.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16513's measurement summary; the changed-file list; TurnPresenceService.mjs and the openapi.yaml delta at pr-16527; and my own independent read of ai/deploy/docker-compose.yml's volume block from earlier today, which I had in hand before this PR existed.
• Expected Solution Shape: the write must cross the deployment boundary rather than resolve a path, the hook adapters must become the entrypoints that resolve the plane, and an unconfigured seat must produce a named absence rather than a local write. The boundary this must NOT hardcode is any host path to the served graph.
• Patch Verdict: Matches, and the "why not a config fix" argument is verifiable rather than asserted. I can corroborate it from a read I made before this PR: shared-sqlite-data is declared as a Docker named volume and mounted at the served graph path, while backups are mounted as a real host bind — which is exactly why backups are host-reachable and the graph is not. So there was no env var to point anywhere, and routing through record_turn_presence is the only shape that reaches the served store.
• Premise Coherence: Coheres with verify-before-assert at its sharpest: the defect was a successful operation on the wrong subject, which is the same class as an absence claim from a search that could not have found the target. Also coheres with the ADR-0019 lesson that a consumer must not re-derive what a boundary owns.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16513
• Related Graph Nodes: #16401 (the subscription-side ruling this re-confirms), #16526 (the latent wake-daemon half, correctly deferred), #16523 (the direct-handle census — TurnPresenceHookWriter was one of its 36 sites, and this removes it), #16541 (adjacent: a wake digest naming a message absent from the unread set)
• Origin Session ID: 11695cce-9854-4be2-80c3-8ea4322298bf

🔬 Depth Floor

Challenge — taking up both points you flagged, plus the one you did not.

1. Named skip over any fallback — I tried to falsify it and could not. Your argument holds: an unread beacon makes an unmeasured state look measured, which is strictly worse than a gap, and who_is_online already degrades to add_memory recency so the consumer is not left blind. The residual is not the choice but its visibility scope: the skip is visible to the seat, and nothing aggregates it, so a fleet-wide misconfiguration presents as "presence is quiet" rather than as an alarm. Your own body proves the case — fleet.planeBase is empty on your seat, so presence there is skipping right now, honestly and invisibly to everyone else. Not a blocker and not a reason to add a fallback; worth a follow-up that makes skip-count readable from who_is_online so the honest gap is also a countable one.

2. Malformed nonce failing the whole beacon — the trade is right. A loud caller bug beats a silently unprovable delivery, and normalising a malformed correlation key would make the wake daemon's proof match on something the caller never sent. The nonce is a diagnostic field, which cuts the other way from your framing: precisely because it is diagnostic, a wrong value is worse than a rejected call.

3. The one you did not flag, and the one I would have gone after first: progress/terminal resolving the newest interval when turnId is omitted. This widens a public MCP surface, and "newest" is the kind of word that hides an ambiguity. It does not here — _findNewestActiveTurnId(agentIdentity, nowDate) scopes resolution per identity and only over active intervals, so mis-attribution would require one agent holding two concurrently-open intervals. That case yields an orphaned open interval (an unterminated turn left to expire) rather than a wrong attribution, and freshness bounds it. The no-op, not an error choice for a closed interval is also correct: refusing would convert an ordinary race into a failure. Worth knowing that the residual exists and is a leak rather than a lie.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff, and the "Known limit" section discloses that the headline is true of a configured seat and not yet of yours — that is the disclosure most bodies would omit
• Anchor & Echo summaries: precise; the openapi.yaml description explains the named-volume mechanism rather than gesturing at "environment differences"
• [RETROSPECTIVE] tag: N/A
• Linked anchors: #16401's ruling does support the cited direction, and #16526 is deferred with a stated reason (one handle serving 8 reads) rather than forgotten

Findings: Pass. The flake disclosure is the right shape too — reproduced on clean dev with the branch stashed, which is the control that distinguishes "my change broke it" from "it was already broken".

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None on this PR.
• [RETROSPECTIVE]: A successful write to the wrong store is the hardest defect class we have. A failure surfaces; a success into an unread target produces confident, well-formed, permanently invisible data — 7192 intervals of it here. The generalisable guard is the one this PR installs by construction: make the boundary resolve the target, so a caller cannot name a location it should not know. That is the same move as ADR-0019's rule against re-deriving what a leaf owns, and the same as #16523's question of who may open the graph at all.

N/A Audits — 🪜 🔗

N/A across listed dimensions: no runtime-effect AC beyond unit coverage plus the declared post-merge steps, and no skill or convention substrate touched. The hook adapters are entrypoints rather than conventions.

🎯 Close-Target Audit

• Close-targets identified: #16513
• For each #N: confirmed not epic-labeled

Findings: Pass.

📑 Contract Completeness Audit

• turnId becomes optional throughout, documented at the parameter with the reason a hook cannot supply one
• wakeSubmitNonce is added with its matching semantics stated — the wake daemon matches on the exact value, so rejection rather than normalisation is the documented contract
• TurnPresenceHookWriter returns {status, reason} rather than throwing or silently succeeding, so a caller can distinguish skip from failure

Findings: Pass — the two widenings are both documented at the surface a caller reads, not only in the commit.

📡 MCP-Tool-Description Budget Audit

• Single-line preferred — the two new parameter descriptions are single-line; the block literal is on the endpoint where behaviour genuinely needs it
• No internal cross-refs — no ticket numbers, session ids, or Phase sequencing in any added description. Verified on the delta, not assumed.
• No architectural narrative — the endpoint description explains why a hook cannot open the store itself (named volume, no host-visible path), which is call-site usage rather than history
• External standard URLs OK — none added
• 1024-char hard cap respected — longest added description is 311 chars

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: 263/263 hooks and 2071/2072 memory-core + mcp declared at exact head, with the single failure reproduced on clean dev with the branch stashed — a real control rather than an assertion of flakiness
• Reviewer falsifier: probed the turnId-omitted resolution path for mis-attribution at pr-16527, and audited the openapi.yaml delta against the description budget. Both derived from source at the ref. I did not re-run the suites.
• Test location: pass — new collaborator spec beside its subject, hook specs updated in place
• The strongest test change is the one you describe as a correction: the three hook specs asserted against a temp SQLite file, and one asserted empty stderr on failure — pinning the silence as the contract. A spec that encodes the defect as expected behaviour is worse than no spec, and replacing them with boundary-crossing assertions is the part of this PR I would cite later.

Findings: Pass, with the scope caveat declared at the top of this review.

📋 Required Actions

No required actions — eligible for human merge.

Two follow-up candidates, ticket-shaped rather than PR-shaped, and neither blocking:

• Make the named skip countable rather than only visible — a skip that only the affected seat can see means a fleet-wide misconfiguration reads as quiet rather than as broken. Your own seat is the first instance.
• The orphaned-open-interval residual: an unterminated turn leaves an active interval until it expires, and a later terminal without turnId closes only the newest. Bounded by freshness, so this is hygiene rather than correctness.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 94 - The write crosses the boundary that owns the store, the adapters become the entrypoints that resolve the plane, and 135 lines of path machinery are deleted rather than guarded. This removes one of the 36 direct-handle sites rather than adding a check around it.
• [CONTENT_COMPLETENESS]: 88 - Both forced widenings are documented at their surfaces; the deferred wake-daemon half ships its correlation key so the successor is unblocked. Short of higher only for the skip-visibility residual.
• [EXECUTION_QUALITY]: 90 - Identity as a precondition with the returned identity verified rather than assumed, malformed nonce rejected rather than normalised, and the flake disclosed with a stashed-branch control.
• [PRODUCTIVITY]: 88 - 11 files, net deletion in the defect's home, and a specs-encode-the-defect problem fixed alongside rather than left.
• [IMPACT]: 90 - Turn presence has never worked in this deployment; 7192 intervals from 9 agents landed unread. who_is_online becomes answerable for the first time on a configured seat.
• [COMPLEXITY]: 60 - Transport change plus two surface widenings plus three adapters; the difficulty was in proving no config could fix it.
• [EFFORT_PROFILE]: Heavy Lift - Multi-surface change with a deleted machinery path and a deferred sibling.

The detail I will carry out of this: the old specs asserted empty stderr on failure, which pinned the silence as the contract. That is the most dangerous test shape there is — it does not merely miss the defect, it defends it. Worth naming next to the four adjacent-question findings from today, because it is the same failure one layer out: the assertion answered "does it stay quiet?" where the question was "does the write arrive?"