Skip to content

intake: one bounded read and one scan of every hook payload - #61

Open
k4rthiknerella wants to merge 17 commits into
record/group-commitfrom
record/intake
Open

k4rthiknerella wants to merge 17 commits into
record/group-commitfrom
record/intake

Conversation

@k4rthiknerella

@k4rthiknerella k4rthiknerella commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #60 (store: group commit, one full sync per hook process); review and merge after it.

Every hook entry read its payload with io.ReadAll and decoded it into Go values: about eight copies of an attacker-influenced payload that can be 8 MiB, and a peak RSS of 56 MiB at the cap. This replaces that with one bounded read, one json.Valid pass and one walk of the top level that records where each field is rather than decoding it (RES-4, RES-5), reads large fields through fixed buffers (RES-6) and streams the Stop payload instead of holding it (RES-7). It also switches on the 1, 4 and 8 MiB rss cases, and records each case's measured figure beside a line that holds on the reference machine with no tolerance (RES-28).

The branch carries the contracts PR's commit (contracts: the types and stubs the 1.2.0 tracks build against) at its base because this change needs it. Review the commits above it: the intake change, the perf and H-17 follow-ups, and the schema commit. The contracts commit drops out when that PR merges.

What changed

  • internal/hookio (new): Read takes a 4 KiB first chunk, then chunks that grow to an eighth of what has been read, capped at 64 KiB, then makes one exact-size copy. It reads at most MaxPayloadBytes+1 bytes, accepts exactly the cap and refuses one byte more with ErrTooLarge. From 1 MiB up it collects its chunks before returning, so the allocations after it reuse those pages (hook.1MiB: 10.09 MiB without, 9.59 with). Intake validates with json.Valid, then walks the top level once and records the spans of 22 keys, in a frozen order (TestPayloadKeyOrderIsFrozen; later PRs append and never reorder). Keys are matched as the old payload structs matched their fields: exactly or case-insensitively, the last match winning. Payload.Str, Bool, Int and Strs decode the small fields the way the struct decode did: null and absent read as empty or nil, and any other type fails the whole intake. Strs takes Into(key, &dst) pairs, so a destination cannot drift from its key; it returns on the first field of the wrong type, and every caller discards what it wrote on that error. A payload over the cap is refused with a *TooLargeError (errors.Is(err, ErrTooLarge) holds) that carries the session_id and cwd named in its first 4 KiB, which were read before the cap was reached, so the refusal is recorded in the run that sent it. IntakeStream validates and walks a payload in one pass and never holds it. It refuses a root that is not an object or null on its first byte, before anything is parsed, and returns ErrStopped when the callback ends the walk early. It has no cap of its own; its caller bounds the reader.
  • internal/jsonscan (new, standard library only, nothing built at init): Top and Field walk a validated buffer and record byte ranges. The last duplicate key wins, as in encoding/json, and a top-level null leaves every key absent. Head, Tail and Lines unescape a string span through fixed buffers and never split a character. Stream and StreamOne validate as they go through a window that starts at 4 KiB and grows to 64 KiB. They copy out only the values at the wanted paths, as a head, a tail, both, a presence bit or lines. A string is passed unescaped, any other value as its JSON text, and null is not passed at all. Lookup and ValueSpan serve shape's single-field reads. NewKeySet matches keys exactly after unescaping, as a map decode does; NewFoldKeySet and Path.Fold match them as a struct field matches its name, under Unicode simple folding. Field, Lookup and ValueSpan document their precondition: a buffer json.Valid accepted. They do not validate.
  • internal/hook: hook, post, probe and paused fill their fields from the spans Intake recorded (Payload.from, PostPayload.from, sessionPayload.from), and an over-cap payload's refusal goes to the session its head names (refusedSession), or to unattributed when it names none. tool_response is still never a value: backgroundedCall reads two presence bits off spans, in any case, as its old struct did. tool_input and tool_response are walked again for the few keys read from them; nothing decodes the payload. The failure message is read to 8 KiB, because the exit code is on its first line. closeReason maps a capture error to its coverage reason, payload_too_large among them. scratchpad_dir was declared and never read, and it is not among the scanned keys.
  • internal/shape: command, run_in_background and the host fields are read as spans (inputField, stringValue). run_in_background was a struct field and is matched in any case; command and the host fields came from a map decode and stay exact. The package doc states the validated-input precondition its readers rely on. canonical keeps its decode into any for digest compatibility until RES-48, and that is the one decode a hook path may still make. It now streams json.Marshal's bytes into the HMAC through a 4 KiB buffer instead of building them (canonEncoder: sorted keys, HTML-safe escapes, ES6 number formatting, and U+2028/9 and invalid UTF-8 written as json.Marshal writes them). FuzzCanonicalMatchesMarshal holds the stream to json.Marshal byte for byte, and TestCanonicalDigestIsUnchanged pins a digest to the value the Marshal-based form gave.
  • cmd/rashomon/intake.go: recap and digest --stdin stream the Stop payload through IntakeStream, reading at most MaxPayloadBytes+1 bytes through io.LimitReader, so a stdin that never ends is refused rather than read forever. The final message is captured as a 256 KiB head and a 64 KiB tail. The account reads 64 Ki runes, which UTF-8 holds in at most 256 KiB, so the head is exactly what the whole decode gave it. The tail is for the comparator (RES-17), and nothing on this base reads it. A payload over 8 MiB is still no payload, as before. Keys are matched in any case, session_id is read from its last copy, and a copy over 4 KiB names no session rather than failing the payload.
  • internal/store: ReasonPayloadTooLarge, with its sentence in the report's reason text. docs/store-schema.json gains payload_too_large on coverage.reason in a separate commit, together with flush_failed on terminal.reason from the group-commit PR.
  • Budgets: the 1, 4 and 8 MiB rss cases gate from this PR. Every case carries measured_mib, the median of nine runs on the reference machine when its line was set. LineHolds refuses a missing figure, and any figure over the line, with no tolerance (TestRES28h_EveryLineHoldsOnTheReferenceMachine, TestRES28h_LineHoldsTakesNoTolerance). Until RES-48 the hook cases carry 3.4 MiB per MiB of payload and the post cases 2.45 (RES-4's 2.0x read plus canonical's decoded copy of tool_input), with the basis on each row. RES28BudgetMiB takes the slope. New benchmark rows cover Top, Lines, Stream and Read. Each row this change moves is restated at what it now measures, with max equal to measured.
  • Mutants re-anchored to the new code: H-14 (2), H-20 (2), TB (2), RES-7 (1, the stream's key match) and RES-4 (1, read's three results). Mutants re-anchored to the restated rows: DEL-7 (1), RES-36 (2).
  • TestTopAllocatesNothingPerValue measures the walk through Field. Under -race, json.Valid's sync.Pool drops values at random by design, so Top as a whole is asserted allocation-free only without -race. Linux CI had failed it on that pool, not on the walk.
  • RES-7's growth bound is 6 MiB under -race and stays 4 MiB without it. Linux CI on amd64 measured up to 4.32 MiB of race-mode growth; a recap that holds the 7 MB payload measured 86 MiB.
  • hookio.Read is also tested under iotest.OneByteReader and a reader that errors partway through, so its io.ReadFull loop is checked against both a one-byte-at-a-time source and a mid-stream error. readFinalMessage's own cap (separate from Read's: recap streams through IntakeStream, which never calls Read) is tested at exactly MaxPayloadBytes and one byte over. Post.Capture and RunProbe get the same over-cap test Handler already had, since all three share hookio.Read's cap and closeReason's mapping to payload_too_large.
  • The package comment on internal/hookio said large fields are read through Head, Tail and Lines; only Head has a caller yet, and the comment now says so.

Decisions

  • Payload keys are matched as the old structs matched them, in any case, so {"SESSION_ID":"s"} still attributes the call to s. FuzzRES5_IntakeMatchesTheStructDecode, FuzzRES5_ToolInputFieldsMatchTheirOldDecode and FuzzRES7_FinalMessageMatchesTheStructDecode hold every field each entry reads to the struct or map it was decoded from.
  • A repeated key is read from its last copy on the buffered path. The struct decode kept an earlier string under a later null, failed on any copy of the wrong type and merged repeated objects; Claude Code repeats no key, and the fuzz targets leave repeated keys out. The streamed path already matches the struct decode here, since it skips null and checks every copy's type.
  • An over-long session_id (over 4 KiB) on the Stop payload names no session: the message is still read, and recap falls back to the newest run, its rule for a payload with no session_id. Technical spec 1.2.11 buckets an over-cap session_id under unattributed for records; recap writes none.
  • The 64 KiB tail is still captured: Technical spec 8.2.2 has it captured beside the head until the comparator lands. Its ring grows by append to the value's own size, so a short message costs only its own bytes.
  • Chunk growth is an eighth of what has been read, between 4 KiB and 64 KiB. Fixed 64 KiB chunks cost 2.7x at 40 KB, outside RES-4's 2.2x.
  • Recap's head is 256 KiB, not 64 KiB, because a 64 KiB head would shrink the account for non-ASCII text.
  • Path.String makes a non-string at a path stop the stream with ErrNotString. That keeps the old decode's type failure for session_id and last_assistant_message.
  • RES-5's scan (TestRES5_PayloadScannedOnce, an AST walk) treats the whole of cmd/rashomon as hook path. Any json.Unmarshal or json.NewDecoder there fails it unless it is allowlisted by function name with its reason. A future non-hook command that decodes JSON does so in a package outside the list.
  • BenchmarkTop measures the KeySet built per call plus the walk, after a warm-up call. Top alone allocates nothing per value, and the gate refuses a row of zero as a placeholder. Without the warm-up, json.Valid's pooled scanner refilled after a GC and moved the row between 0 and 6,652 B/op.

Measured

All figures are from this tree on the reference machine (Apple M3, go1.27.2, release flags for the RSS runs).

hookio.Read allocation per call (RES-4, TestRES4_ReadAllocBound):

Payload Allocated Ratio Allocations
400 B 4,584 B 11.46x (the bound at 400 B is 8 KiB, not a ratio) 4
40 KB 83,456 B 2.09x 16
1 MiB 2,181,536 B 2.08x 48
4 MiB 8,459,136 B 2.02x 97
8 MiB 16,823,168 B 2.01x 161

Peak RSS, median of nine, against each case's line (RES-28, TestRES28_MaxRSSFollowsThePayloadBudget: pass):

Case Measured MiB Line MiB Before this change
hook.400B 6.14 6.5008
hook.40KB 6.17 6.5839
hook.1MiB 9.52 9.90 (slope 3.4) 11.84
hook.4MiB 19.75 20.10 (slope 3.4) 27.59
hook.8MiB 31.81 33.70 (slope 3.4) 47.89
post.540B 5.98 6.5011
post.40KB 6.16 6.5839
post.1MiB 8.94 8.95 (slope 2.45) 13.28
post.4MiB 15.48 16.30 (slope 2.45) 32.97
post.8MiB 23.58 26.10 (slope 2.45) 55.80

Every case holds its line with no tolerance applied. The platform tolerance (15% on this runner until 2026-11-06, then 10%) covers other machines and run-to-run noise. The measured_mib on each row is this run's median.

Entry rows (TestRES28b_EveryEntryStaysWithinItsRow: pass):

Entry row Measured MiB Row p50
hook.bash_short 6.02 6.50
hook.edit_50k 6.22 6.61
post.bash_short 6.11 6.50
post.edit_314k 6.81 8.00
recap.stop 6.92 12.00
recap.pending 4.81 6.50
probe.start 6.00 8.00
probe.end 6.03 6.00

probe.end is 0.03 MiB over its row, which is inside the tolerance. That row is derived, not measured, and this PR moves probe onto Intake.

TestRES2_InitBudget: pass, with 42 packages, 32,360 bytes, 211 allocations and 0 GC lines.

Lines over a 4 MiB stdout of 145,398 lines, with the summary on the last line, allocates 24 bytes (TestRES6_LargeFieldScanIsConstantMemory). Recap's peak RSS at a 7 MB final message is measured by TestRES7_HugeFinalMessageIsConstant, which bounds it at 12 MiB and at no more than 4 MiB above a 2 KiB message. The 12 MiB bound is not applied under the race detector. The 4 MiB bound is.

Benchmark rows restated (best of three at 20 iterations; max equals measured)
Benchmark Before (B/op / allocs/op) Now
BenchmarkDerive/bash_short 5,386 / 77 4,624 / 71
BenchmarkDerive/bash_long_4k 428,860 / 2,333 430,688 / 2,333
BenchmarkDerive/edit_50k 116,032 / 38 63,464 / 28
BenchmarkLexTokens 128,544 / 750 128,544 / 750
BenchmarkRunHook/bash_short 49,394 / 352 53,828 / 361
BenchmarkRunHook/edit_50k 429,464 / 308 218,442 / 311
BenchmarkRunPost/bash_short 42,909 / 280 47,638 / 298
BenchmarkRunPost/edit_314k 1,172,882 / 284 893,014 / 298
BenchmarkRecap/10 238,319 / 534 262,910 / 553
BenchmarkRecap/100 1,047,751 / 3,822 1,072,590 / 3,843
BenchmarkRecap/1000 9,456,658 / 35,328 9,481,832 / 35,350
BenchmarkRecapPending/10 237,619 / 537 261,194 / 553
BenchmarkRecapPending/1000 9,457,111 / 35,339 9,480,470 / 35,351
BenchmarkTop (new) 616 / 6
BenchmarkLines/4MiB (new) 24 / 2
BenchmarkStream/64MiB_batch (new) 158,322 / 17
BenchmarkStream/187MB_line (new) 349,379 / 30
BenchmarkRead/400B (new) 4,584 / 4
BenchmarkRead/40KB (new) 83,456 / 16
BenchmarkRead/1MiB (new) 2,163,584 / 48
BenchmarkRead/4MiB (new) 8,459,136 / 97
BenchmarkRead/8MiB (new) 16,823,173 / 161

Where a row rose:

  • The small-payload hook and post rows carry about 4 KB more. That is the 4 KiB first chunk, which replaces io.ReadAll's 512-byte start and doubling, plus the KeySet built per call.
  • The recap rows carry about 24 KB more per call: the streamer's 4 KiB window, its nesting table, and the head and tail capture buffers, which grow by doubling to the message's size. This replaces the decode of the whole payload. For a 2 KiB Stop payload that costs more than the decode did. For a 7 MB one it is about 50 MiB less (RES-7).

The large-payload rows fall: edit_50k by half, edit_314k by a quarter.

Limits and what is still to run

Not in this PR

  • The event entries (PostToolBatch, SubagentStop, PostCompact) do not exist on this base. Here, IntakeStream serves recap's Stop and catch-up. A later PR moves those entries onto it, with per-path caps.
  • RES-12 (no whole-run read on a hook path).
  • RES-48, the streaming canonicaliser. It retires canonical's decode and returns the hook and post rss lines to 2.2 MiB per MiB.
  • The token, leading-cd and nesting bounds, runtime settings and the init tests.
  • A reader for the 64 KiB tail of the final message. The tail is captured for the comparator, and nothing on this base reads it.
  • An rss case for post with a large tool_input. The hook cases already gate canonical's decode of a tool_input that carries the whole payload (Edit payloads, slope 3.4); the post cases pad tool_response, so the same decode on the post path has no row of its own until RES-48 removes it.
  • A production caller for jsonscan.Lines and jsonscan.Tail. Both are staged for later PRs in this stack; Head is the only one of the three a consumer calls today.

What could go wrong

  • canonEncoder must stay byte-identical to json.Marshal, or every non-shell digest moves. The fuzz target and the pinned digest guard it, and RES-48 retires it.
  • Read forces one GC from 1 MiB up, which holds hook.1MiB under its 9.90 MiB line (9.59 with it, 10.09 without). BenchmarkRead puts its cost at about 0.1 ms at 1 and 4 MiB, against the hook rows' 10 to 15 ms p50.
  • A fold KeySet compares a key that misses exactly against every wanted key with EqualFold: 22 short comparisons at most, per top-level key.
  • An over-cap payload is attributed from its first 4 KiB only: a session_id repeated past the head is not seen, and a head that is not a sound start of one object names nothing.
  • Stream delivers CaptureLines pieces before it meets a syntax error. A caller discards them on error, as readFinalMessage does.

Still to run before review

  • go test ./... -race -count=1: ran, and was red on internal/jsonscan's zero-allocation check for Top -- json.Valid's scanner comes from a sync.Pool the race detector drops, so the pool is never warm under -race. Fixed by measuring the walk through Field instead and checking Top as a whole only without -race, where the pool stays warm (jsonscan: measure the walk, not Valid's own pool, for zero allocation and jsonscan: keep Top's zero-allocation check where the pool is deterministic). After the commits that match keys in any case, attribute a refusal and bound recap's read, go test -race -count=1 reports ok for internal/jsonscan, internal/hookio, internal/hook, internal/shape, cmd/..., test/acceptance and test/mutation, and go test ./... -count=1 reports ok for every package.
  • CI on hookio: say which fields Strs may have already written: Perf gates (linux) was red, the only red job there: about 9.5 MiB measured against roughly 6 MiB darwin rows. Mostly inherited from perf: budgets.json and the resource gates #59, which never recorded a linux baseline, so linux still gates against the M3 rows; this PR adds its own share by switching on three P-10 cases (hook.1MiB, post.1MiB, post.4MiB) that have the same gap.
  • The eight entry rows are moved. test/perf/budgets.json now carries this tree's measured medians: hook.bash_short 5.95, hook.edit_50k 6.20, post.bash_short 5.94, post.edit_314k 6.73 (p95 moved to RES-28's line, 7.16, in place of 8 MiB), recap.stop 7.06, recap.pending 6.06, probe.start 5.97, probe.end 5.98, all MiB. recap.pending no longer reads the no-store early return (4.81 MiB); the seeded rig gives 6.06.
  • The Linux recap memory measurement (TestRES7_HugeFinalMessageIsConstant, through the reaper): measured 2026-10-13, linux/arm64, go1.25.8 -race, an instrumented child's floor before recap does anything is 20.0 MiB at a 2 KiB message and 22.9 MiB at a 7 MB one (4.4 and 5.5 MiB without -race). Under the race detector the floor is already over the 12 MiB absolute bound, so that bound is skipped for an instrumented child and only the growth bound (no more than 4 MiB above the 2 KiB case) applies.
  • Why the reaper and its child-running test helpers may import os/exec and syscall while hook-path code may not: TestH17_TestPackagesStayTestOnly now scopes the import ban to the packages a shipped binary can actually link — every package outside test/, plus any test/ package a non-test package imports. test/perf/peakrss and test/perf/peakrss/reaper are imported only from _test.go files, so they fall outside that scope and may use os/exec and syscall to run and reap the child processes RES-7 and the RSS cases spawn.
  • Fuzz targets that exist on this tree: FuzzRES5_TopMatchesStdlib and FuzzRES7_StreamMatchesStdlib (internal/jsonscan), FuzzCanonicalMatchesMarshal and FuzzRES5_ToolInputFieldsMatchTheirOldDecode (internal/shape), FuzzRES5_IntakeMatchesTheStructDecode (internal/hook) and FuzzRES7_FinalMessageMatchesTheStructDecode (cmd/rashomon). The three struct-decode targets ran clean for 60 s, 45 s and 30 s (hook, recap, shape). No log on this tree shows a 120 s run of any target; that run is still open.
  • The 12 mutants in RES-4_refusal-head.py, RES-5_fold-keys.py and RES-7_final-message-bound.py, and the two moved anchors in RES-4_read-bound.py and RES-7_stream.py, were each applied alone and turned their named tests red. The full mutation sweep over the RES-4, RES-5, RES-6, RES-7 and RES-28 mutant files has not been run on this tree. It no longer runs on every push or pull request — it is now a required, by-hand step (Actions > CI > Run workflow) before a release is tagged, or whenever a change adds or moves mutants, run locally with python3 test/mutation/sweep.py. Still open for this PR.
  • The full benchmark gate: not run on this tree; still open.
Mutants and the test that must catch each (28)
Mutant Test
RES-4 the payload is read with io.ReadAll's doubling TestRES4_ReadAllocBound
RES-4 a payload of exactly the cap is refused TestRES4_CapAtNAndNPlusOne
RES-4 the refusal is recorded as an internal error TestRES4_CapAtNAndNPlusOne
RES-4 a refusal drops the head that names its session TestRES4_TooLargeNamesTheSessionInItsHead, TestRES4_CapAtNAndNPlusOne
RES-4 a head cut short of its end still names a session TestRES4_TooLargeNamesTheSessionInItsHead
RES-4 a refusal is not ErrTooLarge TestRES4_CapAtNAndNPlusOne
RES-5 post decodes the payload again for the background keys TestRES5_PayloadScannedOnce
RES-5 Top walks without json.Valid's verdict TestRES5_TopDefersToJSONValid
RES-5 a fold KeySet matches only exactly TestRES5_FoldKeySetMatchesStructFieldNames
RES-5 a streamed path ignores Fold TestRES5_StreamFoldPathMatchesStructFieldNames
RES-5 intake matches payload keys exactly TestRES5_IntakeMatchesStructFieldNames
RES-5 run_in_background is matched exactly TestRES5_RunInBackgroundMatchesItsOldField
RES-5 the background keys are matched exactly TestRES5_BackgroundKeysMatchTheirOldFields
RES-6 Lines unescapes the whole span into one slice TestRES6_LargeFieldScanIsConstantMemory
RES-7 recap reads the Stop payload whole through Intake TestRES7_HugeFinalMessageIsConstant
RES-7 Stream copies a value at a key nobody asked for (re-anchored) TestRES7_StreamCopiesOnlyWantedPaths
RES-7 the final message is counted, not limited TestRES7_FinalMessageReadStopsAtTheCap
RES-7 a top-level string passes the root check TestRES7_IntakeStreamRejectsANonObjectRoot, TestRES7_FinalMessageRefusesATopLevelString
RES-7 an over-long session_id fails the payload TestRES7_FinalMessageSessionIDLastCopyWins
RES-7 a stopped walk reads as a whole payload TestRES7_IntakeStreamReportsAnEarlyStop
RES-28 a line is padded with the tolerance before the measurement is compared TestRES28h_LineHoldsTakesNoTolerance
RES-28 a case with no measured figure beside its line passes TestRES28h_LineHoldsTakesNoTolerance
H-14 shell digest covers the whole tool_input again (re-anchored) TestH14_ShellDigestCoversTheCommandAlone
H-14 non-shell digest covers only the tool name (re-anchored) TestH14_NonShellDigestCoversTheWholeInput
H-20 the post payload declares tool_response, and it reaches the debug log (re-anchored) TestH20_ToolResponseNeverReachesDisk
H-20 the response is measured, and the measurement moves the record's width (re-anchored) TestH20_ExecutionWidthIsIndependentOfTheResponse
TB the post path never reads the background keys (re-anchored) TestTestBending_ARunMovedToTheBackground, TestH20_Backgrounded
TB a false backgroundedByUser sets the bit (re-anchored) TestH20_BackgroundedIsReadForBashOnly

Rows closed

  • RES-4: TestRES4_CapAtNAndNPlusOne, TestRES4_ReadAllocBound, TestRES4_TooLargeNamesTheSessionInItsHead
  • RES-5: TestRES5_PayloadScannedOnce, TestRES5_TopDefersToJSONValid, FuzzRES5_TopMatchesStdlib, FuzzRES5_IntakeMatchesTheStructDecode
  • RES-6: TestRES6_LargeFieldScanIsConstantMemory
  • RES-7: TestRES7_HugeFinalMessageIsConstant, FuzzRES7_StreamMatchesStdlib, TestRES7_FinalMessageReadStopsAtTheCap, TestRES7_IntakeStreamRejectsANonObjectRoot
  • RES-28: TestRES28h_EveryLineHoldsOnTheReferenceMachine, TestRES28h_LineHoldsTakesNoTolerance

@lutic1 lutic1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k4rthiknerella, the intake logic is correct. The bound and its off-by-one are right (8 MiB accepted, 8 MiB+1 refused), the scanner matches the old decoder, and all three fuzz targets ran for 45 s clean. CI is red, though, and the description doesn't say so. Please fix:

Blocking

  1. internal/jsonscan/jsonscan_test.go:90-99 TestTopAllocatesNothingPerValue fails under -race, which is how CI runs it ("Top allocated 1.0 times per run, want 0"). It failed 4 of 6 runs locally and failed "Test" on run 38610392847. json.Valid gets its scanner from a sync.Pool, and the race detector drops pooled items. Skip the test under race (cmd already has race_on_test.go/race_off_test.go), or measure only walkObject. Every PR stacked on this one (#62–#66) inherits the failure.

Should fix
2. The description says go test ./... -race -count=1 passed for every package; CI says otherwise. "Perf gates (linux)" is also red (about 9.5 MiB against roughly 6 MiB darwin rows). That's inherited from #59, but this PR tightens those rows. Please state both.
3. Missing tests:

  • hookio.Read with short readers (iotest.OneByteReader) and a mid-stream error (ErrReader).
  • readFinalMessage at N and N+1. Its cap at cmd/rashomon/intake.go:73 is now separate from Read's.
  • payload_too_large for post and probe, not just Handler.

Nits
4. jsonscan.Lines/Tail have no production caller yet. Say they're staged for later PRs, or drop them.
5. internal/report/text.go:239: "refused it unread" is inaccurate, since the payload is read; parsing is what's skipped. Use "refused it without parsing".
6. hookio/payload.go:213-216: the Strs comment says it fails "before any is used", but it writes earlier fields into dst first.


Generated by Claude Code

@lutic1 lutic1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k4rthiknerella, this is a follow-up to the earlier review: an adversarial pass over this PR. Please fix each item, or reply on the PR saying why it doesn't apply. The earlier -race blocker still stands.

Regressions from the old decoder

  • cmd/rashomon/intake.go:56-76 readFinalMessage no longer limits how much it reads: it consumes the whole of stdin and only then checks cr.n > MaxPayloadBytes. The old code stopped at Max+1 through io.LimitReader. digest --stdin can now hang forever on an endless stream, and recap's goroutine keeps scanning after its 3 s give-up. Wrap the reader in io.LimitReader(in, MaxPayloadBytes+1).

  • hookio/stream.go:18-34: the root-object check uses the first decoded byte, so a top-level string passes as an object. The reviewer confirmed that "", "{" and "{\"x\":1}" all return ok=true, where the old struct decode rejected them. Record the root value's kind, and add these inputs to the bad-input table.

  • Key matching is now exact-case, but encoding/json matched struct fields case-insensitively, with the last key winning. Confirmed:

    • {"Last_Assistant_Message":"hi","SESSION_ID":"s"} now gives an empty message and an empty session.
    • With duplicate case-folded keys, a different value wins than before.
    • shape.backgrounded no longer matches Run_In_Background.

    The RES-5 fuzz oracle compares against map[string]any, so it can't see this. Implement struct-decode semantics, or document the change, and fuzz against the old struct types.

  • intake.go:48,61-62,73: a truncated session_id (over 4 KiB) rejects the whole payload, and a repeated session_id keeps long set even when the last copy is short. The old decode accepted both. Make the last occurrence win, and decide and test what an over-long id should do.

Accuracy of the "one scan" claim

  • Top runs a full json.Valid pass and then a walk; backgroundedCall walks tool_response again; and shape runs a separate Lookup, building a fresh KeySet, for each field. Build the key sets once at package level, scan tool_input once, and correct the "scanned once" wording.
  • The non-shell path (Write/Edit) still decodes all of tool_input with json.Unmarshal into any (exempted in res5Allowed), so an 8 MiB Write is fully amplified in memory. Add an RSS gate for that path, and document the remaining amplification.

Robustness

  • hookio/read.go:69-75 forces a synchronous runtime.GC() for every payload of 1 MiB or more, on the hook's critical path, justified by a single RSS measurement. Use GOMEMLIMIT/SetGCPercent instead, or gate it on the remaining time budget.
  • Payload.Strs pairs []*string with variadic keys by position. A length mismatch panics, which means exit 2 and a blocked tool call, and a reordering silently swaps fields. Take pairs instead, or check the lengths.
  • shape.Derive/LeadingDirectory now rely on input that hookio.Intake has already validated, because Lookup/Field never validate. Garbage after the object is accepted. Document the precondition, or require a validated type.
  • IntakeStream: when fn stops early, the root check is skipped and the rest is never validated. Return a sentinel, or validate the rest.
  • An over-cap payload's coverage record always goes to "unattributed", even though session_id is in the first chunk already read. Stream the heads of session_id and cwd so the record lands in its real run.
  • The 64 KiB LastMessageTail ring is captured on every Stop, but nothing reads it. Drop it until RES-17/P-40 lands.
  • report/text.go:239 "refused it unread" is inaccurate: the payload is read up to Max+1.

Missing tests

  • Top-level string payloads, case-variant keys, an over-long or duplicated session_id.
  • readFinalMessage on a reader producing 4×Max bytes, asserting it reads at most Max+1.
  • A Post.Capture/Close N+1 case asserting payload_too_large.

Generated by Claude Code

k4rthiknerella added a commit that referenced this pull request Oct 17, 2026
Three byte tests stood in front of a full decode and disagreed with it.

spend's main-transcript prefilter skipped, once the file was dated, every
line without the exact bytes "usage". encoding/json matches the key in any
case under Unicode folding ("Usage", "uſage") and through escapes
("us\u0061ge"), so such a usage line was dropped without being counted,
priced or unparsed. mayCarryUsage now also keeps a line holding a \u escape
or a quoted five- or six-byte word that folds to "usage". Not every
backslash, which the rubric rejected: nearly every real line has one.
Unparsed stays tied to the exact marker.

report's turn reader skipped every wanted assistant line without the bytes
"text", which dropped a reply whose content is a plain string, a shape the
reader's own decoder handles. FinalAssistantTexts and FinalAssistantText then
disagreed on one file. The header now records whether message.content is a
plain string (contentShape, a bool and nothing of the text), and mayHoldText
keeps a line that is plain, holds "text", or holds a \u escape. Dropping the
prefilter outright was measured first: about 2.9x slower on a turn of 1,000
tool_use lines with 10 KB inputs (33 ms against 11.5 ms); the widened test
costs nothing measurable.

topLevelSidechain took the first isSidechain key spelt exactly, while a
struct decode takes the last key matching under folding and leaves the value
alone on null. The walk now applies the struct decode's rule, reading the
key's value as its literal only.

FuzzUsagePrefilter, FuzzTextPrefilter and FuzzSidechainKey check each test
against the decode it stands in front of (45 s each locally, no failures);
TestRubricPERF3 and TestRubricTEST14 run the seeds untagged for the sweep.
The rubric's fourth sub-item, FuzzPresent for post.go's present type, is moot:
#61 replaced that type with jsonscan.Field spans (internal/hook/post.go:218),
which FuzzRES5_TopMatchesStdlib checks against encoding/json.

Rubric perf-3, test-14. Mutants: RUBRIC_perf-3_usage-prefilter.py,
RUBRIC_test-14_transcript-prefilters.py; two SP_join.py anchors re-anchored
on the renamed skip variable.
k4rthiknerella added a commit that referenced this pull request Oct 17, 2026
Three enums used their zero value as a meaningful member, so a value nobody
set read as a decision. report.LineKind's zero was LineBlank: a reader that
forgot to take DecodeHeader's answer skipped every line as blank.
budget.Phase's zero was Verify (#58): an unset phase would stand down as
verify_budget. jsonscan.CaptureMode's zero was CaptureHead (#61): a Path
whose Mode was forgotten copied the value's head out unasked.

Each now reserves its zero for a neutral member. LineKind's is a blank
constant DecodeHeader never returns. Phase's is the unexported phaseUnset,
which has no stand-down word (the empty string no writer may record) and
which Phases does not list; the thirteen exported phases, their order and
their frozen reasons are unchanged, and TestPhaseReasonsAreTheFrozenList
still passes. CaptureMode's is a blank constant that Stream's existing mode
switch refuses with ErrPaths; the one test that relied on the old default
now names CaptureHead, as every production Path already did.

Rubric naming-12. Mutant: RUBRIC_naming-12_neutral-zero.py.
k4rthiknerella added a commit that referenced this pull request Oct 17, 2026
…uzz in CI

Nothing in the tree would find a crash in the parsers that read
model-written bytes. In a hook a recovered panic costs the call's
declaration, and a runtime fatal error such as stack exhaustion exits 2
and blocks the user's tool call. #61 added fuzz targets for the payload
scan and the canonical digest; the shell lexer and the transcript readers
still ran only on curated tables.

FuzzDerive, FuzzHosts, FuzzLabel and FuzzLeadingDirectory are seeded with
the leak corpus, the program claims and the comsub nesting bound passed by
one in quoted, unquoted and backtick forms. Beside no panic: Derive's verb
class is in VerbClasses(), argc is null or non-negative, a program is a
command name, both digests are 64 lower-case hex characters, status_masked
is in MaskedKinds(), and Derive is deterministic. Every wire host is a
fixed point of host.Canonical, every ssh host is hostname-shaped, and both
lists are sorted and unique. Label returns "" for a tool that names no
file and a word from Labels() otherwise. LeadingDirectory's ok says
whether it found directories, each is a word the shell takes as it
stands, and leadingSteps folds the same count. The row-10 host predicate
and 253-byte bound on wire hosts wait for that row to land.

FuzzTranscriptLines is seeded with every line of the live 2.1.280
transcript. Its oracle is line independence: what TranscriptIDs and
FinalAssistantText read from a file is what they read from its lines one
at a time, so a line that does not parse costs that line and nothing
after it; topLevelSidechain agrees with encoding/json on valid lines.
FuzzTranscriptBlocks writes a call and its result through encoding/json
and reads the id, the result or denial, and the final text back.

Each target ran 60 s locally and found nothing (FuzzDerive about 4k
execs/s; the transcript line target about 230, because it writes a file a
line). TestRubricTest1_ShapeSeeds and TestRubricTest1_TranscriptSeeds run
every property over the seeds as plain tests, which is what nine new
mutants name.

A Fuzz job in ci.yaml finds every func Fuzz target in the tree, gives each
30 s, prints any failing input so it can be committed as a seed, and
fails if it finds fewer than the ten targets the tree holds today.

Rubric fix list entry 23 (test-1, errors-7). The settings round-trip
target is entry 52's, committed with it.
@k4rthiknerella

Copy link
Copy Markdown
Contributor Author

Thanks for the careful pass. Point by point:

Blocking 1. Fixed and verified: under -race the zero-allocation check measures the walk through Field, Top as a whole is checked only without -race where Valid's pool stays warm, and RES-7's race growth bound is its own 6 MiB, clear of CI's observed 4.32 MiB. and restack #62-#66 on top.

Should fix 2. Updated the description directly: it now says Test was red on the jsonscan allocation check specifically (fixed above), and Perf gates (linux) is red mostly because linux still gates against #59's darwin baseline, plus hook.1MiB/post.1MiB/post.4MiB, which this PR turns on. No code change, this is a description call.

Should fix 3. Added all three: Read under iotest.OneByteReader and a mid-stream iotest.ErrReader, readFinalMessage's own cap tested at exactly MaxPayloadBytes and one byte over, and payload_too_large through Post.Capture and RunProbe the same way Handler already had it.

Nit 4. Right, nothing calls Lines or Tail yet. The package comment now says only Head is used today and the other two are staged for later PRs.

Nit 5. Agreed, it reads up to 8 MiB plus one byte before refusing. Changed the text to 'refused it without parsing'.

Nit 6. Behavior is fine, since every caller discards dst on error, but the comment overstated it. Reworded to say earlier fields may already be written to dst and every caller drops it on error.

@lutic1 lutic1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k4rthiknerella, I re-reviewed the PR at a3f9a9e. All six items from the first review are fixed and verified: go test -race -count=3 passes for jsonscan, hookio and cmd/rashomon, and the new short-reader and post/probe over-cap tests pass. The description is accurate now, apart from citing commits 2a7e2f9/c74f1c6, which aren't on the branch (the real ones are 82dce08/df17877).

The adversarial pass has had no reply, and the intake commit itself is unchanged since the review. These still reproduce on the head:

Blocking

  1. readFinalMessage has no read limit (cmd/rashomon/intake.go:57,73). On a reader producing 4× the cap it consumed 33,554,432 bytes; it should stop at 8,388,609. digest --stdin can still hang on a stream that never ends. Wrap the reader in io.LimitReader(in, MaxPayloadBytes+1). The new N/N+1 test reads from a finite strings.Reader, so it can't catch this. Add a test with a 4×Max reader.
  2. A top-level JSON string passes as an object (hookio/stream.go:19-34). "", "{" and "{\"x\":1}" all still return ok=true. Listing it under "What could go wrong" doesn't fix it. Check the root value's kind.
  3. session_id handling. A 5,000-byte id still rejects the whole payload. A long id followed by a short one also returns ok=false, because intake.go:62 ORs long instead of letting the last copy win. Make the last copy win, and decide what an over-long id should do.

Should fix, or reply with reasoning

  • Case-variant keys. "No case folding" is documented, but {"Last_Assistant_Message":"hi","SESSION_ID":"s"} now silently loses both the message and the session. Please reply explicitly, and fuzz against the old struct types.
  • The "one scan" claim is still inaccurate. Validation and walking are separate passes, tool_response is walked again, and each Lookup builds its own KeySet.
  • Write/Edit payloads still go through json.Unmarshal into any, and that path has no memory gate.
  • read.go:75 forces a runtime.GC() on every payload of 1 MiB or more.
  • Payload.Strs pairs values by position, so mismatched lengths panic.
  • The precondition on shape Lookup/Field is undocumented.
  • IntakeStream skips the root check when fn stops early.
  • An over-cap record always lands in "unattributed".
  • The unused 64 KiB LastMessageTail is still captured.

Missing tests: top-level strings, case-variant keys, over-long or duplicated session_id, and a 4×Max reader.


Generated by Claude Code

A hook payload is attacker-influenced and can be 8 MiB, and every entry
read it with io.ReadAll and decoded it into Go values: about eight
copies of the payload, and a peak RSS of 56 MiB at the cap. Two packages
replace that.

internal/hookio reads stdin once with bounded growth (a 4 KiB first
chunk, chunks growing to 64 KiB, one exact-size copy), accepts exactly
MaxPayloadBytes and refuses one byte more after a bounded read, so a
refusal costs no more than the cap. A refused payload is recorded in the
unattributed run under a new coverage reason, payload_too_large: nothing
broke inside the program, so it is not internal_error. Intake validates
the payload with json.Valid and scans its top level once into the spans
of the 22 keys any consumer may read, in a frozen order later PRs append
to and never reorder. IntakeStream walks a payload that is never held.

internal/jsonscan locates values instead of decoding them. Top and Field
walk a validated buffer and record byte ranges; Head, Tail and Lines
read a string field through fixed buffers, so a 4 MiB stdout is scanned
for its summary line with 24 bytes allocated; Stream validates as it
goes through a 64 KiB window and copies out only the wanted paths, as a
head, a tail, a presence bit or lines, and is fuzzed against
encoding/json as its oracle. Key matching is exact after unescaping: the
old case-insensitive match was an encoding/json accident, and a
wrong-case key now leaves the call unattributed, visibly.

hook, post, probe and paused fill their fields from the one scan.
tool_response is still never a value: backgroundedCall reads two
presence bits off spans. The failure message is read to 8 KiB, the exit
code being on its first line. shape reads command, run_in_background and
the host fields as spans. canonical keeps its decode into any -- the one
decode a hook path may still make, named by TestRES5_PayloadScannedOnce
and kept for digest compatibility until RES-48 -- but streams
json.Marshal's bytes into the HMAC instead of building them; a fuzz
target holds the stream to json.Marshal byte for byte and a pinned
digest shows nothing moved. recap and digest --stdin stream the Stop
payload: the final message is captured as a 256 KiB head (the account's
64 Ki runes, in UTF-8) and a 64 KiB tail for the comparator, never
whole. scratchpad_dir, declared and never read, is gone.

Budgets: the 1, 4 and 8 MiB rss cases gate from this PR. Each case now
records the median measured on the reference machine beside its line,
and a line holds there with no tolerance (LineHolds): until RES-48 the
hook cases carry 3.4 MiB per MiB of payload and the post cases 2.45, the
read plus canonical's decoded copy of tool_input, with the basis on the
row. Measured here: hook 6.14 / 6.17 / 9.52 / 19.75 / 31.81 MiB against
6.50 / 6.58 / 9.90 / 20.10 / 33.70; post 5.98 / 6.16 / 8.94 / 15.48 /
23.58 against 6.50 / 6.58 / 8.95 / 16.30 / 26.10; the large cases were
11.84 / 27.59 / 47.89 and 13.28 / 32.97 / 55.80 before. New benchmark
rows for Top, Lines, Stream and Read; the rows this change moves are
restated at what it measures, down by half on edit_50k and a quarter on
edit_314k, up by about 4 KB on the small payloads (the 4 KiB first
chunk) and about 24 KB on recap (the streamer's window and captures in
place of a decode of the whole payload).

Rows closed: RES-4 (TestRES4_CapAtNAndNPlusOne, TestRES4_ReadAllocBound),
RES-5 (TestRES5_PayloadScannedOnce, TestRES5_TopDefersToJSONValid,
FuzzRES5_TopMatchesStdlib), RES-6 (TestRES6_LargeFieldScanIsConstantMemory),
RES-7 (TestRES7_HugeFinalMessageIsConstant, FuzzRES7_StreamMatchesStdlib),
RES-28 (TestRES28h_EveryLineHoldsOnTheReferenceMachine,
TestRES28h_LineHoldsTakesNoTolerance).
The windows cross-build vet failed on cmd/rashomon/res7_test.go: syscall.Rusage
has no Maxrss field on windows, and the test read it directly. The same eight
lines were copied into test/perf/rss_test.go and test/perf/parallel_test.go,
where the perf tag hid the same break from the untagged vet.

test/perf/peakrss now holds the one read. MiB is built on unix and returns the
child's ru_maxrss in MiB; on every other platform it returns ErrUnavailable,
and the RES-7 test skips by name there. The three callers use it.

The copies multiplied by 1024 on linux only, so freebsd, netbsd and openbsd
were read as bytes when getrusage reports KiB there. fromMaxrss treats darwin
and ios as bytes and every other unix as KiB, and TestRES28i_ pins both sides;
two RES-28 mutants that slip either unit turn it red.
…hine

The entry rows in budgets.json were measured with group commit, before P-10's
bounded intake replaced the hook and post read path. Each row's
maxrss_mib_p50 is now this tree's median of nine from TestRES28b_ on the
reference machine (darwin/arm64, Apple M3, go1.27.2, release flags, a pinned
and installed store), the middle of three runs, run 2026-10-13:
hook.bash_short 5.95, hook.edit_50k 6.20, post.bash_short 5.94,
post.edit_314k 6.73, recap.stop 7.06 on a 100-call session, recap.pending
6.06 on a 10-call session, probe.start 5.97 with nothing queued, and
probe.end 5.98. No run of the three moved a row by more than 3 percent.

Each basis starts with the measurement and keeps the archive's derivation
after it, as the record of where the row's p95 came from. Every p95 is
unchanged and at or above its p50. The generated table is regenerated, and
the DEL-7 floor-row mutant and RES-28's entry-row mutant are re-anchored on
the new rows.

TestRES28b_EveryEntryRowIsMeasured fails on any entry row whose basis does
not start with a measurement, so a row cannot go back to a derived figure
unnoticed; a RES-28 mutant that restores one turns it red.
TestRES28_ ran three times on the reference machine on 2026-10-13, on a
pinned and installed store, and every case held its line with no tolerance
applied. Each case's measured_mib is now the middle of those three medians of
nine: hook 5.97, 6.12, 9.48, 19.72 and 31.88 MiB at 400 B, 40 KB, 1, 4 and
8 MiB; post 6.06, 6.05, 8.86, 15.42 and 23.66 MiB at 540 B, 40 KB, 1, 4 and
8 MiB. No line, slope or basis changed. post.1MiB sits 0.09 MiB under its
8.95 MiB line, the thinnest margin in the table; RES-48's removal of the
decoded copy is what widens it. The generated table is regenerated, and the
RES-28 case-set mutants are re-anchored on the new figures.
TestRES7_HugeFinalMessageIsConstant failed in CI on linux under -race,
44.18 MiB at a 2 KiB message and 130.42 MiB at 7 MB, and read as "the
payload is held". The payload is not held, and the race runtime is not the
cause. On Linux, wait4's ru_maxrss for a child is at least its parent's own
peak: Go starts every child with CLONE_VFORK|CLONE_VM, and exec records the
shared address space's high-water mark into the child's signal struct before
replacing it (fs/exec.c, exec_mmap). The test parent builds the 7 MB payload,
so the second child read the parent.

Measured in a linux/arm64 VM (kernel 6.8, go1.25.8): /bin/true read 129.75
MiB from a parent that had touched 100 MiB, and 29.50 MiB from the same
parent before; on darwin it read 1.11 MiB both times. RES-7 failed without
-race too (4.50 then 29.50 MiB). In-process, recap allocates 34 KiB at a
2 KiB message and 1.02 MiB at 7 MB with no GC cycle, so the message is
streamed as intended.

The same read fed every RSS gate under the perf tag, so the linux RSS step
would have compared the perf test binary's own peak, not rashomon's, on
every row.

test/perf/peakrss now runs each child under a reaper: a release-flag binary
built from test/perf/peakrss/reaper that starts the child on its own stdin,
stdout and stderr, waits, and reports the child's ru_maxrss and its own peak
on descriptor 3. Its peak is 2.0 MiB on linux/arm64, under the release
binary's smallest entry (recap pending, 3.12 MiB); the test binary itself
was 3.6 MiB at rest and could not serve. A reading that does not exceed the
reaper's own peak (VmHWM on Linux, since getrusage there carries the
parent's peak too) is refused, never taken. The reaper exits with the
child's status, so a failing child is still an *exec.ExitError.

Through the reaper on linux/arm64, RES-7 reads 4.38 then 5.50 MiB without
-race and 20.0 then 22.9 MiB with it; the Intake mutant still reads 18.88
MiB and turns it red. The race floor alone is over 12 MiB, which is why only
the growth bound applies under -race; the test now says so with the figures.
Darwin readings through the reaper match the recorded medians.

Every entry row on linux/arm64, median of nine through the reaper, three
runs: hook.bash_short 3.50-3.62, hook.edit_50k 3.75, post.bash_short
3.50-3.62, post.edit_314k 4.25, recap.stop 4.75, recap.pending 3.12,
probe.start 3.50, probe.end 3.50-3.62 MiB, each 31 to 41 percent under the
darwin row the linux gate falls back to. linux/amd64 is not measured here.

TestRES28i_ covers a child beside a 96 MiB parent, a child holding 48 MiB,
the child's exit status, a child that never started, the floor refusal, the
report format and VmHWM. Seven RES-28 mutants break the reaper; two of them
(measuring directly, and reading the reaper's peak from rusage) turn red
only on Linux, where the defect lives.
The row's basis read "8 / 10 MiB until P-10, then per RES-28 (7.16 MiB at
314 KB)" beside a p50 measured on the P-10 tree, and its p95 stayed at the
pre-P-10 10 MiB. The archive's row applies RES-28's line from P-10 on, so
p95 is now 7.16 MiB and the basis says where it comes from. Sixty runs on
the reference machine (darwin/arm64, Apple M3, go1.27.2, release flags,
through the reaper, on a pinned and installed store) gave min 6.50, p50
6.73, p90 6.91, p95 6.92 and max 6.95 MiB, so the line holds every run. The
generated table is regenerated.

TestRES28b_NoBasisWaitsOnALandedPR fails on any entry or rss case whose
basis still reads "until" a PR this tree carries; the set of landed PRs
moves from runs.go to gate.go, beside the comparisons the untagged tests
read, and CaseGates keeps reading it. A RES-28 mutant that restores the old
basis turns it red.

post.1MiB was measured in three batches of nine on the reference machine:
batch medians 8.77, 8.91 and 8.86 MiB, median 8.86, under its 8.95 line, so
the post slope stays 2.45 and the recorded 8.86 stands. The margin is 0.09
MiB and a single batch has read within 0.04 of the line; RES-48, which
removes canonical's decoded copy, is what widens it.
TestTopAllocatesNothingPerValue called Top, whose first step is
encoding/json's Valid, which borrows a *scanner from a package-level
sync.Pool. Under -race, that pool's Put intentionally drops its argument
on about 1 in 4 calls (sync/pool.go's "Randomly drop x on floor") so the
race detector can catch Pool misuse. Under testing.AllocsPerRun's forced
GOMAXPROCS(1) plus the CPU contention a shared CI runner produces, this
was measured driving 48-68 of 50 Top calls to a fresh scanner allocation,
failing the test on linux/amd64 CI while passing locally and on other
PRs' runs of the identical code.

Measuring walkObject in isolation, via Field (which Top itself uses once
a buffer is already validated), held at 0 allocations across 50 calls in
the same contended, race-built environment, isolating the allocation to
json.Valid's pool rather than to jsonscan's own code.
TestRES7_HugeFinalMessageIsConstant checked the same 4 MiB growth bound
under -race and without it. CI's linux/amd64 race run produced 4.32,
4.04 and 4.15 MiB of growth across three PRs on this code, over that
bound, while linux/arm64, darwin/arm64, darwin/amd64 and a clean or
contended linux/amd64 docker run all measured 2.74-3.15 MiB here.

Non-race growth keeps the 4 MiB bound; it was never close (the existing
floor note already puts it under 1.5 MiB). The race bound is now 6 MiB,
clearing CI's observed 4.32 MiB high-water mark by about 39% while
staying two orders of magnitude under the 86-90 MiB measured here for a
recap that holds the whole payload, the defect this test exists to
catch.
…istic

Measuring the walk through Field keeps the zero-allocation property
testable under -race, but it left Top's own code unchecked: an
allocation added to Top outside the walk passed. Without -race,
json.Valid's sync.Pool stays warm, so Top as a whole allocates nothing
there; the test now asserts that too, and skips it only under -race,
where the pool drops values at random.
Read now has tests under a one-byte-at-a-time reader and a reader that
errors partway through, proving its io.ReadFull loop tolerates both.
readFinalMessage gets its own exact-cap test, separate from Read's: recap
streams through IntakeStream, which never calls Read, so its cap is a
different line of code. Post.Capture and RunProbe get the same over-cap
test Handler already had, since all three share hookio.Read's cap and
closeReason's mapping to payload_too_large.
The payload is read in full before the size check refuses it; what never
happens is parsing it.
The package comment named all three jsonscan readers as what consumers use
today; only Head has a caller yet.
The comment claimed it fails before any field is used; a field before the
bad one is written to dst, and every caller just discards dst on error.
encoding/json matches a struct field's name exactly or, failing that,
case-insensitively under Unicode simple folding, and every payload struct
the hook entries decoded into before the scan got that match for free.
The scan matched exactly, so {"SESSION_ID":"s"} lost its session.

NewFoldKeySet builds a KeySet that matches the way a struct field does,
the last match winning, and Path.Fold does the same for a streamed path.
NewKeySet keeps the exact match a map decode has. A folded character can
take more bytes than the one it matches (KELVIN SIGN is three bytes and
matches k), so the unescape limit and the stream's key buffer allow
utf8.UTFMax bytes per character of a fold key.

Field, Lookup and ValueSpan now say what they rely on: a buffer json.Valid
accepted. They do not validate, and on other bytes they can report keys a
decoder would have refused.
…its head

Intake, backgroundedCall and shape's run_in_background read keys their
old structs matched case-insensitively, so they now use a fold KeySet:
{"SESSION_ID":"s"} is attributed to s again, and Run_In_Background still
marks a call backgrounded. command and the other fields shape reads came
from a map decode and stay exact. FuzzRES5_IntakeMatchesTheStructDecode
holds Payload, PostPayload, sessionPayload and backgroundedCall to the
old structs, and FuzzRES5_ToolInputFieldsMatchTheirOldDecode does the
same for shape's fields; a payload that repeats a key is left out, since
the scan reads every key from its last copy where the struct decode kept
an earlier string under a later null and failed on any copy of the wrong
type.

An over-cap payload is refused without being parsed, but its first 4 KiB
were read before the cap was reached. Intake returns a *TooLargeError
carrying the session_id and cwd that head names, so hook, post, probe and
paused record the refusal in the run that sent it rather than always in
unattributed. A head that is not a sound start of one object, or a value
the head cuts, names nothing.

IntakeStream took the root's kind from the first decoded byte of its value,
so a top-level string such as "{\"x\":1}" passed as an object. It now
looks at the first byte past leading whitespace before anything is parsed,
so a non-object root is refused and fn is never called for it. A walk that fn
stops early returns ErrStopped, since the rest of the payload was not read.

Strs took destinations and keys as two lists paired by position, so a
length mismatch panicked and a reordering swapped fields. It takes
Into(key, &dst) pairs. The payload's KeySet is built once per process.

The forced collection after a read of 1 MiB or more stays: it holds
hook.1MiB under its 9.90 MiB line (9.59 measured, 10.09 without it), and
BenchmarkRead puts its cost at about 0.1 ms at 1 and 4 MiB.
readFinalMessage counted what IntakeStream read and only then compared it
with the cap, so it read all of stdin: a payload four times the cap was
read whole, and digest --stdin on a stream that never ends never
returned. The reader is now io.LimitReader(in, MaxPayloadBytes+1), and
the count still refuses one byte over.

session_id is read from its last copy, as the struct decode read it. A
copy over 4 KiB used to fail the whole payload, even when a later copy was
short; now the last copy wins, and an over-long one only names no session:
the message is still read and recap falls back to the newest run, its rule
for a payload with no session_id. Both keys are matched in any case, as the
old struct matched them; FuzzRES7_FinalMessageMatchesTheStructDecode holds
recap and digest --stdin to the structs they decoded into.

lutic1 commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

@k4rthiknerella, since you don't have Claude Code, I ran the repo's live test test/live/l-items.sh against this PR's head (a3f9a9e). Each run used a real Claude Code 2.1.303 session with its own HOME and store, and main (7b325a0) is the baseline.

  • L-1, a session using Bash, Write and an MCP tool: ✅ All 4 calls were recorded and all 4 transcript ids matched, with 0 missing. Coverage was verified, the same as main.
  • L-2, another tool's PreToolUse hook beside ours: ✅ The other hook fired and we recorded that Bash call (1/1), the same as main.
  • L-3, detach mid-session: coverage unverified, end_recorded false (run 1: run_not_closed; run 2: run_not_closed, transcript_mismatch, execution_mismatch). That's the same end state as main, but it isn't evidence either way.
    • On main, the reasons change from run to run (I ran it 4 times). Claude Code 2.1.303 runs sleep 25 in the background, so the second Bash call lands 0.08–1.1 s later and races detach.
    • l-items.sh needs a fix before L-3 can judge a PR. That's separate from this PR.

Recap line (#61): I ran it twice. Records landed both times (L-1 verified 4/4), but the end-of-turn ※ rashomon line never appeared. That's by design: it only prints when there is something to report. So this run doesn't show the recap path working for #61.

This adds to my review above; it doesn't replace it, and the open items there still stand.


Generated by Claude Code

k4rthiknerella added a commit that referenced this pull request Oct 19, 2026
Three byte tests stood in front of a full decode and disagreed with it.

spend's main-transcript prefilter skipped, once the file was dated, every
line without the exact bytes "usage". encoding/json matches the key in any
case under Unicode folding ("Usage", "uſage") and through escapes
("us\u0061ge"), so such a usage line was dropped without being counted,
priced or unparsed. mayCarryUsage now also keeps a line holding a \u escape
or a quoted five- or six-byte word that folds to "usage". Not every
backslash, which the rubric rejected: nearly every real line has one.
Unparsed stays tied to the exact marker.

report's turn reader skipped every wanted assistant line without the bytes
"text", which dropped a reply whose content is a plain string, a shape the
reader's own decoder handles. FinalAssistantTexts and FinalAssistantText then
disagreed on one file. The header now records whether message.content is a
plain string (contentShape, a bool and nothing of the text), and mayHoldText
keeps a line that is plain, holds "text", or holds a \u escape. Dropping the
prefilter outright was measured first: about 2.9x slower on a turn of 1,000
tool_use lines with 10 KB inputs (33 ms against 11.5 ms); the widened test
costs nothing measurable.

topLevelSidechain took the first isSidechain key spelt exactly, while a
struct decode takes the last key matching under folding and leaves the value
alone on null. The walk now applies the struct decode's rule, reading the
key's value as its literal only.

FuzzUsagePrefilter, FuzzTextPrefilter and FuzzSidechainKey check each test
against the decode it stands in front of (45 s each locally, no failures);
TestRubricPERF3 and TestRubricTEST14 run the seeds untagged for the sweep.
The rubric's fourth sub-item, FuzzPresent for post.go's present type, is moot:
#61 replaced that type with jsonscan.Field spans (internal/hook/post.go:218),
which FuzzRES5_TopMatchesStdlib checks against encoding/json.

Rubric perf-3, test-14. Mutants: RUBRIC_perf-3_usage-prefilter.py,
RUBRIC_test-14_transcript-prefilters.py; two SP_join.py anchors re-anchored
on the renamed skip variable.
k4rthiknerella added a commit that referenced this pull request Oct 19, 2026
Three enums used their zero value as a meaningful member, so a value nobody
set read as a decision. report.LineKind's zero was LineBlank: a reader that
forgot to take DecodeHeader's answer skipped every line as blank.
budget.Phase's zero was Verify (#58): an unset phase would stand down as
verify_budget. jsonscan.CaptureMode's zero was CaptureHead (#61): a Path
whose Mode was forgotten copied the value's head out unasked.

Each now reserves its zero for a neutral member. LineKind's is a blank
constant DecodeHeader never returns. Phase's is the unexported phaseUnset,
which has no stand-down word (the empty string no writer may record) and
which Phases does not list; the thirteen exported phases, their order and
their frozen reasons are unchanged, and TestPhaseReasonsAreTheFrozenList
still passes. CaptureMode's is a blank constant that Stream's existing mode
switch refuses with ErrPaths; the one test that relied on the old default
now names CaptureHead, as every production Path already did.

Rubric naming-12. Mutant: RUBRIC_naming-12_neutral-zero.py.
k4rthiknerella added a commit that referenced this pull request Oct 19, 2026
…uzz in CI

Nothing in the tree would find a crash in the parsers that read
model-written bytes. In a hook a recovered panic costs the call's
declaration, and a runtime fatal error such as stack exhaustion exits 2
and blocks the user's tool call. #61 added fuzz targets for the payload
scan and the canonical digest; the shell lexer and the transcript readers
still ran only on curated tables.

FuzzDerive, FuzzHosts, FuzzLabel and FuzzLeadingDirectory are seeded with
the leak corpus, the program claims and the comsub nesting bound passed by
one in quoted, unquoted and backtick forms. Beside no panic: Derive's verb
class is in VerbClasses(), argc is null or non-negative, a program is a
command name, both digests are 64 lower-case hex characters, status_masked
is in MaskedKinds(), and Derive is deterministic. Every wire host is a
fixed point of host.Canonical, every ssh host is hostname-shaped, and both
lists are sorted and unique. Label returns "" for a tool that names no
file and a word from Labels() otherwise. LeadingDirectory's ok says
whether it found directories, each is a word the shell takes as it
stands, and leadingSteps folds the same count. The row-10 host predicate
and 253-byte bound on wire hosts wait for that row to land.

FuzzTranscriptLines is seeded with every line of the live 2.1.280
transcript. Its oracle is line independence: what TranscriptIDs and
FinalAssistantText read from a file is what they read from its lines one
at a time, so a line that does not parse costs that line and nothing
after it; topLevelSidechain agrees with encoding/json on valid lines.
FuzzTranscriptBlocks writes a call and its result through encoding/json
and reads the id, the result or denial, and the final text back.

Each target ran 60 s locally and found nothing (FuzzDerive about 4k
execs/s; the transcript line target about 230, because it writes a file a
line). TestRubricTest1_ShapeSeeds and TestRubricTest1_TranscriptSeeds run
every property over the seeds as plain tests, which is what nine new
mutants name.

A Fuzz job in ci.yaml finds every func Fuzz target in the tree, gives each
30 s, prints any failing input so it can be committed as a seed, and
fails if it finds fewer than the ten targets the tree holds today.

Rubric fix list entry 23 (test-1, errors-7). The settings round-trip
target is entry 52's, committed with it.
@k4rthiknerella

Copy link
Copy Markdown
Contributor Author

Thanks for the second pass. Point by point:

Description. You're right, those two SHAs were never on the branch. The description now names the commits by subject instead: "jsonscan: measure the walk, not Valid's own pool, for zero allocation" and "jsonscan: keep Top's zero-allocation check where the pool is deterministic".

Blocking 1. Fixed in "recap: read the Stop payload to the cap and no further": readFinalMessage now reads through io.LimitReader(in, MaxPayloadBytes+1). I reproduced your 33,554,432 bytes first; TestRES7_FinalMessageReadStopsAtTheCap now feeds it a 4x-cap reader and a reader that never ends, and asserts at most 8,388,609 bytes are taken from either.

Blocking 2. You're right, and listing it under "What could go wrong" was not a fix. In "hookio: match payload keys in any case, and attribute a refusal from its head", IntakeStream looks at the first byte past leading whitespace before anything is parsed, so "", "{" and "{\"x\":1}" are refused with ErrNotObject and the callback never runs (TestRES7_IntakeStreamRejectsANonObjectRoot, plus TestRES7_FinalMessageRefusesATopLevelString on the recap side).

Blocking 3. Fixed in "recap: read the Stop payload to the cap and no further": the last copy of session_id wins, and a copy over 4 KiB now means no session named, not no payload. The message is still read and recap falls back to the newest run, the rule it already applies to a payload without session_id. TestRES7_FinalMessageSessionIDLastCopyWins covers both of your shapes plus a null last copy, which keeps the earlier id, as the struct decode did.

Should fix: case-variant keys. You're right, and calling the old match an encoding/json accident was wrong: every payload struct got it, so I put it back. "jsonscan: match a key as a struct field matches its name, when asked" adds NewFoldKeySet and Path.Fold (exact match first, then Unicode simple folding, last match wins), and intake, backgroundedCall, run_in_background and recap use them, while command and the host fields stay exact because they came from a map decode. Three fuzz targets now hold every field to the old struct or map decode (FuzzRES5_IntakeMatchesTheStructDecode, FuzzRES5_ToolInputFieldsMatchTheirOldDecode, FuzzRES7_FinalMessageMatchesTheStructDecode), clean for 60 s, 30 s and 45 s; the buffered path still reads a repeated key from its last copy, so those targets leave repeated keys out, and the description says why.

Should fix: the "one scan" claim. Agreed, it overstated what happens. I reworded it everywhere it appears (hookio's package doc, Intake, hook's Payload, post, RES-5's test doc and mutant comment, and the description) to say what is true: one read, one json.Valid pass and one walk of the top level, then a walk of tool_input or tool_response for the few keys read from them. Intake's KeySet is now built once per process, and Lookup's one-key set never reaches the heap (go build -gcflags=-m reports []string{...} does not escape at jsonscan.go:371).

Should fix: Write/Edit payloads. Half right: the hook path is gated, since hook.1MiB, 4MiB and 8MiB are Edit payloads whose tool_input carries the whole payload (test/perf/payload/payload.go:88-101), gated at 3.4 MiB per MiB with canonical's decode named as the basis (test/perf/budgets.json:19-21). The post cases pad tool_response instead (payload.go:104-124), so post's own decode of a large tool_input has no row, and the description now lists that gap under "Not in this PR" until RES-48 removes the decode.

Should fix: read.go:75's runtime.GC(). Kept, and the comment now carries the measurement: it holds hook.1MiB at 9.59 MiB against its 9.90 line (10.09 without it), and BenchmarkRead puts its cost at about 0.1 ms at 1 and 4 MiB, against the hook rows' 10 to 15 ms p50 (read.go:82-89). GOMEMLIMIT or SetGCPercent would tune every later collection to save one, and with this line carrying no tolerance I'd rather keep the one collection I can measure.

Should fix: Payload.Strs pairing. Fixed in "hookio: match payload keys in any case, and attribute a refusal from its head": Strs takes Into(key, &dst) pairs, so a length mismatch can't happen and a reorder can't swap fields.

Should fix: the shape Lookup/Field precondition. Documented in "jsonscan: match a key as a struct field matches its name, when asked" and "hookio: match payload keys in any case, and attribute a refusal from its head": Field, Lookup and ValueSpan say they rely on a buffer json.Valid accepted and do not validate, and shape's package doc says Derive, LeadingDirectory, Hosts and Label require one value json.Valid accepts, as hookio.Intake's tool_input span is.

Should fix: IntakeStream when fn stops early. Fixed in "hookio: match payload keys in any case, and attribute a refusal from its head": the root's kind is now checked on its first byte, before fn can be called, and a walk that fn stops early returns ErrStopped, so a caller can't take a half-read payload for a validated one (TestRES7_IntakeStreamReportsAnEarlyStop).

Should fix: the over-cap record's attribution. Fixed in "hookio: match payload keys in any case, and attribute a refusal from its head": the first 4 KiB were already read, so Intake returns a TooLargeError carrying the session_id and cwd they name, and hook, post, probe and paused record the refusal in that run. A head that is cut inside the value, is not a sound start of an object, or names no session still goes to unattributed (TestRES4_TooLargeNamesTheSessionInItsHead, and the three RES-4 entry tests plus a new paused one).

Should fix: the unused 64 KiB LastMessageTail. Not changed: the tail is captured now on purpose, because the spec has it captured beside the head until the comparator lands (Technical spec 8.2.2), so that PR doesn't have to change recap's read. It costs only what it holds: the ring grows by append up to the value's own size (unquote.go:189-190), and fm.Tail is only filled when the head was cut (intake.go:76).

Missing tests. All four added: top-level strings (TestRES7_IntakeStreamRejectsANonObjectRoot, TestRES7_FinalMessageRefusesATopLevelString), case-variant keys (TestRES5_FoldKeySetMatchesStructFieldNames, TestRES5_IntakeMatchesStructFieldNames and the three struct-decode fuzz targets), over-long and repeated session_id (TestRES7_FinalMessageSessionIDLastCopyWins), and the 4x-cap reader (TestRES7_FinalMessageReadStopsAtTheCap). Each new decision has a mutant in RES-4_refusal-head.py, RES-5_fold-keys.py or RES-7_final-message-bound.py, and each one turns its named tests red.

@lutic1 lutic1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k4rthiknerella, I re-checked #61 at af6a631 and I'm approving it.

Confirmed fixed:

  • readFinalMessage is bounded. Our 4×cap reader consumed 33,554,432 bytes at a3f9a9e and now consumes 8,388,609. An endless reader returns too.
  • A top-level string, a bare string with leading space, and an array are refused before the callback runs.
  • session_id: the last copy wins, an over-long id means no session but keeps the message, and a null last copy keeps the earlier id.
  • Case-insensitive keys are back (Last_Assistant_Message / SESSION_ID read correctly).
  • The "one scan" wording, the Into pairs, the documented precondition, ErrStopped, and the over-cap attribution from the head.
  • The answers on runtime.GC, LastMessageTail and the post-side Write/Edit gap are fine.

Build, vet, -race and sweep --check are clean, and Test is green. Perf gates (linux) is inherited from #59. Note that #59's report-only default currently leaves this PR's 1/4/8 MiB cases ungated on Linux; that's being raised on #59.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants