fix(compaction): stop null persisted tool arguments from killing the turn - #4633
Conversation
|
Adding the one piece of evidence a reviewer would reasonably challenge first: if Because the pruning pass never sees a rehydrated entry. Cold-spill rehydration is wired Compaction pruning reads path.push(cloneSessionEntry(materializeResidentEntryForReadSync(current, this.#residentBlobStores(), cache)));No The two facts are complementary, not redundant:
I deliberately did not change the eviction writer. A probe across memory → disk → reopen One adjacent reader I checked and left alone: |
19577d3 to
094ff69
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Independent adversarial review at exact head 094ff69 (rebased onto dev 6696988 by maintainer; original commit 19577d3 preserved as author — cherry-pick kept Author: Dayoooun).
Verified:
- Guard coverage: toolArguments() covers all four persisted-arg readers in pruning.ts (toolCallPath, editToolPathGroups, normalizedIdempotentBashCommand, toolTargetKey) + /copy guard at command-controller.ts:401. Independent sweep found no other unguarded persisted-arguments dereference on the reload/pruning path.
- Cold-spill interaction: marker.payloads basePath is checked first in rehydrateColdSpillValue, so null-args entries with payload refs rehydrate fully on the provider-context path; null is never mistaken for a spill/prune sentinel; no double-spill; JSON.stringify(null) null-safe. Data preservation confirmed (author's live-store audit: 5175/5175 payloads rehydratable, 0 still-null).
- Malformed shapes: primitives guarded by the same typeof check; arrays traced through header parsing and target keys without mis-keying.
- #4625 relationship: complementary (write/rehydrate-side invariant, zero file overlap), not a duplicate; should merge separately after rebase.
- Validation at this head: pruning-null-arguments 3 pass; pruning suites 105 pass; agent + coding-agent typecheck/biome clean.
Known residual (base-preexisting, outside this diff, tracked for fix-forward): serializeConversation (compaction/utils.ts:149-153) Object.entries on raw args — same class, one step downstream on the same getBranch() path; plus two contained UI-formatting sites (tree-selector #formatToolCall, formatToolArgs). None regress with this PR; PR fully fixes its named turn-fatal defect.
Verdict footer in PR body regenerated for base 6696988...head 094ff69.
094ff69 to
87a5b73
Compare
|
Maintainer rebase delivered at 87a5b73 (base 44d7b6e, live dev). Exact final three-dot binary digest, canonical Authorship preserved: single commit, Local validation at this head: pruning-null-arguments 3 pass; pruning red-team/staleness/gate suites + maintenance-prune-gate 105 pass; session-compaction-eviction 25 pass; agent + coding-agent biome/tsc clean; verify-gjc-state-writers --fail 0 violations; changelog-history-guard 12 pass; git diff --check clean. Fail-before re-verified structurally on base 44d7b6e (unguarded Status: the prior Yeachan-Heo approval targets superseded head 094ff69 only and does NOT satisfy the exact-head contract at 87a5b73. No self-approval. PR body carries exactly one needs-human verdict (reviewer-id:pending) with the current-head digest. New CI runs 32089919311 / 32089918809 are being tracked; results will be posted here. Requesting a fresh authorized non-author exact-head review at 87a5b73. On approval + green contract/product CI, the verdict flips to merge-approved with the truthful reviewer-id and evidence, then squash-merge into dev. — |
|
Signed exact-head CI/review status — head 87a5b73, base 44d7b6e:
Next: on fresh authenticated APPROVED review at 87a5b73 + green contract/product CI, the verdict line flips to — |
87a5b73 to
c13bf36
Compare
|
Maintainer rebase delivered at c13bf36 onto live dev 7265a61 (which now includes #4616 — verified zero product-file overlap; #4616 touched only
@snowykr — requesting fresh authorized non-author exact-head review at c13bf36. On your APPROVED review at this head plus green contract/product CI, the verdict flips to merge-approved with reviewer-id=snowykr and the squash-merge into dev follows immediately. — |
|
Signed exact-head blocker/status — head Blocker 1 — no fresh authorized non-author review exists for this head. The only APPROVED review on record (user Blocker 2 — contract gate intentionally red (truthful). "Validate exact-head PR contract" (runs 32094959558, 32094900736) fails with exactly one diagnostic: Product CI at this head: Dev CI run 32094959573 in progress (gjcs state-gates ×4 pass, affected-plan pass, native-build running). An earlier affected-path aggregate (run 32094900614) recorded What was delivered this turn (all verified): rebase of the complete five-file delta onto live dev Merge will happen only after: (a) fresh authenticated APPROVED review at — |
c13bf36 to
9ed9fe0
Compare
Dismissed as stale by maintainer: this approval was submitted 2026-08-18T00:24:28Z against head 094ff69 (its body names that head), before any current-head commit existed. GitHub re-anchored the orphaned record to the current head after force-pushes; it never reviewed this code. A genuinely fresh authorized non-author exact-head review is required before any merge-approved verdict. See the signed reconstruction evidence comment.
|
Signed reconstruction evidence — head Stale-approval disposition (this is the important one). The APPROVED record formerly displayed on this PR (id 4955837654, user Yeachan-Heo) was submitted Reconstruction (this turn). Complete accepted five-file scope cherry-picked onto Overlap inspection (all dev commits since 7265a61): #4616 (session-list read syscalls), #4590 (deep-interview continuation), #4603 (team checkpoint prefixes), #4619/#4636 (ai providers), #4623/#4643/#4645 (sdk/tests), #4585 (darwin nested managed reads pin). Product-file overlap with this PR's scope: none — dev touched Exact final digest: canonical Local validation at 9ed9fe0: pruning-null-arguments 3 pass; pruning red-team/staleness/gate + maintenance-prune-gate 105 pass; changelog-history-guard 12 pass; session-compaction-eviction 25 pass (one earlier 7.6s timeout on a cold run re-ran green in isolation, and green again in the full suite — flake, not a regression; same test passes on bare base); agent + coding-agent biome/tsc clean; CI: this push triggered fresh runs on On fresh approval at this head + green contract/product CI: verdict flips to — |
|
Signed current-head status — head Product CI: GREEN (Dev CI run 32100764476, completed). All 20 product jobs pass at this exact head: Contract gate: intentionally red, singular cause. Review state: the stale re-anchored Yeachan-Heo approval (id 4955837654, submitted 00:24:28Z against old head 094ff69) is formally DISMISSED and will never be reused. Fresh exact-head reviews requested from @probepark and @snowykr. PR body carries exactly one On a genuinely fresh non-author APPROVED review at this head: I will independently verify its timestamp/body/commit binding and reviewer authority, flip the single verdict line to — |
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review — merge blocked on a one-line contract fix. The fix itself is right.
minor — any in production code
packages/agent/src/compaction/pruning.ts:246 returns Record<string, any>. AGENTS.md: no any unless absolutely necessary. Every consumer already narrows (typeof path === "string", Array.isArray(paths), …), so Record<string, unknown> is a drop-in with no callsite changes and actually enforces those guards.
otherwise
Correct call: this is consumer-side defense against legacy persisted records, and it does not need a producer change — the current cold-spill producer already emits the __gjcColdSpillArguments sentinel (packages/coding-agent/src/session/session-manager.ts:5362-5369) and is covered by existing persisted-reopen tests. The new regression tests genuinely fail on the old direct dereferences.
Swap the any and this is good to land.
9ed9fe0 to
b1dbda7
Compare
…turn
Sessions written by an earlier cold-spill eviction path persist
`toolCall.arguments` as `null` where the spill sentinel belongs. The
compaction pruning pass dereferenced those arguments unguarded, so
reloading such a session threw
TypeError: null is not an object (evaluating 'args.path')
which surfaced as a turn-killing provider error rather than a skipped
call. 43 sessions in a live store reproduce it.
`ToolCall.arguments` is typed non-nullable, so the type system never
flagged the gap. Route every read of a persisted argument bag through a
`toolArguments()` guard that treats a non-object payload as absent:
path/file_path/filePath extraction, apply_patch header parsing,
idempotent-bash key building, and search target keys. Also guard the
`/copy` last-bash-command lookup, which had the same shape.
Data is not lost: the eviction marker still names the blob, and
rehydration restores the original arguments (verified across the same 43
sessions, 5,175 cold-spilled argument payloads restored, zero still
null).
b1dbda7 to
650fc18
Compare
|
Signed reconstruction evidence — head Review-blocker fix applied (probepark's sole CHANGES_REQUESTED item at 9ed9fe0/b1dbda74): Reconstruction: complete five-file delta on top of Exact final digest: canonical Local validation at 650fc18: pruning-null-arguments 3 pass; pruning red-team/staleness/gate + maintenance-prune-gate 105 pass; changelog-history-guard 12 pass; session-compaction-eviction 26 pass (includes #4625's new regression — both invariants green together); agent + coding-agent biome/tsc clean; Prior state, all superseded by this push: Yeachan-Heo approval DISMISSED (old-head, re-anchored artifact); probepark CHANGES_REQUESTED addressed as above; every CI run and verdict digest from heads 19577d3/094ff698/87a5b730/c13bf36e/9ed9fe06/b1dbda74 is void. @probepark — the one-line On approval here + green contract/product CI: single verdict flips to — |
probepark
left a comment
There was a problem hiding this comment.
Independent maintainer review at exact head 650fc184 — approved. Good catch, and the fix is in the right place.
prior finding resolved
toolArguments() now returns Record<string, unknown> instead of Record<string, any>, and every consumer narrows the fields it reads.
the guard is wider than the reported symptom, correctly
I checked whether the fix handles only literal null while a sibling shape still throws — it does not. toolArguments() treats null, missing/undefined, "", "null", and other JSON scalars as "absent" for indexing purposes. Only null/undefined actually threw before; the other scalars were property-access-safe but were still invalid argument bags being indexed as if they were objects. Normalizing all of them through one accessor is the right call — it means the next malformed shape that shows up in a persisted transcript does not reopen this bug.
call/result pairing survives — this was my main worry
A compaction fix that drops a malformed tool call while keeping its result (or the reverse) desynchronizes the transcript and produces a worse failure than the crash. That does not happen here: the call is not dropped and not replaced, the result is not dropped, and the call is merely excluded from target/staleness indexing. Ordinary output pruning may still replace result text with its existing truncation notice, but call and result IDs are untouched. The compacted transcript stays coherent for the next turn.
Legacy records also keep their cold-spill ref, so provider/fidelity materialization restores the original argument object; an unrecoverable ref degrades to the typed malformed-arguments object on current base.
command-controller.ts — checked, and it belongs
/copy cmd had its own direct arguments.command dereference, so the same malformed live tool-call state crashed there too. The one-line optional chain is adjacent hardening rather than scope creep, and it is disclosed in both the body and the coding-agent changelog. It is unpinned by tests (copy-command.test.ts covers default /copy, not command extraction) — worth a line if you touch the file again, not worth blocking.
minor — two of the three tests are bare not.toThrow()
packages/agent/test/pruning-null-arguments.test.ts:78-98. The repo test contract calls these out specifically: a non-throw assertion pins "no crash" but not "correct behavior", so a future fix that stops throwing by silently discarding the whole message would still pass. Assert the returned entry count and call/result-pair preservation instead.
Both still genuinely fail on base (test 1 through buildAssistantArgumentStalenessIndex → editToolPathGroups → toolCallPath; test 2 through buildStalenessIndex → toolTargetKey → toolCallPath, which additionally proves the multi-reader defense — partial guarding would expose the bash/search dereferences), so they are real pins, just weakly stated. The third test is the strong one: it proves a malformed neighbor does not suppress pruning of a valid stale edit.
All three hand-build SessionEntry objects. They reproduce the exact inner canonical shape pruning consumes, but none reopens a complete persisted cold-spill record with its entry-level eviction marker — the shape that produced the original report. Not blocking, since the inner shape is what the code reads, but a reopen-based case would be stronger evidence.
sweep
No new any (the ToolCall.arguments: Record<string, any> you see referenced is pre-existing), no ReturnType<>, no inline await import(), no console.* in packages/coding-agent/. Both changelog entries are under ## [Unreleased].
Reviewed by @probepark — method: detached worktree at 650fc184, full read of toolArguments() and every pruning dereference, reachable-shape check against the persistence writer, call/result pairing trace through the pruning path, per-test base-vs-head discrimination. Tests not executed.
gajae.pr-review-verdict.v1 merge-approved sha256:0ea9f8328636d0f81cd97c3ee32e1ffd6d4250928d523d116d2952d743e8e1f2 reviewer:human reviewer-id:probepark evidence:exact-head-650fc184-guard-covers-all-reachable-malformed-shapes-call-result-pairing-preserved
|
Terminal evidence — PR #4633 MERGED to dev. Lane retired.
Complementary context for the record: merged #4625 (write-side cold-spill object invariant, — |
…turn (Yeachan-Heo#4633) A persisted tool call carrying `arguments: null` crashed compaction pruning on every indexing read, taking down the turn. All argument access now goes through a single `toolArguments()` accessor returning `Record<string, unknown>`, which treats null, undefined, empty strings, and other JSON scalars as absent for indexing purposes. The malformed call is excluded from target/staleness indexing only — it is not dropped and neither is its result, so call/result pairing and transcript coherence survive compaction. `/copy cmd` carried the same direct dereference and is hardened alongside it. Reviewed-by: probepark Verdict: merge-approved sha256:0ea9f8328636d0f81cd97c3ee32e1ffd6d4250928d523d116d2952d743e8e1f2 Confidence: high Scope-risk: narrow Tested: null-argument edit-class indexing, tool-output staleness indexing, stale-edit pruning with a malformed neighbor Not-tested: reopen of a complete persisted cold-spill record; /copy cmd command extraction (cherry picked from commit ceb3134)
Problem
Reloading a session whose persisted
toolCall.argumentsisnullkills the turn:The user sees
Error: null is not an object (evaluating 'args.path')with a zero-tokenassistant turn (
stopReason: "error"). The whole conversation stops rather than the oneunusable tool call being skipped.
ToolCall.argumentsis declaredRecord<string, any>— non-nullable — so nothing in thetype system flagged the dereference. The value nevertheless arrives from disk, where an
earlier cold-spill eviction path persisted
nullin place of the__gjcColdSpillArgumentssentinel.Scope, measured on a live store
Scanning 1,282 session files under
~/.gjc/agent/sessions:"arguments": nulltoolCallblocksmessage.content.N.argumentscold-spill refEvery null is paired with a live eviction marker, so this is a broken in-line value, not
lost data. Tool names affected:
bash(1,853),write(1,087),edit(804),task,browser,subagent,todo_write,search,goal.Current eviction code is correct — a probe run confirmed the sentinel survives memory,
disk, and reopen for both long-string and many-small-field argument shapes. This PR is the
missing read-side invariant for sessions already written.
Fix
Route every read of a persisted argument bag through one guard that treats a non-object
payload as absent, instead of scattering
?.at each crash site:Applied to all eight unguarded dereferences in
pruning.ts:path/file_path/filePathextraction,
apply_patchheader parsing (arguments.input), idempotent-bash key building(
command,cwd), and search target keys (pattern,paths,skip,i,gitignore).Fixing only the first site is not enough — after guarding
toolCallPath, the regressiontest failed again inside
editToolPathGroups, thennormalizedIdempotentBashCommand.Also guarded
/copy(command-controller.ts:401), which readtc.arguments.commandwiththe identical shape. It has not thrown yet only because the lookup starts from the newest
turn.
Testing
packages/agent/test/pruning-null-arguments.test.ts— three cases: the argument-pruningpass, the tool-output pruning pass, and a mixed history proving a genuinely stale edit is
still pruned when a null-argument call sits beside it. Reverting the fix reproduces the
exact production string.
Focused suites, all green on this Windows box:
bun test packages/agent/test/pruning-*.test.ts packages/agent/test/maintenance-prune-gate.test.tsbun test packages/coding-agent/test/session-compaction-eviction.test.tstsc --noEmitinpackages/agentandpackages/coding-agentbiome checkon changed filesbun scripts/verify-gjc-state-writers.ts --failgit merge-base --is-ancestor origin/dev HEADEnd-to-end against the real store — open all 43 affected sessions, run the pruning pass,
and check rehydration of every cold-spilled argument payload:
Before the fix the same walk throws on the first affected session. No data loss: 5,175
argument payloads restore from their blobs, zero remain null.
I'd appreciate a CI run on Linux and macOS to confirm.
GJC verdict
Rebased by maintainer onto current dev
2bd7b4a48c(2026-08-18); Dayoooun authorship andoriginal author date preserved. The previous maintainer approval and merge-approved verdict
targeted superseded head
094ff6989fand are withdrawn. No authenticated approving reviewexists for THIS head yet, so per the template this is
needs-humanpending fresh authorizednon-author review.
Maintainer rebase + validation evidence (2026-08-18, head 650fc18, base 2bd7b4a)
2bd7b4a48c(liveorigin/dev, includes fix(session): stop resume listing paying one read syscall per 4 KiB #4616/fix(deep-interview): retire per-round tiered continuation asks for active interviews #4590/fix(team): exclude worker runtime state under .gjc/state from auto-checkpoints #4603/fix(sdk): resolve symlinked workspace cwd scope in SessionRouter endpoint reads (successor of #4639) #4645/fix(session): keep cold-spilled tool arguments an object when the blob is gone #4625/fix(ai): bound the Anthropic connect/headers phase with an SDK request timeout #4666 — inspected, no overlap with this PR scope; fix(session): keep cold-spilled tool arguments an object when the blob is gone #4625 is the complementary write-side cold-spill invariant, touching session-manager.ts only); three-dot binary diff SHA-2560ea9f8328636d0f81cd97c3ee32e1ffd6d4250928d523d116d2952d743e8e1f2(canonicalgit diff --binary --full-index --no-ext-diff 2bd7b4a4...650fc18).Author: Dayoooun <dayoooun@gmail.com>, author date2026-08-17T21:57:29+09:00(identical to original 19577d3). Five-file delta byte-identical in intent to the original; both CHANGELOG adjacencies resolved additively (no sibling entries removed).toolCallPathreadscall.arguments.pathunguarded (verified structurally: base pruning.ts contains notoolArgumentsguard) and throwsnull is not an object (evaluating 'args.path'); full live-store fail-before evidence remains the author's original audit (43 sessions / 3,916 blocks / 0 throws after fix / 5,175 payloads rehydrated).pruning-null-arguments3 pass; pruning red-team/staleness/gate suites +maintenance-prune-gate105 pass;session-compaction-eviction25 pass;bun --cwd=packages/agent run checkclean;bun --cwd=packages/coding-agent run checkclean (biome + tsc);verify-gjc-state-writers --fail0 violations;changelog-history-guard12 pass;git diff --checkclean.dev—
[repo owner's gaebal-gajae (clawdbot) 🦞]