From 3bc92496928733666ae40ffc15b9480e887f5d9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Fr=C3=B8yland?= <81354124+Andreas-Froyland@users.noreply.github.com> Date: Tue, 22 Sep 2026 09:55:45 +0200 Subject: [PATCH 1/3] feat(cli): add doctor, designate and status commands (Task 2.2, part 2) Task 2.2 of the plan, the first slice of the CLI: environment- and project-inspection commands, testable end to end without a candidate, a driver or a real scenario. `run`/`resume` (which need those) land in a following PR alongside the Tauri driver and the sample project. - `release-qa doctor --project --profile [--json]` measures this machine against a profile a project.json declares and reports whether it matches, alongside the raw capabilities and the display kind. - `release-qa designate [--root ] [--json]` and `release-qa status [--root ] [--json]` wrap the existing test-root and ledger primitives from Task 2.1; `--root` defaults to `.release-qa` under the current directory (gitignored) when not given. - `args.ts` never throws on bad input: unknown/missing commands, unknown flags, flags given more than once, a flag with no value (including one whose "value" looks like another flag, which would otherwise be silently swallowed), and extra positional arguments are all reported, not guessed past. - Exit codes match the plan's taxonomy: 0 passed/ready, 2 this machine does not meet the requested profile, 3 everything else that stopped the command (bad usage, an unreadable/invalid project file, an undeclared profile). `status` reports what it finds (undesignated, dirty) as a fact, not a command failure, matching `git status`. 1 (a scenario the candidate failed) is defined but not yet reachable, since no command runs a scenario yet. `--candidate` verification and cancellation, both mentioned in the plan's test list, apply to `run` and are deferred with it. - The CLI is proven to run as `node packages/qa/src/cli/main.ts ...` from a separate process (test/cli/executable.test.ts), not just imported into the test worker, exercising the malformed-args and unsupported-profile cases the plan calls out through the executable specifically. - README documents the new commands, the default root, output modes and exit codes, and its Status section no longer says the tool is unbuilt. Test-first throughout (57 new tests). Mutation-checked with a throwaway script covering the trickiest branches in each new file (duplicate/unknown/missing flags, a flag's value looking like another flag, JSON/schema failures, unknown profile, mismatch reporting, swallowed errors, wrong exit codes); one survivor (a flag's value looking like another flag was not distinctly tested) was closed with a new test, then reconfirmed killed. Clean npm ci, typecheck, and full suite (512 passed, 2 skipped); no leaked processes, temp directories, or home markers. Co-Authored-By: Claude Sonnet 5 --- .gitignore | 2 + README.md | 20 ++- packages/qa/src/cli/args.ts | 102 ++++++++++++++ packages/qa/src/cli/doctor.ts | 36 +++++ packages/qa/src/cli/environment-commands.ts | 48 +++++++ packages/qa/src/cli/main.ts | 114 +++++++++++++++ packages/qa/src/cli/project.ts | 28 ++++ packages/qa/test/cli/args.test.ts | 84 +++++++++++ packages/qa/test/cli/doctor.test.ts | 50 +++++++ .../qa/test/cli/environment-commands.test.ts | 70 ++++++++++ packages/qa/test/cli/executable.test.ts | 100 ++++++++++++++ packages/qa/test/cli/main.test.ts | 130 ++++++++++++++++++ packages/qa/test/cli/project.test.ts | 75 ++++++++++ 13 files changed, 858 insertions(+), 1 deletion(-) create mode 100644 packages/qa/src/cli/args.ts create mode 100644 packages/qa/src/cli/doctor.ts create mode 100644 packages/qa/src/cli/environment-commands.ts create mode 100644 packages/qa/src/cli/main.ts create mode 100644 packages/qa/src/cli/project.ts create mode 100644 packages/qa/test/cli/args.test.ts create mode 100644 packages/qa/test/cli/doctor.test.ts create mode 100644 packages/qa/test/cli/environment-commands.test.ts create mode 100644 packages/qa/test/cli/executable.test.ts create mode 100644 packages/qa/test/cli/main.test.ts create mode 100644 packages/qa/test/cli/project.test.ts diff --git a/.gitignore b/.gitignore index 76962b1..a76f7ce 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,5 @@ experiments/**/output/ app-icon.png # Tauri regenerates these schemas on every build examples/**/src-tauri/gen/ +# the CLI's default test-root directory (release-qa designate/status use it when --root is not given) +.release-qa/ diff --git a/README.md b/README.md index b9ca9a2..39b66e4 100644 --- a/README.md +++ b/README.md @@ -11,12 +11,30 @@ The first targets are Tauri applications on Windows and Linux, with Dot X as the ## Status -Stage 0 (proving the assumptions) is under way; the tool itself is not built yet. Results so far: +Stage 0 (proving the assumptions) is complete. The runner (environment checks, scenario execution, the durable run +journal) and a first slice of the CLI (`doctor`, `designate`, `status`) exist; running an actual scenario from the +CLI (`run`, `resume`), the Tauri driver, the dashboard and the GitHub integration do not yet. - [Native automation](docs/decisions/native-automation.md): unchanged packaged Tauri apps can be driven on Windows and Ubuntu. The Dot X feasibility check is still open. - [GitHub merge gate](docs/decisions/github-gate.md): a no-service required check works, with documented design changes and unproven items. - [Tool layout and defaults](docs/decisions/tool-layout.md): runtime, package manager, baselines and repository structure. +## CLI + +Run it straight from a checkout; no build step (see [tool layout](docs/decisions/tool-layout.md)): + +```sh +node packages/qa/src/cli/main.ts doctor --project qa/project.json --profile windows [--json] +node packages/qa/src/cli/main.ts designate [--root ] [--json] # marks a directory safe to install and delete into +node packages/qa/src/cli/main.ts status [--root ] [--json] # reports what a designated root holds, dirty or clean +``` + +`--root` defaults to `.release-qa` under the current directory (gitignored) when not given. Every command prints to +stdout on success and to stderr on failure; `--json` switches both to one line of machine-readable JSON. Exit codes: +`0` passed/ready, `1` a scenario the candidate failed (not reachable yet — no command runs a scenario), `2` this +machine does not meet a requested profile, `3` anything else that stopped the command (bad usage, an unreadable or +invalid project file, a profile the project does not declare). + ## Development Requires Node.js 22.18 or newer (the CLI needs no build step because Node strips types directly from that version on). diff --git a/packages/qa/src/cli/args.ts b/packages/qa/src/cli/args.ts new file mode 100644 index 0000000..4b7a7a5 --- /dev/null +++ b/packages/qa/src/cli/args.ts @@ -0,0 +1,102 @@ +/** + * Parses `process.argv.slice(2)`. Never throws: a malformed invocation is a `{ ok: false }` result the caller turns + * into an exit code, not an exception that would print a stack trace instead of a usable message. + */ + +export interface DoctorCommand { + name: 'doctor'; + project: string; + profile: string; + json: boolean; +} + +export interface DesignateCommand { + name: 'designate'; + /** Absent means the caller applies its own default; parsing does not know or guess a working directory. */ + root: string | undefined; + json: boolean; +} + +export interface StatusCommand { + name: 'status'; + root: string | undefined; + json: boolean; +} + +export type Command = DoctorCommand | DesignateCommand | StatusCommand; + +export type ParsedArgs = { ok: true; command: Command } | { ok: false; error: string }; + +interface Spec { + /** Flags that must be given a value. */ + required: readonly string[]; + /** Flags that may be given a value but need not be. */ + optional: readonly string[]; +} + +const SPECS: Record = { + doctor: { required: ['project', 'profile'], optional: [] }, + designate: { required: [], optional: ['root'] }, + status: { required: [], optional: ['root'] }, +}; + +const COMMAND_NAMES = Object.keys(SPECS) as Command['name'][]; + +export function parseArgs(argv: readonly string[]): ParsedArgs { + const [name, ...rest] = argv; + if (name === undefined) return fail(`expected a command: ${COMMAND_NAMES.join(', ')}`); + if (!isCommandName(name)) return fail(`unknown command "${name}"; expected one of ${COMMAND_NAMES.join(', ')}`); + + const flags = parseFlags(rest, SPECS[name]); + if (!flags.ok) return flags; + + switch (name) { + case 'doctor': + return { ok: true, command: { name, project: flags.values.project as string, profile: flags.values.profile as string, json: flags.json } }; + case 'designate': + case 'status': + return { ok: true, command: { name, root: flags.values.root as string | undefined, json: flags.json } }; + } +} + +function isCommandName(value: string): value is Command['name'] { + return (COMMAND_NAMES as string[]).includes(value); +} + +type FlagResult = { ok: true; values: Record; json: boolean } | { ok: false; error: string }; + +/** `--json` is a boolean flag common to every command; every other flag takes exactly one value. */ +function parseFlags(args: readonly string[], spec: Spec): FlagResult { + const known = new Set([...spec.required, ...spec.optional]); + const values: Record = {}; + let json = false; + const positional: string[] = []; + + for (let i = 0; i < args.length; i += 1) { + const token = args[i]; + if (token === undefined) break; + if (token === '--json') { + json = true; + continue; + } + if (!token.startsWith('--')) { + positional.push(token); + continue; + } + const flag = token.slice(2); + if (!known.has(flag)) return fail(`unknown flag "--${flag}"`); + if (Object.hasOwn(values, flag)) return fail(`--${flag} was given more than once`); + const value = args[i + 1]; + if (value === undefined || value.startsWith('--')) return fail(`--${flag} needs a value`); + values[flag] = value; + i += 1; + } + + if (positional.length > 0) return fail(`unexpected argument "${positional[0]}"`); + for (const flag of spec.required) if (values[flag] === undefined) return fail(`--${flag} is required`); + return { ok: true, values, json }; +} + +function fail(error: string): { ok: false; error: string } { + return { ok: false, error }; +} diff --git a/packages/qa/src/cli/doctor.ts b/packages/qa/src/cli/doctor.ts new file mode 100644 index 0000000..03aa1c5 --- /dev/null +++ b/packages/qa/src/cli/doctor.ts @@ -0,0 +1,36 @@ +import { inspectEnvironment, type EnvironmentProbes } from '../runner/environment.ts'; +import type { DisplayKind } from '../runner/display.ts'; +import type { Project } from '../model/project.ts'; +import type { MeasuredEnvironment } from '../model/result.ts'; + +export interface DoctorReport { + /** Whether this machine is what the requested profile expects; not whether any particular suite can run. */ + ok: boolean; + profile: string; + machine: MeasuredEnvironment; + display: { kind: DisplayKind; detail: string }; + mismatch?: string; +} + +export type DoctorResult = { ok: true; report: DoctorReport } | { ok: false; error: string }; + +/** Measures this machine against a profile the project declares. Never throws: an unknown profile is a result, not an exception. */ +export async function runDoctor(project: Project, profileId: string, probes?: EnvironmentProbes): Promise { + const profile = project.profiles.find((p) => p.id === profileId); + if (profile === undefined) { + const known = project.profiles.map((p) => p.id).join(', ') || '(none declared)'; + return { ok: false, error: `profile "${profileId}" is not defined by this project; known profiles: ${known}` }; + } + + const inspected = await inspectEnvironment(profile, probes); + return { + ok: true, + report: { + ok: inspected.profileMismatch === undefined, + profile: profileId, + machine: inspected.environment, + display: inspected.display, + ...(inspected.profileMismatch === undefined ? {} : { mismatch: inspected.profileMismatch }), + }, + }; +} diff --git a/packages/qa/src/cli/environment-commands.ts b/packages/qa/src/cli/environment-commands.ts new file mode 100644 index 0000000..4ac47d9 --- /dev/null +++ b/packages/qa/src/cli/environment-commands.ts @@ -0,0 +1,48 @@ +import { checkTestRoot, designateTestRoot, readDirty, readLedger, type OwnedResource, type TestRootCheck } from '../runner/resources.ts'; + +const message = (error: unknown): string => (error instanceof Error ? error.message : String(error)); + +/** `options.home` is a test seam only: production callers never pass it, so the real home directory is always used. */ +export interface EnvironmentCommandOptions { + home?: string; +} + +export type DesignateResult = { ok: true; root: string } | { ok: false; error: string }; + +/** Marks a directory as somewhere the runner may install, launch and delete. Never throws. */ +export async function runDesignate(root: string, options: EnvironmentCommandOptions = {}): Promise { + try { + await designateTestRoot(root, options); + return { ok: true, root }; + } catch (error) { + return { ok: false, error: message(error) }; + } +} + +export interface StatusReport { + root: string; + designated: boolean; + /** Present only when `designated` is false: which check failed. */ + reason?: Exclude['reason']; + /** Present only when the environment was left dirty by an earlier run. */ + dirty?: string; + owned: OwnedResource[]; +} + +export type StatusResult = { ok: true; report: StatusReport } | { ok: false; error: string }; + +/** + * Reports what this root is and holds, without changing anything. An undesignated or malformed root is a normal + * report (`designated: false`), not an error; only a root this cannot read at all is an error. + */ +export async function runStatus(root: string, options: EnvironmentCommandOptions = {}): Promise { + const check = await checkTestRoot(root, options); + if (!check.ok) return { ok: true, report: { root, designated: false, reason: check.reason, owned: [] } }; + + try { + const [dirty, owned] = await Promise.all([readDirty(check.root), readLedger(check.root)]); + return { ok: true, report: { root: check.root, designated: true, owned, ...(dirty === undefined ? {} : { dirty }) } }; + } catch (error) { + return { ok: false, error: `could not read the state of ${check.root}: ${message(error)}` }; + } +} diff --git a/packages/qa/src/cli/main.ts b/packages/qa/src/cli/main.ts new file mode 100644 index 0000000..cf1773b --- /dev/null +++ b/packages/qa/src/cli/main.ts @@ -0,0 +1,114 @@ +import { join } from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { parseArgs } from './args.ts'; +import { runDoctor, type DoctorReport } from './doctor.ts'; +import { runDesignate, runStatus, type StatusReport } from './environment-commands.ts'; +import { loadProject } from './project.ts'; + +/** + * `0` passed. `1` a scenario the candidate failed (not yet reachable: no command runs a scenario in this build). + * `2` a missing prerequisite the tester, not the tool, must resolve (e.g. this machine does not match a profile). + * `3` everything else that stops the command: bad usage, a file that cannot be read, an unknown profile. + */ +export const EXIT = { ok: 0, scenarioFailure: 1, missingPrerequisite: 2, infrastructure: 3 } as const; + +export interface Io { + log(line: string): void; + error(line: string): void; +} + +const defaultIo: Io = { log: (line) => console.log(line), error: (line) => console.error(line) }; + +/** Runs one CLI invocation and returns the process exit code; never throws and never touches `process` itself. */ +export async function main(argv: readonly string[], io: Io = defaultIo, cwd: () => string = () => process.cwd()): Promise { + const parsed = parseArgs(argv); + if (!parsed.ok) { + io.error(parsed.error); + return EXIT.infrastructure; + } + const { command } = parsed; + + switch (command.name) { + case 'doctor': { + const loaded = await loadProject(command.project); + if (!loaded.ok) { + io.error(command.json ? JSON.stringify({ ok: false, error: loaded.error, issues: loaded.issues ?? [] }) : loaded.error); + return EXIT.infrastructure; + } + const result = await runDoctor(loaded.project, command.profile); + if (!result.ok) { + io.error(command.json ? JSON.stringify({ ok: false, error: result.error }) : result.error); + return EXIT.infrastructure; + } + printDoctor(io, command.json, result.report); + return result.report.ok ? EXIT.ok : EXIT.missingPrerequisite; + } + + case 'designate': { + const root = command.root ?? defaultRoot(cwd); + const result = await runDesignate(root); + if (!result.ok) { + io.error(command.json ? JSON.stringify({ ok: false, error: result.error }) : result.error); + return EXIT.infrastructure; + } + io.log(command.json ? JSON.stringify(result) : `designated ${result.root}`); + return EXIT.ok; + } + + case 'status': { + const root = command.root ?? defaultRoot(cwd); + const result = await runStatus(root); + if (!result.ok) { + io.error(command.json ? JSON.stringify({ ok: false, error: result.error }) : result.error); + return EXIT.infrastructure; + } + // What status finds (undesignated, dirty) is a fact it reports, never a failure of the status command itself. + printStatus(io, command.json, result.report); + return EXIT.ok; + } + } +} + +const defaultRoot = (cwd: () => string): string => join(cwd(), '.release-qa'); + +function printDoctor(io: Io, json: boolean, report: DoctorReport): void { + if (json) { + io.log(JSON.stringify(report)); + return; + } + io.log( + [ + `profile: ${report.profile}`, + `ready: ${report.ok}`, + `os: ${report.machine.os} ${report.machine.osVersion}`, + `arch: ${report.machine.arch}`, + `capabilities: ${report.machine.capabilities.join(', ') || '(none)'}`, + `display: ${report.display.kind} (${report.display.detail})`, + ...(report.mismatch === undefined ? [] : [`mismatch: ${report.mismatch}`]), + ].join('\n'), + ); +} + +function printStatus(io: Io, json: boolean, report: StatusReport): void { + if (json) { + io.log(JSON.stringify(report)); + return; + } + io.log( + [ + `root: ${report.root}`, + `designated: ${report.designated}`, + ...(report.reason === undefined ? [] : [`reason: ${report.reason}`]), + ...(report.dirty === undefined ? [] : [`dirty: ${report.dirty}`]), + `owned: ${report.owned.length} resource(s)`, + ].join('\n'), + ); +} + +// Runs only when this file is the process's entry point (`node packages/qa/src/cli/main.ts ...`), never when a test +// imports it as a module. +if (process.argv[1] !== undefined && pathToFileURL(process.argv[1]).href === import.meta.url) { + main(process.argv.slice(2)).then((code) => { + process.exitCode = code; + }); +} diff --git a/packages/qa/src/cli/project.ts b/packages/qa/src/cli/project.ts new file mode 100644 index 0000000..8b29c02 --- /dev/null +++ b/packages/qa/src/cli/project.ts @@ -0,0 +1,28 @@ +import { readFile } from 'node:fs/promises'; +import { parseProject, type Project } from '../model/project.ts'; +import type { ValidationIssue } from '../model/validate.ts'; + +export type LoadedProject = { ok: true; project: Project; path: string } | { ok: false; error: string; issues?: readonly ValidationIssue[] }; + +const message = (error: unknown): string => (error instanceof Error ? error.message : String(error)); + +/** Reads and validates a `qa/project.json`. Never throws: every way the file can be wrong becomes a `{ ok: false }`. */ +export async function loadProject(path: string): Promise { + let text: string; + try { + text = await readFile(path, 'utf8'); + } catch (error) { + return { ok: false, error: `could not read ${path}: ${message(error)}` }; + } + + let parsed: unknown; + try { + parsed = JSON.parse(text); + } catch (error) { + return { ok: false, error: `${path} is not valid JSON: ${message(error)}` }; + } + + const result = parseProject(parsed); + if (!result.ok) return { ok: false, error: `${path}: ${result.error.message}`, issues: result.error.issues }; + return { ok: true, project: result.value, path }; +} diff --git a/packages/qa/test/cli/args.test.ts b/packages/qa/test/cli/args.test.ts new file mode 100644 index 0000000..10ef2be --- /dev/null +++ b/packages/qa/test/cli/args.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, test } from 'vitest'; +import { parseArgs } from '../../src/cli/args.ts'; + +const err = (argv: string[]): string => { + const result = parseArgs(argv); + if (result.ok) throw new Error(`expected an error, got ${JSON.stringify(result.command)}`); + return result.error; +}; + +describe('no command', () => { + test('an empty argument list names the valid commands', () => { + expect(err([])).toMatch(/doctor.*designate.*status/); + }); + + test('an unrecognised first word names it and the valid commands', () => { + expect(err(['fly'])).toMatch(/"fly"/); + expect(err(['fly'])).toMatch(/doctor.*designate.*status/); + }); +}); + +describe('doctor', () => { + test('reads --project and --profile, and defaults json to false', () => { + const result = parseArgs(['doctor', '--project', 'qa/project.json', '--profile', 'windows']); + expect(result).toEqual({ ok: true, command: { name: 'doctor', project: 'qa/project.json', profile: 'windows', json: false } }); + }); + + test('--json sets json to true', () => { + const result = parseArgs(['doctor', '--project', 'p.json', '--profile', 'w', '--json']); + expect(result).toEqual({ ok: true, command: { name: 'doctor', project: 'p.json', profile: 'w', json: true } }); + }); + + test('flags may come in any order', () => { + const result = parseArgs(['doctor', '--json', '--profile', 'w', '--project', 'p.json']); + expect(result).toEqual({ ok: true, command: { name: 'doctor', project: 'p.json', profile: 'w', json: true } }); + }); + + test.each([['--project'], ['--profile']])('missing %s is an error naming it', (flag) => { + const argv = flag === '--project' ? ['doctor', '--profile', 'w'] : ['doctor', '--project', 'p.json']; + expect(err(argv)).toMatch(new RegExp(flag.replace('--', ''))); + }); + + test('a flag this command does not have is an error', () => { + expect(err(['doctor', '--project', 'p.json', '--profile', 'w', '--root', 'x'])).toMatch(/root/); + }); + + test('a flag repeated is an error, not silently the last value', () => { + expect(err(['doctor', '--project', 'a.json', '--project', 'b.json', '--profile', 'w'])).toMatch(/project/); + }); + + test('a flag with nothing after it is an error', () => { + expect(err(['doctor', '--project'])).toMatch(/project/); + }); + + test("a flag's value must not look like another flag, so a missing value is never silently swallowed", () => { + // Without this guard, --project would swallow "--profile" as its value and "w" would be left over as an + // unexplained extra argument, hiding that --profile itself never got a value. + expect(err(['doctor', '--project', '--profile', 'w'])).toMatch(/project/); + }); + + test('an extra positional argument is an error', () => { + expect(err(['doctor', '--project', 'p.json', '--profile', 'w', 'extra'])).toMatch(/extra/); + }); +}); + +describe('designate', () => { + test('--root is optional; absent means the caller decides the default', () => { + expect(parseArgs(['designate'])).toEqual({ ok: true, command: { name: 'designate', root: undefined, json: false } }); + }); + + test('--root is read when given', () => { + expect(parseArgs(['designate', '--root', '.release-qa'])).toEqual({ ok: true, command: { name: 'designate', root: '.release-qa', json: false } }); + }); +}); + +describe('status', () => { + test('reads --root and --json like designate', () => { + const result = parseArgs(['status', '--root', 'r', '--json']); + expect(result).toEqual({ ok: true, command: { name: 'status', root: 'r', json: true } }); + }); + + test('--root is optional here too', () => { + expect(parseArgs(['status'])).toEqual({ ok: true, command: { name: 'status', root: undefined, json: false } }); + }); +}); diff --git a/packages/qa/test/cli/doctor.test.ts b/packages/qa/test/cli/doctor.test.ts new file mode 100644 index 0000000..fef9ead --- /dev/null +++ b/packages/qa/test/cli/doctor.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, test } from 'vitest'; +import { runDoctor } from '../../src/cli/doctor.ts'; +import type { Project } from '../../src/model/project.ts'; +import { hostOs, hostProfile } from '../fixtures/processes.ts'; + +const project = (overrides: Partial = {}): Project => ({ + schemaVersion: 1, + projectId: 'sample', + releaseBranch: 'main', + profiles: [hostProfile(), { id: 'other', os: hostOs() === 'windows' ? 'linux' : 'windows', arch: 'x86_64' }], + requirements: [], + suites: [], + scenarioFiles: [], + lifecycleModule: 'lifecycle.ts', + workflows: { prepare: 'a.yml', gate: 'b.yml', publish: 'c.yml' }, + markers: { releaseNotes: 'release-notes', qa: 'qa' }, + ...overrides, +}); + +const probes = (display: boolean, audio: boolean) => ({ display: async () => display, audio: async () => audio }); + +describe('doctor', () => { + test('an unknown profile id is a clear error listing the known ones', async () => { + const result = await runDoctor(project(), 'macos', probes(true, true)); + expect(result.ok).toBe(false); + expect(!result.ok && result.error).toContain(hostOs()); + expect(!result.ok && result.error).toContain('macos'); + }); + + test('a machine matching the requested profile is reported ready', async () => { + const result = await runDoctor(project(), hostProfile().id, probes(true, true)); + expect(result.ok).toBe(true); + expect(result.ok && result.report.ok).toBe(true); + expect(result.ok && result.report.mismatch).toBeUndefined(); + expect(result.ok && result.report.machine.capabilities).toEqual(expect.arrayContaining(['audio', 'display'])); + }); + + test('a profile this machine does not match is reported, not silently passed', async () => { + const result = await runDoctor(project(), 'other', probes(true, true)); + expect(result.ok).toBe(true); + expect(result.ok && result.report.ok).toBe(false); + expect(result.ok && result.report.mismatch).toBeDefined(); + }); + + test('reports the display kind alongside the raw capability list', async () => { + const result = await runDoctor(project(), hostProfile().id, { display: async () => false, audio: async () => false }); + expect(result.ok && result.report.display.kind).toBe('none'); + expect(result.ok && result.report.machine.capabilities).toEqual([]); + }); +}); diff --git a/packages/qa/test/cli/environment-commands.test.ts b/packages/qa/test/cli/environment-commands.test.ts new file mode 100644 index 0000000..2c7c777 --- /dev/null +++ b/packages/qa/test/cli/environment-commands.test.ts @@ -0,0 +1,70 @@ +import { mkdir, writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { afterEach, describe, expect, test } from 'vitest'; +import { runDesignate, runStatus } from '../../src/cli/environment-commands.ts'; +import { spawnOwned, markDirty } from '../../src/runner/resources.ts'; +import { cleanUpProcessesAndRoots, makeTempDir, makeTestRoot, trackProcess } from '../fixtures/processes.ts'; + +afterEach(cleanUpProcessesAndRoots); + +describe('designate', () => { + test('marks a fresh directory as a test root', async () => { + const root = await makeTestRoot(false); + const result = await runDesignate(root); + expect(result).toEqual({ ok: true, root }); + expect((await runStatus(root)).ok).toBe(true); + }); + + // A throwaway "home" (never the real one): a wrong implementation would write into it, and that is exactly the + // mistake this test exists to catch, without any risk to the machine actually running the suite. + test('refuses the home directory, and reports why, instead of throwing', async () => { + const home = await makeTempDir('qa-cli-home-'); + const result = await runDesignate(home, { home }); + expect(result.ok).toBe(false); + expect(!result.ok && result.error).toMatch(/unsafe/i); + }); +}); + +describe('status', () => { + test('an undesignated directory is reported as such, not as an error', async () => { + const root = await makeTestRoot(false); + const result = await runStatus(root); + expect(result).toEqual({ ok: true, report: { root, designated: false, reason: 'missing-marker', owned: [] } }); + }); + + test('a designated, clean, empty root is reported clean', async () => { + const root = await makeTestRoot(); + const result = await runStatus(root); + expect(result).toEqual({ ok: true, report: { root, designated: true, owned: [] } }); + }); + + test('reports what the root owns', async () => { + const root = await makeTestRoot(); + const child = await spawnOwned(root, 'helper', process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { stdio: 'ignore' }); + trackProcess(child); + const result = await runStatus(root); + expect(result.ok && result.report.owned).toHaveLength(1); + }); + + test('reports why the environment is dirty', async () => { + const root = await makeTestRoot(); + await markDirty(root, 'a previous run left the app installed'); + const result = await runStatus(root); + expect(result).toEqual({ ok: true, report: { root, designated: true, dirty: 'a previous run left the app installed', owned: [] } }); + }); + + test('an unreadable ledger is a command error, not a silently empty report', async () => { + const root = await makeTestRoot(); + await mkdir(join(root, '.release-qa-owned.json')); + const result = await runStatus(root); + expect(result.ok).toBe(false); + }); + + test('a directory that is not a directory at all is reported, not thrown', async () => { + const root = await makeTestRoot(); + const file = join(root, 'not-a-root'); + await writeFile(file, 'x'); + const result = await runStatus(file); + expect(result).toEqual({ ok: true, report: { root: file, designated: false, reason: 'not-a-directory', owned: [] } }); + }); +}); diff --git a/packages/qa/test/cli/executable.test.ts b/packages/qa/test/cli/executable.test.ts new file mode 100644 index 0000000..e3f3d5c --- /dev/null +++ b/packages/qa/test/cli/executable.test.ts @@ -0,0 +1,100 @@ +// Proves the CLI genuinely runs as `node packages/qa/src/cli/main.ts ...`, with no build step, from another process +// (not just imported as a module inside the test worker) — the thing test/no-build.test.ts proves for the package +// as a whole, exercised here specifically through the paths the plan calls out: malformed args and an unsupported +// profile. "Missing candidate" and "cancellation" apply to `run`, which this build does not implement yet. +import { execFile } from 'node:child_process'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { dirname, join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { fileURLToPath } from 'node:url'; +import { promisify } from 'node:util'; +import { afterEach, describe, expect, test } from 'vitest'; +import { EXIT } from '../../src/cli/main.ts'; +import { hostOs } from '../fixtures/processes.ts'; + +const run = promisify(execFile); +const cliPath = join(dirname(fileURLToPath(import.meta.url)), '..', '..', 'src', 'cli', 'main.ts'); + +const dirs: string[] = []; +afterEach(async () => { + await Promise.allSettled(dirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); +}); + +async function run_(...args: string[]): Promise<{ stdout: string; stderr: string; code: number }> { + try { + const { stdout, stderr } = await run(process.execPath, [cliPath, ...args]); + return { stdout, stderr, code: 0 }; + } catch (error) { + const failure = error as { stdout: string; stderr: string; code: number }; + return { stdout: failure.stdout, stderr: failure.stderr, code: failure.code }; + } +} + +describe('the CLI executable', () => { + test('malformed args: no command at all prints usage on stderr and exits 3, printing nothing on stdout', async () => { + const result = await run_(); + expect(result.code).toBe(EXIT.infrastructure); + expect(result.stdout).toBe(''); + expect(result.stderr).toMatch(/doctor/); + }); + + test('malformed args: an unknown flag exits 3 naming it', async () => { + const result = await run_('doctor', '--project', 'x', '--profile', 'y', '--nope', 'z'); + expect(result.code).toBe(EXIT.infrastructure); + expect(result.stderr).toContain('nope'); + }); + + test('an unsupported profile exits 3 and names the profiles the project does declare', async () => { + const dir = await mkdtemp(join(tmpdir(), 'qa-cli-exe-')); + dirs.push(dir); + const path = join(dir, 'project.json'); + await writeFile( + path, + JSON.stringify({ + schemaVersion: 1, + projectId: 'sample', + releaseBranch: 'main', + profiles: [{ id: 'here', os: hostOs(), arch: 'x86_64' }], + requirements: [{ key: 'here/persistence', mode: 'automated', title: 'Persists data across restarts', capabilities: [] }], + suites: [], + scenarioFiles: [], + lifecycleModule: 'lifecycle.ts', + workflows: { prepare: 'a.yml', gate: 'b.yml', publish: 'c.yml' }, + markers: { releaseNotes: 'release-notes', qa: 'qa' }, + }), + ); + + const result = await run_('doctor', '--project', path, '--profile', 'nowhere', '--json'); + + expect(result.code).toBe(EXIT.infrastructure); + expect(result.stderr).toContain('here'); + }); + + test('a machine matching the profile exits 0 with a JSON report on stdout', async () => { + const dir = await mkdtemp(join(tmpdir(), 'qa-cli-exe-')); + dirs.push(dir); + const path = join(dir, 'project.json'); + await writeFile( + path, + JSON.stringify({ + schemaVersion: 1, + projectId: 'sample', + releaseBranch: 'main', + profiles: [{ id: 'here', os: hostOs(), arch: 'x86_64' }], + requirements: [{ key: 'here/persistence', mode: 'automated', title: 'Persists data across restarts', capabilities: [] }], + suites: [], + scenarioFiles: [], + lifecycleModule: 'lifecycle.ts', + workflows: { prepare: 'a.yml', gate: 'b.yml', publish: 'c.yml' }, + markers: { releaseNotes: 'release-notes', qa: 'qa' }, + }), + ); + + const result = await run_('doctor', '--project', path, '--profile', 'here', '--json'); + + expect(result.code).toBe(EXIT.ok); + expect(result.stderr).toBe(''); + const printed = JSON.parse(result.stdout) as { ok: boolean; profile: string }; + expect(printed).toMatchObject({ ok: true, profile: 'here' }); + }); +}); diff --git a/packages/qa/test/cli/main.test.ts b/packages/qa/test/cli/main.test.ts new file mode 100644 index 0000000..4add300 --- /dev/null +++ b/packages/qa/test/cli/main.test.ts @@ -0,0 +1,130 @@ +import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, test } from 'vitest'; +import { EXIT, main } from '../../src/cli/main.ts'; +import { hostOs } from '../fixtures/processes.ts'; + +const dirs: string[] = []; +afterEach(async () => { + await Promise.allSettled(dirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); +}); + +async function makeDir(): Promise { + const dir = await import('node:fs/promises').then((fs) => fs.mkdtemp(join(tmpdir(), 'qa-cli-main-'))); + dirs.push(dir); + return dir; +} + +function io(): { log: string[]; error: string[]; sink: { log: (s: string) => void; error: (s: string) => void } } { + const log: string[] = []; + const error: string[] = []; + return { log, error, sink: { log: (s) => log.push(s), error: (s) => error.push(s) } }; +} + +const validProject = { + schemaVersion: 1, + projectId: 'sample', + releaseBranch: 'main', + profiles: [{ id: 'here', os: hostOs(), arch: 'x86_64' }], + requirements: [{ key: 'here/persistence', mode: 'automated', title: 'Persists data across restarts', capabilities: [] }], + suites: [], + scenarioFiles: [], + lifecycleModule: 'lifecycle.ts', + workflows: { prepare: 'a.yml', gate: 'b.yml', publish: 'c.yml' }, + markers: { releaseNotes: 'release-notes', qa: 'qa' }, +}; + +describe('bad usage', () => { + test('no command is exit 3 with a message on stderr, nothing on stdout', async () => { + const out = io(); + const code = await main([], out.sink); + expect(code).toBe(EXIT.infrastructure); + expect(out.log).toEqual([]); + expect(out.error.join(' ')).toMatch(/doctor/); + }); + + test('an unknown command is exit 3', async () => { + const out = io(); + expect(await main(['launch'], out.sink)).toBe(EXIT.infrastructure); + }); +}); + +describe('doctor', () => { + test('a missing project file is exit 3', async () => { + const out = io(); + const dir = await makeDir(); + const code = await main(['doctor', '--project', join(dir, 'nope.json'), '--profile', 'here'], out.sink); + expect(code).toBe(EXIT.infrastructure); + expect(out.error.join(' ')).toContain('nope.json'); + }); + + test('an unsupported profile is exit 3, naming the profiles the project does declare', async () => { + const out = io(); + const dir = await makeDir(); + const path = join(dir, 'project.json'); + await writeFile(path, JSON.stringify(validProject)); + const code = await main(['doctor', '--project', path, '--profile', 'nowhere'], out.sink); + expect(code).toBe(EXIT.infrastructure); + expect(out.error.join(' ')).toContain('here'); + }); + + test('a machine matching the profile is exit 0 and prints readiness', async () => { + const out = io(); + const dir = await makeDir(); + const path = join(dir, 'project.json'); + await writeFile(path, JSON.stringify(validProject)); + const code = await main(['doctor', '--project', path, '--profile', 'here', '--json'], out.sink); + expect(code).toBe(EXIT.ok); + const printed = JSON.parse(out.log.join('')) as { ok: boolean }; + expect(printed.ok).toBe(true); + }); + + test('a profile this machine does not match is exit 2, not 0 or 1', async () => { + const out = io(); + const dir = await makeDir(); + const path = join(dir, 'project.json'); + const other = hostOs() === 'windows' ? 'linux' : 'windows'; + await writeFile( + path, + JSON.stringify({ + ...validProject, + profiles: [{ id: 'there', os: other, arch: 'x86_64' }], + requirements: [{ key: 'there/persistence', mode: 'automated', title: 'Persists data across restarts', capabilities: [] }], + }), + ); + const code = await main(['doctor', '--project', path, '--profile', 'there', '--json'], out.sink); + expect(code).toBe(EXIT.missingPrerequisite); + }); +}); + +describe('designate and status', () => { + test('designating then checking status round-trips, defaulting the root to .release-qa under the given directory', async () => { + const out = io(); + const dir = await makeDir(); + expect(await main(['designate'], out.sink, () => dir)).toBe(EXIT.ok); + const statusOut = io(); + expect(await main(['status', '--json'], statusOut.sink, () => dir)).toBe(EXIT.ok); + const report = JSON.parse(statusOut.log.join('')) as { designated: boolean; root: string }; + expect(report.designated).toBe(true); + expect(report.root).toBe(join(dir, '.release-qa')); + }); + + test('an explicit --root overrides the default', async () => { + const out = io(); + const dir = await makeDir(); + const explicit = join(dir, 'somewhere-else'); + await mkdir(explicit); + expect(await main(['designate', '--root', explicit], out.sink, () => dir)).toBe(EXIT.ok); + const statusOut = io(); + await main(['status', '--root', explicit, '--json'], statusOut.sink, () => dir); + expect((JSON.parse(statusOut.log.join('')) as { root: string }).root).toBe(explicit); + }); + + test('status on an undesignated root is exit 0: it reports a fact, it is not itself a failure', async () => { + const out = io(); + const dir = await makeDir(); + expect(await main(['status'], out.sink, () => dir)).toBe(EXIT.ok); + expect(out.log.join(' ')).toContain('designated: false'); + }); +}); diff --git a/packages/qa/test/cli/project.test.ts b/packages/qa/test/cli/project.test.ts new file mode 100644 index 0000000..db4fcfd --- /dev/null +++ b/packages/qa/test/cli/project.test.ts @@ -0,0 +1,75 @@ +import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { afterEach, describe, expect, test } from 'vitest'; +import { loadProject } from '../../src/cli/project.ts'; + +const dirs: string[] = []; +afterEach(async () => { + await Promise.allSettled(dirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); +}); + +async function makeDir(): Promise { + const dir = await import('node:fs/promises').then((fs) => fs.mkdtemp(join(tmpdir(), 'qa-cli-project-'))); + dirs.push(dir); + return dir; +} + +const validProject = { + schemaVersion: 1, + projectId: 'sample', + releaseBranch: 'main', + profiles: [{ id: 'windows', os: 'windows', arch: 'x86_64' }], + requirements: [{ key: 'windows/persistence', mode: 'automated', title: 'Persists data across restarts', capabilities: [] }], + suites: [{ id: 'default', requirements: ['windows/persistence'] }], + scenarioFiles: ['persistence.spec.ts'], + lifecycleModule: 'lifecycle.ts', + workflows: { prepare: 'qa-prepare.yml', gate: 'qa-gate.yml', publish: 'qa-publish.yml' }, + markers: { releaseNotes: 'release-notes', qa: 'qa' }, +}; + +describe('loading a project file', () => { + test('a valid file is parsed into a Project', async () => { + const dir = await makeDir(); + const path = join(dir, 'project.json'); + await writeFile(path, JSON.stringify(validProject)); + + const result = await loadProject(path); + + expect(result).toMatchObject({ ok: true, path }); + expect(result.ok && result.project.projectId).toBe('sample'); + }); + + test('a missing file is a clear error, not a thrown exception', async () => { + const dir = await makeDir(); + const result = await loadProject(join(dir, 'nope.json')); + expect(result.ok).toBe(false); + expect(!result.ok && result.error).toContain('nope.json'); + }); + + test('a file that is not JSON is a clear error naming the file', async () => { + const dir = await makeDir(); + const path = join(dir, 'project.json'); + await writeFile(path, 'not json'); + const result = await loadProject(path); + expect(result.ok).toBe(false); + expect(!result.ok && result.error).toContain(path); + }); + + test('a file that fails schema validation reports the issues, not just "invalid"', async () => { + const dir = await makeDir(); + const path = join(dir, 'project.json'); + await writeFile(path, JSON.stringify({ ...validProject, profiles: [] })); + const result = await loadProject(path); + expect(result.ok).toBe(false); + expect(!result.ok && (result.issues ?? []).some((i) => i.path === 'profiles')).toBe(true); + }); + + test('a directory instead of a file is a clear error, not a thrown exception', async () => { + const dir = await makeDir(); + const asDir = join(dir, 'project.json'); + await mkdir(asDir); + const result = await loadProject(asDir); + expect(result.ok).toBe(false); + }); +}); From 24e090211d7eeaf94616eaf2128c53c67cb5f067 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Fr=C3=B8yland?= <81354124+Andreas-Froyland@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:12:36 +0200 Subject: [PATCH 2/3] fix(cli): fix a Windows 8.3-path test failure, reject duplicate --json, catch checkTestRoot throwing - Windows CI failure: on the runner's account (runneradmin), the temp directory's short (8.3) name and its realpath- resolved long name differ; checkTestRoot deliberately reports the resolved path, so four tests comparing it against the pre-resolution path they created the directory with failed there specifically. They now resolve their own expected path the same way before comparing. - args.ts: a second --json is now rejected like every other duplicate flag, instead of silently accepted. --json is now decided by one scan of the whole invocation, independent of where it appears and of everything else wrong with it, so a malformed invocation that also asked for --json is still reported as JSON on stderr instead of silently downgrading to plain text. - environment-commands.ts: runStatus now also catches checkTestRoot itself failing (it can throw, not just reject cleanly, if the root is removed between its own existence check and resolving the real path), closing the one gap in its "never throws" contract; test exercises it by mocking checkTestRoot rather than racing the real filesystem. - Test fixture fix: the "machine matches profile" test hardcoded arch: 'x86_64', which fails on an arm64 host; now built from hostProfile() like the rest of the suite already does. - Test quality: `expect(result.ok && result.report.X)` obscures the real error when result.ok is false (a confusing matcher error instead of the underlying message); replaced with an explicit ok check that throws the real error. - README: the doctor example didn't run from a fresh checkout (qa/project.json doesn't exist yet); reworded so designate/status are shown as runnable now and doctor is clearly marked as needing a project-supplied file. Mutation-checked the three behavioral fixes (duplicate --json, json preserved through a parse failure, runStatus catching checkTestRoot) against a throwaway script; all three killed, files restored identical. Clean typecheck and full suite: 518 passed, 2 skipped; no leaked processes, temp directories, or home markers. Co-Authored-By: Claude Sonnet 5 --- README.md | 11 ++++- packages/qa/src/cli/args.ts | 49 ++++++++++--------- packages/qa/src/cli/environment-commands.ts | 10 ++-- packages/qa/src/cli/main.ts | 15 ++++-- packages/qa/test/cli/args.test.ts | 25 ++++++++++ packages/qa/test/cli/doctor.test.ts | 31 +++++++----- .../qa/test/cli/environment-commands.test.ts | 33 +++++++++++-- packages/qa/test/cli/main.test.ts | 21 ++++++-- 8 files changed, 139 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 39b66e4..7b7452c 100644 --- a/README.md +++ b/README.md @@ -21,14 +21,21 @@ CLI (`run`, `resume`), the Tauri driver, the dashboard and the GitHub integratio ## CLI -Run it straight from a checkout; no build step (see [tool layout](docs/decisions/tool-layout.md)): +Run it straight from a checkout; no build step (see [tool layout](docs/decisions/tool-layout.md)). `designate` and +`status` work as they stand, against any directory: ```sh -node packages/qa/src/cli/main.ts doctor --project qa/project.json --profile windows [--json] node packages/qa/src/cli/main.ts designate [--root ] [--json] # marks a directory safe to install and delete into node packages/qa/src/cli/main.ts status [--root ] [--json] # reports what a designated root holds, dirty or clean ``` +`doctor` needs a `qa/project.json` from a project that has one (this repository does not ship a sample yet — that +lands with the CLI's `run`/`resume` commands): + +```sh +node packages/qa/src/cli/main.ts doctor --project path/to/qa/project.json --profile windows [--json] +``` + `--root` defaults to `.release-qa` under the current directory (gitignored) when not given. Every command prints to stdout on success and to stderr on failure; `--json` switches both to one line of machine-readable JSON. Exit codes: `0` passed/ready, `1` a scenario the candidate failed (not reachable yet — no command runs a scenario), `2` this diff --git a/packages/qa/src/cli/args.ts b/packages/qa/src/cli/args.ts index 4b7a7a5..ed0ab67 100644 --- a/packages/qa/src/cli/args.ts +++ b/packages/qa/src/cli/args.ts @@ -1,6 +1,8 @@ /** * Parses `process.argv.slice(2)`. Never throws: a malformed invocation is a `{ ok: false }` result the caller turns - * into an exit code, not an exception that would print a stack trace instead of a usable message. + * into an exit code, not an exception that would print a stack trace instead of a usable message. `--json` is + * decided by a single scan of the whole invocation, independent of where it appears and of everything else that + * might be wrong with it, so the caller can render even a parse failure as JSON when that is what was asked for. */ export interface DoctorCommand { @@ -25,7 +27,7 @@ export interface StatusCommand { export type Command = DoctorCommand | DesignateCommand | StatusCommand; -export type ParsedArgs = { ok: true; command: Command } | { ok: false; error: string }; +export type ParsedArgs = { ok: true; command: Command } | { ok: false; error: string; json: boolean }; interface Spec { /** Flags that must be given a value. */ @@ -43,60 +45,63 @@ const SPECS: Record = { const COMMAND_NAMES = Object.keys(SPECS) as Command['name'][]; export function parseArgs(argv: readonly string[]): ParsedArgs { + const json = countJson(argv) === 1; const [name, ...rest] = argv; - if (name === undefined) return fail(`expected a command: ${COMMAND_NAMES.join(', ')}`); - if (!isCommandName(name)) return fail(`unknown command "${name}"; expected one of ${COMMAND_NAMES.join(', ')}`); + if (name === undefined) return fail(`expected a command: ${COMMAND_NAMES.join(', ')}`, json); + if (!isCommandName(name)) return fail(`unknown command "${name}"; expected one of ${COMMAND_NAMES.join(', ')}`, json); + if (countJson(argv) > 1) return fail('--json was given more than once', true); const flags = parseFlags(rest, SPECS[name]); - if (!flags.ok) return flags; + if (!flags.ok) return fail(flags.error, json); switch (name) { case 'doctor': - return { ok: true, command: { name, project: flags.values.project as string, profile: flags.values.profile as string, json: flags.json } }; + return { ok: true, command: { name, project: flags.values.project as string, profile: flags.values.profile as string, json } }; case 'designate': case 'status': - return { ok: true, command: { name, root: flags.values.root as string | undefined, json: flags.json } }; + return { ok: true, command: { name, root: flags.values.root as string | undefined, json } }; } } +const countJson = (argv: readonly string[]): number => argv.filter((token) => token === '--json').length; + function isCommandName(value: string): value is Command['name'] { return (COMMAND_NAMES as string[]).includes(value); } -type FlagResult = { ok: true; values: Record; json: boolean } | { ok: false; error: string }; +type FlagResult = { ok: true; values: Record } | { ok: false; error: string }; -/** `--json` is a boolean flag common to every command; every other flag takes exactly one value. */ +/** `--json` is handled by the caller; every other token is a `--flag value` pair or an unexpected extra. */ function parseFlags(args: readonly string[], spec: Spec): FlagResult { const known = new Set([...spec.required, ...spec.optional]); const values: Record = {}; - let json = false; const positional: string[] = []; for (let i = 0; i < args.length; i += 1) { const token = args[i]; - if (token === undefined) break; - if (token === '--json') { - json = true; - continue; - } + if (token === undefined || token === '--json') continue; if (!token.startsWith('--')) { positional.push(token); continue; } const flag = token.slice(2); - if (!known.has(flag)) return fail(`unknown flag "--${flag}"`); - if (Object.hasOwn(values, flag)) return fail(`--${flag} was given more than once`); + if (!known.has(flag)) return failFlags(`unknown flag "--${flag}"`); + if (Object.hasOwn(values, flag)) return failFlags(`--${flag} was given more than once`); const value = args[i + 1]; - if (value === undefined || value.startsWith('--')) return fail(`--${flag} needs a value`); + if (value === undefined || value.startsWith('--')) return failFlags(`--${flag} needs a value`); values[flag] = value; i += 1; } - if (positional.length > 0) return fail(`unexpected argument "${positional[0]}"`); - for (const flag of spec.required) if (values[flag] === undefined) return fail(`--${flag} is required`); - return { ok: true, values, json }; + if (positional.length > 0) return failFlags(`unexpected argument "${positional[0]}"`); + for (const flag of spec.required) if (values[flag] === undefined) return failFlags(`--${flag} is required`); + return { ok: true, values }; } -function fail(error: string): { ok: false; error: string } { +function failFlags(error: string): { ok: false; error: string } { return { ok: false, error }; } + +function fail(error: string, json: boolean): { ok: false; error: string; json: boolean } { + return { ok: false, error, json }; +} diff --git a/packages/qa/src/cli/environment-commands.ts b/packages/qa/src/cli/environment-commands.ts index 4ac47d9..70917b8 100644 --- a/packages/qa/src/cli/environment-commands.ts +++ b/packages/qa/src/cli/environment-commands.ts @@ -36,13 +36,15 @@ export type StatusResult = { ok: true; report: StatusReport } | { ok: false; err * report (`designated: false`), not an error; only a root this cannot read at all is an error. */ export async function runStatus(root: string, options: EnvironmentCommandOptions = {}): Promise { - const check = await checkTestRoot(root, options); - if (!check.ok) return { ok: true, report: { root, designated: false, reason: check.reason, owned: [] } }; - try { + const check = await checkTestRoot(root, options); + if (!check.ok) return { ok: true, report: { root, designated: false, reason: check.reason, owned: [] } }; + const [dirty, owned] = await Promise.all([readDirty(check.root), readLedger(check.root)]); return { ok: true, report: { root: check.root, designated: true, owned, ...(dirty === undefined ? {} : { dirty }) } }; } catch (error) { - return { ok: false, error: `could not read the state of ${check.root}: ${message(error)}` }; + // Covers checkTestRoot too: it can throw if the root is removed between its own existence check and resolving + // the real path, a narrow race this function's "never throws" promise still needs to hold against. + return { ok: false, error: `could not read the state of ${root}: ${message(error)}` }; } } diff --git a/packages/qa/src/cli/main.ts b/packages/qa/src/cli/main.ts index cf1773b..6b641ce 100644 --- a/packages/qa/src/cli/main.ts +++ b/packages/qa/src/cli/main.ts @@ -4,6 +4,7 @@ import { parseArgs } from './args.ts'; import { runDoctor, type DoctorReport } from './doctor.ts'; import { runDesignate, runStatus, type StatusReport } from './environment-commands.ts'; import { loadProject } from './project.ts'; +import type { ValidationIssue } from '../model/validate.ts'; /** * `0` passed. `1` a scenario the candidate failed (not yet reachable: no command runs a scenario in this build). @@ -23,7 +24,7 @@ const defaultIo: Io = { log: (line) => console.log(line), error: (line) => conso export async function main(argv: readonly string[], io: Io = defaultIo, cwd: () => string = () => process.cwd()): Promise { const parsed = parseArgs(argv); if (!parsed.ok) { - io.error(parsed.error); + reportError(io, parsed.json, parsed.error); return EXIT.infrastructure; } const { command } = parsed; @@ -32,12 +33,12 @@ export async function main(argv: readonly string[], io: Io = defaultIo, cwd: () case 'doctor': { const loaded = await loadProject(command.project); if (!loaded.ok) { - io.error(command.json ? JSON.stringify({ ok: false, error: loaded.error, issues: loaded.issues ?? [] }) : loaded.error); + reportError(io, command.json, loaded.error, loaded.issues); return EXIT.infrastructure; } const result = await runDoctor(loaded.project, command.profile); if (!result.ok) { - io.error(command.json ? JSON.stringify({ ok: false, error: result.error }) : result.error); + reportError(io, command.json, result.error); return EXIT.infrastructure; } printDoctor(io, command.json, result.report); @@ -48,7 +49,7 @@ export async function main(argv: readonly string[], io: Io = defaultIo, cwd: () const root = command.root ?? defaultRoot(cwd); const result = await runDesignate(root); if (!result.ok) { - io.error(command.json ? JSON.stringify({ ok: false, error: result.error }) : result.error); + reportError(io, command.json, result.error); return EXIT.infrastructure; } io.log(command.json ? JSON.stringify(result) : `designated ${result.root}`); @@ -59,7 +60,7 @@ export async function main(argv: readonly string[], io: Io = defaultIo, cwd: () const root = command.root ?? defaultRoot(cwd); const result = await runStatus(root); if (!result.ok) { - io.error(command.json ? JSON.stringify({ ok: false, error: result.error }) : result.error); + reportError(io, command.json, result.error); return EXIT.infrastructure; } // What status finds (undesignated, dirty) is a fact it reports, never a failure of the status command itself. @@ -71,6 +72,10 @@ export async function main(argv: readonly string[], io: Io = defaultIo, cwd: () const defaultRoot = (cwd: () => string): string => join(cwd(), '.release-qa'); +function reportError(io: Io, json: boolean, error: string, issues?: readonly ValidationIssue[]): void { + io.error(json ? JSON.stringify({ ok: false, error, ...(issues === undefined ? {} : { issues }) }) : error); +} + function printDoctor(io: Io, json: boolean, report: DoctorReport): void { if (json) { io.log(JSON.stringify(report)); diff --git a/packages/qa/test/cli/args.test.ts b/packages/qa/test/cli/args.test.ts index 10ef2be..0defbbd 100644 --- a/packages/qa/test/cli/args.test.ts +++ b/packages/qa/test/cli/args.test.ts @@ -7,6 +7,12 @@ const err = (argv: string[]): string => { return result.error; }; +const failure = (argv: string[]): { error: string; json: boolean } => { + const result = parseArgs(argv); + if (result.ok) throw new Error(`expected an error, got ${JSON.stringify(result.command)}`); + return result; +}; + describe('no command', () => { test('an empty argument list names the valid commands', () => { expect(err([])).toMatch(/doctor.*designate.*status/); @@ -16,6 +22,11 @@ describe('no command', () => { expect(err(['fly'])).toMatch(/"fly"/); expect(err(['fly'])).toMatch(/doctor.*designate.*status/); }); + + test('json is reported even when there is no valid command at all', () => { + expect(failure(['fly', '--json']).json).toBe(true); + expect(failure([]).json).toBe(false); + }); }); describe('doctor', () => { @@ -60,6 +71,20 @@ describe('doctor', () => { test('an extra positional argument is an error', () => { expect(err(['doctor', '--project', 'p.json', '--profile', 'w', 'extra'])).toMatch(/extra/); }); + + test('--json given twice is an error, not silently the same as once', () => { + expect(err(['doctor', '--project', 'p.json', '--profile', 'w', '--json', '--json'])).toContain('json'); + }); + + test('a malformed invocation that also asked for --json is still reported as wanting json', () => { + // The caller decides how to render the error; parsing must not silently downgrade it to plain text. + expect(failure(['doctor', '--project', 'p.json', '--profile', 'w', '--nope', '--json']).json).toBe(true); + expect(failure(['doctor', '--project', 'p.json', '--profile', 'w', '--json', '--nope']).json).toBe(true); + }); + + test('a malformed invocation without --json is reported as not wanting json', () => { + expect(failure(['doctor', '--project', 'p.json', '--profile', 'w', '--nope']).json).toBe(false); + }); }); describe('designate', () => { diff --git a/packages/qa/test/cli/doctor.test.ts b/packages/qa/test/cli/doctor.test.ts index fef9ead..f35bba2 100644 --- a/packages/qa/test/cli/doctor.test.ts +++ b/packages/qa/test/cli/doctor.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'vitest'; -import { runDoctor } from '../../src/cli/doctor.ts'; +import { runDoctor, type DoctorReport } from '../../src/cli/doctor.ts'; import type { Project } from '../../src/model/project.ts'; import { hostOs, hostProfile } from '../fixtures/processes.ts'; @@ -19,6 +19,13 @@ const project = (overrides: Partial = {}): Project => ({ const probes = (display: boolean, audio: boolean) => ({ display: async () => display, audio: async () => audio }); +/** Unwraps a successful result so a failure surfaces its own message instead of a confusing matcher error. */ +async function report(...args: Parameters): Promise { + const result = await runDoctor(...args); + if (!result.ok) throw new Error(result.error); + return result.report; +} + describe('doctor', () => { test('an unknown profile id is a clear error listing the known ones', async () => { const result = await runDoctor(project(), 'macos', probes(true, true)); @@ -28,23 +35,21 @@ describe('doctor', () => { }); test('a machine matching the requested profile is reported ready', async () => { - const result = await runDoctor(project(), hostProfile().id, probes(true, true)); - expect(result.ok).toBe(true); - expect(result.ok && result.report.ok).toBe(true); - expect(result.ok && result.report.mismatch).toBeUndefined(); - expect(result.ok && result.report.machine.capabilities).toEqual(expect.arrayContaining(['audio', 'display'])); + const doctorReport = await report(project(), hostProfile().id, probes(true, true)); + expect(doctorReport.ok).toBe(true); + expect(doctorReport.mismatch).toBeUndefined(); + expect(doctorReport.machine.capabilities).toEqual(expect.arrayContaining(['audio', 'display'])); }); test('a profile this machine does not match is reported, not silently passed', async () => { - const result = await runDoctor(project(), 'other', probes(true, true)); - expect(result.ok).toBe(true); - expect(result.ok && result.report.ok).toBe(false); - expect(result.ok && result.report.mismatch).toBeDefined(); + const doctorReport = await report(project(), 'other', probes(true, true)); + expect(doctorReport.ok).toBe(false); + expect(doctorReport.mismatch).toBeDefined(); }); test('reports the display kind alongside the raw capability list', async () => { - const result = await runDoctor(project(), hostProfile().id, { display: async () => false, audio: async () => false }); - expect(result.ok && result.report.display.kind).toBe('none'); - expect(result.ok && result.report.machine.capabilities).toEqual([]); + const doctorReport = await report(project(), hostProfile().id, { display: async () => false, audio: async () => false }); + expect(doctorReport.display.kind).toBe('none'); + expect(doctorReport.machine.capabilities).toEqual([]); }); }); diff --git a/packages/qa/test/cli/environment-commands.test.ts b/packages/qa/test/cli/environment-commands.test.ts index 2c7c777..a8c7cfe 100644 --- a/packages/qa/test/cli/environment-commands.test.ts +++ b/packages/qa/test/cli/environment-commands.test.ts @@ -1,6 +1,6 @@ -import { mkdir, writeFile } from 'node:fs/promises'; +import { mkdir, realpath, writeFile } from 'node:fs/promises'; import { join } from 'node:path'; -import { afterEach, describe, expect, test } from 'vitest'; +import { afterEach, describe, expect, test, vi } from 'vitest'; import { runDesignate, runStatus } from '../../src/cli/environment-commands.ts'; import { spawnOwned, markDirty } from '../../src/runner/resources.ts'; import { cleanUpProcessesAndRoots, makeTempDir, makeTestRoot, trackProcess } from '../fixtures/processes.ts'; @@ -35,7 +35,9 @@ describe('status', () => { test('a designated, clean, empty root is reported clean', async () => { const root = await makeTestRoot(); const result = await runStatus(root); - expect(result).toEqual({ ok: true, report: { root, designated: true, owned: [] } }); + // checkTestRoot reports the resolved path, which on some machines (e.g. an 8.3 short name in the temp path) + // is not byte-identical to the path this test created the directory with, though both name the same directory. + expect(result).toEqual({ ok: true, report: { root: await realpath(root), designated: true, owned: [] } }); }); test('reports what the root owns', async () => { @@ -43,14 +45,15 @@ describe('status', () => { const child = await spawnOwned(root, 'helper', process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { stdio: 'ignore' }); trackProcess(child); const result = await runStatus(root); - expect(result.ok && result.report.owned).toHaveLength(1); + if (!result.ok) throw new Error(result.error); + expect(result.report.owned).toHaveLength(1); }); test('reports why the environment is dirty', async () => { const root = await makeTestRoot(); await markDirty(root, 'a previous run left the app installed'); const result = await runStatus(root); - expect(result).toEqual({ ok: true, report: { root, designated: true, dirty: 'a previous run left the app installed', owned: [] } }); + expect(result).toEqual({ ok: true, report: { root: await realpath(root), designated: true, dirty: 'a previous run left the app installed', owned: [] } }); }); test('an unreadable ledger is a command error, not a silently empty report', async () => { @@ -68,3 +71,23 @@ describe('status', () => { expect(result).toEqual({ ok: true, report: { root: file, designated: false, reason: 'not-a-directory', owned: [] } }); }); }); + +describe('the root check itself failing', () => { + test('a root removed between the existence check and resolving it is a command error, not a thrown exception', async () => { + // checkTestRoot can throw (not return a TestRootCheck) if the directory is removed in that narrow window; this + // is the only way to exercise that path deterministically rather than racing the real filesystem for it. + vi.doMock('../../src/runner/resources.ts', async (importOriginal) => { + const real = await importOriginal(); + return { ...real, checkTestRoot: async () => { throw new Error('ENOENT: no longer there'); } }; + }); + vi.resetModules(); + const { runStatus: runStatusWithBrokenCheck } = await import('../../src/cli/environment-commands.ts'); + + const result = await runStatusWithBrokenCheck(await makeTestRoot()); + + expect(result.ok).toBe(false); + expect(!result.ok && result.error).toContain('no longer there'); + vi.doUnmock('../../src/runner/resources.ts'); + vi.resetModules(); + }); +}); diff --git a/packages/qa/test/cli/main.test.ts b/packages/qa/test/cli/main.test.ts index 4add300..fec4fc9 100644 --- a/packages/qa/test/cli/main.test.ts +++ b/packages/qa/test/cli/main.test.ts @@ -1,9 +1,9 @@ -import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { mkdir, realpath, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, expect, test } from 'vitest'; import { EXIT, main } from '../../src/cli/main.ts'; -import { hostOs } from '../fixtures/processes.ts'; +import { hostOs, hostProfile } from '../fixtures/processes.ts'; const dirs: string[] = []; afterEach(async () => { @@ -26,7 +26,7 @@ const validProject = { schemaVersion: 1, projectId: 'sample', releaseBranch: 'main', - profiles: [{ id: 'here', os: hostOs(), arch: 'x86_64' }], + profiles: [{ ...hostProfile(), id: 'here' }], requirements: [{ key: 'here/persistence', mode: 'automated', title: 'Persists data across restarts', capabilities: [] }], suites: [], scenarioFiles: [], @@ -48,6 +48,15 @@ describe('bad usage', () => { const out = io(); expect(await main(['launch'], out.sink)).toBe(EXIT.infrastructure); }); + + test('a malformed invocation with --json still gets a JSON error on stderr, not plain text', async () => { + const out = io(); + const code = await main(['doctor', '--project', 'p.json', '--profile', 'w', '--nope', '--json'], out.sink); + expect(code).toBe(EXIT.infrastructure); + expect(out.log).toEqual([]); + const printed = JSON.parse(out.error.join('')) as { ok: boolean; error: string }; + expect(printed).toMatchObject({ ok: false, error: expect.stringContaining('nope') }); + }); }); describe('doctor', () => { @@ -107,7 +116,9 @@ describe('designate and status', () => { expect(await main(['status', '--json'], statusOut.sink, () => dir)).toBe(EXIT.ok); const report = JSON.parse(statusOut.log.join('')) as { designated: boolean; root: string }; expect(report.designated).toBe(true); - expect(report.root).toBe(join(dir, '.release-qa')); + // status reports the resolved path, which is not always byte-identical to the path given (e.g. an 8.3 short + // name in the temp path on some Windows machines), though both name the same directory. + expect(report.root).toBe(await realpath(join(dir, '.release-qa'))); }); test('an explicit --root overrides the default', async () => { @@ -118,7 +129,7 @@ describe('designate and status', () => { expect(await main(['designate', '--root', explicit], out.sink, () => dir)).toBe(EXIT.ok); const statusOut = io(); await main(['status', '--root', explicit, '--json'], statusOut.sink, () => dir); - expect((JSON.parse(statusOut.log.join('')) as { root: string }).root).toBe(explicit); + expect((JSON.parse(statusOut.log.join('')) as { root: string }).root).toBe(await realpath(explicit)); }); test('status on an undesignated root is exit 0: it reports a fact, it is not itself a failure', async () => { From 9555e44f805f48ed7d19af704c85bf605804c6e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andreas=20Fr=C3=B8yland?= <81354124+Andreas-Froyland@users.noreply.github.com> Date: Tue, 22 Sep 2026 14:51:35 +0200 Subject: [PATCH 3/3] fix(cli): keep the issues field always present in JSON errors, tear down the checkTestRoot mock in afterEach - doctor's JSON error output always includes issues now (empty when there are none), rather than omitting the key for read/parse failures and only including it for schema failures: a consumer keying on its presence unconditionally no longer sees it vanish depending on which way loading failed. - The checkTestRoot mock in the "root check itself failing" test is now torn down in an afterEach, not at the end of the test body, so a failed assertion cannot leave it registered and confusingly break a later test in the file. Co-Authored-By: Claude Sonnet 5 --- packages/qa/src/cli/main.ts | 3 ++- packages/qa/test/cli/environment-commands.test.ts | 9 +++++++-- packages/qa/test/cli/main.test.ts | 12 +++++++++++- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/packages/qa/src/cli/main.ts b/packages/qa/src/cli/main.ts index 6b641ce..fa0c2d5 100644 --- a/packages/qa/src/cli/main.ts +++ b/packages/qa/src/cli/main.ts @@ -72,8 +72,9 @@ export async function main(argv: readonly string[], io: Io = defaultIo, cwd: () const defaultRoot = (cwd: () => string): string => join(cwd(), '.release-qa'); +/** `issues` is always present in JSON output, empty when there are none, so a consumer can key on it unconditionally. */ function reportError(io: Io, json: boolean, error: string, issues?: readonly ValidationIssue[]): void { - io.error(json ? JSON.stringify({ ok: false, error, ...(issues === undefined ? {} : { issues }) }) : error); + io.error(json ? JSON.stringify({ ok: false, error, issues: issues ?? [] }) : error); } function printDoctor(io: Io, json: boolean, report: DoctorReport): void { diff --git a/packages/qa/test/cli/environment-commands.test.ts b/packages/qa/test/cli/environment-commands.test.ts index a8c7cfe..56a9d01 100644 --- a/packages/qa/test/cli/environment-commands.test.ts +++ b/packages/qa/test/cli/environment-commands.test.ts @@ -73,6 +73,13 @@ describe('status', () => { }); describe('the root check itself failing', () => { + // Torn down here rather than at the end of the test, so a failed assertion cannot leave the mock registered and + // confusingly break whatever test happens to run after this one. + afterEach(async () => { + vi.doUnmock('../../src/runner/resources.ts'); + vi.resetModules(); + }); + test('a root removed between the existence check and resolving it is a command error, not a thrown exception', async () => { // checkTestRoot can throw (not return a TestRootCheck) if the directory is removed in that narrow window; this // is the only way to exercise that path deterministically rather than racing the real filesystem for it. @@ -87,7 +94,5 @@ describe('the root check itself failing', () => { expect(result.ok).toBe(false); expect(!result.ok && result.error).toContain('no longer there'); - vi.doUnmock('../../src/runner/resources.ts'); - vi.resetModules(); }); }); diff --git a/packages/qa/test/cli/main.test.ts b/packages/qa/test/cli/main.test.ts index fec4fc9..56a347c 100644 --- a/packages/qa/test/cli/main.test.ts +++ b/packages/qa/test/cli/main.test.ts @@ -54,8 +54,10 @@ describe('bad usage', () => { const code = await main(['doctor', '--project', 'p.json', '--profile', 'w', '--nope', '--json'], out.sink); expect(code).toBe(EXIT.infrastructure); expect(out.log).toEqual([]); - const printed = JSON.parse(out.error.join('')) as { ok: boolean; error: string }; + const printed = JSON.parse(out.error.join('')) as { ok: boolean; error: string; issues: unknown[] }; expect(printed).toMatchObject({ ok: false, error: expect.stringContaining('nope') }); + // issues is always present in JSON error output, even empty, so a consumer can key on it unconditionally. + expect(printed.issues).toEqual([]); }); }); @@ -68,6 +70,14 @@ describe('doctor', () => { expect(out.error.join(' ')).toContain('nope.json'); }); + test('a missing project file with --json still includes issues, empty, alongside the read error', async () => { + const out = io(); + const dir = await makeDir(); + await main(['doctor', '--project', join(dir, 'nope.json'), '--profile', 'here', '--json'], out.sink); + const printed = JSON.parse(out.error.join('')) as { issues: unknown[] }; + expect(printed.issues).toEqual([]); + }); + test('an unsupported profile is exit 3, naming the profiles the project does declare', async () => { const out = io(); const dir = await makeDir();