From 4e70489c7335080ad7c6b13d8ad2360180f65133 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 00:04:48 +0800 Subject: [PATCH] docs(AGENTS): collapse the section to its theme; drop the two Defensive-patterns additions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bulleted lessons and the two Defensive-patterns traps did not earn their place. Reduce the new section to its one load-bearing sentence — gates prove lines ran, not semantics/doc-accuracy/that a test guards anything; layer judgment on top and lean on an independent reviewer for what gates can't see — and drop the spawn-narrowing and AgentLoop.create-meta bullets entirely. --- AGENTS.md | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4b408fafb5..5f06b26765 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,15 +34,9 @@ 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 +## Landing changes cleanly: gates and judgment -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. +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 — and lean on an independent agent to review for the class of defect gates structurally cannot catch (prose/RFC/comment drift, a bug introduced while fixing, a test that asserts nothing load-bearing). ## Architecture @@ -285,8 +279,6 @@ 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 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