diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 65fd446..387e05d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,5 +39,11 @@ jobs: - name: Type check run: bun run typecheck + - name: Build + run: bun run build + - name: Run tests run: bun run test + + - name: Run e2e dogfood spec + run: bun run test:e2e diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d99bb4..1b82733 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Displays monorepo warnings with detected markers - Reports nearby existing FlowSpec configuration and specs directories +- **CLI surface adapter** (PRD-0007): a flow can now declare `surface: cli` and drive a command-line tool instead of a browser — no `agent-browser` involved, and a CLI-only project doesn't need it installed at all. A flow with no `surface` key, or an explicit `surface: web`, parses and runs exactly as before; the entire existing web test suite passed unmodified throughout this mission. Mixing web and CLI verbs in one flow is a parse error naming the offending verb and the flow's surface (#800, #812). + - **CLI step grammar**: `run` (a whitespace-split string or an untouched array), plus optional `stdin`, `env`, `timeout`, and `expect_exit` (#800). + - **No-shell process execution** (`src/exec.ts`): commands are spawned directly via argv — never through a shell — so shell interpolation is structurally impossible rather than filtered or escaped. A hung command is killed (`SIGTERM`, escalating to `SIGKILL`) at its timeout, and stdout/stderr are captured independently up to a configurable byte limit, with truncation made visible (#801, #806). + - **Surface-agnostic matchers** (`src/matchers.ts`): reusable `contains`, `regex`, and dot-path JSON comparison, reporting structured failures instead of throwing (#802). + - **Retryable file matchers** (`src/file-matchers.ts`): file-existence and file-content checks that poll within the flow's timeout, for files an async process may still be writing (#803). + - **Per-flow working directory** (`src/workdir.ts`): each CLI flow gets a fresh temporary directory, deleted on success and kept (with its path printed in the failure report) on failure — the CLI analog of the web surface's failure screenshot (#804). + - **Eight CLI assertions**: `exit_code`, `stdout_contains`, `stdout_matches`, `stderr_contains`, `stderr_matches`, `file_exists`, `file_contains`, and `json_output`, evaluated against the last run step's captured result (`src/cli-assertions.ts`), plus widened `FlowError` fields carrying exit code, stdio excerpt, and working directory (#805, #809). + - **CLI runner** (`src/cli-runner.ts`): fail-fast exit-code semantics — a non-final step's exit code must match its `expect_exit` (default 0) or the flow fails immediately, while the final step's exit code is only fatal if it declares its own `expect_exit`, making "this command should fail" a first-class spec. A CLI flow can also declare its own flow-level `setup` phase, in the same grammar, run before `steps` in the same working directory; a setup failure is reported as a setup failure, not a step failure (#808, #811). + - **`runFlow` dispatches on surface** (`src/runner.ts`): the CLI path never resolves or launches a browser (#812). + - **Reporter renders CLI failures** (`src/reporter.ts`) alongside web failures in one summary, naming the command, its exit code, and what its output actually was (#810). + - **Config gains `cwd` and `captureLimit`** (`src/config.ts`): CLI-surface-only config keys — a configured working directory and an output capture limit — that now survive the CLI merge instead of being silently dropped; both are ignored by web flows, and config-level `setup` stays web-only (#807). + +- **Dogfooding**: FlowSpec now specs its own `flowspec init` command. `specs/init.flow.yaml` is a protected, human-authored `surface: cli` spec (the repo's own PreToolUse hook blocks agent edits to `specs/**/*.flow.yaml` — see `adr/0005`) exercised by `bun run test:e2e`, which builds the CLI, links the `flowspec` binary, and runs the specs directory — locally and now as a CI step. This is the first time `test:e2e` has passed (#815, #816). + ### Changed - Config files that fail to load or validate — including an unset `${VAR}` reference — now consistently exit with code **2** and print the underlying message without an `"Unexpected error:"` prefix, before any flow is parsed or browser session opened. Exit code **1** continues to mean flows ran and at least one failed; **0** means all flows passed. This is now documented explicitly in the README's exit code table. @@ -61,6 +75,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `docs/specification.md`: the flow schema reference now lists `setup` as an optional flow field, so the spec format documentation no longer contradicts the shipped schema. - Added ADRs recording the mission's key design decisions: skipped flows are represented as an ordinary `FlowResult` with a `skipped` flag rather than a new status enum (`adr/0001`); config faults (parse/validation/unset `${VAR}`) exit 2 (`adr/0002`); and `${VAR}` interpolation is scoped to committed config, not flow specs, keeping specs immutable (`adr/0003`). +- **CLI surface documentation** (#814): README gains a new "CLI Flows (`surface: cli`)" section — step grammar, the no-shell rule and its two escape hatches, all eight assertions, working-directory semantics, CLI-flow `setup`, and the `expect_exit` fail-fast/final-step rule — plus a `cwd`/`captureLimit` subsection under Configuration File. `docs/specification.md` gets the matching schema reference, a full worked example, and simplified CLI-runner-dispatch pseudocode under Execution Modes. +- Added ADRs recording this mission's key design decisions: `expect_exit` is honored on every step, but only the final step's *absence* of `expect_exit` makes its exit code non-fatal (`adr/0004`); protected specs such as `specs/init.flow.yaml` are drafted outside `specs/` and moved in by a human, since the repo's own PreToolUse hook blocks agent writes there (`adr/0005`); and `specsDir`-based path discovery for `flowspec run` stays out of scope for this mission (`adr/0006`). + ## [0.1.2] - 2026-02-20 ### Fixed diff --git a/README.md b/README.md index 2294067..58390f4 100644 --- a/README.md +++ b/README.md @@ -92,6 +92,9 @@ flowspec run specs/ --timeout 10000 # Disable assertion retries (fail immediately) flowspec run specs/ --timeout 0 +# Give each CLI (surface: cli) run step longer before it is killed +flowspec run specs/ --step-timeout 300000 + # Send an extra HTTP header (repeatable) flowspec run specs/ --header "x-vercel-protection-bypass: $BYPASS_TOKEN" @@ -105,12 +108,38 @@ FlowSpec looks for `flowspec.config.yaml` in the current directory or parent dir ```yaml baseUrl: http://localhost:3000 -timeout: 10000 +timeout: 10000 # assertion retry budget (web + file assertions) +stepTimeout: 60000 # CLI run-step process deadline (surface: cli only) specsDir: specs/ ``` CLI options override config file values. +`timeout` and `stepTimeout` are two different clocks and are never +interchangeable: `timeout` is how long an assertion keeps being re-checked +before it fails, while `stepTimeout` is how long a single `surface: cli` run +step's process may live before it is killed. `--step-timeout` must be a +positive integer — `0` or a negative value is rejected with exit code 2, +since a zero-millisecond deadline would kill every step the instant it +starts. + +#### `cwd` and `captureLimit`: CLI-Surface Settings + +Two config keys exist only for `surface: cli` flows (see [CLI Flows](#cli-flows-surface-cli) below) and are ignored by web flows: + +```yaml +# flowspec.config.yaml +cwd: ./sandbox # optional — see "Working Directory" below +captureLimit: 1048576 # optional — bytes per captured stream, default 5 MB (5 * 1024 * 1024) +``` + +- **`cwd`** — the directory a CLI flow's commands run in. A relative path resolves against the directory FlowSpec itself was invoked from. When absent, each CLI flow gets its own fresh temporary directory instead (see [Working Directory](#working-directory)). +- **`captureLimit`** — the ceiling, in bytes, on how much of a command's stdout and stderr FlowSpec captures (each stream is bounded independently). Output beyond the limit is truncated with a `[truncated]` marker. There is no config-level default — an absent `captureLimit` means each CLI step falls back to the built-in 5 MB default at execution time, not at config-load time. + +Neither key has a CLI-flag equivalent, and neither affects web flows at all. + +**Config-level `setup` stays web-only.** The `setup` block described in [Setup: Shared Steps Before Every Flow](#setup-shared-steps-before-every-flow) above uses the web step grammar (`visit`, `click`, `fill`, `select`, `wait_for`) and is never applied to a CLI flow — a `run` step in config-level `setup` is a validation error. A CLI flow that needs setup work declares its own flow-level `setup` block, in the CLI step grammar, instead (see [CLI Flows](#cli-flows-surface-cli)). + #### Setup: Shared Steps Before Every Flow A `setup` block runs once per flow, inside that flow's own browser session, immediately before its `steps`. It's the way to establish state every flow needs — most commonly, authenticating against a password-protected preview deployment. @@ -179,7 +208,7 @@ flowspec run specs/ --base-url https://myapp-preview.vercel.app --header "x-verc #### ${VAR} Interpolation -String values in `flowspec.config.yaml` — `baseUrl`, `specsDir`, any string inside `setup`, and any value inside `headers` — support `${VAR_NAME}` references, resolved from `process.env` when the config is loaded: +String values in `flowspec.config.yaml` — `baseUrl`, `specsDir`, `cwd`, any string inside `setup`, and any value inside `headers` — support `${VAR_NAME}` references, resolved from `process.env` when the config is loaded: ```yaml baseUrl: https://preview-abc123.myshopify.dev?_ab=${PREVIEW_TOKEN} @@ -198,6 +227,8 @@ This keeps tokens and secrets out of committed config files. Set `PREVIEW_TOKEN` | 1 | One or more flows failed | | 2 | Parse error, a malformed `--header`, or a config file that fails to load or validate (invalid YAML, schema, or an unset `${VAR}`) | +A spec that mixes surfaces — a web verb (`visit`, `click`, ...) inside a `surface: cli` flow, or a `run` step inside a web flow — is a parse error and exits **2**, before any command runs or any browser opens. + ## Flow File Format Flow files use YAML with a simple structure: @@ -216,6 +247,19 @@ expect: - visible: Welcome back ``` +### `surface`: web (default) or cli + +An optional `surface` field picks the flow's grammar: + +```yaml +surface: web # default — omit the key entirely for the same effect +surface: cli # this flow drives a command-line tool instead of a browser +``` + +Absent (or explicit `surface: web`) is the browser-driven grammar documented on this page — byte-for-byte the same behavior as every flow written before `surface` existed. `surface: cli` switches the flow's `steps`, `setup`, and `expect` to the CLI grammar described in [CLI Flows](#cli-flows-surface-cli) below. A flow may not mix the two: a web verb (`visit`, `click`, ...) inside a `surface: cli` flow, or a `run` step inside a web flow, is a parse error naming the offending verb and the flow's surface (exit code 2 — see [Exit Codes](#exit-codes)). + +**Requires flowspec v0.2.0 or later.** An older FlowSpec binary silently drops the unrecognized `surface` key (top-level fields aren't strictly checked) and then tries to validate the flow's `steps` against the web-only grammar it knows — a CLI flow's `run` steps don't match any web action, so the result is a confusing parse failure rather than a clear "upgrade FlowSpec" message. Pin a minimum FlowSpec version in CI before adopting `surface: cli`. + ### Setup (Optional) A flow can declare its own `setup` block — steps that run once, in the same browser session, before `steps`. It uses the same step grammar as `steps` (see [Step Actions](#step-actions) below): @@ -239,6 +283,8 @@ A flow-level `setup` **replaces** any `setup` configured in `flowspec.config.yam ### Step Actions +These are the **web** step verbs (`surface: web`, the default). A `surface: cli` flow uses a different grammar entirely — see [CLI Flows](#cli-flows-surface-cli) below. + | Action | Description | Example | | ------ | ----------- | ------- | | `visit` | Navigate to a URL (relative or absolute) | `visit: /login` | @@ -249,6 +295,8 @@ A flow-level `setup` **replaces** any `setup` configured in `flowspec.config.yam ### Assertions +These are the **web** assertions. A `surface: cli` flow's `expect` block uses the eight CLI assertions in [CLI Flows](#cli-flows-surface-cli) below instead. + | Assertion | Description | Example | | --------- | ----------- | ------- | | `url` | Check current URL contains value | `url: /dashboard` | @@ -256,6 +304,125 @@ A flow-level `setup` **replaces** any `setup` configured in `flowspec.config.yam | `matches` | Check page content matches regex | `matches: "Order #\\d+"` | | `not_visible` | Check text is NOT on page | `not_visible: "Error"` | +## CLI Flows (`surface: cli`) + +A `surface: cli` flow drives a command-line tool instead of a browser: its `steps` run real commands, and its `expect` block checks exit codes, captured output, and files the commands wrote — no `agent-browser` involved at all. + +```yaml +name: build-succeeds +description: The production build completes and writes the expected bundle +surface: cli +steps: + - run: "npm run build" + - run: ["node", "-e", "console.log('done')"] + expect_exit: 0 +expect: + - exit_code: 0 + - file_exists: dist/bundle.js + - stdout_contains: "done" +``` + +### CLI Step Grammar + +| Field | Required | Description | +| ----- | -------- | ----------- | +| `run` | Yes | The command to execute — a string or an array (see [No Shell, Ever](#no-shell-ever) below) | +| `stdin` | No | Text written to the command's standard input, then the stream is closed | +| `env` | No | Environment variables overlaid onto the inherited environment for this step only (does not leak to other steps) | +| `timeout` | No | Milliseconds before the command is killed. Falls back to `--step-timeout` (or its `stepTimeout` config value, or the 60000ms default) when absent | +| `expect_exit` | No | The exit code this step must produce — see [Exit Codes Within a CLI Flow](#exit-codes-within-a-cli-flow) below | + +Note that a step's `timeout` is a hard deadline, not a retry window: the command is killed (`SIGTERM`, escalating to `SIGKILL` if it doesn't exit) the moment it elapses. It is a completely separate setting from the assertion retry budget, which is why the run-wide fallback for it is `--step-timeout`/`stepTimeout` rather than `--timeout`. `--timeout 0` therefore disables assertion retries without putting any command at risk of being killed. + +`run` accepts two forms: + +```yaml +steps: + - run: "node build.js --mode production" # string form + - run: ["node", "build.js", "--flow", "a b.yaml"] # array form +``` + +A worked example of every optional field together: + +```yaml +steps: + - run: ["flowspec", "run", "--flow", "checkout.flow.yaml"] + stdin: "y\n" + env: + NO_COLOR: "1" + timeout: 5000 + expect_exit: 0 +``` + +### No Shell, Ever + +CLI steps never invoke a shell. The **string form** of `run` is split on whitespace only — no quote handling, no metacharacter interpretation. `run: "echo a && echo b"` runs the single command `echo` with the literal arguments `a`, `&&`, `echo`, `b` — `&&` is not chaining anything, and a quoted substring like `"two words"` is **not** reassembled into one argument; it becomes two separate, literally-quoted tokens. + +Two escape hatches, for the two things a shell would otherwise be doing: + +1. **An argument containing spaces or quotes** — use the **array form**, where each element is passed through untouched: + + ```yaml + - run: ["node", "-e", "console.log('has a space')"] + ``` + +2. **Pipes, redirects, globbing, `&&` chaining, or anything else that genuinely needs a shell** — invoke a shell explicitly, or wrap the logic in a script file: + + ```yaml + - run: ["bash", "-c", "cat *.log | grep ERROR > errors.txt"] + # or: + - run: ["bash", "scripts/build-and-check.sh"] + ``` + +Both hatches are ordinary uses of the array form — there is no special "shell mode" flag. FlowSpec spawns exactly the command you wrote; if that command happens to be a shell, the shell does its own parsing on its own arguments, same as running it by hand. + +### CLI Assertions + +| Assertion | Description | Example | +| --------- | ----------- | ------- | +| `exit_code` | The last step's exit code equals this value | `exit_code: 0` | +| `stdout_contains` | The last step's stdout contains this substring | `stdout_contains: "Build succeeded"` | +| `stdout_matches` | The last step's stdout matches this regex | `stdout_matches: "Order #\\d+"` | +| `stderr_contains` | The last step's stderr contains this substring | `stderr_contains: "deprecated"` | +| `stderr_matches` | The last step's stderr matches this regex | `stderr_matches: "^warning:"` | +| `file_exists` | A file exists, path resolved against the flow's working directory | `file_exists: dist/bundle.js` | +| `file_contains` | A file (path resolved the same way) contains a substring | `file_contains: { path: dist/bundle.js, text: "//# sourceMappingURL" }` | +| `json_output` | A dot-path into the last step's stdout, parsed as JSON, equals a value | `json_output: { path: "$.status", equals: "ok" }` | + +`exit_code`, the `*_contains`/`*_matches` pairs, and `json_output` are checked once, immediately, against the already-captured output of the flow's last step — there's nothing to retry, since that output can't change after the command has exited. `file_exists` and `file_contains` **do** retry, polling within the flow's timeout, because the file they're checking for may still be written by something asynchronous even after the command that triggered it has returned. + +### Working Directory + +Every `surface: cli` flow runs its `steps` (and its own `setup`, if it has one) inside a single working directory, shared across all of them: + +- **No `cwd` configured:** FlowSpec creates a fresh, empty temporary directory for the flow (prefixed `flowspec-` so a kept one is identifiable). On a passing flow, the directory is deleted afterward. On a **failing** flow, the directory is **kept**, and its absolute path is printed in the failure report — exactly the CLI analog of the web surface's failure screenshot, giving you somewhere to go look. +- **`cwd` configured** (in `flowspec.config.yaml` — see [`cwd` and `captureLimit`](#cwd-and-capturelimit-cli-surface-settings) above): that directory is used as-is and is **never** created or deleted by FlowSpec, whether the flow passes or fails. Point `cwd` at a real, already-existing directory. + +### Setup for CLI Flows + +A `surface: cli` flow can declare its own `setup` block, in the CLI step grammar, run before its `steps` in the same working directory: + +```yaml +name: migration-runs-cleanly +surface: cli +setup: + - run: ["node", "scripts/seed-fixture.js"] +steps: + - run: ["node", "scripts/migrate.js"] +expect: + - exit_code: 0 +``` + +Config-level `setup` (web-only, see [Configuration File](#configuration-file)) is never applied to a CLI flow — only a flow's own `setup` block runs for it. A failing setup step fails the flow the same way a failing step does (see below), naming the setup step's own index; the flow's own `steps` never run. + +### Exit Codes Within a CLI Flow + +This is the least guessable rule in the grammar, so it's worth spelling out on its own: **`expect_exit` is honored on every step, including the last one — but only the *absence* of `expect_exit` on the final step makes its exit code non-fatal.** + +- **Every step but the last** must produce the exit code it declared with `expect_exit` (default **0**, if `expect_exit` is omitted). A mismatch fails the flow immediately, at that step, and no later step runs. This is fail-fast: a setup or build step that didn't succeed makes the rest of the flow meaningless. +- **The last step** is different, precisely so a flow can assert that a command is *supposed* to fail — "this command should exit with an error" is a first-class, error-path spec, not a workaround. If the last step declares `expect_exit`, it's checked exactly like any other step. If it does **not**, its exit code is never fatal by itself: the flow proceeds to the `expect` block regardless of what the command returned, and the exit code becomes just one more thing `expect: [{ exit_code: ... }]` can check if you want it checked at all. +- **Setup steps** always use the non-final rule above — including the last step in the `setup` block. Setup has no assertion phase of its own to hand a bare exit code off to, so every setup step's exit code must match its `expect_exit` (default 0) or the flow fails. + ## Quick Example ```yaml diff --git a/adr/0004-expect-exit-honored-on-every-step.md b/adr/0004-expect-exit-honored-on-every-step.md new file mode 100644 index 0000000..aa2409e --- /dev/null +++ b/adr/0004-expect-exit-honored-on-every-step.md @@ -0,0 +1,47 @@ +# ADR 0004: expect_exit is honored on every step, including the final one + +**Status:** Accepted +**Date:** 2026-08-18 +**Deciders:** Face + Sosa (mission: PRD-0007 CLI Surface Adapter) + +## Context + +PRD-0007 establishes two rules that collide on one step. Non-final `run` steps fail +fast: a step whose exit code differs from its `expect_exit` (default 0) fails the flow +there, so a broken command never produces a misleading assertion failure three steps +later. The final `run` step's exit code, by contrast, is "pure assertion territory" — +it never fails the flow on its own, which is what makes error-path specs (`bad flag +exits 1 and names the flag on stderr`) first-class. + +The collision: what happens when the *final* step carries an explicit `expect_exit`? +The two rules give opposite answers, and the PRD does not say which wins. + +## Decision + +`expect_exit` is enforced wherever it appears, including on the final step. A final +step declaring `expect_exit: 1` and exiting 1 proceeds to assertions; the same step +exiting 0 fails the flow at that step. + +Only the **absence** of `expect_exit` makes the final step's exit code non-fatal. An +explicit `expect_exit` is a declaration by the spec author, not an instance of +"failing on exit code alone." + +## Alternatives Considered + +- **Parse error on `expect_exit` on the final step.** Rejected: makes a step's legal + modifiers depend on its position in the list, so adding a step at the end silently + invalidates the one before it. Position-dependent grammar is the kind of surprise + that costs more than the ambiguity it removes. +- **Silently ignore `expect_exit` on the final step.** Rejected: a spec would state an + expectation that never runs — exactly the class of hazard (assertions that look + enforced but aren't) this PRD exists to eliminate. + +## Consequences + +The rule to document and teach is "`expect_exit` always means what it says; the final +step is special only when you say nothing." This is the least guessable rule in the CLI +grammar, so it carries its own documentation criterion in the docs item. + +Surfaces #7 (Conduit) and #8 (API) inherit this step grammar. They should adopt the +same rule rather than re-deciding it per surface — a per-surface answer would make +`expect_exit` mean different things in specs that otherwise read identically. diff --git a/adr/0005-protected-specs-authored-outside-specs-dir.md b/adr/0005-protected-specs-authored-outside-specs-dir.md new file mode 100644 index 0000000..33bcdcf --- /dev/null +++ b/adr/0005-protected-specs-authored-outside-specs-dir.md @@ -0,0 +1,52 @@ +# ADR 0005: Protected specs are drafted outside `specs/` and moved in by a human + +**Status:** Accepted +**Date:** 2026-08-18 +**Deciders:** Face + Sosa (mission: PRD-0007 CLI Surface Adapter) + +## Context + +PRD-0007 ships FlowSpec's first dogfood spec: `specs/init.flow.yaml`, covering +`flowspec init`. This repo already installs its own PreToolUse hook (in +`.claude/settings.local.json`), which blocks any Edit or Write whose `file_path` +matches `specs/**/*.flow.yaml`. + +So the mission's deliverable is a file the implementing agent is forbidden to create — +the immutability guarantee working exactly as designed, aimed at us. Every repo that +installs the hook and later wants a new protected spec hits this, so it needs an answer +that is not "handle it ad hoc this once." + +## Decision + +The agent drafts the spec at a path outside the protected glob (`spec-drafts/init.flow.yaml`), +writes its parse test against that draft path, and stops with a request for the human to +review and `git mv` the file into `specs/`. The test is then repointed at the final path. + +The hook is never lifted, and the agent never routes around it. + +## Alternatives Considered + +- **A human authors the spec from scratch.** Rejected: throws away the agent's + translation of acceptance criteria into flow steps, which is the expensive part. The + human's judgment is needed for *review and admission*, not transcription. +- **Temporarily disable the hook for the duration of the item.** Rejected: the + guarantee is off precisely while a spec is being written — the window when it matters + most. It also normalizes lifting the hook as a routine step. +- **`git mv` or `cp` from Bash, by the agent.** Rejected, and explicitly forbidden in + the item's context. The hook matches only Edit/Write `file_path`, so a shell move + bypasses it silently. An agent that learns this move has learned to defeat the + guarantee the product sells. + +## Consequences + +Protected specs get a human admission step by construction: agents propose, humans +admit. That is the intended shape of the trust boundary, not a workaround for it. + +Any mission adding a spec to a hook-installing repo inherits this flow, so plan for a +human handoff mid-item rather than an uninterrupted agent run. The draft directory +(`spec-drafts/`) should stay out of the protected glob and should not accumulate stale +drafts — a merged draft is deleted, not left behind. + +The silent-bypass property of the hook (Edit/Write only, not Bash) is worth knowing +independently: it means the hook is a guardrail for well-behaved tools, not a security +boundary. diff --git a/adr/0006-specsdir-discovery-out-of-scope.md b/adr/0006-specsdir-discovery-out-of-scope.md new file mode 100644 index 0000000..75b1605 --- /dev/null +++ b/adr/0006-specsdir-discovery-out-of-scope.md @@ -0,0 +1,47 @@ +# ADR 0006: `specsDir`-based discovery stays out of scope for PRD-0007 + +**Status:** Accepted +**Date:** 2026-08-18 +**Deciders:** Face + Sosa (mission: PRD-0007 CLI Surface Adapter) + +## Context + +PRD-0007 requires the dogfood spec to run in CI. Wiring that up surfaced a gap that +predates this mission: `specsDir` is declared in `FlowSpecConfigSchema`, loaded, +validated, and interpolated — and then never read by anything. `discoverFlowFiles` +(`src/index.ts`) works purely from the path argument, and `flowspec run` with no path +exits 1 with "No path specified". + +The tempting fix is to make `flowspec run` fall back to `specsDir` when no path is +given. That would also repair `flowspec init`'s scaffolded `test:e2e` script, which is +a bare `flowspec run` and therefore exits 1 for every user who runs it. + +## Decision + +Discovery via `specsDir` is deliberately NOT implemented in PRD-0007. The `test:e2e` +script passes an explicit path (`flowspec run specs/`). + +The root `flowspec.config.yaml` this mission adds still declares `specsDir: specs/` — +as dogfooding and as documentation of the config surface, not as a discovery mechanism. + +## Alternatives Considered + +- **Implement the `specsDir` fallback here.** Rejected: unrequested scope in a mission + already spanning the type layer, a new spawn primitive, the runner, the reporter, and + the config. It changes the behavior of every existing `flowspec run` invocation — a + meaningful CLI contract change that deserves its own decision, not a ride-along on a + surface adapter. +- **Drop `specsDir` from the config schema as dead weight.** Rejected: it is documented + and scaffolded, so removing it is a breaking change to every existing project's + config for no gain within this mission. + +## Consequences + +`specsDir` remains load-bearing in documentation and inert in behavior until a mission +takes it on deliberately. Anyone reading the config schema should know it is not yet +consumed. + +The related `flowspec init` scaffold bug (a `test:e2e` script that cannot succeed) is +filed separately: fixing it changes init's scaffolded output and `test/init.test.ts`, +so it belongs with whichever mission takes on discovery, not with this one. These two +should be resolved together — the fallback is what makes the scaffolded script correct. diff --git a/docs/specification.md b/docs/specification.md index e7382e4..7806347 100644 --- a/docs/specification.md +++ b/docs/specification.md @@ -80,6 +80,11 @@ This context helps agents understand what business goal they're preserving when name: string # Identifier for the flow description: string # Business context and intent +surface: "web" | "cli" # Optional, default "web". Selects the grammar + # for setup/steps/expect below — see "Surface: + # web or cli" for the CLI grammar and its + # required minimum FlowSpec version. + setup: # Optional: steps to run once, in the same # browser session, before `steps` (e.g. # planting an auth session). Same grammar as @@ -112,6 +117,56 @@ spell that URL out absolutely; writing `visit: "/"` against that `baseUrl` navig to `https://preview.example.dev/` with the token stripped, and whatever session the token would have planted is never established. +### Surface: web or cli + +`surface` is the discriminator between the two grammars a flow can be written in. It is optional, and its absence means exactly the same thing as `surface: web` — the schema above, unchanged. `surface: cli` switches `setup`, `steps`, and `expect` to a different, command-line-oriented grammar entirely; the two never mix within one flow (a web verb in a CLI flow, or a `run` step in a web flow, is a schema validation error naming the offending verb and the flow's surface). + +**Minimum version: flowspec v0.2.0.** `surface` did not exist before this version. Because FlowSpec's top-level schema does not reject unrecognized keys, an older binary parsing a `surface: cli` flow silently drops the `surface` key and then validates `steps`/`expect` against the web-only grammar it knows, which a `run` step or a CLI assertion cannot satisfy — the practical result is a confusing parse failure, not a silent misinterpretation as a passing web flow. Projects adopting `surface: cli` should pin a minimum FlowSpec version. + +```yaml +name: build-succeeds +description: The production build completes and writes the expected bundle +surface: cli + +setup: # Optional: same CLI step grammar as `steps`, + # run first, in the same working directory. + # Every setup step's exit code is fatal on + # mismatch (no "final step" leniency — see + # "Exit codes within a CLI flow" below). + - run: ["node", "scripts/seed-fixture.js"] + +steps: # CLI run steps, executed in order + - run: "npm run build" # string form: whitespace-split, no shell + - run: ["node", "-e", "console.log('done')"] # array form: passed through untouched + stdin: "y\n" # optional: written to the command, then closed + env: # optional: overlaid on the inherited env for + NO_COLOR: "1" # this step only, never leaked to siblings + timeout: 5000 # optional: ms before the command is killed + expect_exit: 0 # optional: see "Exit codes within a CLI flow" + +expect: # The eight CLI assertions, checked against + - exit_code: 0 # the LAST step's captured result + - stdout_contains: "done" + - stdout_matches: "^done$" + - stderr_contains: "warning" + - stderr_matches: "^warning:" + - file_exists: "dist/bundle.js" # path resolved against the + - file_contains: { path: "dist/bundle.js", text: "//# sourceMappingURL" } # working directory + - json_output: { path: "$.status", equals: "ok" } # dot-path into stdout, parsed as JSON +``` + +**No shell, ever.** CLI steps never invoke a shell. String-form `run` is split on whitespace only (no quote handling, no metacharacter interpretation): `run: "echo a && echo b"` runs the single command `echo` with four literal arguments `a`, `&&`, `echo`, `b` — nothing is chained, and a quoted substring is not reassembled into one argument. Two escape hatches cover what a shell would otherwise provide: the **array form** for an argument containing spaces or quotes (`run: ["node", "-e", "an arg with spaces"]`), and invoking a shell explicitly (or a script file) for pipes, redirects, globbing, or `&&` chaining (`run: ["bash", "-c", "cat *.log | grep ERROR"]`). + +**Retry split.** `exit_code`, `stdout_contains`/`stdout_matches`, `stderr_contains`/`stderr_matches`, and `json_output` are checked exactly once against the last step's already-captured output — that output cannot change, so there is nothing to retry. `file_exists` and `file_contains` poll within the flow's timeout instead, because the file they check for may still be written by something asynchronous after the triggering command has already returned — the same rationale as the web surface's `wait_for` and assertion retries. + +**Exit codes within a CLI flow.** This is the least guessable rule in the grammar: `expect_exit` is honored on every step, including the last one, but only the **absence** of `expect_exit` on the final step makes its exit code non-fatal. + +- A **non-final** step's exit code must equal its `expect_exit` (default `0`) or the flow fails immediately at that step, and no later step runs — fail-fast, because a setup or build step that didn't succeed makes everything after it meaningless. +- The **final** step is different, deliberately: if it declares `expect_exit`, that's checked exactly like any other step; if it does **not**, its exit code is never fatal by itself, and the flow proceeds to `expect` regardless of what the command returned. This is what makes "this command should fail" a first-class, error-path spec rather than something the runner treats as broken. +- **Setup steps** always use the non-final rule, including the last step in `setup` — setup has no assertion phase of its own for a bare exit code to defer to. + +**Working directory.** Every CLI flow's `setup` and `steps` run inside one shared working directory. With no `cwd` configured (see [Configuration File](#configuration-file)), FlowSpec creates a fresh, empty temporary directory per flow (prefixed `flowspec-`), deletes it when the flow passes, and keeps it — printing its absolute path in the failure report, the CLI analog of the web surface's failure screenshot — when the flow fails. A configured `cwd` is used as-is and is never created or deleted by FlowSpec, on pass or fail. + ### Full Example ```yaml @@ -142,14 +197,19 @@ the current directory. CLI options override file values. ```yaml baseUrl: string # Origin every relative `visit:` resolves against -timeout: number # Assertion retry timeout, in milliseconds +timeout: number # Assertion retry timeout, in milliseconds. Never a + # process deadline — see `stepTimeout`. +stepTimeout: number # Optional, CLI-surface only. Milliseconds a single + # `run` step's process may live before it is killed. + # Positive integer; defaults to 60000. specsDir: string # Directory flows are loaded from setup: # Optional: steps run once per flow, in that flow's # own browser session, before its `steps`. Shared by # every flow; a flow-level `setup` replaces it, and # `setup: []` on a flow opts out. Same grammar as a - # flow's `steps`. + # flow's `steps`. WEB-ONLY — never applied to a + # `surface: cli` flow (see below). - visit: "https://preview.example.dev?_ab=${PREVIEW_TOKEN}" headers: # Optional: HTTP headers applied to each flow's @@ -163,6 +223,13 @@ headersScope: "origin" | "all" # Optional: how far `headers` travel. Default # "origin" — only requests to `baseUrl`'s origin # carry them. "all" sends them context-wide, on # every request to every origin. + +cwd: string # Optional, CLI-surface only. Working directory + # for surface: cli flows — see below. Must be + # non-empty when present. + +captureLimit: number # Optional, CLI-surface only. Bytes per captured + # stdout/stderr stream — see below. ``` `headers` is config-level only — there is no `headers` block in a flow file. Header @@ -194,6 +261,18 @@ flow: the flow being run is reported as failed with a `Failed to apply headers: error and every remaining flow is reported as skipped — the same contract as a config-level `setup` failure. +### CLI-Surface Settings + +`cwd`, `stepTimeout` and `captureLimit` configure `surface: cli` flows only; web flows ignore all three. `stepTimeout` has the `--step-timeout ` flag; `cwd` and `captureLimit` have no `--flag` equivalent. + +`cwd` is the working directory every CLI flow's commands run in — a relative value resolves against the directory FlowSpec was invoked from. Leave it unset and each CLI flow gets its own fresh, isolated temporary directory instead (deleted on pass, kept on fail — see "Working directory" under [Surface: web or cli](#surface-web-or-cli)). An empty (or whitespace-only) `cwd` is a validation error rather than a silent fallback: it would otherwise resolve to the directory FlowSpec was invoked from — the real project tree — turning an isolated, disposable workspace into the working copy, uncleaned. + +`stepTimeout` is the process-kill deadline for one `run` step, and is deliberately a separate key from `timeout`: an assertion retry budget and a process deadline answer different questions, and using one value for both meant a ten-second retry window silently killed any command that legitimately ran longer (an install, a build, a network fetch). A step's own `timeout` overrides it. `--step-timeout` rejects `0`, negatives and non-integers with exit code 2, since those reach the spawn primitive as a real, immediate kill deadline rather than as "no limit". + +`captureLimit` bounds how much of a CLI step's stdout and stderr FlowSpec captures, in bytes, each stream independent of the other; output beyond it is truncated with a `[truncated]` marker. There is **no config-level default** for `captureLimit` — leaving it unset does not write a value into the loaded config. The 5 MB (`5 * 1024 * 1024` byte) default is applied downstream, at the point a CLI step actually executes, not at config-load time; this config key only overrides that downstream default when present. + +Config-level `setup` (documented above) uses the web step grammar and is never applied to a `surface: cli` flow — a `run` step inside config-level `setup` is a validation error. A CLI flow that needs setup work declares its own flow-level `setup` block instead, in the CLI step grammar (see [Surface: web or cli](#surface-web-or-cli)). + ## Execution Modes ### CI Mode: Deterministic Runner @@ -254,6 +333,44 @@ bunx flowspec run specs/ # Run all flows bunx flowspec run specs/checkout.flow.yaml # Run single flow ``` +### CLI Mode: No Browser at All + +A `surface: cli` flow (see [Surface: web or cli](#surface-web-or-cli)) is dispatched before any of the logic above runs — before a browser session name is even generated, before `headers` are validated. `flowspec run` decides which surface a flow uses purely by reading its `surface` field, and a CLI flow never resolves or launches `agent-browser`. This is what makes a CLI-only project work on a machine that doesn't have `agent-browser` installed at all: the dependency noted in [Installation](../README.md#installation) is required only if at least one flow actually uses the web surface. + +Simplified CLI runner logic: + +```typescript +import { spawn } from 'node:child_process'; // FlowSpec spawns directly — no shell + +async function runCliFlow(flow: Flow) { + const workdir = createWorkingDirectory(flow); // fresh temp dir, or the configured cwd + + let lastResult; + for (const [index, step] of flow.steps.entries()) { + const argv = Array.isArray(step.run) ? step.run : step.run.split(/\s+/); + const result = await spawnAndCapture(argv, { cwd: workdir, ...step }); + lastResult = result; + + const isLast = index === flow.steps.length - 1; + const expected = step.expect_exit ?? 0; + const exitCodeIsFatal = !isLast || step.expect_exit !== undefined; + if (exitCodeIsFatal && result.exitCode !== expected) { + return fail(workdir, `step ${index} exited ${result.exitCode}, expected ${expected}`); + } + } + + for (const assertion of flow.expect) { + const failure = await evaluateCliAssertion(assertion, lastResult, workdir); + if (failure) return fail(workdir, failure.message); + } + + deleteWorkingDirectory(workdir); // only on pass — a fresh temp dir is kept on failure + return pass(); +} +``` + +Every command is spawned directly from its `run` array (or the whitespace-split string form) — never through a shell, and never through `agent-browser`. See [No shell, ever](#surface-web-or-cli) for what that means for pipes, quoting, and shell metacharacters. + ### Development Mode: Agent-Driven Execution During development, an agent can run flows interactively using the `agent-browser` skill. This enables: diff --git a/flowspec.config.yaml b/flowspec.config.yaml new file mode 100644 index 0000000..8f38ca3 --- /dev/null +++ b/flowspec.config.yaml @@ -0,0 +1 @@ +specsDir: specs/ diff --git a/package.json b/package.json index c07964d..d99fb99 100644 --- a/package.json +++ b/package.json @@ -35,7 +35,8 @@ "lint": "biome check .", "lint:fix": "biome check --write .", "format": "biome format --write .", - "test:e2e": "flowspec run" + "pretest:e2e": "bun run build && chmod +x dist/index.js && mkdir -p node_modules/.bin && ln -sf ../../dist/index.js node_modules/.bin/flowspec", + "test:e2e": "flowspec run specs/" }, "dependencies": { "agent-browser": "0.9.3", diff --git a/prd/0007-cli-surface-adapter.md b/prd/0007-cli-surface-adapter.md new file mode 100644 index 0000000..978c2a8 --- /dev/null +++ b/prd/0007-cli-surface-adapter.md @@ -0,0 +1,397 @@ +--- +missionId: ~ +--- + +# PRD-0007: CLI Surface Adapter + +**Author:** Josh +**Date:** 2026-08-16 +**Status:** Draft +**Issue:** https://github.com/queso/FlowSpec/issues/6 + +## Executive Summary + +FlowSpec's immutable-spec guarantee currently exists for exactly one surface: web flows +driven through agent-browser. This PRD adds `surface: cli` so a spec can run a command and +assert on its exit code, stdout/stderr, and the files it writes — using the same YAML +grammar, parser, reporter, and PreToolUse immutability hook. It is deliberately the first +of three surface adapters (CLI, then Conduit #7, then API #8): the simplest surface pays +the one-time cost of making FlowSpec multi-surface, and it lets FlowSpec spec its own CLI +as the first consumer. + +## Definition of Done + + + +- [ ] +- [ ] +- [ ] + +## 1. Context & Background + +FlowSpec exists to close one hole: when a spec fails, agents fix the implementation, not +the spec — because the spec is protected and the test code is not. Today that guarantee is +only available to web apps. Every CLI-surface repo in the fleet (this repo itself, +bambu-cloud-bridge, promptdiff, the decker CLI, the ateam CLI) keeps its +definition-of-done in mutable test files, which is exactly the "agent fixes the test +instead of the bug" hole FlowSpec was built to close. + +This is part of the multi-surface substrate plan (theaiteam-dev/the-ai-team-plugin#51, +PRD 010): one immutable `steps`/`expect` grammar, per-surface adapters, one graduation +pipeline (DoD → protected spec → CI) feeding the earned-auto-merge ladder. + +CLI goes first, ahead of the Conduit adapter (#7), for three reasons: + +- **The first adapter pays the architecture tax.** Whichever surface goes first introduces + the `surface` discriminator, per-surface step/assertion schemas, and runner dispatch. + Those decisions should be made against the simplest surface, not entangled with the + Conduit kernel's output conventions. +- **The plumbing is a close cousin of something that already exists.** `execCommand` + (`src/runner.ts:112`) already spawns a process with captured stdout/stderr/exit code, + with a Bun-native path and a Node fallback — but it hardcodes stdin to `ignore` and has + no `cwd`/`env` support, so it can't be reused as-is. The CLI runner introduces a sibling + primitive, `spawnProcess` (`src/exec.ts`), built on the same Bun-native/Node-fallback + pattern; the CLI runner itself is mostly schema and assertions, not new execution + machinery. +- **Conduit is roughly a specialization of CLI.** `run_flow` is "run a command"; `env`, + `file_exists`, and JSON-file assertions appear in both proposals. Landing CLI first + means #7 inherits most of its machinery. + +## 2. Problem Statement + +A team shipping a CLI tool has no way to state, immutably, what a green run of that tool +means. The FlowSpec grammar has no verb for "run this command" and no assertions over exit +codes, stdio, or produced files — and the runner unconditionally drives a browser. +FlowSpec cannot even spec its own `flowspec init`, so the tool that enforces +protected-spec discipline on other repos has none of its own. + +## 3. Target Users & Use Cases + +**Primary users:** + +- **CLI tool maintainers** running agent-driven development, who need a + definition-of-done that agents cannot quietly weaken. +- **FlowSpec itself** — the first consumer; `flowspec init` behavior becomes a protected + spec running in this repo's CI. + +**Key use cases:** + +- A maintainer needs to spec "`flowspec init` scaffolds config, a sample spec, and the + protection hook" so that scaffolding regressions fail CI against a spec no agent edits. +- A maintainer needs to spec an *error path* ("bad flag exits 1 and names the flag on + stderr") so that error behavior is a contract, not an accident — which requires nonzero + exit codes to be assertable rather than treated as failures. +- A maintainer needs specs to run in a fresh working directory per flow so that runs are + deterministic and parallelizable, and a spec can't pass by luck of leftover state. +- An agent-driven repo owner needs web and CLI specs to live in the same `specs/` tree + under the same hook and produce one unified report. + +## 4. Goals & Success Metrics + +| Goal | Metric | Target | +|------|--------|--------| +| Establish the multi-surface architecture | Existing web flows and tests | Pass unchanged; specs with no `surface` field behave identically | +| FlowSpec specs itself | A protected `flowspec init` spec in this repo's CI | Green in CI | +| Cover the issue-#6 grammar | The sketch in issue #6 | Expressible and passing verbatim (modulo paths) | +| No browser dependency for CLI runs | A CLI-only run on a machine without agent-browser | Completes without attempting to launch a browser | +| Legible failures | Failed CLI assertion output | Includes exit code and a bounded stderr/stdout excerpt | + +## 5. Scope + +### In Scope + +- An optional `surface` field on flow files: `web` (default when absent) or `cli` +- CLI step grammar: `run` (command as string or argv array) with optional per-step + `stdin`, `env`, and `timeout` modifiers; multiple `run` steps per flow +- CLI assertions: `exit_code`, `stdout_contains`, `stdout_matches`, `stderr_contains`, + `stderr_matches`, `file_exists`, `file_contains: {path, text}`, + `json_output: {path, equals}` +- A fresh temporary working directory per CLI flow; config-level `cwd` override +- Flow-level `setup` in CLI flows, using CLI steps (e.g. seeding files before the run) +- Runner dispatch on `surface`; the CLI path never touches agent-browser +- Reporter rendering for CLI steps and assertions, unified summary across surfaces +- Parse-time rejection of surface/verb mismatches (web verbs in a CLI flow and vice versa) +- A dogfood spec for `flowspec init` in this repo's `specs/`, running in CI +- Documentation: README and `docs/specification.md` + +### Out of Scope + +- **Shell features** — pipes, redirects, globbing, `&&` chains. Commands are spawned + directly, no shell. See Design. +- **Interactive processes / PTY emulation** — `stdin` is write-then-close only. +- **`${VAR}` interpolation inside `specs/**`** — already decided in ADR-0003; specs stay + literal on every surface. +- **The Conduit (#7) and API (#8) surfaces** — they build on this; nothing here should + preclude them, but their verbs ship separately. +- **Parallel flow execution** — per-flow tmp dirs make it *possible* later; scheduling is + its own feature. +- **Snapshot / golden-file assertions** — `file_contains` and `json_output` cover current + needs; snapshots bring update-workflow questions that deserve their own PRD. +- **Config-level `setup` for CLI flows** — config-level setup remains web-surface setup; + see Design. + +## 6. Design + +### The surface discriminator + +`FlowSpecSchema` (`src/types.ts`) gains an optional `surface` enum. Absent means `web`, +so every existing spec file is untouched and means what it meant yesterday. The +steps/assertions a flow may use are determined by its surface: parsing a CLI flow +validates steps against the CLI step schema and `expect` against the CLI assertion +schema. A web verb in a CLI flow (or vice versa) is a parse error, reported before +anything executes, with the same exit-2 semantics parse errors already have (ADR-0002). + +Two silent-ignore hazards, both instances of the class PRD-0006 documented: + +- `FlowSpecSchema` is not `.strict()` at the top level, so `surface` must be a real + schema field — otherwise a `surface: cli` flow would silently run as a web flow. +- An *older* flowspec binary given a CLI spec would drop the unknown `surface` key and + fail bizarrely trying to browse. Nothing to build here — but the docs should note the + minimum version next to the feature. + +### Step grammar: `run`, no shell + +A CLI step is a `run` with optional modifiers: + +```yaml +steps: + - run: "flowspec init --dir ./proj" + - run: ["badcmd", "--provoke-error"] + expect_exit: 1 + - run: ["flowspec", "run", "--flow", "spec with spaces.flow.yaml"] + env: { NO_COLOR: "1" } + stdin: "y\n" + timeout: 5000 +``` + +Commands are spawned directly with an argv array — **no shell**. This repo just moved +browser execution to args-array spawning for exactly this reason: no quoting bugs, no +injection surface, no platform-dependent shell semantics. The string form is split on +whitespace as a convenience; arguments containing spaces require the array form. This +limitation is documented rather than papered over with quoting rules. + +Consequences accepted: no pipes or redirects in specs. A spec that needs a pipeline is +specifying a shell script's behavior — wrap it in a script and `run` that. + +- `env` overlays the inherited process environment for that step. Values are literal + (ADR-0003: specs never interpolate). +- `stdin` is written to the child and the stream closed — sufficient for confirmation + prompts, not for interaction. +- `stepTimeout` bounds the step; on expiry the process is killed and the step fails with a + timeout error. Default comes from the config `stepTimeout` (60s), a key distinct from + the assertion-retry `timeout` — the two clocks measure different things and the config + key names them separately. + +### Execution model: fresh cwd per flow + +Each CLI flow runs in a freshly created temporary directory — deleted when the flow +passes, kept (with its path printed in the failure report) when it fails. The kept +directory is the CLI analog of the web surface's failure screenshot: `file_contains` +failures beg the question "so what *is* in that file?", and the evidence from the run +that actually failed is worth more than tidy tmp space, which the OS reaps anyway. + +The fresh directory itself plays the role the fresh browser session plays for web: no +state leaks between flows, runs are deterministic, and future parallelism is not +precluded. A +config-level `cwd` can override it for tools that must run inside a real checkout — at +the cost of determinism, which is the user's call to make, in the mutable config file, +outside the immutable spec. + +Relative paths — in `file_exists`, `file_contains`, and `run` commands' arguments — are +resolved by the child process and the assertion checker against this cwd, so the sketch +in issue #6 can say `file_exists: proj/flowspec.config.yaml` without absolute paths. + +### Assertion semantics + +Assertions evaluate after all steps complete, against **the last `run` step's** captured +output. Multi-step flows are chains ("init, then run, assert on the run"), and the final +state is what the spec is about. Intermediate steps' output is not addressable in v1. + +Exit codes are handled differently for the final step and the steps before it: + +- **The final step's exit code is pure assertion territory.** `exit_code` is an + assertion like any other, because error-path specs ("bad flag exits 1") are + first-class. The final step never fails on its exit code alone. +- **Non-final steps fail fast.** A non-final `run` step must exit 0, or the flow fails + at that step, with that step's stderr — pointing at the root cause instead of letting + a later step fail confusingly in a broken world. This is the same misleading-failure + reasoning PRD-0006 applied to web setup. A per-step `expect_exit: ` modifier + declares an intermediate command that is *supposed* to fail, keeping error-state + chains expressible. + +Retry semantics extend the PRD-0004 model by what retrying can actually change: + +- `exit_code`, `stdout_*`, `stderr_*`, `json_output` are **final** — the process has + exited; its output cannot change. One evaluation, no retry. +- `file_exists` and `file_contains` **retry within the timeout window**, because a + just-exited process may have async writers (a spawned daemon, a flushing logger) still + completing. Same model, same timeout source as web assertion retry. + +`json_output` parses the last step's stdout as JSON and compares at a dot-path +(`$.foo.bar`). Non-JSON stdout is an assertion failure that says so, with the head of +the offending output — not a crash. + +### Runner dispatch, reporting, and what stays shared + +`runFlow` (`src/runner.ts:709`) branches on surface before any session exists. The web +path is untouched. The CLI path builds on `spawnProcess` (`src/exec.ts`) and never +resolves, launches, or requires agent-browser — a CLI-only run must work on a box where +it isn't installed. + +Everything around the runner stays shared: one parser, one reporter, one `FlowResult` +stream, one summary (`formatSummary`) across a mixed-surface run, one PreToolUse hook +protecting `specs/**` regardless of surface. `FlowError` gains nothing surface-specific +except that `screenshot` is simply never set; failure reports instead carry the exit +code and a bounded excerpt of stdout/stderr, because "which assertion failed" without +"what the process actually printed" sends the user straight to re-running by hand. + +The assertion primitives this PRD introduces — contains/matches text checks, path-based +JSON comparison, retryable file checks — are precisely what the Conduit (#7) and API +(#8) adapters consume next. They should land as surface-agnostic machinery, not +CLI-runner internals; that is a strategy constraint, not an implementation prescription. + +### Setup interplay with PRD-0006 + +Config-level `setup` is a sequence of *web* steps that plants browser state; it has no +meaning for a process spawn. So: config-level setup applies to web flows only, CLI flows +skip it, and its failure-abort semantics (PRD-0006) continue to govern the run +unchanged. A CLI flow may declare its own flow-level `setup` of CLI steps — the natural +place to seed fixture files — with PRD-0006's phase-labeled error reporting intact. + +## 7. Requirements + +### Functional Requirements + +1. `FlowSpecSchema` shall accept an optional `surface` field with values `web` and + `cli`; absent shall mean `web`. +2. A flow with `surface: cli` shall accept steps of the form `run` (string or array of + strings) with optional `stdin` (string), `env` (string-to-string map), `timeout` + (milliseconds), and `expect_exit` (integer) modifiers. +3. A web verb in a CLI flow, or a CLI verb in a web flow, shall be a parse error + reported before any flow executes, with exit code 2. +4. The runner shall execute `run` steps by direct process spawn — no shell — capturing + stdout, stderr, and exit code; string-form commands shall be whitespace-split into + argv. +5. Each CLI flow shall execute in a freshly created temporary working directory, + removed when the flow passes and kept when it fails (NFR-3); a config-level `cwd` + shall override it. +6. `env` entries shall overlay the inherited environment for that step; `stdin` shall + be written to the child and closed; `timeout` expiry shall kill the process and fail + the step as a timeout, defaulting to the config `timeout`. +7. A spawn failure (e.g. command not found) shall fail the step with the underlying + error. A non-final `run` step shall fail the flow at that step when its exit code + differs from its expected exit code (`expect_exit`, default 0), reporting that + step's stderr; the final step's exit code shall never fail the flow by itself. +8. CLI flows shall support the assertions `exit_code`, `stdout_contains`, + `stdout_matches` (regex), `stderr_contains`, `stderr_matches` (regex), + `file_exists`, `file_contains: {path, text}`, and `json_output: {path, equals}`. +9. `exit_code`, `stdout_*`, `stderr_*`, and `json_output` shall evaluate once, against + the last `run` step's captured output, with no retry. +10. `file_exists` and `file_contains` shall retry within the timeout window using the + PRD-0004 model; relative paths shall resolve against the flow's working directory. +11. A failed CLI assertion shall be reported with the assertion, the exit code, a + bounded excerpt of captured stdout/stderr, and — when the flow ran in a temporary + working directory — the path to the kept directory. +12. A CLI flow may declare flow-level `setup` composed of CLI steps, executed in the + flow's working directory before its steps, with setup-phase error labeling per + PRD-0006; config-level setup shall not apply to CLI flows. +13. A run containing only CLI flows shall complete on a machine without agent-browser + installed, and shall not attempt to resolve or launch it. +14. A flow file with no `surface` field shall parse, execute, and report exactly as + today; the existing test suite shall pass without modification. +15. `formatSummary` shall report mixed-surface runs in one unified summary. +16. This repo shall ship a spec under `specs/` covering `flowspec init` (scaffolds + config, sample spec, and protection hook), running in CI. +17. README and `docs/specification.md` shall document the `surface` field, the CLI + grammar, the no-shell rule and its array-form escape hatch, and the fresh-cwd + execution model. + +### Non-Functional Requirements + +1. Captured stdout/stderr shall be bounded at 5 MB per stream per step by default, + overridable via an optional `captureLimit` (bytes) in `flowspec.config.yaml`; + output beyond the cap is truncated with an explicit truncation marker, not + silently dropped. +2. The web execution path shall be byte-for-byte behaviorally unchanged — dispatch + happens before any browser concern, and no web-flow run incurs CLI-adapter work. +3. Temporary working directories shall be removed when the flow passes and kept when + it fails, with the kept path named in the failure report; a config-level `cwd` + override is never deleted in either case. +4. An invalid regex in `stdout_matches`/`stderr_matches` shall be a parse-time error, + not a runtime one. + +## 8. Edge Cases & Error States + +- **Command not found:** step fails with the spawn error, naming the command. Distinct + from a nonzero exit, which is assertion territory on the final step. +- **Non-final step exits nonzero without `expect_exit`:** flow fails at that step with + its stderr; later steps do not run. With `expect_exit: 1` and an actual exit of 1, + the chain continues; any other exit fails the step. +- **Process exceeds `timeout`:** killed; step fails as a timeout; assertions do not run. +- **Spec asserts `exit_code: 1`:** passes when the command exits 1 — error-path specs + are first-class. +- **`json_output` on non-JSON stdout:** assertion failure with a parse message and the + head of the output. +- **`file_contains` on a file that never appears:** retries within the timeout, then + fails naming the resolved path — mirroring web `wait_for` failure shape. +- **String-form `run` with a quoted argument** (`run: 'echo "two words"'`): the quotes + are not shell-parsed; the documented answer is the array form. +- **`stdin` provided to a program that never reads it:** harmless; the stream closes. +- **A CLI flow in a run whose config declares web `setup`:** the CLI flow skips it; if + that shared setup fails, PRD-0006 abort semantics apply to the run unchanged. +- **`surface: cli` under an old flowspec binary:** unknown key dropped, flow misread as + web. Docs note the minimum version; no runtime mitigation is possible from the old + binary's side. +- **Unknown surface value (`surface: api`):** parse error, exit 2 — the enum is closed + until #8 opens it. +- **Huge stdout (a build log):** captured up to the cap (5 MB per stream by default, + `captureLimit` in config to raise it), truncated with a marker; assertions evaluate + against the captured portion, so the marker is the tell when a needle "missing" from + output was actually printed past the cap. + +## 9. Dependencies + +- Builds on the no-shell argv-spawning pattern `execCommand` (`src/runner.ts:112`) + established, via a sibling primitive `spawnProcess` (`src/exec.ts`), plus PRD-0004 + assertion retry and PRD-0006's setup phases and skip accounting. +- Touches `src/types.ts`, `src/parser.ts`, `src/runner.ts`, `src/reporter.ts`, + `src/config.ts` (`cwd`), and docs. No new packages anticipated. +- Downstream: the Conduit adapter (#7) consumes the surface dispatch and the + file/JSON assertion machinery; the API adapter (#8) consumes the path-based JSON + comparison. Neither blocks this PRD; both are shaped by it. + +## 10. Risks & Open Questions + +| Risk | Likelihood | Impact | Mitigation | +|------|-----------|--------|------------| +| Surface dispatch destabilizes the web path | Low | High — every existing consumer | FR-14: existing suite passes unmodified; dispatch precedes all browser code | +| No-shell rule surprises users expecting pipes | Medium | Low — confusing first-run | Documented prominently with the wrap-it-in-a-script recipe | +| Whitespace-split string form mis-parses a command | Medium | Low | Array form documented as the exact escape hatch | +| Assertion machinery built CLI-shaped, hurting #7/#8 reuse | Medium | Medium | Design constraint: primitives land surface-agnostic | + +### Open Questions + +None remaining — all four resolved 2026-08-16; outcomes recorded in Decisions. + +## Decisions + +- `surface` is optional and defaults to `web` — zero existing specs change meaning. +- No shell, ever. String form is whitespace-split sugar; array form is the exact path. + Pipelines belong in scripts the spec runs. +- Non-final steps fail fast on unexpected exit codes, with `expect_exit` as the + per-step escape hatch; the final step's exit code is pure assertion territory, so + error paths stay contracts too. *(Resolved from Open Questions, 2026-08-16.)* +- Assertions target the last `run` step; intermediate output is unaddressable in v1. + Fail-fast polices intermediate steps, side effects are checkable via file + assertions, and cross-step addressability waits for #8's `capture` design. + *(Resolved from Open Questions, 2026-08-16.)* +- Final vs. retryable assertions split by what retrying could change: process output is + final, filesystem state retries within the timeout. +- Fresh tmp cwd per flow: deleted on pass, kept and named in the report on failure — + the CLI analog of the failure screenshot. `cwd` override lives in mutable config, + never in specs, and is never deleted. *(Resolved from Open Questions, 2026-08-16.)* +- Config-level setup stays web-only; CLI flows get flow-level setup in CLI grammar. +- Capture cap: 5 MB per stream per step, `captureLimit` config override, explicit + truncation marker. *(Resolved from Open Questions, 2026-08-16.)* +- The dogfood spec for `flowspec init` ships in this PRD, not a follow-up — the adapter + is not done until FlowSpec specs itself. diff --git a/specs/init.flow.yaml b/specs/init.flow.yaml new file mode 100644 index 0000000..e1103ce --- /dev/null +++ b/specs/init.flow.yaml @@ -0,0 +1,17 @@ +name: flowspec-init-scaffolds-and-protects +description: | + `flowspec init` scaffolds a working project (config, example spec, and the + spec-protection hook) and, run a second time against an already-initialized + tree, reports the existing setup instead of clobbering it. This is + FlowSpec's own first CLI-surface consumer: a regression in `init` should + fail this spec, not just a human noticing scaffolded files look wrong. +surface: cli +steps: + - run: ["flowspec", "init"] + - run: ["flowspec", "init"] +expect: + - file_exists: flowspec.config.yaml + - file_exists: specs/example.flow.yaml + - file_exists: .claude/settings.local.json + - file_contains: { path: .claude/settings.local.json, text: "Flow specs are immutable" } + - stdout_contains: "(already exists)" diff --git a/src/cli-assertions.ts b/src/cli-assertions.ts new file mode 100644 index 0000000..ddb0b98 --- /dev/null +++ b/src/cli-assertions.ts @@ -0,0 +1,325 @@ +/** + * The CLI assertion dispatcher: evaluates one of the eight CLI assertions + * (src/types.ts's CliAssertion) against a completed run step's captured + * result. Deliberately decoupled from the CLI runner — this module takes a + * plain last-step result, a working directory, and a timeout, and does not + * import src/cli-runner.ts — so it can be built independently and reused. + * + * All real matching is delegated to the shared primitives: substring/regex/ + * dot-path-JSON from src/matchers.ts, and the polling file checks from + * src/file-matchers.ts. This module's own job is purely dispatch (which of + * the eight assertion keys is present) and assembling a uniform failure + * shape — it never reimplements matching logic inline. + * + * Per the PRD's retry split: exit_code, stdout/stderr contains/matches, and + * json_output all evaluate exactly once (an already-captured stream cannot + * change). file_exists/file_contains retry within `timeout`, because an + * async writer can still change the answer. + */ + +import { realpathSync } from "node:fs"; +import { dirname, resolve, sep } from "node:path"; +import { fileContains, fileExists } from "./file-matchers.js"; +import { + boundedExcerpt, + matchContains, + matchJsonPath, + matchRegex, +} from "./matchers.js"; +import type { CliAssertion } from "./types.js"; + +export interface CliAssertionFailure { + message: string; + /** The last run step's exit code — always attached, regardless of which assertion failed. */ + exitCode: number; + /** The last run step's stdout, bounded to EXCERPT_LIMIT. */ + stdout: string; + /** The last run step's stderr, bounded to EXCERPT_LIMIT. */ + stderr: string; + /** Passed straight through from the caller. */ + workdir: string; +} + +export interface LastStepResult { + stdout: string; + stderr: string; + exitCode: number; +} + +type ResolvedPath = + | { ok: true; absolutePath: string } + | { ok: false; message: string }; + +/** + * Resolve the real, symlink-free path of the deepest ancestor of + * `absolutePath` that currently exists on disk, walking upward one path + * component at a time until something resolves (or the filesystem root is + * reached without finding anything). + * + * This exists to defend against symlink traversal WITHOUT breaking + * file_exists/file_contains's retry-for-a-not-yet-created-file contract + * (see src/file-matchers.ts's pollUntilPass): those two assertions poll + * while an async writer may still be creating the target file, and + * `realpathSync` throws ENOENT on a path that doesn't exist yet. Naively + * realpathing the full requested path would turn every "waiting for the + * file to appear" check into an immediate hard failure. + * + * The insight that makes walking up to the deepest EXISTING ancestor + * sufficient: a symlink is itself a filesystem entry, so a path component + * that doesn't exist yet cannot itself be a symlink. Once the deepest + * existing ancestor's real path is confirmed to stay inside the workdir, + * every remaining (not-yet-existing) trailing component is a plain, inert + * path segment — there is nothing further for a symlink to hijack. + */ +function realpathDeepestExisting(absolutePath: string): string { + let current = absolutePath; + for (;;) { + try { + return realpathSync(current); + } catch { + const parent = dirname(current); + if (parent === current) { + // Walked all the way to a filesystem root without resolving + // anything. Return the unresolved path rather than looping forever + // or throwing; the containment check downstream compares it against + // the already-realpath'd workdir root. + return current; + } + // Keep walking up on ANY failure, not just ENOENT (CodeRabbit review, + // this PR). Returning the unresolved path on a non-ENOENT error would + // fail OPEN, not closed: `current` is still the lexically-resolved + // path, which the caller has already confirmed sits under the root, so + // the containment check would trivially accept it having canonicalized + // nothing. That is exactly the bypass this function exists to prevent + // — e.g. requesting "linked/blocker/target.txt" where "linked" is a + // symlink out of the workdir and "blocker" is a regular file makes + // realpathSync fail with ENOTDIR (not ENOENT), and the symlink would + // never be resolved or noticed. EACCES on an unreadable intermediate + // directory hides a symlink the same way. Walking up instead means the + // ancestor that finally resolves is genuinely canonicalized, so the + // containment check always runs against real data. + current = parent; + } + } +} + +/** + * realpathSync the workdir root itself, falling back to the plain resolved + * path if that fails (the workdir should always exist by the time an + * assertion runs, but this avoids a hard crash over a defensive edge case). + * This matters on macOS, where `/tmp` is itself a symlink to `/private/tmp` + * — without realpath'ing the root too, a workdir created via + * `mkdtempSync(tmpdir())` would never compare equal to its own + * realpath'd descendants (see test/dogfood-plumbing.test.ts, which + * realpathSync's its temp dir for the same reason). + */ +function safeRealpath(path: string): string { + try { + return realpathSync(path); + } catch { + return path; + } +} + +/** + * Resolve a file-assertion path against `workdir` and confine it there. + * + * The per-flow temp working directory is the whole point of CLI isolation, + * so a path that resolves outside it — an absolute path like "/etc/passwd", + * or a "../" traversal — is rejected as out-of-bounds rather than answered + * by whatever happens to exist elsewhere on disk. Without this, such an + * assertion silently PASSES (a wrong green, not an exploit: specs are + * user-authored) and says nothing about the flow's own output. + * + * The containment check compares fully resolved absolute paths and requires + * either an exact match or a separator-terminated prefix, so a sibling + * directory whose name merely starts with the workdir's name + * ("/tmp/wd-extra" against "/tmp/wd") is correctly treated as outside. + * + * That string-prefix check alone is not enough, though: it operates on the + * syntactically resolved path (node:path's `resolve`, which normalizes + * "../" segments but never touches the filesystem) and so does not account + * for symlinks. A symlink living INSIDE the workdir that points outside it + * (e.g. requesting "linked/secret.txt" where "linked" is a symlink to + * /etc) resolves, post-symlink, to somewhere the prefix check never sees — + * it would wrongly report that path as contained. So containment is + * checked a second time against the REAL (symlink-resolved) path of the + * deepest existing ancestor, per realpathDeepestExisting above. + */ +function resolveWithinWorkdir( + requestedPath: string, + workdir: string, +): ResolvedPath { + const root = safeRealpath(resolve(workdir)); + const absolutePath = resolve(root, requestedPath); + + // A root that is already a filesystem root ("/", "C:\\") ends in the + // separator; appending another would produce "//" and reject everything. + const prefix = root.endsWith(sep) ? root : `${root}${sep}`; + const isInside = (candidate: string) => + candidate === root || candidate.startsWith(prefix); + + const outOfBounds: ResolvedPath = { + ok: false, + message: `File path "${requestedPath}" resolves to ${absolutePath}, which is outside the flow working directory ${root}`, + }; + + if (!isInside(absolutePath)) { + return outOfBounds; + } + + const realAncestor = realpathDeepestExisting(absolutePath); + if (!isInside(realAncestor)) { + return outOfBounds; + } + + return { ok: true, absolutePath }; +} + +/** + * Assemble a CliAssertionFailure from a raw message: the last step's + * exitCode/stdout/stderr and the caller's workdir are attached uniformly, + * regardless of which assertion produced the message. + */ +function toFailure( + message: string, + lastStep: LastStepResult, + workdir: string, +): CliAssertionFailure { + return { + message, + exitCode: lastStep.exitCode, + stdout: boundedExcerpt(lastStep.stdout), + stderr: boundedExcerpt(lastStep.stderr), + workdir, + }; +} + +/** + * Evaluate `assertion` against `lastStep`'s captured output (and, for the + * two file assertions, the filesystem under `workdir`). Returns `undefined` + * on pass, or a CliAssertionFailure naming the specific problem. + * + * This deliberately does NOT return a full FlowError — it does not set + * FlowErrorSchema's `assertion` field (still typed web-only/StepAssertion, + * unchanged by WI-805). Assembling a full FlowError (adding `step`, `phase`, + * etc.) is the CLI runner's job, one layer up. + */ +export async function evaluateCliAssertion( + assertion: CliAssertion, + lastStep: LastStepResult, + workdir: string, + timeout: number, +): Promise { + if ("exit_code" in assertion) { + if (lastStep.exitCode === assertion.exit_code) { + return undefined; + } + return toFailure( + `Expected exit code ${assertion.exit_code} but got ${lastStep.exitCode}`, + lastStep, + workdir, + ); + } + + // The four stream branches below guard explicitly against a missing + // value, for the same reason file_contains/json_output do (see their + // comments): a schema-validated CliAssertion always has the verb's value + // present as a non-empty string, but this function is also reachable + // directly by a caller that bypassed validation. Left unguarded, + // `haystack.includes(undefined)` coerces to `haystack.includes("undefined")` + // and `new RegExp(undefined)` compiles to `/(?:)/`, which matches every + // string — both would let a never-specified assertion silently PASS + // rather than fail with a clear "missing" message. + + if ("stdout_contains" in assertion) { + if (typeof assertion.stdout_contains !== "string") { + return toFailure('Missing "stdout_contains" text', lastStep, workdir); + } + const failure = matchContains(lastStep.stdout, assertion.stdout_contains); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + if ("stderr_contains" in assertion) { + if (typeof assertion.stderr_contains !== "string") { + return toFailure('Missing "stderr_contains" text', lastStep, workdir); + } + const failure = matchContains(lastStep.stderr, assertion.stderr_contains); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + if ("stdout_matches" in assertion) { + if (typeof assertion.stdout_matches !== "string") { + return toFailure('Missing "stdout_matches" pattern', lastStep, workdir); + } + const failure = matchRegex(lastStep.stdout, assertion.stdout_matches); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + if ("stderr_matches" in assertion) { + if (typeof assertion.stderr_matches !== "string") { + return toFailure('Missing "stderr_matches" pattern', lastStep, workdir); + } + const failure = matchRegex(lastStep.stderr, assertion.stderr_matches); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + if ("json_output" in assertion) { + // `path`/`equals` are typed optional on CliAssertion because + // src/types.ts enforces their presence via a superRefine (rather than a + // bare required field, so a missing key can be named instead of a + // generic "Required" message) — a schema-validated CliAssertion always + // has `path` set. But this function is also reachable directly (as this + // item's dispatch wiring does, and as a caller bypassing schema + // validation could), so `path` is guarded explicitly rather than + // trusted: without this, `matchJsonPath` crashes with + // "path.startsWith is not a function" on a missing path, even against + // otherwise-valid JSON stdout (verified interactively). + const { path, equals } = assertion.json_output; + if (path === undefined) { + return toFailure('Missing "path" in json_output', lastStep, workdir); + } + const failure = matchJsonPath(lastStep.stdout, path, equals); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + if ("file_exists" in assertion) { + const resolved = resolveWithinWorkdir(assertion.file_exists, workdir); + if (!resolved.ok) { + return toFailure(resolved.message, lastStep, workdir); + } + const failure = await fileExists(resolved.absolutePath, timeout); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + if ("file_contains" in assertion) { + // Same reasoning as json_output's `path` guard above, plus a second, + // more insidious case: an unguarded `text: undefined` doesn't crash — + // `content.includes(undefined)` coerces to `content.includes("undefined")`, + // so a file whose content genuinely contains the literal word + // "undefined" would silently PASS an assertion that never specified + // real text to look for. Both are guarded explicitly rather than + // trusted, verified interactively before writing this. + const { path, text } = assertion.file_contains; + if (path === undefined) { + return toFailure('Missing "path" in file_contains', lastStep, workdir); + } + if (text === undefined) { + return toFailure('Missing "text" in file_contains', lastStep, workdir); + } + const resolved = resolveWithinWorkdir(path, workdir); + if (!resolved.ok) { + return toFailure(resolved.message, lastStep, workdir); + } + const failure = await fileContains(resolved.absolutePath, text, timeout); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + // No recognized CLI assertion key matched. A schema-validated CliAssertion + // can never reach here, but this function is reachable directly by a + // caller that bypassed validation (as a defensive guard, not a case this + // item's own dispatch wiring exercises) — without this, the code used to + // fall through unconditionally into the file_contains branch above and + // crash on `assertion.file_contains` being undefined. + return toFailure("Unrecognized CLI assertion shape", lastStep, workdir); +} diff --git a/src/cli-runner.ts b/src/cli-runner.ts new file mode 100644 index 0000000..0f06c9b --- /dev/null +++ b/src/cli-runner.ts @@ -0,0 +1,289 @@ +/** + * The CLI execution core: runs a `surface: cli` flow's `run` steps, in + * declaration order, inside the flow's own working directory, applying the + * PRD's fail-fast exit-code contract — a non-final step fails the flow the + * moment its exit code isn't what was expected, while the final step's exit + * code is pure assertion territory (an error-path spec, e.g. "this command + * should fail", is a first-class flow, not a broken one). + * + * Five phases, in order: + * 1. Create the working directory (src/workdir.ts). + * 2. Setup phase — a flow-level setup block of CLI steps, run before the + * flow's own steps in the same working directory; failures are + * `phase: "setup"` with a local index into the setup array. + * 3. Steps phase — the flow's own run steps, applying the fail-fast + * exit-code contract described above. + * 4. Assertion phase — evaluates `flow.expect` in order against the last + * step's captured result via src/cli-assertions.ts's + * evaluateCliAssertion, stopping at the first failure. + * 5. Dispose the working directory, keyed on whether the flow passed. + * + * Only the LAST run step's result is kept available past its own iteration + * — intermediate steps' output is deliberately unaddressable in v1. + */ + +import { evaluateCliAssertion } from "./cli-assertions.js"; +import { DEFAULT_STEP_TIMEOUT } from "./config.js"; +import { spawnProcess } from "./exec.js"; +import { boundedExcerpt } from "./matchers.js"; +import type { CliFlowSpec, CliStep, FlowError, FlowResult } from "./types.js"; +import { createFlowWorkdir } from "./workdir.js"; + +export interface CliRunOptions { + cwd?: string; + /** + * Assertion-retry budget, in milliseconds — how long the file assertions + * keep re-checking. NOT a process deadline: a step is never killed for + * outliving this. + */ + timeout?: number; + /** + * Process-kill deadline for a single run step, in milliseconds. A step's + * own `timeout` wins over it; absent, DEFAULT_STEP_TIMEOUT applies. + */ + stepTimeout?: number; + captureLimit?: number; +} + +/** + * Build the argv for a run step. Array form passes through element for + * element, untouched. String form is split on whitespace only — never + * shell-parsed: no quote stripping, no metacharacter handling. A quoted + * substring is NOT reassembled into one argv element; the array form is the + * documented escape hatch for arguments containing spaces. + */ +function buildArgv(run: string | string[]): string[] { + if (Array.isArray(run)) { + return run; + } + return run.split(/\s+/).filter((token) => token.length > 0); +} + +/** + * Build the CLI FlowError fields (WI-805's shape) shared by every step-phase + * failure. + * + * stdout/stderr are bounded through src/matchers.ts's boundedExcerpt — the + * same helper the assertion path uses (src/cli-assertions.ts) — so a failing + * STEP reports output bounded exactly like a failing ASSERTION does. The + * per-stream capture cap in src/exec.ts is megabytes wide and is about not + * exhausting memory, not about what's readable in a terminal; without this + * a single failing step would dump its entire captured stream to the user. + */ +function stepFailure( + message: string, + step: CliStep, + index: number, + workdir: string, + execResult?: { exitCode: number; stdout: string; stderr: string }, + phase?: "setup", +): FlowError { + return { + message, + step: index, + action: step, + workdir, + phase, + ...(execResult + ? { + exitCode: execResult.exitCode, + stdout: boundedExcerpt(execResult.stdout), + stderr: boundedExcerpt(execResult.stderr), + } + : {}), + }; +} + +type StepOutcome = + | { + ok: true; + execResult: { exitCode: number; stdout: string; stderr: string }; + } + | { ok: false; error: FlowError }; + +/** + * Spawn one step (a flow step or a setup step — same grammar, same exec + * primitive) and apply the exit-code contract. `exitCodeIsFatal` is decided + * by the caller: the steps phase passes `false` only for a final step with + * no `expect_exit` (assertion territory); the setup phase always passes + * `true` — setup has no assertion phase of its own, so every setup step, + * including the last, uses the non-final rule. + */ +async function executeStep( + step: CliStep, + index: number, + workdirPath: string, + options: CliRunOptions | undefined, + exitCodeIsFatal: boolean, + phase?: "setup", +): Promise { + const label = phase === "setup" ? "Setup step" : "Step"; + const argv = buildArgv(step.run); + + let execResult: Awaited>; + try { + execResult = await spawnProcess(argv, { + cwd: workdirPath, + env: step.env, + stdin: step.stdin, + // The process deadline, in precedence order: the step's own timeout, + // the run-wide stepTimeout, then the realistic default. Deliberately + // NOT options.timeout — that is the assertion-retry budget, and using + // it here killed any command that outlived a retry window. + timeout: step.timeout ?? options?.stepTimeout ?? DEFAULT_STEP_TIMEOUT, + captureLimit: options?.captureLimit, + }); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + return { + ok: false, + error: stepFailure(message, step, index, workdirPath, undefined, phase), + }; + } + + if (execResult.timedOut) { + return { + ok: false, + error: stepFailure( + // Bounded for the same reason stepFailure bounds the fields it + // attaches: a timed-out command's stderr is often its largest + // output, and it lands in the message a user reads first. + `${label} ${index} timed out: ${boundedExcerpt(execResult.stderr)}`, + step, + index, + workdirPath, + execResult, + phase, + ), + }; + } + + const expectExitDeclared = step.expect_exit !== undefined; + const expectedExitCode = expectExitDeclared + ? (step.expect_exit as number) + : 0; + + if (exitCodeIsFatal && execResult.exitCode !== expectedExitCode) { + return { + ok: false, + error: stepFailure( + `${label} ${index} exited with code ${execResult.exitCode}, expected ${expectedExitCode}`, + step, + index, + workdirPath, + execResult, + phase, + ), + }; + } + + return { ok: true, execResult }; +} + +/** + * Run a `surface: cli` flow's steps and (eventually) its assertions, + * managing the flow's working directory lifecycle around them. + */ +export async function runCliFlow( + flow: CliFlowSpec, + options?: CliRunOptions, +): Promise { + const startTime = Date.now(); + const workdir = createFlowWorkdir(options?.cwd); + + const fail = async (error: FlowError): Promise => { + await workdir.dispose(false); + return { + success: false, + flowName: flow.name, + duration: Date.now() - startTime, + error, + }; + }; + + // Setup phase: a flow-level setup block of CLI steps, run before the + // flow's own steps, in the SAME working directory. Setup has no + // assertion phase of its own, so every setup step — including the last + // one in the array — always uses the non-final (exitCodeIsFatal: true) + // rule; there is no "final step is assertion territory" concept here. + // Failures are phase: "setup" with the setup step's own local index. + if (flow.setup && flow.setup.length > 0) { + const setupSteps = flow.setup; + for (let setupIndex = 0; setupIndex < setupSteps.length; setupIndex++) { + const outcome = await executeStep( + setupSteps[setupIndex], + setupIndex, + workdir.path, + options, + true, + "setup", + ); + if (!outcome.ok) { + return fail(outcome.error); + } + } + } + + const steps = flow.steps ?? []; + + // Only the LAST run step's result is kept available past its own + // iteration — intermediate steps' output is deliberately unaddressable + // in v1. Setup steps never populate this: assertions evaluate against + // the flow's own steps, not its setup. + let lastExecResult: + | { exitCode: number; stdout: string; stderr: string } + | undefined; + + for (let index = 0; index < steps.length; index++) { + const step = steps[index]; + const isLastStep = index === steps.length - 1; + // A non-final step, or a final step with an EXPLICIT expect_exit, is + // fatal on mismatch. A final step with no expect_exit declared is + // never fatal on exit code alone — its exit code is purely the + // assertion phase's business. + const exitCodeIsFatal = !isLastStep || step.expect_exit !== undefined; + + const outcome = await executeStep( + step, + index, + workdir.path, + options, + exitCodeIsFatal, + ); + if (!outcome.ok) { + return fail(outcome.error); + } + lastExecResult = outcome.execResult; + } + + // Assertion phase: evaluate flow.expect in order against the last step's + // captured result, stopping at the first failure. A schema-validated CLI + // flow always has at least one assertion; a hand-built fixture with none + // simply makes this loop a no-op. + if (lastExecResult) { + for (const assertion of flow.expect ?? []) { + const failure = await evaluateCliAssertion( + assertion, + lastExecResult, + workdir.path, + options?.timeout ?? 0, + ); + if (failure) { + return fail({ + message: failure.message, + exitCode: failure.exitCode, + stdout: failure.stdout, + stderr: failure.stderr, + workdir: failure.workdir, + }); + } + } + } + + await workdir.dispose(true); + return { + success: true, + flowName: flow.name, + duration: Date.now() - startTime, + }; +} diff --git a/src/config.ts b/src/config.ts index e7a2225..2d679d4 100644 --- a/src/config.ts +++ b/src/config.ts @@ -2,28 +2,143 @@ import { existsSync, readFileSync } from "node:fs"; import { join, resolve } from "node:path"; import yaml from "js-yaml"; import { z } from "zod"; -import { FlowStepSchema } from "./types.js"; +import { addSchemaFailureIssues, FlowStepSchema } from "./types.js"; + +/** + * Default process-kill deadline for a CLI run step, in milliseconds. + * + * Deliberately far larger than `timeout` (the assertion-retry budget): these + * are two different clocks that used to share one key. A retry budget of ten + * seconds is generous; ten seconds as a process deadline kills any real + * install, build, or network fetch mid-flight and reports it as a timeout. + */ +export const DEFAULT_STEP_TIMEOUT = 60000; + +/** + * The key that names what a raw (not-yet-validated) config-level setup step + * object is trying to do, for use in a human-facing error message — mirrors + * the `stepVerb()` helper in src/types.ts. + */ +function configStepVerb(step: unknown): string { + if (step && typeof step === "object" && !Array.isArray(step)) { + const [firstKey] = Object.keys(step as Record); + return firstKey ?? "unknown"; + } + return "unknown"; +} + +/** The web-surface step verbs — mirrors WEB_STEP_VERBS in src/types.ts. */ +const WEB_STEP_VERBS = [ + "visit", + "click", + "fill", + "select", + "wait_for", +] as const; + +/** The web verb present on `step`, if any (checked against the known set, not just "the first key"). */ +function matchedWebVerb(step: unknown): string | undefined { + if (!step || typeof step !== "object" || Array.isArray(step)) { + return undefined; + } + const record = step as Record; + return WEB_STEP_VERBS.find((verb) => verb in record); +} /** * Schema for FlowSpec project configuration + * + * `setup`'s field type is deliberately permissive (`z.any()` items) so a + * non-web step (e.g. a CLI `run` step) parses structurally and the + * superRefine below can name the offending verb explicitly — binding the + * field directly to `z.array(FlowStepSchema)` would make Zod's own + * invalid_union error the one reported, whose top-level message is just + * "Invalid input" (the useful "Unrecognized key(s): 'run'" detail is buried + * three levels deep in `unionErrors[].issues[]`, which loadConfigFile's + * error formatting never reaches). Config-level setup stays web-only by + * design (see adr/0003 and the PRD) — `FlowStepSchema` is WI-800's WEB step + * schema, unchanged; this does not accept CLI run steps. */ -export const FlowSpecConfigSchema = z.object({ - baseUrl: z.string().url().optional().default("http://localhost:3000"), - timeout: z.number().positive().optional().default(10000), - specsDir: z.string().optional().default("specs/"), - setup: z.array(FlowStepSchema).optional(), - // Config-level only: HTTP headers applied to the browser session for - // header-protected deployments (e.g. a Vercel/Netlify bypass token). - // Deliberately absent from FlowSpecSchema — auth/environment concerns - // stay in config, out of the committed flow specs (see adr/0003). - headers: z.record(z.string()).optional(), - // How far those headers travel. Only meaningful alongside `headers`. - // Absent means the runner's default, "origin": headers go to baseUrl's - // origin only, so a bypass token is never handed to a CDN, an analytics - // pixel, or any other third party the page happens to request. "all" is - // the explicit opt-out for deployments that need them context-wide. - headersScope: z.enum(["origin", "all"]).optional(), -}); +export const FlowSpecConfigSchema = z + .object({ + baseUrl: z.string().url().optional().default("http://localhost:3000"), + // Assertion-retry budget ONLY: how long an assertion keeps being + // re-checked before it is called a failure. Never a process deadline — + // see stepTimeout below. + timeout: z.number().positive().optional().default(10000), + // CLI-surface process-kill deadline: how long a single `run` step may + // take before its process is killed. A distinct key from `timeout` + // because the two answer completely different questions, and sharing one + // value meant the (web-harmless) 10s retry budget silently killed any + // command that legitimately runs longer. + stepTimeout: z + .number() + .int() + .positive() + .optional() + .default(DEFAULT_STEP_TIMEOUT), + specsDir: z.string().optional().default("specs/"), + setup: z.array(z.any()).optional(), + // Config-level only: HTTP headers applied to the browser session for + // header-protected deployments (e.g. a Vercel/Netlify bypass token). + // Deliberately absent from FlowSpecSchema — auth/environment concerns + // stay in config, out of the committed flow specs (see adr/0003). + headers: z.record(z.string()).optional(), + // How far those headers travel. Only meaningful alongside `headers`. + // Absent means the runner's default, "origin": headers go to baseUrl's + // origin only, so a bypass token is never handed to a CDN, an analytics + // pixel, or any other third party the page happens to request. "all" is + // the explicit opt-out for deployments that need them context-wide. + headersScope: z.enum(["origin", "all"]).optional(), + // CLI-surface working-directory override. A relative value resolves + // against process.cwd() — that resolution, and creating a temp + // directory when this is absent, is src/workdir.ts's job, not + // config-loading's. + // + // Blank values are rejected rather than accepted: an empty (or + // whitespace-only) cwd resolves to process.cwd() — the real project + // directory — so a typo would quietly turn "isolated, disposable temp + // dir" into "run every step against the working tree, and never clean + // up". Absent means temp dir; present means a real path. + cwd: z + .string() + .refine((value) => value.trim().length > 0, { + message: + "cwd must be a non-empty path (omit the key entirely to use a temporary directory)", + }) + .optional(), + // CLI-surface per-stream capture ceiling, in bytes. No schema-level + // default: this stays undefined when absent, and DEFAULT_CAPTURE_LIMIT + // (exported by src/exec.ts) is applied downstream by the consumer that + // actually enforces it, not by config loading. + captureLimit: z.number().int().positive().optional(), + }) + .superRefine((config, ctx) => { + // Two-tier check, mirroring src/types.ts's validateStepForSurface: a + // verb from outside the web family (e.g. a CLI `run` step) gets the + // custom "Unsupported step" message naming the offending verb; a step + // whose verb IS a valid web verb but is otherwise malformed (an extra + // key, a wrong value type) surfaces that specific problem instead of a + // misleading "unsupported" wrapper around a verb that actually is + // supported. + config.setup?.forEach((step: unknown, index: number) => { + const verb = matchedWebVerb(step); + if (verb === undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["setup", index], + message: `Unsupported step "${configStepVerb(step)}" in config-level setup (web steps only)`, + }); + return; + } + + // Shared with src/types.ts's flow-level step validation: each + // specific problem is re-issued at its own field (e.g. + // "setup.0.visit: Expected string, received number") instead of one + // generic union message with the field discarded. + addSchemaFailureIssues(FlowStepSchema, step, verb, ["setup", index], ctx); + }); + }); export type FlowSpecConfig = z.infer; @@ -85,6 +200,7 @@ function interpolateValue(value: unknown, configPath: string): unknown { export const DEFAULT_CONFIG: FlowSpecConfig = { baseUrl: "http://localhost:3000", timeout: 10000, + stepTimeout: DEFAULT_STEP_TIMEOUT, specsDir: "specs/", }; @@ -192,12 +308,17 @@ export function mergeConfig( cliOptions: { baseUrl?: string; timeout?: number; + stepTimeout?: number; headers?: Record; }, ): FlowSpecConfig { return { baseUrl: cliOptions.baseUrl ?? config.baseUrl, timeout: cliOptions.timeout ?? config.timeout, + // The CLI layer validates its own --step-timeout before this point (see + // src/index.ts): `??` would otherwise let a 0 or negative flag value + // past the schema's `.positive()` and arm a zero-delay process kill. + stepTimeout: cliOptions.stepTimeout ?? config.stepTimeout, specsDir: config.specsDir, setup: config.setup, // CLI --header flags REPLACE the config headers block outright — they are @@ -206,5 +327,9 @@ export function mergeConfig( // which of the two sources supplied any given header. headers: cliOptions.headers ?? config.headers, headersScope: config.headersScope, + // Config-only, like setup/headersScope: no CLI-options equivalent for + // either key in this item's scope. + cwd: config.cwd, + captureLimit: config.captureLimit, }; } diff --git a/src/exec.ts b/src/exec.ts new file mode 100644 index 0000000..35d8fe2 --- /dev/null +++ b/src/exec.ts @@ -0,0 +1,517 @@ +/** + * The CLI-surface spawn primitive: runs a command directly from an argv + * array — never through a shell — and captures its stdout, stderr, and exit + * code. This is distinct from runner.ts's execCommand, which is purpose-built + * for driving the agent-browser binary (fixed stdin: "ignore", no cwd/env + * support) and must stay behaviorally unchanged. + */ + +export interface ExecOptions { + cwd?: string; + env?: Record; + stdin?: string; + /** Milliseconds before the process is killed. Unset means no limit. */ + timeout?: number; + /** Bytes per stream before truncating. Defaults to DEFAULT_CAPTURE_LIMIT. */ + captureLimit?: number; +} + +export interface ExecResult { + stdout: string; + stderr: string; + exitCode: number; + /** True when `options.timeout` expired and the process was killed. */ + timedOut: boolean; + /** True when either stream's captured text was truncated at its limit. */ + truncated: boolean; +} + +/** + * Default ceiling on captured stdout/stderr size. Declared here so both the + * config layer and the capture-cap enforcement below consume this single + * named constant rather than restating the literal. + */ +export const DEFAULT_CAPTURE_LIMIT = 5 * 1024 * 1024; + +/** + * Grace period between a timed-out process's SIGTERM and a follow-up + * SIGKILL, for a child that traps or ignores SIGTERM (verified interactively + * against Bun 1.3.11: SIGTERM alone left such a child alive 3000ms+ later; + * Bun does not escalate signals on its own). Not exported/configurable — + * this is termination hygiene, not a contract surface. + */ +const TIMEOUT_KILL_GRACE_PERIOD_MS = 500; + +/** + * How long to keep waiting for stdout/stderr to reach EOF *after* the child + * has exited. Killing the direct child does not necessarily close the pipes: + * a detached grandchild it spawned (`sh -c "sleep 20 & echo started"`) + * inherits the same write ends and holds them open for its own lifetime, so + * reading the streams to completion can outlive the timeout by an unbounded + * margin — verified empirically: a 500ms timeout was still pending 5000ms + * later. Past this grace period the reads are cancelled and whatever was + * already buffered is returned, so the timeout is a real deadline. Long + * enough for a well-behaved process's pending output to flush, short enough + * not to be felt. Not exported/configurable — like the kill grace period, + * this is termination hygiene, not a contract surface. + */ +const STREAM_DRAIN_GRACE_PERIOD_MS = 250; + +/** + * Appended to a stream's captured text when it was cut off at its capture + * limit — matches src/matchers.ts's own excerpt-truncation marker + * convention. Duplicated here rather than imported: exec.ts otherwise has no + * dependency on matchers.ts, and the marker is a stable, trivial literal. + */ +const TRUNCATION_MARKER = "[truncated]"; + +/** + * Minimal shape of the pieces of Bun's global spawn API this module uses. + * Deliberately a local interface accessed via a `globalThis` cast, not a + * `declare global { var Bun }` ambient augmentation: runner.ts already + * declares that global with a narrower shape (no cwd/env, fixed stdin + * modes), and a second, differently-shaped ambient declaration for the same + * global `var` would conflict under TypeScript's declaration-merging rules. + * Touching runner.ts's declaration is out of scope for this item. + */ +interface BunFileSink { + write(chunk: string): number | Promise; + end(): Promise | number | undefined; +} + +interface BunSubprocess { + stdin: BunFileSink; + stdout: ReadableStream; + stderr: ReadableStream; + exited: Promise; + kill(signal?: string): void; +} + +interface BunSpawnOptions { + cwd?: string; + env?: Record; + stdout?: "pipe" | "inherit" | "ignore"; + stderr?: "pipe" | "inherit" | "ignore"; + stdin?: "pipe" | "inherit" | "ignore"; +} + +interface BunRuntime { + spawn: (cmd: string[], options?: BunSpawnOptions) => BunSubprocess; +} + +function getBunRuntime(): BunRuntime | undefined { + return (globalThis as unknown as { Bun?: BunRuntime }).Bun; +} + +interface BoundedRead { + text: string; + truncated: boolean; +} + +/** + * Bytes captured so far by an in-flight bounded read. Kept as mutable state + * the reader appends to (rather than only as the read's return value) so + * that a read which is abandoned mid-flight — see + * STREAM_DRAIN_GRACE_PERIOD_MS — can still be finalized into whatever it had + * already buffered, instead of the partial output being thrown away. + */ +interface StreamCapture { + chunks: Uint8Array[]; + bytes: number; + truncated: boolean; +} + +/** Handle on a bounded read that is already running. */ +interface BoundedReadHandle { + capture: StreamCapture; + /** Resolves when the stream reaches EOF (or the read is cancelled). */ + done: Promise; + /** Stop reading and release the pipe, keeping what was captured. */ + cancel(): void; +} + +/** + * Start reading `stream` in the background, capturing at most `limit` bytes + * into a StreamCapture. The stream is drained in full regardless of the + * limit — bytes past it are counted but discarded, never buffered — so a + * runaway writer can never block on pipe backpressure waiting for a reader + * that stopped consuming. This is the enforcement point for the "streaming, + * not capture-then-slice" requirement: memory use is bounded by `limit` + * throughout the read, not just in the final result. + * + * The read starts immediately rather than being awaited by the caller first: + * a child writing more than one pipe buffer's worth of output blocks until + * someone consumes it, so the reads must already be in flight while the + * caller waits on `proc.exited`. + */ +function startBoundedRead( + stream: ReadableStream, + limit: number, +): BoundedReadHandle { + const capture: StreamCapture = { chunks: [], bytes: 0, truncated: false }; + const reader = stream.getReader(); + + const done = (async () => { + try { + while (true) { + const { done: finished, value } = await reader.read(); + if (finished) { + break; + } + if (!value || value.length === 0) { + continue; + } + + const remaining = limit - capture.bytes; + if (remaining <= 0) { + capture.truncated = true; + continue; + } + if (value.length > remaining) { + capture.chunks.push(value.subarray(0, remaining)); + capture.bytes += remaining; + capture.truncated = true; + } else { + capture.chunks.push(value); + capture.bytes += value.length; + } + } + } finally { + reader.releaseLock(); + } + })(); + + return { + capture, + done, + cancel() { + // Cancelling settles the in-flight read(), which lets the loop above + // finish and release the lock. Any rejection here is irrelevant: the + // pipe is being discarded either way. + void reader.cancel().catch(() => {}); + }, + }; +} + +/** + * captureLimit is a byte ceiling, so a slice landing inside a multi-byte + * UTF-8 sequence is possible — the head bytes of that sequence would + * otherwise decode as a U+FFFD replacement-character artifact rather than + * cleanly stopping before it. Walks back from the end of `buffer` (at most + * 3 bytes — the longest possible UTF-8 continuation run) to find the start + * of the trailing sequence, and drops it if it isn't fully present. + */ +function trimIncompleteUtf8Tail(buffer: Uint8Array): Uint8Array { + const CONTINUATION_MASK = 0xc0; + const CONTINUATION_TAG = 0x80; + const MAX_SEQUENCE_LENGTH = 4; + + let leadIndex = buffer.length - 1; + let walked = 0; + while ( + leadIndex >= 0 && + walked < MAX_SEQUENCE_LENGTH && + (buffer[leadIndex] & CONTINUATION_MASK) === CONTINUATION_TAG + ) { + leadIndex--; + walked++; + } + if (leadIndex < 0) { + // Nothing but continuation bytes for the whole lookback window — not a + // shape a real truncation boundary produces; leave it untouched rather + // than risk trimming legitimate content. + return buffer; + } + + const lead = buffer[leadIndex] as number; + let expectedLength: number; + if ((lead & 0x80) === 0x00) { + expectedLength = 1; // ASCII + } else if ((lead & 0xe0) === 0xc0) { + expectedLength = 2; + } else if ((lead & 0xf0) === 0xe0) { + expectedLength = 3; + } else if ((lead & 0xf8) === 0xf0) { + expectedLength = 4; + } else { + // Not a valid UTF-8 lead byte — the source stream wasn't valid UTF-8 + // to begin with, which is out of scope here; leave it as-is. + return buffer; + } + + const actualLength = buffer.length - leadIndex; + return actualLength < expectedLength ? buffer.subarray(0, leadIndex) : buffer; +} + +/** Decode what a capture holds so far into its final text form. */ +function finalizeCapture(capture: StreamCapture): BoundedRead { + const buffer = new Uint8Array(capture.bytes); + let offset = 0; + for (const chunk of capture.chunks) { + buffer.set(chunk, offset); + offset += chunk.length; + } + + // Only trim when the stream was actually cut off at the capture limit: an + // untruncated capture is the process's real, complete output and must + // never be silently shortened, even if it happens to end in a byte + // sequence that looks incomplete for some unrelated reason. + const decodable = capture.truncated ? trimIncompleteUtf8Tail(buffer) : buffer; + + return { + text: new TextDecoder("utf-8").decode(decodable), + truncated: capture.truncated, + }; +} + +/** + * Spawn `argv[0]` with `argv.slice(1)` as arguments, via Bun's native spawn + * (falling back to Node's execFileSync when Bun isn't the runtime). + * + * `options.env` OVERLAYS the inherited environment: Bun.spawn's own `env` + * option replaces the environment wholesale when provided, so it is merged + * with `process.env` here — an explicit entry wins over an inherited one of + * the same name, and everything else inherited stays visible to the child. + * + * `options.stdin`, when given, is written to the child and the stream is + * then closed. Writing to a child that has already exited throws + * synchronously (EPIPE); that write failure is swallowed — it reflects a + * reader that was never there, not a problem with the command's own result — + * and the real exit code is still awaited and reported. + * + * A command that cannot be spawned at all (e.g. not found) rejects: this is + * simply Bun.spawn's own synchronous throw (naming the executable) + * propagating out of this async function un-caught, rather than being + * papered over as an artificial exit code. + * + * `options.timeout`, when set, kills the process once it expires rather than + * letting the run hang: Bun's `.kill()` still lets `proc.exited` resolve + * (observed exit code 143) and the stdout/stderr streams still yield + * whatever was written before the kill. A timeout of 0 is a real deadline of + * zero milliseconds — the child is killed at once — not a synonym for + * "unset"; callers meaning "no limit" leave `timeout` undefined. The wait on + * stdout/stderr reaching EOF is itself bounded after the child exits (see + * STREAM_DRAIN_GRACE_PERIOD_MS), because a detached grandchild holding the + * inherited pipes open would otherwise outlast the timeout entirely. + * `options.captureLimit` (default DEFAULT_CAPTURE_LIMIT) bounds each + * stream independently; a stream that hits its limit is truncated with an + * explicit marker and reported via `truncated: true`. + */ +export async function spawnProcess( + argv: string[], + options?: ExecOptions, +): Promise { + const bun = getBunRuntime(); + + if (bun) { + return spawnWithBun(bun, argv, options); + } + + return spawnWithNode(argv, options); +} + +async function spawnWithBun( + bun: BunRuntime, + argv: string[], + options?: ExecOptions, +): Promise { + const hasStdin = options?.stdin !== undefined; + const captureLimit = options?.captureLimit ?? DEFAULT_CAPTURE_LIMIT; + + const proc = bun.spawn(argv, { + cwd: options?.cwd, + // process.env's values may be `string | undefined`; every value that + // actually exists at runtime is a string, so this overlay is cast back + // to Record for BunSpawnOptions. + env: { ...process.env, ...options?.env } as Record, + stdout: "pipe", + stderr: "pipe", + stdin: hasStdin ? "pipe" : "ignore", + }); + + if (hasStdin) { + try { + // Bun's FileSink write()/end() can reject (rather than throw + // synchronously) when the underlying pipe is already broken — e.g. a + // large payload written to a child that exited before reading it. Both + // calls must be awaited so that rejection lands in this try/catch + // instead of surfacing as an unhandled promise rejection. + await proc.stdin.write(options?.stdin as string); + await proc.stdin.end(); + } catch { + // The child may have already exited without reading stdin (EPIPE). + // Not a failure of spawnProcess — the real exit code below still + // gets reported. + } + } + + // Started before anything is awaited: a child writing more than one pipe + // buffer's worth of output blocks until a reader consumes it, so these + // must already be draining while `proc.exited` is awaited below. + const stdoutRead = startBoundedRead(proc.stdout, captureLimit); + const stderrRead = startBoundedRead(proc.stderr, captureLimit); + const reads = Promise.all([stdoutRead.done, stderrRead.done]); + // Separate, always-handled branch: on the abandoned-drain path nothing + // awaits `reads` any more, and a late failure there must not surface as an + // unhandled rejection. The branch awaited below still propagates normally. + reads.catch(() => {}); + + let timedOut = false; + let timer: ReturnType | undefined; + let graceTimer: ReturnType | undefined; + let drainTimer: ReturnType | undefined; + if (typeof options?.timeout === "number") { + timer = setTimeout(() => { + timedOut = true; + // A bare SIGTERM is not a guarantee: Bun does not escalate to SIGKILL + // on its own, so a child that traps or ignores SIGTERM would + // otherwise hang here until it exits on some unrelated schedule — + // defeating the entire point of a timeout. SIGTERM first (so a + // well-behaved child still gets a chance to shut down cleanly), then + // a hard SIGKILL if it hasn't actually exited after a short grace + // period. `proc.exited` resolving cancels the grace timer below + // before it ever fires for the common (compliant-child) case. + proc.kill(); + graceTimer = setTimeout(() => { + proc.kill("SIGKILL"); + }, TIMEOUT_KILL_GRACE_PERIOD_MS); + }, options.timeout); + } + + let exitCode: number; + try { + exitCode = await proc.exited; + + // The child is gone, but its pipes are not necessarily closed — a + // detached grandchild can still hold the write ends open indefinitely, + // so waiting for EOF here is waiting on something the timeout has no + // control over. Bound that wait: past the grace period, cancel the + // reads and report what was already captured. + const drained = await Promise.race([ + reads.then(() => true), + new Promise((resolve) => { + drainTimer = setTimeout( + () => resolve(false), + STREAM_DRAIN_GRACE_PERIOD_MS, + ); + }), + ]); + if (!drained) { + stdoutRead.cancel(); + stderrRead.cancel(); + } + } finally { + // In a finally so that a rejected stream read (or exit) can never leak + // the kill/escalation timers, which would otherwise fire against a + // process that is already reaped. + clearTimeout(timer); + clearTimeout(graceTimer); + clearTimeout(drainTimer); + } + + const stdoutResult = finalizeCapture(stdoutRead.capture); + const stderrResult = finalizeCapture(stderrRead.capture); + + const stdout = stdoutResult.truncated + ? stdoutResult.text + TRUNCATION_MARKER + : stdoutResult.text; + let stderr = stderrResult.truncated + ? stderrResult.text + TRUNCATION_MARKER + : stderrResult.text; + + if (timedOut) { + stderr += `\n[timed out after ${options?.timeout}ms]`; + } + + return { + stdout, + stderr, + exitCode, + timedOut, + truncated: stdoutResult.truncated || stderrResult.truncated, + }; +} + +/** + * Node fallback, mirroring the execFileSync-based pattern in runner.ts's + * execCommand. Best-effort only: the project's whole test suite runs under + * Bun, so no test exercises this branch — it honors the same contract by + * inspection, but coverage is not claimed for it anywhere. + * + * `execFileSync`'s `timeout`/`maxBuffer` are the closest Node primitives to + * this contract, but they don't match it exactly: both cause a *throw* + * (never a graceful resolve), and there is no guarantee of exactly-truncated + * (rather than fully-discarded) partial output on a maxBuffer overflow — + * this fallback maps their errors onto `timedOut`/`truncated` as best it + * can, but a caller relying on the Node path should not expect the same + * precision (exact byte-bounded truncation, guaranteed partial capture on + * timeout) that the Bun path guarantees and this item's tests verify. + */ +async function spawnWithNode( + argv: string[], + options?: ExecOptions, +): Promise { + const { execFileSync } = await import("node:child_process"); + const [command, ...args] = argv; + const captureLimit = options?.captureLimit ?? DEFAULT_CAPTURE_LIMIT; + + try { + const stdout = execFileSync(command, args, { + encoding: "utf-8", + cwd: options?.cwd, + env: { ...process.env, ...options?.env }, + input: options?.stdin, + timeout: options?.timeout, + maxBuffer: captureLimit, + }); + return { + stdout, + stderr: "", + exitCode: 0, + timedOut: false, + truncated: false, + }; + } catch (error: unknown) { + const execError = error as { + stderr?: Buffer | string; + stdout?: Buffer | string; + status?: number | null; + code?: string; + killed?: boolean; + signal?: string | null; + }; + + const timedOut = + execError.killed === true && + typeof options?.timeout === "number" && + execError.code !== "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"; + const truncated = execError.code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"; + + // A genuine spawn failure (command not found) throws with no exit status + // and isn't a timeout/maxBuffer kill — propagate it rather than + // resolving with a synthetic result, matching the Bun branch's + // reject-on-spawn-failure contract. + if (execError.status == null && !timedOut && !truncated) { + throw error; + } + + const stderr = + typeof execError.stderr === "string" + ? execError.stderr + : (execError.stderr?.toString() ?? ""); + const stdout = + typeof execError.stdout === "string" + ? execError.stdout + : (execError.stdout?.toString() ?? ""); + + return { + stdout, + stderr: timedOut + ? `${stderr}\n[timed out after ${options?.timeout}ms]` + : stderr, + exitCode: execError.status ?? (timedOut || truncated ? 1 : 0), + timedOut, + truncated, + }; + } +} diff --git a/src/file-matchers.ts b/src/file-matchers.ts new file mode 100644 index 0000000..087ec09 --- /dev/null +++ b/src/file-matchers.ts @@ -0,0 +1,181 @@ +/** + * Retryable file-existence and file-content matchers. Surface-agnostic + * (callers pass an already-resolved absolute path — path resolution against + * a flow's working directory is a different item's job) so the CLI + * assertion dispatcher and, later, the Conduit surface can reuse these + * unchanged. + * + * Both matchers follow executeAssertion's retry shape (src/runner.ts:599): + * a zero-overhead first check, then poll on POLL_INTERVAL until a deadline, + * returning the last failure. A timeout of 0 or absent means exactly one + * evaluation — no polling loop is entered at all. + */ + +import { access, open } from "node:fs/promises"; +import { DEFAULT_CAPTURE_LIMIT } from "./exec.js"; +import { type MatchFailure, matchContains } from "./matchers.js"; +import { POLL_INTERVAL } from "./runner.js"; + +/** + * Upper bound on how much of a file fileContains reads, reusing the same + * per-stream cap src/exec.ts applies to captured stdout/stderr rather than + * introducing a second, unrelated number. This matters because the check + * re-reads from disk on EVERY poll tick (once per POLL_INTERVAL for the + * whole timeout window): an unbounded readFile would pull an arbitrarily + * large file fully into memory, repeatedly. + */ +const FILE_READ_LIMIT = DEFAULT_CAPTURE_LIMIT; + +/** + * Read at most FILE_READ_LIMIT bytes from the head of a file. Allocates + * against the file's actual size when it is smaller than the cap, so the + * common small-file case doesn't pay for the cap. + */ +async function readBoundedFile(absolutePath: string): Promise { + const handle = await open(absolutePath, "r"); + try { + const { size } = await handle.stat(); + const length = Math.min(Number(size), FILE_READ_LIMIT); + if (length <= 0) { + return ""; + } + const buffer = Buffer.alloc(length); + const { bytesRead } = await handle.read(buffer, 0, length, 0); + return buffer.subarray(0, bytesRead).toString("utf-8"); + } finally { + await handle.close(); + } +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +/** + * Run `check` once immediately (zero overhead); if it fails and `timeout` is + * a positive number, keep re-running it every POLL_INTERVAL until `timeout` + * elapses, returning the last failure. A file's presence/content can change + * between polls (an async writer still flushing), so `check` re-reads from + * disk on every call rather than being memoized. + */ +async function pollUntilPass( + check: () => Promise, + timeout: number | undefined, +): Promise { + const firstResult = await check(); + if (!firstResult) { + return undefined; + } + + if (!timeout || timeout <= 0) { + return firstResult; + } + + const deadline = Date.now() + timeout; + let lastResult: MatchFailure | undefined = firstResult; + + while (Date.now() < deadline) { + await sleep(Math.max(0, Math.min(POLL_INTERVAL, deadline - Date.now()))); + + // The sleep above is clamped to land the wake-up at (not past) the + // deadline, so the intended final look happens right at the deadline + // moment — that at-the-deadline check is deliberate, not a bug. What + // IS a bug: a timer that was scheduled to fire at the deadline can, if + // the event loop is busy elsewhere, actually resume much later than + // that. Evaluating `check()` unconditionally at that point would let a + // file that only appeared during that extra, unbudgeted lag slip + // through as a pass. + // + // The two failure modes pull in opposite directions, so the guard has + // to draw a deliberate line rather than reject on any overshoot: normal + // timer resolution means even a healthy, on-time resume routinely lands + // a few milliseconds past `deadline`, and rejecting that too would + // silently swallow the at-the-deadline check on nearly every call, + // turning "poll until timeout" into "poll until timeout minus one poll + // interval" for every caller. So only an overshoot bigger than a full + // POLL_INTERVAL — the hallmark of real event-loop lag, not scheduler + // noise — counts as the budget having been genuinely spent, and skips + // the final evaluation outright rather than trusting a stale-by-design + // check. This is a fixed, deadline-relative comparison, not a race + // against timer jitter: ordinary jitter (single-digit milliseconds) + // never comes close to a whole POLL_INTERVAL, so it can't flip this + // branch by accident. + if (Date.now() - deadline > POLL_INTERVAL) { + break; + } + + lastResult = await check(); + if (!lastResult) { + return undefined; + } + } + + return lastResult; +} + +async function checkFileExists( + absolutePath: string, +): Promise { + try { + await access(absolutePath); + return undefined; + } catch { + return { + message: `Expected file to exist at ${absolutePath} but it was not found`, + expected: absolutePath, + }; + } +} + +async function checkFileContains( + absolutePath: string, + expected: string, +): Promise { + let content: string; + try { + content = await readBoundedFile(absolutePath); + } catch { + return { + message: `Expected file at ${absolutePath} to contain "${expected}" but the file was not found`, + expected, + }; + } + + const failure = matchContains(content, expected); + if (!failure) { + return undefined; + } + + return { + message: `File ${absolutePath}: ${failure.message}`, + expected: failure.expected, + actual: failure.actual, + }; +} + +/** + * Passes once `absolutePath` exists on disk. On failure, the message names + * the absolute path. + */ +export async function fileExists( + absolutePath: string, + timeout?: number, +): Promise { + return pollUntilPass(() => checkFileExists(absolutePath), timeout); +} + +/** + * Passes once the file at `absolutePath` exists and its UTF-8 content + * contains `expected` as a substring. On failure, the message names both the + * absolute path and the expected text. + */ +export async function fileContains( + absolutePath: string, + expected: string, + timeout?: number, +): Promise { + return pollUntilPass( + () => checkFileContains(absolutePath, expected), + timeout, + ); +} diff --git a/src/index.ts b/src/index.ts index 08cb751..1fba658 100755 --- a/src/index.ts +++ b/src/index.ts @@ -1,7 +1,13 @@ #!/usr/bin/env node import { existsSync, mkdirSync, readdirSync, statSync } from "node:fs"; import { join, resolve } from "node:path"; -import { CONFIG_FILE_NAME, loadConfig, mergeConfig } from "./config.js"; +import { + CONFIG_FILE_NAME, + DEFAULT_STEP_TIMEOUT, + type FlowSpecConfig, + loadConfig, + mergeConfig, +} from "./config.js"; import { formatInitResult, initProject } from "./init.js"; import { parseFlowFile } from "./parser.js"; import { formatResult, formatSummary } from "./reporter.js"; @@ -10,11 +16,15 @@ import type { FlowResult, FlowSpec, FlowStep } from "./types.js"; interface CliOptions { path?: string; - baseUrl?: string; timeout?: number; + baseUrl?: string; + /** CLI-surface process-kill deadline for one run step, in milliseconds. */ + stepTimeout?: number; headers?: Record; /** First malformed --header argument, if any. */ headerError?: string; + /** First malformed --step-timeout argument, if any. */ + stepTimeoutError?: string; showHelp: boolean; } @@ -29,6 +39,10 @@ Commands: Run Command Options: --base-url Base URL for relative paths (default from config or http://localhost:3000) --timeout Assertion retry timeout in milliseconds (default: ${DEFAULT_TIMEOUT}) + --step-timeout + Milliseconds a single CLI (surface: cli) run step may + take before its process is killed. Must be a positive + integer (default: ${DEFAULT_STEP_TIMEOUT}) --header "Name: value" Extra HTTP header to send. Repeatable; overrides the config headers block entirely @@ -85,6 +99,26 @@ function recordHeaderError(options: CliOptions, message: string): void { options.headerError ??= message; } +/** + * Parse one `--step-timeout ` argument. + * + * Validated here rather than left to the config schema: mergeConfig applies + * CLI values with `??`, so an out-of-range flag value would otherwise sail + * past the schema's `.positive()` and reach the spawn primitive intact — + * where 0 is a real, zero-millisecond deadline that kills every step the + * instant it starts. A bad value is a misconfiguration (exit 2), not a + * silently-ignored flag. + */ +function parseStepTimeoutArg(arg: string): number | string { + const value = Number(arg); + + if (!Number.isInteger(value) || value <= 0) { + return `Error: invalid --step-timeout "${arg}" - expected a positive integer number of milliseconds`; + } + + return value; +} + function parseArgs(args: string[]): CliOptions { const options: CliOptions = { showHelp: false, @@ -102,6 +136,21 @@ function parseArgs(args: string[]): CliOptions { if (!Number.isNaN(timeoutValue)) { options.timeout = timeoutValue; } + } else if (arg === "--step-timeout") { + const rawValue = args[i + 1]; + if (rawValue === undefined) { + options.stepTimeoutError ??= + "Error: --step-timeout requires a positive integer number of milliseconds"; + continue; + } + i++; + + const parsed = parseStepTimeoutArg(rawValue); + if (typeof parsed === "string") { + options.stepTimeoutError ??= parsed; + } else { + options.stepTimeout = parsed; + } } else if (arg === "--header") { const headerArg = args[i + 1]; if (headerArg === undefined) { @@ -183,24 +232,29 @@ function parseFlowFiles(filePaths: string[]): { return { flows, errors }; } +/** + * Takes the merged config as one object rather than a long positional list: + * several of its fields are same-typed millisecond numbers (`timeout` is the + * assertion-retry budget, `stepTimeout` the CLI process-kill deadline) and + * transposing two of those at a call site would be silent. + */ async function runFlows( parsedFlows: ParsedFlow[], - baseUrl: string, - timeout: number | undefined, - configSetup: FlowStep[] | undefined, - configHeaders: Record | undefined, - configHeadersScope: "origin" | "all" | undefined, + config: FlowSpecConfig, ): Promise { const results: FlowResult[] = []; for (let i = 0; i < parsedFlows.length; i++) { const { flow } = parsedFlows[i]; const result = await runFlow(flow, { - baseUrl, - timeout, - setup: configSetup, - headers: configHeaders, - headersScope: configHeadersScope, + baseUrl: config.baseUrl, + timeout: config.timeout, + stepTimeout: config.stepTimeout, + setup: config.setup as FlowStep[] | undefined, + headers: config.headers, + headersScope: config.headersScope, + cwd: config.cwd, + captureLimit: config.captureLimit, }); console.log(formatResult(result)); results.push(result); @@ -293,11 +347,13 @@ async function handleRunCommand(args: string[]): Promise { process.exit(0); } - // A malformed --header is a misconfiguration, not a flow failure: exit 2 - // before any flow parses and before any browser session opens, exactly as - // an invalid config file does. - if (options.headerError) { - console.error(options.headerError); + // A malformed --header or --step-timeout is a misconfiguration, not a flow + // failure: exit 2 before any flow parses, before any browser session opens + // and before any command is spawned, exactly as an invalid config file + // does. + const flagError = options.headerError ?? options.stepTimeoutError; + if (flagError) { + console.error(flagError); process.exit(2); } @@ -324,6 +380,7 @@ async function handleRunCommand(args: string[]): Promise { mergedConfig = mergeConfig(config, { baseUrl: options.baseUrl, timeout: options.timeout, + stepTimeout: options.stepTimeout, headers: options.headers, }); } catch (error) { @@ -351,14 +408,7 @@ async function handleRunCommand(args: string[]): Promise { } // Run all flows - const results = await runFlows( - flows, - mergedConfig.baseUrl, - mergedConfig.timeout, - mergedConfig.setup, - mergedConfig.headers, - mergedConfig.headersScope, - ); + const results = await runFlows(flows, mergedConfig); // Print summary console.log(); diff --git a/src/matchers.ts b/src/matchers.ts new file mode 100644 index 0000000..1ac00da --- /dev/null +++ b/src/matchers.ts @@ -0,0 +1,218 @@ +/** + * Surface-agnostic assertion primitives: substring containment, regex + * matching, and dot-path JSON value comparison. Pure functions over plain + * strings and values — no dependency on any surface's step/assertion types — + * so the CLI assertion vocabulary and the upcoming Conduit/API surfaces can + * all reuse them unchanged. + * + * None of the three ever throws: malformed regex and invalid JSON are + * reported as a structured failure, never an exception. + */ + +export interface MatchFailure { + message: string; + expected?: unknown; + actual?: unknown; +} + +/** + * Shared bound for how much source text a failure message quotes back. + * Kept as a single named constant so every matcher — and the CLI-assertion + * item that reuses it — points at the same number. + */ +export const EXCERPT_LIMIT = 200; + +const TRUNCATION_MARKER = "[truncated]"; + +/** + * Head-truncate `text` to EXCERPT_LIMIT characters, appending an explicit + * marker only when truncation actually happened. Text at or under the limit + * is returned unchanged. + * + * Exported as the SINGLE source of truth for excerpt bounding: the CLI + * assertion dispatcher (src/cli-assertions.ts) and the CLI runner's + * step-failure path (src/cli-runner.ts) both call this rather than + * reimplementing the limit + marker convention, so a failing step and a + * failing assertion can never report output bounded two different ways. + */ +export function boundedExcerpt(text: string): string { + if (text.length <= EXCERPT_LIMIT) { + return text; + } + return `${text.slice(0, EXCERPT_LIMIT)}${TRUNCATION_MARKER}`; +} + +/** Render a value for inclusion in a failure message. */ +function describeValue(value: unknown): string { + return typeof value === "string" ? value : JSON.stringify(value); +} + +/** + * Passes when `needle` is a substring of `haystack`. On failure, the message + * names the needle and carries a bounded excerpt of the haystack. + */ +export function matchContains( + haystack: string, + needle: string, +): MatchFailure | undefined { + if (haystack.includes(needle)) { + return undefined; + } + return { + message: `Expected text to contain "${needle}" but it was not found. Actual: ${boundedExcerpt(haystack)}`, + expected: needle, + actual: haystack, + }; +} + +/** + * Multiline: the text these patterns run against — CLI stdout/stderr and + * file contents — is multi-line by nature, so `^`/`$` anchor per LINE, which + * is what a spec author writing `stderr_matches: "^error"` against real + * command output means. A no-op for single-line text, where whole-string + * anchoring behaves exactly as it did before. + */ +const REGEX_FLAGS = "m"; + +/** + * Passes when `pattern` (compiled as a RegExp with REGEX_FLAGS) matches + * `haystack`. Never throws: a pattern that fails to compile is reported as a + * structured failure rather than propagating the SyntaxError. On failure, + * the message names the pattern and carries a bounded excerpt of the + * haystack. + */ +export function matchRegex( + haystack: string, + pattern: string, +): MatchFailure | undefined { + let regex: RegExp; + try { + regex = new RegExp(pattern, REGEX_FLAGS); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + return { + message: `Invalid regex pattern "${pattern}": ${reason}. Actual: ${boundedExcerpt(haystack)}`, + expected: pattern, + actual: haystack, + }; + } + + if (regex.test(haystack)) { + return undefined; + } + + return { + message: `Expected text to match pattern "${pattern}" but it did not. Actual: ${boundedExcerpt(haystack)}`, + expected: pattern, + actual: haystack, + }; +} + +type PathResolution = { found: true; value: unknown } | { found: false }; + +/** + * Resolve a "$.a.b" style dot-path against a parsed JSON value. The leading + * "$" is stripped before splitting on ".". Traversing into a missing key, or + * into a non-object/array value, resolves to not-found rather than throwing. + */ +function resolvePath(root: unknown, path: string): PathResolution { + const withoutRoot = path.startsWith("$") ? path.slice(1) : path; + const segments = withoutRoot + .split(".") + .filter((segment) => segment.length > 0); + + let current = root; + for (const key of segments) { + if (current === null || typeof current !== "object") { + return { found: false }; + } + const container = current as Record; + if (!Object.hasOwn(container, key)) { + return { found: false }; + } + current = container[key]; + } + + return { found: true, value: current }; +} + +/** Structural (deep) equality over JSON-shaped values. */ +function deepEqual(a: unknown, b: unknown): boolean { + if (a === b) { + return true; + } + if (typeof a !== typeof b) { + return false; + } + if (a === null || b === null) { + return false; + } + if (Array.isArray(a) !== Array.isArray(b)) { + return false; + } + if (Array.isArray(a) && Array.isArray(b)) { + return ( + a.length === b.length && + a.every((item, index) => deepEqual(item, b[index])) + ); + } + if (typeof a === "object" && typeof b === "object") { + const aObj = a as Record; + const bObj = b as Record; + const aKeys = Object.keys(aObj); + const bKeys = Object.keys(bObj); + return ( + aKeys.length === bKeys.length && + aKeys.every( + (key) => Object.hasOwn(bObj, key) && deepEqual(aObj[key], bObj[key]), + ) + ); + } + return false; +} + +/** + * Parses `text` as JSON, resolves `path` (a "$.a.b" style dot-path) against + * it, and compares the resolved value to `expected` by deep equality. Never + * throws: invalid JSON, an absent path segment, and a mismatch are all + * reported as a structured failure. + * + * - Invalid JSON: the message carries the parser's own error plus a bounded + * excerpt of the raw text. + * - Absent path (including traversal into a primitive): the message names + * the full original path string. + * - Value mismatch: the message names both the expected and actual values. + */ +export function matchJsonPath( + text: string, + path: string, + expected: unknown, +): MatchFailure | undefined { + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + return { + message: `Could not parse JSON: ${reason}. Text: ${boundedExcerpt(text)}`, + }; + } + + const resolved = resolvePath(parsed, path); + if (!resolved.found) { + return { + message: `Path "${path}" was not found in the JSON document`, + expected: path, + }; + } + + if (deepEqual(resolved.value, expected)) { + return undefined; + } + + return { + message: `Path "${path}" expected ${describeValue(expected)} but got ${describeValue(resolved.value)}`, + expected, + actual: resolved.value, + }; +} diff --git a/src/reporter.ts b/src/reporter.ts index aa658d5..f3da67d 100644 --- a/src/reporter.ts +++ b/src/reporter.ts @@ -1,4 +1,4 @@ -import type { FlowError, FlowResult, StepAction } from "./types.js"; +import type { CliStep, FlowError, FlowResult, StepAction } from "./types.js"; // ANSI color codes const GREEN = "\x1b[32m"; @@ -18,9 +18,15 @@ function formatDuration(ms: number): string { } /** - * Format a StepAction into a human-readable string + * Format a StepAction (web) or CliStep (CLI) into a human-readable string. + * + * CLI action rendering here is intentionally minimal — a short "run ..." + * label, not a full CLI-aware report. The full CLI failure report (exit + * code, stdout/stderr excerpts, kept workdir) is a separate item's scope; + * this only keeps a CLI action from silently falling through to "unknown + * action" now that FlowErrorSchema.action accepts one. */ -function formatAction(action: StepAction): string { +function formatAction(action: StepAction | CliStep): string { if ("visit" in action) { return `visit "${action.visit}"`; } @@ -33,9 +39,77 @@ function formatAction(action: StepAction): string { if ("select" in action) { return "select"; } + if ("run" in action) { + const command = Array.isArray(action.run) + ? action.run.join(" ") + : action.run; + return `run "${command}"`; + } return "unknown action"; } +/** + * The indent every continuation line of the failure report carries. Both + * entry points below indent by this much, so a multi-line field's own + * embedded newlines have to be re-indented to the same depth. + */ +const REPORT_INDENT = " "; + +/** + * Re-indent every line of a possibly multi-line value so it lines up under + * its label. Without this, only the FIRST physical line of a captured + * stdout/stderr renders indented and the rest run flush against the left + * margin, visually escaping the failure block they belong to. + */ +function indentLines(text: string, indent: string): string { + return text.split("\n").join(`\n${indent}`); +} + +/** + * Build the CLI-specific failure lines (exit code, bounded stdout/stderr + * excerpts, and — when present — the kept working directory) shared by + * formatError and formatResult, so the two entry points can never drift out + * of sync with each other. + * + * The two groups gate INDEPENDENTLY, because they answer to different + * fields: + * + * - exitCode/stdout/stderr gate on `error.exitCode !== undefined`: that + * field's presence alone signals a completed CLI process (a web failure + * never sets it) — independent of whether `step`/`action` are also + * present, since an assertion failure doesn't carry a step index the way + * an action-step failure does. stdout/stderr are rendered as-is apart + * from re-indentation: they're already bounded/truncated upstream + * (src/matchers.ts's boundedExcerpt, applied by both the assertion and + * step-failure paths), so a truncation marker baked in there is preserved + * verbatim, never re-cut here. + * + * - The workdir line gates on `error.workdir !== undefined` ALONE. A spawn + * failure (command not found) never produces an exit code, but its + * working directory is genuinely kept on disk (dispose(false) still ran), + * so folding it into the exitCode gate would mean the kept directory's + * path is never printed and the user can't find it. A configured cwd (the + * user's own directory) never sets `workdir`, so nothing "working + * directory"-shaped is printed for that case either way. + */ +function formatCliFailureLines(error: FlowError): string[] { + const lines: string[] = []; + + if (error.exitCode !== undefined) { + lines.push( + `Exit code: ${error.exitCode}`, + `stdout: ${indentLines(error.stdout ?? "", REPORT_INDENT)}`, + `stderr: ${indentLines(error.stderr ?? "", REPORT_INDENT)}`, + ); + } + + if (error.workdir !== undefined) { + lines.push(`Kept working directory: ${error.workdir}`); + } + + return lines; +} + /** * Format a FlowError into a human-readable string * @param error - The FlowError to format @@ -49,9 +123,10 @@ export function formatError(error: FlowError): string { parts.push(`${stepLabel} ${error.step}: ${formatAction(error.action)}`); } - parts.push(`Error: ${error.message}`); + parts.push(`Error: ${indentLines(error.message, REPORT_INDENT)}`); + parts.push(...formatCliFailureLines(error)); - return parts.join("\n "); + return parts.join(`\n${REPORT_INDENT}`); } /** @@ -78,10 +153,15 @@ export function formatResult(result: FlowResult): string { if (result.error.step !== undefined && result.error.action) { const stepLabel = result.error.phase === "setup" ? "Setup step" : "Step"; lines.push( - ` ${stepLabel} ${result.error.step}: ${formatAction(result.error.action)}`, + `${REPORT_INDENT}${stepLabel} ${result.error.step}: ${formatAction(result.error.action)}`, ); } - lines.push(` Error: ${result.error.message}`); + lines.push( + `${REPORT_INDENT}Error: ${indentLines(result.error.message, REPORT_INDENT)}`, + ); + for (const cliLine of formatCliFailureLines(result.error)) { + lines.push(`${REPORT_INDENT}${cliLine}`); + } } return lines.join("\n"); diff --git a/src/runner.ts b/src/runner.ts index c73f388..b4797e4 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -1,6 +1,7 @@ import { existsSync } from "node:fs"; import { createRequire } from "node:module"; import { dirname, join } from "node:path"; +import { runCliFlow } from "./cli-runner.js"; import type { FlowError, FlowResult, @@ -9,6 +10,7 @@ import type { StepAction, StepAssertion, } from "./types.js"; +import { asCliFlow, asWebFlow } from "./types.js"; // Declare minimal Bun types for TypeScript when running in Bun runtime declare global { @@ -36,7 +38,18 @@ declare global { */ export interface RunnerOptions { baseUrl?: string; + /** + * Assertion-retry budget, in milliseconds: how long an assertion keeps + * being re-checked before it fails. Never a process deadline — a CLI run + * step's kill deadline is `stepTimeout`. + */ timeout?: number; + /** + * CLI-surface process-kill deadline for a single `run` step, in + * milliseconds. Web flows ignore this. Absent falls back to + * DEFAULT_STEP_TIMEOUT (src/config.ts), not to `timeout`. + */ + stepTimeout?: number; setup?: FlowStep[]; headers?: Record; /** @@ -46,6 +59,10 @@ export interface RunnerOptions { * "all" is the deliberate opt-out — context-wide, every request, every host. */ headersScope?: "origin" | "all"; + /** CLI-surface working-directory override. Web flows ignore this. */ + cwd?: string; + /** CLI-surface per-stream capture ceiling, in bytes. Web flows ignore this. */ + captureLimit?: number; } /** @@ -469,9 +486,24 @@ async function executeWaitFor( // Calculate deadline for retry loop const deadline = Date.now() + timeout; - // Poll loop: sleep, then re-check until found or deadline + // Poll loop: sleep (clamped to whatever's left of the budget, so the + // wake-up lands at the deadline instead of routinely overshooting it by + // a full POLL_INTERVAL), then re-check until found or deadline. while (Date.now() < deadline) { - await sleep(POLL_INTERVAL); + await sleep(Math.max(0, Math.min(POLL_INTERVAL, deadline - Date.now()))); + + // See src/file-matchers.ts's pollUntilPass for the full reasoning + // (this loop has the same shape and the same bug): a timer clamped to + // wake at the deadline can, under event-loop lag, resume much later + // than that, and checking again at that point could accept text that + // only appeared during the unbudgeted overrun. Only an overshoot + // bigger than a full POLL_INTERVAL is treated as the deadline having + // been genuinely blown through — small overshoot is ordinary timer + // jitter, and the at-the-deadline check it would otherwise skip is the + // intentional last look, not the bug being fixed here. + if (Date.now() - deadline > POLL_INTERVAL) { + break; + } lastError = await checkTextVisible(text, session); if (!lastError) { @@ -635,9 +667,25 @@ async function executeAssertion( // Calculate deadline for retry loop const deadline = Date.now() + timeout; - // Poll loop: sleep, then re-check until pass or deadline + // Poll loop: sleep (clamped to whatever's left of the budget, so the + // wake-up lands at the deadline instead of routinely overshooting it by + // a full POLL_INTERVAL), then re-check until pass or deadline. while (Date.now() < deadline) { - await sleep(POLL_INTERVAL); + await sleep(Math.max(0, Math.min(POLL_INTERVAL, deadline - Date.now()))); + + // See src/file-matchers.ts's pollUntilPass for the full reasoning + // (this loop has the same shape and the same bug): a timer clamped to + // wake at the deadline can, under event-loop lag, resume much later + // than that, and re-checking at that point could accept page state + // that only became true during the unbudgeted overrun. Only an + // overshoot bigger than a full POLL_INTERVAL is treated as the + // deadline having been genuinely blown through — small overshoot is + // ordinary timer jitter, and the at-the-deadline check it would + // otherwise skip is the intentional last look, not the bug being + // fixed here. + if (Date.now() - deadline > POLL_INTERVAL) { + break; + } // Re-check assertion (re-fetches page state from browser) lastError = await checkAssertion(assertion, session); @@ -730,6 +778,27 @@ export async function runFlow( flow: FlowSpec, options?: RunnerOptions, ): Promise { + // Surface dispatch: the ABSOLUTE FIRST thing runFlow does, before a + // browser session name is even generated, before headers are validated — + // a CLI flow must never resolve or launch agent-browser. runCliFlow owns + // the whole CLI lifecycle (working directory, steps, assertions); the + // web path below is completely untouched for surface: "web" (or absent). + if (flow.surface === "cli") { + // The one place the surface is decided is the one place the flow is + // narrowed to its CLI shape (see asCliFlow) — the CLI runner then works + // in CliStep/CliAssertion terms throughout, with no per-use casts. + return runCliFlow(asCliFlow(flow), { + cwd: options?.cwd, + timeout: options?.timeout, + stepTimeout: options?.stepTimeout, + captureLimit: options?.captureLimit, + }); + } + + // Everything past the dispatch is the web path, so the flow is narrowed to + // the web vocabulary once, here, rather than at each step/assertion use. + const webFlow = asWebFlow(flow); + const startTime = Date.now(); const baseUrl = options?.baseUrl ?? DEFAULT_BASE_URL; const timeout = options?.timeout ?? DEFAULT_TIMEOUT; @@ -783,7 +852,7 @@ export async function runFlow( // config/CLI-level one entirely (no merging). An empty array is not // nullish, so an explicit `setup: []` on the flow opts out even when // options.setup is supplied. - const setupSteps = flow.setup ?? options?.setup; + const setupSteps = webFlow.setup ?? options?.setup; // Execute setup steps (if any) in the same browser session, before the // flow's own steps, so state established during setup (e.g. an auth @@ -815,8 +884,8 @@ export async function runFlow( } // Execute all steps - for (let stepIndex = 0; stepIndex < flow.steps.length; stepIndex++) { - const step = flow.steps[stepIndex]; + for (let stepIndex = 0; stepIndex < webFlow.steps.length; stepIndex++) { + const step = webFlow.steps[stepIndex]; try { await executeStep(step, baseUrl, session, timeout, scopedHeaders); @@ -837,7 +906,7 @@ export async function runFlow( } // Execute all assertions with retry/polling - for (const assertion of flow.expect) { + for (const assertion of webFlow.expect) { const assertionError = await executeAssertion( assertion, session, diff --git a/src/types.ts b/src/types.ts index 0dbb5d1..1d2b819 100644 --- a/src/types.ts +++ b/src/types.ts @@ -59,19 +59,745 @@ export const FlowStepSchema = StepActionSchema; export type FlowStep = z.infer; +/** + * Discriminates which step grammar a flow's steps/setup/expect must conform + * to. "web" (the default) is the existing browser-driven verb family + * (visit/click/fill/select/wait_for). "cli" is the command-line adapter: + * run steps executed as subprocesses instead of browser actions. + */ +export const SurfaceSchema = z.enum(["web", "cli"]); + +export type Surface = z.infer; + +/** + * Schema for a CLI run step: executes a command and optionally checks its + * result. `run` accepts either a single command string or an argv array. + * Unknown keys are rejected by name via `.strict()`. + */ +export const CliStepSchema = z + .object({ + run: z.union([z.string(), z.array(z.string())]), + stdin: z.string().optional(), + env: z.record(z.string()).optional(), + timeout: z.number().positive().optional(), + expect_exit: z.number().int().optional(), + }) + .strict(); + +export type CliStep = z.infer; + +function compilesAsRegex(pattern: string): boolean { + try { + new RegExp(pattern); + return true; + } catch { + return false; + } +} + +/** + * Every string-valued assertion payload is non-empty by construction. + * + * An empty needle, pattern, or path is not a weaker assertion — it is a + * vacuous one: `"".includes("")` is true, an empty regex matches every + * string, and an empty path resolves to the working directory itself. A spec + * carrying one reports green while checking nothing, which is worse than no + * assertion at all, so it is rejected at parse time. + */ +function nonEmptyString(field: string): z.ZodString { + return z.string().min(1, `${field} must not be empty`); +} + +// CLI assertion schemas - each is a distinct object shape, matching the +// .strict() convention of the web assertion schemas above. +const ExitCodeAssertionSchema = z + .object({ exit_code: z.number().int() }) + .strict(); +const StdoutContainsAssertionSchema = z + .object({ stdout_contains: nonEmptyString("stdout_contains") }) + .strict(); +const StderrContainsAssertionSchema = z + .object({ stderr_contains: nonEmptyString("stderr_contains") }) + .strict(); +const FileExistsAssertionSchema = z + .object({ file_exists: nonEmptyString("file_exists") }) + .strict(); + +/** + * `stdout_matches`/`stderr_matches` must be a compilable regex, checked at + * parse time (before any flow runs) via `.refine()` so an uncompilable + * pattern is a schema-level error naming the pattern, not a runtime crash. + */ +const StdoutMatchesAssertionSchema = z + .object({ stdout_matches: nonEmptyString("stdout_matches") }) + .strict() + .refine( + (value) => compilesAsRegex(value.stdout_matches), + (value) => ({ + message: `Invalid regex pattern "${value.stdout_matches}" for stdout_matches`, + }), + ); + +const StderrMatchesAssertionSchema = z + .object({ stderr_matches: nonEmptyString("stderr_matches") }) + .strict() + .refine( + (value) => compilesAsRegex(value.stderr_matches), + (value) => ({ + message: `Invalid regex pattern "${value.stderr_matches}" for stderr_matches`, + }), + ); + +/** + * Compound assertions (`file_contains`, `json_output`) need a required-key + * naming a missing sub-key explicitly. Zod's own "Required" message for a + * missing field never names the field, and — because a missing key must + * still let the base object parse succeed for the superRefine below to run + * at all — each required sub-key is typed permissively (`.optional()` / + * `z.unknown()`) at the base level, with real presence enforcement done here. + */ +const FileContainsAssertionSchema = z + .object({ + file_contains: z + .object({ + path: nonEmptyString("path").optional(), + text: nonEmptyString("text").optional(), + }) + .strict() + .superRefine((value, ctx) => { + if (value.path === undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["path"], + message: 'Missing required key "path" in file_contains', + }); + } + if (value.text === undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["text"], + message: 'Missing required key "text" in file_contains', + }); + } + }), + }) + .strict(); + +/** + * `equals` is deliberately `z.unknown()` (any expected value is valid), which + * means Zod treats an entirely-absent "equals" key as satisfied — verified + * interactively. `Object.hasOwn` against the parsed value is required to + * actually detect absence; the parsed value only carries the key at all when + * the input carried it (confirmed: Zod does not synthesize an + * `equals: undefined` placeholder for a key that was never present). + */ +const JsonOutputAssertionSchema = z + .object({ + json_output: z + .object({ path: nonEmptyString("path").optional(), equals: z.unknown() }) + .strict() + .superRefine((value, ctx) => { + if (value.path === undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["path"], + message: 'Missing required key "path" in json_output', + }); + } + if (!Object.hasOwn(value, "equals")) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["equals"], + message: 'Missing required key "equals" in json_output', + }); + } + }), + }) + .strict(); + +/** + * Schema for CLI assertions: exit_code, stdout_contains, stdout_matches, + * stderr_contains, stderr_matches, file_exists, file_contains, json_output. + */ +export const CliAssertionSchema = z.union([ + ExitCodeAssertionSchema, + StdoutContainsAssertionSchema, + StdoutMatchesAssertionSchema, + StderrContainsAssertionSchema, + StderrMatchesAssertionSchema, + FileExistsAssertionSchema, + FileContainsAssertionSchema, + JsonOutputAssertionSchema, +]); + +export type CliAssertion = z.infer; + +/** + * Resolves a verb key directly to its own schema (rather than the surface's + * whole union) so a structurally-matched-but-malformed assertion (e.g. a + * `file_contains` missing `path`) surfaces that specific schema's own + * message. Running the whole union instead would make every OTHER member + * fail too (they don't have this assertion's key at all), and Zod's + * invalid_union error swallows every member's specific message behind a + * generic one. + * + * This is the single source of truth for the CLI assertion vocabulary: + * CLI_ASSERTION_VERBS below is derived from its keys rather than listed + * separately, so a verb added here can never drift out of sync with the + * verbs `validateAssertionForSurface` recognizes. + */ +const CLI_ASSERTION_SCHEMAS = { + exit_code: ExitCodeAssertionSchema, + stdout_contains: StdoutContainsAssertionSchema, + stdout_matches: StdoutMatchesAssertionSchema, + stderr_contains: StderrContainsAssertionSchema, + stderr_matches: StderrMatchesAssertionSchema, + file_exists: FileExistsAssertionSchema, + file_contains: FileContainsAssertionSchema, + json_output: JsonOutputAssertionSchema, +} satisfies Record; + +/** The CLI-surface assertion verbs — anything else is not a CLI assertion. */ +const CLI_ASSERTION_VERBS = Object.keys( + CLI_ASSERTION_SCHEMAS, +) as (keyof typeof CLI_ASSERTION_SCHEMAS)[]; + +/** + * Single source of truth for the web assertion vocabulary — see + * CLI_ASSERTION_SCHEMAS above for why WEB_ASSERTION_VERBS is derived from + * this rather than listed separately. + */ +const WEB_ASSERTION_SCHEMAS = { + url: UrlAssertionSchema, + visible: VisibleAssertionSchema, + matches: MatchesAssertionSchema, + not_visible: NotVisibleAssertionSchema, +} satisfies Record; + +/** The web-surface assertion verbs — anything else is not a web assertion. */ +const WEB_ASSERTION_VERBS = Object.keys( + WEB_ASSERTION_SCHEMAS, +) as (keyof typeof WEB_ASSERTION_SCHEMAS)[]; + +/** The web-surface step verbs — anything else is not a web step. */ +const WEB_STEP_VERBS = [ + "visit", + "click", + "fill", + "select", + "wait_for", +] as const; + +/** + * The key that names what a raw (not-yet-validated) step object is trying to + * do, for use in a human-facing error message. `run` wins when present so a + * CLI step with an invalid modifier still reports "run", not "stdin"/"env". + */ +function stepVerb(step: unknown): string { + if (step && typeof step === "object" && !Array.isArray(step)) { + const record = step as Record; + if ("run" in record) { + return "run"; + } + const [firstKey] = Object.keys(record); + return firstKey ?? "unknown"; + } + return "unknown"; +} + +function isCliVerbStep(step: unknown): boolean { + return ( + !!step && + typeof step === "object" && + !Array.isArray(step) && + "run" in (step as Record) + ); +} + +function isWebVerbStep(step: unknown): boolean { + return ( + !!step && + typeof step === "object" && + !Array.isArray(step) && + WEB_STEP_VERBS.some((verb) => verb in (step as Record)) + ); +} + +/** + * One specific problem found in a failed parse: Zod's own message, at the + * path (relative to the value that was parsed) where it belongs. + */ +interface SchemaIssueDetail { + path: (string | number)[]; + message: string; +} + +/** + * Unwrap Zod's generic union wrapper down to the issues that actually say + * something. + * + * A union's top-level issue is always the useless "Invalid input"; the real + * detail ("Expected string, received number") lives one level down, in + * `unionErrors[branch].issues[]` — one branch per union member. Since the + * verb the author was reaching for is already known, the branches that + * recognize that verb as a field of their OWN are preferred: every other + * branch failed merely because it has no such key at all, and its + * "Unrecognized key(s)" complaint describes the schema, not the mistake. + * When no branch recognizes the verb (an entirely unrelated shape), every + * branch's issues are kept rather than reporting nothing. + */ +function flattenIssues( + issues: readonly z.ZodIssue[], + verb: string, +): SchemaIssueDetail[] { + const details: SchemaIssueDetail[] = []; + + for (const issue of issues) { + if (issue.code === z.ZodIssueCode.invalid_union) { + const relevant = issue.unionErrors.filter((branch) => + branch.issues.some((branchIssue) => branchIssue.path[0] === verb), + ); + const branches = relevant.length > 0 ? relevant : issue.unionErrors; + details.push( + ...flattenIssues( + branches.flatMap((branch) => branch.issues), + verb, + ), + ); + continue; + } + + details.push({ path: [...issue.path], message: issue.message }); + } + + return details; +} + +/** Distinct issues only: the same message at the same path is reported once. */ +function detailsFromError( + error: z.ZodError, + verb: string, +): SchemaIssueDetail[] { + const seen = new Set(); + return flattenIssues(error.issues, verb).filter((detail) => { + // The separator between the joined path and the message must be a + // character that can never appear in either half, or two genuinely + // different (path, message) pairs could collide into the same key and + // get wrongly deduped. NUL (\0) is the one character with that + // guarantee: it can't occur in a Zod issue path segment (a JS property + // name or array index) or in a Zod-generated message string. Written as + // the escape `\0` rather than a raw embedded byte — a literal NUL byte + // in the source file makes `grep`/`ripgrep` classify the whole file as + // binary, which silently breaks text search over this file. + const key = `${detail.path.join(".")}\0${detail.message}`; + if (seen.has(key)) { + return false; + } + seen.add(key); + return true; + }); +} + +/** + * Push every specific problem `value` has against `schema` as its own issue, + * each located at `basePath` + the field within `value` that carries it — so + * two different malformed sub-fields of the same step produce two different, + * individually-located errors instead of the identical generic message both + * used to get. + * + * Shared by the flow-level step/assertion validation below and by the + * config-level setup validation in src/config.ts, which had the same gap. + * + * A no-op when `value` actually parses. If a failure somehow yields no + * detail at all, Zod's own joined messages are reported at `basePath` rather + * than nothing, so a malformed value can never slip through silently. + */ +export function addSchemaFailureIssues( + schema: z.ZodTypeAny, + value: unknown, + verb: string, + basePath: (string | number)[], + ctx: z.RefinementCtx, +): void { + const result = schema.safeParse(value); + if (result.success) { + return; + } + + const details = detailsFromError(result.error, verb); + if (details.length === 0) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: basePath, + message: result.error.issues.map((issue) => issue.message).join("; "), + }); + return; + } + + for (const detail of details) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [...basePath, ...detail.path], + message: detail.message, + }); + } +} + +/** + * Re-validates a single steps/setup entry against the grammar for the flow's + * resolved surface, pushing a custom issue when it doesn't conform. + * + * The base `steps`/`setup` field type (see FlowSpecSchema) is deliberately + * permissive so that both the web and CLI grammars parse structurally at + * that level — enforcement happens entirely here, so a mismatch is reported + * with the actual offending verb and surface, and a same-surface structural + * problem (an unrecognized key, a bad value type) surfaces Zod's own + * message naming it, rather than a generic "no union member matched" error. + */ +function validateStepForSurface( + step: unknown, + surface: Surface, + path: (string | number)[], + ctx: z.RefinementCtx, +): void { + const matchesVerbFamily = + surface === "cli" ? isCliVerbStep(step) : isWebVerbStep(step); + + if (!matchesVerbFamily) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path, + message: `Step verb "${stepVerb(step)}" is not valid for surface "${surface}"`, + }); + return; + } + + const schema = surface === "cli" ? CliStepSchema : FlowStepSchema; + // The verb this step was reaching for, checked against the known set + // rather than taken as "whatever key came first" — it selects which union + // branch's issues actually describe the mistake (see flattenIssues). + const verb = + surface === "cli" + ? "run" + : (matchedVerb(step, WEB_STEP_VERBS) ?? stepVerb(step)); + addSchemaFailureIssues(schema, step, verb, path, ctx); +} + +function validateStepsForSurface( + steps: unknown, + surface: Surface, + field: "steps" | "setup", + ctx: z.RefinementCtx, +): void { + if (!Array.isArray(steps)) { + return; + } + steps.forEach((step, index) => { + validateStepForSurface(step, surface, [field, index], ctx); + }); +} + +/** + * The key that names what a raw (not-yet-validated) assertion object is + * trying to check, for use in a human-facing error message. + */ +function assertionVerb(assertion: unknown): string { + if (assertion && typeof assertion === "object" && !Array.isArray(assertion)) { + const [firstKey] = Object.keys(assertion as Record); + return firstKey ?? "unknown"; + } + return "unknown"; +} + +/** The first of `verbs` present as a key on `assertion`, if any. */ +function matchedVerb( + assertion: unknown, + verbs: readonly string[], +): string | undefined { + if (!assertion || typeof assertion !== "object" || Array.isArray(assertion)) { + return undefined; + } + const record = assertion as Record; + return verbs.find((verb) => verb in record); +} + +/** + * Re-validates a single expect-list entry against the assertion vocabulary + * for the flow's resolved surface, mirroring `validateStepForSurface`: a + * verb from the wrong surface's family is a mismatch naming the verb and + * surface, while a same-surface structural problem (missing sub-key, extra + * key, uncompilable regex) surfaces that specific assertion schema's own + * message. + */ +function validateAssertionForSurface( + assertion: unknown, + surface: Surface, + index: number, + ctx: z.RefinementCtx, +): void { + const verbs = surface === "cli" ? CLI_ASSERTION_VERBS : WEB_ASSERTION_VERBS; + const schemas = + surface === "cli" ? CLI_ASSERTION_SCHEMAS : WEB_ASSERTION_SCHEMAS; + const verb = matchedVerb(assertion, verbs); + const path = ["expect", index]; + + if (!verb) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path, + message: `Assertion "${assertionVerb(assertion)}" is not valid for surface "${surface}"`, + }); + return; + } + + // `schemas` is one of two object-literal maps whose keys are known, + // narrow string literals (enforced via `satisfies Record` at their declarations) — precise enough that + // Object.keys(...) derives CLI_ASSERTION_VERBS/WEB_ASSERTION_VERBS without + // hand-listing them (see those declarations), but too precise for `verb` + // (a plain string narrowed only at runtime by `matchedVerb`, since + // `surface` can't correlate which literal-key union it belongs to here). + // The runtime guarantee that `verb` is actually a key of `schemas` already + // comes from `matchedVerb` searching exactly this surface's own verb list. + const schema = (schemas as Record)[verb]; + addSchemaFailureIssues(schema, assertion, verb, path, ctx); +} + +function validateExpectForSurface( + expectList: unknown, + surface: Surface, + ctx: z.RefinementCtx, +): void { + if (!Array.isArray(expectList)) { + return; + } + expectList.forEach((assertion, index) => { + validateAssertionForSurface(assertion, surface, index, ctx); + }); +} + /** * Schema for a complete flow specification * Defines a user flow with steps to execute and assertions to verify + * + * `surface` selects the step/assertion grammar: absent or "web" keeps + * today's browser-driven behavior byte-identical; "cli" switches + * steps/setup/expect to the CLI vocabulary (run steps, exit_code/ + * stdout_contains/etc. assertions). + * + * `steps`/`setup`/`expect` carry the union of both surfaces' element types. + * The element schema is `z.custom` — runtime-permissive, exactly as the + * previous `z.any()` was — because the real verb/shape enforcement lives in + * the superRefine below, keyed off the resolved surface, and Zod skips a + * refinement entirely when the object it wraps already failed to parse. + * Binding these fields to a real `z.union([...])` would therefore trade + * every specific, surface-aware error message ("Step verb \"click\" is not + * valid for surface \"cli\"") for the generic "Invalid input" a union + * failure reports. What `z.custom` restores over `z.any()` is the + * COMPILE-time guarantee: `FlowSpec["steps"]` is a real union again, not + * `any[]`, so a bogus property chain on a step is a type error at every + * call site. */ -export const FlowSpecSchema = z.object({ +export const FlowSpecSchema = z + .object({ + name: z.string(), + description: z.string(), + surface: SurfaceSchema.optional().default("web"), + setup: z.array(z.custom()).optional(), + steps: z.array(z.custom()).min(1), + expect: z.array(z.custom()), + }) + .superRefine((flow, ctx) => { + const { surface } = flow; + + validateStepsForSurface(flow.steps, surface, "steps", ctx); + if (flow.setup) { + validateStepsForSurface(flow.setup, surface, "setup", ctx); + } + + // Both surfaces: a flow whose expect list is empty asserts nothing, and + // its assertion loop passes trivially. The web surface has always + // required at least one; the CLI surface needs the same floor for the + // same reason. + if (flow.expect.length === 0) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["expect"], + message: `expect must contain at least one assertion for surface ${surface}`, + }); + } else { + validateExpectForSurface(flow.expect, surface, ctx); + } + }); + +export type FlowSpec = z.infer; + +/** + * A FlowSpec already known to be CLI-surface: its steps, setup and + * assertions are the CLI vocabulary, not the union of both surfaces. + * + * This is what the CLI runner takes, so it never has to cast its way from + * "some step" to "a run step" element by element. + */ +export type CliFlowSpec = Omit< + FlowSpec, + "surface" | "setup" | "steps" | "expect" +> & { + surface: "cli"; + setup?: CliStep[]; + steps: CliStep[]; + expect: CliAssertion[]; +}; + +/** + * A FlowSpec already known to be web-surface: the browser-driven step and + * assertion vocabulary, with no CLI members mixed in. + */ +export type WebFlowSpec = Omit< + FlowSpec, + "surface" | "setup" | "steps" | "expect" +> & { + surface: "web"; + setup?: FlowStep[]; + steps: FlowStep[]; + expect: StepAssertion[]; +}; + +/** + * Schemas backing asCliFlow/asWebFlow's runtime check (CodeRabbit review, + * PR #16): CliFlowSpec/WebFlowSpec were hand-written TypeScript narrowings + * with no schema of their own, so a caller that builds a flow object + * without going through FlowSpecSchema got no runtime validation at all — + * exactly what test/cli-runner.test.ts's `cliFlow()` fixture (and its + * siblings) do, via `as unknown as CliFlowSpec`. + * + * These use real `z.array(CliStepSchema)` / `z.array(CliAssertionSchema)` + * (and the web equivalents) rather than FlowSpecSchema's `z.custom` + + * superRefine dance. That's deliberately safe here even though a real + * union at a field like this is what FlowSpecSchema itself avoids (see its + * doc comment): FlowSpecSchema is the PRIMARY, per-field error-reporting + * pass — the one a spec author's YAML actually goes through, where superRefine + * is what turns a union's generic "Invalid input" into a message naming the + * specific bad verb/key. CliFlowSpecSchema/WebFlowSpecSchema instead run + * only inside asCliFlow/asWebFlow, on a value that (on every production path) + * has ALREADY passed FlowSpecSchema's superRefine once — this is a + * confirming re-parse guarding against a caller that skipped that pass + * entirely, not the place a human-facing per-field message needs to come + * from. A coarser "this whole value doesn't match its surface" failure is + * an acceptable, deliberately-chosen tradeoff for that narrower job. + * + * `expect` deliberately has no `.min(1)` here, unlike FlowSpecSchema's own + * enforced floor of at least one assertion. That floor is an AUTHORING rule + * ("a published spec must actually assert something"), which is + * FlowSpecSchema's job at parse time — not a grammar-correctness question, + * which is this schema's only concern (does `expect` actually hold this + * surface's assertion vocabulary, whatever its length). Plenty of tests + * exercise runFlow directly with a hand-built, `expect: []` flow object to + * test dispatch/execution mechanics unrelated to assertions at all (see + * test/runner-dispatch.test.ts's own `cliFlow()` fixture) — re-enforcing the + * authoring floor here would reject exactly that established, legitimate + * pattern for a reason that has nothing to do with what this schema exists + * to catch (a wrong-surface verb slipping through an unchecked cast). + */ +const CliFlowSpecSchema = z.object({ name: z.string(), description: z.string(), - setup: z.array(FlowStepSchema).optional(), - steps: z.array(FlowStepSchema).min(1), - expect: z.array(StepAssertionSchema).min(1), + surface: z.literal("cli"), + setup: z.array(CliStepSchema).optional(), + steps: z.array(CliStepSchema).min(1), + expect: z.array(CliAssertionSchema), }); -export type FlowSpec = z.infer; +const WebFlowSpecSchema = z + .object({ + name: z.string(), + description: z.string(), + // Mirrors FlowSpecSchema's own `surface` field exactly (optional, + // defaulting to "web") rather than a bare `z.literal("web")`: runFlow's + // dispatch (`flow.surface === "cli"`) treats an absent surface as web, and + // plenty of callers — runner.test.ts's fixtures among them — build a + // FlowSpec object directly (never through FlowSpecSchema.parse) with no + // `surface` key at all. A bare literal would reject exactly the shape + // production code has always accepted as web. + // `expect` has no `.min(1)` for the same reason CliFlowSpecSchema's + // doesn't — see its comment. + surface: SurfaceSchema.optional().default("web"), + setup: z.array(FlowStepSchema).optional(), + steps: z.array(FlowStepSchema).min(1), + expect: z.array(StepAssertionSchema), + }) + .refine((value) => value.surface === "web", { + message: 'surface must be "web" (or absent) for a WebFlowSpec', + path: ["surface"], + }); + +/** + * Narrow a flow to one surface's vocabulary, at the ONE place the surface is + * dispatched on (src/runner.ts's runFlow). + * + * FlowSpecSchema's superRefine already rejects, at parse time, any flow + * whose steps/setup/expect don't match its declared surface — so for a + * flow that actually went through FlowSpecSchema.parse, the re-validation + * below restates a guarantee already enforced. It exists for the caller + * that DIDN'T: `flow` is typed as FlowSpec, but nothing prevents a caller + * from constructing one by hand (test fixtures already do). Parsing against + * CliFlowSpecSchema/WebFlowSpecSchema here, instead of a bare `as` cast, + * means such a caller fails loudly at the narrowing point instead of + * silently handing CliStep/CliAssertion-typed data that was never actually + * checked to every use site downstream. + * + * asCliFlow returns the ORIGINAL `flow`, not CliFlowSpecSchema's parse + * result — and that's fine, unlike asWebFlow below. CliFlowSpecSchema's + * `surface` field is a bare `z.literal("cli")`: it has no `.default(...)`, + * so for `.parse(flow)` to succeed at all, `flow.surface` must ALREADY be + * the literal `"cli"` on the input. There is no defaulted/derived value the + * parse produces that the input didn't already have, so returning `flow` + * unchanged discards nothing. + */ +export function asCliFlow(flow: FlowSpec): CliFlowSpec { + CliFlowSpecSchema.parse(flow); + return flow as CliFlowSpec; +} + +/** + * asWebFlow, unlike asCliFlow above, must return the PARSED value rather + * than the original `flow` (CodeRabbit review, this PR). WebFlowSpecSchema's + * `surface` field is `SurfaceSchema.optional().default("web")` — deliberately + * not a bare literal, per that schema's own doc comment, because plenty of + * legitimate callers hand it a flow object with no `surface` key at all. The + * `.default("web")` only ever takes effect on the value `.parse()` RETURNS; + * it does nothing to the object passed in. The previous implementation ran + * `WebFlowSpecSchema.parse(flow)` purely for its throwing side effect and + * then returned `flow as WebFlowSpec` — so a hand-built flow with no + * `surface` key parsed successfully (correctly) but came back with + * `surface === undefined` at runtime, while its WebFlowSpec type claimed the + * literal `"web"`. That is a real type lie: a caller trusting + * `webFlow.surface === "web"` could be wrong about a value TypeScript + * insists is web-surface. + * + * The fix returns `WebFlowSpecSchema.parse(flow)` itself, so the defaulted + * `surface` (and everything else the schema validated) is what callers + * actually get. This is safe from the "a real z.object() schema silently + * drops any key it doesn't declare" risk that would otherwise make a + * re-parse dangerous on a production path: WebFlowSpecSchema declares + * exactly the five fields FlowSpec itself has (name, description, surface, + * setup, steps, expect) — there is no sixth FlowSpec field for it to drop. + * `setup`/`steps`/`expect` re-parse through FlowStepSchema/StepAssertionSchema + * element-by-element, which reconstructs equivalent objects (same single + * key, same value) rather than mutating or dropping anything, since those + * are the same schemas FlowSpecSchema's own superRefine already validated + * each element against. + * + * The one behavior change this causes: the returned object is no longer the + * same reference as the `flow` argument (a fresh object from `.parse()`). + * No production call site (src/runner.ts's runFlow) or test in this repo + * relies on that identity — every consumer reads fields off the result, none + * compares it by reference to the input — so this is a safe tradeoff for + * closing a genuine type lie. + */ +export function asWebFlow(flow: FlowSpec): WebFlowSpec { + return WebFlowSpecSchema.parse(flow) as WebFlowSpec; +} /** * Schema for flow execution errors @@ -81,14 +807,22 @@ export type FlowSpec = z.infer; * - "setup" - a shared setup step failed * - "headers" - applying config-level HTTP headers to the browser session * failed, before any step ran (so no step/action accompanies it) + * + * `exitCode`/`stdout`/`stderr`/`workdir` are the CLI analogs of `screenshot`: + * populated by a CLI flow's failure, left absent for a web flow's (and vice + * versa — `screenshot` is simply never set for a CLI failure). */ export const FlowErrorSchema = z.object({ message: z.string(), phase: z.enum(["setup", "headers"]).optional(), step: z.number().optional(), - action: StepActionSchema.optional(), + action: z.union([StepActionSchema, CliStepSchema]).optional(), assertion: StepAssertionSchema.optional(), screenshot: z.string().optional(), + exitCode: z.number().optional(), + stdout: z.string().optional(), + stderr: z.string().optional(), + workdir: z.string().optional(), }); export type FlowError = z.infer; diff --git a/src/workdir.ts b/src/workdir.ts new file mode 100644 index 0000000..035a359 --- /dev/null +++ b/src/workdir.ts @@ -0,0 +1,83 @@ +/** + * Per-flow working directory lifecycle for the CLI execution model: a fresh, + * empty temp directory per flow — removed when the flow passes, kept (with + * its absolute path available for the failure report) when it fails. The + * CLI analog of the web surface's failure screenshot. + * + * A configured working directory is used as-is and is never removed by + * dispose, regardless of pass or fail — only a directory this module itself + * created is ever a candidate for cleanup. + * + * Deliberately zero dependencies on the rest of the FlowSpec codebase (takes + * the configured cwd as a plain `string | undefined`, not a config object), + * so it can be built and consumed independently of the config/CLI-runner + * items. + */ + +import { mkdtempSync } from "node:fs"; +import { rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { isAbsolute, join, resolve } from "node:path"; + +export interface FlowWorkdir { + /** Absolute path to the directory the flow should run in. */ + path: string; + /** True when this module created `path` (and so may remove it); false for a configured cwd. */ + isTemp: boolean; + /** + * Release the workdir. A temp directory is removed when `passed` is true + * and left in place when `passed` is false; a configured cwd is never + * removed either way. Never throws or rejects — a cleanup failure must + * never mask the flow's own pass/fail result. + */ + dispose(passed: boolean): Promise; +} + +const TEMP_DIR_PREFIX = "flowspec-"; + +/** + * Create the working directory a flow will run in. + * + * - No `configuredCwd`: creates a fresh, unique, empty temp directory + * (prefixed "flowspec-" so a kept one is identifiable) and returns it with + * `isTemp: true`. + * - `configuredCwd` given: used as-is, `isTemp: false`. A relative path is + * resolved against `process.cwd()`; an absolute path is passed through + * unchanged. Not created and not existence-checked here — the caller is + * expected to have supplied a real, usable directory. + */ +export function createFlowWorkdir(configuredCwd?: string): FlowWorkdir { + if (configuredCwd !== undefined) { + const path = isAbsolute(configuredCwd) + ? configuredCwd + : resolve(process.cwd(), configuredCwd); + + return { + path, + isTemp: false, + dispose: async () => { + // A configured cwd is never ours to remove, on pass or on fail. + }, + }; + } + + const path = mkdtempSync(join(tmpdir(), TEMP_DIR_PREFIX)); + + return { + path, + isTemp: true, + dispose: async (passed: boolean) => { + if (!passed) { + // Keep the directory (and everything written into it) for + // inspection alongside the failure report. + return; + } + try { + await rm(path, { recursive: true, force: true }); + } catch { + // Already gone, or removal was blocked (e.g. permissions) — either + // way, a cleanup failure must not surface as a dispose() rejection. + } + }, + }; +} diff --git a/test/cli-assertions.test.ts b/test/cli-assertions.test.ts new file mode 100644 index 0000000..a639944 --- /dev/null +++ b/test/cli-assertions.test.ts @@ -0,0 +1,791 @@ +import { + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + evaluateCliAssertion, + type LastStepResult, +} from "../src/cli-assertions"; +import * as FileMatchersModule from "../src/file-matchers"; +import * as MatchersModule from "../src/matchers"; +import { EXCERPT_LIMIT } from "../src/matchers"; +import { POLL_INTERVAL } from "../src/runner"; + +/** + * Tests for WI-809: the CLI assertion dispatcher — evaluates one of the + * eight CLI assertions against a completed run step's captured result. + * + * Contract pinned by the work item: + * - evaluateCliAssertion(assertion, lastStep, workdir, timeout) where + * lastStep is { stdout: string; stderr: string; exitCode: number }. + * Returns undefined on pass, or a failure object on fail: + * { message: string; exitCode: number; stdout: string; stderr: string; workdir: string } + * — every field always populated on failure, regardless of which + * assertion type failed (exitCode/stdout/stderr are the LAST STEP's + * values, bounded via src/matchers.ts's EXCERPT_LIMIT, not re-derived + * per assertion type). + * - exit_code, stdout_contains/matches, stderr_contains/matches, and + * json_output all evaluate EXACTLY ONCE — no retry/poll — even when a + * nonzero timeout is supplied. + * - file_exists / file_contains DO retry within the timeout window, and + * resolve a relative assertion path against `workdir` before handing an + * absolute path to src/file-matchers.ts. + * - This module delegates real matching to src/matchers.ts and + * src/file-matchers.ts (never reimplements substring/regex/JSON/file + * logic inline) — verified below with bare module spies, per the + * Integration Item Wiring Tests convention, since this item's whole job + * is wiring three already-tested modules together. + * + * Return-shape design note (mine — no prior convention existed): this + * module returns a minimal CliAssertionFailure, NOT a full FlowError. It + * does not set FlowErrorSchema's `assertion` field (which is still typed + * web-only, StepAssertionSchema, and was not widened by WI-805) — only + * `message`, `exitCode`, `stdout`, `stderr`, `workdir` are this module's + * concern. Assembling a full FlowError (adding `step`, `phase`, etc.) is + * the CLI runner item's job, one layer up. + */ + +function lastStep(overrides: Partial = {}) { + return { + stdout: "some output", + stderr: "", + exitCode: 0, + ...overrides, + }; +} + +const tempDirs: string[] = []; + +function makeTempDir(): string { + const dir = realpathSync( + mkdtempSync(join(tmpdir(), "flowspec-cli-assertions-")), + ); + tempDirs.push(dir); + return dir; +} + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +describe("exit_code", () => { + it("passes when it equals the last run step's exit code", async () => { + const result = await evaluateCliAssertion( + { exit_code: 0 }, + lastStep({ exitCode: 0 }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeUndefined(); + }); + + it("fails naming both the expected and actual code when they differ", async () => { + const result = await evaluateCliAssertion( + { exit_code: 0 }, + lastStep({ exitCode: 7 }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toContain("0"); + expect(result?.message).toContain("7"); + }); + + it("evaluates exactly once with no retry, even when a timeout is configured", async () => { + const start = Date.now(); + const result = await evaluateCliAssertion( + { exit_code: 0 }, + lastStep({ exitCode: 7 }), + "/tmp/irrelevant", + 5000, + ); + const elapsed = Date.now() - start; + expect(result).toBeDefined(); + expect(elapsed).toBeLessThan(POLL_INTERVAL); + }); +}); + +describe("stdout_contains / stderr_contains", () => { + it("stdout_contains passes when the needle is present in stdout", async () => { + const result = await evaluateCliAssertion( + { stdout_contains: "some" }, + lastStep({ stdout: "some output" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeUndefined(); + }); + + it("stdout_contains fails with a bounded excerpt of stdout when absent", async () => { + const result = await evaluateCliAssertion( + { stdout_contains: "missing-needle" }, + lastStep({ stdout: "some output" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toContain("some output"); + }); + + it("stderr_contains passes when the needle is present in stderr", async () => { + const result = await evaluateCliAssertion( + { stderr_contains: "boom" }, + lastStep({ stderr: "error: boom happened" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeUndefined(); + }); + + it("stderr_contains fails with a bounded excerpt of stderr when absent", async () => { + const result = await evaluateCliAssertion( + { stderr_contains: "missing" }, + lastStep({ stderr: "error: boom happened" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toContain("boom happened"); + }); + + it("evaluates exactly once with no retry, even when a timeout is configured", async () => { + const start = Date.now(); + const result = await evaluateCliAssertion( + { stdout_contains: "missing-needle" }, + lastStep({ stdout: "some output" }), + "/tmp/irrelevant", + 5000, + ); + const elapsed = Date.now() - start; + expect(result).toBeDefined(); + expect(elapsed).toBeLessThan(POLL_INTERVAL); + }); + + it("delegates to the real matchContains primitive", async () => { + const spy = vi.spyOn(MatchersModule, "matchContains"); + await evaluateCliAssertion( + { stdout_contains: "some" }, + lastStep({ stdout: "some output" }), + "/tmp/irrelevant", + 0, + ); + expect(spy).toHaveBeenCalled(); + spy.mockRestore(); + }); +}); + +describe("stdout_matches / stderr_matches", () => { + it("stdout_matches passes when the pattern matches stdout", async () => { + const result = await evaluateCliAssertion( + { stdout_matches: "^\\d{3}-\\d{4}$" }, + lastStep({ stdout: "555-1234" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeUndefined(); + }); + + it("stdout_matches fails with a bounded excerpt of stdout when it does not match", async () => { + const result = await evaluateCliAssertion( + { stdout_matches: "^\\d{3}-\\d{4}$" }, + lastStep({ stdout: "not-a-phone-number" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toContain("not-a-phone-number"); + }); + + it("stderr_matches passes when the pattern matches stderr", async () => { + const result = await evaluateCliAssertion( + { stderr_matches: "^error:" }, + lastStep({ stderr: "error: something broke" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeUndefined(); + }); + + it("stderr_matches fails with a bounded excerpt of stderr when it does not match", async () => { + const result = await evaluateCliAssertion( + { stderr_matches: "^error:" }, + lastStep({ stderr: "warning: something odd" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toContain("warning: something odd"); + }); + + it("evaluates exactly once with no retry, even when a timeout is configured", async () => { + const start = Date.now(); + const result = await evaluateCliAssertion( + { stdout_matches: "^\\d{3}-\\d{4}$" }, + lastStep({ stdout: "not-a-phone-number" }), + "/tmp/irrelevant", + 5000, + ); + const elapsed = Date.now() - start; + expect(result).toBeDefined(); + expect(elapsed).toBeLessThan(POLL_INTERVAL); + }); + + it("delegates to the real matchRegex primitive", async () => { + const spy = vi.spyOn(MatchersModule, "matchRegex"); + await evaluateCliAssertion( + { stdout_matches: "^\\d{3}-\\d{4}$" }, + lastStep({ stdout: "555-1234" }), + "/tmp/irrelevant", + 0, + ); + expect(spy).toHaveBeenCalled(); + spy.mockRestore(); + }); +}); + +describe("stream assertions guard against a missing value the schema would normally prevent", () => { + // A schema-validated CliAssertion always has its verb's value present as a + // non-empty string (src/types.ts's nonEmptyString), but this function is + // also reachable directly by a caller that bypassed validation — exactly + // the case file_contains/json_output already guard against (see their + // comments in src/cli-assertions.ts). The four stream branches used to + // pass an unguarded value straight to matchContains/matchRegex, which + // reach it via JS's string coercion: `haystack.includes(undefined)` + // coerces to `haystack.includes("undefined")`, and `new RegExp(undefined)` + // compiles to `/(?:)/`, which matches every string. Both let a + // never-specified assertion silently PASS instead of failing clearly. + + it('stdout_contains with a missing value fails clearly instead of matching the coerced string "undefined"', async () => { + const result = await evaluateCliAssertion( + { stdout_contains: undefined } as unknown as CliAssertion, + lastStep({ stdout: "the value is undefined here" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toBe('Missing "stdout_contains" text'); + }); + + it('stderr_contains with a missing value fails clearly instead of matching the coerced string "undefined"', async () => { + const result = await evaluateCliAssertion( + { stderr_contains: undefined } as unknown as CliAssertion, + lastStep({ stderr: "the value is undefined here" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toBe('Missing "stderr_contains" text'); + }); + + it("stdout_matches with a missing pattern fails clearly instead of matching every string via new RegExp(undefined)", async () => { + const result = await evaluateCliAssertion( + { stdout_matches: undefined } as unknown as CliAssertion, + lastStep({ stdout: "anything at all" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toBe('Missing "stdout_matches" pattern'); + }); + + it("stderr_matches with a missing pattern fails clearly instead of matching every string via new RegExp(undefined)", async () => { + const result = await evaluateCliAssertion( + { stderr_matches: undefined } as unknown as CliAssertion, + lastStep({ stderr: "anything at all" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toBe('Missing "stderr_matches" pattern'); + }); +}); + +describe("json_output", () => { + it("passes when the dot-path value equals the expected value", async () => { + const result = await evaluateCliAssertion( + { json_output: { path: "$.a.b", equals: 2 } }, + lastStep({ stdout: '{"a":{"b":2}}' }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeUndefined(); + }); + + it("fails naming both the expected and actual values when they differ", async () => { + const result = await evaluateCliAssertion( + { json_output: { path: "$.a.b", equals: 3 } }, + lastStep({ stdout: '{"a":{"b":2}}' }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toContain("3"); + expect(result?.message).toContain("2"); + }); + + it("fails with the parse message and a bounded head of stdout when stdout is not JSON, without crashing", async () => { + const result = await evaluateCliAssertion( + { json_output: { path: "$.a", equals: 1 } }, + lastStep({ stdout: "not valid json {{{" }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.message).toContain("not valid json {{{"); + }); + + it("evaluates exactly once with no retry, even when a timeout is configured", async () => { + const start = Date.now(); + const result = await evaluateCliAssertion( + { json_output: { path: "$.a.b", equals: 3 } }, + lastStep({ stdout: '{"a":{"b":2}}' }), + "/tmp/irrelevant", + 5000, + ); + const elapsed = Date.now() - start; + expect(result).toBeDefined(); + expect(elapsed).toBeLessThan(POLL_INTERVAL); + }); + + it("delegates to the real matchJsonPath primitive", async () => { + const spy = vi.spyOn(MatchersModule, "matchJsonPath"); + await evaluateCliAssertion( + { json_output: { path: "$.a.b", equals: 2 } }, + lastStep({ stdout: '{"a":{"b":2}}' }), + "/tmp/irrelevant", + 0, + ); + expect(spy).toHaveBeenCalled(); + spy.mockRestore(); + }); +}); + +describe("file_exists", () => { + it("resolves a relative path against workdir and passes when the file is already present", async () => { + const workdir = makeTempDir(); + writeFileSync(join(workdir, "output.txt"), "content"); + + const result = await evaluateCliAssertion( + { file_exists: "output.txt" }, + lastStep(), + workdir, + 0, + ); + expect(result).toBeUndefined(); + }); + + it("retries within the timeout window and passes once the file appears mid-window", async () => { + const workdir = makeTempDir(); + const filePath = join(workdir, "appears-later.txt"); + + const resultPromise = evaluateCliAssertion( + { file_exists: "appears-later.txt" }, + lastStep(), + workdir, + 1000, + ); + setTimeout(() => writeFileSync(filePath, "now it exists"), 300); + const result = await resultPromise; + + expect(result).toBeUndefined(); + }); + + it("fails after the deadline naming the resolved absolute path when the file never appears", async () => { + const workdir = makeTempDir(); + + const result = await evaluateCliAssertion( + { file_exists: "never-appears.txt" }, + lastStep(), + workdir, + POLL_INTERVAL + 50, + ); + + expect(result).toBeDefined(); + expect(result?.message).toContain(join(workdir, "never-appears.txt")); + }); + + it("delegates to the real fileExists primitive", async () => { + const workdir = makeTempDir(); + writeFileSync(join(workdir, "output.txt"), "content"); + const spy = vi.spyOn(FileMatchersModule, "fileExists"); + + await evaluateCliAssertion( + { file_exists: "output.txt" }, + lastStep(), + workdir, + 0, + ); + + expect(spy).toHaveBeenCalled(); + spy.mockRestore(); + }); +}); + +describe("file_contains", () => { + it("resolves a relative path against workdir and passes when the file already contains the expected text", async () => { + const workdir = makeTempDir(); + writeFileSync(join(workdir, "output.txt"), "the needle is here"); + + const result = await evaluateCliAssertion( + { file_contains: { path: "output.txt", text: "needle" } }, + lastStep(), + workdir, + 0, + ); + expect(result).toBeUndefined(); + }); + + it("fails after the deadline naming the resolved absolute path when the file never appears", async () => { + const workdir = makeTempDir(); + + const result = await evaluateCliAssertion( + { file_contains: { path: "never-appears.txt", text: "needle" } }, + lastStep(), + workdir, + POLL_INTERVAL + 50, + ); + + expect(result).toBeDefined(); + expect(result?.message).toContain(join(workdir, "never-appears.txt")); + }); + + it("delegates to the real fileContains primitive", async () => { + const workdir = makeTempDir(); + writeFileSync(join(workdir, "output.txt"), "the needle is here"); + const spy = vi.spyOn(FileMatchersModule, "fileContains"); + + await evaluateCliAssertion( + { file_contains: { path: "output.txt", text: "needle" } }, + lastStep(), + workdir, + 0, + ); + + expect(spy).toHaveBeenCalled(); + spy.mockRestore(); + }); +}); + +describe("file assertion paths are confined to the workdir", () => { + // The per-flow temp workdir is the whole point of CLI isolation: a path + // that resolves outside it must be REJECTED as out-of-bounds, not + // silently answered by whatever happens to exist elsewhere on disk. + it("rejects an absolute file_exists path pointing outside the workdir, even though that path exists", async () => { + const workdir = makeTempDir(); + const outside = makeTempDir(); + const outsidePath = join(outside, "real-file.txt"); + writeFileSync(outsidePath, "this really does exist"); + + const result = await evaluateCliAssertion( + { file_exists: outsidePath }, + lastStep(), + workdir, + 0, + ); + + expect(result).toBeDefined(); + expect(result?.message).toContain(outsidePath); + expect(result?.message).toContain(workdir); + expect(result?.message.toLowerCase()).toContain("outside"); + }); + + it("rejects a ../ traversal file_exists path, even though the traversed-to directory exists", async () => { + const workdir = makeTempDir(); + + const result = await evaluateCliAssertion( + { file_exists: "../" }, + lastStep(), + workdir, + 0, + ); + + expect(result).toBeDefined(); + expect(result?.message.toLowerCase()).toContain("outside"); + }); + + it("rejects an absolute file_contains path pointing outside the workdir", async () => { + const workdir = makeTempDir(); + const outside = makeTempDir(); + const outsidePath = join(outside, "real-file.txt"); + writeFileSync(outsidePath, "the needle is here"); + + const result = await evaluateCliAssertion( + { file_contains: { path: outsidePath, text: "needle" } }, + lastStep(), + workdir, + 0, + ); + + expect(result).toBeDefined(); + expect(result?.message).toContain(outsidePath); + expect(result?.message.toLowerCase()).toContain("outside"); + }); + + it("rejects a ../ traversal file_contains path that would otherwise match", async () => { + const workdir = makeTempDir(); + const parentFile = join(workdir, "..", "flowspec-traversal-target.txt"); + writeFileSync(parentFile, "the needle is here"); + try { + const result = await evaluateCliAssertion( + { + file_contains: { + path: "../flowspec-traversal-target.txt", + text: "needle", + }, + }, + lastStep(), + workdir, + 0, + ); + + expect(result).toBeDefined(); + expect(result?.message.toLowerCase()).toContain("outside"); + } finally { + rmSync(parentFile, { force: true }); + } + }); + + it("still accepts a nested relative path that stays inside the workdir", async () => { + const workdir = makeTempDir(); + mkdirSync(join(workdir, "nested")); + writeFileSync(join(workdir, "nested", "output.txt"), "the needle is here"); + + expect( + await evaluateCliAssertion( + { file_exists: "nested/output.txt" }, + lastStep(), + workdir, + 0, + ), + ).toBeUndefined(); + expect( + await evaluateCliAssertion( + { file_contains: { path: "nested/output.txt", text: "needle" } }, + lastStep(), + workdir, + 0, + ), + ).toBeUndefined(); + }); + + it("does not reject a sibling directory whose name merely shares the workdir's prefix", async () => { + // Guards the naive `resolved.startsWith(workdir)` prefix check: + // "/tmp/wd-extra/x" starts with "/tmp/wd" as a string but is NOT under + // it as a path. + const workdir = makeTempDir(); + const sibling = `${workdir}-extra`; + mkdirSync(sibling); + tempDirs.push(sibling); + writeFileSync(join(sibling, "output.txt"), "content"); + + const result = await evaluateCliAssertion( + { file_exists: join(sibling, "output.txt") }, + lastStep(), + workdir, + 0, + ); + + expect(result).toBeDefined(); + expect(result?.message.toLowerCase()).toContain("outside"); + }); + + // Regression coverage for the symlink-traversal fix: `resolve()` alone + // does not follow symlinks, so a symlink planted INSIDE the workdir that + // points somewhere else can resolve, post-symlink, outside the workdir + // while still passing a naive string-prefix check against the + // pre-symlink resolved path. See resolveWithinWorkdir's comment in + // src/cli-assertions.ts. + it("rejects a file_exists path traversing a symlink inside the workdir that points outside it", async () => { + const workdir = makeTempDir(); + const outside = makeTempDir(); + writeFileSync(join(outside, "secret.txt"), "top secret"); + symlinkSync(outside, join(workdir, "linked")); + + const result = await evaluateCliAssertion( + { file_exists: "linked/secret.txt" }, + lastStep(), + workdir, + 0, + ); + + expect(result).toBeDefined(); + expect(result?.message.toLowerCase()).toContain("outside"); + }); + + it("rejects a file_contains path traversing a symlink inside the workdir that points outside it", async () => { + const workdir = makeTempDir(); + const outside = makeTempDir(); + writeFileSync(join(outside, "secret.txt"), "the needle is here"); + symlinkSync(outside, join(workdir, "linked")); + + const result = await evaluateCliAssertion( + { file_contains: { path: "linked/secret.txt", text: "needle" } }, + lastStep(), + workdir, + 0, + ); + + expect(result).toBeDefined(); + expect(result?.message.toLowerCase()).toContain("outside"); + }); + + // Regression coverage for the follow-up CodeRabbit finding on the fix + // above: realpathDeepestExisting originally walked upward only on ENOENT + // and returned the UNRESOLVED path on any other error, which failed open + // — that path is the lexically-resolved one the caller already confirmed + // sits under the root, so containment accepted it having canonicalized + // nothing at all. + // + // This is the concrete bypass, and it needs no special permissions to + // reproduce: "linked" is a symlink out of the workdir and "blocker" is a + // regular FILE, so realpathSync on "linked/blocker/target.txt" fails with + // ENOTDIR rather than ENOENT — and the symlink is never resolved or + // noticed. (An unreadable intermediate directory hides a symlink the same + // way via EACCES, but that variant can't be tested as root.) + it("rejects a path whose symlink escape is hidden behind a non-ENOENT realpath failure", async () => { + const workdir = makeTempDir(); + const outside = makeTempDir(); + // A regular file, not a directory: traversing THROUGH it yields ENOTDIR. + writeFileSync(join(outside, "blocker"), "i am a regular file"); + symlinkSync(outside, join(workdir, "linked")); + + const result = await evaluateCliAssertion( + { file_exists: "linked/blocker/target.txt" }, + lastStep(), + workdir, + 0, + ); + + expect(result).toBeDefined(); + expect(result?.message.toLowerCase()).toContain("outside"); + }); + + // Regression guard: the symlink-traversal fix must not break the + // retry-for-a-not-yet-created-file contract. realpathSync throws ENOENT + // on a path that doesn't exist yet, so a naive fix that realpath'd the + // full requested path would misreport every not-yet-created file as an + // "outside workdir" rejection instead of the normal "expected file to + // exist" failure. A file that never appears, with no symlink involved at + // all, must still fail with the ordinary missing-file message. + it("still reports the normal 'file does not exist' failure (not an outside-workdir rejection) for a plain not-yet-created file", async () => { + const workdir = makeTempDir(); + + const result = await evaluateCliAssertion( + { file_exists: "not-created-yet.txt" }, + lastStep(), + workdir, + 0, + ); + + expect(result).toBeDefined(); + expect(result?.message.toLowerCase()).not.toContain("outside"); + expect(result?.message).toContain("Expected file to exist"); + }); + + // Regression guard for the same retry contract via file_exists's actual + // polling path (not just the zero-timeout case above): a file created + // mid-window, reached through no symlink at all, must still be found — + // proving the fix didn't accidentally realpath the not-yet-existing + // target and throw ENOENT partway through a poll. + it("still retries and passes once a plain (non-symlinked) file appears mid-window", async () => { + const workdir = makeTempDir(); + const filePath = join(workdir, "appears-later-again.txt"); + + const resultPromise = evaluateCliAssertion( + { file_exists: "appears-later-again.txt" }, + lastStep(), + workdir, + 1000, + ); + setTimeout(() => writeFileSync(filePath, "now it exists"), 300); + const result = await resultPromise; + + expect(result).toBeUndefined(); + }); + + // Regression guard for macOS-style symlinked tmp dirs (e.g. /tmp -> + // /private/tmp): a workdir that is itself reached only through a + // symlink must still accept its own files, since realpathSync on the + // workdir root is what makes the two sides of the containment + // comparison agree. + it("accepts a file inside a workdir that is itself reached only through a symlink", async () => { + const realDir = mkdtempSync(join(tmpdir(), "flowspec-cli-assertions-")); + tempDirs.push(realDir); + const symlinkedWorkdir = `${realDir}-symlinked`; + symlinkSync(realDir, symlinkedWorkdir); + tempDirs.push(symlinkedWorkdir); + writeFileSync(join(realDir, "output.txt"), "content"); + + const result = await evaluateCliAssertion( + { file_exists: "output.txt" }, + lastStep(), + symlinkedWorkdir, + 0, + ); + + expect(result).toBeUndefined(); + }); +}); + +describe("every failure carries the last step's exit code, bounded stdout/stderr excerpts, and workdir", () => { + it("an exit_code failure carries stdout, stderr, and workdir alongside exitCode", async () => { + const result = await evaluateCliAssertion( + { exit_code: 0 }, + lastStep({ exitCode: 1, stdout: "out text", stderr: "err text" }), + "/tmp/some-workdir", + 0, + ); + expect(result?.exitCode).toBe(1); + expect(result?.stdout).toBe("out text"); + expect(result?.stderr).toBe("err text"); + expect(result?.workdir).toBe("/tmp/some-workdir"); + }); + + it("a stdout_contains failure ALSO carries the last step's exit code and workdir, not just its own excerpt", async () => { + const result = await evaluateCliAssertion( + { stdout_contains: "missing" }, + lastStep({ exitCode: 2, stdout: "present output", stderr: "" }), + "/tmp/another-workdir", + 0, + ); + expect(result?.exitCode).toBe(2); + expect(result?.workdir).toBe("/tmp/another-workdir"); + }); + + it("a file_exists failure ALSO carries the last step's exitCode, stdout, and stderr, not just the file failure info", async () => { + const workdir = makeTempDir(); + + const result = await evaluateCliAssertion( + { file_exists: "never-appears.txt" }, + lastStep({ exitCode: 0, stdout: "step output", stderr: "step stderr" }), + workdir, + 0, + ); + + expect(result?.exitCode).toBe(0); + expect(result?.stdout).toBe("step output"); + expect(result?.stderr).toBe("step stderr"); + expect(result?.workdir).toBe(workdir); + }); + + it("bounds a long stdout excerpt to EXCERPT_LIMIT rather than attaching the full raw text", async () => { + const longStdout = "x".repeat(EXCERPT_LIMIT * 5); + const result = await evaluateCliAssertion( + { exit_code: 0 }, + lastStep({ exitCode: 1, stdout: longStdout }), + "/tmp/irrelevant", + 0, + ); + expect(result).toBeDefined(); + expect(result?.stdout.length).toBeLessThan(longStdout.length); + }); +}); diff --git a/test/cli-runner-setup.test.ts b/test/cli-runner-setup.test.ts new file mode 100644 index 0000000..dec23b5 --- /dev/null +++ b/test/cli-runner-setup.test.ts @@ -0,0 +1,255 @@ +import { + existsSync, + mkdtempSync, + readFileSync, + realpathSync, + rmSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { runCliFlow } from "../src/cli-runner"; +import type { CliFlowSpec } from "../src/types"; + +/** + * Tests for WI-811: the CLI runner's flow-level setup phase — a `surface: + * cli` flow's own `setup` block of CLI steps, run before its steps, in the + * same working directory, with PRD-0006 phase-labeled error reporting. + * + * Contract pinned by the work item: + * - Setup steps run in declaration order, in the SAME working directory + * as the flow's own steps (a file setup writes is visible to step 0). + * - A failing setup step fails the flow with `error.phase === "setup"`, + * `error.step` set to THAT STEP'S OWN INDEX within the setup array + * (local, 0-based — mirrors src/runner.ts's web setupIndex), and + * `error.stderr` carrying that step's stderr. None of the flow's own + * steps run afterward. + * - Setup steps follow the SAME exit-code contract as a NON-FINAL step — + * including the LAST setup step. Unlike the main steps phase (where the + * final step's bare exit code is never fatal), setup has no equivalent + * "last step" leniency: every setup step's exit code must match its + * expect_exit (default 0) or the setup phase fails, even for a + * single-step setup array. + * - Config-level (web) setup is never applied to a CLI flow. runCliFlow's + * options type (CliRunOptions) has no `setup` field at all — verified + * defensively by forwarding one anyway (bypassing the type system, as + * a future wiring bug plausibly could) and confirming it's ignored. + * + * Scope boundary (like WI-808's own tests): the "does not abort the run" + * criterion describes EXISTING, unchanged logic in src/index.ts's + * (unexported, not directly testable) runFlows — it aborts only when + * `result.error?.phase === "setup" && flow.setup === undefined`. Since the + * CLI dispatch item (wiring runCliFlow into runFlows) doesn't exist yet, + * this can't be exercised end-to-end via the real CLI binary the way + * test/cli-setup-abort.test.ts does for the web surface. Instead, this + * file reconstructs that exact (unexported) condition inline against a + * REAL runCliFlow result, proving the two are compatible: a CLI flow's own + * setup failure always has `flow.setup !== undefined` (it only fails in + * setup because it HAD one), so the existing abort condition already + * evaluates to false for it — nothing here needs to change once dispatch + * wires the two together. + */ + +const execPath = process.execPath; +const ownedDirs: string[] = []; + +function ownedTempDir(): string { + const dir = realpathSync( + mkdtempSync(join(tmpdir(), "flowspec-cli-runner-setup-")), + ); + ownedDirs.push(dir); + return dir; +} + +afterEach(() => { + for (const dir of ownedDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +function keepFailedWorkdir(result: { error?: { workdir?: string } }): string { + const workdir = result.error?.workdir; + expect(workdir).toBeDefined(); + ownedDirs.push(workdir as string); + return workdir as string; +} + +function cliFlow( + steps: Record[], + overrides: Record = {}, +): CliFlowSpec { + return { + name: "cli-flow", + description: "A cli flow", + surface: "cli", + steps, + expect: [], + ...overrides, + } as unknown as CliFlowSpec; +} + +describe("setup runs before steps, sharing the same working directory", () => { + it("runs setup steps before the flow's own steps, and a file setup writes is visible to step 0", async () => { + const cwd = ownedTempDir(); + const flow = cliFlow( + [ + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('from-step.txt', require('fs').readFileSync('from-setup.txt','utf-8'))", + ], + }, + ], + { + setup: [ + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('from-setup.txt', 'seeded-by-setup')", + ], + }, + ], + }, + ); + + const result = await runCliFlow(flow, { cwd }); + + expect(result.success).toBe(true); + expect(existsSync(join(cwd, "from-setup.txt"))).toBe(true); + expect(readFileSync(join(cwd, "from-step.txt"), "utf-8")).toBe( + "seeded-by-setup", + ); + }); +}); + +describe("a failing setup step fails the flow with phase: setup", () => { + it("fails naming phase setup, the setup step's own index, and its stderr — and no flow step runs", async () => { + const flow = cliFlow( + [ + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('should-not-exist.txt','x')", + ], + }, + ], + { + setup: [ + { run: [execPath, "-e", "process.exit(0)"] }, + { + run: [ + execPath, + "-e", + "process.stderr.write('boom from setup step 1');process.exit(1)", + ], + }, + ], + }, + ); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.phase).toBe("setup"); + expect(result.error?.step).toBe(1); + expect(result.error?.stderr).toContain("boom from setup step 1"); + const workdir = keepFailedWorkdir(result); + expect(existsSync(join(workdir, "should-not-exist.txt"))).toBe(false); + }); +}); + +describe("setup steps follow the same exit-code contract as non-final steps", () => { + it("continues the setup chain when a setup step's exit code matches its expect_exit", async () => { + const cwd = ownedTempDir(); + const flow = cliFlow( + [ + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('reached.txt','yes')", + ], + }, + ], + { + setup: [{ run: [execPath, "-e", "process.exit(1)"], expect_exit: 1 }], + }, + ); + + const result = await runCliFlow(flow, { cwd }); + + expect(result.success).toBe(true); + expect(existsSync(join(cwd, "reached.txt"))).toBe(true); + }); + + it("fails the setup phase when a setup step's exit code does not match its expect_exit", async () => { + const flow = cliFlow([{ run: [execPath, "-e", "process.exit(0)"] }], { + setup: [{ run: [execPath, "-e", "process.exit(2)"], expect_exit: 1 }], + }); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.phase).toBe("setup"); + expect(result.error?.exitCode).toBe(2); + keepFailedWorkdir(result); + }); + + it("has no 'final step' leniency: the LAST (and only) setup step still fails on a bare nonzero exit with no expect_exit", async () => { + // Unlike the main steps phase, where a lone/final step's bare exit code + // is never fatal by itself, setup has no assertion phase of its own — + // every setup step, including the last one in the array, is always + // "non-final" for exit-code purposes. + const flow = cliFlow([{ run: [execPath, "-e", "process.exit(0)"] }], { + setup: [{ run: [execPath, "-e", "process.exit(1)"] }], + }); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.phase).toBe("setup"); + expect(result.error?.step).toBe(0); + keepFailedWorkdir(result); + }); +}); + +describe("config-level (web) setup is never applied to a CLI flow", () => { + it("ignores a web setup option entirely, even if present on the options object", async () => { + const cwd = ownedTempDir(); + const flow = cliFlow([{ run: [execPath, "-e", "process.exit(0)"] }]); + // CliRunOptions has no `setup` field — cast past the type system to + // prove runCliFlow never reads a stray one, defending against a future + // dispatch-wiring mistake that forwards the config's web setup here. + const optionsWithWebSetup = { + cwd, + setup: [{ visit: "/should-never-be-touched" }], + } as unknown as Parameters[1]; + + const result = await runCliFlow(flow, optionsWithWebSetup); + + expect(result.success).toBe(true); + }); +}); + +describe("compatibility with index.ts's existing (unchanged) abort condition", () => { + it("a CLI flow's own setup failure always has flow.setup defined, so the existing abort-only-on-config-setup condition already evaluates to false for it", async () => { + const flow = cliFlow([{ run: [execPath, "-e", "process.exit(0)"] }], { + setup: [{ run: [execPath, "-e", "process.exit(1)"] }], + }); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.phase).toBe("setup"); + keepFailedWorkdir(result); + + // Mirrors src/index.ts's runFlows abort condition verbatim (not + // exported, so reconstructed here) — see file header for why. + const wouldAbortAsConfigSetupFailure = + result.error?.phase === "setup" && flow.setup === undefined; + expect(wouldAbortAsConfigSetupFailure).toBe(false); + }); +}); diff --git a/test/cli-runner.test.ts b/test/cli-runner.test.ts new file mode 100644 index 0000000..60a0d9e --- /dev/null +++ b/test/cli-runner.test.ts @@ -0,0 +1,453 @@ +import { + existsSync, + mkdtempSync, + readFileSync, + realpathSync, + rmSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { runCliFlow } from "../src/cli-runner"; +import { EXCERPT_LIMIT } from "../src/matchers"; +import type { CliFlowSpec } from "../src/types"; + +/** + * Tests for WI-808: the CLI execution core — runs a `surface: cli` flow's + * `run` steps in declaration order inside the flow's working directory, + * applying the PRD's fail-fast exit-code contract. + * + * Contract pinned by the work item: + * - runCliFlow(flow: FlowSpec, options?): Promise, options + * carrying { cwd?, timeout?, stepTimeout?, captureLimit? } (the merged + * config shape; stepTimeout was split out of timeout by the + * post-mission sweep, see test/cli-step-timeout.test.ts). + * - Phases in order: create workdir (src/workdir.ts) -> setup (now + * executed — see WI-811 and its dedicated coverage in + * test/cli-runner-setup.test.ts) -> steps (this item) -> + * assertion hook (a later item plugs in; every fixture here uses + * `expect: []` so this item's tests never depend on that wiring) -> + * dispose workdir keyed on pass/fail. + * - On step failure, `error` carries the CLI FlowError fields from + * WI-805: `step` (index), `action` (the failing CliStep), `exitCode`, + * `stdout`, `stderr`, `workdir`. + * + * Observability strategy (deliberate, see below): a PASSED flow's workdir + * is deleted (WI-804 dispose-on-pass), so pass-path tests that need to + * inspect a step's side effects (order, env, stdin, argv fidelity) pass an + * explicit `options.cwd` pointing at a test-owned temp dir, which + * createFlowWorkdir treats as "configured" and never deletes. Fail-path + * tests instead let runCliFlow create its own temp workdir and inspect it + * via `result.error.workdir` (which also doubles as a proof that failure + * keeps the directory and reports its path) — cleaned up manually in + * `afterEach` since a kept directory is not this suite's to leave behind. + * + * Every fixture uses `expect: []` so these tests exercise ONLY the steps + * phase this item implements, independent of whichever item wires the + * assertion-evaluation hook. These fixtures are built in code rather than + * parsed, so they bypass FlowSpecSchema — which, since the post-mission + * sweep (fix S7), requires a real CLI flow to declare at least one + * assertion. An empty list here just makes the assertion loop a no-op. + */ + +const execPath = process.execPath; + +const ownedDirs: string[] = []; + +function ownedTempDir(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "flowspec-cli-runner-"))); + ownedDirs.push(dir); + return dir; +} + +afterEach(() => { + for (const dir of ownedDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +/** Track a fail-path result's kept workdir for cleanup, and return it. */ +function keepFailedWorkdir(result: { error?: { workdir?: string } }): string { + const workdir = result.error?.workdir; + expect(workdir).toBeDefined(); + ownedDirs.push(workdir as string); + return workdir as string; +} + +function cliFlow( + steps: Record[], + overrides: Record = {}, +): CliFlowSpec { + return { + name: "cli-flow", + description: "A cli flow", + surface: "cli", + steps, + expect: [], + ...overrides, + } as unknown as CliFlowSpec; +} + +describe("step execution: order, env, stdin, argv fidelity", () => { + it("executes run steps in declaration order, inside the flow's working directory", async () => { + const cwd = ownedTempDir(); + const append = (label: string) => + `${execPath} -e require('fs').appendFileSync('order.log','${label}\\n')`; + const flow = cliFlow([ + { run: append("step0") }, + { run: append("step1") }, + { run: append("step2") }, + ]); + + const result = await runCliFlow(flow, { cwd }); + + expect(result.success).toBe(true); + const content = readFileSync(join(cwd, "order.log"), "utf-8"); + expect(content).toBe("step0\nstep1\nstep2\n"); + }); + + it("applies each step's own env overlay, without leaking it to a sibling step", async () => { + const cwd = ownedTempDir(); + const envScript = + "require('fs').appendFileSync('env.log',(process.env.MARKER||'ABSENT')+'\\n')"; + const flow = cliFlow([ + { run: [execPath, "-e", envScript], env: { MARKER: "hello" } }, + { run: [execPath, "-e", envScript] }, + ]); + + const result = await runCliFlow(flow, { cwd }); + + expect(result.success).toBe(true); + const content = readFileSync(join(cwd, "env.log"), "utf-8"); + expect(content).toBe("hello\nABSENT\n"); + }); + + it("writes each step's own stdin to the child and closes the stream", async () => { + const cwd = ownedTempDir(); + const stdinScript = + "let d='';process.stdin.setEncoding('utf8');process.stdin.on('data',c=>{d+=c});process.stdin.on('end',()=>{require('fs').writeFileSync('stdin.log',d)})"; + const flow = cliFlow([ + { run: [execPath, "-e", stdinScript], stdin: "stdin-content" }, + ]); + + const result = await runCliFlow(flow, { cwd }); + + expect(result.success).toBe(true); + const content = readFileSync(join(cwd, "stdin.log"), "utf-8"); + expect(content).toBe("stdin-content"); + }); + + it("passes an array-form element containing a space through untouched, as one argv entry", async () => { + const cwd = ownedTempDir(); + const flow = cliFlow([ + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('arrayform.log', process.argv.at(-1))", + "two words", + ], + }, + ]); + + const result = await runCliFlow(flow, { cwd }); + + expect(result.success).toBe(true); + const content = readFileSync(join(cwd, "arrayform.log"), "utf-8"); + expect(content).toBe("two words"); + }); + + it("splits string-form run on whitespace only, passing shell metacharacters through literally", async () => { + const cwd = ownedTempDir(); + const flow = cliFlow([ + { + run: `${execPath} -e require('fs').writeFileSync('metachar.log','a&&b')`, + }, + ]); + + const result = await runCliFlow(flow, { cwd }); + + expect(result.success).toBe(true); + const content = readFileSync(join(cwd, "metachar.log"), "utf-8"); + expect(content).toBe("a&&b"); + }); + + it("does not shell-parse a quoted substring in string form back into one argv element", async () => { + // A naive whitespace split of `-e "process.exit(process.argv.length)" "two words"` + // yields 5 tokens with the quote characters embedded literally, which + // makes the -e argument a syntactically valid but INERT string-literal + // statement (verified interactively) — so the step exits 0 naturally. + // A shell-aware parser would instead strip the quotes and produce a + // working script + one reassembled "two words" argument, exiting with + // some other (nonzero) code from process.argv.length. Asserting the + // flow PASSES (expect_exit: 0 matched) pins the naive-split behavior. + const cwd = ownedTempDir(); + const flow = cliFlow([ + { + run: `${execPath} -e "process.exit(process.argv.length)" "two words"`, + expect_exit: 0, + }, + { run: [execPath, "-e", "process.exit(0)"] }, + ]); + + const result = await runCliFlow(flow, { cwd }); + + expect(result.success).toBe(true); + }); +}); + +describe("fail-fast: non-final step, no expect_exit", () => { + it("fails the flow at that step index, carrying the step's stderr, and never runs later steps", async () => { + const flow = cliFlow([ + { + run: [ + execPath, + "-e", + "process.stderr.write('boom from step0');process.exit(1)", + ], + }, + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('should-not-exist.txt','x')", + ], + }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.step).toBe(0); + expect(result.error?.stderr).toContain("boom from step0"); + const workdir = keepFailedWorkdir(result); + expect(existsSync(join(workdir, "should-not-exist.txt"))).toBe(false); + }); +}); + +describe("step-failure output is bounded the same way assertion-failure output is", () => { + // The excerpt bound the assertion path already applies (src/matchers.ts's + // EXCERPT_LIMIT) has to apply here too: without it a failing step dumps + // its entire captured stream — up to the multi-megabyte per-stream + // capture cap — straight to the terminal. + const MAX_EXCERPT = EXCERPT_LIMIT + "[truncated]".length; + + it("bounds a failing step's stdout and stderr to an excerpt", async () => { + const flow = cliFlow([ + { + run: [ + execPath, + "-e", + `process.stdout.write('o'.repeat(${EXCERPT_LIMIT * 20}));process.stderr.write('e'.repeat(${EXCERPT_LIMIT * 20}));process.exit(1)`, + ], + }, + { run: [execPath, "-e", "process.exit(0)"] }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.stdout?.length).toBeLessThanOrEqual(MAX_EXCERPT); + expect(result.error?.stderr?.length).toBeLessThanOrEqual(MAX_EXCERPT); + keepFailedWorkdir(result); + }); + + it("bounds stdout and stderr on an expect_exit mismatch too", async () => { + const flow = cliFlow([ + { + run: [ + execPath, + "-e", + `process.stdout.write('o'.repeat(${EXCERPT_LIMIT * 20}));process.exit(2)`, + ], + expect_exit: 1, + }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.exitCode).toBe(2); + expect(result.error?.stdout?.length).toBeLessThanOrEqual(MAX_EXCERPT); + keepFailedWorkdir(result); + }); + + it("bounds the stderr interpolated into a timeout failure's message", async () => { + const flow = cliFlow([ + { + run: [ + execPath, + "-e", + `process.stderr.write('e'.repeat(${EXCERPT_LIMIT * 20}));setTimeout(()=>{},10000)`, + ], + timeout: 500, + }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.message.toLowerCase()).toContain("timed out"); + expect(result.error?.message.length).toBeLessThanOrEqual(MAX_EXCERPT + 200); + expect(result.error?.stderr?.length).toBeLessThanOrEqual(MAX_EXCERPT); + keepFailedWorkdir(result); + }); + + it("leaves output already under the excerpt limit untouched", async () => { + const flow = cliFlow([ + { + run: [ + execPath, + "-e", + "process.stderr.write('boom from step0');process.exit(1)", + ], + }, + { run: [execPath, "-e", "process.exit(0)"] }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.stderr).toBe("boom from step0"); + keepFailedWorkdir(result); + }); +}); + +describe("fail-fast: non-final step with expect_exit", () => { + it("continues the chain when the exit code matches expect_exit", async () => { + const cwd = ownedTempDir(); + const flow = cliFlow([ + { run: [execPath, "-e", "process.exit(1)"], expect_exit: 1 }, + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('reached.txt','yes')", + ], + }, + ]); + + const result = await runCliFlow(flow, { cwd }); + + expect(result.success).toBe(true); + expect(existsSync(join(cwd, "reached.txt"))).toBe(true); + }); + + it("fails the flow at that step when the exit code does not match expect_exit", async () => { + const flow = cliFlow([ + { run: [execPath, "-e", "process.exit(2)"], expect_exit: 1 }, + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('should-not-exist.txt','x')", + ], + }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.step).toBe(0); + expect(result.error?.exitCode).toBe(2); + const workdir = keepFailedWorkdir(result); + expect(existsSync(join(workdir, "should-not-exist.txt"))).toBe(false); + }); +}); + +describe("final step: bare exit code never fails the flow", () => { + it("completes the step phase successfully when the last step exits nonzero and declares no expect_exit", async () => { + const flow = cliFlow([{ run: [execPath, "-e", "process.exit(1)"] }]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(true); + }); +}); + +describe("final step: expect_exit applies uniformly", () => { + it("proceeds to assertions when the final step's exit code matches its expect_exit", async () => { + const flow = cliFlow([ + { run: [execPath, "-e", "process.exit(1)"], expect_exit: 1 }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(true); + }); + + it("fails the flow at the final step when its exit code does not match its expect_exit", async () => { + const flow = cliFlow([ + { run: [execPath, "-e", "process.exit(0)"], expect_exit: 1 }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.step).toBe(0); + expect(result.error?.exitCode).toBe(0); + keepFailedWorkdir(result); + }); +}); + +describe("timeout and spawn failure stop the flow before assertions", () => { + it("fails the flow as a timeout at the step that exceeded it, and never runs a later step", async () => { + const flow = cliFlow([ + { run: [execPath, "-e", "setTimeout(()=>{},10000)"], timeout: 300 }, + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('should-not-exist.txt','x')", + ], + }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.step).toBe(0); + expect(result.error?.message.toLowerCase()).toContain("timed out"); + const workdir = keepFailedWorkdir(result); + expect(existsSync(join(workdir, "should-not-exist.txt"))).toBe(false); + }); + + it("honors options.stepTimeout as the fallback when a step declares no timeout of its own", async () => { + // Re-pointed at `stepTimeout` by the post-mission sweep (fix M2). The + // fallback contract is unchanged — only the key that carries it. It used + // to be `options.timeout`, which is the ASSERTION-RETRY budget: any + // command outliving a retry window (an install, a build, a fetch) was + // killed and reported as a timeout. See test/cli-step-timeout.test.ts. + const flow = cliFlow([ + { run: [execPath, "-e", "setTimeout(()=>{},10000)"] }, + ]); + + const result = await runCliFlow(flow, { stepTimeout: 300 }); + + expect(result.success).toBe(false); + expect(result.error?.message.toLowerCase()).toContain("timed out"); + keepFailedWorkdir(result); + }); + + it("fails the flow with an error naming the command when it cannot be spawned, and never runs a later step", async () => { + const missingCommand = "flowspec-definitely-not-a-real-binary-xyz"; + const flow = cliFlow([ + { run: [missingCommand] }, + { + run: [ + execPath, + "-e", + "require('fs').writeFileSync('should-not-exist.txt','x')", + ], + }, + ]); + + const result = await runCliFlow(flow, {}); + + expect(result.success).toBe(false); + expect(result.error?.step).toBe(0); + expect(result.error?.message).toContain(missingCommand); + const workdir = keepFailedWorkdir(result); + expect(existsSync(join(workdir, "should-not-exist.txt"))).toBe(false); + }); +}); diff --git a/test/cli-step-timeout.test.ts b/test/cli-step-timeout.test.ts new file mode 100644 index 0000000..6150563 --- /dev/null +++ b/test/cli-step-timeout.test.ts @@ -0,0 +1,313 @@ +/** + * Post-mission sweep fix M2: the CLI step process-kill deadline is its OWN + * config/CLI key (`stepTimeout` / `--step-timeout`), separate from the + * assertion-retry budget (`timeout` / `--timeout`). + * + * Two defects this file pins: + * 1. One `timeout` value used to mean both "how long to keep re-checking an + * assertion" (web-harmless, defaults to 10000ms) and "how long before a + * run step's process is killed" (CLI-fatal). Any real command that takes + * longer than the assertion budget — an install, a build, a network + * fetch — was silently killed and reported as "timed out". + * 2. `--timeout 0` reached the spawn primitive unvalidated (mergeConfig uses + * `??`, so a CLI-supplied 0 bypassed the config schema's `.positive()`), + * arming a zero-delay kill. The kill deadline now has its own flag, and + * that flag rejects 0/negative/non-integer values with exit 2 before any + * flow runs. + * + * The assertion-retry `timeout` key keeps its existing meaning and its + * existing tolerance of 0 ("no retries"), which is why the flag-level + * validation lives on the new key only. + */ + +import { spawn } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { runCliFlow } from "../src/cli-runner"; +import { + CONFIG_FILE_NAME, + DEFAULT_CONFIG, + DEFAULT_STEP_TIMEOUT, + loadConfigFile, + mergeConfig, +} from "../src/config"; +import { runFlow } from "../src/runner"; +import type { CliFlowSpec } from "../src/types"; + +const CLI_PATH = join(__dirname, "..", "src", "index.ts"); +const execPath = process.execPath; + +const ownedDirs: string[] = []; + +function ownedTempDir(): string { + const dir = mkdtempSync(join(tmpdir(), "flowspec-step-timeout-")); + ownedDirs.push(dir); + return dir; +} + +afterEach(() => { + for (const dir of ownedDirs.splice(0)) { + rmSync(dir, { recursive: true, force: true }); + } +}); + +function cliFlow(steps: Record[]): CliFlowSpec { + return { + name: "cli-flow", + description: "A cli flow", + surface: "cli", + steps, + expect: [{ exit_code: 0 }], + } as unknown as CliFlowSpec; +} + +/** A run step that stays alive for `ms` and then exits 0. */ +function sleepStep(ms: number): Record { + return { run: [execPath, "-e", `setTimeout(()=>{},${ms})`] }; +} + +function writeConfig(dir: string, contents: string): string { + const configPath = join(dir, CONFIG_FILE_NAME); + writeFileSync(configPath, contents); + return configPath; +} + +async function runCLI( + args: string[], + cwd: string, +): Promise<{ stdout: string; stderr: string; exitCode: number }> { + return new Promise((resolve) => { + const child = spawn("bun", ["run", CLI_PATH, ...args], { + cwd, + timeout: 20000, + }); + + let stdout = ""; + let stderr = ""; + child.stdout?.on("data", (data) => { + stdout += data.toString(); + }); + child.stderr?.on("data", (data) => { + stderr += data.toString(); + }); + child.on("close", (code) => { + resolve({ stdout, stderr, exitCode: code ?? 1 }); + }); + child.on("error", (error) => { + resolve({ stdout, stderr: stderr + error.message, exitCode: 1 }); + }); + }); +} + +describe("config: stepTimeout is a distinct key from timeout", () => { + it("defaults stepTimeout to a realistic process deadline, well past the assertion budget", () => { + const dir = ownedTempDir(); + const configPath = writeConfig(dir, "baseUrl: http://custom.com\n"); + + const config = loadConfigFile(configPath); + + // The assertion-retry budget keeps its own, unchanged default... + expect(config.timeout).toBe(10000); + // ...while the process-kill deadline is its own key with a default that + // does not kill an install/build/network fetch mid-flight. + expect(config.stepTimeout).toBe(DEFAULT_STEP_TIMEOUT); + expect(DEFAULT_STEP_TIMEOUT).toBeGreaterThanOrEqual(60000); + expect(DEFAULT_CONFIG.stepTimeout).toBe(DEFAULT_STEP_TIMEOUT); + }); + + it("loads an explicit stepTimeout without disturbing timeout", () => { + const dir = ownedTempDir(); + const configPath = writeConfig(dir, "timeout: 2000\nstepTimeout: 120000\n"); + + const config = loadConfigFile(configPath); + + expect(config.timeout).toBe(2000); + expect(config.stepTimeout).toBe(120000); + }); + + it.each([ + ["zero", "0"], + ["negative", "-1"], + ["fractional", "1500.5"], + ["non-numeric", '"soon"'], + ])("rejects a %s stepTimeout, naming the field", (_label, value) => { + const dir = ownedTempDir(); + const configPath = writeConfig(dir, `stepTimeout: ${value}\n`); + + expect(() => loadConfigFile(configPath)).toThrow(/invalid configuration/i); + try { + loadConfigFile(configPath); + throw new Error("expected loadConfigFile to throw"); + } catch (error) { + expect((error as Error).message).toContain("stepTimeout"); + } + }); + + it("carries stepTimeout through mergeConfig, with a CLI value winning over the config value", () => { + const config = { + baseUrl: "http://config.com", + timeout: 10000, + stepTimeout: 60000, + specsDir: "specs/", + }; + + expect(mergeConfig(config, {}).stepTimeout).toBe(60000); + expect(mergeConfig(config, { stepTimeout: 90000 }).stepTimeout).toBe(90000); + // The two keys stay independent through the merge. + expect(mergeConfig(config, { stepTimeout: 90000 }).timeout).toBe(10000); + expect(mergeConfig(config, { timeout: 250 }).stepTimeout).toBe(60000); + }); +}); + +describe("--step-timeout flag validation", () => { + function projectWithCliFlow(): string { + const dir = ownedTempDir(); + writeFileSync( + join(dir, "quick.flow.yaml"), + `name: quick-cli-flow +description: exits immediately +surface: cli +steps: + - run: ["${execPath}", "-e", "process.exit(0)"] +expect: + - exit_code: 0 +`, + ); + return dir; + } + + it.each([ + ["zero", "0"], + ["negative", "-5"], + ["non-numeric", "soon"], + ["fractional", "1500.5"], + ])( + "rejects --step-timeout %s with exit code 2 and a message naming the flag", + async (_label, value) => { + const dir = projectWithCliFlow(); + + const result = await runCLI( + ["run", join(dir, "quick.flow.yaml"), "--step-timeout", value], + dir, + ); + + expect(result.exitCode).toBe(2); + expect(result.stderr).toContain("--step-timeout"); + }, + 20000, + ); + + it("rejects --step-timeout with no value at all", async () => { + const dir = projectWithCliFlow(); + + const result = await runCLI( + ["run", join(dir, "quick.flow.yaml"), "--step-timeout"], + dir, + ); + + expect(result.exitCode).toBe(2); + expect(result.stderr).toContain("--step-timeout"); + }, 20000); + + it("accepts a positive --step-timeout and runs the flow", async () => { + const dir = projectWithCliFlow(); + + const result = await runCLI( + ["run", join(dir, "quick.flow.yaml"), "--step-timeout", "90000"], + dir, + ); + + expect(result.exitCode).toBe(0); + }, 20000); + + it("still accepts --timeout 0: the assertion-retry budget is unchanged by this split", async () => { + const dir = projectWithCliFlow(); + + const result = await runCLI( + ["run", join(dir, "quick.flow.yaml"), "--timeout", "0"], + dir, + ); + + expect(result.exitCode).toBe(0); + expect(result.stderr).not.toContain("--timeout"); + }, 20000); + + it("documents --step-timeout in the help output", async () => { + const dir = ownedTempDir(); + + const result = await runCLI(["run", "--help"], dir); + + const output = result.stdout + result.stderr; + expect(output).toContain("--step-timeout"); + }, 20000); +}); + +describe("the assertion-retry budget no longer kills a run step", () => { + it("does not kill a step that outlives the assertion-retry timeout", async () => { + const cwd = ownedTempDir(); + // 300ms assertion budget, a step that lives ~1s: under the old + // single-key behavior this was killed at 300ms and reported as a + // timeout. + const result = await runCliFlow(cliFlow([sleepStep(1000)]), { + cwd, + timeout: 300, + }); + + expect(result.error?.message ?? "").not.toContain("timed out"); + expect(result.success).toBe(true); + }, 20000); + + it("does not kill a step that runs longer than the OLD 10s default deadline", async () => { + const cwd = ownedTempDir(); + // The config default assertion budget (10000ms) used to double as the + // process-kill deadline, so an 11s command was killed mid-flight. With + // no stepTimeout supplied the new default (>= 60s) applies instead. + const result = await runCliFlow(cliFlow([sleepStep(11000)]), { + cwd, + timeout: DEFAULT_CONFIG.timeout, + }); + + expect(result.error?.message ?? "").not.toContain("timed out"); + expect(result.success).toBe(true); + }, 30000); + + it("still kills a step that exceeds the explicit stepTimeout option", async () => { + const result = await runCliFlow(cliFlow([sleepStep(10000)]), { + stepTimeout: 300, + }); + + expect(result.success).toBe(false); + expect(result.error?.message.toLowerCase()).toContain("timed out"); + if (result.error?.workdir) { + ownedDirs.push(result.error.workdir); + } + }, 20000); + + it("lets a step's own timeout win over the stepTimeout option", async () => { + const result = await runCliFlow( + cliFlow([{ ...sleepStep(10000), timeout: 300 }]), + { stepTimeout: 60000 }, + ); + + expect(result.success).toBe(false); + expect(result.error?.message.toLowerCase()).toContain("timed out"); + if (result.error?.workdir) { + ownedDirs.push(result.error.workdir); + } + }, 20000); + + it("threads stepTimeout from runFlow's options through the surface dispatch", async () => { + const result = await runFlow(cliFlow([sleepStep(10000)]), { + timeout: 5000, + stepTimeout: 300, + }); + + expect(result.success).toBe(false); + expect(result.error?.message.toLowerCase()).toContain("timed out"); + if (result.error?.workdir) { + ownedDirs.push(result.error.workdir); + } + }, 20000); +}); diff --git a/test/config-cli-keys.test.ts b/test/config-cli-keys.test.ts new file mode 100644 index 0000000..b5bcdb6 --- /dev/null +++ b/test/config-cli-keys.test.ts @@ -0,0 +1,269 @@ +/** + * Tests for WI-807: the `cwd` and `captureLimit` config keys the CLI + * surface needs, threaded through mergeConfig (which rebuilds its result + * field by field and would otherwise silently drop them), plus a + * regression pin that config-level `setup` stays web-only. + * + * Contract pinned by the work item: + * - cwd: z.string().optional() on FlowSpecConfigSchema — no schema-level + * resolution against process.cwd() (that's the workdir module's job). + * - captureLimit: a positive integer (bytes), optional, NO schema-level + * default — stays undefined when absent (mirrors `setup`'s existing + * optional-with-no-default pattern). DEFAULT_CAPTURE_LIMIT is applied + * downstream by the exec/CLI-runner consumer, not by config loading. + * - mergeConfig must carry both through untouched when CLI options + * declare neither (cliOptions has no cwd/captureLimit fields per this + * item's scope — mirrors setup's config-only, never-CLI-overridable + * pattern; adding CLI overrides for these is not in this AC). + * - A config declaring neither key loads exactly as today (both fields + * undefined), and the existing config test suites are unaffected. + */ + +import { existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + CONFIG_FILE_NAME, + DEFAULT_CONFIG, + loadConfig, + loadConfigFile, + mergeConfig, +} from "../src/config"; +import { DEFAULT_CAPTURE_LIMIT } from "../src/exec"; + +let tempDir: string; + +beforeEach(() => { + tempDir = join(tmpdir(), `flowspec-config-cli-keys-test-${Date.now()}`); + mkdirSync(tempDir, { recursive: true }); +}); + +afterEach(() => { + if (existsSync(tempDir)) { + rmSync(tempDir, { recursive: true, force: true }); + } +}); + +function writeConfig(contents: string): string { + const configPath = join(tempDir, CONFIG_FILE_NAME); + writeFileSync(configPath, contents); + return configPath; +} + +describe("FlowSpecConfigSchema / loadConfigFile: cwd and captureLimit", () => { + it("loads cwd and captureLimit together when both are declared", () => { + const configPath = writeConfig("cwd: ./sandbox\ncaptureLimit: 1048576\n"); + + const config = loadConfigFile(configPath); + + expect(config.cwd).toBe("./sandbox"); + expect(config.captureLimit).toBe(1048576); + }); + + it("leaves cwd and captureLimit undefined, and other fields at their defaults, when neither key is declared", () => { + const configPath = writeConfig("baseUrl: http://custom.com\n"); + + const config = loadConfigFile(configPath); + + expect(config.cwd).toBeUndefined(); + expect(config.captureLimit).toBeUndefined(); + expect(config.baseUrl).toBe("http://custom.com"); + expect(config.timeout).toBe(DEFAULT_CONFIG.timeout); + expect(config.specsDir).toBe(DEFAULT_CONFIG.specsDir); + }); + + it("substitutes a ${VAR} reference in cwd before validation", () => { + const original = process.env.FLOWSPEC_TEST_SANDBOX_DIR; + process.env.FLOWSPEC_TEST_SANDBOX_DIR = "./from-env-sandbox"; + try { + const configPath = writeConfig("cwd: ${FLOWSPEC_TEST_SANDBOX_DIR}\n"); + + const config = loadConfigFile(configPath); + + expect(config.cwd).toBe("./from-env-sandbox"); + } finally { + if (original === undefined) { + delete process.env.FLOWSPEC_TEST_SANDBOX_DIR; + } else { + process.env.FLOWSPEC_TEST_SANDBOX_DIR = original; + } + } + }); +}); + +describe("captureLimit validation", () => { + it.each([ + ["zero", 0], + ["negative", -1024], + ])("rejects a %s captureLimit, naming the field", (_label, value) => { + const configPath = writeConfig(`captureLimit: ${value}\n`); + + expect(() => loadConfigFile(configPath)).toThrow(/invalid configuration/i); + try { + loadConfigFile(configPath); + throw new Error("expected loadConfigFile to throw"); + } catch (error) { + expect((error as Error).message).toContain("captureLimit"); + } + }); + + it("rejects a non-numeric captureLimit, naming the field", () => { + const configPath = writeConfig('captureLimit: "not-a-number"\n'); + + try { + loadConfigFile(configPath); + throw new Error("expected loadConfigFile to throw"); + } catch (error) { + expect((error as Error).message).toContain("captureLimit"); + } + }); +}); + +describe("cwd type validation", () => { + it("rejects a non-string cwd, naming the field", () => { + const configPath = writeConfig("cwd: 12345\n"); + + try { + loadConfigFile(configPath); + throw new Error("expected loadConfigFile to throw"); + } catch (error) { + expect((error as Error).message).toContain("cwd"); + } + }); + + // Post-mission sweep fix S6: an empty-string cwd used to pass validation + // and then resolve to process.cwd() — the real project directory — so a + // config typo silently turned "isolated disposable temp dir" into "run + // every CLI step against the real repo, and never clean it up". + it.each([ + ["an explicitly empty cwd", 'cwd: ""\n'], + ["a whitespace-only cwd", 'cwd: " "\n'], + ])("rejects %s, naming the field", (_label, contents) => { + const configPath = writeConfig(contents); + + expect(() => loadConfigFile(configPath)).toThrow(/invalid configuration/i); + try { + loadConfigFile(configPath); + throw new Error("expected loadConfigFile to throw"); + } catch (error) { + expect((error as Error).message).toContain("cwd"); + } + }); + + it("still accepts a non-empty cwd", () => { + const configPath = writeConfig("cwd: ./sandbox\n"); + + expect(loadConfigFile(configPath).cwd).toBe("./sandbox"); + }); +}); + +describe("mergeConfig: cwd and captureLimit pass-through", () => { + const configWithCliKeys = { + baseUrl: "http://config.com", + timeout: 5000, + specsDir: "specs/", + cwd: "./sandbox", + captureLimit: 2_000_000, + }; + + it("preserves cwd and captureLimit from the loaded config when CLI options declare neither", () => { + const merged = mergeConfig(configWithCliKeys, { + baseUrl: "http://cli.com", + }); + + expect(merged.cwd).toBe("./sandbox"); + expect(merged.captureLimit).toBe(2_000_000); + }); + + it("preserves cwd and captureLimit through a merge with no CLI options at all", () => { + const merged = mergeConfig(configWithCliKeys, {}); + + expect(merged.cwd).toBe("./sandbox"); + expect(merged.captureLimit).toBe(2_000_000); + }); + + it("does not fabricate cwd or captureLimit when config-level values are absent", () => { + const configWithoutCliKeys = { + baseUrl: "http://config.com", + timeout: 5000, + specsDir: "specs/", + }; + + const merged = mergeConfig(configWithoutCliKeys, { + baseUrl: "http://cli.com", + }); + + expect(merged.cwd).toBeUndefined(); + expect(merged.captureLimit).toBeUndefined(); + }); +}); + +describe("config-level setup stays web-only", () => { + it("rejects a run step in config-level setup, naming the offending verb", () => { + const configPath = writeConfig('setup:\n - run: "flowspec init"\n'); + + try { + loadConfigFile(configPath); + throw new Error("expected loadConfigFile to throw"); + } catch (error) { + const message = (error as Error).message; + expect(message).toContain("setup"); + expect(message).toContain("run"); + } + }); + + it.each([ + [ + "an extra unrecognized key on an otherwise-valid verb", + "setup:\n - visit: /x\n bogus_extra_key: y\n", + "bogus_extra_key", + ], + [ + "a wrong value type on an otherwise-valid verb", + "setup:\n - visit: 123\n", + "Expected string", + ], + ])("rejects %s with the schema's own specific message, not the generic wrong-verb wrapper", (_label, configYaml, expectedDetail) => { + // A malformed-but-VALID-verb step (visit IS a supported config-level + // setup verb) must surface FlowStepSchema's own specific issue — e.g. + // "Unrecognized key(s)" for an extra key, or a type-mismatch message + // for a bad value — not the generic wrong-verb-family wrapper message + // ("Unsupported step ... (web steps only)"), which would misleadingly + // imply `visit` itself isn't a supported verb when it is. Mirrors + // src/types.ts's validateStepForSurface two-tier pattern: verb-family + // mismatch gets the custom wrapper; same-family-but-malformed gets the + // matched schema's own safeParse issues. + const configPath = writeConfig(configYaml); + + try { + loadConfigFile(configPath); + throw new Error("expected loadConfigFile to throw"); + } catch (error) { + const message = (error as Error).message; + expect(message).toContain(expectedDetail); + expect(message).not.toContain("Unsupported step"); + } + }); +}); + +describe("captureLimit's downstream default is DEFAULT_CAPTURE_LIMIT, not a config-level default", () => { + it("DEFAULT_CONFIG has no captureLimit or cwd field", () => { + expect(DEFAULT_CONFIG.captureLimit).toBeUndefined(); + expect(DEFAULT_CONFIG.cwd).toBeUndefined(); + }); + + it("an empty config file loads with captureLimit and cwd both absent, via loadConfig", () => { + const configPath = writeConfig(""); + expect(existsSync(configPath)).toBe(true); + + const config = loadConfig(tempDir); + + expect(config.captureLimit).toBeUndefined(); + expect(config.cwd).toBeUndefined(); + // Sanity: the real enforcement constant exists and is what a consumer + // (WI-806's exec path) falls back to — this file only asserts config + // loading does not itself apply that default. + expect(DEFAULT_CAPTURE_LIMIT).toBe(5 * 1024 * 1024); + }); +}); diff --git a/test/dogfood-plumbing.test.ts b/test/dogfood-plumbing.test.ts new file mode 100644 index 0000000..e8df4fe --- /dev/null +++ b/test/dogfood-plumbing.test.ts @@ -0,0 +1,149 @@ +import { execFileSync, execSync } from "node:child_process"; +import { + existsSync, + mkdtempSync, + readFileSync, + realpathSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import yaml from "js-yaml"; +import { describe, expect, it } from "vitest"; +import { CONFIG_FILE_NAME, loadConfigFile } from "../src/config"; + +/** + * Tests for WI-815: the plumbing that makes the (separately-landed, WI-816) + * dogfood spec runnable — a root flowspec.config.yaml, a `test:e2e`/ + * `pretest:e2e` script pair in package.json, and a CI step. + * + * This is a `type: task` scaffolding item: per the tdd-workflow skill, that + * means smoke tests proving the toolchain actually works end-to-end, not + * file-existence checks (banned anti-pattern #10 — a script or config file + * can exist on disk and still be completely unwired). Every test below + * either parses real config/CI YAML through the project's own real parsing + * code, or actually RUNS the real build+link mechanism and invokes the + * resulting binary against a throwaway fixture spec. + * + * Deliberately NOT tested here: running the real `specs/` directory itself + * — that content is WI-816's job, a separate, undependent item, and doesn't + * exist yet. This file proves the MECHANISM (build, link, explicit-path + * invocation) works against a test-owned fixture spec instead, so it never + * depends on WI-816 landing first. + * + * AC "adding a root flowspec.config.yaml does not change any existing + * test's behavior" is verified by running the full `bun run test` suite + * (not re-tested here as a meta-test) — every existing config fixture + * builds under os.tmpdir(), a sibling branch of the filesystem tree that + * findConfigFile's walk-up can never reach the repo root through. + */ + +const repoRoot = resolve(__dirname, ".."); +const execPath = process.execPath; + +describe("root flowspec.config.yaml", () => { + it("exists and declares specsDir: 'specs/', parsed through the real config loader", () => { + const configPath = join(repoRoot, CONFIG_FILE_NAME); + const config = loadConfigFile(configPath); + + expect(config.specsDir).toBe("specs/"); + }); +}); + +describe("package.json: test:e2e / pretest:e2e scripts", () => { + const packageJson = JSON.parse( + readFileSync(join(repoRoot, "package.json"), "utf-8"), + ) as { scripts?: Record }; + const scripts = packageJson.scripts ?? {}; + + it("test:e2e passes an explicit 'specs/' path rather than relying on discovery", () => { + // Pinned per the item's own literal wording — flowspec run intentionally + // has no specsDir-based discovery (out of scope by human decision), so + // the script must name the path itself. + expect(scripts["test:e2e"]).toBe("flowspec run specs/"); + }); + + it("pretest:e2e is present and its command builds the project and links node_modules/.bin/flowspec to dist/index.js", () => { + expect(scripts["pretest:e2e"]).toBeDefined(); + const command = scripts["pretest:e2e"] as string; + expect(command).toContain("dist/index.js"); + expect(command).toMatch(/\.bin\/flowspec/); + }); +}); + +describe("pretest:e2e actually builds and links a working 'flowspec' binary", () => { + it("running pretest:e2e twice in a row produces a working node_modules/.bin/flowspec both times", () => { + // Real toolchain proof, not a file-existence check: actually run the + // real script, then actually invoke the linked binary against a + // throwaway fixture spec (not the real specs/ — that's WI-816's, + // separately-landed content) and confirm it genuinely executes. + const fixtureDir = realpathSync( + mkdtempSync(join(tmpdir(), "flowspec-dogfood-plumbing-")), + ); + const binPath = join(repoRoot, "node_modules", ".bin", "flowspec"); + + try { + writeFileSync( + join(fixtureDir, "smoke.flow.yaml"), + `name: plumbing-smoke-test +description: proves the linked flowspec binary actually runs a cli spec +surface: cli +steps: + - run: "${execPath} -e process.exit(0)" +expect: + - exit_code: 0 +`, + ); + + for (let attempt = 1; attempt <= 2; attempt++) { + execSync("bun run pretest:e2e", { + cwd: repoRoot, + stdio: "pipe", + timeout: 45000, + }); + + expect(existsSync(join(repoRoot, "dist", "index.js"))).toBe(true); + expect(existsSync(binPath)).toBe(true); + + // Pass args as an array (no shell) so binPath/fixtureDir can't be + // reinterpreted by a shell — safe even if a path ever contained + // spaces or shell metacharacters like $, `, or quotes. + const output = execFileSync(binPath, ["run", fixtureDir], { + cwd: repoRoot, + encoding: "utf-8", + timeout: 15000, + }); + expect(output).toContain("plumbing-smoke-test"); + expect(output).toContain("1 flow"); + } + } finally { + rmSync(fixtureDir, { recursive: true, force: true }); + } + }, 60000); +}); + +describe("CI workflow: build before, and run, the dogfood e2e step", () => { + it("builds the project and runs test:e2e as its own step after the unit tests", () => { + const workflowPath = join(repoRoot, ".github", "workflows", "ci.yml"); + const workflow = yaml.load(readFileSync(workflowPath, "utf-8")) as { + jobs: { + test: { steps: Array<{ name?: string; run?: string }> }; + }; + }; + const steps = workflow.jobs.test.steps; + const stepNames = steps.map((step) => step.run ?? ""); + + const buildIndex = stepNames.findIndex((run) => /\bbuild\b/.test(run)); + const unitTestIndex = stepNames.findIndex( + (run) => /\btest\b/.test(run) && !/e2e/.test(run), + ); + const e2eIndex = stepNames.findIndex((run) => /test:e2e/.test(run)); + + expect(buildIndex).toBeGreaterThanOrEqual(0); + expect(unitTestIndex).toBeGreaterThanOrEqual(0); + expect(e2eIndex).toBeGreaterThanOrEqual(0); + expect(buildIndex).toBeLessThan(e2eIndex); + expect(unitTestIndex).toBeLessThan(e2eIndex); + }); +}); diff --git a/test/dogfood-spec.test.ts b/test/dogfood-spec.test.ts new file mode 100644 index 0000000..a91ac4c --- /dev/null +++ b/test/dogfood-spec.test.ts @@ -0,0 +1,85 @@ +import { execSync } from "node:child_process"; +import { delimiter, join, resolve } from "node:path"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { runCliFlow } from "../src/cli-runner"; +import { parseFlowFile } from "../src/parser"; +import { asCliFlow } from "../src/types"; + +/** + * Tests for WI-816: the dogfood spec — FlowSpec specs its own `flowspec + * init` command as a protected, immutable `surface: cli` flow. + * + * AUTHORING NOTE (per the item's own context, human decision 2026-08-17): + * this repo's PreToolUse hook blocks Edit/Write on any specs/**\/*.flow.yaml + * path, so the spec content was drafted at spec-drafts/init.flow.yaml (not + * the hook-protected path) and reviewed by a human before being moved into + * place. The operator has since run `git mv spec-drafts/init.flow.yaml + * specs/init.flow.yaml` — the draft location no longer exists, and this + * file now points at the final, protected path. + * + * Test design: rather than asserting on the spec's internal YAML shape + * (array-form run steps, exact assertion list, step count/order — testing + * implementation detail, not behavior), this file does the two things the + * item's own ACs actually call for: + * 1. The literal last AC bullet: the shipped spec parses as a valid + * `surface: cli` FlowSpec, so a malformed spec fails `bun run test` + * and not only the e2e step. + * 2. ACTUALLY RUNS the parsed spec via the real runCliFlow engine, twice + * in a row. This is a far stronger, more honest proof than static + * shape assertions: if the array-form run steps are wrong, if any of + * the three file assertions fail, if the second init's stdout doesn't + * say "Found existing config:", or if state leaks between runs, this + * test fails — for exactly the reason the AC cares about, not a proxy + * for it. It exercises the same execution path `flowspec run specs/` + * (WI-815's test:e2e) uses in production. + * + * `flowspec` must resolve on PATH for the spec's own run steps to spawn + * it — this test builds+links it itself (mirroring WI-815's pretest:e2e + * mechanism) rather than assuming the invoking process's PATH already has + * node_modules/.bin, and temporarily prepends it to process.env.PATH so + * runCliFlow's spawned children inherit it. + */ + +const repoRoot = resolve(__dirname, ".."); +const DOGFOOD_SPEC_PATH = join(repoRoot, "specs", "init.flow.yaml"); + +describe("specs/init.flow.yaml", () => { + let originalPath: string | undefined; + + beforeAll(() => { + execSync("bun run pretest:e2e", { + cwd: repoRoot, + stdio: "pipe", + timeout: 45000, + }); + + const binDir = join(repoRoot, "node_modules", ".bin"); + originalPath = process.env.PATH; + process.env.PATH = originalPath + ? `${binDir}${delimiter}${originalPath}` + : binDir; + }, 60000); + + afterAll(() => { + if (originalPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = originalPath; + } + }); + + it("parses as a valid surface: cli FlowSpec", () => { + const flow = parseFlowFile(DOGFOOD_SPEC_PATH); + expect(flow.surface).toBe("cli"); + }); + + it("passes when actually run via the real CLI engine, twice in a row with no leftover state between runs", async () => { + const flow = asCliFlow(parseFlowFile(DOGFOOD_SPEC_PATH)); + + const first = await runCliFlow(flow, {}); + expect(first.success).toBe(true); + + const second = await runCliFlow(flow, {}); + expect(second.success).toBe(true); + }, 30000); +}); diff --git a/test/exec-limits.test.ts b/test/exec-limits.test.ts new file mode 100644 index 0000000..ed32bfe --- /dev/null +++ b/test/exec-limits.test.ts @@ -0,0 +1,311 @@ +import { describe, expect, it } from "vitest"; +import { DEFAULT_CAPTURE_LIMIT, spawnProcess } from "../src/exec"; + +/** + * Tests for WI-806: giving spawnProcess's ExecResult.timedOut/truncated + * fields (stubbed always-false by WI-801) real enforcement behavior. + * + * Contract pinned by the work item, plus decisions this test file makes + * explicit (no prior convention existed for these — see the handoff for + * why each was chosen): + * - options.timeout (ms): on expiry, the process is killed, the promise + * still RESOLVES (never hangs, never rejects), ExecResult.timedOut is + * true, and the timeout value (in ms) appears as text in `stderr` — + * the conventional diagnostic stream — alongside the word "timed out". + * - options.captureLimit (bytes, default DEFAULT_CAPTURE_LIMIT): output + * beyond the cap is truncated PER STREAM, with the literal marker + * "[truncated]" (matching src/matchers.ts's excerpt convention) + * appended after the head-truncated captured text, and + * ExecResult.truncated is true. + * - A killed command's partial stdout/stderr captured before the kill + * are still returned (not discarded), alongside timedOut: true. + * - Within both limits: timedOut false, truncated false, stdout + * byte-identical to what the command wrote (verified with a fixture + * containing unicode, a newline, and a tab — not just plain ASCII). + * + * Not tested here (implementation-technique requirement, not an + * observable-behavior one): the context requires truncation to be applied + * WHILE STREAMING rather than by capturing everything and slicing + * afterward, "or the memory bound is not real." That distinction produces + * identical black-box output for any test fixture small enough to run in + * a unit test — proving it would require actually flooding memory, which + * this suite deliberately does not do. That property belongs to code + * review of the implementation diff, not to a test assertion here. + */ + +describe("timeout", () => { + it("kills a command that runs longer than the timeout, reporting timedOut true with the limit named", async () => { + const timeout = 200; + const result = await spawnProcess( + [process.execPath, "-e", "setTimeout(() => {}, 10000)"], + { timeout }, + ); + expect(result.timedOut).toBe(true); + expect(result.stderr).toContain(String(timeout)); + expect(result.stderr.toLowerCase()).toContain("timed out"); + }); + + it("actually terminates a command that traps/ignores SIGTERM, not just sends the signal", async () => { + // Regression test for a bug Amy found via probing: a bare kill() sends + // SIGTERM by default. A child that installs its own SIGTERM handler and + // ignores it (a broken or adversarial command — exactly what the + // timeout feature exists to guard against) never dies from that signal, + // and Bun.spawn does not escalate to SIGKILL on its own. If + // spawnProcess only sends SIGTERM and waits, this resolves only once + // the child's own UNRELATED 15s sleep elapses (or hangs indefinitely) — + // not because of the timeout. A correct implementation must guarantee + // real termination (SIGKILL directly, or SIGTERM escalating to SIGKILL + // after a short grace period), resolving well before the child's own + // schedule. Verified interactively against Bun 1.3.11: a bare kill() + // against this exact fixture left the process alive at least 3000ms + // later (25x+ the timeout), while proc.kill("SIGKILL") terminated a + // real subprocess immediately (exit code 137). + const timeout = 300; + const start = Date.now(); + const result = await spawnProcess( + [ + process.execPath, + "-e", + "process.on('SIGTERM', () => {}); setTimeout(() => {}, 15000);", + ], + { timeout }, + ); + const elapsed = Date.now() - start; + + expect(result.timedOut).toBe(true); + expect(elapsed).toBeLessThan(3000); + }); + + it("resolves (does not hang) when the command exceeds the timeout", async () => { + const start = Date.now(); + await spawnProcess( + [process.execPath, "-e", "setTimeout(() => {}, 10000)"], + { timeout: 200 }, + ); + const elapsed = Date.now() - start; + expect(elapsed).toBeLessThan(2000); + }); + + it("returns partial stdout and stderr captured before the kill, not discarded", async () => { + const result = await spawnProcess( + [ + process.execPath, + "-e", + "process.stdout.write('hello-before-kill'); process.stderr.write('err-before-kill'); setTimeout(() => {}, 10000);", + ], + { timeout: 300 }, + ); + expect(result.timedOut).toBe(true); + expect(result.stdout).toContain("hello-before-kill"); + expect(result.stderr).toContain("err-before-kill"); + }); + + it("does not report timedOut for a command that finishes before the timeout", async () => { + const result = await spawnProcess( + [process.execPath, "-e", "process.stdout.write('quick')"], + { timeout: 5000 }, + ); + expect(result.timedOut).toBe(false); + }); + + it("is not defeated by a backgrounded grandchild that keeps the stdout/stderr pipes open", async () => { + // Regression test: SIGTERM-then-SIGKILL escalation kills the DIRECT + // child, but a detached grandchild it spawned (here, `sleep 20 &`) + // inherits the same stdout/stderr pipes and holds them open for its own + // lifetime. Reading those streams to EOF therefore does NOT finish when + // the child dies, and an implementation that awaits the reads + // unconditionally alongside proc.exited hangs for the grandchild's full + // lifetime — verified empirically against the pre-fix code: a 500ms + // timeout was still pending 5000ms later, 10x its own deadline. A + // correct implementation bounds the post-exit drain and returns + // whatever was already buffered, so the timeout is a real deadline. + // + // The call is raced against the test's own deadline rather than just + // having its elapsed time measured: against the buggy code it never + // settles at all, which would hang the whole test file instead of + // failing this one test. + const deadlineMs = 2000; + const start = Date.now(); + const outcome = await Promise.race([ + spawnProcess(["sh", "-c", "sleep 20 & echo started; sleep 30"], { + timeout: 500, + }).then((result) => ({ kind: "resolved" as const, result })), + new Promise<{ kind: "deadline" }>((resolve) => + setTimeout(() => resolve({ kind: "deadline" }), deadlineMs), + ), + ]); + const elapsed = Date.now() - start; + + expect(outcome.kind).toBe("resolved"); + expect(elapsed).toBeLessThan(deadlineMs); + if (outcome.kind === "resolved") { + expect(outcome.result.timedOut).toBe(true); + // Output buffered before the drain was abandoned is still reported, + // exactly as it is for the ordinary kill case above — bounding the + // wait must not mean discarding what was already captured. + expect(outcome.result.stdout).toContain("started"); + } + }); + + it("returns promptly when the command itself exits but a backgrounded grandchild holds the pipes open", async () => { + // The same pipe-inheritance problem without any timeout expiring: `sh` + // exits immediately here, so this is a completely successful run whose + // streams simply never reach EOF. Pre-fix, this hung indefinitely even + // though the command had already succeeded. + const deadlineMs = 2000; + const outcome = await Promise.race([ + spawnProcess(["sh", "-c", "sleep 20 & echo started"], { + timeout: 10000, + }).then((result) => ({ kind: "resolved" as const, result })), + new Promise<{ kind: "deadline" }>((resolve) => + setTimeout(() => resolve({ kind: "deadline" }), deadlineMs), + ), + ]); + + expect(outcome.kind).toBe("resolved"); + if (outcome.kind === "resolved") { + expect(outcome.result.timedOut).toBe(false); + expect(outcome.result.exitCode).toBe(0); + expect(outcome.result.stdout).toContain("started"); + } + }); + + it("treats an explicit timeout of 0 as kill-immediately, not as no-limit", async () => { + // Pins the primitive-level semantics of the falsy-but-present value: 0 + // is a real deadline of zero milliseconds, so the child is killed at + // once and timedOut is true. It must NOT be silently treated as + // "unset"/no-limit (which would let this 10s sleep run to completion), + // and it must not throw. Callers that mean "no limit" leave timeout + // undefined; choosing a sane default for an unset/zero config value is + // the surface layer's job, not this primitive's. + const start = Date.now(); + const result = await spawnProcess( + [process.execPath, "-e", "setTimeout(() => {}, 10000)"], + { timeout: 0 }, + ); + const elapsed = Date.now() - start; + + expect(result.timedOut).toBe(true); + expect(elapsed).toBeLessThan(2000); + expect(result.stderr).toContain("timed out after 0ms"); + }); +}); + +describe("output capture bounds", () => { + it("truncates stdout beyond a custom captureLimit, appending an explicit marker, and reports truncated true", async () => { + const captureLimit = 100; + const written = "y".repeat(captureLimit * 5); + const result = await spawnProcess( + [ + process.execPath, + "-e", + `process.stdout.write(${JSON.stringify(written)})`, + ], + { captureLimit }, + ); + expect(result.truncated).toBe(true); + expect(result.stdout.startsWith(written.slice(0, captureLimit))).toBe(true); + expect(result.stdout).toContain("[truncated]"); + expect(result.stdout.length).toBeLessThan(written.length); + }); + + it("truncates stderr independently of stdout", async () => { + const captureLimit = 100; + const writtenErr = "z".repeat(captureLimit * 5); + const result = await spawnProcess( + [ + process.execPath, + "-e", + `process.stdout.write('short'); process.stderr.write(${JSON.stringify(writtenErr)})`, + ], + { captureLimit }, + ); + expect(result.truncated).toBe(true); + expect(result.stdout).toBe("short"); + expect(result.stderr).toContain("[truncated]"); + expect(result.stderr.length).toBeLessThan(writtenErr.length); + }); + + it("a captureLimit well under 5MB still truncates output that would fit under the 5MB default", async () => { + const captureLimit = 1000; + const written = "a".repeat(captureLimit * 5); + expect(written.length).toBeLessThan(DEFAULT_CAPTURE_LIMIT); + + const result = await spawnProcess( + [ + process.execPath, + "-e", + `process.stdout.write(${JSON.stringify(written)})`, + ], + { captureLimit }, + ); + expect(result.truncated).toBe(true); + }); + + it("uses DEFAULT_CAPTURE_LIMIT (5 MB) when no captureLimit is supplied", async () => { + const size = DEFAULT_CAPTURE_LIMIT + 1000; + const result = await spawnProcess([ + process.execPath, + "-e", + `process.stdout.write('b'.repeat(${size}))`, + ]); + expect(result.truncated).toBe(true); + expect(result.stdout.length).toBeLessThan(size); + }); + + it("does not truncate output that fits within the limit, reporting truncated false", async () => { + const written = "hello world, this fits easily"; + const result = await spawnProcess([ + process.execPath, + "-e", + `process.stdout.write(${JSON.stringify(written)})`, + ]); + expect(result.truncated).toBe(false); + expect(result.stdout).toBe(written); + }); + + it("truncates cleanly at a captureLimit that lands mid-character, without splitting the multi-byte UTF-8 sequence", async () => { + // Regression test (CodeRabbit review, PR #16): captureLimit is a BYTE + // ceiling, but every other truncation fixture in this file uses + // single-byte ASCII, so a byte-level slice landing inside a multi-byte + // UTF-8 sequence was never exercised. "é" is U+00E9, encoded as the + // 2-byte UTF-8 sequence 0xC3 0xA9. captureLimit: 5 lands after 2 whole + // characters (4 bytes) plus one stray leading byte (0xC3) of a third — + // a byte offset that sits mid-character. A blind byte slice fed + // straight to TextDecoder would decode that dangling lead byte as a + // U+FFFD replacement-character artifact instead of cleanly stopping + // before it. + const char = "é"; + const captureLimit = 5; + const written = char.repeat(50); + const result = await spawnProcess( + [ + process.execPath, + "-e", + `process.stdout.write(${JSON.stringify(written)})`, + ], + { captureLimit }, + ); + expect(result.truncated).toBe(true); + // The captured head must be exactly the whole characters that fit + // (2 of them, "éé") plus the truncation marker — never a partial + // character or a replacement-character artifact. + expect(result.stdout).toBe(`${char.repeat(2)}[truncated]`); + expect(result.stdout).not.toContain("�"); + }); +}); + +describe("within limits: exact capture, no flags set", () => { + it("reports timedOut false and truncated false, with stdout byte-identical to what the command wrote", async () => { + const written = + "hello world — unicode: café, 日本語, emoji: 🎉\nwith a newline\ttab too"; + const result = await spawnProcess([ + process.execPath, + "-e", + `process.stdout.write(${JSON.stringify(written)})`, + ]); + expect(result.timedOut).toBe(false); + expect(result.truncated).toBe(false); + expect(result.stdout).toBe(written); + }); +}); diff --git a/test/exec.test.ts b/test/exec.test.ts new file mode 100644 index 0000000..d77b97f --- /dev/null +++ b/test/exec.test.ts @@ -0,0 +1,292 @@ +import { existsSync, mkdtempSync, realpathSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { DEFAULT_CAPTURE_LIMIT, spawnProcess } from "../src/exec"; + +/** + * Tests for WI-801: the CLI-surface spawn primitive. + * + * Contract pinned by the work item: + * - spawnProcess(argv, options?) spawns argv[0] directly with argv.slice(1) + * as arguments — NO shell — capturing stdout/stderr/exitCode. + * - options.cwd, options.env, options.stdin are all optional. + * - options.env OVERLAYS the inherited process.env (both stay visible; an + * explicit entry wins over an inherited one of the same name). + * - A command that cannot be spawned at all (e.g. not found) REJECTS the + * returned promise with an error naming the command — distinct from a + * command that runs and exits nonzero, which RESOLVES normally. + * - Writing stdin to a process that never reads it must not reject or + * throw out of spawnProcess; the real exit code is still reported. + * - ExecResult always carries `timedOut` and `truncated`, both false in + * this item (a follow-up item gives them real meaning). + * - DEFAULT_CAPTURE_LIMIT is exported as 5 * 1024 * 1024. + * + * Fixture note: commands are `process.execPath -e