From 26339144e39ca4f44319e1e2508e317cb3fdfbf8 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sat, 4 Jul 2026 15:43:51 +0800 Subject: [PATCH] =?UTF-8?q?fix(docs):=20address=20Codex=20review=20round?= =?UTF-8?q?=202=20=E2=80=94=20repoint=20every=20remaining=20moved-policy?= =?UTF-8?q?=20citation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The definitive sweep (audit every AGENTS.md mention in packages/, docs/, examples/, scripts/) found thirteen more citations of relocated policy and two citations of rules that never existed as quoted: - with-key policy comments (web deepseek/perplexity e2e headers) -> docs/testing.md; real-impl-over-mock comments (acp harness, load, stream-update specs) -> docs/testing.md; defensive-pattern quotes (acp index.ts x3, stream-update) -> docs/defensive-patterns.md. - md-tier repoints: real-api-e2e RFC, tool-schema-catalog RFC, postmortem 0001 guardrail row, adding-a-package cookbook, drop-bash-output-spill-files RFC, acp-subagent-backend RFC phrasing. - Two false attributions dropped in favor of self-contained reasoning: tool-todo's 'don't validate scenarios that can't happen' and the bash-stdin-env RFC's 'Don't add features beyond what the task requires' (neither rule ever existed under those names). - Citations of the two 'not golden truth' doctrines stay: those bullets survive verbatim in the root conventions. Note: packages/support/ui-stdio readline TTY spec flakes under full coverage on a heavily loaded box (passes standalone and passed the same tree's coverage run minutes earlier); untouched by this stack. --- docs/cookbook/adding-a-package.md | 2 +- docs/postmortem/0001-acp-default-export-drops-inject.md | 2 +- .../2026-06-30-bash-stdin-env-trusted-plugin-surface.md | 2 +- .../implemented/feature/2026-06-22-acp-subagent-backend.md | 2 +- .../implemented/process/2026-07-02-tool-schema-catalog.md | 2 +- docs/rfc/implemented/testing/2026-06-19-real-api-e2e-ci.md | 2 +- .../2026-06-20-drop-bash-output-spill-files.md | 2 +- packages/todo/tool-todo/src/index.ts | 4 ++-- packages/ui/acp/src/index.ts | 6 +++--- packages/ui/acp/tests/harness.ts | 2 +- packages/ui/acp/tests/load.spec.ts | 2 +- packages/ui/acp/tests/stream-update.spec.ts | 4 ++-- packages/web/web-search-deepseek/tests/deepseek.e2e.ts | 2 +- packages/web/web-search-perplexity/tests/perplexity.e2e.ts | 2 +- 14 files changed, 18 insertions(+), 18 deletions(-) diff --git a/docs/cookbook/adding-a-package.md b/docs/cookbook/adding-a-package.md index ef5f48d624..1ab6931696 100644 --- a/docs/cookbook/adding-a-package.md +++ b/docs/cookbook/adding-a-package.md @@ -46,4 +46,4 @@ pnpm run test:coverage # 100% per-file over src (types.ts exempt) pnpm run build && pnpm run hygiene ``` -Test expectations: every registry/registration needs an HMR-safety test (register from a child fiber, dispose it, assert cleanup). Excessive tests are welcome — see AGENTS.md. +Test expectations: every registry/registration needs an HMR-safety test (register from a child fiber, dispose it, assert cleanup). Excessive tests are welcome — see [docs/testing.md](../testing.md). diff --git a/docs/postmortem/0001-acp-default-export-drops-inject.md b/docs/postmortem/0001-acp-default-export-drops-inject.md index 12eee7a2b7..e49c68516a 100644 --- a/docs/postmortem/0001-acp-default-export-drops-inject.md +++ b/docs/postmortem/0001-acp-default-export-drops-inject.md @@ -101,7 +101,7 @@ Both bugs share one root process gap: **no test exercised the plugin through its - **`AgentLoop.resume` reads `this.ctx.get('sessionPersistence')`** (`packages/core/agent-loop/src/index.ts`) — the Bug #2 fix, with a comment explaining the shadow-walk trap. - **No-key `session/new` e2e over real stdio** (`examples/acp-agent/tests/acp.e2e.ts`): boots the example as a subprocess through the real Loader and asserts `session/new` resolves. This fails loudly on Bug #1 with no API key. Verified it fails when `export default apply` is restored. - **`TSX_TSCONFIG_PATH` in the e2e spawn**: the subprocess runs from a temp cwd, where tsx cannot find the repo-root tsconfig `paths` map by searching upward — so dsh-* imports silently fell back to built `lib/`. Pointing tsx at the repo tsconfig makes resolution cwd-independent and ensures the test runs *source*, not a possibly-stale build. -- **AGENTS.md defensive pattern**: "Line coverage is not behavior coverage; test the REAL entry path, not a synthetic stand-in" — codifies the lesson for every future plugin. +- **[docs/testing.md](../testing.md) rule**: "test the real entry path", line coverage is not behavior coverage — codifies the lesson for every future plugin. ## Lessons diff --git a/docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md b/docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md index 153317bc25..0d797fa8a2 100644 --- a/docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md +++ b/docs/rfc/implemented/architecture/2026-06-30-bash-stdin-env-trusted-plugin-surface.md @@ -26,7 +26,7 @@ Three deliberate choices: ## Scope: configurable scrub pattern is NOT included -An earlier sketch of this work also proposed making `SENSITIVE_ENV_PATTERN` configurable. Validating against the code, that is **speculative and already subsumed**: `run.ts` documents a configurable whitelist as future work, and the new explicit `env` field — merged after the scrub — already gives a caller full control, including over credential-shaped vars. There is no current caller that needs to *broaden* the ambient scrub (the hazard runs the other way). Adding a config knob now would be a feature with no consumer, against [AGENTS.md](../../../../AGENTS.md) § "Don't add features beyond what the task requires". If a real workflow ever needs to forward a specific ambient credential, the explicit `env` field is the supported path; a configurable scrub can be reconsidered then. +An earlier sketch of this work also proposed making `SENSITIVE_ENV_PATTERN` configurable. Validating against the code, that is **speculative and already subsumed**: `run.ts` documents a configurable whitelist as future work, and the new explicit `env` field — merged after the scrub — already gives a caller full control, including over credential-shaped vars. There is no current caller that needs to *broaden* the ambient scrub (the hazard runs the other way). Adding a config knob now would be a speculative surface with no consumer. If a real workflow ever needs to forward a specific ambient credential, the explicit `env` field is the supported path; a configurable scrub can be reconsidered then. ## Consequences diff --git a/docs/rfc/implemented/feature/2026-06-22-acp-subagent-backend.md b/docs/rfc/implemented/feature/2026-06-22-acp-subagent-backend.md index 6be0f6746b..aa56483f32 100644 --- a/docs/rfc/implemented/feature/2026-06-22-acp-subagent-backend.md +++ b/docs/rfc/implemented/feature/2026-06-22-acp-subagent-backend.md @@ -36,7 +36,7 @@ The child is a separate process, so it inherits an environment. Credential-shape ## Testing -Designed at every tier the backend touches, per the AGENTS.md "design test infrastructure up front" rule: +Designed at every tier the backend touches, per the root AGENTS.md rule that a new capability shape names its coverage at every tier at plan time: - **Keyless unit/integration** (`subagent-acp.spec.ts`): spawns a scripted mock ACP server subprocess (`tests/mock-acp-server.ts`) and drives it through the real backend over real ACP stdio. Covers: the prompt round-trip + output accumulation; every StopReason mapping; cancellation via `run.cancel()` and via the request signal; the already-aborted-before-start case; the cancel-races-ahead-of-newSession case; a torn-pipe-after-cancel (child crashes on cancel) settling `aborted`; permission auto-answer under both policies (including the allow-policy-no-allow-option fallback); a non-message update consumed but not accumulated; a nonexistent-command spawn failure settling `error`; HMR provider cleanup; and the namespace export shape. 100% per-file coverage. - **With-key e2e** (`subagent-acp.e2e.ts`): the harness drives ITSELF — the backend spawns the real `acp-agent` example process and a real model in that child answers a prompt (PONG) and does real file work (writes `proof.txt`, verified on disk). Self-skips without `DEEPSEEK_API_KEY`. This is the "talk to our own process" smoke and the out-of-process analogue of the in-process spawn e2e. diff --git a/docs/rfc/implemented/process/2026-07-02-tool-schema-catalog.md b/docs/rfc/implemented/process/2026-07-02-tool-schema-catalog.md index 96355941b3..a085568429 100644 --- a/docs/rfc/implemented/process/2026-07-02-tool-schema-catalog.md +++ b/docs/rfc/implemented/process/2026-07-02-tool-schema-catalog.md @@ -19,7 +19,7 @@ The cordis catalog is a pure TypeScript-AST pass because every event/service nam - `tool-subagent`'s tool name is `config.toolName ?? 'subagent'` — chosen at load, not a literal. - An MCP plugin can register **raw JSON Schema** directly via `ctx.tools.register()` without `defineTool` at all, so enumerating `defineTool(` call sites structurally under-counts. -The only faithful source of truth is the schema the registry actually holds after the plugin loads. Booting is the [unit-test discipline](../../../../AGENTS.md) "verify the world, not a synthetic stand-in" applied to a doc generator: read the shipped artifact, not a re-derivation of it. +The only faithful source of truth is the schema the registry actually holds after the plugin loads. Booting is the [testing-policy discipline](../../../testing.md) "verify the world, not the self-report" applied to a doc generator: read the shipped artifact, not a re-derivation of it. ### Restoring "nothing silently omitted" 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 95d168953e..42b2e55ffd 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 @@ -22,7 +22,7 @@ ci.yml's value is that it is keyless, forkable, and always-green: any contributo ### Cost is not the constraint; reliability is -The usual reason to ration real-API CI — token cost — does not apply here: we are DeepSeek and internal inference is effectively free. So the design optimizes for *coverage and signal*, not for minimizing calls. The suite runs in full (all six `*.e2e.ts` files), on multiple triggers, on every trusted PR. This is the CI embodiment of the AGENTS.md "lean on with-key e2e tests" policy. +The usual reason to ration real-API CI — token cost — does not apply here: we are DeepSeek and internal inference is effectively free. So the design optimizes for *coverage and signal*, not for minimizing calls. The suite runs in full (all six `*.e2e.ts` files), on multiple triggers, on every trusted PR. This is the CI embodiment of the [docs/testing.md](../../../testing.md) with-key policy. ### Triggers: trusted events only diff --git a/docs/rfc/rejected/simplification/2026-06-20-drop-bash-output-spill-files.md b/docs/rfc/rejected/simplification/2026-06-20-drop-bash-output-spill-files.md index a6a47c66a0..8f971bb15a 100644 --- a/docs/rfc/rejected/simplification/2026-06-20-drop-bash-output-spill-files.md +++ b/docs/rfc/rejected/simplification/2026-06-20-drop-bash-output-spill-files.md @@ -20,7 +20,7 @@ This proposal can land independently of [a generic long-running tool runtime](.. - `OutputCollector` keeps bounded buffers only and deletes the temp-file machinery. - `renderResult()` reports truncation without a filesystem path. - Tests cover tail truncation and no longer assert full-output file contents. -- Security guidance in [root AGENTS.md](../../../../AGENTS.md) stops treating private spill files as a model-visible interface. +- Security guidance in [docs/defensive-patterns.md](../../../defensive-patterns.md) stops treating private spill files as a model-visible interface. ## What we give up diff --git a/packages/todo/tool-todo/src/index.ts b/packages/todo/tool-todo/src/index.ts index 01d862e4d5..d2175f49b9 100644 --- a/packages/todo/tool-todo/src/index.ts +++ b/packages/todo/tool-todo/src/index.ts @@ -51,8 +51,8 @@ const DESCRIPTION = * `InferArgs` maps an `enum` string prop to plain `string`, so the compiler sees * `args.todos` as `{ content: string; status: string }[]`; the * `status as TodoItem['status']` narrowing records that registry guarantee - * rather than re-checking it (an unreachable re-check would be dead code — see - * AGENTS.md "don't validate scenarios that can't happen"). What remains is the + * rather than re-checking it (an unreachable re-check would be dead code the + * coverage gate would flag). What remains is the * value rules the DSL has no vocabulary for: non-empty unique content (stored * trimmed, so the persisted value matches the dedupe/length key), and at most * one `in_progress` task. diff --git a/packages/ui/acp/src/index.ts b/packages/ui/acp/src/index.ts index d908f3d2db..70aa7a9493 100644 --- a/packages/ui/acp/src/index.ts +++ b/packages/ui/acp/src/index.ts @@ -284,7 +284,7 @@ export function apply(ctx: Context, config: AcpConfig): void { // sessionUpdate returns a promise; a closed connection rejects it. The // update is best-effort UI feed, never load-bearing for correctness, so a // throwing/rejecting send must not break the turn (the chunk is emitted - // inside the model step — see AGENTS.md "contain callback exceptions"). + // inside the model step — see docs/defensive-patterns.md "contain callback exceptions"). /* v8 ignore next 3 -- the rejection only fires on a stdout/connection write failure (closed pipe), which the in-memory test transport never induces; the swallow is a defensive best-effort guard like the loop's emit traps */ @@ -639,7 +639,7 @@ export function apply(ctx: Context, config: AcpConfig): void { conn = new AgentSideConnection(makeAgent, stream) /** - * Tear ALL live sessions down to quiescence (AGENTS.md "dispose must reach + * Tear ALL live sessions down to quiescence (docs/defensive-patterns.md "dispose must reach * quiescence"): for each session settle any pending prompt `cancelled`, then * run that session's {@link AgentHandle} `dispose()` — which stops the loop * (sets `disposed`, aborts the in-flight step), AWAITS the loop's exit (the @@ -892,7 +892,7 @@ export class ToolPresenter { * @param onError invoked when a tool's `presentCall`/`presentResult` THROWS; * the presenter swallows the error and falls back to the generic * presentation so a buggy display callback can never fail a live turn or a - * `session/load` replay (AGENTS.md "contain callback exceptions at the + * `session/load` replay (docs/defensive-patterns.md "contain callback exceptions at the * boundary"). Defaults to a no-op for callers that don't supply a logger. */ constructor( diff --git a/packages/ui/acp/tests/harness.ts b/packages/ui/acp/tests/harness.ts index 664ffbea5a..01a5d30abc 100644 --- a/packages/ui/acp/tests/harness.ts +++ b/packages/ui/acp/tests/harness.ts @@ -158,7 +158,7 @@ export async function makeBridgeHarness(options: { * Plug the REAL `dsh-bash-local` executor + `dsh-tool-bash` tools (instead of * a test's own inline tool). Lets a test drive the actual `bash` tool — its * real `presentCall`/`presentResult` — through the bridge, so tool-call UI - * tests verify the SHIPPING tool, not a stand-in (AGENTS.md "prefer the real + * tests verify the SHIPPING tool, not a stand-in (docs/testing.md "prefer the real * implementation over a mock in tests"). */ withBash?: boolean diff --git a/packages/ui/acp/tests/load.spec.ts b/packages/ui/acp/tests/load.spec.ts index c07ef93274..2fbc95b3d9 100644 --- a/packages/ui/acp/tests/load.spec.ts +++ b/packages/ui/acp/tests/load.spec.ts @@ -62,7 +62,7 @@ describe('acp bridge — session/load replay', () => { // bridge. The replayed tool_call/tool_call_update must carry the tool's OWN // presentation — identical to how it streamed live — via a throwaway // presenter that pairs call→result as the log replays in order. Uses the - // shipping tool (withBash), not a stand-in (AGENTS.md "prefer the real + // shipping tool (withBash), not a stand-in (docs/testing.md "prefer the real // implementation over a mock in tests"). live = await makeBridgeHarness({ storageDir, diff --git a/packages/ui/acp/tests/stream-update.spec.ts b/packages/ui/acp/tests/stream-update.spec.ts index b580a5fc3d..2b4fcbd74f 100644 --- a/packages/ui/acp/tests/stream-update.spec.ts +++ b/packages/ui/acp/tests/stream-update.spec.ts @@ -287,7 +287,7 @@ describe('ToolPresenter (tool-owned presentation via the tool registry)', () => it('a THROWING presentCall/presentResult is contained: generic fallback + onError, never propagates', () => { // A buggy tool whose display callbacks throw must NOT fail a live turn or a - // session/load replay (AGENTS.md "contain callback exceptions at the + // session/load replay (docs/defensive-patterns.md "contain callback exceptions at the // boundary"). The presenter swallows the throw, reports via onError, and // falls back to the generic presentation. const boom: ToolDefinition = { @@ -379,7 +379,7 @@ describe('ToolPresenter (tool-owned presentation via the tool registry)', () => it('forwards fs-tool render intents onto the wire (REAL read → generic locations, edit → diff content)', async () => { // Use the SHIPPING fs tools (not a stand-in), booted through their real // plugins, so the wire tool_call carries the actual presentCall output — - // read's follow-along `locations` and edit's `diff` content block. (AGENTS.md + // read's follow-along `locations` and edit's `diff` content block. (docs/testing.md // "prefer the real implementation over a mock".) const ctx = new Context() await ctx.plugin(SystemPrompt) diff --git a/packages/web/web-search-deepseek/tests/deepseek.e2e.ts b/packages/web/web-search-deepseek/tests/deepseek.e2e.ts index d06e384b31..ae4dc3bf6a 100644 --- a/packages/web/web-search-deepseek/tests/deepseek.e2e.ts +++ b/packages/web/web-search-deepseek/tests/deepseek.e2e.ts @@ -10,7 +10,7 @@ import { /** * Real-API smoke for the DeepSeek search provider. Self-skips without - * `$DEEPSEEK_API_KEY`, per the with-key e2e policy in AGENTS.md § Secrets. This + * `$DEEPSEEK_API_KEY`, per the with-key e2e policy in docs/testing.md. This * is the only test that proves DeepSeek's Anthropic-compatible endpoint actually * triggers native `web_search` and returns the structured result blocks the * provider parses — a mock cannot confirm the wire shape is real. diff --git a/packages/web/web-search-perplexity/tests/perplexity.e2e.ts b/packages/web/web-search-perplexity/tests/perplexity.e2e.ts index 9414d46937..02aaa914e6 100644 --- a/packages/web/web-search-perplexity/tests/perplexity.e2e.ts +++ b/packages/web/web-search-perplexity/tests/perplexity.e2e.ts @@ -3,7 +3,7 @@ import { PerplexitySearchProvider, PERPLEXITY_DEFAULT_BASE_URL, PERPLEXITY_DEFAU /** * Real-API smoke for the Perplexity search provider. Self-skips without - * `$PERPLEXITY_API_KEY`, per the with-key e2e policy in AGENTS.md § Secrets. + * `$PERPLEXITY_API_KEY`, per the with-key e2e policy in docs/testing.md. */ const apiKey = process.env.PERPLEXITY_API_KEY const maybe = apiKey !== undefined && apiKey.length > 0 ? describe : describe.skip