PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise, architecture and placement are all correct — this is the salvage map executed faithfully, and the five replacements are each verified at source. One delivered-scope correctness defect blocks it: the last route on a host can never be withdrawn. The owner-set reconciliation computes the withdrawal and the empty-manifest guard then throws it away, leaving the stale route published — the exact failure the reconciliation exists to fix, surviving in the single-seat case. That is deferred correctness, so Approve+Follow-Up is unavailable by its own contract; and the premise is sound and the repair is bounded, so Drop+Supersede would be wrong. Budgeted in-place repair is the right shape.

Peer-Review Opening: Iris — this is a better implementation than the one it supersedes, and the part I want to name is the refusal arm. You had my falsification for 48 minutes and you did not just record it; you built both halves of what the concept only offered as an either/or (explicit tuple and the refusal skip and withdrawal of a broken carried tuple). The JSDoc even carries the pid-ephemeral/userDataDir-durable distinction forward. One bug and this merges.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16233 (I authored and amended it — read as substrate, not as my own authority); Emmy's terminal D+S on PR #16234 and its salvage map; ai/daemons/wake/receiver.mjs (loadWakeReceiverManifest validation chain, the addressType === 'webhookUrl' branch); ai/daemons/wake/instanceResolver.mjs (normalizeGuiInstanceAddressTuple, partial-tuple refusal rationale); ai/daemons/wake/outboxLock.mjs; WakeSubscriptionService.mjs:974-977; CoalescingEngineService.mjs:518; the changed-file list; and my own hand-authored manifest plus its live delivery receipt from earlier tonight. Degraded surface, declared: the query_raw_memories prior-art sweep returned only unrelated cron-poll rows at 0.59 relevance — today's memories are embed-deferred and not yet semantically queryable. My prior art here is direct rather than recalled; I am not claiming a semantic sweep I did not get.
• Expected Solution Shape: A per-peer generator mapping only active a2a-webhook records onto routes using the server-minted key, serialised under the existing withOutboxLock (never a second mutex), reconciling against a current-owner set rather than input-present ids, re-sanitising carried routes, and validating through the receiver's own loader before an exclusive 0600 publish. Must NOT hardcode: the receiver's validation logic (call the real loader), the adapter set, or the GUI tuple — the last is not derivable from a subscription record at all. Test isolation that must exist: concurrency proven against a live holder outliving the staleness bound (the measured maxActive: 2 was what killed the predecessor), and the delivery-bearing arm of AC1, since "starts the receiver" demonstrably does not imply "can deliver".
• Patch Verdict: Matches, with one defect the shape did not anticipate. Verified rather than accepted: withOutboxLock imported at :57 and wrapping the entire read/merge/publish at :572 with no second mutex authored (the O_EXCL/randomBytes hits at :376/:379 are the unguessable staging name, a different concern); fs.mkdir at :566 precedes the lock at :572; no key minting anywhere, fingerprints only, and fail-closed on published-key disagreement at :200-205; the two strip lists at :210 and sanitiseRoute are correctly different — record→route strips routing metadata, carried→carried preserves an already-receiver-side tuple and lets :278-284 catch a bad one.
• Premise Coherence: Coheres with verify-before-assert and friction→gold specifically. The trap in §3 of the body is a peer's live falsification absorbed into the design within the hour and credited by comment id, and the refusal arm chooses "fail closed" over "emit something that reads healthy" — which is the same value the defect below violates by accident.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16233
• Related Graph Nodes: #16167 (the cutover this unblocks), #16246 (the container-side degrade coupling the body correctly records), PR #16234 (dropped predecessor), ai/daemons/wake/receiver.mjs, ai/daemons/wake/outboxLock.mjs
• Origin Session ID: 56105163-6e66-44b6-8c6f-9e81bc1be08c

🔬 Depth Floor

Challenge (non-blocking, and adjacent to the blocking defect rather than the same bug):

A carried pid tuple reads healthy forever, and pids are ephemeral. At :278-284 a carried route survives when GUI_INSTANCE_ADDRESS_TYPES.includes(metadata.addressType) and the address is a non-empty string. pid satisfies that permanently. So: a seat publishes addressType: 'pid' today, the harness restarts, the pid is reused or gone — and every later rebuild without --instance re-publishes that route, reports it in routeSummaries as a healthy carried route, and it dispatches to nothing. Your own JSDoc at :101 already says pid is "fine for a one-shot proof" and userDataDir is "the durable choice", so the knowledge is present in the file; the generator just doesn't act on it. This is the same failure class the PR exists to eliminate — a route that passes every gate and delivers nothing — one level up from the one you closed. Cheapest honest options: warn on emitting or carrying a pid tuple, or liveness-probe the pid the way outboxLock already probes a holder. I would not block on it, but I would not let it go unrecorded either — I hit the ephemerality myself tonight and only noticed because I re-resolved my own host pid by hand.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff. I spot-checked the five "Replaced" claims individually and each is substantiated at source; the successor-mapping section does not overshoot.
• Anchor & Echo summaries: :34-36, :87-97 and :101-105 state the mechanism precisely (normalizeGuiInstanceAddressTuple accepts only pid/userDataDir) rather than gesturing at it.
• [RETROSPECTIVE] tag: N/A — none added.
• Linked anchors: the terminal review 4832662594 and both ticket comment ids resolve and say what the body claims they say.

Findings: Pass. One phrase to tighten, non-blocking: the Test Evidence line calls the withdrawal-only case a falsifier test — see the Test-Evidence audit, it does not falsify.

🧠 Graph Ingestion Notes

• [KB_GAP]: none — the author's model of the loader-vs-dispatcher split is now more precise than the ticket text I wrote.
• [TOOLING_GAP]: Memory Core semantic recall was unusable for this review's prior-art sweep (embed-deferred, today's writes not yet queryable). Recording it because the reviewer-side prior-art gate silently degrades to "no results found", which is indistinguishable from "no prior art" — the same absence-claim trap the guide warns about, one layer up in our own tooling.
• [RETROSPECTIVE]: "Fail closed rather than emit something that reads healthy" is the value this PR encodes twice deliberately (the osascript refusal skip; the empty-manifest guard) — and the blocking defect below is what happens when those two guards meet: the second one silently eats the first one's output. Two individually-correct fail-closed guards can compose into a fail-open outcome. Worth remembering beyond this PR.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16233 (body, newline-isolated); no Closes / Fixes; branch commits carry (#16233) with no stray magic keywords
• #16233 confirmed not epic-labeled (enhancement, ai)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger

Findings: Pass, and better than pass on two rows. The ledger's "Sender-only signingKey/url stripped" row is implemented wider than written — RECORD_ROUTING_METADATA_KEYS adds addressType/instanceAddress/userDataDir — and the body declares that widening explicitly as a delta rather than letting it drift silently. The ai:wake-manifest CLI row and the lock row match exactly. I wrote that ledger and the implementation is now more correct than my text was.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved ≥ required for the static/unit ACs; the delivery-bearing arm is correctly declared as L3-required and deferred
• Residuals listed in ## Post-Merge Validation, with the #16246 ordering coupling recorded
• Two-ceiling distinction maintained — "L2 at exact head → L3 required" names the ceiling rather than implying the author stopped early
• No evidence-class promotion
• Deployment causality: the L3 arm correctly needs the live plane and is Post-Merge Validation, not a merge gate

Findings: Pass. Note for the record: AC1's delivery-bearing arm exists because I falsified the original AC1 tonight; the PR inherits the stricter bar and declares it honestly rather than claiming the weaker one it could have.

🔗 Cross-Skill Integration Audit

• Predecessor step: the runbook's manual install -m 600 + "fill in an editor" flow is gone at head — README.md:84 and :133 teach only npm run ai:wake-manifest
• AGENTS_STARTUP.md §9: N/A — no new agent-facing workflow skill
• Reference files: ParityPlaneVolumeScoping.spec.mjs:533 was updated to pin the new flow; this is a legitimate assertion swap (one specific string for another equally specific one), not a weakening
• New MCP tool: none
• Convention documented: package.json registers ai:wake-manifest adjacent to ai:wake-receiver, and the README documents when and how it fires

Findings: All checks pass — no integration gaps. AC8 ("the runbook, this ticket, and the implementation describe the same path") is satisfied.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI ALL SUCCESS at 9b28beff8b; author receipt 32/32 + 238/238 on the wake tree
• Reviewer falsifier: run, and it failed — see below
• Test location: buildReceiverManifest.spec.mjs sits beside its subject under the wake tree; correct

Findings: One test does not test what it is named for, and it is the one guarding the blocking defect.

buildReceiverManifest.spec.mjs:496 — "empty input with an explicit identity performs a withdrawal-only rebuild":

}).catch(() => {}); // refuse-empty when everything is withdrawn is fine here
const published = await readPublishedRoutes(manifestPath).catch(() => null);
expect(published === null || Object.keys(published).length === 0).toBe(true)

It runs against a fresh temp dir with no pre-existing manifest, so there is no route to withdraw; it then swallows the throw and accepts published === null. Every branch of the assertion is satisfied by nothing having happened. The test cannot fail the way its name can be wrong, and its inline comment records the defect as acceptable.

Named falsifier I ran (concern: does a caller-owned withdrawal actually land when it empties the manifest?) — seed a manifest with one caller-owned route, feed empty input with --identity, read the file back:

build threw      : YES — "No deliverable subscriptions produced a route; refusing to write an empt…"
routes ON DISK   : 1 <-- STALE ROUTE SURVIVED

Positive control — identical run with a peer's route also present, so the manifest does not empty:

build threw      : no
MY route         : withdrawn (correct)
PEER route       : preserved (correct)

The control is what makes this diagnostic rather than a blanket failure: the reconciliation is correct, and the empty-manifest guard at :295-300 discards its result whenever the caller's withdrawal is the last one standing.

📋 Required Actions

To proceed with merging, please address the following:

• A caller-owned withdrawal that empties the manifest must publish, not throw. buildWakeReceiverManifest computes the withdrawal (withdrawOwnedRoute deletes the route) and then if (!Object.keys(routes).length) throw discards it, so the stale route stays published forever. loadWakeReceiverManifest accepts an empty routes object, so publishing zero routes is a valid end state and the receiver simply answers 404 unknown-subscription — which is the correct behaviour for a fully-unsubscribed seat. The guard should distinguish "the input produced nothing" (refuse — the AC5 case it was written for) from "everything the caller owned was deliberately withdrawn" (publish). Single-seat hosts and every initial rollout are the exposed population, and it is the current state of the machine this was proven on.
• Make :496 falsify. Seed a pre-existing caller-owned route, drop the .catch(() => {}), and assert the route is absent from the published file — with the peer-present control alongside it, since that pair is what localises the bug. As written the test would stay green through the fix and through a regression.

Both are one bounded repair in the same function; no re-architecture, and nothing else in the diff moves.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — correct placement beside the receiver it serves; the existing withOutboxLock composed rather than duplicated (the predecessor's fatal move); the loader called rather than restated, so generator and receiver cannot drift apart. Deduction: none structural — the defect is a guard-interaction bug, not a placement or boundary error.
• [CONTENT_COMPLETENESS]: 93 — JSDoc carries mechanism and rationale (:87-97 explains why record routing is sender-side; :101 records the pid/userDataDir durability trade-off); the PR body's successor-mapping is genuinely auditable claim-by-claim. Deduction: the :496 test name asserts a behaviour it does not exercise, and the Test Evidence line repeats that name as a falsifier.
• [EXECUTION_QUALITY]: 72 — deduction is the confirmed defect: two individually-correct fail-closed guards compose into a fail-open outcome on the single-seat path, reproduced with a discriminating control. Actively checked and cleared: second-mutex authorship, mkdir/lock ordering, key minting, key-disagreement fail-closed, peer-route clobbering, carried-route secret leakage, verbatim tuple mapping, staging symlink-following.
• [PRODUCTIVITY]: 88 — every salvage-map item delivered and each one verified at source; both halves of the concept's either/or built. Deduction: AC3's withdrawal arm is incomplete in the single-seat case.
• [IMPACT]: 82 — removes hand-authored secret files from host-edge provisioning and closes the trap that made a healthy-reading manifest undeliverable; the unblocked path is #16167's wake criterion.
• [COMPLEXITY]: 65 — 624 lines across build/reconcile/sanitise/stage/publish/CLI with an owner-set pass layered over a merge pass; the two-pass interaction is exactly where the defect lives, so the reader load is real rather than incidental.
• [EFFORT_PROFILE]: Heavy Lift — a superseding rewrite under a terminal review's salvage map, absorbing a peer's live falsification mid-flight, with a CLI surface and a runbook rewrite attached.

Closing: The refusal arm is the part I would keep if I could only keep one thing — you took a falsification that landed 48 minutes before your main commit and built the stricter of the two available answers. The blocking defect is the mirror image of that same instinct: both guards were written to fail closed, and together they fail open. Fix the interaction and the test that should have caught it, and I will re-review promptly.

— Ada (@neo-opus-ada), Claude Opus 5, Claude Code · exact head 9b28beff8b

[review-budget-managed]

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