A repo-wide Not Invented Here audit (ten parallel domain surveys covering every package group, scripts/, native/, vendor/ edges, python/, test infrastructure, and CI) asked of each hand-rolled surface whether a maintained external package or Node builtin deletes it with a net win. Policy: new implemented process note records that a dependency which genuinely deletes owned code is a preferred simplification (bar: net deletion, health, boundary fit, settled seams stay); root AGENTS.md carries the one-line rule and dsh-find-simplifications now surveys for hand-rolled-where-a-dependency-exists candidates. Findings, all bilingual from birth: - proposed/simplification: eventsource-parser for llm-deepseek SSE, node:timers/promises for three hand-rolled sleeps, turndown (or minimal 'entities') for tool-web HTML->markdown, gate-script consolidation onto mdast/parseArgs/globSync - proposed/testing: execa + parseArgs + loadEnvFile + vi.waitFor for hand-rolled test subprocess plumbing - proposed/process: pnpm/action-setup for symmetric CI caching - proposed/feature: evaluate landstrip before building a Windows sandbox launcher - rejected/simplification: ~30 swap verdicts recorded (vscode-jsonrpc, p-retry, Ajv, write-file-atomic, msw, hono, better-sqlite3, wireit, landstrip-for-linux, YAML consolidation, ...) so the survey is not re-litigated from scratch Also drops the stale prompt/ entry from the AGENTS.md layout map (workspace instructions live in packages/context/workspace-context).
5.3 KiB
Agent Note: Adopt execa for hand-rolled test subprocess plumbing
Status: proposed
English | 中文
Problem
Roughly ten e2e/smoke files re-derive the same spawn-collect-timeout choreography by hand: let stdout = '' accumulation with setEncoding and data handlers, a setTimeout → kill('SIGKILL') deadline, and once('exit')/once('error') settlement, each with small variations. The sites: the inner spawn block of runLoaderSmoke (packages/support/loader-smoke/src/index.ts), runBuiltBin in apps/cli/tests/built-bin.e2e.ts and packages/examples/cli-demo/tests/built-bin.e2e.ts, runBinExpectingExit in packages/examples/acp-demo/tests/built-bin.e2e.ts, the built-lib e2e helpers in lsp-local and code-runtime-worker, the outer collector of examples/tui-agent/tests/pty-harness.ts, examples/jsonrpc-agent/tests/keyless-smoke.e2e.ts, and partially apps/web/tests/smoke-real.e2e.ts and session-checkpoint-policy/tests/crash-recovery.e2e.ts. Net deletable: ~100–150 lines of test infrastructure.
Two related test-infra hand-rolls compound the case:
packages/support/llm-mock-server/src/cli.tshand-tokenizes 18--flag valueoptions (~45–60 lines of loop and value-extraction helpers) where thenode:utilparseArgsbuiltin is already the repo idiom (cli-demo,acp-demo,verify-runtime-closure.ts,packages/sdk/scripts).apps/web/tests/smoke-real.e2e.tsandapps/web/tests/scaffold.tscarry two verbatim copies of a regex.envparser (~20 lines) where theprocess.loadEnvFilebuiltin has exactly the required no-override semantics — and the vitest e2e/snapshot/web configs already load root.envwith it before these files run, making the copies arguably dead.- The snapshot harness hand-rolls three poll-until-deadline loops (
waitForPersistedTurnStart/waitForPersistedTurnEnd/waitForWorkspaceFileinpackages/support/acp-snapshot/src/harness.ts, ~55 lines) pluswaitForFileincrash-recovery.e2e.ts, wherevi.waitFor/expect.pollcover the shape — vitest is already a runtime dependency ofdsh-acp-snapshot, so this adds nothing.
Proposal
- Add
execaas a root devDependency and rewrite the spawn-collect-timeout sites ontoawait execa(cmd, args, { cwd, env, timeout, killSignal: 'SIGKILL', reject: false }), whose result reports{ stdout, stderr, exitCode, signal, timedOut }as independent fields — matching the repo's own defensive-patterns rule to report orthogonal subprocess outcomes independently. Keep the genuinely custom parts custom: cli-demo's interrupt-on-marker mid-stream logic, jsonrpc's line-predicate protocol driving, and crash-recovery's SIGKILL-at-failpoint choreography. - Swap
llm-mock-server's CLI tokenizer forparseArgs(numeric coercion, bounds, and cross-option constraints stay manual; pinned error-message texts update with the tests). - Delete both
loadRootEnvcopies in favor ofprocess.loadEnvFilein a try/catch, or remove them outright if the vitest-config loading already covers them. - Replace the four poll loops with
vi.waitFor/expect.poll, passing explicit{ interval, timeout }and throwing descriptive errors from the callback.
Alternatives considered
tinyexecinstead of execa. Already innode_modulestransitively via vitest, smaller API — but no kill-escalation, no rich error output embedding, and being transitive is not a contract; if the lighter package is preferred the swap shape is identical.- A repo-local shared spawn helper (no new dep). Viable and cheaper on supply chain, but it keeps the maintenance of deadline/kill/settlement logic in-repo when a battle-tested package owns exactly this; contrary to the dependency policy, it also has to re-earn Windows behavior (taskkill, exit codes) that execa already carries.
get-port,wait-on,tempy,tree-kill. Rejected individually: the repo's single port probe is break-even, the file waits are dominated byvi.waitFor, temp-dir handling already usesmkdtemp+rm {recursive}builtins everywhere, and acp-snapshot'sclose()is drain-ordering logic, not tree traversal.
Acceptance criteria
- The listed sites spawn through execa (or the chosen equivalent); the hand-rolled collect/timeout blocks and the two
/* v8 ignore */un-inducible OS-error branches inloader-smokeare gone. llm-mock-serverCLI parses viaparseArgs; its cli spec passes with updated message expectations.- No hand-rolled
.envparser remains underapps/web/tests. - The affected e2e and snapshot suites pass on both POSIX and Windows CI lanes.
Risks
loader-smokeis asrc/file under the per-file-100% coverage gate; the swap actually simplifies its coverage story (removes un-inducible branches) but the new call shape needs coverage.- Each rewritten e2e must be re-run on both platforms; subtle differences in kill escalation or stdin-close semantics (
input: ''for loader-smoke's stdin-close contract) are the risk to verify per site. - execa is a new root devDependency (currently absent from the lockfile entirely); it is one of the most-depended-on packages on npm and actively maintained, so health is not a concern, but the exe/runtime closure is unaffected either way (tests only).