diff --git a/.changeset/name-the-missing-record.md b/.changeset/name-the-missing-record.md new file mode 100644 index 000000000..8a1f08ffc --- /dev/null +++ b/.changeset/name-the-missing-record.md @@ -0,0 +1,5 @@ +--- +"@qawolf/cli": patch +--- + +A 404 on a trigger, issue, flow, agent session or file now names that record and the id it was asked for — `QA Wolf has no trigger trg-1 (HTTP 404).` It used to name the endpoint instead, which read as though the endpoint itself were gone. A request with no such id to name says it matched nothing, rather than that it could not be found. diff --git a/src/core/messages/authErrors.ts b/src/core/messages/authErrors.ts index c27aea40d..34e5f5a8d 100644 --- a/src/core/messages/authErrors.ts +++ b/src/core/messages/authErrors.ts @@ -47,8 +47,14 @@ export const authErrorMessages = { */ 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 ?? ""}.`, + notFound404Record: (noun: string, id: string) => + `QA Wolf has no ${noun} ${id} (HTTP 404).`, + // Says the request matched nothing rather than that the endpoint is + // missing, which is how "could not find " read. notFound404: (noun: string | undefined) => - `QA Wolf API could not find ${noun ?? "what the request named"} (HTTP 404).`, + noun === undefined + ? "QA Wolf found nothing matching the request (HTTP 404)." + : `QA Wolf found nothing matching the ${noun} request (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 index 78daf62aa..3705462c3 100644 --- a/src/core/publicApi/notFoundSubject.test.ts +++ b/src/core/publicApi/notFoundSubject.test.ts @@ -36,12 +36,50 @@ describe("notFoundSubject", () => { ).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({ + it.each([ + ["trigger.get", { triggerId: "trg-1" }, "trigger", "trg-1"], + ["issue.get", { issueId: "iss-1" }, "issue", "iss-1"], + ["flow.update", { flowId: "flw-1" }, "flow", "flw-1"], + ["agent.get", { sessionId: "ses-1" }, "session", "ses-1"], + [ + "file.requestDownload", + { filePath: "logs/out.txt" }, + "file", + "logs/out.txt", + ], + ] as const)( + "names the record %s resolves", + (contractName, input, noun, id) => { + expect(notFoundSubject(contractName, input)).toEqual({ + id, + kind: "record", + noun, + }); + }, + ); + + // The record is what was not found; the environment it was looked up in is + // beside the point, and blaming it is the bug this family of fixes is about. + it("names the record even when the request also scoped an environment", () => { + expect( + notFoundSubject("trigger.update", { + environmentId: "env-1", + triggerId: "trg-1", + }), + ).toEqual({ id: "trg-1", kind: "record", noun: "trigger" }); + }); + + // An id the route takes as a parameter is not the thing being resolved. + it("ignores the workspace and the ids a route only takes as arguments", () => { + expect(notFoundSubject("tag.list", { workspaceId: "wsp-1" })).toEqual({ kind: "other", }); + expect( + notFoundSubject("trigger.create", { + environmentId: "env-1", + timezoneId: "UTC", + }), + ).toEqual({ kind: "environment" }); }); it("survives an input that is not an object", () => { diff --git a/src/core/publicApi/notFoundSubject.ts b/src/core/publicApi/notFoundSubject.ts index 6457864d9..6f4059c7b 100644 --- a/src/core/publicApi/notFoundSubject.ts +++ b/src/core/publicApi/notFoundSubject.ts @@ -6,6 +6,7 @@ export type NotFoundSubject = | { kind: "runner"; runnerId: string | undefined } | { kind: "run"; runId: string | undefined } | { kind: "environment" } + | { kind: "record"; noun: string; id: string } | { kind: "other" }; // `launch` starts a runner and `list` names none, so neither can 404 over a @@ -24,6 +25,22 @@ const runRoutesThatResolveOneRun: ReadonlySet = new Set([ "run.stop", ]); +/** + * The input fields that name the record a route resolves, and what to call it. + * + * An allowlist rather than every key ending in `Id`, because several routes + * carry an id that is a parameter rather than the subject: `trigger.create` + * takes a `timezoneId`, `run.create` an `aiTaskId`, and almost everything + * carries the `workspaceId` the client injects. + */ +const recordFieldNouns: readonly (readonly [string, string])[] = [ + ["sessionId", "session"], + ["triggerId", "trigger"], + ["issueId", "issue"], + ["flowId", "flow"], + ["filePath", "file"], +]; + function field(input: unknown, name: string): string | undefined { if (typeof input !== "object" || input === null) return undefined; const value = (input as Record)[name]; @@ -48,6 +65,12 @@ export function notFoundSubject( if (runRoutesThatResolveOneRun.has(contractName)) { return { kind: "run", runId: field(input, "runId") }; } + // Before the environment rule: a route that resolves a record can also be + // scoped to an environment, and it is the record that was not found. + for (const [name, noun] of recordFieldNouns) { + const id = field(input, name); + if (id !== undefined) return { id, kind: "record", noun }; + } 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 index 34779b5fa..0999a03f2 100644 --- a/src/shell/platform/callPublicApi.notFound.test.ts +++ b/src/shell/platform/callPublicApi.notFound.test.ts @@ -54,6 +54,16 @@ describe("a public API 404", () => { expect(result.errorBody).toContain("qawolf runner run"); }); + it("reads a trigger lookup as a trigger this team does not hold", async () => { + const result = await client().callPublicApi(publicContractsV1.trigger.get, { + triggerId: "trg-1", + }); + + expect(result.ok).toBe(false); + if (result.ok) return; + expect(result.error).toBe("QA Wolf has no trigger trg-1 (HTTP 404)."); + }); + // 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, { diff --git a/src/shell/platform/describeNotFound.test.ts b/src/shell/platform/describeNotFound.test.ts index fd9d329e5..a877e561e 100644 --- a/src/shell/platform/describeNotFound.test.ts +++ b/src/shell/platform/describeNotFound.test.ts @@ -145,18 +145,33 @@ describe("describeNotFound", () => { }); }); - describe("anything else", () => { - it("names what was asked for without blaming an environment", () => { + describe("a record the platform does not hold", () => { + it("names the record and the id, not the endpoint", () => { const described = describeNotFound( - { kind: "other" }, + { id: "trg-1", kind: "record", noun: "trigger" }, "trigger.get", bareNotFound, ); + expect(described.error).toBe("QA Wolf has no trigger trg-1 (HTTP 404)."); + expect(described.exitCode).toBe(exitCodes.notFound); + expect("errorBody" in described).toBe(false); + }); + }); + + describe("a request with nothing to name", () => { + // "could not find tag.list" read as though the endpoint were gone. + it("says the request matched nothing", () => { + const described = describeNotFound( + { kind: "other" }, + "tag.list", + bareNotFound, + ); + expect(described.error).toBe( - "QA Wolf API could not find trigger.get (HTTP 404).", + "QA Wolf found nothing matching the tag.list request (HTTP 404).", ); - expect("errorBody" in described).toBe(false); + expect(described.error).not.toContain("environment"); }); }); }); diff --git a/src/shell/platform/describeNotFound.ts b/src/shell/platform/describeNotFound.ts index 2dc7c7a0b..7745311af 100644 --- a/src/shell/platform/describeNotFound.ts +++ b/src/shell/platform/describeNotFound.ts @@ -62,6 +62,8 @@ export function describeNotFound( guess: m.runIdMayBeRunnerLocal(subject.runId), statement: m.notFound404Run(subject.runId), }); + case "record": + return say({ statement: m.notFound404Record(subject.noun, subject.id) }); case "other": return say({ statement: m.notFound404(noun) }); // A caller that named no subject is one of the environment-scoped reads: