daemon(pipeline): hash emitter input/output into receipts#333
daemon(pipeline): hash emitter input/output into receipts#333ojongerius merged 4 commits intomainfrom
Conversation
Phase 1 of the daemon (#322) deliberately rejected emitter frames that populated `input` or `output` because the canonical-hash path was not yet wired through `receipt.Create`. Section 3 of the thin-emitter refactor cannot ship until those fields are accepted and committed to the receipt — every emitter is supposed to send real tool I/O. Lift the reject in `validateFrame`; in `buildAndSign`, hash a non-null `f.Input` into `Action.ParametersHash` via a new `canonicalSHA256` helper, and pass `f.Output` through `CreateInput.ResponseBody` so `receipt.Create` populates `Outcome.ResponseHash` along the same path. Both paths gate on `hasJSONPayload` so a literal JSON `null` (or omitted field) still means "no payload" — hashing the literal "null" would falsely commit the daemon to a value the emitter never sent. Tests cover (a) input/output hashing matches independent re-canonicalisation, (b) whitespace and key-order variants produce identical hashes (cross-language determinism), (c) primitive and array payloads are accepted, and (d) null input/output produces empty hash fields. Removed three cases from `TestProcess_RejectsMalformedFrames` that asserted the Phase 1 reject; the positive tests above replace that coverage. Closes #332. Unblocks #236 Section 3. Pre-existing flake noticed: several daemon integration tests fail on macOS hosts whose `t.TempDir()` produces socket paths longer than the 104-byte `AF_UNIX` `sun_path` limit (`/var/folders/...` is typically 119 chars). Reproduces on `main` without these changes; out of scope for this PR.
There was a problem hiding this comment.
Pull request overview
Enables Phase 2 prerequisite behavior in the daemon pipeline: accept emitter input/output payloads (including primitives/arrays) and commit them into receipts via canonical (RFC 8785) SHA-256 hashes (action.parameters_hash, outcome.response_hash) while treating JSON null/omitted as “no payload”.
Changes:
- Removes the Phase 1 validation hard-reject of non-null
input/outputframes. - Hashes
inputintoAction.ParametersHashand wiresoutputhashing through the receipt creation path. - Updates daemon docs and adds/adjusts pipeline tests for hashing, canonicalization invariants, primitives, and explicit
null.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
daemon/README.md |
Updates the documented emitter frame example and clarifies input/output hashing + null-as-absent behavior. |
daemon/internal/pipeline/build.go |
Accepts input/output, computes/stores parameters_hash, and attempts to populate response_hash from output. |
daemon/internal/pipeline/build_test.go |
Adds tests asserting correct hashing, canonical stability, primitive support, and null yields empty hashes. |
| @@ -256,6 +288,7 @@ func (p *Pipeline) buildAndSign( | |||
| PreviousReceiptHash: alloc.PrevHash, | |||
| ChainID: p.State.ChainID(), | |||
| }, | |||
| ResponseBody: responseBody, | |||
| }) | |||
There was a problem hiding this comment.
Good catch — verified: 1e400 is a syntactically valid JSON token (so it survives json.RawMessage storage on the initial EmitterFrame unmarshal) but errors when re-unmarshaled into any (overflows float64). Through receipt.CreateInput.ResponseBody that becomes a panic, and worse, pipeline.Process does not defer alloc.Rollback() — the panic also orphans the chain.State mutex, so even after a hypothetical recover the daemon would deadlock on the next frame.
Fixed in 0242e24: Output is now hashed via canonicalSHA256 directly (symmetric with Input), so buildAndSign returns a clean error → Process calls alloc.Rollback() → daemon stays up.
Added TestProcess_RejectsUnrepresentableNumbers covering 1e400 in both input and output positions; asserts no panic, returns error, chain not advanced.
(The lock-orphan-on-panic property in pipeline.Process is a separate hardening — defer alloc.Rollback() plus an idempotent rollback would close the door on any future panic source. Holding it as a follow-up unless you'd prefer it in this PR.)
| // raw is expected to already be valid JSON — json.Unmarshal into EmitterFrame | ||
| // would have failed earlier otherwise — so the unmarshal here cannot fail | ||
| // in practice. We still surface the error rather than panicking; a daemon | ||
| // crash on a malformed inbound frame would be the wrong failure mode even | ||
| // for an "impossible" path. |
There was a problem hiding this comment.
You're right — the comment overclaimed. json.RawMessage only validates the JSON token shape; numeric range/precision errors don't surface until re-unmarshal into a typed value. Reworded in 0242e24 to acknowledge that errors are real and expected on syntactically valid frames (with 1e400 as the canonical example), and that returning an error rather than panicking is the intended behavior — not just a defensive overhang.
Copilot caught a remote-DoS in PR #333: Output was routed through receipt.CreateInput.ResponseBody, which panics if json.Unmarshal or RFC 8785 canonicalisation fails. f.Output is emitter-controlled and can be syntactically valid JSON yet still fail re-unmarshal — `1e400` parses as a token but overflows float64. The panic crashed the daemon AND orphaned the chain.State mutex (Process did not defer Rollback), so even after a hypothetical recover the daemon would deadlock on the next frame. Stop using ResponseBody. Hash Output directly via canonicalSHA256 the same way Input is hashed; both paths surface unmarshal/canonicalise errors as buildAndSign error returns, which Process maps to alloc.Rollback() and a clean per-frame failure. Also reword canonicalSHA256's doc comment, which previously claimed the unmarshal "cannot fail in practice" — wrong on syntactically valid frames carrying out-of-range numbers. New test: TestProcess_RejectsUnrepresentableNumbers covers `1e400` in both input and output, asserts no panic and no chain advance. Addresses PR #333 review by copilot-pull-request-reviewer[bot].
| // canonicalSHA256 unmarshals raw, canonicalises per RFC 8785, and returns the | ||
| // "sha256:<hex>" digest. Callers must check hasJSONPayload first; passing a | ||
| // null/empty value canonicalises to "null" and produces a misleading hash. | ||
| // |
There was a problem hiding this comment.
Right, you're correct on both counts:
- Empty
RawMessage→json.Unmarshalon[]byte{}returnsio.EOF, never reachescanonicalize. (And the rewritten helper passesrawstraight toreceipt.Canonicalize, which surfaces that as acanonicalize: ...error rather than a misleading "null" hash.) - Literal
null→ would canonicalise to"null"and produce a hashable digest if it ever reached this function. Both cases are filtered byhasJSONPayloadupstream, so neither path actually fires today, but the doc shouldn't conflate them.
Reworded in 4952b58 to call out both shapes explicitly and to credit the upstream hasJSONPayload gate.
| // `1e400` is valid JSON syntax but overflows float64. The daemon MUST | ||
| // surface that as a per-frame error and keep running; a panic here would | ||
| // let any authenticated emitter DoS every other emitter on the same socket | ||
| // (and the orphaned chain.State allocation would leave the lock held). |
There was a problem hiding this comment.
Agreed — folding the hardening into this PR.
Done in 9ba38ed:
chain.Allocation.CommitandRollbackare now idempotent via a sharedsync.Once. Whichever runs first wins; subsequent calls are no-ops rather thanunlock of unlocked mutexpanics. Tests cover Rollback-after-Commit, Commit-after-Rollback, and double-Rollback.pipeline.Processdoesdefer alloc.Rollback()immediately afterAllocate. On the success path Commit runs first and the deferred Rollback no-ops. On any error or panic the deferred Rollback releases the mutex.
I considered adding recover() → error in Process but decided against it: silently swallowing panics would mask real bugs from the listener-side error log, and a panic is already a clear crash signal. The mutex no longer leaks, which was the actual DoS vector.
New regression test TestProcess_PanicReleasesChainAllocation injects a KeySource that panics on Sign, expects Process to panic, then asserts the next state.Allocate succeeds within 2s rather than hanging.
| func canonicalSHA256(raw json.RawMessage) (string, error) { | ||
| var v any | ||
| if err := json.Unmarshal(raw, &v); err != nil { | ||
| return "", fmt.Errorf("unmarshal: %w", err) | ||
| } | ||
| canonical, err := receipt.Canonicalize(v) | ||
| if err != nil { | ||
| return "", fmt.Errorf("canonicalize: %w", err) | ||
| } | ||
| return receipt.SHA256Hash(canonical), nil |
There was a problem hiding this comment.
Good catch on the redundant parse. Verified: json.RawMessage implements json.Marshaler and returns its bytes verbatim, so receipt.Canonicalize's internal marshalNoHTMLEscape → json.Unmarshal round-trip handles parsing without us needing to do an extra Unmarshal first.
Simplified in 4952b58 — canonicalSHA256 now passes raw straight into receipt.Canonicalize. One fewer unmarshal per hashed event in the hot path; same RFC 8785 behavior; same error shape (out-of-range numbers like 1e400 still surface as a per-frame error rather than panicking).
pipeline.Process held the chain.State mutex from Allocate until either Commit or Rollback. Any panic between those points (signer crash, store transient, future receipt-build refactor) would leak the lock and deadlock the daemon for every subsequent emitter on the same socket. Copilot called this out as the still-open class of DoS the prior output-hashing fix didn't fully close. Two pieces: 1. Make chain.Allocation.Commit and Rollback idempotent via a shared sync.Once on the allocation. Whichever runs first wins; subsequent calls are no-ops rather than "unlock of unlocked mutex" panics. Tests cover Rollback-after-Commit, Commit-after-Rollback, and double Rollback. 2. In pipeline.Process, `defer alloc.Rollback()` immediately after Allocate. On the success path Commit runs first and the deferred Rollback no-ops. On any error or panic the deferred Rollback releases the mutex. New regression test TestProcess_PanicReleasesChainAllocation injects a KeySource that panics on Sign, expects Process to panic, and asserts the next state.Allocate succeeds within 2s rather than hanging. Addresses PR #333 review by copilot-pull-request-reviewer[bot].
json.RawMessage implements json.Marshaler and returns its bytes verbatim, so receipt.Canonicalize already handles the parse via its internal marshal+unmarshal. The explicit json.Unmarshal here was a wasted parse on every hashed event in the daemon's hot path. Pass raw straight into Canonicalize. Same RFC 8785 behavior, same error shape (an out-of-range number like 1e400 still surfaces as an error instead of a panic), one fewer unmarshal per frame. Also tighten the doc comment per Copilot's other note: distinguish empty RawMessage (Canonicalize returns EOF, but hasJSONPayload filters it before we get here) from literal JSON null (canonicalises to "null" — also filtered upstream). The previous wording conflated the two. Addresses two PR #333 review threads from copilot-pull-request-reviewer[bot] on commit 0242e24.
| // Hash Output here rather than via receipt.CreateInput.ResponseBody | ||
| // (which panics on bad JSON). f.Output is emitter-controlled and may | ||
| // be syntactically valid JSON yet still fail re-unmarshal — see | ||
| // canonicalSHA256 doc — so the daemon MUST surface that as an error, | ||
| // not a crash. |
There was a problem hiding this comment.
Right, the PR description was stale — when I rewrote Output away from CreateInput.ResponseBody in 0242e24 to fix the DoS, I never updated the body. The implementation is the intended end state; description was wrong.
Updated the PR description to describe the actual symmetric canonicalSHA256 path for both Input and Output, plus an explicit note that ResponseBody was deliberately avoided because of the panic-on-bad-JSON behavior. Also folded in the panic-safety hardening that landed in 9ba38ed and rebuilt the Tests section to list the new regression tests.
What
Lift the Phase 1 hard-reject of non-null
input/outputin emitter frames. Inpipeline.buildAndSign, hash any non-nullf.Inputandf.Outputsymmetrically via a newcanonicalSHA256helper (receipt.Canonicalize+SHA256Hash); the digests land onAction.ParametersHashandOutcome.ResponseHashrespectively. Both gate onhasJSONPayloadso a literal JSONnull(or omitted field) still produces no hash — hashing the literal"null"would falsely commit the daemon to a value the emitter never sent.The implementation deliberately does not route Output through
receipt.CreateInput.ResponseBody: that helperpanics on JSON unmarshal/canonicalize failure, and emitter-controlled bytes can be syntactically valid (so they passEmitterFrameparse) yet fail re-unmarshal —1e400overflows float64. Routing Output through it would have given any authenticated emitter a remote DoS. Both hash paths now surface unmarshal/canonicalize errors asbuildAndSignreturns, which the caller maps toalloc.Rollback()and a clean per-frame failure.pipeline.Processis also hardened against panics from any source betweenAllocateandCommit:chain.Allocation.Commit/Rollbackare idempotent viasync.Once, andProcessdoesdefer alloc.Rollback()immediately afterAllocate. Any panic now releases the chain mutex instead of orphaning it (the orphan would have deadlocked the daemon for every subsequent emitter).The receipt format is unchanged: this enables the existing
action.parameters_hashandoutcome.response_hashfields. No SDK hashing path was touched.Why
Phase 1 of the daemon (#322) deliberately rejected
input/outputbecause the canonical-hash path was not yet wired throughreceipt.Create. Section 3 of the daemon-process-separation rollout (#236) refactors mcp-proxy and the three SDKs into thin emitters that send real tool I/O. Per OQ3 (ADR-0010 amendment 2026-05-06) those five modules ship in one release — none of them can ship until the daemon accepts what they need to send.Closes #332.
Tests
Added in
daemon/internal/pipeline/build_test.go:TestProcess_HashesInput/TestProcess_HashesOutput— assert the resulting receipts' hashes match independent re-canonicalization of the original payload (no shortcut through the daemon's internals).TestProcess_HashesAreCanonical— same logical payload sent two different ways (key order, whitespace) MUST produce identical hashes. Guards the cross-language determinism property.TestProcess_AcceptsPrimitiveInputOutput— primitives (string) and arrays are accepted and hashed; not all tool I/O is a JSON object.TestProcess_RejectsUnrepresentableNumbers— emitter-controlled1e400(syntactically valid JSON, fails re-unmarshal into float64) returns a per-frame error, no panic, no chain advance.TestProcess_PanicReleasesChainAllocation— injects aKeySourcethat panics onSign, expectsProcessto panic, then asserts the nextstate.Allocatereturns within 2s rather than hanging. Guards future panic sources.TestProcess_AcceptsExplicitNullInputOutputto assert thatnullproduces empty hash fields (regression-safety on the documented null-as-absent behavior).TestProcess_RejectsMalformedFramesthat asserted the Phase 1 reject — the positive tests above replace that coverage.Added in
daemon/internal/chain/state_test.go:TestRollbackAfterCommitIsNoOp/TestCommitAfterRollbackIsNoOp/TestDoubleRollbackIsNoOp— pin thesync.Once-guarded idempotency that pipeline's deferredRollbackrelies on.Checklist
daemon/internal/pipelineanddaemon/internal/chaingreen with-race; full unit suite passes; cross-sdk-tests integration suite passesgo vet ./...)cross-sdk-testsgo test -tags=integration✓; no SDK hashing or canonicalization logic changed, daemon usessdk/go/receipt.Canonicalize/SHA256Hashunchanged)parameters_hash/response_hashfields documented in spec/)Pre-existing flake noticed (out of scope)
Five integration tests in
daemon/integration_test.goand one indaemon/internal/socket/listener_test.gofail on macOS hosts whoset.TempDir()produces socket paths longer than the 104-byteAF_UNIXsun_pathlimit (/var/folders/.../events.sockis typically ~119 chars). Reproduces onmainwithout these changes; passes on GitHub's macOS runner (shorter$TMPDIR). Worth a short follow-up to use a shorter path or honor a test override; not appropriate to fix in this PR's scope.