From 1bb201365d30bb5e4216fc71fbacf074dceb07b4 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 20 Jun 2026 12:42:53 +0800 Subject: [PATCH 1/2] docs(agents): add doc-current-state convention + sharpen the summary worked example (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings on the AGENTS.md additions: - Add a convention to § Type Safety and Documentation: document the CURRENT state (what + why), never the PROCESS/HISTORY of how the code got there. No "previously/now/used-to/replaces/the old X" in comments or JSDoc — that rots on the next change and belongs in the commit message / PR / RFC. A standing contrast against a live alternative is fine; a contrast against the codebase's past is not. - The "tests document behavior" worked example overstated the audit as "nothing in production read or wrote" the summary. The backends DID write it (JSONL sidecar, SQLite updated_at); what made it dead was no CONSUMER and no update() caller. Corrected so a future reader does not infer the write path never existed. --- AGENTS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1bb32121e9..9c584b2072 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -14,7 +14,7 @@ A passing test pins the behavior the code **currently** has — not necessarily Before you preserve a behavior solely to keep a test green, ask: is this behavior load-bearing (a real consumer depends on it, a contract promises it, a user observes it), or is it an artifact? If it's an artifact, **change the behavior AND its test together, in the same change, and say why in the PR** — do not contort new code to keep an obsolete assertion passing, and do not treat "but the test expects X" as a reason X must stay. Conversely, do not delete a test just because it is inconvenient: the discipline cuts both ways — you must show the *behavior* is dead, not merely that the test is in your way. -The worked example is [Drop the mutable session summary](docs/rfc/implemented/2026-06-19-drop-mutable-session-summary.md): an entire `SessionSummary` type, a `SessionPersistence.update()` method, a JSONL sidecar, and SQLite columns existed and were exercised by their own contract test — yet nothing in production read or wrote any of it. The tests documented the behavior perfectly; the behavior was dead. Deleting the behavior and its tests together removed ~400 lines and erased a durability divergence the next refactor would have had to model. (This is the test-tier echo of "verify the world, not a synthetic stand-in" in § Defensive patterns: a test agrees with whatever it was written to assert; only a real consumer proves the behavior matters.) +The worked example is [Drop the mutable session summary](docs/rfc/implemented/2026-06-19-drop-mutable-session-summary.md): an entire `SessionSummary` type, a `SessionPersistence.update()` method, a JSONL sidecar, and SQLite columns existed and were exercised by their own contract test — yet **nothing in production CONSUMED any of it, and `update()` had no production caller**. (The backends did *write* summary state — JSONL touched the sidecar after a durable append, SQLite bumped `updated_at` in the append transaction — but those writes fed only reads that nothing performed.) The tests documented the behavior perfectly; the behavior was dead. Deleting the behavior and its tests together removed ~400 lines and erased a durability divergence the next refactor would have had to model. (This is the test-tier echo of "verify the world, not a synthetic stand-in" in § Defensive patterns: a test agrees with whatever it was written to assert; only a real consumer proves the behavior matters.) ## Architecture @@ -176,7 +176,7 @@ In the **core** packages (`packages/llm`, `packages/tools`, `packages/agent`, `p Verbose documentation is fine **as long as docs and code stay strictly in sync**. Out-of-sync docs are worse than no docs. **When you change code, update its docs in the SAME change** — grep the package README and the module/JSDoc comments for the old behavior (config keys, defaults, error codes, wire field names, event names) and fix every hit. CI runs `pnpm run doc-sync` (`doc-typecheck` + `verify-event-taxonomy` + `verify-md-wrap` + `verify-md-links`), which typechecks every fenced `ts` block in `README.md`, `docs/**/*.md`, and `packages/*/README.md`, verifies the event-taxonomy table against source, asserts no hard-wrapped prose paragraphs, and checks that every relative Markdown cross-link resolves — across those files plus `AGENTS.md` / `packages/AGENTS.md` — but that scope does NOT catch prose drift in `AGENTS.md` / `packages/AGENTS.md` / `packages/README.md` (config keys, defaults, error codes), so keeping those in sync remains on the author. Every module has a module-level doc comment explaining its role. Every exported class, interface, type, function, and non-obvious method has a JSDoc that explains semantics (not just the name) — contracts (what events fire when), disposal behavior, error behavior, and extension intent. Internal helpers get docs only where non-obvious. Prefer one-liners when one line suffices. -**Write an RFC when — and only when — a PR makes a decision that is durable, contested, and surprising.** RFCs (`docs/rfc/`, grouped into `proposed/` / `implemented/` / `rejected/`) record the *why* behind choices a future reader would otherwise re-litigate (the vendoring policy, event-sourcing, the schema DSL are the existing examples). A PR that introduces such a decision — a new third-party runtime dependency over the vendoring default, a cross-package contract, a security/isolation model, a deviation from a documented architecture rule — writes the RFC in `implemented/` **in the same PR**, and links it from the relevant code. A proposal for future work not yet built goes in `proposed/`. A PR whose changes are mechanical, self-evident, or already covered by an existing RFC needs none — do not manufacture an RFC for a routine change. When unsure, the test is: would a competent maintainer six months from now ask "why was it done this way?" and be unable to answer from the code alone? If yes, write it. See [docs/rfc/README.md](docs/rfc/README.md) for the naming scheme and [docs/AGENTS.md](docs/AGENTS.md) for the cross-link convention. +**Document the CURRENT state — the "what" and "why" — never the PROCESS or HISTORY of how it got there.** A comment, JSDoc, or doc paragraph describes what the code *is* and why it is that way, as if it had always been so. Do NOT narrate the change that produced it: no "previously X, now Y", "changed from", "used to", "this replaces", "the old map", "renamed", "moved here", "as of this PR", or "(was …)". Such phrasing rots the instant the next change lands, and a reader of the current code does not need the diff narrated in prose — that belongs in the commit message, the PR description, or an RFC (the durable home for "why we moved away from X"). Write "the owner token lives on the task in the executor" — not "ownership *now* lives on the executor instead of a plugin-local map". When a contrast genuinely aids understanding (a non-obvious choice between live alternatives), frame it against the alternative as a standing fact ("stored on the executor, NOT the tool plugin, so it survives an HMR reload"), not against the codebase's past. The same rule governs review-fix commits: the *commit message* records what the review caught; the *code comment* it touches states only the resulting truth. RFCs (`docs/rfc/`, grouped into `proposed/` / `implemented/` / `rejected/`) record the *why* behind choices a future reader would otherwise re-litigate (the vendoring policy, event-sourcing, the schema DSL are the existing examples). A PR that introduces such a decision — a new third-party runtime dependency over the vendoring default, a cross-package contract, a security/isolation model, a deviation from a documented architecture rule — writes the RFC in `implemented/` **in the same PR**, and links it from the relevant code. A proposal for future work not yet built goes in `proposed/`. A PR whose changes are mechanical, self-evident, or already covered by an existing RFC needs none — do not manufacture an RFC for a routine change. When unsure, the test is: would a competent maintainer six months from now ask "why was it done this way?" and be unable to answer from the code alone? If yes, write it. See [docs/rfc/README.md](docs/rfc/README.md) for the naming scheme and [docs/AGENTS.md](docs/AGENTS.md) for the cross-link convention. **Markdown is not hard-wrapped**: write one line per paragraph and let the editor soft-wrap. Hard line breaks mid-paragraph make docs harder to edit and diff — a one-word change reflows and re-diffs the whole paragraph. This applies to prose only: leave fenced code blocks, tables, and list structure intact (a wrapped list item folds to one line per bullet). Code comments / JSDoc are exempt — they stay under the linter's column limit. `pnpm run verify-md-wrap` (part of `doc-sync`) enforces this across `README.md`, `docs/**/*.md`, `packages/*/README.md`, and `AGENTS.md` / `packages/AGENTS.md`; `pnpm run verify-md-links` (also part of `doc-sync`) checks that every relative cross-link in those files resolves. From 301a3d12338ecaba475342655140f49e3f61bca1 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 20 Jun 2026 12:50:55 +0800 Subject: [PATCH 2/2] fix(session-persistence): scope the ownerless-state claim to the cwd (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reviewer found a cross-cwd hole: the ownerless-state claim path validated only the seed prefix (via loadStored, any scope) and never compared the tracked header's cwd to the live session's. So an ownerless `create(meta(id, "/a"))` with cursor 0 (seed matches trivially) was claimed by a live session with the same id at cwd "/b", and the "/b" events then appended under the "/a" header — bypassing the cwd-scoped loadLive() guard that the HMR-adopt path (case 2) uses. Add a cwd equality check before the seed check in the ownerless-claim branch: a same-id ownerless artifact at a different cwd is a collision, not a claim. This is a coordinator-level invariant (the live session's cwd must match the tracked meta's cwd) and applies to both backends. Tests (shared coordinator contract, run per backend): a live session at a different cwd cannot claim cursor-0 ownerless state, cannot claim a loaded-prefix even when the seed matches, and a no-cwd state cannot be claimed by a cwd'd session. All fail without the guard. Also documents WHY the `materialized` flag is needed (lazy create leaves no artifact; it distinguishes registered-but-unwritten from durably-present for has()/reclaim) and reframes the module doc to current-state, not the refactor history (per the new AGENTS.md doc convention). --- .../session-persistence/src/coordinator.ts | 54 +++++++++++++------ .../tests/coordinator-contract.ts | 53 ++++++++++++++++++ 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/packages/session-persistence/src/coordinator.ts b/packages/session-persistence/src/coordinator.ts index f35685c677..5371873097 100644 --- a/packages/session-persistence/src/coordinator.ts +++ b/packages/session-persistence/src/coordinator.ts @@ -2,21 +2,20 @@ * The backend-agnostic write-path orchestration shared by every first-party * {@link SessionPersistence} backend. * - * The two durable backends (`dsh-session-persistence-jsonl` over file bytes, - * `dsh-session-persistence-sqlite` over `node:sqlite` rows) were byte-identical - * — or same-algorithm — for ALL of their orchestration: the in-memory - * bookkeeping (the per-id state, the write-behind buffers, the per-id - * serialization chains, the per-session init promises), the `session/event` → - * buffer → `session/flush` drain, lazy materialization, crash-tail repair on - * load, the four `session/created` adoption cases (new / HMR-adopt / collision / - * ownerless-claim), and dispose-time quiescence. Only the STORAGE primitives - * differed (write bytes vs. INSERT rows). {@link PersistenceCoordinator} owns - * the orchestration once; a backend supplies the storage primitives as a small + * Every durable backend needs the same orchestration: the in-memory bookkeeping + * (the per-id state, the write-behind buffers, the per-id serialization chains, + * the per-session init promises), the `session/event` → buffer → `session/flush` + * drain, lazy materialization, crash-tail repair on load, the four + * `session/created` adoption cases (new / HMR-adopt / collision / + * ownerless-claim), and dispose-time quiescence. Only the STORAGE primitives are + * backend-specific (file bytes for `dsh-session-persistence-jsonl`, `node:sqlite` + * rows for `dsh-session-persistence-sqlite`). {@link PersistenceCoordinator} owns + * the orchestration; a backend supplies the storage primitives as a small * {@link PersistenceBackend} hook object. * - * The abstract {@link SessionPersistence} service's public API is unchanged: a - * backend still IS a `SessionPersistence` (its six public methods delegate to a - * coordinator it composes), so a third-party backend MAY implement the service + * The abstract {@link SessionPersistence} service's public API is independent of + * this: a backend IS a `SessionPersistence` (its six public methods delegate to + * a coordinator it composes), so a third-party backend MAY implement the service * directly without using the coordinator at all. * * See the write-coordinator RFC (docs/rfc/implemented/2026-06-18-shared-persistence-write-coordinator.md) @@ -115,7 +114,19 @@ interface SessionState { meta: SessionHeader /** The next seq the backend expects to append (the stored log length). */ cursor: number - /** Whether the session has been physically materialized. */ + /** + * Whether the backend has physically written this session (a JSONL file / + * SQLite row exists). `create()` registers state LAZILY — cursor 0, + * materialized false, nothing on disk — so an empty session leaves no + * artifact and the FIRST `appendBatch` writes the header + its events in ONE + * transaction (the "a row exists ⇔ it has events" invariant `has`/`list` + * rely on; a separate up-front materialize could crash leaving a row with + * zero events). The flag is the only signal that distinguishes a session + * registered-but-never-written from one durably present, which two callers + * need: `has()` (lazy-but-unwritten is not yet durable) and the reclaim path + * (an abandoned id with no artifact AND no buffered events is free to reuse; + * a materialized one is a real collision). + */ materialized: boolean /** * The live Session this state was bound to via `onCreated`, if any. State @@ -450,9 +461,18 @@ export class PersistenceCoordinator { if (tracked.owner === session) return if (tracked.owner === undefined) { // Ownerless state from the public create()/load() API. The FIRST live - // session claims it — but ONLY if its seed reproduces the persisted - // prefix (else a fresh, unrelated session reusing the id would have its - // seq 0..cursor-1 events filtered as already-written and grafted on). + // session claims it — but ONLY if BOTH the cwd scope and the seed match. + // The cwd guard mirrors case-2's cwd-scoped loadLive(): a same-id + // ownerless artifact at a DIFFERENT cwd is a collision, not a claim + // (claiming it would append the live cwd's events under the stored + // header's cwd, the exact cross-cwd corruption the loadLive scope + // prevents). The seed guard then ensures the live events reproduce the + // persisted prefix (else a fresh, unrelated session reusing the id would + // have its seq 0..cursor-1 events filtered as already-written and + // grafted on). + if (tracked.meta.cwd !== session.header.cwd) { + throw new Error(`session "${id}" is already persisted at a different cwd (persisted: ${String(tracked.meta.cwd)}, live: ${String(session.header.cwd)}) (id collision)`) + } if (!await this.seedMatchesPersisted(id, seed, tracked.cursor)) { throw new Error(`session "${id}" is already persisted with ${tracked.cursor} event(s) that do not match this live session (id collision)`) } diff --git a/packages/session-persistence/tests/coordinator-contract.ts b/packages/session-persistence/tests/coordinator-contract.ts index 24a8f4c0cf..7769e6f5ec 100644 --- a/packages/session-persistence/tests/coordinator-contract.ts +++ b/packages/session-persistence/tests/coordinator-contract.ts @@ -68,6 +68,7 @@ export interface CoordinatorFixture { /** A constant absolute cwd; jsonl keys directories off it, memory/sqlite ignore it. */ const WORK = '/w' +const OTHER = '/other' /** The per-session init map a backend exposes for white-box init awaits. */ function inits(persistence: SessionPersistence): Map> { @@ -535,6 +536,58 @@ export function runCoordinatorContract(name: string, makeFixture: () => Promise< } }) + it('a live session at a DIFFERENT cwd cannot claim cursor-0 ownerless state (cwd scope)', async () => { + const fix = await makeFixture() + const { ctx, fiber } = await freshCtx(fix) + try { + // create() registers ownerless state at cwd /a (cursor 0 — claims would + // otherwise match trivially on the seed). + await ctx.sessionPersistence.create(meta('wrong-cwd-claim', OTHER)) + // A live session reusing the id but at cwd WORK must NOT claim it — the + // cwd scope is the fence (without it, WORK events would append under the + // OTHER header). Rejected as a collision. + const live = ctx.sessions.create('wrong-cwd-claim', { seed: oneTurnLog(), meta: { cwd: WORK } }) + await expect(inits(ctx.sessionPersistence).get(live)).rejects.toThrow(/different cwd|id collision/) + } finally { + await fiber.dispose() + await fix.cleanup() + } + }) + + it('a live session at a DIFFERENT cwd cannot claim loaded-prefix ownerless state (cwd scope)', async () => { + const fix = await makeFixture() + const { ctx, fiber } = await freshCtx(fix) + try { + // Materialize + load at cwd OTHER (ownerless, cursor = 6). + await ctx.sessionPersistence.create(meta('wrong-cwd-load', OTHER)) + await ctx.sessionPersistence.append(SessionId('wrong-cwd-load'), oneTurnLog()) + const { events } = await ctx.sessionPersistence.load(SessionId('wrong-cwd-load')) + // A live session whose SEED matches the loaded prefix but whose cwd is + // WORK must still be rejected — the cwd guard runs before the seed check. + const live = ctx.sessions.create('wrong-cwd-load', { seed: events, meta: { cwd: WORK } }) + await expect(inits(ctx.sessionPersistence).get(live)).rejects.toThrow(/different cwd|id collision/) + } finally { + await fiber.dispose() + await fix.cleanup() + } + }) + + it('a no-cwd ownerless state cannot be claimed by a live session WITH a cwd (cwd scope, undefined side)', async () => { + const fix = await makeFixture() + const { ctx, fiber } = await freshCtx(fix) + try { + // Ownerless state created WITHOUT a cwd (the no-cwd bucket). + await ctx.sessionPersistence.create(meta('no-cwd-state')) + // A live session reusing the id but WITH cwd WORK is a cwd mismatch + // (undefined vs WORK) and must be rejected. + const live = ctx.sessions.create('no-cwd-state', { seed: oneTurnLog(), meta: { cwd: WORK } }) + await expect(inits(ctx.sessionPersistence).get(live)).rejects.toThrow(/different cwd|id collision/) + } finally { + await fiber.dispose() + await fix.cleanup() + } + }) + // --- append adopts a storage-only session (fresh instance, no prior create/load) --- it('append adopts a storage-only session (fresh instance) and continues the seq', async () => {