From c3424e54876509c797b092919bc4674adec12d2f Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 15:11:49 +0800 Subject: [PATCH] fix(docs): address Codex review round 1 on the AGENTS.md rewrite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Restore the universal JSDoc rule the rewrite dropped (module doc comment + semantic JSDoc on every export), in root AGENTS.md § Type safety and documentation — the generated-catalog RFC cites it as the rule the generator enforces at the source. - Repoint the six remaining citations of moved content that the section-name grep missed (rule-title quotes and prose references): agent-loop agent.ts, acp index.ts, acp turns.spec.ts, the Exa e2e header, the real-api-e2e RFC, the doc-sync-enforcement RFC amendment, and rfc/implemented/AGENTS.md's section-name casing. - Fix two docs/testing.md overstatements: the unit tier also runs examples/*/tests specs, and keyless-by-nature examples have no with-key smoke. - Displacement trims keep root AGENTS.md at 1,498/1,500. --- AGENTS.md | 20 +++++++++---------- docs/rfc/implemented/AGENTS.md | 2 +- .../2026-06-11-doc-sync-enforcement.md | 2 +- .../testing/2026-06-19-real-api-e2e-ci.md | 2 +- docs/testing.md | 4 ++-- packages/core/agent-loop/src/agent.ts | 2 +- packages/ui/acp/src/index.ts | 4 ++-- packages/ui/acp/tests/turns.spec.ts | 2 +- packages/web/web-search-exa/tests/exa.e2e.ts | 2 +- 9 files changed, 20 insertions(+), 20 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 840c684b6b..19f279319a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,7 @@ This is the monorepo of the DeepSeek Harness group; it hosts **DeepSeek Code**, ## Pre-release stance: foundation over blast radius -**This applies only while the harness is unreleased — remove this section at the first tagged release.** There are no external consumers, so optimize for the correct foundation, not a small diff: move files, rename public symbols, repackage plugins, and update every reference in the same change. No backward-compat shims, deprecation aliases, or re-export stubs. On-disk formats need no migrations — a backend REJECTS anything not at the current version. Two sanctioned version stances: monotonic bump-and-reject (the SQLite backend's `SCHEMA_VERSION`), and a pinned `0` that absorbs all shape churn (`SESSION_FORMAT_VERSION` in `dsh-session`, documented "no compatibility implied") so the instability stays explicit. Real version policy begins at the first release. +**This applies only while the harness is unreleased — remove this section at the first tagged release.** There are no external consumers, so optimize for the correct foundation, not a small diff: move files, rename public symbols, repackage plugins, and update every reference in the same change. No backward-compat shims, deprecation aliases, or re-export stubs. On-disk formats need no migrations — a backend REJECTS anything not at the current version. Two sanctioned version stances: monotonic bump-and-reject (the SQLite backend's `SCHEMA_VERSION`), and a pinned `0` that absorbs all shape churn (`SESSION_FORMAT_VERSION` in `dsh-session`, documented "no compatibility implied"). Real version policy begins at the first release. ## Repository layout @@ -38,8 +38,8 @@ pnpm install # pnpm workspaces, node >= 24 pnpm run test # vitest unit tests pnpm run test:coverage # THE gating test run: per-file 100% coverage on packages/*/*/src pnpm run test:e2e # real-API tests; self-skip without DEEPSEEK_API_KEY -pnpm run test:snapshot # keyless ACP replay vs committed goldens; filter one: pnpm run test:snapshot -t -pnpm run test:snapshot:record # re-record goldens against the real API (needs key) +pnpm run test:snapshot # keyless ACP replay vs goldens; filter: -t +pnpm run test:snapshot:record # re-record goldens (needs key) pnpm run typecheck pnpm run lint pnpm run build # tsc emits lib/types, tsdown bundles runtime @@ -47,12 +47,12 @@ pnpm run hygiene # knip + publint + workspace constraints + NodeNext cons pnpm run doc-sync # all documentation gates; see the doc-sync script in package.json pnpm run demo:echo # mock-model REPL, no key needed pnpm run demo:repl # real REPL coding agent (needs DEEPSEEK_API_KEY) -pnpm run demo:acp # ACP server agent over JSON-RPC stdio (needs DEEPSEEK_API_KEY) +pnpm run demo:acp # ACP server agent (needs DEEPSEEK_API_KEY) ``` ### Run the CI gates locally before marking a PR ready -CI is the backstop, not the first place a gate runs. From a fresh clone or worktree, run `pnpm run build` once first — publint and the NodeNext check validate built `lib/`. The CI-equivalent run: +CI is the backstop, not the first run. From a fresh clone or worktree, `pnpm run build` first — publint and the NodeNext check validate built `lib/`. The CI-equivalent run: ```sh set -euo pipefail @@ -80,9 +80,9 @@ Real-API tests and demos read `DEEPSEEK_API_KEY` (and optional `DEEPSEEK_BASE_UR ## Conventions -- Every npm package is `@deepseek-ai/dsh-`; vendored packages keep upstream names and are `private: true`. `cordis` is a peerDependency (+ devDependency) of every harness package. +- Every npm package is `@deepseek-ai/dsh-`; vendored packages keep upstream names and are `private: true`. `cordis` is a peerDependency (+ dev) of every harness package. - ESM everywhere (`"type": "module"`). Cross-package imports use package names, never relative paths; in-package relative imports use explicit `.ts` extensions. Dev/test/demo run unbuilt via tsx + the root tsconfig `paths` map; building is only for consumers outside the repo. -- **Registrations are effects**: every contribution goes through `ctx.effect()` / `ctx.on()`; a registry's `register()` returns the disposer. Every registry gets an HMR-safety test. +- **Registrations are effects**: every contribution goes through `ctx.effect()` / `ctx.on()`; a registry's `register()` returns the disposer. - **Typed events via declaration merging**; extensible unions use the merge-extensible-map pattern (`ContentBlockMap`, `SessionEventMap`, …). Every new event's JSDoc carries an `@mode` tag — the catalog generator hard-errors without it; mode semantics are in the [generated catalog](docs/cordis-catalog/events-and-services.md) header and [the catalog RFC](docs/rfc/implemented/process/2026-06-20-generated-cordis-catalog.md). - **Discriminated unions: `switch` on the tag**, not if-chains. Closed unions end with `default: assertNever(...)`; merge-extensible unions must NOT — handle known cases and fall through `default` with a comment. - **Waterfall listeners MUST call `next()`** to delegate; returning without it is the veto ([semantics](docs/architecture.md#cordis-waterfall-semantics-important)). @@ -98,16 +98,16 @@ Real-API tests and demos read `DEEPSEEK_API_KEY` (and optional `DEEPSEEK_BASE_UR - **A tool's ACP render intent is part of its design**, decided up front (`generic`/`terminal`/`diff`, `locations`); presentation methods are pure functions of `args` ([render-intent RFC](docs/rfc/implemented/architecture/2026-07-02-tool-render-intent-union.md), [cookbook](docs/cookbook/adding-a-tool.md)). - **A new capability seam, lifecycle shape, or transcript surface names its coverage at every tier (unit, e2e, snapshot) at plan time** and verifies the harness can express it — a gap is scheduled work, not a mid-build surprise. - **Merge PRs with merge commits** (`gh pr merge --merge`), never squash/rebase. **Never rewrite a pushed branch**; update a child by merging its parent down. **A review fix lands on the PR that introduced the issue, as a separate commit**, then merges down ([stacked-review guide](docs/cookbook/responding-to-pr-review-on-a-stack.md)). -- TODO markers by urgency: `FIXME` / `TODO` / `XXX` ([semantics](docs/development.md)). +- TODO markers: `FIXME`/`TODO`/`XXX` by urgency ([semantics](docs/development.md)). - Files end with exactly one trailing newline; `git diff --check` (pre-push) gates it. ## Defensive patterns -[docs/defensive-patterns.md](docs/defensive-patterns.md) carries the hard-won bug-class rules: report orthogonal outcomes independently; honor cross-seam contracts on both sides; async state is not synchronous state; dispose must reach quiescence; contain callback exceptions; never hand untrusted output the ambient environment or predictable paths. Read it before writing lifecycle, concurrency, subprocess, or teardown code. +[docs/defensive-patterns.md](docs/defensive-patterns.md) carries the hard-won bug-class rules: report orthogonal outcomes independently; honor cross-seam contracts on both sides; async state is not synchronous state; dispose must reach quiescence; contain callback exceptions; never hand untrusted output the ambient environment or predictable paths. Read it before lifecycle, concurrency, subprocess, or teardown work. ## Type safety and documentation -Everything compiles under `strict: true` with `noImplicitAny`; every remaining `any` carries a comment saying why a narrower type is infeasible. Lean toward the stricter lint rule and the extra mechanical gate: encode invariants in checks (`verify-*` scripts), preferring a narrow justified escape hatch over a rule left off globally. Type gymnastics are acceptable inside core packages when they buy plugin-author DX (the `defineTool` schema DSL is the canonical example). +Everything compiles under `strict: true` with `noImplicitAny`; every remaining `any` carries a comment saying why a narrower type is infeasible. Every module has a module-level doc comment; every export (and non-obvious method) has a JSDoc explaining semantics — contracts, disposal, errors — not the name restated; internal helpers only where non-obvious; one-liners when one line suffices. Lean toward the stricter lint rule and the extra mechanical gate: encode invariants in checks (`verify-*` scripts), preferring a narrow justified escape hatch over a rule left off globally. Type gymnastics are acceptable inside core packages when they buy plugin-author DX (the `defineTool` schema DSL is the canonical example). Docs are part of every change: code changes update their README and JSDoc in the SAME change; a bilingual-pair edit updates the counterpart and re-records ([i18n contract](docs/i18n/README.md)). The writing rules — document the current state never the history, one physical line per paragraph, one home per fact — and the word-budget gate live in [docs/AGENTS.md](docs/AGENTS.md). diff --git a/docs/rfc/implemented/AGENTS.md b/docs/rfc/implemented/AGENTS.md index 831b8d5325..c23e9069e8 100644 --- a/docs/rfc/implemented/AGENTS.md +++ b/docs/rfc/implemented/AGENTS.md @@ -1,6 +1,6 @@ # AGENTS.md — Implemented RFCs -These are RFCs whose decision has **shipped**. The repo-wide and docs-wide rules still apply ([root AGENTS.md](../../../AGENTS.md) § "Type Safety and Documentation", [docs/AGENTS.md](../../AGENTS.md)); this file adds one rule specific to this folder. +These are RFCs whose decision has **shipped**. The repo-wide and docs-wide rules still apply ([root AGENTS.md](../../../AGENTS.md) § "Type safety and documentation", [docs/AGENTS.md](../../AGENTS.md)); this file adds one rule specific to this folder. ## Keep an implemented RFC current with what actually shipped diff --git a/docs/rfc/implemented/process/2026-06-11-doc-sync-enforcement.md b/docs/rfc/implemented/process/2026-06-11-doc-sync-enforcement.md index 69fca053f3..8278a1d4b7 100644 --- a/docs/rfc/implemented/process/2026-06-11-doc-sync-enforcement.md +++ b/docs/rfc/implemented/process/2026-06-11-doc-sync-enforcement.md @@ -17,7 +17,7 @@ Two gates, mirroring the existing `scripts/` style (tsx ESM, one job each): Both run via a shared `doc-sync` package.json script that the lefthook pre-push hook and CI both invoke ([mechanical quality gates](2026-06-11-quality-gates.md): hooks and CI call the same scripts, so the gate fires locally before a push — not only after it). They run after `pnpm run typecheck`, which validates the package/vendor build graph that doc-typecheck references. API-extractor golden reports ([the deferred API-extractor-reports proposal](../../proposed/process/2026-06-11-api-extractor-reports.md)) were deliberately **deferred** — low value for an internal monorepo where reviewers already see the source diff, and a heavy, finicky dependency. -**Amendment (2026-06-17):** a third gate, **`verify-md-wrap`**, was later folded into `doc-sync`. It parses each in-scope Markdown file (`README.md`, `docs/**`, `packages/*/README.md`, plus `AGENTS.md` / `packages/AGENTS.md`) with `mdast-util-from-markdown` + GFM and fails on any `paragraph` node spanning more than one source line, enforcing the AGENTS.md "Markdown is not hard-wrapped" convention. Same verify-don't-generate principle: it reports hard-wraps and never rewrites, so it adds no formatting churn. `doc-sync` is now three gates. +**Amendment (2026-06-17):** a third gate, **`verify-md-wrap`**, was later folded into `doc-sync`. It parses each in-scope Markdown file (`README.md`, `docs/**`, `packages/*/README.md`, plus `AGENTS.md` / `packages/AGENTS.md`) with `mdast-util-from-markdown` + GFM and fails on any `paragraph` node spanning more than one source line, enforcing the docs/AGENTS.md "one physical line per paragraph" writing rule. Same verify-don't-generate principle: it reports hard-wraps and never rewrites, so it adds no formatting churn. `doc-sync` is now three gates. ## Consequences diff --git a/docs/rfc/implemented/testing/2026-06-19-real-api-e2e-ci.md b/docs/rfc/implemented/testing/2026-06-19-real-api-e2e-ci.md index 2001147be8..95d168953e 100644 --- a/docs/rfc/implemented/testing/2026-06-19-real-api-e2e-ci.md +++ b/docs/rfc/implemented/testing/2026-06-19-real-api-e2e-ci.md @@ -6,7 +6,7 @@ Status: implemented (accepted 2026-06-19) ## Context -The harness leans hard on real-API tests by policy: AGENTS.md § Secrets argues that a no-key suite proves the plumbing but not the product, and the [ACP inject postmortem](../../../postmortem/0001-acp-default-export-drops-inject.md) is the standing proof — 178 keyless tests stayed green while a real editor session crashed instantly. The real-API e2e suite (`pnpm run test:e2e`, the `*.e2e.ts` files) exists precisely to close that gap: it drives the agent against the live DeepSeek API — real model calls, real bash tools, multi-turn, resume, ACP-over-stdio. +The harness leans hard on real-API tests by policy: [docs/testing.md](../../../testing.md) argues that a no-key suite proves the plumbing but not the product, and the [ACP inject postmortem](../../../postmortem/0001-acp-default-export-drops-inject.md) is the standing proof — 178 keyless tests stayed green while a real editor session crashed instantly. The real-API e2e suite (`pnpm run test:e2e`, the `*.e2e.ts` files) exists precisely to close that gap: it drives the agent against the live DeepSeek API — real model calls, real bash tools, multi-turn, resume, ACP-over-stdio. But until this change **nothing in CI ran it**. The default gate ([.github/workflows/ci.yml](../../../../.github/workflows/ci.yml)) is deliberately keyless — it carries no secret, runs on every push and PR including from forks, and stays green for any contributor. `test:e2e` self-skips without a key (`describe.skipIf(!process.env.DEEPSEEK_API_KEY)`), so even if ci.yml invoked it, a keyless runner would skip it green. The real-API safety net therefore only fired when a developer happened to run it locally with a key in their environment — i.e. unreliably, and never as a merge gate. diff --git a/docs/testing.md b/docs/testing.md index c848efe81a..446272cb92 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -4,14 +4,14 @@ How this repo tests, tier by tier, and the rules that keep a green suite meaning ## Tiers -- **Unit** (`pnpm run test`): vitest, colocated at `packages///tests/*.spec.ts`. Every registry gets an HMR-safety test (dispose the contributing fiber, assert cleanup). Excessive tests are welcome — err toward covering edge cases, error paths, event ordering, and concurrency races; review findings get regression tests (see `packages/core/agent-loop/tests/review-fixes.spec.ts`). +- **Unit** (`pnpm run test`): vitest over `packages|examples/*/tests/**/*.spec.ts`, colocated with what they test. Every registry gets an HMR-safety test (dispose the contributing fiber, assert cleanup). Excessive tests are welcome — err toward covering edge cases, error paths, event ordering, and concurrency races; review findings get regression tests (see `packages/core/agent-loop/tests/review-fixes.spec.ts`). - **Coverage gate** (`pnpm run test:coverage`): the gating run, per-file 100% on `packages/*/*/src`. An uncovered line is often dead code the gate is correctly flagging for deletion, not a missing test to bolt on. Line coverage is necessary, never sufficient — it proves lines ran, not that the feature works as shipped. - **Real-API e2e** (`pnpm run test:e2e`): with-key tests against the live DeepSeek API; self-skip without `DEEPSEEK_API_KEY` so keyless CI stays green ([real-API e2e RFC](rfc/implemented/testing/2026-06-19-real-api-e2e-ci.md)). - **Snapshot** (`pnpm run test:snapshot`): boots the real example subprocess, replays a recorded session keyless, diffs normalized stdout + the re-persisted log against committed goldens ([snapshot RFC](rfc/implemented/testing/2026-06-19-acp-snapshot-tests.md)). Re-record with `pnpm run test:snapshot:record`; reviewing the golden diff is part of the review. ## The with-key policy: inference is cheap here -We are DeepSeek — do not ration real-API tests. A no-key test proves the plumbing; only a with-key run proves the agent works against a real model. Write many: real prompts that write files, multi-turn conversations, tool use, cancellation mid-stream. Cheapest and highest-value are **smoke tests** that boot the real example, send one real prompt, and check the world — they catch the "green unit tests, broken product" class that mocks structurally cannot ([postmortem 0001](postmortem/0001-acp-default-export-drops-inject.md)). The self-skip exists only so secretless CI and keyless contributors aren't blocked; it is not a cost signal. Every example ships both a keyless and a with-key smoke ([examples/AGENTS.md](../examples/AGENTS.md)). +We are DeepSeek — do not ration real-API tests. A no-key test proves the plumbing; only a with-key run proves the agent works against a real model. Write many: real prompts that write files, multi-turn conversations, tool use, cancellation mid-stream. Cheapest and highest-value are **smoke tests** that boot the real example, send one real prompt, and check the world — they catch the "green unit tests, broken product" class that mocks structurally cannot ([postmortem 0001](postmortem/0001-acp-default-export-drops-inject.md)). The self-skip exists only so secretless CI and keyless contributors aren't blocked; it is not a cost signal. Every example ships a keyless smoke and — unless keyless-by-nature — a with-key smoke ([examples/AGENTS.md](../examples/AGENTS.md)). ## Prefer the real implementation over a mock diff --git a/packages/core/agent-loop/src/agent.ts b/packages/core/agent-loop/src/agent.ts index d74c129ddd..433c28326b 100644 --- a/packages/core/agent-loop/src/agent.ts +++ b/packages/core/agent-loop/src/agent.ts @@ -79,7 +79,7 @@ export class ReactLoopAgent implements Agent { // Release quiescence waiters on a transition OUT of running BEFORE emitting // (the disposer handles the disposed transition separately). Settling first // means a throwing `agent/status` subscriber cannot starve a `whenIdle()` - // waiter (AGENTS.md "contain callback exceptions" — a lifecycle await must + // waiter (docs/defensive-patterns.md "contain callback exceptions" — a lifecycle await must // not hang on one bad listener). if (status !== 'running') this.settleIdleWaiters() try { diff --git a/packages/ui/acp/src/index.ts b/packages/ui/acp/src/index.ts index 5492707902..d908f3d2db 100644 --- a/packages/ui/acp/src/index.ts +++ b/packages/ui/acp/src/index.ts @@ -202,8 +202,8 @@ interface SessionRecord { * Drive the in-flight prompt's settle from the harness event stream. The bridge * settles off the durable log: the `turn/end` session event on the * `session/event` feed for the prompt's own turn, with the agent - * erroring/settling to idle as a fallback (AGENTS.md "honor cross-seam contracts - * on BOTH sides") for the case where a throwing peer `session/event` listener + * erroring/settling to idle as a fallback (docs/defensive-patterns.md "honor + * cross-seam contracts on BOTH sides") for the case where a throwing peer `session/event` listener * starved the bridge's listener before it saw the boundary. The first of these * to fire settles the prompt; `settle` is then cleared so the others are no-ops * (settle-exactly-once). diff --git a/packages/ui/acp/tests/turns.spec.ts b/packages/ui/acp/tests/turns.spec.ts index 7634602a35..494bbb8741 100644 --- a/packages/ui/acp/tests/turns.spec.ts +++ b/packages/ui/acp/tests/turns.spec.ts @@ -79,7 +79,7 @@ describe('acp bridge — turn outcomes', () => { it('the REAL bash tool drives the tool-call UI end-to-end: command title + description block + console output', async () => { // Use the SHIPPING tool (dsh-tool-bash + dsh-bash-local), not an inline // stand-in, so this verifies the actual presentCall/presentResult the editor - // sees (AGENTS.md "prefer the real implementation over a mock in tests"). + // sees (docs/testing.md "prefer the real implementation over a mock"). // The mock MODEL still scripts the tool call (no real LLM needed), but the // tool and executor are real: a real `echo` runs and its real output flows // back through the bridge. diff --git a/packages/web/web-search-exa/tests/exa.e2e.ts b/packages/web/web-search-exa/tests/exa.e2e.ts index 32da0a485c..84c0214228 100644 --- a/packages/web/web-search-exa/tests/exa.e2e.ts +++ b/packages/web/web-search-exa/tests/exa.e2e.ts @@ -3,7 +3,7 @@ import { ExaSearchProvider, EXA_DEFAULT_BASE_URL, EXA_DEFAULT_HIGHLIGHTS_PER_RES /** * Real-API smoke for the Exa search provider. Self-skips without `$EXA_API_KEY` - * (CI has no secrets), per the with-key e2e policy in AGENTS.md § Secrets. + * (CI has no secrets), per the with-key e2e policy in docs/testing.md. */ const apiKey = process.env.EXA_API_KEY const maybe = apiKey !== undefined && apiKey.length > 0 ? describe : describe.skip