From f09cc81c03e62b8f48b921a050bbbc0d7b821c0c Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Fri, 19 Jun 2026 04:14:22 +0800 Subject: [PATCH] docs: require a snapshot test for transcript/UX-affecting changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documents the snapshot tier and makes its use a convention. AGENTS.md gains the test:snapshot / test:snapshot:record commands (and corrects the now-stale `pnpm run test` include comment — the unit suite picks up examples/*/tests too), plus a Conventions bullet: a change affecting the editor-facing transcript or end-to-end agent UX needs a snapshot test (or an explicit note why none applies); a pure internal refactor is exempt. The dsh-code-review skill gains a matching reviewer-only check — review the golden diff itself, since a changed *.golden is a behavior change in disguise. --- .agents/skills/dsh-code-review/SKILL.md | 1 + AGENTS.md | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index 7d4dcd6cd0..8d0792624c 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -43,6 +43,7 @@ Where your independent reasoning earns its keep. Start here, then keep going acr - **Plugin export shape + real-loader coverage.** A new/changed `cordis.yml`-loaded plugin: is it a function/namespace plugin (`name`/`inject`/`Config`/`apply` named exports) with NO `export default`? A stray default export makes the Loader's `unwrapExports` drop `inject` and the plugin crashes at load with `cannot get property … without inject` — invisible to hand-built `ctx.plugin({...})` tests and to line coverage. Confirm there's a test driving it through the REAL loader path (the no-key subprocess e2e for ACP is the model). And any opportunistic read of a service NOT in `static inject` should use `ctx.get(name)`, not `ctx.` (the property proxy throws through a foreign shadow). See [packages/AGENTS.md](../../../packages/AGENTS.md) and [docs/postmortem/0001](../../../docs/postmortem/0001-acp-default-export-drops-inject.md). - **Seam discipline.** New swappable capability? Check it's split per the capability-seams RFC (interface / impl / consumer), and that the consumer injects the interface key, never an implementation type. - **Test quality.** A test that passes but asserts the wrong thing is worse than none. Check that new tests would actually fail if the behavior regressed, and that they exercise the contract (events fired, disposal reached) rather than restating the implementation. +- **Snapshot coverage for transcript/UX changes.** If the PR changes the editor-facing transcript or end-to-end agent UX — the ACP bridge's event→update translation, the agent loop's observable output, tool presentation, or anything an editor renders — it must add or update a snapshot scenario (`examples/*/tests/**/*.snapshot.ts`, goldens under `examples/acp-agent/tests/snapshots/`) or note explicitly why none applies (AGENTS.md § Conventions). Review the golden diff itself: a changed `stdout.golden.txt` / `session.golden.txt` is a behavior change in disguise — confirm it's intended, not an accidental regression someone re-recorded away. A pure internal refactor with no observable-output change is exempt, but the PR should say so. See [docs/rfc/implemented/2026-06-19-acp-snapshot-tests.md](../../../docs/rfc/implemented/2026-06-19-acp-snapshot-tests.md). - **Intent and contracts.** Does the change do what the PR says, and honor the documented contract on *both* sides of every seam it touches (see AGENTS.md "Honor cross-seam contracts on BOTH sides")? ## How to respond diff --git a/AGENTS.md b/AGENTS.md index d4b8474865..96861b93ec 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -60,10 +60,18 @@ scripts/ repo maintenance scripts (vendor-manifest guard, publint runner). ```sh pnpm install # pnpm workspaces, node >= 24 -pnpm run test # vitest run (packages/*/tests/**/*.spec.ts) +pnpm run test # vitest run (packages|examples/*/tests/**/*.spec.ts) pnpm run test:coverage # vitest run --coverage (per-file 100% gate on packages/*/src) pnpm run test:e2e # real-API tests (packages|examples/*/tests/**/*.e2e.ts); # self-skips without DEEPSEEK_API_KEY — see Secrets below +pnpm run test:snapshot # ACP snapshot tests (examples/*/tests/**/*.snapshot.ts): + # boot the real acp-agent subprocess, replay a recorded + # session JSONL, diff the normalized stdout + re-persisted + # log against committed goldens. KEYLESS — runs in the + # default gate. Filter one: `pnpm run test:snapshot -- -t `. +pnpm run test:snapshot:record # re-record fixtures + goldens against the real + # API (needs DEEPSEEK_API_KEY); accept-the-diff = re-record + # (or `pnpm run test:snapshot -- -u` to refresh goldens only) pnpm run typecheck # tsc -b tsconfig.build.json (declarations) + tsc -p # tsconfig.typecheck.json (tests/examples typecheck too) pnpm run lint # eslint . @@ -122,6 +130,7 @@ Dev/test/demo run **unbuilt** via tsx + the `paths` map in the root `tsconfig.js - **TODO markers**: use `FIXME`/`TODO`/`XXX` to flag known issues by urgency — see [docs/development.md](docs/development.md) for the semantics of each. - **Tests**: vitest, colocated under `packages//tests/*.spec.ts`. Every registry needs an HMR-safety test (dispose the contributing fiber, assert cleanup). **Excessive tests are welcome** — when in doubt, write the test; err on the side of covering edge cases, error paths, event ordering, and concurrency races even if they seem unlikely. Review findings get regression tests (see `packages/agent-loop/tests/review-fixes.spec.ts`). The same generosity applies to **real-API (with-key) e2e tests — inference is cheap here (we are DeepSeek), so do not ration them**: cover the agent's real flows (a real prompt that writes a file, multi-turn, tool use, cancellation) and run them frequently while developing, especially cheap **smoke tests** that boot the real example and check the world. A green mock/no-key suite proves the plumbing, not the product — the with-key smoke test is what catches "green units, broken product". See § Secrets / .env for the with-key policy and why self-skip is a CI accommodation, not a verdict that real-API tests are expensive. - **Prefer the REAL implementation over a mock/stand-in in tests.** When the genuine collaborator is available in the repo, wire it up instead of hand-rolling a fake — a test that registers an inline `defineTool({ name: 'bash', … })` to stand in for `dsh-tool-bash` proves the *bridge* moves bytes but not that the *shipping tool* renders the way the test asserts; the two drift and the test passes while the product is wrong. Mock only the genuinely expensive/non-deterministic boundary (the LLM adapter, the network, the clock) and keep everything downstream real: a bridge tool-call test runs the scripted mock MODEL but the REAL tool + REAL executor (e.g. `makeBridgeHarness({ withBash: true })` plugs `dsh-bash-local` + `dsh-tool-bash` and runs an actual `echo`), so it verifies the actual `presentCall`/`presentResult` an editor sees. This is the unit-test echo of "verify the world, not a synthetic stand-in" (see § Defensive patterns) — a fake you wrote will agree with whatever you assumed; the real thing won't. +- **A change that affects the editor-facing transcript or end-to-end agent UX needs a snapshot test (or an explicit note in the PR why none applies).** The snapshot tier (`examples/*/tests/**/*.snapshot.ts`, `pnpm run test:snapshot`) boots the real example subprocess, replays a recorded session JSONL deterministically (keyless), and diffs the normalized stdout transcript + re-persisted session log against committed goldens — the full-transcript regression net that mock-level unit tests structurally cannot be (it is what catches a bridge-translation or loop-structure regression that leaves every unit green). When you change the ACP bridge, the agent loop's observable output, tool presentation, or anything an editor renders, add or update a scenario under `examples/acp-agent/tests/snapshots/` and re-record with `pnpm run test:snapshot:record`. Reviewing the golden diff is part of the review. The rule is scoped to transcript/UX-affecting changes — a pure internal refactor with no observable-output change does not need one, but say so. See [docs/rfc/implemented/2026-06-19-acp-snapshot-tests.md](docs/rfc/implemented/2026-06-19-acp-snapshot-tests.md). ## Defensive patterns (hard-won)