docs: fix Codex review findings on the cordis catalog
- Exclude protected methods from the generated service interface: a protected member (e.g. BashExecutor.notifyTaskDone) is a subclass hook, not part of the public ctx.<key> surface a plugin author calls. The method filter now drops private, protected, and static. - Add BashTaskRead to the type cross-link map so readOutput()'s return type links to its core-data-structures page. - Reword the generator module comment and the AGENTS.md @mode rule to state the current capability without narrating the retired event-taxonomy verifier (that history lives in the RFC).
This commit is contained in:
3 files changed
+21
-16
No files matched your search
@@ -177,7 +177,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-cordis-catalog` + `verify-md-wrap` + `verify-md-links` + `verify-type-equiv`), which typechecks every fenced `ts` block in `README.md`, `docs/**/*.md`, and `packages/*/*.md`, regenerates the cordis events/services catalog from source and fails if the committed copy is stale, asserts no hard-wrapped prose paragraphs, checks that every relative Markdown cross-link resolves, and checks that every ` ```ts type-equiv ` doc block still matches its source type — 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.
|
||||
|
||||
**Tag every new event with `@mode`.** The cordis events/services catalog ([docs/cordis-catalog/events-and-services.md](docs/cordis-catalog/events-and-services.md)) is GENERATED from source by `scripts/gen-cordis-catalog.ts` — never hand-edit it; run `pnpm run gen-cordis-catalog` and commit the result. When you add an event to an `interface Events` block, its JSDoc MUST carry a `@mode emit|waterfall|parallel` tag (the generator hard-errors without it): use `waterfall` when the signature ends with a `next: () => …` parameter (the listener transforms or vetoes via `next()`), `parallel` when the loop awaits a fan-out with no veto (e.g. an awaited `Promise<void> | void` checkpoint like `session/flush`), and `emit` for plain fire-and-forget notifications. The generator also cross-checks the tag against the signature where the shape is conclusive (a trailing `next` ⇒ waterfall) and hard-errors on a contradiction. Write the rest of the event's JSDoc to stand alone — it is the catalog entry's prose. The generated catalog is what supersedes the old hand-maintained event-taxonomy table.
|
||||
**Tag every new event with `@mode`.** The cordis events/services catalog ([docs/cordis-catalog/events-and-services.md](docs/cordis-catalog/events-and-services.md)) is GENERATED from source by `scripts/gen-cordis-catalog.ts` — never hand-edit it; run `pnpm run gen-cordis-catalog` and commit the result. When you add an event to an `interface Events` block, its JSDoc MUST carry a `@mode emit|waterfall|parallel` tag (the generator hard-errors without it): use `waterfall` when the signature ends with a `next: () => …` parameter (the listener transforms or vetoes via `next()`), `parallel` when the loop awaits a fan-out with no veto (e.g. an awaited `Promise<void> | void` checkpoint like `session/flush`), and `emit` for plain fire-and-forget notifications. The generator also cross-checks the tag against the signature where the shape is conclusive (a trailing `next` ⇒ waterfall) and hard-errors on a contradiction. Write the rest of the event's JSDoc to stand alone — it is the catalog entry's prose.
|
||||
|
||||
**The core-data-structures catalog is a maintained surface, not a write-once artifact.** [docs/core-data-structures/](docs/core-data-structures/core.md) catalogs the spine vocabulary (core.md) and the per-seam types (sub-pages). When a change adds, removes, or reshapes a type the catalog documents — a new `…Map` variant, a new content-block or session-event type, a field on `GenerateOptions`/`Agent`/`ToolDefinition`/a bash type, or a whole new core/seam type — update the catalog in the SAME change: edit the prose, and for a pasted ` ```ts type-equiv ` block, re-copy it verbatim and keep `scripts/type-equiv.manifest.json` 1:1 with the blocks. The `verify-type-equiv` gate catches a *drifted paste* of an already-documented type, but it canNOT tell you a brand-new core type was never documented — that judgment is on the author and the reviewer. The definition of "core" (the spine-vs-seam line) is in [core.md § What counts as "core"](docs/core-data-structures/core.md#what-counts-as-core); a genuinely spine-level new type belongs in core.md, a new capability's vocabulary on a sub-page. See [development.md](docs/development.md#documenting-types-verbatim-ts-type-equiv) for the `ts type-equiv` mechanics.
|
||||
|
||||
|
||||
@@ -355,10 +355,9 @@ abstract list(): BashTask[]
|
||||
abstract readOutput(id: string): BashTaskRead
|
||||
abstract kill(id: string): boolean
|
||||
onTaskDone(listener: BashTaskListener): () => void
|
||||
protected notifyTaskDone(task: BashTask): void
|
||||
```
|
||||
|
||||
Types: [BashExecRequest](../core-data-structures/bash.md) · [BashExecSpec](../core-data-structures/bash.md) · [BashRunResult](../core-data-structures/bash.md) · [BashTask](../core-data-structures/bash.md)
|
||||
Types: [BashExecRequest](../core-data-structures/bash.md) · [BashExecSpec](../core-data-structures/bash.md) · [BashRunResult](../core-data-structures/bash.md) · [BashTask](../core-data-structures/bash.md) · [BashTaskRead](../core-data-structures/bash.md)
|
||||
|
||||
Source: [`packages/bash/src/index.ts:58`](../../packages/bash/src/index.ts)
|
||||
|
||||
|
||||
@@ -7,16 +7,15 @@
|
||||
* can call (exact public interface). It complements the core-data-structures
|
||||
* catalog (the VOCABULARY axis — the types these signatures move around).
|
||||
*
|
||||
* Unlike the core-data-structures docs (a hand-paste drift-checked by
|
||||
* verify-type-equiv), this file is FULLY GENERATED from source — never
|
||||
* hand-edit it. The codebase is disciplined enough that a pure-AST pass
|
||||
* captures the whole truthful surface: every event/service is a string literal
|
||||
* that round-trips to a static `interface Events` / `interface Context`
|
||||
* declaration (no dynamically-named events, no runtime-only services). So the
|
||||
* committed file is a build artifact and a regenerate-and-diff freshness check
|
||||
* (`--check`) makes drift structurally impossible — which also closes the gap a
|
||||
* name-set verifier could not: a brand-new UNDOCUMENTED event cannot slip
|
||||
* through, because generation enumerates source rather than checking a subset.
|
||||
* The catalog is FULLY GENERATED from source — never hand-edit it. The codebase
|
||||
* is disciplined enough that a pure-AST pass captures the whole truthful
|
||||
* surface: every event/service is a string literal that round-trips to a static
|
||||
* `interface Events` / `interface Context` declaration (no dynamically-named
|
||||
* events, no runtime-only services). So the committed file is a build artifact
|
||||
* and a regenerate-and-diff freshness check (`--check`) makes drift structurally
|
||||
* impossible. Because generation enumerates source rather than checking a
|
||||
* hand-written subset, a brand-new event cannot be silently undocumented — it
|
||||
* appears in the next regenerate, and an un-regenerated file fails `--check`.
|
||||
*
|
||||
* `tsx scripts/gen-cordis-catalog.ts` → write the catalog
|
||||
* `tsx scripts/gen-cordis-catalog.ts --check` → exit 1 if the committed file
|
||||
@@ -76,6 +75,7 @@ const LINK_MAP: Record<string, string> = {
|
||||
BashExecSpec: 'bash.md',
|
||||
BashRunResult: 'bash.md',
|
||||
BashTask: 'bash.md',
|
||||
BashTaskRead: 'bash.md',
|
||||
}
|
||||
|
||||
/** One harness event, extracted from an `interface Events` block. */
|
||||
@@ -276,10 +276,16 @@ export function collectServices(scanRoot: string = root): ServiceEntry[] {
|
||||
const methods: string[] = []
|
||||
for (const member of cls.members) {
|
||||
if (!ts.isMethodDeclaration(member)) continue
|
||||
const isPrivate = member.modifiers?.some(m => m.kind === ts.SyntaxKind.PrivateKeyword)
|
||||
// Only the PUBLIC callable surface a `ctx.<key>` consumer sees. Drop
|
||||
// private/protected (a protected method like `notifyTaskDone` is a
|
||||
// subclass hook, not something a plugin calls through `ctx.bash`) and
|
||||
// static (not reachable through the instance).
|
||||
const nonPublic = member.modifiers?.some(m =>
|
||||
m.kind === ts.SyntaxKind.PrivateKeyword
|
||||
|| m.kind === ts.SyntaxKind.ProtectedKeyword
|
||||
|| m.kind === ts.SyntaxKind.StaticKeyword)
|
||||
|| ts.isPrivateIdentifier(member.name)
|
||||
const isStatic = member.modifiers?.some(m => m.kind === ts.SyntaxKind.StaticKeyword)
|
||||
if (isPrivate || isStatic) continue
|
||||
if (nonPublic) continue
|
||||
const memberName = member.name.getText(sf)
|
||||
if (memberName.startsWith('[')) continue // computed/symbol members
|
||||
methods.push(memberSignature(member, sf))
|
||||
|
||||
Reference in New Issue
Block a user