mirror of
https://github.com/deepseek-ai/deepseek-harness
synced 2026-08-15 21:04:50 +00:00
docs(AGENTS): fix two factual imprecisions caught by Codex
Codex convergence verified the retrospective's claims against the code and caught two inaccuracies (everything else — the spawn-overload claim, create() dropping meta, ask-codex invocation, test:coverage guidance, waitFor, markdown — checked out): - The meta-threading factory `ctx.agents.create()` (createAgent) is SYNCHRONOUS, not "the async factory path"; the async one is `resume` (which reloads the persisted header). Reworded. - The generated-artifact bullet conflated triggers: gen-cordis-catalog reads `interface Events`/`Context` member JSDoc (not top module docs); gen-module-graph is driven by package peerDependencies (not event/doc edits); and module-graph freshness is `verify-module-graph`, a SEPARATE gate from `doc-sync`. Split the guidance per artifact.
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user