diff --git a/.changeset/rule-service-failure-messages.md b/.changeset/rule-service-failure-messages.md new file mode 100644 index 00000000..0b16602c --- /dev/null +++ b/.changeset/rule-service-failure-messages.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +Rule service failures now say what went wrong and whether to retry. A network failure names its cause (for example `getaddrinfo ENOTFOUND`) instead of `fetch failed`, and a network failure, `408`, `429`, or `5xx` adds advice to try again. `check` no longer describes a request the service rejected, such as `validation_error`, as the service being unavailable. `rule restore`, `rollback`, and `revisions` report `validation_error` as `INVALID_INPUT` with the service's details, where they previously reported `NETWORK_ERROR`. Every plan refusal now shows the upgrade link, including on `rule create`, `rule improve`, and fetching a generated rule. diff --git a/packages/cli/src/api/refusal.ts b/packages/cli/src/api/refusal.ts index 7512f309..8ad2fe0c 100644 --- a/packages/cli/src/api/refusal.ts +++ b/packages/cli/src/api/refusal.ts @@ -63,3 +63,18 @@ export function parseRefusal(value: unknown): Refusal | undefined { ...(upgradeUrl === undefined ? {} : { upgradeUrl }), }; } + +/** + * The text to print for a refusal: the service's message, followed by the + * upgrade link when there is one. + * + * The service writes the link into `message` itself (measured against + * production, 2026-09-29), so it is added only when absent. Printing it twice + * reads as two different links, and relying on the service to keep writing it + * would drop it silently the day it stops. + */ +export function describeRefusal({ message, upgradeUrl }: Refusal): string { + return upgradeUrl === undefined || message.includes(upgradeUrl) + ? message + : `${message}\n\nUpgrade: ${upgradeUrl}`; +} diff --git a/packages/cli/src/api/v2.ts b/packages/cli/src/api/v2.ts index 7426ec74..877c2b38 100644 --- a/packages/cli/src/api/v2.ts +++ b/packages/cli/src/api/v2.ts @@ -2,7 +2,7 @@ import createClient from "openapi-fetch"; import type { paths } from "../generated/api-v2"; import { getApiBaseUrl } from "./config"; -import { parseRefusal, type Refusal } from "./refusal"; +import { parseRefusal, stripControlCharacters, type Refusal } from "./refusal"; import { isRecord } from "../util/is-record"; import { CLI_VERSION, CLI_VERSION_HEADER } from "../version"; @@ -60,13 +60,27 @@ export type ErrorCode

= Exclude< * same way (log in again) and it is never a verdict about the request. * - `unavailable`: anything the schema does not describe: a network failure, an * undocumented status or code, or a body that is not what it says it is. + * `retryable` marks the ones a later attempt may not repeat (a network + * failure, `408`, `429`, or a `5xx`); a malformed body or an undocumented + * `4xx` will be answered the same way next time. */ export type V2Outcome = | { status: "ok"; data: T } | { status: "refused"; refusal: Refusal } | { status: "error"; code: C; httpStatus: number; details?: string[] } | { status: "unauthorized" } - | { status: "unavailable"; reason: string }; + | { status: "unavailable"; reason: string; retryable: boolean }; + +/** + * The sentence to append to a message about an `unavailable` outcome: advice + * to try again when the failure is one a later attempt may not repeat, and + * nothing otherwise. Retrying a malformed answer gets the same answer. + */ +export function retryAdvice(outcome: { retryable: boolean }): string { + return outcome.retryable + ? " This is usually temporary; try again in a few minutes." + : ""; +} /** * A list of an operation's error codes, checked for completeness at compile @@ -96,6 +110,28 @@ export function createV2Client(token: string) { type Fetched = { data?: unknown; error?: unknown; response: Response }; +/** Whether an undocumented status is one a later attempt may not repeat. */ +function isTransientStatus(status: number): boolean { + return status === 408 || status === 429 || status >= 500; +} + +/** + * Describe a thrown fetch by its cause. + * + * Node's fetch throws `TypeError("fetch failed")` for every transport failure + * and puts what actually happened (DNS, a refused connection, TLS) on `cause`. + * The outer message says nothing a user can act on, so the cause replaces it + * when there is one. A refused connection to a dual-stack host is an + * `AggregateError` with an empty message, which leaves only its `code`. + */ +function describeNetworkError(error: unknown): string { + if (!(error instanceof Error)) return String(error); + const { cause } = error; + if (cause instanceof Error && cause.message !== "") return cause.message; + if (isRecord(cause) && typeof cause.code === "string") return cause.code; + return error.message; +} + /** * Turn an `openapi-fetch` result into an outcome. * @@ -112,22 +148,36 @@ async function settle( try { fetched = await call(); } catch (error) { - const message = error instanceof Error ? error.message : String(error); - return { status: "unavailable", reason: `network error: ${message}` }; + // A body that failed to parse is an answer, not a transport failure, and + // the same server will most likely give it again. + return error instanceof SyntaxError + ? { + status: "unavailable", + reason: "invalid response body", + retryable: false, + } + : { + status: "unavailable", + reason: `network error: ${describeNetworkError(error)}`, + retryable: true, + }; } const { response } = fetched; if (response.status === 401) return { status: "unauthorized" }; if (response.ok) return accept(fetched.data); + // `details` and an undocumented `code` are server text that ends up printed + // to a terminal, so both lose their control characters here, once, rather + // than at every message that interpolates them. const body = fetched.error; const code = isRecord(body) ? body.error : undefined; if (typeof code === "string" && (codes as readonly string[]).includes(code)) { const details = isRecord(body) && Array.isArray(body.details) - ? body.details.filter( - (detail): detail is string => typeof detail === "string" - ) + ? body.details + .filter((detail): detail is string => typeof detail === "string") + .map((detail) => stripControlCharacters(detail)) : undefined; return { status: "error", @@ -140,15 +190,20 @@ async function settle( status: "unavailable", reason: typeof code === "string" - ? `HTTP ${String(response.status)} (${code})` + ? `HTTP ${String(response.status)} (${stripControlCharacters(code)})` : `HTTP ${String(response.status)}`, + retryable: isTransientStatus(response.status), }; } /** Accept any object body as the documented shape. */ function acceptObject(data: unknown): V2Outcome { if (!isRecord(data)) { - return { status: "unavailable", reason: "invalid response body" }; + return { + status: "unavailable", + reason: "invalid response body", + retryable: false, + }; } return { status: "ok", data: data as T }; } @@ -187,6 +242,7 @@ function acceptServed( return { status: "unavailable", reason: "the response carried neither a rule nor a refusal", + retryable: false, }; } const { restoreRules: _marker, ...served } = data; @@ -333,6 +389,7 @@ function acceptRevisions( return { status: "unavailable", reason: "the response was not a revision listing", + retryable: false, }; } return { status: "ok", data: data as unknown as RevisionList }; diff --git a/packages/cli/src/commands/rules.ts b/packages/cli/src/commands/rules.ts index 0adcc14b..b9af4eca 100644 --- a/packages/cli/src/commands/rules.ts +++ b/packages/cli/src/commands/rules.ts @@ -10,7 +10,13 @@ import { resolveIdentity, type Identity, } from "../auth/identity"; -import { iterateRule, submitRequest, type V2Outcome } from "../api/v2"; +import { describeRefusal } from "../api/refusal"; +import { + iterateRule, + retryAdvice, + submitRequest, + type V2Outcome, +} from "../api/v2"; import { readRuleMetaFile, deleteRuleFiles } from "../rules/files"; import { awaitRequest, @@ -58,12 +64,15 @@ function describeSubmitFailure( } case "unavailable": { return { - message: `Request submission failed: ${outcome.reason}.`, + message: `Request submission failed: ${outcome.reason}.${retryAdvice(outcome)}`, code: "NETWORK_ERROR", }; } case "refused": { - return { message: outcome.refusal.message, code: "NETWORK_ERROR" }; + return { + message: describeRefusal(outcome.refusal), + code: "NETWORK_ERROR", + }; } case "error": { switch (outcome.code) { diff --git a/packages/cli/src/rules/generate.ts b/packages/cli/src/rules/generate.ts index 4a1caa3e..7a97c3cb 100644 --- a/packages/cli/src/rules/generate.ts +++ b/packages/cli/src/rules/generate.ts @@ -1,11 +1,12 @@ import { fetchRule, getRequestStatus, + retryAdvice, type RequestStatus, type ServedRule, } from "../api/v2"; import { notRunOnPlanSentence, parseEntitlementV2 } from "../api/entitlement"; -import { stripControlCharacters } from "../api/refusal"; +import { describeRefusal, stripControlCharacters } from "../api/refusal"; import { CLIError } from "../util/cli-error"; import { getCliPrefix } from "../util/package-manager"; import { writeServedRule } from "./files"; @@ -99,10 +100,15 @@ export async function awaitRequest( "NETWORK_ERROR" ); } - case "refused": + case "refused": { + throw new CLIError( + "Polling failed: unexpected refusal.", + "NETWORK_ERROR" + ); + } case "unavailable": { throw new CLIError( - `Polling failed: ${outcome.status === "unavailable" ? outcome.reason : "unexpected refusal"}.`, + `Polling failed: ${outcome.reason}.${retryAdvice(outcome)}`, "NETWORK_ERROR" ); } @@ -227,7 +233,7 @@ function servedOrThrow( // a revision the CLI did not ask for. Relay its message; it is the only // explanation available. throw new CLIError( - `Rule ${ruleId} was generated but could not be fetched: ${outcome.refusal.message}`, + `Rule ${ruleId} was generated but could not be fetched: ${describeRefusal(outcome.refusal)}`, "RULE_GENERATION_FAILED" ); } @@ -245,7 +251,7 @@ function servedOrThrow( } case "unavailable": { throw new CLIError( - `Rule ${ruleId} could not be fetched: ${outcome.reason}.`, + `Rule ${ruleId} could not be fetched: ${outcome.reason}.${retryAdvice(outcome)}`, "NETWORK_ERROR" ); } diff --git a/packages/cli/src/rules/plan-check.ts b/packages/cli/src/rules/plan-check.ts index e96988b8..e77a557a 100644 --- a/packages/cli/src/rules/plan-check.ts +++ b/packages/cli/src/rules/plan-check.ts @@ -1,6 +1,6 @@ import { getToken } from "../auth/token"; import { resolveActingOrg } from "../auth/org"; -import { reconcileRules } from "../api/v2"; +import { reconcileRules, retryAdvice } from "../api/v2"; import { resolveRepositoryUrl } from "../util/git-remote"; import { getCliPrefix } from "../util/package-manager"; import { recoveryAdvice } from "./recovery-advice"; @@ -212,25 +212,15 @@ export async function planCheck( }` ); if (outcome.status !== "ok") { - const cause = - outcome.status === "unauthorized" - ? `authentication was rejected — run \`${getCliPrefix()} auth login\` to re-authenticate` - : outcome.status === "error" && - outcome.code === "organization_not_found" - ? "the Taskless GitHub App installation does not cover this repository, or your login lost access to the organization" - : `the rule service was unavailable (${ - outcome.status === "unavailable" - ? outcome.reason - : outcome.status === "error" - ? outcome.code - : "unexpected refusal" - })`; + const cause = reconcileFailureCause(outcome); const remaining = await discoverRuntimeRulesIn(runtimeRoot); return { ...empty, skipped: remaining.map((rule) => ({ rule: rule.name, reason: cause })), notices: [ - `Rule verification could not be performed: ${cause}. Static rules ran unverified and runtime rules did not run.`, + `Rule verification could not be performed: ${cause}. Static rules ran unverified and runtime rules did not run.${ + outcome.status === "unavailable" ? retryAdvice(outcome) : "" + }`, ], failures, integrity, @@ -341,3 +331,35 @@ function withheldNotice(entitlement: PlanEntitlement): string { : `. Upgrade at ${entitlement.upgradeUrl}`) ); } + +/** + * Why reconcile gave no verdicts, as a clause for the notice and each skipped + * rule. + * + * "Unavailable" is kept for the service not answering. A documented code is an + * answer, and calling it an outage sends the user to retry something that + * will be rejected identically every time. + */ +function reconcileFailureCause( + outcome: Exclude>, { status: "ok" }> +): string { + switch (outcome.status) { + case "unauthorized": { + return `authentication was rejected — run \`${getCliPrefix()} auth login\` to re-authenticate`; + } + case "unavailable": { + return `the rule service was unavailable (${outcome.reason})`; + } + case "refused": { + return "the rule service answered with an unexpected refusal"; + } + case "error": { + if (outcome.code === "organization_not_found") { + return "the Taskless GitHub App installation does not cover this repository, or your login lost access to the organization"; + } + return `the rule service rejected the verification request (${outcome.code}${ + outcome.details?.length ? `: ${outcome.details.join(", ")}` : "" + })`; + } + } +} diff --git a/packages/cli/src/rules/recover.ts b/packages/cli/src/rules/recover.ts index 5da6b193..b6d23445 100644 --- a/packages/cli/src/rules/recover.ts +++ b/packages/cli/src/rules/recover.ts @@ -2,12 +2,14 @@ import { listRevisions, reconcileRules, restoreRule, + retryAdvice, rollbackRule, type RevisionList, type ServedRule, type V2Outcome, } from "../api/v2"; import { notRunOnPlanSentence, parseEntitlementV2 } from "../api/entitlement"; +import { describeRefusal } from "../api/refusal"; import type { Identity } from "../auth/identity"; import { CLIError } from "../util/cli-error"; import { isRecord } from "../util/is-record"; @@ -73,14 +75,8 @@ function failure( ): CLIError { switch (outcome.status) { case "refused": { - const { message, upgradeUrl } = outcome.refusal; - // The service writes the upgrade link into `message` itself (measured - // against production, 2026-09-29), so it is added only when absent. - // Printing it twice reads as two different links. return new CLIError( - upgradeUrl === undefined || message.includes(upgradeUrl) - ? message - : `${message}\n\nUpgrade: ${upgradeUrl}`, + describeRefusal(outcome.refusal), "RULE_RECOVERY_NOT_IN_PLAN" ); } @@ -92,7 +88,7 @@ function failure( } case "unavailable": { return new CLIError( - `The rule service was unavailable (${outcome.reason}).`, + `The rule service was unavailable (${outcome.reason}).${retryAdvice(outcome)}`, "NETWORK_ERROR" ); } @@ -120,9 +116,17 @@ function failure( "NETWORK_ERROR" ); } + case "validation_error": { + return new CLIError( + `The rule service rejected the request as invalid: ${(outcome.details ?? []).join(", ") || "no details were given"}.`, + "INVALID_INPUT" + ); + } default: { return new CLIError( - `The rule service refused the request (${outcome.code}).`, + `The rule service refused the request (${outcome.code}${ + outcome.details?.length ? `: ${outcome.details.join(", ")}` : "" + }).`, "NETWORK_ERROR" ); } diff --git a/packages/cli/test/api-v2.test.ts b/packages/cli/test/api-v2.test.ts index 5aecf59b..8abe8ed7 100644 --- a/packages/cli/test/api-v2.test.ts +++ b/packages/cli/test/api-v2.test.ts @@ -196,6 +196,7 @@ describe("v2 client", () => { expect(outcome).toEqual({ status: "unavailable", reason: "the response was not a revision listing", + retryable: false, }); }); }); @@ -218,6 +219,21 @@ describe("v2 client", () => { }); }); + it("strips control characters from server-authored details and codes", async () => { + respond(400, { + error: "validation_error", + details: ["prompt: required\u001B[2J", 42], + }); + expect( + await submitRequest("tok", { repositoryUrl: REPO, prompt: "" }) + ).toMatchObject({ details: ["prompt: required[2J"] }); + + respond(404, { error: "not_documented\u001B]0;title\u0007" }); + expect( + await getRequestStatus("tok", "req-1", { repositoryUrl: REPO }) + ).toMatchObject({ reason: "HTTP 404 (not_documented]0;title)" }); + }); + it("maps rule_not_found on iterate, so a caller can report RULE_NOT_FOUND", async () => { respond(404, { error: "rule_not_found" }); const outcome = await iterateRule("tok", "gone-00000000", { @@ -255,6 +271,7 @@ describe("v2 client", () => { expect(outcome).toEqual({ status: "unavailable", reason: "HTTP 404 (rule_not_found)", + retryable: false, }); }); @@ -264,7 +281,39 @@ describe("v2 client", () => { repositoryUrl: REPO, rules: [], }); - expect(outcome).toEqual({ status: "unavailable", reason: "HTTP 503" }); + expect(outcome).toEqual({ + status: "unavailable", + reason: "HTTP 503", + retryable: true, + }); + }); + + it.each([408, 429, 500, 502])( + "marks an undocumented %i as retryable", + async (status) => { + respond(status, "slow down"); + const outcome = await reconcileRules("tok", { + repositoryUrl: REPO, + rules: [], + }); + expect(outcome).toMatchObject({ + status: "unavailable", + retryable: true, + }); + } + ); + + it("does not mark an undocumented 403 as retryable", async () => { + respond(403, "forbidden"); + const outcome = await reconcileRules("tok", { + repositoryUrl: REPO, + rules: [], + }); + expect(outcome).toEqual({ + status: "unavailable", + reason: "HTTP 403", + retryable: false, + }); }); it("reads a network failure as unavailable, never a throw", async () => { @@ -276,6 +325,41 @@ describe("v2 client", () => { expect(outcome).toEqual({ status: "unavailable", reason: "network error: fetch failed", + retryable: true, + }); + }); + + it("names a network failure by its cause, which is where Node's fetch puts it", async () => { + const cause = Object.assign( + new Error("getaddrinfo ENOTFOUND example.invalid"), + { code: "ENOTFOUND" } + ); + fetchMock.mockRejectedValue(new TypeError("fetch failed", { cause })); + const outcome = await reconcileRules("tok", { + repositoryUrl: REPO, + rules: [], + }); + expect(outcome).toEqual({ + status: "unavailable", + reason: "network error: getaddrinfo ENOTFOUND example.invalid", + retryable: true, + }); + }); + + it("falls back to the cause's code when its message is empty", async () => { + // A refused connection to a dual-stack host: one error per address, + // gathered into an AggregateError whose own message is empty. + // eslint-disable-next-line unicorn/error-message -- the empty message is the case under test + const cause = Object.assign(new AggregateError([], ""), { + code: "ECONNREFUSED", + }); + fetchMock.mockRejectedValue(new TypeError("fetch failed", { cause })); + const outcome = await reconcileRules("tok", { + repositoryUrl: REPO, + rules: [], + }); + expect(outcome).toMatchObject({ + reason: "network error: ECONNREFUSED", }); }); @@ -285,7 +369,11 @@ describe("v2 client", () => { repositoryUrl: REPO, rules: [], }); - expect(outcome.status).toBe("unavailable"); + expect(outcome).toEqual({ + status: "unavailable", + reason: "invalid response body", + retryable: false, + }); }); }); }); diff --git a/packages/cli/test/refusal.test.ts b/packages/cli/test/refusal.test.ts index 4ea29bd1..cf560e9c 100644 --- a/packages/cli/test/refusal.test.ts +++ b/packages/cli/test/refusal.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from "vitest"; -import { parseRefusal, stripControlCharacters } from "../src/api/refusal"; +import { + describeRefusal, + parseRefusal, + stripControlCharacters, +} from "../src/api/refusal"; const UPGRADE = "https://app.taskless.io/org/1/upgrade?from=restore"; @@ -87,3 +91,26 @@ describe("stripControlCharacters", () => { expect(stripControlCharacters("restaurée — ✓")).toBe("restaurée — ✓"); }); }); + +describe("describeRefusal", () => { + const reason = "RESTORE_RULES_NOT_IN_PLAN"; + + it("appends the upgrade link when the message does not carry it", () => { + expect( + describeRefusal({ reason, message: "Not in plan.", upgradeUrl: UPGRADE }) + ).toBe(`Not in plan.\n\nUpgrade: ${UPGRADE}`); + }); + + it("does not repeat a link the message already carries", () => { + const message = `Not in plan. See ${UPGRADE}`; + expect(describeRefusal({ reason, message, upgradeUrl: UPGRADE })).toBe( + message + ); + }); + + it("is the message alone when there is no link", () => { + expect(describeRefusal({ reason, message: "Not in plan." })).toBe( + "Not in plan." + ); + }); +}); diff --git a/packages/cli/test/rule-recovery.test.ts b/packages/cli/test/rule-recovery.test.ts index 491c7a94..337641ff 100644 --- a/packages/cli/test/rule-recovery.test.ts +++ b/packages/cli/test/rule-recovery.test.ts @@ -555,6 +555,26 @@ describe("rule restore / rule rollback", () => { expect(String(output.message)).toContain(`rule revisions ${RULE_ID}`); }); + it("rollback reports a transient outage as NETWORK_ERROR and says to try again", async () => { + stub({ verdict: { kind: "run" }, served: {}, status: 503 }); + const output = await run(["rollback", RULE_ID, "r1"]); + expect(output).toMatchObject({ ok: false, code: "NETWORK_ERROR" }); + expect(String(output.message)).toContain("(HTTP 503)"); + expect(String(output.message)).toContain("try again"); + }); + + it("rollback reports validation_error as INVALID_INPUT with the service's details", async () => { + stub({ + verdict: { kind: "run" }, + served: { error: "validation_error", details: ["revisionId: invalid"] }, + status: 400, + }); + const output = await run(["rollback", RULE_ID, "r1"]); + expect(output).toMatchObject({ ok: false, code: "INVALID_INPUT" }); + expect(String(output.message)).toContain("revisionId: invalid"); + expect(String(output.message)).not.toMatch(/unavailable|try again/); + }); + it("rollback relays a plan refusal", async () => { stub({ verdict: { kind: "run" }, served: REFUSAL }); const output = await run(["rollback", RULE_ID, "r1"]); diff --git a/packages/cli/test/runtime-check.test.ts b/packages/cli/test/runtime-check.test.ts index 524b3fa4..19b14211 100644 --- a/packages/cli/test/runtime-check.test.ts +++ b/packages/cli/test/runtime-check.test.ts @@ -678,6 +678,20 @@ describe("check: static vs runtime dispatch", () => { expect(output.notices?.join("\n")).toMatch( /verification could not be performed/ ); + expect(output.notices?.join("\n")).toMatch(/try again/); + }); + + it("reconcile validation_error: reported as a rejection, never as an outage", async () => { + const { stdout, exitCode } = await authedCheck(() => ({ + statusCode: 400, + body: { error: "validation_error", details: ["rules: too many"] }, + })); + const output = parseJson(stdout); + expect(exitCode).toBe(0); + const notices = output.notices?.join("\n") ?? ""; + expect(notices).toMatch(/rejected the verification request/); + expect(notices).toContain("rules: too many"); + expect(notices).not.toMatch(/unavailable|try again/); }); it("--anonymous with a token: skips runtime and never calls reconcile", async () => {