diff --git a/AGENTS.md b/AGENTS.md index 8438295683..47109ebfbe 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,6 +34,19 @@ A wave of review comments lands across several PRs in a dependent stack (`A ← - **Delegated work is trust-but-verify.** When sub-agents implement fixes in parallel, their report describes what they INTENDED, not necessarily what landed. Re-run the gates yourself on the actual tree, and for a regression guard, **prove it FAILS on the unfixed code** (introduce the regression, watch the test go red, revert) — a guard that passes both ways guards nothing. A sub-agent that "reframes the problem as already-handled" instead of fixing it is a signal to dig in personally, not to accept the reframing. - **Triage on the merits, then reply in-thread.** Verify each comment against the code before acting (a reviewer flagging the right symptom can still mis-diagnose the cause — confirm both). Reply in the GitHub review thread (`gh api …/pulls/{pr}/comments/{id}/replies`), not as a top-level comment, stating the fix and the commit that carries it. +## Landing changes cleanly: gates, Codex, and scope + +Hard-won from the hooks stack (#118–#129). The recurring theme: a mechanical gate proves lines ran and types check; it does NOT prove semantics, doc accuracy, or that a test guards anything. Layer the cheap human/AI judgment on top, in the right order, and keep each unit of work honestly scoped. + +- **Every regression test must be proven RED on the unfixed code, and this is the top-billed discipline, not a footnote.** Neuter the fix (comment out the one line, or revert the source), run the new test, watch it fail, then restore. A guard that passes both ways guards nothing — and a green 100%-coverage suite actively hides this (the line ran; it just asserted nothing load-bearing). This caught real bugs repeatedly here: a `structuredClone` aliasing fix, a bridge `expectedEventName` discriminator guard, a blocking-Stop-hook reason fallback. Do it for EVERY guard, every time; the proof takes thirty seconds and is the only thing that certifies the test. +- **Run `pnpm run test:coverage` (the FULL suite), not an isolated `-t` filter, before trusting green.** Test-isolation bugs surface only in the full run: here twelve fixed-`setTimeout` waits raced under full-suite load and passed in isolation but flaked together — fixed by replacing every fixed sleep with a `waitFor(predicate)` poll (ties to [§ Defensive patterns](#defensive-patterns-hard-won) "Async state is not synchronous state"). A suite that is green under `-t ` but red under `test:coverage` is telling you about shared state, not a flake to rerun. +- **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. +- **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. + + ## Architecture This codebase is based on the **Cordis** framework, built microkernel-style: **everything is a plugin**. All necessary Cordis dependencies are copied into this monorepo as vendored source (under `vendor/`) instead of being depended on via npm. @@ -275,6 +288,8 @@ Each bullet is a bug class that bit us; the rule prevents the reoccurrence. - **A real-load-path test only GUARDS the export shape if a broken shape actually FAILS it.** The original crash (`cannot get property … without inject`) fired because that plugin HAS `inject`. A plugin with NO `inject` (a composition/bundle plugin that mounts children carrying their own inject, e.g. `dsh-agent-core` and the app packages) does NOT crash on a stray `export default` — `unwrapExports` silently drops `Config`/`name` and the plugin boots anyway — so a Loader smoke stays green while the export shape is broken. For such plugins add an EXPLICIT assertion that the regression fails: `expect('default' in mod).toBe(false)` plus running the module through the real `Loader.prototype.unwrapExports` and asserting `name`/`Config`/`apply` survive. Prove it: add `export default apply`, watch the test go red, revert. - **"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`. ## Type Safety and Documentation