diff --git a/.changeset/not-found-names-what-is-missing.md b/.changeset/not-found-names-what-is-missing.md new file mode 100644 index 000000000..d076ec5d5 --- /dev/null +++ b/.changeset/not-found-names-what-is-missing.md @@ -0,0 +1,9 @@ +--- +"@qawolf/cli": minor +--- + +A 404 from the QA Wolf API now names what the command could not find, instead of telling everyone to check `--env`. A runner-targeting command says the runner is not running, names it, says whether `--runner`, `QAWOLF_RUNNER_ID` or this directory's stored default chose that id, and gives the launch command. `qawolf run get` says there is no such run on this team, and that ids printed by `qawolf runner run` are the runner's own — read those with `qawolf runner events run-status --run `. Only a request that really is scoped to an environment still points at `--env`. + +A failed `@qawolf/cli/runner-sdk` call now carries that second line as `errorDetail`, which the SDK used to build and throw away. + +These failures now exit `8` rather than `4`. Exit `4` means retry; a runner that was terminated or idled out never comes back, so a caller that kept retrying burned its budget on an id that could not work. Bound your retries on `4` as before, and stop on `8`. diff --git a/docs/exit-codes.md b/docs/exit-codes.md index fafe0e06d..3b470d50e 100644 --- a/docs/exit-codes.md +++ b/docs/exit-codes.md @@ -2,16 +2,17 @@ CI consumers depend on consistent exit codes. The CLI commits to the following codes; do not introduce new ones without updating this document and the central helper in [`src/shell/exit.ts`](../src/shell/exit.ts). -| Code | Name | Meaning | -| ---- | ------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `0` | `success` | Command completed successfully. | -| `1` | `testFailure` | A flow failed (non-zero result from running tests); a `qawolf runner run --follow` whose run did not pass; a runner action or snippet that was attempted and did not succeed. | -| `2` | `invalidArgs` | Commander parse error, unknown subcommand, bad flag value, a `flows run` selection with no runnable flow (pass `--allow-no-match` to exit `0` instead), no runner available, a flow needing a different runner image, or a runner asked for something it can never do (no screen to see or drive). | -| `3` | `auth` | Missing or invalid `QAWOLF_API_KEY`. | -| `4` | `network` | Apex unreachable, GCS download failure, registry unreachable, or a runner that could not serve the request now (unreachable, or its screen not yet up). | -| `5` | `config` | `qawolf.config.ts` invalid, file collision during `init`, or a run file that could not be read. | -| `6` | `timeout` | A `--follow` reached its `--timeout`: `runner run` before its run settled (the run may still be going), or `runner events`. | -| `7` | `payment` | The QA Wolf API refused the request with HTTP 402: billing prevented it — the organization is over its monthly spend limit or has no valid payment method. | +| Code | Name | Meaning | +| ---- | ------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `0` | `success` | Command completed successfully. | +| `1` | `testFailure` | A flow failed (non-zero result from running tests); a `qawolf runner run --follow` whose run did not pass; a runner action or snippet that was attempted and did not succeed. | +| `2` | `invalidArgs` | Commander parse error, unknown subcommand, bad flag value, a `flows run` selection with no runnable flow (pass `--allow-no-match` to exit `0` instead), no runner available, a flow needing a different runner image, or a runner asked for something it can never do (no screen to see or drive). | +| `3` | `auth` | Missing or invalid `QAWOLF_API_KEY`. | +| `4` | `network` | Apex unreachable, GCS download failure, registry unreachable, or a runner that could not serve the request now (unreachable, or its screen not yet up). | +| `5` | `config` | `qawolf.config.ts` invalid, file collision during `init`, or a run file that could not be read. | +| `6` | `timeout` | A `--follow` reached its `--timeout`: `runner run` before its run settled (the run may still be going), or `runner events`. | +| `7` | `payment` | The QA Wolf API refused the request with HTTP 402: billing prevented it — the organization is over its monthly spend limit or has no valid payment method. | +| `8` | `notFound` | The QA Wolf API answered HTTP 404: the runner, run, or other thing the command named was not there. A runner will not come back by waiting — launch it, or name one that is running. Other routes can answer `8` for something that does arrive shortly, such as a run still being created, and say so in the message. | ## Using the helper diff --git a/skills/qawolf-cli/references/run-results.md b/skills/qawolf-cli/references/run-results.md index da9b37e2a..de8b235e3 100644 --- a/skills/qawolf-cli/references/run-results.md +++ b/skills/qawolf-cli/references/run-results.md @@ -25,6 +25,14 @@ run `runId` in the response is canonical and can differ from the id you asked for. Use the returned value for follow-up calls. +`run get` resolves platform runs only. A run id printed by `qawolf runner run` +belongs to that runner, so `run get` answers exit `8` and no such run on this +team. Read one of those with `qawolf runner events run-status --run `. + +A run that has been requested but not yet created answers exit `8` too, and +says it is still being created. That one clears on its own, so read the message +rather than the code before deciding whether to poll. + Poll `status` until it reaches `passed`, `failed` or `canceled`. The other values mean the run is still going. diff --git a/skills/qawolf-cli/references/runner.md b/skills/qawolf-cli/references/runner.md index 46675f80c..7477553b7 100644 --- a/skills/qawolf-cli/references/runner.md +++ b/skills/qawolf-cli/references/runner.md @@ -30,8 +30,8 @@ then launched without `--id` ends up with a pod it is not addressing. Pass And a runner id that is set is treated as found, whether or not anything is running under it. So exporting `QAWOLF_RUNNER_ID=agent-1` turns off the auto-launch described next: instead of starting `agent-1`, commands try to reach -it and fail with exit code `4`, which reads as "retry" and never succeeds. -Launch that id once yourself and the rest follows. +it and fail with exit code `8`, naming the id and saying the variable is what +chose it. Launch that id once yourself and the rest follows. And launching an id that differs from `QAWOLF_RUNNER_ID` prints a warning on stderr naming both ids: the variable still outranks the directory default, so @@ -119,11 +119,14 @@ and says so on stderr. Everything else on this page waits for a run. Retry on the exit code, not on the message text: -- `4` is usually transient. The screen is up but cannot serve this instant: - restarting after a display-size change, or busy with another request. Retry in - a second or two — but bound the retries, because `4` also covers a runner that - was reaped after inactivity, which no amount of retrying brings back. If `4` - persists past a few tries, relaunch the id. +- `4` is transient. The screen is up but cannot serve this instant: restarting + after a display-size change, or busy with another request. Retry in a second + or two, and bound the retries. +- `8` means there is no such runner. It was never launched, or it was + terminated, or it idled out. Retrying never brings one back, so stop and + launch the id or name one that is running. The message says which runner was + meant and whether `--runner`, `QAWOLF_RUNNER_ID` or this directory's stored + default chose it — read that line before you pick an id to launch. - `2` will not clear on its own. Either nothing has run on this runner yet, so run a flow, or the runner has no browser at all, so launch with `--name playwright` instead. The message says which. @@ -224,7 +227,8 @@ what the page shows. One failure covers three causes, because a runner cannot tell them apart: no live page, no element matching the selector, no variable under that name. All three exit `2` and none clears by waiting, so read the message, which carries -whatever the runner said. An unreachable runner exits `4` and is worth retrying. +whatever the runner said. An unreachable runner exits `4` and is worth retrying; +a runner that is not running at all exits `8` and is not. Use `inspect` before reaching for `exec`. Reading a value through a snippet means printing it and then fishing it back out of the `console` stream, which is diff --git a/src/commands/qawolfCliRunResults.template.md b/src/commands/qawolfCliRunResults.template.md index 8b8e5afdf..6057cd9a6 100644 --- a/src/commands/qawolfCliRunResults.template.md +++ b/src/commands/qawolfCliRunResults.template.md @@ -25,6 +25,14 @@ run `runId` in the response is canonical and can differ from the id you asked for. Use the returned value for follow-up calls. +`run get` resolves platform runs only. A run id printed by `qawolf runner run` +belongs to that runner, so `run get` answers exit `8` and no such run on this +team. Read one of those with `qawolf runner events run-status --run `. + +A run that has been requested but not yet created answers exit `8` too, and +says it is still being created. That one clears on its own, so read the message +rather than the code before deciding whether to poll. + Poll `status` until it reaches `passed`, `failed` or `canceled`. The other values mean the run is still going. diff --git a/src/core/interactiveRunner/runnerIdSource.ts b/src/core/interactiveRunner/runnerIdSource.ts new file mode 100644 index 000000000..fa5e6ea77 --- /dev/null +++ b/src/core/interactiveRunner/runnerIdSource.ts @@ -0,0 +1,8 @@ +import type { IdSource } from "~/core/resolveId.js"; + +/** + * Where the runner a command drives was named: the three levels `resolveIdFrom` + * picks between, the runner the CLI started because none of them named one, and + * the id an SDK caller passed in directly. + */ +export type RunnerIdSource = IdSource | "launched" | "given"; diff --git a/src/core/messages/authErrors.ts b/src/core/messages/authErrors.ts index c4e539cdd..c27aea40d 100644 --- a/src/core/messages/authErrors.ts +++ b/src/core/messages/authErrors.ts @@ -19,8 +19,36 @@ export const authErrorMessages = { `QA Wolf API refused the${noun ? ` ${noun}` : ""} request (HTTP 402): billing prevented it.`, rejected403: (noun: string | undefined) => `QA Wolf API rejected the${noun ? ` ${noun}` : ""} request (HTTP 403). Check that your API key has access to this environment.`, - notFound404: (noun: string | undefined) => + /** + * A 404 is answered by what the request named, because almost none of them + * are about an environment. Only `notFound404Environment` keeps the wording + * that blames one, and only routes that carry an environment reach it. + */ + notFound404Environment: (noun: string | undefined) => `QA Wolf API could not find ${noun ? `${noun} for that environment` : "that environment"} (HTTP 404). Check the --env value.`, + notFound404Runner: (runnerId: string | undefined) => + runnerId === undefined + ? "That runner is not running (HTTP 404)." + : `Runner ${runnerId} is not running (HTTP 404).`, + /** Why a runner is gone, when the platform did not say. */ + runnerIsGone: + "It was never launched, or it has since been terminated or idled out.", + /** True whatever the platform said, so it is offered either way. */ + launchTheRunner: (runnerId: string | undefined) => + runnerId === undefined + ? "Launch one with qawolf runner launch --id , or name a running one with --runner." + : `Launch it with qawolf runner launch --id ${runnerId}, or send this to a different runner with --runner.`, + notFound404Run: (runId: string | undefined) => + `QA Wolf has no run ${runId ?? "by that id"} on this team (HTTP 404).`, + /** + * Why a run id that exists can still be unknown to the platform. A guess, + * so it is offered only when the platform did not say — a run being created + * also answers 404, and that one clears by waiting. + */ + runIdMayBeRunnerLocal: (runId: string | undefined) => + `A run id printed by qawolf runner run belongs to that runner rather than to the platform, so this command cannot resolve it. Read that run with qawolf runner events run-status --run ${runId ?? ""}.`, + notFound404: (noun: string | undefined) => + `QA Wolf API could not find ${noun ?? "what the request named"} (HTTP 404).`, failedWithStatus: (status: number, noun: string | undefined) => `QA Wolf API${noun ? ` ${noun}` : ""} request failed (HTTP ${status}).`, networkUnreachable: (baseUrl: string, noun: string | undefined) => diff --git a/src/core/messages/interactiveRunner/lifecycle.ts b/src/core/messages/interactiveRunner/lifecycle.ts index 533620c32..9d6820945 100644 --- a/src/core/messages/interactiveRunner/lifecycle.ts +++ b/src/core/messages/interactiveRunner/lifecycle.ts @@ -1,3 +1,5 @@ +import type { RunnerIdSource } from "~/core/interactiveRunner/runnerIdSource.js"; + // Ends the line with no period: a terminal that linkifies takes the period as // part of the address. // @@ -42,4 +44,16 @@ export const lifecycleMessages = { `Runner ${id} had nothing to stop. The run had already finished, or none had been submitted.`, runnerUnreachable: "The runner could not be reached. It may still be starting, or it may have terminated after inactivity. Retry, or launch it again.", + // Named on every failure to reach a runner, because a transcript that only + // says which runner was missed leaves a reader unable to tell whether the + // wrong id was typed, exported, or left behind in .qawolf by an earlier + // launch — three different things to go and change. + runnerIdCameFrom: (id: string, source: RunnerIdSource) => + ({ + environment: `The id ${id} came from QAWOLF_RUNNER_ID.`, + flag: `The id ${id} came from --runner.`, + given: `The id ${id} is the one this call named.`, + launched: `Runner ${id} was launched for this command.`, + stored: `The id ${id} came from the runner this directory last launched, recorded in .qawolf.`, + })[source], } as const; diff --git a/src/core/publicApi/notFoundSubject.test.ts b/src/core/publicApi/notFoundSubject.test.ts new file mode 100644 index 000000000..78daf62aa --- /dev/null +++ b/src/core/publicApi/notFoundSubject.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from "bun:test"; + +import { notFoundSubject } from "./notFoundSubject.js"; + +describe("notFoundSubject", () => { + it("reads the runner id off a route relayed to one runner", () => { + expect(notFoundSubject("runner.takeScreenshot", { id: "agent-1" })).toEqual( + { kind: "runner", runnerId: "agent-1" }, + ); + }); + + // Launching creates a runner and listing names none, so neither can 404 over + // a runner that has gone. + it.each(["runner.launch", "runner.list"])( + "does not read %s as a missing runner", + (contractName) => { + expect(notFoundSubject(contractName, { id: "agent-1" }).kind).not.toBe( + "runner", + ); + }, + ); + + it("reads the run id off a route that resolves one run", () => { + expect(notFoundSubject("run.get", { runId: "abc" })).toEqual({ + kind: "run", + runId: "abc", + }); + }); + + it("reads a request that names an environment as environment-scoped", () => { + expect(notFoundSubject("run.create", { environmentId: "env-1" })).toEqual({ + kind: "environment", + }); + expect( + notFoundSubject("environment.get", { environmentId: "env-1" }), + ).toEqual({ kind: "environment" }); + }); + + // The bug this replaces: a trigger or an issue that does not exist was + // reported as an environment problem. + it("leaves a request that names no environment unattributed", () => { + expect(notFoundSubject("trigger.get", { triggerId: "t-1" })).toEqual({ + kind: "other", + }); + }); + + it("survives an input that is not an object", () => { + expect(notFoundSubject("runner.inspect", undefined)).toEqual({ + kind: "runner", + runnerId: undefined, + }); + }); +}); diff --git a/src/core/publicApi/notFoundSubject.ts b/src/core/publicApi/notFoundSubject.ts new file mode 100644 index 000000000..6457864d9 --- /dev/null +++ b/src/core/publicApi/notFoundSubject.ts @@ -0,0 +1,54 @@ +/** + * What a request asked the platform to find, so a 404 can name that thing + * rather than blame the environment for every miss. + */ +export type NotFoundSubject = + | { kind: "runner"; runnerId: string | undefined } + | { kind: "run"; runId: string | undefined } + | { kind: "environment" } + | { kind: "other" }; + +// `launch` starts a runner and `list` names none, so neither can 404 over a +// runner that has gone. Every other runner route is relayed to one live pod. +const runnerRoutesThatNameNoRunner: ReadonlySet = new Set([ + "runner.launch", + "runner.list", +]); + +// The run routes that resolve one run by id. `run.create` and `run.find` take +// an environment instead, and fall through to the environment rule below. +const runRoutesThatResolveOneRun: ReadonlySet = new Set([ + "run.diagnose", + "run.get", + "run.reattempt", + "run.stop", +]); + +function field(input: unknown, name: string): string | undefined { + if (typeof input !== "object" || input === null) return undefined; + const value = (input as Record)[name]; + return typeof value === "string" ? value : undefined; +} + +/** + * Reads the subject off the contract name and the input that was sent, rather + * than off a hand-kept list of every contract: a route that resolves one runner + * carries its id, and a route scoped to an environment carries that. + */ +export function notFoundSubject( + contractName: string, + input: unknown, +): NotFoundSubject { + if ( + contractName.startsWith("runner.") && + !runnerRoutesThatNameNoRunner.has(contractName) + ) { + return { kind: "runner", runnerId: field(input, "id") }; + } + if (runRoutesThatResolveOneRun.has(contractName)) { + return { kind: "run", runId: field(input, "runId") }; + } + if (field(input, "environmentId") !== undefined) + return { kind: "environment" }; + return { kind: "other" }; +} diff --git a/src/core/resolveId.test.ts b/src/core/resolveId.test.ts index a735eadbd..76829e0f0 100644 --- a/src/core/resolveId.test.ts +++ b/src/core/resolveId.test.ts @@ -6,13 +6,13 @@ const stored = async () => "stored"; describe("resolveIdFrom", () => { it("takes the given id over everything", async () => { - const id = await resolveIdFrom({ + const resolved = await resolveIdFrom({ env: { X_ID: "env" }, environmentVariable: "X_ID", given: "given", readStored: stored, }); - expect(id).toBe("given"); + expect(resolved).toEqual({ id: "given", source: "flag" }); }); it("takes the environment over the store, ignoring a blank variable", async () => { @@ -21,11 +21,26 @@ describe("resolveIdFrom", () => { given: undefined, readStored: stored, }; - expect(await resolveIdFrom({ ...options, env: { X_ID: " env " } })).toBe( - "env", - ); - expect(await resolveIdFrom({ ...options, env: { X_ID: " " } })).toBe( - "stored", + expect(await resolveIdFrom({ ...options, env: { X_ID: " env " } })).toEqual( + { + id: "env", + source: "environment", + }, ); + expect(await resolveIdFrom({ ...options, env: { X_ID: " " } })).toEqual({ + id: "stored", + source: "stored", + }); + }); + + it("resolves to nothing when no level names an id", async () => { + expect( + await resolveIdFrom({ + env: {}, + environmentVariable: "X_ID", + given: undefined, + readStored: async () => undefined, + }), + ).toBeUndefined(); }); }); diff --git a/src/core/resolveId.ts b/src/core/resolveId.ts index c4559effc..72e37e84c 100644 --- a/src/core/resolveId.ts +++ b/src/core/resolveId.ts @@ -1,15 +1,29 @@ +/** Which of the three places an id was taken from. */ +export type IdSource = "flag" | "environment" | "stored"; + +export type ResolvedId = { id: string; source: IdSource }; + /** * Which id a command means: the one given, else the environment, else the one * stored. Most explicit wins, and each level is one a caller can see and change. + * + * The source travels with the id because a command that cannot reach what the + * id names has to say which of the three chose it — otherwise a caller reading + * the failure cannot tell what to change. */ export async function resolveIdFrom(options: { given: string | undefined; env: Record; environmentVariable: string; readStored: () => Promise; -}): Promise { - if (options.given !== undefined) return options.given; +}): Promise { + if (options.given !== undefined) { + return { id: options.given, source: "flag" }; + } const fromEnvironment = options.env[options.environmentVariable]?.trim(); - if (fromEnvironment) return fromEnvironment; - return options.readStored(); + if (fromEnvironment) { + return { id: fromEnvironment, source: "environment" }; + } + const stored = await options.readStored(); + return stored === undefined ? undefined : { id: stored, source: "stored" }; } diff --git a/src/domains/agent/resolveSessionId.ts b/src/domains/agent/resolveSessionId.ts index c223e610f..b83adcdbd 100644 --- a/src/domains/agent/resolveSessionId.ts +++ b/src/domains/agent/resolveSessionId.ts @@ -23,13 +23,15 @@ export function chooseGivenSessionId(given: { } /** The session a command means: given, else the environment, else the last one this workspace started. */ -export const resolveSessionId = ( +export const resolveSessionId = async ( session: string | undefined, deps: AgentDeps, ): Promise => - resolveIdFrom({ - env: deps.env, - environmentVariable: sessionIdEnvironmentVariable, - given: session, - readStored: deps.store.readLastSessionId, - }); + ( + await resolveIdFrom({ + env: deps.env, + environmentVariable: sessionIdEnvironmentVariable, + given: session, + readStored: deps.store.readLastSessionId, + }) + )?.id; diff --git a/src/domains/interactiveRunner/evaluateSnippet.ts b/src/domains/interactiveRunner/evaluateSnippet.ts index a53a6847b..5bd2b26a6 100644 --- a/src/domains/interactiveRunner/evaluateSnippet.ts +++ b/src/domains/interactiveRunner/evaluateSnippet.ts @@ -11,6 +11,7 @@ import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { InteractiveRunnerDeps } from "./deps.js"; import { describeEvaluateSnippetFailure } from "./evaluateSnippetFailure.js"; import { announceRunner, resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { resolveSnippetScope } from "./snippetScope.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; @@ -82,10 +83,7 @@ export async function handleRunnerExec( runnerCallOptions, ); if (!result.ok) { - return { - ...failureFields(result), - exitCode: result.exitCode ?? exitCodes.network, - }; + return runnerRequestFailure(result, resolved); } if (result.value.outcome === "failure") { diff --git a/src/domains/interactiveRunner/events.ts b/src/domains/interactiveRunner/events.ts index 8b8175be6..c133b8972 100644 --- a/src/domains/interactiveRunner/events.ts +++ b/src/domains/interactiveRunner/events.ts @@ -79,7 +79,7 @@ export async function handleRunnerEvents( ); } - const read = createJournalCursor(ctx, resolved.runnerId, parsed.value); + const read = createJournalCursor(ctx, resolved, parsed.value); const unreachable = createUnreachableBudget(pollIntervalMs); // Polls rather than a clock, like followRun: the loop sleeps a known interval diff --git a/src/domains/interactiveRunner/followPrinters.ts b/src/domains/interactiveRunner/followPrinters.ts index a7420876d..e0cb16b40 100644 --- a/src/domains/interactiveRunner/followPrinters.ts +++ b/src/domains/interactiveRunner/followPrinters.ts @@ -11,6 +11,7 @@ import { readJournal, unreachableFailure, } from "./readJournal.js"; +import type { TargetedRunner } from "./resolveRunner.js"; const anchorPollIntervalMs = 1_000; @@ -30,11 +31,11 @@ type RecorderAnchor = */ export async function resolveRecorderAnchor( ctx: AuthCommandContext, - resolved: { runnerId: string; type: "launched" | "resolved" }, + resolved: TargetedRunner & { type: "launched" | "resolved" }, deps: { sleep: (ms: number) => Promise }, ): Promise { if (resolved.type === "launched") return { ok: true, sinceSequence: 0 }; - return anchorRecorderCursor(ctx, resolved.runnerId, deps); + return anchorRecorderCursor(ctx, resolved, deps); } /** @@ -46,12 +47,12 @@ export async function resolveRecorderAnchor( */ async function anchorRecorderCursor( ctx: AuthCommandContext, - runnerId: string, + runner: TargetedRunner, deps: { sleep: (ms: number) => Promise }, ): Promise { const unreachable = createUnreachableBudget(anchorPollIntervalMs); for (;;) { - const anchor = await readJournal(ctx, runnerId, { + const anchor = await readJournal(ctx, runner, { stream: "recorder", tail: 1, }); @@ -79,7 +80,7 @@ export type FollowStreamOptions = { recorderSinceSequence: number | undefined; runEvents: boolean; runId: string; - runnerId: string; + runner: TargetedRunner; }; /** @@ -98,7 +99,7 @@ export function createFollowPrinters( printers.push( createPrintingCursor( ctx, - options.runnerId, + options.runner, { runId: options.runId, stream: "run-logs" }, formatRunLogLine, ), @@ -108,7 +109,7 @@ export function createFollowPrinters( printers.push( createPrintingCursor( ctx, - options.runnerId, + options.runner, { runId: options.runId, stream: "run-events" }, jsonLine, ), @@ -118,7 +119,7 @@ export function createFollowPrinters( printers.push( createPrintingCursor( ctx, - options.runnerId, + options.runner, { sinceSequence: options.recorderSinceSequence, stream: "recorder" }, jsonLine, ), diff --git a/src/domains/interactiveRunner/followRun.events.test.ts b/src/domains/interactiveRunner/followRun.events.test.ts index 787c7584e..102701f07 100644 --- a/src/domains/interactiveRunner/followRun.events.test.ts +++ b/src/domains/interactiveRunner/followRun.events.test.ts @@ -18,7 +18,7 @@ const follow = ( recorderSinceSequence: options.recorderSinceSequence, runEvents: options.runEvents ?? false, runId: "run-a", - runnerId: "ci", + runner: { runnerId: "ci", source: "flag" }, timeoutSeconds: 3600, }, makeTestDeps(), diff --git a/src/domains/interactiveRunner/followRun.quiet.test.ts b/src/domains/interactiveRunner/followRun.quiet.test.ts index f59c1b91b..675fe8931 100644 --- a/src/domains/interactiveRunner/followRun.quiet.test.ts +++ b/src/domains/interactiveRunner/followRun.quiet.test.ts @@ -15,7 +15,7 @@ const follow = (ctx: ReturnType["ctx"]) => recorderSinceSequence: undefined, runEvents: false, runId: "run-a", - runnerId: "ci", + runner: { runnerId: "ci", source: "flag" }, timeoutSeconds: 3600, }, makeTestDeps(), diff --git a/src/domains/interactiveRunner/followRun.test.ts b/src/domains/interactiveRunner/followRun.test.ts index ba53d0e08..99dbeaf76 100644 --- a/src/domains/interactiveRunner/followRun.test.ts +++ b/src/domains/interactiveRunner/followRun.test.ts @@ -29,7 +29,7 @@ const follow = ( recorderSinceSequence: options.recorderSinceSequence, runEvents: options.runEvents ?? false, runId: "run-a", - runnerId: "ci", + runner: { runnerId: "ci", source: "flag" }, timeoutSeconds: options.timeoutSeconds ?? 3600, }, makeTestDeps(), diff --git a/src/domains/interactiveRunner/followRun.ts b/src/domains/interactiveRunner/followRun.ts index ab576abab..7fd8ca0bf 100644 --- a/src/domains/interactiveRunner/followRun.ts +++ b/src/domains/interactiveRunner/followRun.ts @@ -69,7 +69,7 @@ export async function followRun( deps: InteractiveRunnerDeps, ): Promise { const printers = createFollowPrinters(ctx, options); - const readStatus = createJournalCursor(ctx, options.runnerId, { + const readStatus = createJournalCursor(ctx, options.runner, { runId: options.runId, stream: "run-status", }); @@ -138,7 +138,7 @@ export async function followRun( return { error: interactiveRunnerMessages.followTimedOut( options.runId, - options.runnerId, + options.runner.runnerId, options.timeoutSeconds, ), exitCode: exitCodes.timeout, diff --git a/src/domains/interactiveRunner/highlightSelector.ts b/src/domains/interactiveRunner/highlightSelector.ts index 752037e2e..ccb68b553 100644 --- a/src/domains/interactiveRunner/highlightSelector.ts +++ b/src/domains/interactiveRunner/highlightSelector.ts @@ -10,6 +10,7 @@ import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { InteractiveRunnerDeps } from "./deps.js"; import { resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; /** @@ -49,7 +50,7 @@ export async function handleRunnerHighlightSelector( runnerCallOptions, ); if (!result.ok) { - return { ...failureFields(result), exitCode: exitCodes.network }; + return runnerRequestFailure(result, resolved); } const { outcome } = result.value; diff --git a/src/domains/interactiveRunner/importPackage.ts b/src/domains/interactiveRunner/importPackage.ts index 2bd372956..19163c1bd 100644 --- a/src/domains/interactiveRunner/importPackage.ts +++ b/src/domains/interactiveRunner/importPackage.ts @@ -15,6 +15,7 @@ import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { InteractiveRunnerDeps } from "./deps.js"; import { resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; const defaultPackageVersion = "latest"; @@ -81,10 +82,7 @@ export async function handleRunnerImportPackage( runnerCallOptions, ); if (!result.ok) { - return { - ...failureFields(result), - exitCode: result.exitCode ?? exitCodes.network, - }; + return runnerRequestFailure(result, resolved); } if (result.value.outcome === "failure") { diff --git a/src/domains/interactiveRunner/inspect.ts b/src/domains/interactiveRunner/inspect.ts index 485151f2a..81c8c5258 100644 --- a/src/domains/interactiveRunner/inspect.ts +++ b/src/domains/interactiveRunner/inspect.ts @@ -17,6 +17,7 @@ import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { InteractiveRunnerDeps } from "./deps.js"; import { resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; /** @@ -59,7 +60,7 @@ export async function handleRunnerInspect( runnerCallOptions, ); if (!result.ok) { - return { ...failureFields(result), exitCode: exitCodes.network }; + return runnerRequestFailure(result, resolved); } if (result.value.outcome === "failure") { diff --git a/src/domains/interactiveRunner/inspectMobile.ts b/src/domains/interactiveRunner/inspectMobile.ts index 6d8b5b16d..27a146c2d 100644 --- a/src/domains/interactiveRunner/inspectMobile.ts +++ b/src/domains/interactiveRunner/inspectMobile.ts @@ -15,6 +15,7 @@ import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { InteractiveRunnerDeps } from "./deps.js"; import { describeSession, streamLine } from "./inspectMobileAnswer.js"; import { resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; /** @@ -52,7 +53,7 @@ export async function handleRunnerInspectMobile( runnerCallOptions, ); if (!result.ok) { - return { ...failureFields(result), exitCode: exitCodes.network }; + return runnerRequestFailure(result, resolved); } if (result.value.outcome === "success") { diff --git a/src/domains/interactiveRunner/journalCursor.test.ts b/src/domains/interactiveRunner/journalCursor.test.ts index bbd9cf468..99b90d402 100644 --- a/src/domains/interactiveRunner/journalCursor.test.ts +++ b/src/domains/interactiveRunner/journalCursor.test.ts @@ -7,6 +7,8 @@ import { } from "./journalCursor.js"; import { makeJournal } from "./journal.testUtils.js"; +const runner = { runnerId: "ci", source: "flag" } as const; + describe("createJournalCursor", () => { // The first read carries no cursor, so it starts at the oldest entry the runner // still holds and by definition missed nothing. Measuring it against sequence @@ -22,7 +24,7 @@ describe("createJournalCursor", () => { }), ); - await createJournalCursor(ctx, "ci", { stream: "recorder" })(); + await createJournalCursor(ctx, runner, { stream: "recorder" })(); expect(warnings()).toEqual([]); }); @@ -40,7 +42,7 @@ describe("createJournalCursor", () => { ], }), ); - const read = createJournalCursor(ctx, "ci", { stream: "recorder" }); + const read = createJournalCursor(ctx, runner, { stream: "recorder" }); await read(); await read(); @@ -53,7 +55,7 @@ describe("createJournalCursor", () => { callPublicApi.mockImplementation( makeJournal({ recorder: [[{ code: "a" }], [{ code: "b" }]] }), ); - const read = createJournalCursor(ctx, "ci", { + const read = createJournalCursor(ctx, runner, { stream: "recorder", tail: 1, }); @@ -93,7 +95,7 @@ describe("createJournalCursor", () => { outcome: "read", }, }); - const read = createJournalCursor(ctx, "ci", { stream: "recorder" }); + const read = createJournalCursor(ctx, runner, { stream: "recorder" }); await read(); await read(); @@ -113,7 +115,7 @@ describe("createJournalCursor", () => { { hasUnsearchedHistory: { recorder: true } }, ), ); - const read = createJournalCursor(ctx, "ci", { stream: "recorder" }); + const read = createJournalCursor(ctx, runner, { stream: "recorder" }); await read(); await read(); @@ -128,7 +130,7 @@ describe("createJournalCursor", () => { makeJournal({ recorder: ["unreachable"] }), ); - const read = await createJournalCursor(ctx, "ci", { + const read = await createJournalCursor(ctx, runner, { stream: "recorder", })(); diff --git a/src/domains/interactiveRunner/journalCursor.ts b/src/domains/interactiveRunner/journalCursor.ts index 27fcc9403..72000ffc8 100644 --- a/src/domains/interactiveRunner/journalCursor.ts +++ b/src/domains/interactiveRunner/journalCursor.ts @@ -5,6 +5,7 @@ import { interactiveRunnerMessages } from "~/core/messages/index.js"; import type { AuthCommandContext } from "~/shell/commandContext.js"; import { type JournalRequest, readJournal } from "./readJournal.js"; +import type { TargetedRunner } from "./resolveRunner.js"; export type CursorRead = | { type: "entries"; entries: JournalEntry[] } @@ -27,7 +28,7 @@ export type CursorRead = */ export function createJournalCursor( ctx: AuthCommandContext, - runnerId: string, + runner: TargetedRunner, request: JournalRequest, ): () => Promise { let sinceSequence = request.sinceSequence; @@ -35,7 +36,7 @@ export function createJournalCursor( let warnedUnsearchedHistory = false; return async function read(): Promise { - const window = await readJournal(ctx, runnerId, { + const window = await readJournal(ctx, runner, { ...request, sinceSequence, tail, @@ -74,11 +75,11 @@ export function createJournalCursor( */ export function createPrintingCursor( ctx: AuthCommandContext, - runnerId: string, + runner: TargetedRunner, request: JournalRequest, format: (payload: unknown) => string, ): () => Promise { - const read = createJournalCursor(ctx, runnerId, request); + const read = createJournalCursor(ctx, runner, request); return async () => { const window = await read(); if (window.type !== "entries") return window; diff --git a/src/domains/interactiveRunner/keepalive.ts b/src/domains/interactiveRunner/keepalive.ts index 107e4dfe8..3f8252324 100644 --- a/src/domains/interactiveRunner/keepalive.ts +++ b/src/domains/interactiveRunner/keepalive.ts @@ -41,7 +41,7 @@ export async function handleRunnerKeepalive( return { ...failureFields(resolved), exitCode: resolved.exitCode }; } - const window = await readJournal(ctx, resolved.runnerId, { + const window = await readJournal(ctx, resolved, { stream: "run-status", tail: 1, }); diff --git a/src/domains/interactiveRunner/performAction.ts b/src/domains/interactiveRunner/performAction.ts index 9569d25cb..fb3f4defc 100644 --- a/src/domains/interactiveRunner/performAction.ts +++ b/src/domains/interactiveRunner/performAction.ts @@ -20,6 +20,7 @@ import { import { readAction } from "./readAction.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; import { announceRunner, resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; /** * Performs one raw browser action. @@ -84,19 +85,16 @@ export async function handleRunnerAct( // A lost answer at the transport is the same hazard as the unreachable // outcome below: the action may have taken effect before the answer was // lost, so this failure must not invite a bare repeat either. - const fields = failureFields(result); - return { - ...fields, - ...(result.mayHaveArrived - ? { - error: appendSentence( - fields.error, - interactiveRunnerMessages.actionMayHaveHappened, - ), - } - : {}), - exitCode: exitCodes.network, - }; + const fields = runnerRequestFailure(result, resolved); + return result.mayHaveArrived + ? { + ...fields, + error: appendSentence( + fields.error, + interactiveRunnerMessages.actionMayHaveHappened, + ), + } + : fields; } const answer = result.value; diff --git a/src/domains/interactiveRunner/promoteSnapshot.ts b/src/domains/interactiveRunner/promoteSnapshot.ts index 653a7d61b..039804094 100644 --- a/src/domains/interactiveRunner/promoteSnapshot.ts +++ b/src/domains/interactiveRunner/promoteSnapshot.ts @@ -10,6 +10,7 @@ import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { InteractiveRunnerDeps } from "./deps.js"; import { resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; /** @@ -51,7 +52,7 @@ export async function handleRunnerPromoteSnapshot( runnerCallOptions, ); if (!result.ok) { - return { ...failureFields(result), exitCode: exitCodes.network }; + return runnerRequestFailure(result, resolved); } if (result.value.outcome === "success") { diff --git a/src/domains/interactiveRunner/readJournal.ts b/src/domains/interactiveRunner/readJournal.ts index 8ab1b06de..64586cc47 100644 --- a/src/domains/interactiveRunner/readJournal.ts +++ b/src/domains/interactiveRunner/readJournal.ts @@ -3,9 +3,10 @@ import { type JournalEntry, publicContractsV1 } from "@qawolf/api-contracts/v1"; import { interactiveRunnerMessages } from "~/core/messages/index.js"; import type { RunnerApiContext } from "~/shell/commandContext.js"; import { exitCodes } from "~/shell/exit.js"; -import { failureFields } from "~/shell/platform/requestWithRetry.js"; +import type { TargetedRunner } from "./resolveRunner.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; type JournalWindow = { entries: JournalEntry[]; @@ -53,13 +54,13 @@ export const unreachableFailure = { /** One window of one stream. */ export async function readJournal( ctx: RunnerApiContext, - runnerId: string, + runner: TargetedRunner, request: JournalRequest, ): Promise { const result = await ctx.platformClient.callPublicApi( publicContractsV1.runner.readJournal, { - id: runnerId, + id: runner.runnerId, stream: request.stream, ...(request.runId === undefined ? {} : { runId: request.runId }), ...(request.sinceSequence === undefined @@ -70,11 +71,7 @@ export async function readJournal( runnerCallOptions, ); if (!result.ok) { - return { - ...failureFields(result), - exitCode: exitCodes.network, - type: "failed", - }; + return { ...runnerRequestFailure(result, runner), type: "failed" }; } if (result.value.outcome === "failure") { result.value.failureReason satisfies "runner-unreachable"; diff --git a/src/domains/interactiveRunner/resolveRunner.test.ts b/src/domains/interactiveRunner/resolveRunner.test.ts index 8e3873f6d..1fd187dca 100644 --- a/src/domains/interactiveRunner/resolveRunner.test.ts +++ b/src/domains/interactiveRunner/resolveRunner.test.ts @@ -24,7 +24,7 @@ describe("resolveRunner", () => { expect( await resolveRunner(ctx, { autoLaunch: true, runner: "from-flag" }, deps), - ).toEqual({ runnerId: "from-flag", type: "resolved" }); + ).toEqual({ runnerId: "from-flag", source: "flag", type: "resolved" }); expect(callPublicApi).not.toHaveBeenCalled(); }); @@ -35,7 +35,11 @@ describe("resolveRunner", () => { expect( await resolveRunner(ctx, { autoLaunch: true, runner: undefined }, deps), - ).toEqual({ runnerId: "from-env", type: "resolved" }); + ).toEqual({ + runnerId: "from-env", + source: "environment", + type: "resolved", + }); }); it("falls back to the stored default", async () => { @@ -45,7 +49,7 @@ describe("resolveRunner", () => { expect( await resolveRunner(ctx, { autoLaunch: true, runner: undefined }, deps), - ).toEqual({ runnerId: "from-store", type: "resolved" }); + ).toEqual({ runnerId: "from-store", source: "stored", type: "resolved" }); }); it("ignores a blank environment variable", async () => { @@ -55,7 +59,7 @@ describe("resolveRunner", () => { expect( await resolveRunner(ctx, { autoLaunch: true, runner: undefined }, deps), - ).toEqual({ runnerId: "from-store", type: "resolved" }); + ).toEqual({ runnerId: "from-store", source: "stored", type: "resolved" }); }); // The caller has to be able to tell a fresh browser from one it already set @@ -67,7 +71,12 @@ describe("resolveRunner", () => { expect( await resolveRunner(ctx, { autoLaunch: true, runner: undefined }, deps), - ).toEqual({ runnerId: "cli-minted", type: "launched", url: launched.url }); + ).toEqual({ + runnerId: "cli-minted", + source: "launched", + type: "launched", + url: launched.url, + }); expect(callPublicApi).toHaveBeenCalledWith( publicContractsV1.runner.launch, diff --git a/src/domains/interactiveRunner/resolveRunner.ts b/src/domains/interactiveRunner/resolveRunner.ts index 1b6cbd273..1b385ff19 100644 --- a/src/domains/interactiveRunner/resolveRunner.ts +++ b/src/domains/interactiveRunner/resolveRunner.ts @@ -1,5 +1,6 @@ +import type { RunnerIdSource } from "~/core/interactiveRunner/runnerIdSource.js"; import { interactiveRunnerMessages } from "~/core/messages/index.js"; -import { resolveIdFrom } from "~/core/resolveId.js"; +import { resolveIdFrom, type ResolvedId } from "~/core/resolveId.js"; import type { AuthCommandContext } from "~/shell/commandContext.js"; import { exitCodes } from "~/shell/exit.js"; import { failureFields } from "~/shell/platform/requestWithRetry.js"; @@ -17,16 +18,22 @@ import { parseRunnerId } from "./runnerIds.js"; * same would leave an agent acting on a page it believes it already set up. */ export type ResolvedRunner = - | { type: "resolved"; runnerId: string } - | { type: "launched"; runnerId: string; url: string } + | ({ type: "resolved" } & TargetedRunner) + | ({ type: "launched"; url: string } & TargetedRunner) | { type: "failed"; error: string; errorBody?: string; exitCode: number }; +/** A runner a command can act on, and where its id was named. */ +export type TargetedRunner = { + runnerId: string; + source: RunnerIdSource; +}; + export const runnerIdEnvironmentVariable = "QAWOLF_RUNNER_ID"; const chooseRunnerId = ( runner: string | undefined, deps: InteractiveRunnerDeps, -): Promise => +): Promise => resolveIdFrom({ env: deps.env, environmentVariable: runnerIdEnvironmentVariable, @@ -50,9 +57,9 @@ export async function resolveRunner( ): Promise { const chosen = await chooseRunnerId(options.runner, deps); if (chosen !== undefined) { - const parsed = parseRunnerId(chosen); + const parsed = parseRunnerId(chosen.id); return parsed.ok - ? { runnerId: parsed.id, type: "resolved" } + ? { runnerId: parsed.id, source: chosen.source, type: "resolved" } : { error: parsed.error, exitCode: exitCodes.invalidArgs, @@ -82,6 +89,7 @@ export async function resolveRunner( } return { runnerId: launched.value.id, + source: "launched", type: "launched", url: launched.value.url, }; diff --git a/src/domains/interactiveRunner/runFlow.ts b/src/domains/interactiveRunner/runFlow.ts index 6a11dfd80..3e61fbcca 100644 --- a/src/domains/interactiveRunner/runFlow.ts +++ b/src/domains/interactiveRunner/runFlow.ts @@ -133,7 +133,7 @@ export async function handleRunnerRun( recorderSinceSequence, runEvents: options.runEvents, runId, - runnerId: resolved.runnerId, + runner: resolved, timeoutSeconds: timeout.seconds, }, deps, diff --git a/src/domains/interactiveRunner/runnerNotFound.test.ts b/src/domains/interactiveRunner/runnerNotFound.test.ts new file mode 100644 index 000000000..dd32ed7a3 --- /dev/null +++ b/src/domains/interactiveRunner/runnerNotFound.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, it } from "bun:test"; + +import { exitCodes } from "~/shell/exit.js"; + +import { makeAuthCtx, makeTestDeps } from "./deps.testUtils.js"; +import { handleRunnerEvents } from "./events.js"; +import { handleRunnerScreenshot } from "./takeScreenshot.js"; + +// What the platform layer builds from a 404 on a runner route, which is what a +// handler sees. +const notRunning = { + error: "Runner ci is not running (HTTP 404).", + errorBody: "It was never launched, or it has since been terminated.", + exitCode: exitCodes.notFound, + ok: false as const, +}; + +const eventsOptions = { + envelope: false, + follow: false, + run: undefined, + since: undefined, + stream: "run-status", + tail: undefined, + timeout: "60", +}; + +describe("a runner-targeting command whose runner is gone", () => { + it("names the runner and does not send the caller to --env", async () => { + const { callPublicApi, ctx } = makeAuthCtx(); + callPublicApi.mockResolvedValue(notRunning); + + const result = await handleRunnerScreenshot( + ctx, + { out: "shot.jpg", runner: "ci" }, + makeTestDeps(), + ); + + expect(result?.error).toBe(notRunning.error); + expect(result?.error).not.toContain("--env"); + }); + + // Exit 4 reads as "retry", and the published guidance says to. A terminated + // runner never comes back, so a caller that retries it burns its budget. + it("exits not-found rather than network", async () => { + const { callPublicApi, ctx } = makeAuthCtx(); + callPublicApi.mockResolvedValue(notRunning); + + const result = await handleRunnerScreenshot( + ctx, + { out: "shot.jpg", runner: "ci" }, + makeTestDeps(), + ); + + expect(result?.exitCode).toBe(exitCodes.notFound); + }); + + it.each([ + ["--runner", { runner: "ci" }, {}, "The id ci came from --runner."], + [ + "QAWOLF_RUNNER_ID", + { runner: undefined }, + { env: { QAWOLF_RUNNER_ID: "ci" } }, + "The id ci came from QAWOLF_RUNNER_ID.", + ], + ] as const)( + "says the id came from %s", + async (_name, options, deps, line) => { + const { callPublicApi, ctx } = makeAuthCtx(); + callPublicApi.mockResolvedValue(notRunning); + + const result = await handleRunnerScreenshot( + ctx, + { out: "shot.jpg", ...options }, + makeTestDeps(deps), + ); + + expect(result?.errorBody).toContain(line); + }, + ); + + it("says the id came from the directory's stored default", async () => { + const { callPublicApi, ctx } = makeAuthCtx(); + callPublicApi.mockResolvedValue(notRunning); + const deps = makeTestDeps(); + await deps.store.writeDefaultRunnerId("ci"); + + const result = await handleRunnerScreenshot( + ctx, + { out: "shot.jpg", runner: undefined }, + deps, + ); + + expect(result?.errorBody).toContain(".qawolf"); + }); + + // A journal read goes through its own result type, so it has to carry the + // same answer rather than collapsing back to a network failure. + it("answers a journal read the same way", async () => { + const { callPublicApi, ctx } = makeAuthCtx(); + callPublicApi.mockResolvedValue(notRunning); + + const result = await handleRunnerEvents( + ctx, + { ...eventsOptions, runner: "ci" }, + makeTestDeps(), + ); + + expect(result?.exitCode).toBe(exitCodes.notFound); + expect(result?.errorBody).toContain("The id ci came from --runner."); + }); +}); diff --git a/src/domains/interactiveRunner/runnerRequestFailure.test.ts b/src/domains/interactiveRunner/runnerRequestFailure.test.ts new file mode 100644 index 000000000..5b13f01b4 --- /dev/null +++ b/src/domains/interactiveRunner/runnerRequestFailure.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, it } from "bun:test"; + +import { exitCodes } from "~/shell/exit.js"; + +import { runnerRequestFailure } from "./runnerRequestFailure.js"; + +const notRunning = { + error: "Runner agent-1 is not running (HTTP 404).", + errorBody: "It was never launched, or it has since been terminated.", + exitCode: exitCodes.notFound, +}; + +describe("runnerRequestFailure", () => { + // A transcript that only says which runner was missed leaves a reader unable + // to tell what to go and change. + it.each([ + ["flag", "The id agent-1 came from --runner."], + ["environment", "The id agent-1 came from QAWOLF_RUNNER_ID."], + ["stored", "recorded in .qawolf"], + ["launched", "Runner agent-1 was launched for this command."], + ["given", "The id agent-1 is the one this call named."], + ] as const)("says a %s id was what chose the runner", (source, expected) => { + const failure = runnerRequestFailure(notRunning, { + runnerId: "agent-1", + source, + }); + + expect(failure.errorBody).toContain(expected); + }); + + it("keeps the explanation the platform layer built", () => { + const failure = runnerRequestFailure(notRunning, { + runnerId: "agent-1", + source: "flag", + }); + + expect(failure.errorBody).toContain("It was never launched"); + expect(failure.error).toBe(notRunning.error); + }); + + // Exits `notFound` rather than `network` so a caller can stop instead of + // retrying an id no amount of waiting brings back. + it("exits not-found rather than network", () => { + expect( + runnerRequestFailure(notRunning, { runnerId: "agent-1", source: "flag" }) + .exitCode, + ).toBe(exitCodes.notFound); + }); + + it("says where the id came from even when the platform gave no reason", () => { + const failure = runnerRequestFailure( + { error: notRunning.error, exitCode: exitCodes.notFound }, + { runnerId: "agent-1", source: "environment" }, + ); + + expect(failure.errorBody).toBe( + "The id agent-1 came from QAWOLF_RUNNER_ID.", + ); + }); + + it("leaves an unreachable platform as a network failure", () => { + const failure = runnerRequestFailure( + { error: "Could not reach the QA Wolf API." }, + { runnerId: "agent-1", source: "flag" }, + ); + + expect(failure).toEqual({ + error: "Could not reach the QA Wolf API.", + exitCode: exitCodes.network, + }); + }); + + it("keeps an exit code the platform layer already mapped", () => { + const failure = runnerRequestFailure( + { error: "rejected", exitCode: exitCodes.auth }, + { runnerId: "agent-1", source: "flag" }, + ); + + expect(failure.exitCode).toBe(exitCodes.auth); + }); +}); diff --git a/src/domains/interactiveRunner/runnerRequestFailure.ts b/src/domains/interactiveRunner/runnerRequestFailure.ts new file mode 100644 index 000000000..650f2a87c --- /dev/null +++ b/src/domains/interactiveRunner/runnerRequestFailure.ts @@ -0,0 +1,42 @@ +import { interactiveRunnerMessages } from "~/core/messages/index.js"; +import { exitCodes } from "~/shell/exit.js"; +import { + failureFields, + type PlatformFailure, +} from "~/shell/platform/requestWithRetry.js"; + +import type { TargetedRunner } from "./resolveRunner.js"; + +type RunnerFailure = { + error: string; + errorBody?: string; + exitCode: number; +}; + +/** + * A failed call to a runner, as a command result. + * + * The platform answers a runner that is not running with a 404, which is the + * one failure here that no amount of retrying clears — the pod was terminated + * or idled out, and only a launch brings one back. It exits `notFound` rather + * than `network` so a caller can stop instead of retrying a dead id, and it + * names where the id came from, since that is what a reader has to change. + */ +export function runnerRequestFailure( + failure: PlatformFailure, + runner: TargetedRunner, +): RunnerFailure { + const fields = failureFields(failure); + if (failure.exitCode !== exitCodes.notFound) { + return { ...fields, exitCode: failure.exitCode ?? exitCodes.network }; + } + const cameFrom = interactiveRunnerMessages.runnerIdCameFrom( + runner.runnerId, + runner.source, + ); + return { + ...fields, + errorBody: fields.errorBody ? `${fields.errorBody}\n${cameFrom}` : cameFrom, + exitCode: exitCodes.notFound, + }; +} diff --git a/src/domains/interactiveRunner/sendRunFlowRequest.ts b/src/domains/interactiveRunner/sendRunFlowRequest.ts index d3b20917b..b6d98ef1a 100644 --- a/src/domains/interactiveRunner/sendRunFlowRequest.ts +++ b/src/domains/interactiveRunner/sendRunFlowRequest.ts @@ -7,10 +7,10 @@ import { import { interactiveRunnerMessages } from "~/core/messages/index.js"; import type { RunnerApiContext } from "~/shell/commandContext.js"; import { exitCodes } from "~/shell/exit.js"; -import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { ResolvedRunner } from "./resolveRunner.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { type RunSubmitRefusal, describeRunSubmitFailure, @@ -60,10 +60,7 @@ export async function sendRunFlowRequest( ); if (!result.ok) { return { - failure: { - ...failureFields(result), - exitCode: result.exitCode ?? exitCodes.network, - }, + failure: runnerRequestFailure(result, options.resolved), type: "failed", }; } diff --git a/src/domains/interactiveRunner/stopRun.ts b/src/domains/interactiveRunner/stopRun.ts index 1f17e9f97..c0141e1be 100644 --- a/src/domains/interactiveRunner/stopRun.ts +++ b/src/domains/interactiveRunner/stopRun.ts @@ -10,6 +10,7 @@ import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { InteractiveRunnerDeps } from "./deps.js"; import { resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; /** @@ -42,7 +43,7 @@ export async function handleRunnerStopRun( runnerCallOptions, ); if (!result.ok) { - return { ...failureFields(result), exitCode: exitCodes.network }; + return runnerRequestFailure(result, resolved); } if (result.value.outcome === "failure") { diff --git a/src/domains/interactiveRunner/takeScreenshot.ts b/src/domains/interactiveRunner/takeScreenshot.ts index 139b2d6a9..4c0598645 100644 --- a/src/domains/interactiveRunner/takeScreenshot.ts +++ b/src/domains/interactiveRunner/takeScreenshot.ts @@ -14,6 +14,7 @@ import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { InteractiveRunnerDeps } from "./deps.js"; import { resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; /** @@ -69,9 +70,7 @@ export async function handleRunnerScreenshot( { id: resolved.runnerId }, runnerCallOptions, ); - if (!result.ok) { - return { ...failureFields(result), exitCode: exitCodes.network }; - } + if (!result.ok) return runnerRequestFailure(result, resolved); if (result.value.outcome === "success") { const written = await deps.writeScreenshot({ diff --git a/src/domains/interactiveRunner/terminate.ts b/src/domains/interactiveRunner/terminate.ts index 135bc272c..d905280f8 100644 --- a/src/domains/interactiveRunner/terminate.ts +++ b/src/domains/interactiveRunner/terminate.ts @@ -5,11 +5,11 @@ import type { AuthCommandContext, CommandResult, } from "~/shell/commandContext.js"; -import { exitCodes } from "~/shell/exit.js"; import { failureFields } from "~/shell/platform/requestWithRetry.js"; import type { InteractiveRunnerDeps } from "./deps.js"; import { resolveRunner } from "./resolveRunner.js"; +import { runnerRequestFailure } from "./runnerRequestFailure.js"; import { runnerCallOptions } from "./runnerCallOptions.js"; export async function handleRunnerTerminate( @@ -36,7 +36,7 @@ export async function handleRunnerTerminate( runnerCallOptions, ); if (!result.ok) { - return { ...failureFields(result), exitCode: exitCodes.network }; + return runnerRequestFailure(result, resolved); } // Whether it was running or already gone, this runner is not somewhere later diff --git a/src/runnerSdk/errorDetail.test.ts b/src/runnerSdk/errorDetail.test.ts new file mode 100644 index 000000000..6c4def84b --- /dev/null +++ b/src/runnerSdk/errorDetail.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from "bun:test"; + +import { exitCodes } from "~/shell/exit.js"; +import type { PlatformClient } from "~/shell/platform/createPlatformClient.js"; + +import type { SdkContext } from "./createContext.js"; +import { createLifecycleVerbs } from "./lifecycleVerbs.js"; + +// What the platform layer builds from a 404 on a runner route. +const notRunning = { + error: "Runner agent-1 is not running (HTTP 404).", + errorBody: [ + "It was never launched, or it has since been terminated or idled out.", + "Launch it with qawolf runner launch --id agent-1, or send this to a different runner with --runner.", + "The id agent-1 is the one this call named.", + ].join("\n"), + exitCode: exitCodes.notFound, + ok: false as const, +}; + +function lifecycleAnswering(answer: unknown) { + const platformClient = { + callPublicApi: async () => answer, + } as unknown as PlatformClient; + return createLifecycleVerbs({ platformClient } as unknown as SdkContext); +} + +describe("an SDK verb whose runner is gone", () => { + // Without this the caller got the headline alone, and the half that says + // what to do about it was built and then thrown away. + it("carries the detail line, not just the headline", async () => { + const result = await lifecycleAnswering(notRunning).keepalive({ + runnerId: "agent-1", + }); + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toBe(notRunning.error); + expect(result.errorDetail).toContain("qawolf runner launch --id agent-1"); + expect(result.errorDetail).toContain("the one this call named"); + }); + + it("leaves an unreachable runner with no detail to add", async () => { + const result = await lifecycleAnswering({ + ok: true, + value: { failureReason: "runner-unreachable", outcome: "failure" }, + }).keepalive({ runnerId: "agent-1" }); + + expect(result).toEqual({ + error: "The runner could not be reached.", + ok: false, + }); + }); +}); diff --git a/src/runnerSdk/givenRunner.ts b/src/runnerSdk/givenRunner.ts new file mode 100644 index 000000000..6b7755ee0 --- /dev/null +++ b/src/runnerSdk/givenRunner.ts @@ -0,0 +1,10 @@ +import type { TargetedRunner } from "~/domains/interactiveRunner/resolveRunner.js"; + +/** + * An SDK caller names the runner in the call itself, so there is no flag, + * variable or stored default behind it for a failure to report. + */ +export const givenRunner = (runnerId: string): TargetedRunner => ({ + runnerId, + source: "given", +}); diff --git a/src/runnerSdk/lifecycleVerbs.ts b/src/runnerSdk/lifecycleVerbs.ts index dfcc8eef6..095eafee5 100644 --- a/src/runnerSdk/lifecycleVerbs.ts +++ b/src/runnerSdk/lifecycleVerbs.ts @@ -4,6 +4,8 @@ import { readJournal } from "~/domains/interactiveRunner/readJournal.js"; import { runnerCallOptions } from "~/domains/interactiveRunner/runnerCallOptions.js"; import type { SdkContext } from "./createContext.js"; +import { givenRunner } from "./givenRunner.js"; +import { toSdkFailure } from "./toSdkFailure.js"; import { toSdkResult } from "./toSdkResult.js"; import type { KeptAlive, @@ -24,19 +26,15 @@ export function createLifecycleVerbs({ platformClient }: SdkContext) { async keepalive({ runnerId, }: RunnerRequest): Promise> { - const read = await readJournal(ctx, runnerId, { + const read = await readJournal(ctx, givenRunner(runnerId), { stream: "run-status", tail: 1, }); if (read.type === "read") return { ok: true, value: { id: runnerId } }; - return { - error: - read.type === "unreachable" - ? "The runner could not be reached." - : read.error, - ok: false, - }; + return read.type === "unreachable" + ? { error: "The runner could not be reached.", ok: false } + : toSdkFailure(read); }, async launch({ diff --git a/src/runnerSdk/runVerbs.ts b/src/runnerSdk/runVerbs.ts index 548268cb6..974dc640b 100644 --- a/src/runnerSdk/runVerbs.ts +++ b/src/runnerSdk/runVerbs.ts @@ -3,6 +3,8 @@ import { readJournal } from "~/domains/interactiveRunner/readJournal.js"; import { submitRun } from "~/domains/interactiveRunner/submitRun.js"; import type { SdkContext } from "./createContext.js"; +import { givenRunner } from "./givenRunner.js"; +import { toSdkFailure } from "./toSdkFailure.js"; import type { EventsRequest, Journal, @@ -34,7 +36,7 @@ export function createRunVerbs({ deps, platformClient }: SdkContext) { stream, window, }: EventsRequest): Promise> { - const read = await readJournal(ctx, runnerId, { + const read = await readJournal(ctx, givenRunner(runnerId), { stream, ...(runFilter === "all-runs" ? {} : { runId: runFilter.runId }), ...(window === "newest" @@ -45,13 +47,9 @@ export function createRunVerbs({ deps, platformClient }: SdkContext) { }); if (read.type === "read") return { ok: true, value: read.value }; - return { - error: - read.type === "unreachable" - ? "The runner could not be reached." - : read.error, - ok: false, - }; + return read.type === "unreachable" + ? { error: "The runner could not be reached.", ok: false } + : toSdkFailure(read); }, async run({ @@ -79,12 +77,12 @@ export function createRunVerbs({ deps, platformClient }: SdkContext) { environment: prepared.environment, environmentId: prepared.environmentId, files: prepared.files, - resolved: { runnerId, type: "resolved" }, + resolved: { ...givenRunner(runnerId), type: "resolved" }, selection: prepared.selection, }, deps, ); - if (!submitted.ok) return { error: submitted.error, ok: false }; + if (!submitted.ok) return toSdkFailure(submitted); return { ok: true, diff --git a/src/runnerSdk/toSdkFailure.ts b/src/runnerSdk/toSdkFailure.ts new file mode 100644 index 000000000..0654e2e22 --- /dev/null +++ b/src/runnerSdk/toSdkFailure.ts @@ -0,0 +1,20 @@ +import type { PlatformFailure } from "~/shell/platform/requestWithRetry.js"; + +import type { SdkResult } from "./types.js"; + +export type SdkFailure = Extract, { ok: false }>; + +/** + * A failure from an inner layer, as the SDK's own shape. + * + * Keeps the detail line rather than the headline alone: for a runner that is + * not running, the headline names it and the detail says why and what to do + * about it, which is the half a caller acts on. + */ +export function toSdkFailure(failure: PlatformFailure): SdkFailure { + return { + error: failure.error, + ...(failure.errorBody ? { errorDetail: failure.errorBody } : {}), + ok: false, + }; +} diff --git a/src/runnerSdk/toSdkResult.ts b/src/runnerSdk/toSdkResult.ts index cb577a7de..9baea2bf7 100644 --- a/src/runnerSdk/toSdkResult.ts +++ b/src/runnerSdk/toSdkResult.ts @@ -1,11 +1,10 @@ import type { PlatformResult } from "~/shell/platform/requestWithRetry.js"; +import { toSdkFailure } from "./toSdkFailure.js"; import type { SdkResult } from "./types.js"; export function toSdkResult( result: PlatformResult, ): SdkResult { - return result.ok - ? { ok: true, value: result.value } - : { error: result.error, ok: false }; + return result.ok ? { ok: true, value: result.value } : toSdkFailure(result); } diff --git a/src/runnerSdk/types.ts b/src/runnerSdk/types.ts index 2dfc250e3..30bfd8f53 100644 --- a/src/runnerSdk/types.ts +++ b/src/runnerSdk/types.ts @@ -21,7 +21,16 @@ export type RunnerSdkOptions = { }; export type SdkResult = - | { error: string; ok: false } + | { + error: string; + /** + * What the platform said beyond the headline, and what to do about it: + * the reason a runner is gone, which of `--runner`, `QAWOLF_RUNNER_ID` or + * the stored default named it, the command that brings one back. + */ + errorDetail?: string; + ok: false; + } | { ok: true; value: Value }; export type RunSelection = diff --git a/src/shell/exit.test.ts b/src/shell/exit.test.ts index 2229418eb..8b33379b0 100644 --- a/src/shell/exit.test.ts +++ b/src/shell/exit.test.ts @@ -66,6 +66,7 @@ describe("exit", () => { config: 5, timeout: 6, payment: 7, + notFound: 8, }); }); }); diff --git a/src/shell/exit.ts b/src/shell/exit.ts index 6e14faf32..1f4d9da4e 100644 --- a/src/shell/exit.ts +++ b/src/shell/exit.ts @@ -7,6 +7,7 @@ export const exitCodes = { config: 5, timeout: 6, payment: 7, + notFound: 8, } as const; type ExitCode = (typeof exitCodes)[keyof typeof exitCodes]; diff --git a/src/shell/platform/callPublicApi.notFound.test.ts b/src/shell/platform/callPublicApi.notFound.test.ts new file mode 100644 index 000000000..34779b5fa --- /dev/null +++ b/src/shell/platform/callPublicApi.notFound.test.ts @@ -0,0 +1,68 @@ +import { afterEach, describe, expect, it, mock } from "bun:test"; +import { publicContractsV1 } from "@qawolf/api-contracts/v1"; + +import { createPlatformClient } from "./createPlatformClient.js"; + +afterEach(() => { + mock.restore(); +}); + +const baseUrl = "https://test.qawolf.com"; + +// What apex answers today, before WIZ-12139 names the missing runner: the CLI +// has to stand on its own wording against this body. +function notFound(): typeof fetch { + return mock().mockResolvedValue( + new Response( + JSON.stringify({ error: { json: { message: "Not found" } } }), + { + headers: { "content-type": "application/json" }, + status: 404, + }, + ), + ) as unknown as typeof fetch; +} + +const client = () => + createPlatformClient("qawolf_key", { + baseUrl, + fetch: notFound(), + sleep: async () => {}, + }); + +describe("a public API 404", () => { + it("reads a runner route as a runner that is not running", async () => { + const result = await client().callPublicApi( + publicContractsV1.runner.takeScreenshot, + { id: "agent-1" }, + ); + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toContain("Runner agent-1 is not running"); + expect(result.error).not.toContain("--env"); + }); + + it("reads a run lookup as a run this team does not hold", async () => { + const result = await client().callPublicApi(publicContractsV1.run.get, { + runId: "abc", + }); + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toContain("no run abc on this team"); + expect(result.errorBody).toContain("qawolf runner run"); + }); + + // The one case the old wording was true of, and the only one that keeps it. + it("keeps pointing an environment-scoped route at --env", async () => { + const result = await client().callPublicApi(publicContractsV1.run.create, { + environmentId: "env-1", + flowIds: ["flow-1"], + }); + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toContain("Check the --env value"); + }); +}); diff --git a/src/shell/platform/callPublicApi.ts b/src/shell/platform/callPublicApi.ts index fe314dcf4..5ecc41d70 100644 --- a/src/shell/platform/callPublicApi.ts +++ b/src/shell/platform/callPublicApi.ts @@ -1,6 +1,8 @@ import { type PublicApiContractKind } from "@qawolf/api-contracts/v1"; import type { z } from "zod"; +import { notFoundSubject } from "~/core/publicApi/notFoundSubject.js"; + import { applyWorkspaceId } from "./applyWorkspaceId.js"; import { describeRequestError } from "./describeErrors.js"; import type { @@ -59,17 +61,17 @@ export function makeCallPublicApiMethod( deps: MethodDeps, readBackoffMs: readonly number[], ): CallPublicApiMethod { - return async (contract, input, options) => - requestWithRetry({ - call: () => - callPublicApi( - trpc, - contract, - applyWorkspaceId(contract.input, input, deps.workspaceId), - options, - ), + return async (contract, input, options) => { + const sent = applyWorkspaceId(contract.input, input, deps.workspaceId); + // Read from the request rather than from the reply, so a 404 names what was + // asked for even when the platform answers a bare "Not found". + const subject = notFoundSubject(contract.name, sent); + return requestWithRetry({ + call: () => callPublicApi(trpc, contract, sent, options), backoffMs: contract.kind === "read" ? readBackoffMs : [], - describe: (err) => describeRequestError(err, deps.baseUrl, contract.name), + describe: (err) => + describeRequestError(err, deps.baseUrl, contract.name, subject), sleep: deps.sleep, }); + }; } diff --git a/src/shell/platform/describeErrors.test.ts b/src/shell/platform/describeErrors.test.ts index de5dc6e6e..0d87ac868 100644 --- a/src/shell/platform/describeErrors.test.ts +++ b/src/shell/platform/describeErrors.test.ts @@ -18,7 +18,9 @@ const envelope = (message: string) => JSON.stringify({ error: { json: { message } } }); describe("describeRequestError", () => { - it.each([401, 402, 403, 404, 500])( + // A 404 leads with the reason instead of carrying it underneath, so it is + // covered by describeNotFound's own tests. + it.each([401, 402, 403, 500])( "carries the server's reason on HTTP %i", (status) => { const described = describeRequestError( @@ -32,7 +34,7 @@ describe("describeRequestError", () => { }, ); - it.each([401, 402, 403, 404, 500])( + it.each([401, 402, 403, 500])( "omits the body entirely when HTTP %i carries no reason", (status) => { const described = describeRequestError( @@ -65,18 +67,21 @@ describe("describeRequestError", () => { expect(described.exitCode).toBe(exitCodes.payment); }); - it.each([403, 404, 500])( - "leaves the exit code unset on HTTP %i", - (status) => { - const described = describeRequestError( - httpError(status), - baseUrl, - "run.create", - ); + it.each([403, 500])("leaves the exit code unset on HTTP %i", (status) => { + const described = describeRequestError( + httpError(status), + baseUrl, + "run.create", + ); - expect("exitCode" in described).toBe(false); - }, - ); + expect("exitCode" in described).toBe(false); + }); + + it("maps HTTP 404 to the not-found exit code", () => { + const described = describeRequestError(httpError(404), baseUrl, "run.get"); + + expect(described.exitCode).toBe(exitCodes.notFound); + }); it("omits the body for a network failure", () => { const described = describeRequestError( diff --git a/src/shell/platform/describeErrors.ts b/src/shell/platform/describeErrors.ts index 5089f4f79..e34316634 100644 --- a/src/shell/platform/describeErrors.ts +++ b/src/shell/platform/describeErrors.ts @@ -1,7 +1,9 @@ import { formatSeconds } from "~/core/formatSeconds.js"; import { authMessages } from "~/core/messages/index.js"; +import type { NotFoundSubject } from "~/core/publicApi/notFoundSubject.js"; import { exitCodes } from "~/shell/exit.js"; import type { WireError } from "./createTrpcClient.js"; +import { describeNotFound } from "./describeNotFound.js"; import { parseErrorBody } from "./parseErrorBody.js"; import type { PlatformFailure } from "./requestWithRetry.js"; @@ -43,6 +45,7 @@ export function describeRequestError( err: WireError, baseUrl: string, noun?: string, + notFound?: NotFoundSubject, ): PlatformFailure { if (err.kind === "http") { const reason = parseErrorBody(err.body); @@ -63,8 +66,7 @@ export function describeRequestError( }; if (err.status === 403) return { error: m.request.rejected403(noun), ...body }; - if (err.status === 404) - return { error: m.request.notFound404(noun), ...body }; + if (err.status === 404) return describeNotFound(notFound, noun, reason); return { error: m.request.failedWithStatus(err.status, noun), ...body }; } if (err.kind === "network") { diff --git a/src/shell/platform/describeNotFound.test.ts b/src/shell/platform/describeNotFound.test.ts new file mode 100644 index 000000000..fd9d329e5 --- /dev/null +++ b/src/shell/platform/describeNotFound.test.ts @@ -0,0 +1,162 @@ +import { describe, expect, it } from "bun:test"; + +import { exitCodes } from "~/shell/exit.js"; + +import { describeNotFound } from "./describeNotFound.js"; + +// What apex answers today, before WIZ-12139 names the missing runner. +const bareNotFound = "Not found"; + +describe("describeNotFound", () => { + describe("a runner that is not running", () => { + const runner = { kind: "runner", runnerId: "agent-1" } as const; + + it("names the runner and how to bring one back", () => { + const described = describeNotFound(runner, "runner.runFlow", ""); + + expect(described.error).toContain("Runner agent-1 is not running"); + expect(described.errorBody).toContain( + "qawolf runner launch --id agent-1", + ); + expect(described.errorBody).toContain("--runner"); + expect(described.exitCode).toBe(exitCodes.notFound); + }); + + it("never blames the environment", () => { + const described = describeNotFound(runner, "runner.runFlow", ""); + + expect(described.error).not.toContain("--env"); + expect(described.error).not.toContain("environment"); + }); + + // An old server answers a bare "Not found", which repeats the status and + // crowds out the wording a reader can act on. + it("explains itself when the server said only that it was not found", () => { + const described = describeNotFound( + runner, + "runner.runFlow", + bareNotFound, + ); + + expect(described.errorBody).toContain( + "It was never launched, or it has since been terminated or idled out.", + ); + }); + + // Apex answers " not found" on several routes, which repeats the + // status rather than explaining it. + it.each(["Not found", "Runner not found", "runner not found."])( + "treats %p as no reason at all", + (bare) => { + const described = describeNotFound(runner, "runner.runFlow", bare); + + expect(described.error).toBe( + "Runner agent-1 is not running (HTTP 404).", + ); + expect(described.errorBody).toContain("It was never launched"); + }, + ); + + it("prefers the server's reason when it has one", () => { + const described = describeNotFound( + runner, + "runner.runFlow", + "Runner agent-1 was terminated 4 minutes ago.", + ); + + expect(described.error).toBe( + "Runner agent-1 was terminated 4 minutes ago. (HTTP 404)", + ); + expect(described.errorBody).not.toContain("It was never launched"); + }); + + // Launching is the fix whatever the reason turns out to be. + it("still offers the launch command when the server explained", () => { + const described = describeNotFound( + runner, + "runner.runFlow", + "Runner agent-1 was terminated 4 minutes ago.", + ); + + expect(described.errorBody).toBe( + "Launch it with qawolf runner launch --id agent-1, or send this to a different runner with --runner.", + ); + }); + + it("still reads as a missing runner when no id reached it", () => { + const described = describeNotFound( + { kind: "runner", runnerId: undefined }, + "runner.runFlow", + "", + ); + + expect(described.error).toContain("not running"); + expect(described.exitCode).toBe(exitCodes.notFound); + }); + }); + + describe("a run the platform does not hold", () => { + // A run that is still being created also answers 404, and that one clears + // by waiting. Guessing "the id is runner-local" over the top of the + // platform saying so would send a caller to the wrong fix. + it("says nothing of its own once the platform has explained", () => { + const described = describeNotFound( + { kind: "run", runId: "abc" }, + "run.get", + "Run is still being created. Try again in a few seconds.", + ); + + expect(described.error).toBe( + "Run is still being created. Try again in a few seconds. (HTTP 404)", + ); + expect("errorBody" in described).toBe(false); + }); + it("says the id may be one only the runner knows", () => { + const described = describeNotFound( + { kind: "run", runId: "abc" }, + "run.get", + bareNotFound, + ); + + expect(described.error).toContain("no run abc on this team"); + expect(described.errorBody).toContain("qawolf runner run"); + expect(described.errorBody).toContain( + "qawolf runner events run-status --run abc", + ); + }); + }); + + // The one place the old wording was true. + describe("a request scoped to an environment", () => { + it("keeps pointing at --env", () => { + const described = describeNotFound( + { kind: "environment" }, + "run.create", + "", + ); + + expect(described.error).toContain("Check the --env value"); + }); + + it("answers a caller that named no subject the same way", () => { + const described = describeNotFound(undefined, "env-vars", ""); + + expect(described.error).toContain("Check the --env value"); + }); + }); + + describe("anything else", () => { + it("names what was asked for without blaming an environment", () => { + const described = describeNotFound( + { kind: "other" }, + "trigger.get", + bareNotFound, + ); + + expect(described.error).toBe( + "QA Wolf API could not find trigger.get (HTTP 404).", + ); + expect("errorBody" in described).toBe(false); + }); + }); +}); diff --git a/src/shell/platform/describeNotFound.ts b/src/shell/platform/describeNotFound.ts new file mode 100644 index 000000000..2dc7c7a0b --- /dev/null +++ b/src/shell/platform/describeNotFound.ts @@ -0,0 +1,73 @@ +import { authMessages } from "~/core/messages/index.js"; +import type { NotFoundSubject } from "~/core/publicApi/notFoundSubject.js"; +import { exitCodes } from "~/shell/exit.js"; + +import type { PlatformFailure } from "./requestWithRetry.js"; + +const m = authMessages.errors.request; + +/** + * A server reason that only repeats what the status already said, including the + * " not found" apex answers for several routes. Dropping it leaves room + * for wording a reader can act on. + */ +const saysNothingMore = (reason: string): boolean => + /^(\w+ )*not ?found\.?$/i.test(reason.trim()); + +/** What the CLI says about a 404 when the platform explained nothing. */ +type OwnWording = { + /** The line that stands in for a reason. */ + statement: string; + /** Why it happened. A guess, so it is dropped once the platform says. */ + guess?: string; + /** What to do about it, true whatever the reason turns out to be. */ + guidance?: string; +}; + +/** + * How a 404 reads: the request's own subject, not the environment. Only a route + * that actually resolves an environment keeps the wording that blames one, and + * a caller with no subject at all is one of the environment-scoped reads. + * + * The platform's own reason leads when it has one. It knows things this layer + * can only guess at — a run that answers 404 because it is still being created + * clears by waiting, which no wording invented here would have said. + */ +export function describeNotFound( + subject: NotFoundSubject | undefined, + noun: string | undefined, + reason: string, +): PlatformFailure { + const explained = reason && !saysNothingMore(reason) ? reason : undefined; + const say = (own: OwnWording): PlatformFailure => { + const body = [explained ? undefined : own.guess, own.guidance].filter( + (line) => line, + ); + return { + error: explained ? `${explained} (HTTP 404)` : own.statement, + exitCode: exitCodes.notFound, + ...(body.length > 0 ? { errorBody: body.join("\n") } : {}), + }; + }; + + switch (subject?.kind) { + case "runner": + return say({ + guess: m.runnerIsGone, + guidance: m.launchTheRunner(subject.runnerId), + statement: m.notFound404Runner(subject.runnerId), + }); + case "run": + return say({ + guess: m.runIdMayBeRunnerLocal(subject.runId), + statement: m.notFound404Run(subject.runId), + }); + case "other": + return say({ statement: m.notFound404(noun) }); + // A caller that named no subject is one of the environment-scoped reads: + // the flow bundle, an environment's variables, the team's storage. + case "environment": + case undefined: + return say({ statement: m.notFound404Environment(noun) }); + } +}