From 53dc3c8a28b7af6b727333cdd2fe553e0bbb2d53 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 23:11:42 +0800 Subject: [PATCH] Harden generator against Codex review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A SessionEventMap member that is not a property signature with an explicit payload type is now a hard error instead of silently skipped — a method-form or type-less member joins keyof SessionEventMap and must not escape the catalog. - A top-level interface SessionEventMap outside @deepseek-ai/dsh-session (ownership read from the package manifest) is now a hard error — an unrelated same-named local interface was previously catalogued as the on-disk vocabulary. - JSDoc tag detection runs on the trimmed line, so an extra-indented '* @mode' can no longer bypass the forbidden-tag check and leak into prose. Four new spec cases cover these; RFC and module doc updated to describe the enforced (not just assumed) invariants. --- .../2026-07-04-persistence-log-catalog.md | 2 +- .../tests/gen-persistence-catalog.spec.ts | 30 ++++++++ scripts/gen-persistence-catalog.ts | 72 +++++++++++++++---- 3 files changed, 91 insertions(+), 13 deletions(-) diff --git a/docs/rfc/implemented/process/2026-07-04-persistence-log-catalog.md b/docs/rfc/implemented/process/2026-07-04-persistence-log-catalog.md index 80e6117c66..b205538979 100644 --- a/docs/rfc/implemented/process/2026-07-04-persistence-log-catalog.md +++ b/docs/rfc/implemented/process/2026-07-04-persistence-log-catalog.md @@ -17,7 +17,7 @@ Specific choices: - **JSDoc completeness, enforced.** Every member must carry description prose — the JSDoc becomes the catalog entry, the same forcing function the cordis catalog applies to bus events. An `@mode` tag on a member is a hard error: dispatch modes belong to cordis bus events, and a log event has none — the tag would misread as "this fires on the bus with mode X". Violations aggregate into one error listing every offender. - **The surface badge is derived, not hand-listed.** `SurfaceEventType` — the subset that produces LLM messages and may carry `surfaceOp` — is parsed from its union declaration in the owning package; a union member naming no declared event is a hard error (a stale union member would otherwise silently badge nothing). Everything else renders **log-only**. - **A dedicated fence.** Payload blocks use a ` ```ts persistence-catalog ` info string that `doc-typecheck` recognizes and skips, excluded from the opt-out ratio — the same treatment as `ts cordis-catalog` (a bare payload fragment is not standalone-compilable). -- **Repo scope.** The catalog enumerates the packages in this repo, matching the siblings' packages-only scope; a downstream plugin can merge further event types, which are outside the catalog by construction. Nothing else in the repo may name an interface `SessionEventMap` — the walk treats every such declaration as the merged vocabulary, and a duplicate member across declarations is a hard error. +- **Repo scope.** The catalog enumerates the packages in this repo, matching the siblings' packages-only scope; a downstream plugin can merge further event types, which are outside the catalog by construction. The walk defends its own assumptions: a top-level `interface SessionEventMap` outside `@deepseek-ai/dsh-session` is a hard error (an unrelated same-named local interface cannot be catalogued as the on-disk vocabulary), a member that is not a property signature with an explicit payload type is a hard error (a method-form member would join `keyof SessionEventMap` yet slip past a silent walk), and a duplicate member across declarations is a hard error. This supersedes the hand-copies: the session.md `hook/*` table, the compact README's event table, the hook-protocol README's payload bullets, and the session README's name-list now link the catalog instead of restating payloads (the surrounding semantics prose stays where it was). The two stray `@mode emit` tags on the hook-protocol merge members are removed — the new gate rejects them as the category error they were. diff --git a/packages/core/session/tests/gen-persistence-catalog.spec.ts b/packages/core/session/tests/gen-persistence-catalog.spec.ts index 567fe0ffd5..b5bec1b92e 100644 --- a/packages/core/session/tests/gen-persistence-catalog.spec.ts +++ b/packages/core/session/tests/gen-persistence-catalog.spec.ts @@ -50,9 +50,13 @@ afterEach(() => { while (roots.length) rmSync(roots.pop()!, { recursive: true, force: true }) }) +/** The manifest that marks a fixture package as the owning session package. */ +const OWNER_MANIFEST = '{ "name": "@deepseek-ai/dsh-session" }\n' + describe('gen-persistence-catalog collectLogEvents', () => { it('extracts a documented member of the owning top-level interface', () => { const events = collectLogEvents(make({ + 'packages/core/fix/package.json': OWNER_MANIFEST, 'packages/core/fix/src/types.ts': 'export interface SessionEventMap {\n /** A thing was recorded. */\n \'fix/happened\': { turn: number }\n}\n', })) @@ -66,6 +70,14 @@ describe('gen-persistence-catalog collectLogEvents', () => { }) }) + it('hard-errors on a top-level interface outside the owning package', () => { + expect(() => collectLogEvents(make({ + 'packages/group/alien/package.json': '{ "name": "@deepseek-ai/dsh-alien" }\n', + 'packages/group/alien/src/types.ts': + 'export interface SessionEventMap {\n /** Not the real vocabulary. */\n \'alien/event\': { turn: number }\n}\n', + }))).toThrow(/top-level interface SessionEventMap .* is outside @deepseek-ai\/dsh-session \(package @deepseek-ai\/dsh-alien\)/) + }) + it('extracts a member declaration-merged via the session module', () => { const events = collectLogEvents(make({ 'packages/group/fix/src/types.ts': merge(' /** Merged provenance. */\n \'fix/merged\': { id: string }'), @@ -95,6 +107,24 @@ describe('gen-persistence-catalog collectLogEvents', () => { }))).toThrow(/carries an @mode tag/) }) + it('hard-errors on an extra-indented @mode tag (does not leak into prose)', () => { + expect(() => collectLogEvents(make({ + 'packages/group/fix/src/types.ts': merge(' /**\n * Documented, but mistagged.\n * @mode emit\n */\n \'fix/indented\': { turn: number }'), + }))).toThrow(/carries an @mode tag/) + }) + + it('hard-errors on a method-form member (it still joins keyof SessionEventMap)', () => { + expect(() => collectLogEvents(make({ + 'packages/group/fix/src/types.ts': merge(' /** Documented, wrong shape. */\n \'fix/method\'(turn: number): void'), + }))).toThrow(/not a property signature with an explicit payload type/) + }) + + it('hard-errors on a property member with no payload type annotation', () => { + expect(() => collectLogEvents(make({ + 'packages/group/fix/src/types.ts': merge(' /** Documented, no payload. */\n \'fix/bare\''), + }))).toThrow(/not a property signature with an explicit payload type/) + }) + it('hard-errors on a non-literal member name', () => { expect(() => collectLogEvents(make({ 'packages/group/fix/src/types.ts': merge(' /** Not a literal. */\n unquoted: { turn: number }'), diff --git a/scripts/gen-persistence-catalog.ts b/scripts/gen-persistence-catalog.ts index 31a1e99ac8..4c58c558d2 100644 --- a/scripts/gen-persistence-catalog.ts +++ b/scripts/gen-persistence-catalog.ts @@ -25,7 +25,12 @@ * (it becomes the catalog entry), and an `@mode` tag on a member is a hard * error — dispatch modes belong to cordis bus events, and a log event has none * (see docs/rfc/implemented/process/2026-07-04-persistence-log-catalog.md). - * Violations aggregate into ONE error listing every offender. + * Structural holes are hard errors for the same reason: a member that is not a + * property signature with an explicit payload type, a top-level + * `interface SessionEventMap` outside the owning package, and a duplicate + * declaration of one event would each let something join (or impersonate) + * `keyof SessionEventMap` without a truthful catalog row. Violations aggregate + * into ONE error listing every offender. * * The surface/log-only badge is parsed from the `SurfaceEventType` union in the * owning package (never hand-listed here), and every union member must name a @@ -156,8 +161,12 @@ function parseJsDoc(raw: string): { doc: string; hasMode: boolean } { para = [] } for (const line of inner) { - if (/^@mode\b/.test(line)) { hasMode = true; flushPara(); inTags = true; continue } - if (line.startsWith('@')) { flushPara(); inTags = true; continue } + // Tag detection runs on the trimmed line: the normalization above strips at + // most one post-`*` space, so an extra-indented `* @mode` still reaches + // here with leading whitespace and must not leak into prose. + const tagLine = line.trimStart() + if (/^@mode\b/.test(tagLine)) { hasMode = true; flushPara(); inTags = true; continue } + if (tagLine.startsWith('@')) { flushPara(); inTags = true; continue } if (inTags) continue // block-tag territory: continuations are never prose if (line.trim() === '') { flushPara(); continue } if (/^-\s+/.test(line)) { @@ -194,28 +203,50 @@ function reportViolations(violations: string[]): void { * top-level declaration (in `@deepseek-ai/dsh-session`) and any declaration * merge inside a `declare module '@deepseek-ai/dsh-session'` block. Both forms * declare members of the SAME merged interface, so both are catalogued - * uniformly; nothing else in the repo may name an interface `SessionEventMap`. + * uniformly. `topLevel` distinguishes the owning form so the caller can verify + * it actually lives in the owning package — an unrelated local interface that + * happens to share the name must not be catalogued as the on-disk vocabulary. */ -function sessionEventMapDecls(sf: ts.SourceFile): ts.InterfaceDeclaration[] { - const decls: ts.InterfaceDeclaration[] = [] +function sessionEventMapDecls(sf: ts.SourceFile): { decl: ts.InterfaceDeclaration; topLevel: boolean }[] { + const decls: { decl: ts.InterfaceDeclaration; topLevel: boolean }[] = [] for (const stmt of sf.statements) { - if (ts.isInterfaceDeclaration(stmt) && stmt.name.text === 'SessionEventMap') decls.push(stmt) + if (ts.isInterfaceDeclaration(stmt) && stmt.name.text === 'SessionEventMap') decls.push({ decl: stmt, topLevel: true }) if (ts.isModuleDeclaration(stmt) && ts.isStringLiteral(stmt.name) && stmt.name.text === SESSION_MODULE && stmt.body && ts.isModuleBlock(stmt.body)) { for (const inner of stmt.body.statements) { - if (ts.isInterfaceDeclaration(inner) && inner.name.text === 'SessionEventMap') decls.push(inner) + if (ts.isInterfaceDeclaration(inner) && inner.name.text === 'SessionEventMap') decls.push({ decl: inner, topLevel: false }) } } } return decls } +/** + * The npm package name owning a `packages///…` source file, read + * from that package's manifest — or null when the manifest is missing or + * unparseable (the caller treats null as "ownership unverifiable"). + */ +function packageNameFor(rel: string, scanRoot: string): string | null { + const dir = rel.split('/').slice(0, 3).join('/') + try { + const manifest = JSON.parse(readFileSync(resolve(scanRoot, dir, 'package.json'), 'utf8')) as { name?: string } + return typeof manifest.name === 'string' ? manifest.name : null + } catch { + // Missing or malformed package.json — every real workspace package has one, + // so this only arises in stripped-down fixture trees; either way ownership + // cannot be verified and the caller reports the declaration. + return null + } +} + /** * Walk every `SessionEventMap` declaration (the owning interface plus every * plugin declaration merge) and extract its events, hard-erroring (aggregated) * on any completeness violation: a member without description prose, an - * `@mode` tag (a category error — log events have no dispatch mode), a - * non-literal member name, or the same event declared twice. + * `@mode` tag (a category error — log events have no dispatch mode), a member + * that is not a property signature with an explicit payload type, a + * non-literal member name, a top-level declaration outside the owning package, + * or the same event declared twice. * `scanRoot` defaults to the repo root; tests pass a fixture dir. */ export function collectLogEvents(scanRoot: string = root): LogEventEntry[] { @@ -227,10 +258,27 @@ export function collectLogEvents(scanRoot: string = root): LogEventEntry[] { const text = readFileSync(abs, 'utf8') if (!text.includes('SessionEventMap')) continue const sf = ts.createSourceFile(abs, text, ts.ScriptTarget.Latest, true) - for (const decl of sessionEventMapDecls(sf)) { + for (const { decl, topLevel } of sessionEventMapDecls(sf)) { + if (topLevel) { + // The top-level form is the OWNING vocabulary; anywhere else, a + // same-named local interface is a different type entirely and must not + // be catalogued as on-disk events. + const pkg = packageNameFor(rel, scanRoot) + if (pkg !== SESSION_MODULE) { + violations.push(`top-level interface SessionEventMap (${pointer(rel, sf, decl)}) is outside ${SESSION_MODULE} (package ${pkg ?? 'unknown'}). Rename the interface, or contribute events via declare module '${SESSION_MODULE}'.`) + continue + } + } for (const member of decl.members) { - if (!ts.isPropertySignature(member) || !member.type) continue const src = pointer(rel, sf, member) + if (!ts.isPropertySignature(member) || !member.type) { + // A method-form or type-less member still joins `keyof SessionEventMap`, + // so skipping it silently would be exactly the undocumented-event hole + // this catalog exists to close. + const label = (member as { name?: ts.Node }).name?.getText(sf) ?? member.getText(sf).replace(/\s+/g, ' ') + violations.push(`SessionEventMap member ${label} (${src}) is not a property signature with an explicit payload type; declare every log event as 'scope/name': .`) + continue + } if (!ts.isStringLiteral(member.name)) { violations.push(`log event at ${src} has a non-literal name; the catalog needs string-literal event names.`) continue