From 9a5a3835c83b88fd829ea922adacf2c6d49b238e Mon Sep 17 00:00:00 2001
From: Tianyi Cui <53024+tianyicui@users.noreply.github.com>
Date: Fri, 19 Jun 2026 04:28:09 +0800
Subject: [PATCH] ci+fix: run snapshot tests in CI and load .env only when
 recording
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Holistic-review fixes for integration gaps the per-commit reviews missed:

- CI now runs `pnpm run test:snapshot` (a step after the coverage gate). It was
  wired into pre-push but not .github/workflows/ci.yml, so the RFC/AGENTS claim
  that snapshot replay runs in the default PR gate was only half-true — CI is
  the real gate.
- vitest.snapshot.config.ts loads the repo .env ONLY when DSH_SNAPSHOT=record.
  Loading it unconditionally contradicted the replay safety story (replay must
  never reach the network), and runScenario forwards process.env to the child.
  Non-ENOENT load errors now surface instead of being swallowed.
- start.ts: the graceful-shutdown comment said "RECORD runs" but the path
  applies to both snapshot modes (replay also closes stdin → dispose → exit).
- docs/development.md: list the new pre-push snapshot job and the CI snapshot
  gate.
---
 .github/workflows/ci.yml    |  9 +++++++++
 docs/development.md         |  3 ++-
 examples/acp-agent/start.ts | 14 ++++++++------
 vitest.snapshot.config.ts   | 18 +++++++++++-------
 4 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index f07f01b9ec..5b9a9fca4f 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -60,6 +60,15 @@ jobs:
       - name: Tests with coverage gate (per-file 100%)
         run: pnpm run test:coverage
 
+      # ACP snapshot tests (acp-snapshot-tests RFC): boot the real acp-agent
+      # subprocess and replay recorded session-log fixtures, diffing the
+      # normalized stdout transcript + re-persisted log against committed
+      # goldens. KEYLESS by design — the same `test:snapshot` script the pre-push
+      # hook runs (one source of truth), so the full-transcript regression net
+      # is part of every PR gate, not just local pre-push.
+      - name: Snapshot tests (ACP transcript replay)
+        run: pnpm run test:snapshot
+
       # Before hygiene: publint validates the packed artifacts (lib/index.js),
       # which only the tsdown bundling step emits.
       - name: Build (tsc -b + tsdown bundles)
diff --git a/docs/development.md b/docs/development.md
index 144860a4ff..546be5dc86 100644
--- a/docs/development.md
+++ b/docs/development.md
@@ -57,7 +57,7 @@ DEEPSEEK_BASE_URL=https://... # optional
 lefthook is configured in `lefthook.yml` as an early local checkpoint before review:
 
 - `pre-commit` runs staged-file ESLint fixes, `pnpm run typecheck`, and the vendor manifest guard.
-- `pre-push` runs `pnpm run test`, `pnpm run hygiene`, `pnpm run doc-sync`, and `pnpm run verify-module-graph`.
+- `pre-push` runs `pnpm run test`, `pnpm run test:snapshot`, `pnpm run hygiene`, `pnpm run doc-sync`, and `pnpm run verify-module-graph`.
 
 The vendor manifest guard checks that changes under `vendor/*/src` are staged with the matching `vendor/README.md` manifest update. See `vendor/README.md` before editing vendored code.
 
@@ -74,6 +74,7 @@ The GitHub workflow runs these gates on each pull request:
 - `pnpm run doc-sync`
 - `pnpm run verify-module-graph`
 - `pnpm run test:coverage`
+- `pnpm run test:snapshot`
 - `pnpm run build`
 - `pnpm run knip && pnpm run publint`
 - an echo-agent smoke test that checks the demo's tool call, tool result, and JSONL output
diff --git a/examples/acp-agent/start.ts b/examples/acp-agent/start.ts
index 01bcc32093..028308cb4a 100644
--- a/examples/acp-agent/start.ts
+++ b/examples/acp-agent/start.ts
@@ -45,12 +45,14 @@ await ctx.loader.create({
   },
 })
 
-// Graceful shutdown for snapshot RECORD runs: when the client closes our stdin
-// (it is done driving the session), dispose the whole context. Disposal awaits
-// the agent-loop teardown and the persistence backend's final `session/flush`,
-// so the recorded `.jsonl` is fully written before the process exits and the
-// harness harvests it. (In a normal editor session stdin stays open for the
-// connection's lifetime; the editor kills the process, so this never fires.)
+// Graceful shutdown for snapshot runs (both replay and record): when the client
+// closes our stdin (it is done driving the session), dispose the whole context.
+// Disposal awaits the agent-loop teardown and the persistence backend's final
+// `session/flush`, so the session `.jsonl` is fully written before the process
+// exits and the harness harvests it (and the subprocess exits cleanly so the
+// harness's waitForExit resolves). (In a normal editor session stdin stays open
+// for the connection's lifetime; the editor kills the process, so this never
+// fires.)
 if (snapshotMode !== undefined) {
   process.stdin.on('end', () => {
     void ctx.fiber.dispose().then(() => { process.exit(0) })
diff --git a/vitest.snapshot.config.ts b/vitest.snapshot.config.ts
index 1fb7ee48a4..6dc4144044 100644
--- a/vitest.snapshot.config.ts
+++ b/vitest.snapshot.config.ts
@@ -8,13 +8,17 @@ import { defineConfig } from 'vitest/config'
 // `pnpm run test:snapshot:record` (DSH_SNAPSHOT=record + -u) re-records the
 // fixtures against the real API and refreshes the goldens.
 //
-// Replay loads no .env; record reads DEEPSEEK_API_KEY from the env or a
-// gitignored repo-root .env (loaded here, mirroring the e2e config), so a
-// contributor with a key only in .env can still record.
-try {
-  process.loadEnvFile(new URL('.env', import.meta.url).pathname)
-} catch {
-  // No .env — fine; replay needs no key and record reads it from the env.
+// Replay loads no .env (it must never reach the network — a recorded fixture
+// drives the model). Record reads DEEPSEEK_API_KEY from the env or a gitignored
+// repo-root .env, so a contributor with a key only in .env can still record.
+if (process.env.DSH_SNAPSHOT === 'record') {
+  try {
+    process.loadEnvFile(new URL('.env', import.meta.url).pathname)
+  } catch (error) {
+    // ENOENT (no .env) is fine — the key may already be in the environment.
+    // Surface any other failure rather than silently recording with wrong env.
+    if ((error as NodeJS.ErrnoException | null)?.code !== 'ENOENT') throw error
+  }
 }
 
 export default defineConfig({