PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The HMAC verification is textbook-correct on every axis that matters, the graphless claim is proven by a genuinely transitive closure walk rather than asserted, and the staging is stated honestly — Shape B lands here, Shape C's deletion is #16167's. My two findings are a guard-shape upgrade and a single-layer replay observation, neither blocking. Not Request Changes: the two places this class of change usually fails — a === on a digest, and an HMAC over re-serialized JSON — are both done right.

Peer-Review Opening: Signing rawBody bytes rather than a re-stringified parse is the decision that makes this verification actually sound, and it is the one most often gotten wrong. Everything else follows from getting that right.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16180's framing via the body, ai/daemons/wake/queries.mjs and daemon.mjs on dev (to establish what "graphless" is being measured against — I had verified the direct better-sqlite3 handle at queries.mjs:2 / daemon.mjs:3003 earlier today), the receiverDependencyClosure spec read in full, and the receiver's manifest-validation and verification paths at the PR head. Fresh origin/dev fetched before comparing.
• Expected Solution Shape: a receiver whose dependency closure provably excludes the graph, and an HMAC path that (a) compares in constant time, (b) verifies before the payload is used, and (c) signs the exact received bytes rather than a re-serialization. Key material must not sit in a world-readable file. It must not ship a dormant compatibility layer, and the staged coexistence with Shape C must be named with an owner rather than left implicit.
• Patch Verdict: Matches. Evidence: verifyWakeSignature shape-validates /^[a-f0-9]{64}$/i before Buffer.from, computes over rawBody as a Buffer, checks length before crypto.timingSafeEqual — necessary, since that primitive throws on length mismatch and would otherwise become a crash-oracle — and returns false on every malformed input. The closure spec recurses over relative specifiers and catches all three import forms including dynamic import(...).
• Premise Coherence: Coheres with the operator's no-dormant-compatibility principle, applied by the author to his own diff: the Deltas section records that "current Shape C persists wakePolicy but does not consume it," declines to add a dormant layer, and routes it to #16167 as "activate one real policy gate or retire the unused surface." Finding an unconsumed surface in your own design and refusing to extend it is the discipline this whole migration thread has been about.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16180
• Related Graph Nodes: #16167 (owns the live cutover, the L4 witness, and Shape-C deletion) · #15798 (the epic) · ADR 0002-phase3 wake-substrate standards alignment (amended here) · learn/agentos/wake-substrate/PersistentProcessManagement.md
• Origin Session ID: 0a7f5f1d-cf12-4698-984c-17b64eea5178

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (primary — the guard proving an absence is a denylist, and should be an allowlist): receiverDependencyClosure.spec.mjs enumerates five forbidden paths plus the better-sqlite3 sentinel. Its entire job is proving that something is not reachable — and a denylist can only prove the absence of what someone thought to list. A new graph surface introduced at a sixth path passes this test unless it happens to import better-sqlite3 directly.

  Second, narrower edge in the same walk: recursion follows only specifiers starting with .. A bare specifier other than better-sqlite3 is added to the set but not walked, so anything beneath a non-relative hop into repo code is invisible. That is fine today because ai/ uses relative imports pervasively — but that is a convention, not an enforced property, and this guard's completeness silently depends on it.

  To make this structurally sound: invert it. Assert the closure contains only files under an approved set — ai/daemons/wake/ plus a named few — so any new dependency fails closed and must be justified, rather than only the five you predicted. That is the same negative-result-needs-a-control discipline the rest of this PR embodies, applied to the test that carries the headline claim.

• Observation (replay defense is single-layer, low severity): I searched the receiver for timestamp, issuedAt, freshness, maxAge, and clockSkew — zero hits. So there is no envelope freshness window, and replay protection rests entirely on the durable dedup store in receiverState.mjs.

  That makes the dedup store's retention policy a security property, not housekeeping — which is the part worth naming, because it will otherwise be tuned as a disk-usage concern. My grep for pruning vocabulary in receiverState.mjs returned nothing, and I am deliberately not concluding "no pruning exists" from that: my pattern may simply have missed it. Either reading yields the same guidance — if the store never prunes it grows monotonically, and if it does prune the replay window reopens at the prune boundary.

  Severity is low: replaying requires a captured signed envelope, which requires access to the host bind, and the payload is a wake — a spurious wake is noise, not escalation. A timestamp plus a skew window would make this two-layer and decouple replay safety from retention entirely.

• Where I looked hardest and it was right: the two ways HMAC verification usually breaks. (1) Timing. crypto.timingSafeEqual, not === on hex strings — and the length comparison is placed before it, which is required rather than merely tidy, since timingSafeEqual throws on length mismatch and an unguarded call converts a forged signature into an exception. (2) What gets signed. The HMAC is computed over rawBody as a Buffer. An HMAC over a re-serialized parse is forgeable through key-order and whitespace differences, and it is the single most common defect in this shape. Signing the received bytes closes it by construction.

• And a secret-handling detail I rarely see done: if ((stat.mode & 0o077) !== 0) rejects a routing manifest carrying group or other permissions. That file holds signingKey values, and refusing to read a world-readable secret store — rather than reading it and warning — is the correct polarity. Paired with signingKey.length < 32 as a minimum and schemaVersion !== 1 failing closed, the key material has a real posture rather than an implied one.

Rhetorical-Drift Audit (per guide §7.4):

• The staging is stated honestly, which is the claim most available for inflation here. better-sqlite3 is still present in the wake lane at this head (queries.mjs:2, daemon.mjs), and the body does not pretend otherwise — Shape B is the graphless target-state receiver, Shape C remains until #16167 deletes it after the live witness. Neither over-claiming the contributor win nor hiding that it is staged.
• Evidence: line honest and specific about what L3 covers versus what #16167 owns at L4.
• Anchor & Echo: verifyWakeSignature's summary — "Compares an exact-body HMAC against the hexadecimal Shape-B signature header" — says exact-body, which is the load-bearing property rather than a restatement of the signature.
• [RETROSPECTIVE] tag: N/A — none claimed.

Findings: Pass. Notably: *"Existing non-CI L4 coverage: none found or executable on this machine because no Docker daemon is installed; this remains #16167 cutover evidence, not hidden completion evidence for #16180."* That sentence exists to prevent a reader from laundering deferred evidence into completion, written by the author against his own interest.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Carried. merge-readiness returns IDENTITY_BINDING_MISSING from my seat; get_pull_request_diff's file parameter is ignored (reported #16173), so local per-file git diff throughout.
• [RETROSPECTIVE]: Two worth keeping. First: sign the bytes you received, never the bytes you can reproduce. An HMAC over JSON.stringify(parsed) is forgeable via key ordering, whitespace, and unicode normalisation — the attacker picks a serialization the verifier will regenerate differently. Verifying over the raw Buffer removes the entire class, and it is why this receiver's verification is sound rather than merely careful. Second: a test that proves an absence inherits the shape of its list. A denylist proves only the absence of enumerated things; an allowlist proves the absence of everything unenumerated. When the claim in the PR title is the absence — "graphless" — the guard should be the second kind, because the failure mode is a dependency nobody predicted.

N/A Audits — 📑 📡

N/A across listed dimensions: no Contract Ledger surface beyond the internal wake envelope (no MCP tool or public config leaf added — the single configBase.mjs line is a one-character adjustment), and no OpenAPI tool description touched.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16180, newline-isolated. Related: #16167 non-closing, and it carries the deferred L4 plus Shape-C deletion.
• For each #N: #16180 is the delivered leaf; #16167 correctly retains the cutover and the deletion rather than this PR claiming either.

Findings: Pass. Single commit 4c709b256d carrying (#16180). True surface verified against a freshly-fetched dev: 26 files, +2875/-178.

🔗 Cross-Skill Integration Audit

• Predecessor step updated: ADR 0002-phase3 amended (+69/-…) so the wake-substrate standard describes the signed graphless shape rather than the graph-worker one.
• Operator-facing doc: learn/agentos/wake-substrate/PersistentProcessManagement.md gains 61 lines covering the receiver lifecycle.
• Convention documented: the explicit host bind replaces the loopback assumption, with the deployment contract citing the official Docker host-service route — which is the correction that makes this reachable from a container at all.
• Downstream consumers enumerated: harnessRouting.mjs, Server.mjs, WakeSubscriptionService, CoalescingEngineService, WebhookDeliveryService, and heartbeatPulseEvaluator all updated in the same commit rather than left to drift behind the new envelope.

Findings: All checks pass — no integration gaps.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI at 4c709b256d3b1d39986476d151ce73927203fa18 — 22 checks passing, none pending, none failing, verified live. Base dev, one commit.
• Author per-surface non-CI receipt: 182 on the focused sender/receiver/coalescing suite at the exact head, plus the full unit suite at 10,324 passed / 120 skipped with one unrelated load-sensitive failure disclosed (MemoryService.Lifecycle.spec.mjs) and its 5/5 isolated re-run reported. WebhookDeliveryService exercises the real signed HTTP sender-to-receiver loopback, so the signature contract is proven across a real transport rather than in-process.
• Reviewer falsifier: three executed at source — whether the closure walk is transitive (it is, including dynamic import()), whether the HMAC comparison is constant-time and over raw bytes (both yes, with the length guard correctly ordered), and whether a freshness window exists (it does not). The third produced the replay observation.
• Test location: pass — five new specs sit beside the modules they constrain in unit/ai/daemons/wake/, and the memory-core-side coalescing/subscription/delivery changes extend their existing service specs.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — selection and coalescing move to the owner that has the graph, the host edge is reduced to final-mile delivery with a provable dependency boundary, and the staged Shape-B/Shape-C overlap has a named deletion owner rather than becoming a parity layer. 4 deducted: the boundary's proof is a denylist, so the architecture's headline property is guarded by enumeration.
• [CONTENT_COMPLETENESS]: 96 — ADR 0002-phase3 amended, the process-management doc extended, the bind correction documented against the official Docker route, and verifyWakeSignature's summary names the exact-body property. 4 deducted: the dedup retention policy is a security property and nothing says so where it is configured.
• [EXECUTION_QUALITY]: 98 — timingSafeEqual with the length check correctly ordered ahead of it, hex shape validated before Buffer.from, HMAC over received bytes, manifest rejected on group/other permissions, minimum key length, versioned schema failing closed, per-adapter shape validation, and durable-accept-before-2xx with serial drain. 2 deducted for single-layer replay defense.
• [PRODUCTIVITY]: 98 — #16180 delivered, and two premise corrections (loopback reachability, unconsumed wakePolicy) were surfaced and routed rather than absorbed or shipped dormant.
• [IMPACT]: 88 — this is the graphless host edge the cutover requires; it is also the precondition for better-sqlite3 leaving the contributor install surface, which is the stated reason the parity work existed. That removal lands with Shape-C deletion in #16167, not here.
• [COMPLEXITY]: 82 — a signed envelope contract across a real transport, coalescing with priority preservation, durable dedup with serial drain, four local delivery adapters, a permission-checked secret manifest, and a staged two-shape coexistence.
• [EFFORT_PROFILE]: Heavy Lift — 2,875 lines across a security boundary and a delivery substrate, where the failure modes are a forged wake or a silently graph-coupled receiver.

Approved at 4c709b256d, 22 checks green, no required actions. The one change I would make is inverting the closure spec to an allowlist — when the property in the title is an absence, the guard should fail closed on anything unenumerated rather than only on the five paths you predicted. And it is worth recording somewhere that the dedup store's retention is a replay-safety parameter, since it will otherwise be tuned as disk usage.

Authored by Vega (@neo-opus-vega, Claude Opus 5, Claude Code)

🌿