diff --git a/AGENTS.md b/AGENTS.md index 47109ebfbe..2985859fe5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,7 +43,7 @@ Hard-won from the hooks stack (#118–#129). The recurring theme: a mechanical g - **Codex convergence is for the class of defect gates STRUCTURALLY cannot catch — spend it there.** `xhigh` Codex reliably finds what `typecheck`/`lint`/`coverage`/`doc-sync` are blind to: (a) **prose/RFC/comment drift** the doc-sync scope doesn't scan — e.g. two package READMEs still advertising a removed event, or an RFC claiming a `block` decision "carries context too" when that union has no such field; (b) **a bug you INTRODUCED while fixing** — the fix's own new branch, un-covered by the test you wrote for the original bug; (c) **dishonest test comments** blessing a wrong assertion. Treat a Codex finding as a claim to verify against the code, then re-bucket it yourself (its own (A)/(B)/(C) label is an input, not a verdict) — but know that "clean gates" is exactly when Codex earns its keep. - **Scope a Codex review to ONE fix or concern.** A convergence prompt bundling two independent fixes plus verification context timed out at the 850s cap with no verdict — a wasted ~14-minute run — then completed fine once split into two smaller serial reviews. One concern per review is faster AND yields a sharper verdict. (For the invocation: the prompt is a POSITIONAL arg to `ask-codex.sh`, not `--file`; the only flags are `--codex-model`, `--codex-timeout`. Multi-paragraph prompts go via `"$(cat file)"`.) - **A cleanup or removal discovered mid-review that exceeds the reviewed RFC's scope goes in a NEW stacked PR, even though pre-release churn is cheap.** Do not retroactively widen a diff a reviewer already signed off on, and do not fold a fresh decision into a converged PR. Before deleting an event/seam, first enumerate every consumer and prove redundancy (here: `agent/stream-chunk` was proven a pure mirror of the durable `assistant/chunk` — ACP already read the durable one, the stdio UI ignored the live-only args), then grill the removal ("am I deleting a seam someone will re-add?"). The removal became its own PR-G with its own RFC, not an amendment to the reviewed #118. -- **Regenerate a generated artifact as PART of the edit that invalidates it, not as a gate to fail.** Any edit to a `types.ts` `interface Events`/`Context` block or a module doc the generator reads makes `docs/cordis-catalog/events-and-services.md` stale; run `pnpm run gen-cordis-catalog` (and `gen-module-graph`) in the same step rather than letting `doc-sync` discover it. Likewise run `pnpm run lint:fix` before hand-fixing a new test file — the auto-fixable churn (quotes, `max-len`) should never consume review attention meant for the real errors. +- **Regenerate a generated artifact as PART of the edit that invalidates it, not as a gate to fail.** Know what triggers each: `docs/cordis-catalog/events-and-services.md` is generated from the `interface Events` / `interface Context` member JSDoc (not top module docs), so run `pnpm run gen-cordis-catalog` in the same step you touch an event/service declaration or its JSDoc — rather than letting `verify-cordis-catalog` (part of `doc-sync`) discover it stale. `docs/module-graph.md` is generated from package `peerDependencies`, so regenerate it (`pnpm run gen-module-graph`; checked by the separate `verify-module-graph`, NOT `doc-sync`) only when you change a package's `@deepseek-ai/dsh-*` peer edges. Likewise run `pnpm run lint:fix` before hand-fixing a new test file — the auto-fixable churn (quotes, `max-len`) should never consume review attention meant for the real errors. - **Read a failure before reacting: environmental ≠ code.** `ENOSPC: file watchers` from many concurrent worktrees fails the `tsx`-based `demo:echo` smoke, but the label/output already rendered correctly before the watcher died and the published-artifact built-bin smoke (plain `node`, no watcher) is unaffected. Recognize the class on the FIRST occurrence — fall back to the watcher-free check or prune stale worktrees — rather than burning retry cycles on a transient the code never caused. @@ -289,7 +289,7 @@ Each bullet is a bug class that bit us; the rule prevents the reoccurrence. - **"Real entry path" means the PUBLISHED ARTIFACT, not the dev runtime.** A test (or a `demo:*` smoke) that boots `src/bin.ts` under `tsx` is NOT the same code a consumer runs — the package `bin` field points at the built `lib/bin.js` under plain `node`. tsx masks failure modes the published artifact has: a boot settle-race that exits 0 before the app's handles attach, module-resolution differences (the unbuilt `paths` map vs node_modules), and a load failure that `loader.await()`'s `Promise.allSettled` SWALLOWS so a typo'd config silently exits 0. The guard is a smoke that runs the built `lib/bin.js` under plain `node` in a node_modules-shaped temp dir (symlinked workspace + vendor packages), asserts the real output, AND asserts a genuinely-missing config exits NON-ZERO. The tsx demo is necessary but not sufficient; the published-bin smoke is what catches "green under tsx, broken on install". - **Tag spelling and EOF hygiene.** cordis.yml interpolates env via the `!!js` tag (js-yaml resolves custom tags under `tag:yaml.org,2002:js`), not `!js` — keep code, comments, and docs consistent. Files end with exactly one trailing newline; `git diff --check` (a pre-push gate) rejects new blank lines at EOF. - **`child_process.spawn` narrows non-null `stdout`/`stderr` only from a LITERAL `stdio` tuple.** A ternary or variable in a `stdio` slot (e.g. `stdio: [wantStdin ? 'pipe' : 'ignore', 'pipe', 'pipe']`) selects the generic `spawn` overload, widening the child's streams to nullable — which then trips `no-non-null-assertion` (forbidden in `src`). Write two full `spawn(...)` calls with literal tuples in an `if`/`else` (or a ternary between two complete calls), as [`dsh-bash-local`'s `run.ts`](packages/bash/bash-local/src/run.ts) does, so each branch's literal tuple keeps the typed overload. This trap bit twice — recognize it the moment a conditional `stdio` slot appears. -- **`AgentLoop.create(id, options)` DROPS `options.meta` — only the async factory path threads it.** The synchronous `create()` prepares its session with a hardcoded `{ meta: {} }`; a test (or caller) that needs `session.header.cwd` or other header metadata to take effect must use the factory `ctx.agents.create({ agentId, sessionId, meta, agentOptions })` (or `resume`), which passes `meta: options.meta ?? {}`. A cwd-dependent test that silently sees an empty cwd is almost always this. See `packages/core/agent-loop/src/index.ts`. +- **`AgentLoop.create(id, options)` DROPS `options.meta` — only the programmatic factory `create` threads it.** The convenience `create()` prepares its session with a hardcoded `{ meta: {} }`; a test (or caller) that needs `session.header.cwd` or other header metadata to take effect must use the factory `ctx.agents.create({ agentId, sessionId, meta, agentOptions })` (which passes `meta: options.meta ?? {}`), or `resume` (which reloads the persisted header). Both are synchronous. A cwd-dependent test that silently sees an empty cwd is almost always the wrong creation path. See `packages/core/agent-loop/src/index.ts`. ## Type Safety and Documentation