From 8dba7b17ce5e5254dde8e649507f746e66e25be1 Mon Sep 17 00:00:00 2001 From: Goran Gajic Date: Thu, 17 Sep 2026 13:34:17 +0200 Subject: [PATCH 1/5] feat(cli): add a not-found exit code Exit 4 means the platform could not be reached, which reads as "retry". A 404 fell through to the default code, so a caller could not tell a missing runner or run from a flow failure without parsing the message. --- docs/exit-codes.md | 1 + src/shell/exit.test.ts | 1 + src/shell/exit.ts | 1 + 3 files changed, 3 insertions(+) diff --git a/docs/exit-codes.md b/docs/exit-codes.md index fafe0e06d..4dbf3d2f7 100644 --- a/docs/exit-codes.md +++ b/docs/exit-codes.md @@ -12,6 +12,7 @@ CI consumers depend on consistent exit codes. The CLI commits to the following c | `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 does not exist. Unlike `4`, waiting and retrying never clears it — launch a runner, or name one that is running. | ## Using the helper 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]; From d80e5a660d0ac3df02ef541f126a004417eaed25 Mon Sep 17 00:00:00 2001 From: Goran Gajic Date: Thu, 17 Sep 2026 13:34:30 +0200 Subject: [PATCH 2/5] fix(runner): say what a 404 could not find instead of blaming --env Every 404 read as an environment problem, whatever the request had named. A runner route now says the runner is not running and how to launch one, a run lookup says there is no such run on this team and that ids from qawolf runner run are the runner's own, and only a request that really is scoped to an environment still points at --env. The server's own reason is preferred over anything invented here, so a platform that names the missing runner speaks for itself; the CLI's wording stands alone against the bare "Not found" older servers answer. --- skills/qawolf-cli/references/run-results.md | 4 + src/commands/qawolfCliRunResults.template.md | 4 + src/core/messages/authErrors.ts | 21 +++- src/core/publicApi/notFoundSubject.test.ts | 53 ++++++++ src/core/publicApi/notFoundSubject.ts | 54 ++++++++ .../platform/callPublicApi.notFound.test.ts | 68 ++++++++++ src/shell/platform/callPublicApi.ts | 22 ++-- src/shell/platform/describeErrors.test.ts | 25 ++-- src/shell/platform/describeErrors.ts | 6 +- src/shell/platform/describeNotFound.test.ts | 117 ++++++++++++++++++ src/shell/platform/describeNotFound.ts | 52 ++++++++ 11 files changed, 402 insertions(+), 24 deletions(-) create mode 100644 src/core/publicApi/notFoundSubject.test.ts create mode 100644 src/core/publicApi/notFoundSubject.ts create mode 100644 src/shell/platform/callPublicApi.notFound.test.ts create mode 100644 src/shell/platform/describeNotFound.test.ts create mode 100644 src/shell/platform/describeNotFound.ts diff --git a/skills/qawolf-cli/references/run-results.md b/skills/qawolf-cli/references/run-results.md index da9b37e2a..18d1add18 100644 --- a/skills/qawolf-cli/references/run-results.md +++ b/skills/qawolf-cli/references/run-results.md @@ -25,6 +25,10 @@ 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 `. + Poll `status` until it reaches `passed`, `failed` or `canceled`. The other values mean the run is still going. diff --git a/src/commands/qawolfCliRunResults.template.md b/src/commands/qawolfCliRunResults.template.md index 8b8e5afdf..bffc72b38 100644 --- a/src/commands/qawolfCliRunResults.template.md +++ b/src/commands/qawolfCliRunResults.template.md @@ -25,6 +25,10 @@ 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 `. + Poll `status` until it reaches `passed`, `failed` or `canceled`. The other values mean the run is still going. diff --git a/src/core/messages/authErrors.ts b/src/core/messages/authErrors.ts index c4e539cdd..12939f5c8 100644 --- a/src/core/messages/authErrors.ts +++ b/src/core/messages/authErrors.ts @@ -19,8 +19,27 @@ 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). Launch one with qawolf runner launch --id , or name a running one with --runner." + : `Runner ${runnerId} is not running (HTTP 404). Launch it with qawolf runner launch --id ${runnerId}, or send this to a different runner with --runner.`, + /** 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.", + 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. */ + 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/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/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..2dde53d73 100644 --- a/src/shell/platform/describeErrors.test.ts +++ b/src/shell/platform/describeErrors.test.ts @@ -65,18 +65,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..a39a50015 --- /dev/null +++ b/src/shell/platform/describeNotFound.test.ts @@ -0,0 +1,117 @@ +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.error).toContain("qawolf runner launch --id agent-1"); + expect(described.error).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).toBe( + "It was never launched, or it has since been terminated or idled out.", + ); + }); + + 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.errorBody).toBe( + "Runner agent-1 was terminated 4 minutes ago.", + ); + }); + + 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", () => { + 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..251247424 --- /dev/null +++ b/src/shell/platform/describeNotFound.ts @@ -0,0 +1,52 @@ +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. Dropping it + * leaves room for wording a reader can act on, and a server that names the + * missing thing instead (WIZ-12139) is preferred over anything invented here. + */ +const saysNothingMore = (reason: string): boolean => + /^not ?found\.?$/i.test(reason.trim()); + +/** + * 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. + */ +export function describeNotFound( + subject: NotFoundSubject | undefined, + noun: string | undefined, + reason: string, +): PlatformFailure { + const explain = (error: string, fallback: string): PlatformFailure => { + const detail = reason && !saysNothingMore(reason) ? reason : fallback; + return { + error, + exitCode: exitCodes.notFound, + ...(detail ? { errorBody: detail } : {}), + }; + }; + + switch (subject?.kind) { + case "runner": + return explain(m.notFound404Runner(subject.runnerId), m.runnerIsGone); + case "run": + return explain( + m.notFound404Run(subject.runId), + m.runIdMayBeRunnerLocal(subject.runId), + ); + case "other": + return explain(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 explain(m.notFound404Environment(noun), ""); + } +} From 2b538b33b37275ddbbfa14236b7860ff9e2f57bd Mon Sep 17 00:00:00 2001 From: Goran Gajic Date: Thu, 17 Sep 2026 13:34:43 +0200 Subject: [PATCH 3/5] feat(runner): say where a runner id came from when it is not running Commands pick a runner from --runner, then QAWOLF_RUNNER_ID, then the directory's stored default, and none of that reached the failure. Whoever read a transcript of a session driving a terminated runner could not tell which runner was meant or why that id was chosen. The failure now names the id and which of the three chose it, and exits not-found rather than network, so a caller stops instead of retrying an id no amount of waiting brings back. --- .changeset/not-found-names-what-is-missing.md | 7 ++ skills/qawolf-cli/references/runner.md | 20 ++-- src/core/interactiveRunner/runnerIdSource.ts | 8 ++ .../messages/interactiveRunner/lifecycle.ts | 14 +++ src/core/resolveId.test.ts | 29 +++-- src/core/resolveId.ts | 22 +++- src/domains/agent/resolveSessionId.ts | 16 +-- .../interactiveRunner/evaluateSnippet.ts | 6 +- src/domains/interactiveRunner/events.ts | 2 +- .../interactiveRunner/followPrinters.ts | 17 +-- .../followRun.events.test.ts | 2 +- .../interactiveRunner/followRun.quiet.test.ts | 2 +- .../interactiveRunner/followRun.test.ts | 2 +- src/domains/interactiveRunner/followRun.ts | 4 +- .../interactiveRunner/highlightSelector.ts | 3 +- .../interactiveRunner/importPackage.ts | 6 +- src/domains/interactiveRunner/inspect.ts | 3 +- .../interactiveRunner/inspectMobile.ts | 3 +- .../interactiveRunner/journalCursor.test.ts | 14 ++- .../interactiveRunner/journalCursor.ts | 9 +- src/domains/interactiveRunner/keepalive.ts | 2 +- .../interactiveRunner/performAction.ts | 24 ++-- .../interactiveRunner/promoteSnapshot.ts | 3 +- src/domains/interactiveRunner/readJournal.ts | 13 +- .../interactiveRunner/resolveRunner.test.ts | 19 ++- .../interactiveRunner/resolveRunner.ts | 20 +++- src/domains/interactiveRunner/runFlow.ts | 2 +- .../interactiveRunner/runnerNotFound.test.ts | 112 ++++++++++++++++++ .../runnerRequestFailure.test.ts | 81 +++++++++++++ .../interactiveRunner/runnerRequestFailure.ts | 42 +++++++ .../interactiveRunner/sendRunFlowRequest.ts | 7 +- src/domains/interactiveRunner/stopRun.ts | 3 +- .../interactiveRunner/takeScreenshot.ts | 5 +- src/domains/interactiveRunner/terminate.ts | 4 +- src/runnerSdk/givenRunner.ts | 10 ++ src/runnerSdk/lifecycleVerbs.ts | 3 +- src/runnerSdk/runVerbs.ts | 5 +- 37 files changed, 434 insertions(+), 110 deletions(-) create mode 100644 .changeset/not-found-names-what-is-missing.md create mode 100644 src/core/interactiveRunner/runnerIdSource.ts create mode 100644 src/domains/interactiveRunner/runnerNotFound.test.ts create mode 100644 src/domains/interactiveRunner/runnerRequestFailure.test.ts create mode 100644 src/domains/interactiveRunner/runnerRequestFailure.ts create mode 100644 src/runnerSdk/givenRunner.ts 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..74ec0e98b --- /dev/null +++ b/.changeset/not-found-names-what-is-missing.md @@ -0,0 +1,7 @@ +--- +"@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`. + +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/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/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/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/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/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..18ebd9986 100644 --- a/src/runnerSdk/lifecycleVerbs.ts +++ b/src/runnerSdk/lifecycleVerbs.ts @@ -4,6 +4,7 @@ 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 { toSdkResult } from "./toSdkResult.js"; import type { KeptAlive, @@ -24,7 +25,7 @@ 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, }); diff --git a/src/runnerSdk/runVerbs.ts b/src/runnerSdk/runVerbs.ts index 548268cb6..17d7d84c6 100644 --- a/src/runnerSdk/runVerbs.ts +++ b/src/runnerSdk/runVerbs.ts @@ -3,6 +3,7 @@ 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 type { EventsRequest, Journal, @@ -34,7 +35,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" @@ -79,7 +80,7 @@ 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, From 92f0b2f956fb294477342af8dbd1461a711b13cf Mon Sep 17 00:00:00 2001 From: Goran Gajic Date: Thu, 17 Sep 2026 21:23:02 +0200 Subject: [PATCH 4/5] fix(cli): let the platform's own reason lead a 404 A run that is still being created answers 404 and clears by waiting. The CLI printed "QA Wolf has no run X on this team" over the top of the server saying so, and the exit-code doc promised the status never clears. The platform's sentence is now the message whenever it has one, and the CLI's guess is dropped rather than argued with. What to do next is kept either way, so a missing runner still carries its launch command. --- docs/exit-codes.md | 22 ++++---- skills/qawolf-cli/references/run-results.md | 4 ++ src/commands/qawolfCliRunResults.template.md | 4 ++ src/core/messages/authErrors.ts | 15 ++++-- src/shell/platform/describeErrors.test.ts | 6 ++- src/shell/platform/describeNotFound.test.ts | 53 ++++++++++++++++++-- src/shell/platform/describeNotFound.ts | 51 +++++++++++++------ 7 files changed, 120 insertions(+), 35 deletions(-) diff --git a/docs/exit-codes.md b/docs/exit-codes.md index 4dbf3d2f7..3b470d50e 100644 --- a/docs/exit-codes.md +++ b/docs/exit-codes.md @@ -2,17 +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. | -| `8` | `notFound` | The QA Wolf API answered HTTP 404: the runner, run, or other thing the command named does not exist. Unlike `4`, waiting and retrying never clears it — launch a runner, or name one that is running. | +| 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 18d1add18..de8b235e3 100644 --- a/skills/qawolf-cli/references/run-results.md +++ b/skills/qawolf-cli/references/run-results.md @@ -29,6 +29,10 @@ Use the returned value for follow-up calls. 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/commands/qawolfCliRunResults.template.md b/src/commands/qawolfCliRunResults.template.md index bffc72b38..6057cd9a6 100644 --- a/src/commands/qawolfCliRunResults.template.md +++ b/src/commands/qawolfCliRunResults.template.md @@ -29,6 +29,10 @@ Use the returned value for follow-up calls. 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/messages/authErrors.ts b/src/core/messages/authErrors.ts index 12939f5c8..c27aea40d 100644 --- a/src/core/messages/authErrors.ts +++ b/src/core/messages/authErrors.ts @@ -28,14 +28,23 @@ export const authErrorMessages = { `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). Launch one with qawolf runner launch --id , or name a running one with --runner." - : `Runner ${runnerId} is not running (HTTP 404). Launch it with qawolf runner launch --id ${runnerId}, or send this to a different runner with --runner.`, + ? "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. */ + /** + * 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) => diff --git a/src/shell/platform/describeErrors.test.ts b/src/shell/platform/describeErrors.test.ts index 2dde53d73..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( diff --git a/src/shell/platform/describeNotFound.test.ts b/src/shell/platform/describeNotFound.test.ts index a39a50015..fd9d329e5 100644 --- a/src/shell/platform/describeNotFound.test.ts +++ b/src/shell/platform/describeNotFound.test.ts @@ -15,8 +15,10 @@ describe("describeNotFound", () => { const described = describeNotFound(runner, "runner.runFlow", ""); expect(described.error).toContain("Runner agent-1 is not running"); - expect(described.error).toContain("qawolf runner launch --id agent-1"); - expect(described.error).toContain("--runner"); + expect(described.errorBody).toContain( + "qawolf runner launch --id agent-1", + ); + expect(described.errorBody).toContain("--runner"); expect(described.exitCode).toBe(exitCodes.notFound); }); @@ -36,11 +38,25 @@ describe("describeNotFound", () => { bareNotFound, ); - expect(described.errorBody).toBe( + 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, @@ -48,9 +64,23 @@ describe("describeNotFound", () => { "Runner agent-1 was terminated 4 minutes ago.", ); - expect(described.errorBody).toBe( + 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", () => { @@ -66,6 +96,21 @@ describe("describeNotFound", () => { }); 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" }, diff --git a/src/shell/platform/describeNotFound.ts b/src/shell/platform/describeNotFound.ts index 251247424..2dc7c7a0b 100644 --- a/src/shell/platform/describeNotFound.ts +++ b/src/shell/platform/describeNotFound.ts @@ -7,46 +7,67 @@ import type { PlatformFailure } from "./requestWithRetry.js"; const m = authMessages.errors.request; /** - * A server reason that only repeats what the status already said. Dropping it - * leaves room for wording a reader can act on, and a server that names the - * missing thing instead (WIZ-12139) is preferred over anything invented here. + * 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 => - /^not ?found\.?$/i.test(reason.trim()); + /^(\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 explain = (error: string, fallback: string): PlatformFailure => { - const detail = reason && !saysNothingMore(reason) ? reason : fallback; + 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, + error: explained ? `${explained} (HTTP 404)` : own.statement, exitCode: exitCodes.notFound, - ...(detail ? { errorBody: detail } : {}), + ...(body.length > 0 ? { errorBody: body.join("\n") } : {}), }; }; switch (subject?.kind) { case "runner": - return explain(m.notFound404Runner(subject.runnerId), m.runnerIsGone); + return say({ + guess: m.runnerIsGone, + guidance: m.launchTheRunner(subject.runnerId), + statement: m.notFound404Runner(subject.runnerId), + }); case "run": - return explain( - m.notFound404Run(subject.runId), - m.runIdMayBeRunnerLocal(subject.runId), - ); + return say({ + guess: m.runIdMayBeRunnerLocal(subject.runId), + statement: m.notFound404Run(subject.runId), + }); case "other": - return explain(m.notFound404(noun), ""); + 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 explain(m.notFound404Environment(noun), ""); + return say({ statement: m.notFound404Environment(noun) }); } } From c9d61b999e0cd5ae2527d8600b5c52e48b0cb979 Mon Sep 17 00:00:00 2001 From: Goran Gajic Date: Thu, 17 Sep 2026 21:24:54 +0200 Subject: [PATCH 5/5] feat(sdk): carry the detail line on a failed runner-sdk call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SdkResult held only the headline, so the half a caller acts on — why a runner is gone, which of the flag, the variable or the stored default named it, the command that brings one back — was built and then dropped. --- .changeset/not-found-names-what-is-missing.md | 2 + src/runnerSdk/errorDetail.test.ts | 54 +++++++++++++++++++ src/runnerSdk/lifecycleVerbs.ts | 11 ++-- src/runnerSdk/runVerbs.ts | 13 ++--- src/runnerSdk/toSdkFailure.ts | 20 +++++++ src/runnerSdk/toSdkResult.ts | 5 +- src/runnerSdk/types.ts | 11 +++- 7 files changed, 97 insertions(+), 19 deletions(-) create mode 100644 src/runnerSdk/errorDetail.test.ts create mode 100644 src/runnerSdk/toSdkFailure.ts diff --git a/.changeset/not-found-names-what-is-missing.md b/.changeset/not-found-names-what-is-missing.md index 74ec0e98b..d076ec5d5 100644 --- a/.changeset/not-found-names-what-is-missing.md +++ b/.changeset/not-found-names-what-is-missing.md @@ -4,4 +4,6 @@ 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/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/lifecycleVerbs.ts b/src/runnerSdk/lifecycleVerbs.ts index 18ebd9986..095eafee5 100644 --- a/src/runnerSdk/lifecycleVerbs.ts +++ b/src/runnerSdk/lifecycleVerbs.ts @@ -5,6 +5,7 @@ import { runnerCallOptions } from "~/domains/interactiveRunner/runnerCallOptions import type { SdkContext } from "./createContext.js"; import { givenRunner } from "./givenRunner.js"; +import { toSdkFailure } from "./toSdkFailure.js"; import { toSdkResult } from "./toSdkResult.js"; import type { KeptAlive, @@ -31,13 +32,9 @@ export function createLifecycleVerbs({ platformClient }: SdkContext) { }); 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 17d7d84c6..974dc640b 100644 --- a/src/runnerSdk/runVerbs.ts +++ b/src/runnerSdk/runVerbs.ts @@ -4,6 +4,7 @@ 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, @@ -46,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({ @@ -85,7 +82,7 @@ export function createRunVerbs({ deps, platformClient }: SdkContext) { }, 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 =