feat(session): append-only per-session transcript, preserved across compaction (#441) - #457
Merged
Merged
Conversation
Corrects a false premise. #438 (issue #410) shipped a prompt line telling the model that the session file preserves what compaction discards. It does not: `.graff/sessions/<name>.session.json` is a SINGLE JSON object whose `messages` array is rewritten in place, so the next autosave after a compaction drops the pre-compaction history permanently. "Grep your own conversation log" therefore had no graff equivalent — the trace and trajectory JSONLs record events for telemetry, not the conversation for recall. `.graff/sessions/<name>.transcript.jsonl` is that equivalent: one line per message as it FIRST enters history, appended by a single positional write at the end of file and never rewritten. A line is the message's provider-native JSON verbatim (JSON escapes every control character, so a message is always exactly one line), which makes the file greppable with no tooling and makes a resume's re-seed exact — hashing a line reproduces the digest that wrote it. Hooked at session.queueSave, the point where the autosave already observes a new message, so none of the history's ~25 mutation sites needs a hook of its own. Identity is a MULTISET of the digests on disk, not a position: compact() builds `[handoff summary] ++ recent_messages`, putting the summary at the front of the history while it is the last line in the file, and any position-based match would re-append the retained tail behind it. Counting instead records the summary alone. An ordinary turn takes a fast path (two serializations plus one per new message) and only a rewrite, a resume, or a session's first append pays for a full walk. Bounds, as the issue asks: - Subagents excluded. Their history is never persisted, so there is no durable session to attach a transcript to — #409's rule, unchanged. - One lifecycle, not two. #409's sweep now reclaims transcripts and artifact dirs together (tool_spill.sweepSessionsOnce), by the same rule — the session file is the ground truth for "this session is gone" — and the same grace window. It now also runs at the first transcript append, so a run that never spills still collects what deleted sessions left behind. - The size cap ROTATES rather than head-truncates. Head-truncation means reading the file, dropping a prefix and writing what is left back over it: an in-place rewrite of the history, which is the exact failure mode this change exists to fix, and one a crash halfway through turns into total loss. A rename is one atomic syscall, the old bytes survive intact under a sibling name the model greps identically, and disk use is bounded at two generations (16 MiB each) rather than merely slowed. Tests 1043 -> 1050. The rewrite case asserts the pre-compaction file is an exact byte PREFIX of the post-compaction one and that the discarded detail is still greppable; repeated autosaves over an unchanged history add nothing; a resume re-seeds from disk instead of re-appending its restored history; a subagent writes nothing; rotation leaves the previous generation byte-identical. Verified end to end against a mock provider: two processes, four turns, four lines, no duplicates. session_transcript.activePath(root, arena) is the accessor #411's post-compaction note should use — null for a subagent or a session with no transcript, so the note can never cite a file that does not exist, with lineCount() for its "N messages". Co-Authored-By: Codegraff <blackfloofie@codegraff.com>
One genuine portability defect, not five test bugs. Every Windows failure —
four wrong line counts and one null deref — is the same root cause, and the
CI log names it: each count was "found 0", and activePath was null because
nothing had been recorded.
`appendWhole` opened the file write-only and then called `File.stat` on that
handle to find the append offset. On Windows, Io.Dir.createFile maps `read`
straight onto the NT access mask (`GENERIC = .{ .WRITE = true, .READ =
flags.read }`), so a write-only handle carries FILE_GENERIC_WRITE, which does
NOT include FILE_READ_ATTRIBUTES. `File.stat` is NtQueryInformationFile(.All),
which requires exactly that right, and std handles the resulting ACCESS_DENIED
explicitly. So on Windows the stat failed, `appendWhole` returned null having
ALREADY created the file, and every transcript was a 0-byte file: the feature
was inert on the platform, and #411's note would have cited an empty file.
`.read = true` on the create is the fix; a path-based `statFile` fallback keeps
a future std change from turning this back into a silent no-write rather than
an append. Nothing else about the write changed — it is still one positional
write at the end of file, still append-only.
This class is invisible to the POSIX suite: with the fix reverted, macOS still
runs 1050/1050 green. CI was the only possible signal, which is the argument
for the two hardenings below rather than for trusting a local pass.
The other two suspects, checked and reported rather than assumed:
- Line endings: NOT a defect. '\n' is written explicitly and JSON escapes
every control character, so a message is always exactly one line and no
'\r' can appear. Hardened anyway, since the digests are over line bytes and
a stray '\r' would silently re-append the entire history: `seed` trims a
trailing '\r', and the tests now fail hard if a '\r' ever reaches the file.
- Rotation's rename: NOT a defect. Windows rename does not overwrite, but
Io.Dir.rename is the REPLACING variant on every OS (dirRenameWindows passes
replace_if_exists=true; renamePreserve is the one that refuses a taken
name). The rotation test only ever rotated onto a free name and could not
have told the difference, so it now rotates twice and asserts the previous
generation really was replaced.
INVARIANT, now stated where the paths are built (session_index.zig) because two
downstream branches depend on the answer: every `.graff/` path this harness
builds is forward-slashed on every platform, Windows included. Deliberately —
Windows accepts '/' in the paths reaching Io.Dir, and these strings are shown to
the model (#410's prompt line, #409's cap marker, #441's path inside #411's
note), so a shape that changes per platform buys nothing and costs goldens.
Separators were never the Windows failure here; the null deref was a downstream
symptom of the empty file. The corollary is for tests, and ee28d8c is the
precedent: assert on a basename or on a path built through the helpers, never by
matching a separator by hand. Both `activePath` assertions now go through the
accessor and check the cited file exists; the readers build their paths with
`transcriptPath`/`rotatedPath` so a test can no longer disagree with the code.
The tests moved to session_transcript_tests.zig: the hardening pushed the module
to 614 lines, over the ceiling. Reachability needed the `_ = ...` line in
test_hooks.zig's test block, not just the import — the count caught it at 1045
before it was added, and is 1050 again after.
Co-Authored-By: Codegraff <blackfloofie@codegraff.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #441. Corrects a false premise shipped in #438/#410.
The false premise
That PR's prompt line told the model the session file preserves what compaction discards. It does not.
.graff/sessions/<name>.session.jsonis a single JSON object whosemessagesarray is rewritten in place; the next autosave after a compaction permanently drops the pre-compaction history. So "grep your own conversation log" had no graff equivalent, and the traces/trajectories JSONLs serve telemetry, not recall.This adds
.graff/sessions/<name>.transcript.jsonl: one line per message as it first enters history, never rewritten, unaffected by compaction.How append-only-across-a-rewrite is proved
The test snapshots the whole file, then replaces
root.messageswithcompact()'s real shape ([handoff summary] ++ recent_messages, tail verbatim), records again, and asserts the pre-rewrite file is an exact byte prefix of the post-rewrite one. It further asserts the discardedld: symbol(s) not found …string is still greppable afterwards, the retained tail appears exactly once, and one more turn keeps the chain prefixed.The bug that shaped the design
The first implementation matched messages by position, forward-only, and failed that compaction test.
compact()puts the summary at the front of the history while it is the last line in the file, so position matching re-appended the retained tail behind it. Identity is therefore a multiset of line digests, not a position. Documented trade-off: a message identical to one whose every copy compaction already discarded is not written again. The bytes are in the file either way; only exact multiplicity is affected.Rotation, not head-truncation
16 MiB per generation; at the cap the live file is
renamed to<name>.transcript.1.jsonland a fresh one starts.Head-truncation would mean reading the file, dropping a prefix, and writing the remainder back over it — an in-place rewrite of history, which is the precise failure mode this feature exists to fix, and a crash halfway through loses everything. A rename is one atomic syscall: the old bytes survive intact under a sibling name the model greps identically, and disk is genuinely bounded at two generations rather than merely slowed.
Bounds, per the issue
tool_spill.sweepSessionsOncenow reclaims artifact dirs and transcript generations by the same rule (the session file is ground truth) with the same 1-hour grace, and it now also fires on the first transcript append, so runs that never spill still collect orphans./save <other>and/clearneed no special handling: re-attaching to a new name re-seeds from that name's file, and the orphan is swept.Accessors for #411
activePath(root, arena)is the one to call — it returns null unless a transcript is actually live for that agent's session, so the post-compaction note can never cite a file that does not exist. AlsolineCount(),transcriptPath(),rotatedPath().Verification
zig build test→ 1050/1050, exit 0 (baseline 1043, +7 exactly matching the tests added), re-run from a brand-new cache directory to rule out a stale-cache false green. Reachability confirms 1015 declared tests compiled in. One assertion was deliberately broken to confirm the suite goes red at that line, then reverted.scripts/eval-tier1.shgreen.End-to-end with the real binary against a mock provider: a one-shot run wrote the transcript with 2 correct JSONL lines beside the session file, and a second process with
--resumegrew it to exactly 4 lines with zero duplication of the restored history — proving the seed-from-disk path in production rather than only in tests.Not exercised
Real compaction was modelled in tests, not triggered live; likewise
emergencyTrimandcapOversizedToolOutputs. Rotation was tested at a shrunk cap, not a true 16 MiB. Concurrent sessions in one process undergraff serveare guarded by anIo.Mutexbut untested.src/session.zigis at 590/600, so the next change there needs an extraction first.