From ba44631f81d0095a3b29b01eed7ed7b9d98f0429 Mon Sep 17 00:00:00 2001 From: Josh Owens Date: Wed, 19 Aug 2026 20:39:32 +0000 Subject: [PATCH 1/5] feat: CLI surface adapter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds surface: cli, letting FlowSpec flows drive command-line tools instead of a browser: no-shell process spawning, surface-agnostic and file matchers, a per-flow working directory, eight CLI assertions, fail-fast exit-code semantics with flow-level setup, and a reporter that renders CLI and web failures in one summary. runFlow dispatches on surface, and the CLI path never touches the browser. Web behavior is byte-identical throughout — the existing web test suite passed unmodified. FlowSpec now dogfoods itself: specs/init.flow.yaml specs flowspec init, run via bun run test:e2e locally and in CI. Items completed: - #800: FlowSpec schema accepts surface: cli with the run-step grammar - #801: Spawn CLI commands with cwd, env, and stdin — no shell - #802: Surface-agnostic contains, regex, and dot-path JSON matchers - #803: Retryable file existence and file content matchers - #804: Per-flow working directory: fresh temp dir, kept on failure - #805: CLI assertion schemas and CLI-aware FlowError fields - #806: Bounded and time-limited command capture - #807: Config gains cwd and captureLimit, and stays web-only for setup - #808: CLI flows execute run steps with fail-fast exit-code semantics - #809: Evaluate the eight CLI assertions against the last run step - #810: Reporter renders CLI failures and one summary across surfaces - #811: CLI flows run a flow-level setup phase in their own grammar - #812: runFlow dispatches on surface with no browser on the CLI path - #814: Document the CLI surface in the README and specification - #815: Root config, e2e script, and CI step for the dogfood spec - #816: FlowSpec specs its own flowspec init Co-authored-by: Hannibal Co-authored-by: Face Co-authored-by: Murdock Co-authored-by: B.A. Co-authored-by: Lynch Co-authored-by: Amy Co-authored-by: Tawnia --- .github/workflows/ci.yml | 6 + CHANGELOG.md | 17 + README.md | 157 +++++- adr/0004-expect-exit-honored-on-every-step.md | 47 ++ ...tected-specs-authored-outside-specs-dir.md | 52 ++ adr/0006-specsdir-discovery-out-of-scope.md | 47 ++ docs/specification.md | 116 ++++- flowspec.config.yaml | 1 + package.json | 3 +- prd/0007-cli-surface-adapter.md | 389 +++++++++++++++ specs/init.flow.yaml | 17 + src/cli-assertions.ts | 176 +++++++ src/cli-runner.ts | 266 ++++++++++ src/config.ts | 158 +++++- src/exec.ts | 364 ++++++++++++++ src/file-matchers.ts | 121 +++++ src/index.ts | 6 + src/matchers.ts | 202 ++++++++ src/reporter.ts | 57 ++- src/runner.ts | 18 + src/types.ts | 433 +++++++++++++++- src/workdir.ts | 83 ++++ test/cli-assertions.test.ts | 463 ++++++++++++++++++ test/cli-runner-setup.test.ts | 255 ++++++++++ test/cli-runner.test.ts | 365 ++++++++++++++ test/config-cli-keys.test.ts | 244 +++++++++ test/dogfood-plumbing.test.ts | 140 ++++++ test/dogfood-spec.test.ts | 80 +++ test/exec-limits.test.ts | 199 ++++++++ test/exec.test.ts | 249 ++++++++++ test/file-matchers.test.ts | 231 +++++++++ test/matchers.test.ts | 203 ++++++++ test/reporter-cli.test.ts | 158 ++++++ test/runner-dispatch.test.ts | 359 ++++++++++++++ test/types-cli-assertions.test.ts | 272 ++++++++++ test/types-cli-surface.test.ts | 258 ++++++++++ test/workdir.test.ts | 192 ++++++++ 37 files changed, 6372 insertions(+), 32 deletions(-) create mode 100644 adr/0004-expect-exit-honored-on-every-step.md create mode 100644 adr/0005-protected-specs-authored-outside-specs-dir.md create mode 100644 adr/0006-specsdir-discovery-out-of-scope.md create mode 100644 flowspec.config.yaml create mode 100644 prd/0007-cli-surface-adapter.md create mode 100644 specs/init.flow.yaml create mode 100644 src/cli-assertions.ts create mode 100644 src/cli-runner.ts create mode 100644 src/exec.ts create mode 100644 src/file-matchers.ts create mode 100644 src/matchers.ts create mode 100644 src/workdir.ts create mode 100644 test/cli-assertions.test.ts create mode 100644 test/cli-runner-setup.test.ts create mode 100644 test/cli-runner.test.ts create mode 100644 test/config-cli-keys.test.ts create mode 100644 test/dogfood-plumbing.test.ts create mode 100644 test/dogfood-spec.test.ts create mode 100644 test/exec-limits.test.ts create mode 100644 test/exec.test.ts create mode 100644 test/file-matchers.test.ts create mode 100644 test/matchers.test.ts create mode 100644 test/reporter-cli.test.ts create mode 100644 test/runner-dispatch.test.ts create mode 100644 test/types-cli-assertions.test.ts create mode 100644 test/types-cli-surface.test.ts create mode 100644 test/workdir.test.ts 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..e9df08e 100644 --- a/README.md +++ b/README.md @@ -111,6 +111,23 @@ specsDir: specs/ CLI options override config file values. +#### `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 +196,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 +215,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 +235,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 +271,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 +283,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 +292,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 `--timeout` (or its config value, or the config schema's own 10000ms 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 `timeout` means something different here than it does for a web flow's assertion retries: for a CLI step it's a hard deadline — the command is killed (`SIGTERM`, escalating to `SIGKILL` if it doesn't exit) the moment it elapses — not a "poll until this much time has passed" window. Passing `--timeout 0` disables web assertion retries, but for a CLI step it means "kill almost immediately," so avoid `--timeout 0` for a project that mixes both surfaces. + +`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..5fae48f 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,17 @@ 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 (web), in milliseconds. + # Also the CLI kill-deadline fallback — see + # "CLI-surface settings" below. 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 +221,12 @@ 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. + +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 +258,16 @@ 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` and `captureLimit` configure `surface: cli` flows only; web flows ignore both, and neither has a `--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)). + +`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 +328,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 fa8ec89..86d09d2 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..1fc41aa --- /dev/null +++ b/prd/0007-cli-surface-adapter.md @@ -0,0 +1,389 @@ +--- +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 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. The CLI runner 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. +- `timeout` bounds the step; on expiry the process is killed and the step fails with a + timeout error. Default comes from the existing config `timeout`. + +### 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 `execCommand` 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 `execCommand`'s no-shell argv spawning (`src/runner.ts:112`), 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..943cc39 --- /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", "--dir", "./proj"] +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: "Found existing config:" diff --git a/src/cli-assertions.ts b/src/cli-assertions.ts new file mode 100644 index 0000000..82b1942 --- /dev/null +++ b/src/cli-assertions.ts @@ -0,0 +1,176 @@ +/** + * 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 { resolve } from "node:path"; +import { fileContains, fileExists } from "./file-matchers.js"; +import { + EXCERPT_LIMIT, + 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; +} + +interface LastStepResult { + stdout: string; + stderr: string; + exitCode: number; +} + +const TRUNCATION_MARKER = "[truncated]"; + +/** Head-truncate `text` to EXCERPT_LIMIT, matching src/matchers.ts's excerpt convention. */ +function boundedExcerpt(text: string): string { + if (text.length <= EXCERPT_LIMIT) { + return text; + } + return `${text.slice(0, EXCERPT_LIMIT)}${TRUNCATION_MARKER}`; +} + +/** + * 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, + ); + } + + if ("stdout_contains" in assertion) { + const failure = matchContains(lastStep.stdout, assertion.stdout_contains); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + if ("stderr_contains" in assertion) { + const failure = matchContains(lastStep.stderr, assertion.stderr_contains); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + if ("stdout_matches" in assertion) { + const failure = matchRegex(lastStep.stdout, assertion.stdout_matches); + return failure ? toFailure(failure.message, lastStep, workdir) : undefined; + } + + if ("stderr_matches" in assertion) { + 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 absolutePath = resolve(workdir, assertion.file_exists); + const failure = await fileExists(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 absolutePath = resolve(workdir, path); + const failure = await fileContains(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..4633268 --- /dev/null +++ b/src/cli-runner.ts @@ -0,0 +1,266 @@ +/** + * 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 { spawnProcess } from "./exec.js"; +import type { + CliAssertion, + CliStep, + FlowError, + FlowResult, + FlowSpec, +} from "./types.js"; +import { createFlowWorkdir } from "./workdir.js"; + +export interface CliRunOptions { + cwd?: string; + timeout?: 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. */ +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: execResult.stdout, + stderr: 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, + timeout: step.timeout ?? options?.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( + `${label} ${index} timed out: ${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: FlowSpec, + 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 as CliStep[]; + 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 ?? []) as CliStep[]; + + // 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. Every fixture in + // WI-808/811's own test suites uses expect: [], which has nothing to + // evaluate (the loop below is simply a no-op for them). + if (lastExecResult) { + for (const assertion of (flow.expect ?? []) as CliAssertion[]) { + 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..0edcbd5 100644 --- a/src/config.ts +++ b/src/config.ts @@ -4,26 +4,146 @@ import yaml from "js-yaml"; import { z } from "zod"; import { FlowStepSchema } from "./types.js"; +/** + * 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); +} + +/** + * Re-parse a step whose verb IS a recognized web verb but that otherwise + * failed FlowStepSchema (an extra key, a wrong value type), and surface the + * SPECIFIC problem rather than Zod's generic union-failure wrapper. + * + * FlowStepSchema is a union of five single-verb strict object schemas. When + * none of the five match, Zod's default behavior varies by failure shape: + * an extra-key-only mismatch (verified interactively) happens to surface a + * direct top-level "Unrecognized key(s)" issue, but a wrong-value-type + * mismatch surfaces only the generic "invalid_union" wrapper ("Invalid + * input") at the top level — the useful "Expected string, received number" + * detail is buried inside `unionErrors[branch].issues[]`, one branch per + * verb. Since the matched verb is already known here, the one relevant + * branch (the one that recognizes that verb as its own field, rather than + * complaining it's an unrecognized key) is picked out directly. + */ +function describeMalformedWebStep(step: unknown, verb: string): string { + const result = FlowStepSchema.safeParse(step); + if (result.success) { + return ""; + } + + const [topIssue] = result.error.issues; + if (topIssue?.code === "invalid_union") { + const matchingBranch = topIssue.unionErrors.find((branchError) => + branchError.issues.some((issue) => issue.path[0] === verb), + ); + if (matchingBranch) { + return matchingBranch.issues + .filter((issue) => issue.path[0] === verb) + .map((issue) => issue.message) + .join("; "); + } + } + + return result.error.issues.map((issue) => issue.message).join("; "); +} + /** * 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"), + timeout: z.number().positive().optional().default(10000), + 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. + cwd: z.string().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; + } + + const detail = describeMalformedWebStep(step, verb); + if (detail) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["setup", index], + message: detail, + }); + } + }); + }); export type FlowSpecConfig = z.infer; @@ -206,5 +326,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..1af0ec1 --- /dev/null +++ b/src/exec.ts @@ -0,0 +1,364 @@ +/** + * 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; + +/** + * 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; +} + +/** + * Read `stream` to completion, decoded as UTF-8, capturing at most `limit` + * bytes. 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. + */ +async function readBoundedStream( + stream: ReadableStream, + limit: number, +): Promise { + const reader = stream.getReader(); + const capturedChunks: Uint8Array[] = []; + let capturedBytes = 0; + let truncated = false; + + try { + while (true) { + const { done, value } = await reader.read(); + if (done) { + break; + } + if (!value || value.length === 0) { + continue; + } + + const remaining = limit - capturedBytes; + if (remaining <= 0) { + truncated = true; + continue; + } + if (value.length > remaining) { + capturedChunks.push(value.subarray(0, remaining)); + capturedBytes += remaining; + truncated = true; + } else { + capturedChunks.push(value); + capturedBytes += value.length; + } + } + } finally { + reader.releaseLock(); + } + + const buffer = new Uint8Array(capturedBytes); + let offset = 0; + for (const chunk of capturedChunks) { + buffer.set(chunk, offset); + offset += chunk.length; + } + + return { text: new TextDecoder("utf-8").decode(buffer), 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 — nothing here needs to race against + * a hang. `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. + } + } + + let timedOut = false; + let timer: ReturnType | undefined; + let graceTimer: 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); + } + + const [stdoutResult, stderrResult, exitCode] = await Promise.all([ + readBoundedStream(proc.stdout, captureLimit), + readBoundedStream(proc.stderr, captureLimit), + proc.exited, + ]); + + if (timer) { + clearTimeout(timer); + } + if (graceTimer) { + clearTimeout(graceTimer); + } + + 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..d33d4b2 --- /dev/null +++ b/src/file-matchers.ts @@ -0,0 +1,121 @@ +/** + * 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, readFile } from "node:fs/promises"; +import { type MatchFailure, matchContains } from "./matchers.js"; +import { POLL_INTERVAL } from "./runner.js"; + +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(POLL_INTERVAL); + 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 readFile(absolutePath, "utf-8"); + } 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..499c84d 100755 --- a/src/index.ts +++ b/src/index.ts @@ -190,6 +190,8 @@ async function runFlows( configSetup: FlowStep[] | undefined, configHeaders: Record | undefined, configHeadersScope: "origin" | "all" | undefined, + configCwd: string | undefined, + configCaptureLimit: number | undefined, ): Promise { const results: FlowResult[] = []; @@ -201,6 +203,8 @@ async function runFlows( setup: configSetup, headers: configHeaders, headersScope: configHeadersScope, + cwd: configCwd, + captureLimit: configCaptureLimit, }); console.log(formatResult(result)); results.push(result); @@ -358,6 +362,8 @@ async function handleRunCommand(args: string[]): Promise { mergedConfig.setup, mergedConfig.headers, mergedConfig.headersScope, + mergedConfig.cwd, + mergedConfig.captureLimit, ); // Print summary diff --git a/src/matchers.ts b/src/matchers.ts new file mode 100644 index 0000000..ab7c4eb --- /dev/null +++ b/src/matchers.ts @@ -0,0 +1,202 @@ +/** + * 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. + */ +function excerpt(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: ${excerpt(haystack)}`, + expected: needle, + actual: haystack, + }; +} + +/** + * Passes when `pattern` (compiled as a RegExp) 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); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + return { + message: `Invalid regex pattern "${pattern}": ${reason}. Actual: ${excerpt(haystack)}`, + expected: pattern, + actual: haystack, + }; + } + + if (regex.test(haystack)) { + return undefined; + } + + return { + message: `Expected text to match pattern "${pattern}" but it did not. Actual: ${excerpt(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: ${excerpt(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..ad256e4 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,50 @@ 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"; } +/** + * 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. + * + * Gated on `error.exitCode !== undefined`: that field's presence alone + * signals a CLI failure (a web failure never sets it) — independent of + * whether `step`/`action` are also present, since an assertion failure + * (this item's main concern) doesn't carry a step index the way an + * action-step failure does. stdout/stderr are rendered as-is: they're + * already bounded/truncated upstream (src/cli-assertions.ts), so a + * truncation marker baked in there is preserved verbatim, never re-cut here. + * `workdir` gets its own line only when present — a configured cwd (the + * user's own directory) never sets it, so nothing "working directory"-shaped + * is printed for that case. + */ +function formatCliFailureLines(error: FlowError): string[] { + if (error.exitCode === undefined) { + return []; + } + + const lines = [ + `Exit code: ${error.exitCode}`, + `stdout: ${error.stdout ?? ""}`, + `stderr: ${error.stderr ?? ""}`, + ]; + + 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 @@ -50,6 +97,7 @@ export function formatError(error: FlowError): string { } parts.push(`Error: ${error.message}`); + parts.push(...formatCliFailureLines(error)); return parts.join("\n "); } @@ -82,6 +130,9 @@ export function formatResult(result: FlowResult): string { ); } lines.push(` Error: ${result.error.message}`); + for (const cliLine of formatCliFailureLines(result.error)) { + lines.push(` ${cliLine}`); + } } return lines.join("\n"); diff --git a/src/runner.ts b/src/runner.ts index 51fd76c..3d04392 100644 --- a/src/runner.ts +++ b/src/runner.ts @@ -1,5 +1,6 @@ import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; +import { runCliFlow } from "./cli-runner.js"; import type { FlowError, FlowResult, @@ -45,6 +46,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; } /** @@ -710,6 +715,19 @@ 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") { + return runCliFlow(flow, { + cwd: options?.cwd, + timeout: options?.timeout, + captureLimit: options?.captureLimit, + }); + } + const startTime = Date.now(); const baseUrl = options?.baseUrl ?? DEFAULT_BASE_URL; const timeout = options?.timeout ?? DEFAULT_TIMEOUT; diff --git a/src/types.ts b/src/types.ts index 0dbb5d1..e070550 100644 --- a/src/types.ts +++ b/src/types.ts @@ -59,17 +59,426 @@ 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; + } +} + +// 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: z.string() }) + .strict(); +const StderrContainsAssertionSchema = z + .object({ stderr_contains: z.string() }) + .strict(); +const FileExistsAssertionSchema = z + .object({ file_exists: z.string() }) + .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: z.string() }) + .strict() + .refine( + (value) => compilesAsRegex(value.stdout_matches), + (value) => ({ + message: `Invalid regex pattern "${value.stdout_matches}" for stdout_matches`, + }), + ); + +const StderrMatchesAssertionSchema = z + .object({ stderr_matches: z.string() }) + .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: z.string().optional(), text: z.string().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: z.string().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; + +/** The CLI-surface assertion verbs — anything else is not a CLI assertion. */ +const CLI_ASSERTION_VERBS = [ + "exit_code", + "stdout_contains", + "stdout_matches", + "stderr_contains", + "stderr_matches", + "file_exists", + "file_contains", + "json_output", +] as const; + +/** The web-surface assertion verbs — anything else is not a web assertion. */ +const WEB_ASSERTION_VERBS = [ + "url", + "visible", + "matches", + "not_visible", +] as const; + +/** + * 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. + */ +const CLI_ASSERTION_SCHEMAS: Record = { + exit_code: ExitCodeAssertionSchema, + stdout_contains: StdoutContainsAssertionSchema, + stdout_matches: StdoutMatchesAssertionSchema, + stderr_contains: StderrContainsAssertionSchema, + stderr_matches: StderrMatchesAssertionSchema, + file_exists: FileExistsAssertionSchema, + file_contains: FileContainsAssertionSchema, + json_output: JsonOutputAssertionSchema, +}; + +const WEB_ASSERTION_SCHEMAS: Record = { + url: UrlAssertionSchema, + visible: VisibleAssertionSchema, + matches: MatchesAssertionSchema, + not_visible: NotVisibleAssertionSchema, +}; + +/** 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)) + ); +} + +/** + * 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; + const result = schema.safeParse(step); + if (!result.success) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path, + message: result.error.issues.map((issue) => issue.message).join("; "), + }); + } +} + +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; + } + + const result = schemas[verb].safeParse(assertion); + if (!result.success) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path, + message: result.error.issues.map((issue) => issue.message).join("; "), + }); + } +} + +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` are typed + * loosely (`any`) at this field level on purpose: real verb/shape + * enforcement lives in the superRefine below, keyed off the resolved + * surface, and keeping the field type permissive here avoids narrowing + * FlowSpec's inferred type in a way that would ripple into runner.ts's + * existing StepAction/StepAssertion-typed call sites, which this item does + * not touch. */ -export const FlowSpecSchema = 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), -}); +export const FlowSpecSchema = z + .object({ + name: z.string(), + description: z.string(), + surface: SurfaceSchema.optional().default("web"), + setup: z.array(z.any()).optional(), + steps: z.array(z.any()).min(1), + expect: z.array(z.any()), + }) + .superRefine((flow, ctx) => { + const { surface } = flow; + + validateStepsForSurface(flow.steps, surface, "steps", ctx); + if (flow.setup) { + validateStepsForSurface(flow.setup, surface, "setup", ctx); + } + + if (surface === "web" && flow.expect.length === 0) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["expect"], + message: "expect must contain at least one assertion for surface web", + }); + } else { + validateExpectForSurface(flow.expect, surface, ctx); + } + }); export type FlowSpec = z.infer; @@ -81,14 +490,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..0cb0f49 --- /dev/null +++ b/test/cli-assertions.test.ts @@ -0,0 +1,463 @@ +import { mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { evaluateCliAssertion } 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("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("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..fc101bd --- /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 { FlowSpec } 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 = {}, +): FlowSpec { + return { + name: "cli-flow", + description: "A cli flow", + surface: "cli", + steps, + expect: [], + ...overrides, + } as unknown as FlowSpec; +} + +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..ae6dd92 --- /dev/null +++ b/test/cli-runner.test.ts @@ -0,0 +1,365 @@ +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 { FlowSpec } 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?, captureLimit? } (the merged config shape). + * - Phases in order: create workdir (src/workdir.ts) -> setup (present, + * no-op until a later item fills it in) -> 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: []` (parses today per WI-805 — CLI flows + * with zero assertions have nothing to evaluate) so these tests exercise + * ONLY the steps phase this item implements, independent of whichever + * item wires the assertion-evaluation hook. + */ + +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 = {}, +): FlowSpec { + return { + name: "cli-flow", + description: "A cli flow", + surface: "cli", + steps, + expect: [], + ...overrides, + } as unknown as FlowSpec; +} + +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); + }); + + it("is a no-op for a cli flow's setup block (present but not executed by this item)", async () => { + const cwd = ownedTempDir(); + const flow = cliFlow([{ run: [execPath, "-e", "process.exit(0)"] }], { + setup: [{ 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("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.timeout as the fallback when a step declares no timeout of its own", async () => { + const flow = cliFlow([ + { run: [execPath, "-e", "setTimeout(()=>{},10000)"] }, + ]); + + const result = await runCliFlow(flow, { timeout: 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/config-cli-keys.test.ts b/test/config-cli-keys.test.ts new file mode 100644 index 0000000..8f903e9 --- /dev/null +++ b/test/config-cli-keys.test.ts @@ -0,0 +1,244 @@ +/** + * 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"); + } + }); +}); + +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..476ba80 --- /dev/null +++ b/test/dogfood-plumbing.test.ts @@ -0,0 +1,140 @@ +import { 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: [] +`, + ); + + for (let attempt = 1; attempt <= 2; attempt++) { + execSync("bun run pretest:e2e", { cwd: repoRoot, stdio: "pipe" }); + + expect(existsSync(join(repoRoot, "dist", "index.js"))).toBe(true); + expect(existsSync(binPath)).toBe(true); + + const output = execSync(`"${binPath}" run "${fixtureDir}"`, { + cwd: repoRoot, + encoding: "utf-8", + }); + 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..1e74641 --- /dev/null +++ b/test/dogfood-spec.test.ts @@ -0,0 +1,80 @@ +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"; + +/** + * 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" }); + + 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 = 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..b613727 --- /dev/null +++ b/test/exec-limits.test.ts @@ -0,0 +1,199 @@ +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); + }); +}); + +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); + }); +}); + +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..6d1ea4f --- /dev/null +++ b/test/exec.test.ts @@ -0,0 +1,249 @@ +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