Files

T

kingwl 1097fa3507 fix review finding: an impossible scripted permission click rejects the run

A client-callback throw only becomes a JSON-RPC error RESPONSE to the
agent's session/request_permission — runScenario itself kept going, so a
tolerant agent could treat the error as a denial and the scenario would
pass, or worse, record: the impossible click baked into fixture and
golden, green on every replay. The mismatch is now captured as a harness
error while the agent is answered plain cancelled (a well-defined path
it cannot reinterpret), and the step loop rejects the run on it as soon
as the in-flight step settles. The spec asserts the rejection instead of
the agent-side error echo.

2026-07-08 02:46:23 +08:00

7.5 KiB

Raw Blame History

RFC: Extract the ACP snapshot suite into a support package

Status: implemented

Problem

The ACP snapshot tier (snapshot RFC) was built from three modules living inside one example's test directory: snapshot-harness.ts (boot the real bin subprocess, drive it over ACP JSON-RPC, harvest the persisted logs), snapshot-normalize.ts (the pure golden normalizers), and the ~150-line scenario body plus fixture guards in acp.snapshot.ts (record/replay modes, the stdout-golden and log compares, the pinned-header uniformity guard, the orphan/required-file/single-pin meta-tests).

A second ACP example wanting snapshot coverage — the sandbox/approval composition is the immediate consumer — could only copy those modules, forking exactly the logic that must not drift: record write-back, header scrubbing, child-session harvest ordering. The spawn/client glue was already triplicated across acp.e2e.ts, hooks.e2e.ts, and the harness (TODO(acp-test-harness)). Location also decided test rigor: the per-file 100% coverage gate measures packages/*/*/src only, so none of this machinery was measured — the same gap that had moved dsh-llm-replay out of examples/ into packages/support. And the harness's ACP client hardcoded requestPermission → cancelled, so an approval round-trip — the headline behavior of the sandbox composition — could not be expressed at the snapshot tier at all.

Decision

The machinery lives in packages/support/acp-snapshot (@deepseek-ai/dsh-acp-snapshot); an example's *.snapshot.ts is its scenario table, its agent paths, and one factory call, over its own snapshots/ fixtures and cordis.snapshot.yml overlay (single-source replay config). Reading DSH_SNAPSHOT stays at that edge — the library takes a resolved mode.

src/harness.ts — runScenario and the input-script/result types, parameterized by an AgentUnderTest (binScript, configPath, tsconfigPath; absolute paths the consuming suite resolves from its own import.meta.url). The client's session/request_permission handler consumes an optional InputScript.permissionAnswers FIFO queue, each entry selecting by option kind (ids are agent-issued randoms a committed script cannot know; kinds are the ACP-stable vocabulary, mapped to the offered optionId at answer time); an absent or exhausted queue answers cancelled, and a kind the request never offered rejects the run — the agent itself is answered cancelled, so the scenario bug fails the harness rather than being absorbed as an agent-side denial. This is what lets an approval suite drive allow/reject round-trips deterministically from input.json.

src/normalize.ts — the pure normalizers, hook-free by policy: when a future event carries a new volatile field (an approval duration, say), the shared normalizer learns it in the same change, keeping one home for what "normalized" means rather than per-suite scrub extensions.

src/suite.ts — the Scenario type and defineAcpSnapshotSuite(options), registering the per-scenario compares, record-mode fixture write-back, the header pin with its live uniformity guard, and the fixture guard block (no orphan scenario dirs, required files present, exactly one pin, non-pinning fixtures are scrubRequestHeaders fixed points). The pinned-header contract (pinned-header RFC) is per-suite: each suite flags exactly one pinsHeader scenario (the factory throws on zero, a meta-test rejects more than one; WHICH scenario pins is the table's reviewable choice), and the uniformity guard compares only that suite's sessions. The pure helpers (childFixturePaths, fixtureContext, normalizedHeaders, headerDeltaCount) are exported for direct unit coverage.

Alternatives considered

Copy the modules into each example — the fork this RFC exists to prevent: the record/guard logic is exactly the code that must stay byte-identical across suites, and examples are outside the coverage gate, so each copy is also unmeasured.
A shared module directory under examples/ — keeps the code outside the coverage gate and forces relative imports across example boundaries, against the package-name import convention; examples/ leaves stay thin by design.
A /testing subpath export of dsh-acp-agent — couples test infrastructure into a product package's surface and dependency set; packages/support/ exists precisely for real-but-lower-compatibility dev/test packages, with dsh-llm-replay as the precedent this package completes.
Export raw test-body functions instead of a suite factory — each example would re-own the describe/it skeleton (~80 lines of registration boilerplate per suite) for no flexibility gain; the factory keeps consumers to a scenario table plus one call, and the exported pure helpers preserve unit-testability inside the factory design.
An injectable ACP Client factory instead of declarative permissionAnswers — maximally flexible, but it leaks SDK client construction to every consumer and reopens per-example drift in exactly the layer being unified; a declarative queue keeps input.json the single scripting surface and stays golden-normalizable.
Generalize beyond ACP (a transport-agnostic snapshot harness) — no second transport exists; the harness is ACP-shaped end to end (SDK client, JSON-RPC frames, session/update waiters), and a speculative abstraction would be a seam split ahead of any consumer.

Testing

Extraction parity was proven mechanically: after the move, pnpm run test:snapshot matched the base commit's result with zero byte changes under examples/acp-agent/tests/snapshots/. The package's src/ holds per-file 100% statements/branches/functions/lines under the gating unit run, driven through the REAL spawn path by a scripted fake ACP bin (tests/fixtures/fake-acp-agent.ts, behavior scripted per scenario via a behavior.json beside the fixture): harness.spec.ts covers every step op, both expect-error arms, the permission queue (selection, fallback, impossible-click), env forwarding, workspace seeding, and the harvest ordering/noise/fallback branches; suite.spec.ts runs the factory for real at collection time — a replay suite over committed synthetic fixtures and a record suite over a temp copy (write-back never touches the committed tree; ACP_SNAPSHOT_SPEC_BOOTSTRAP=1 re-bootstraps it) — plus direct cases for the pure helpers. Two structurally unreachable guards carry reasoned v8 ignore comments. The fake bin substitutes the session/new cwd, not process.cwd(), into scripted logs, matching what the real bin's header carries (darwin realpaths /var/folders/… to /private/var/folders/…).

Consequences

A new example gets the whole snapshot tier from a scenario table plus fixtures — the sandbox branch merges master down and adds its own suite (own pin scenario, own overlay, fixtures via test:snapshot:record, approvals via permissionAnswers). The costs: suite.ts imports vitest, so the package is importable only inside a vitest run — a shape no other package has, stated in its README; each suite pins its own ~8 KB header fixture (a genuinely distinct composition deserves its own pin; an identical one would be caught by that suite's uniformity guard); and the e2e launcher duplication remains (TODO(acp-test-harness)) — the harness is the extraction target when that migration lands.

7.5 KiB Raw Blame History