From c699e948e6fd999db39bcd6f278eff2c64bcdce7 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 3 Jul 2026 23:48:32 +0800 Subject: [PATCH] =?UTF-8?q?docs(AGENTS):=20fix=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20correct=20resume=20async,=20cut=20stack-locked=20le?= =?UTF-8?q?ssons?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address ds-review-bot review on #131: - Blocking: "Both are synchronous" was wrong for `resume` (`ctx.agents.resume`/`AgentLoop.resume` return `Promise` and await the persisted load). State the true split: `create` is synchronous, `resume` is async. - De-anchor the section: drop the "#118–#129" opening and every hooks-stack reference (agent/stream-chunk, PR-G, #118, ENOSPC-from-worktrees) so the lessons stand alone for a reader who wasn't there, matching the generalized style of the "Orchestrating review feedback" section above it. - Cut bullets that duplicated existing AGENTS.md content: the standalone "prove regression tests RED" bullet (already stated in the Orchestrating section and Defensive patterns) and the ENOSPC "environmental ≠ code" bullet (no repo-specific rule survives generalizing). - Generalize the survivors to their transferable rule; trim the regenerate-artifact bullet to the one gate-backed example. - Collapse the double blank line before ## Architecture. --- AGENTS.md | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2985859fe5..4b408fafb5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,16 +36,13 @@ A wave of review comments lands across several PRs in a dependent stack (`A ← ## 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.** 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. +The recurring failure mode: a mechanical gate proves lines ran and types check; it never proves 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. +- **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: fixed-`setTimeout` waits that pass in isolation race under full-suite load and flake together — replace every fixed sleep with a `waitFor(predicate)` poll (ties to [§ Defensive patterns](#defensive-patterns-hard-won) "Async state is not synchronous state"). A suite 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 — a package README still advertising a removed event, or an RFC claiming a 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, 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)"`.) +- **Before deleting an event or seam, enumerate every consumer and prove redundancy, then grill the removal ("am I deleting a seam someone will re-add?").** A seam that is a pure mirror of something a consumer already reads is safe to cut; a live-only field one consumer still uses is not — prove which before removing, not after. +- **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. ## Architecture @@ -289,7 +286,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 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`. +- **`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). `create` is synchronous; `resume` is async — it awaits the persisted load. 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