Repository navigation
Session events are validated on read, not on write (invalid payload persists an unreadable session) #11216
Replies: 3 comments
|
Thanks for writing this up — the diagnosis is right, and it is worth being precise about where the seam is, because it decides whether the gap can be closed at all. Where it is. What I built. Measured on a real session from the shipped JSONL store, harness
And the control arm, because it is the point: with the plugin absent, the same append succeeds. The event lands and the log is poisoned on the next cold load. Install (the registry copy is 0.2.0, the diagnostic build; the veto is in 0.3.0, which I can only deliver over git — I have no npm credential in this environment): dsh plugin --profile <profile> add github:argszero/cordis-plugin-session-event-guard#6220dd6Mount entry, after install: What it refuses, and what it leaves alone. The veto refuses the malformed event at On the upstream fix. Issues and pull requests are disabled on this repository, so I can't offer a core patch here — that is a measured statement about the channel, not a deflection. If one were possible, it is one call: run So: this closes the write-side half of what you reported — the malformed event can no longer be persisted through |
|
Hi! I agree that validating events before durable persistence is important. I would also consider the recovery path for sessions that already contain invalid records. Preventing new invalid writes solves only part of the problem. Existing sessions may remain unreadable after the fix is deployed. A useful approach would be:
I would add tests for both normal and interrupted writes, including a malformed message content field, a missing required header field, and a restart after a rejected append. The key invariant should be: a successfully committed event must remain readable by the same version of the session reader. That would address both future prevention and the operational impact on users who already have affected sessions. |
|
Both halves are worth separating, because one of them I can answer from the code and the other I cannot. Your item 4 is not the same program as items 1-3 and 5. Prevention is a veto on the write path; recovery is a reader that survives a log it cannot fully validate. A write-side veto makes new corruption zero — it does nothing for the logs already on disk, which is exactly the operational half you are pointing at. What the reader does today, read at
So your item 3 is half-met, and the half that is missing is the one you named second: nothing on any of the three paths names the producer. The type appears on two of three paths, the position on all three, and the package or plugin that wrote the event on none of them. That is a real gap and it is core-side — no plugin can add it, because the throw happens inside a validation function the plugin does not own. Item 4, honestly. Item 5 is right and is not automatic. Whatever repair eventually exists has to copy the original log before it rewrites anything, because the failure mode you are guarding against is a tool that "fixes" a session into a different, also-unreadable one. What actually exists on my side, with its boundary. On your closing invariant. "A successfully committed event must remain readable by the same version of the session reader" is the right statement of the property, and it is a round-trip claim rather than a validation claim: it can only be checked by writing and then reading back, which is what the veto does. The corresponding read-side obligation — that the reader says which event, of which type, from which producer — is the part still open, and your item 3 is the shortest path to closing it. |
Uh oh!
There was an error while loading. Please reload this page.
[Bug] Session events are validated on read, not on write — an invalid payload persists an unreadable session
Summary
A session event that violates the log's own invariants can be appended successfully. The failure
only surfaces later, when the session is opened:
contentis not a parts array → every following turn throwsTypeError: content.some is not a function, and reloading the session reports it as corrupt(
message has invalid content);cwd→ the session is listed normally, but opening it returnssession/not-found.The validators already exist (
assertMessageEventShape,validateSessionHeader); they run when anevent is adopted/snapshotted, and again when the log is read — but not at the persistence boundary.
So the error is attributed to the reader ("corrupt", "not found") instead of the producer that
wrote it, and the session cannot be repaired from the UI.
Where it happens
Message shape —
assertMessageEventShape(packages/core/session/src/index.ts:332) is invoked byadoptSessionEvent(:167) andsnapshotSessionEvent(:196). The agent inbox path does not reacheither: it validates only message-id uniqueness (
packages/core/agent-loop/src/inbox.ts:223) andthen persists the batch verbatim (
inbox.ts:235):createUserMessageis generic over its input (packages/llm/llm/src/message.ts:246-253) and does notnormalize
content, so a non-array value survives into the log.Missing
cwd—projectDir(root, undefined)deliberately maps tosessions/_no-cwd/(
packages/session/session-persistence-jsonl/src/format.ts:255), so such a header can be written andis returned by the session listing;
historythen rejects it outright(
packages/api/session-controller/src/history.ts:254-257). The bucket is writable, listable andunreadable at the same time.
Reproduction
1. Wrong
contentshape — any in-process producer handing the agent a message whosecontentisnot an array:
Observed in a real log (
0.2.0-rc.2, format v4): the turn ends with{"kind":"error","error":{"message":"content.some is not a function"}}, and after a restart thesession reports:
The record:
{"type":"user/message","seq":N, "data":{"content":"…","source":{"kind":"my-plugin"},"role":"user","id":"…"}}2. Header without
cwd:All four appear in the session list; none can be opened. (These were produced by a local import path
that materialised conversations from another product, where "project directory" has no meaning — but
the inconsistency is on the DSH side: the write is accepted, the read is refused.)
Suggested hardening
assertMessageEventShapebefore anevent is appended, so a malformed producer fails immediately and the error names the producer.
Valid producers are unaffected; invalid data simply cannot be committed.
_no-cwdself-consistent. Either reject headers withoutcwdat write time, or lethistoryload them — reading a log does not require a project directory.Why we think it matters for the plugin ecosystem
DSH's premise is "everything is a plugin", and plugins are plain JavaScript loaded at runtime, so
TypeScript types are erased exactly at that boundary. Today a single wrong field in a plugin turns
into a conversation the user can never open again, discovered hours later with no pointer to the
cause. One write-side check converts that class of failure into an immediate, attributable error.
Environment
dsh 0.2.0-rc.2, Node v26.x, Arch Linux, session format v4, profilewebsession.v4.jsonl.zstdunder~/.dsh/sessions/<project>/<id>/All reactions