diff --git a/.changeset/rule-poll-transient-failures.md b/.changeset/rule-poll-transient-failures.md new file mode 100644 index 00000000..20f94d75 --- /dev/null +++ b/.changeset/rule-poll-transient-failures.md @@ -0,0 +1,7 @@ +--- +"@taskless/cli": patch +--- + +`rule create` and `rule improve` no longer give up on the first transient failure while waiting for a generation request or fetching the rules it produced. A network failure, `408`, `429`, or `5xx` is retried on the next 15-second poll, and the command fails only after 8 in a row. A rejected token, a documented error, or a response the CLI cannot read still fails immediately. + +Both commands now take `--resume ` to pick up a request a previous run submitted, instead of submitting a new one. When a command gives up, its message names the request id, says the request may still complete on the service, and gives the `--resume` command to run. diff --git a/openspec/specs/cli-rules/spec.md b/openspec/specs/cli-rules/spec.md index 4d46d731..807bb136 100644 --- a/openspec/specs/cli-rules/spec.md +++ b/openspec/specs/cli-rules/spec.md @@ -155,6 +155,86 @@ capability requirements. - **WHEN** `taskless rule create` is waiting on the API - **THEN** the CLI SHALL report progress rather than appearing to hang +### Requirement: Rule generation tolerates transient service failures + +While `taskless rule create` or `taskless rule improve` polls a submitted request, and while it +fetches the rules that request produced, an `unavailable` outcome marked `retryable` (a network +failure, `408`, `429`, or a `5xx`) SHALL NOT end the command. The CLI SHALL report the failure as +progress and try again after the poll interval, and SHALL give up only after 8 such outcomes in a +row. Any other answer SHALL reset that count. A non-retryable `unavailable` (an undocumented `4xx` +or a malformed body), a documented `error`, a refusal, or a `401` SHALL still fail on the first +occurrence. When the CLI gives up, the message SHALL name the request id, SHALL say the request +was not cancelled and may still complete, and SHALL name the `--resume` command that picks it up +again. All rules SHALL still be fetched before any is written, so a fetch that gives up writes +nothing. + +#### Scenario: A transient run is ridden out + +- **WHEN** polling answers `503`, `429`, and a network failure before reaching `generated` +- **THEN** the CLI SHALL keep polling and deliver the rules +- **AND** SHALL have submitted exactly one request + +#### Scenario: Persistent failure gives up without implying a resubmit + +- **WHEN** polling answers a retryable failure 8 times in a row +- **THEN** the CLI SHALL fail with `NETWORK_ERROR` +- **AND** the message SHALL name the request id, say it may still complete, and name + `taskless rule --resume ` + +#### Scenario: An answer in between resets the count + +- **WHEN** polling answers 7 retryable failures, then `building`, then 7 more, then `generated` +- **THEN** the CLI SHALL deliver the rules + +#### Scenario: A non-retryable failure fails at once + +- **WHEN** polling answers an undocumented `4xx`, or `401` +- **THEN** the CLI SHALL fail on that answer without polling again + +#### Scenario: Fetching a generated rule is retried the same way + +- **WHEN** `GET /cli/api/v2/rule/{ruleId}` answers a retryable failure 8 times in a row +- **THEN** the CLI SHALL fail with `NETWORK_ERROR`, write no rules, and name the `--resume` command + +### Requirement: Rules create and improve resume a submitted request + +`taskless rule create` and `taskless rule improve` SHALL accept `--resume `, the request +id a previous run of the same command printed. With it, the CLI SHALL NOT submit anything: it SHALL +poll that request and fetch, verify, and write what it produced exactly as it would have after +submitting it, and SHALL report the same output, with `requestId` set to the resumed id. +`--resume` SHALL NOT be combined with `--from`, and its value SHALL be a UUID; either mistake SHALL +fail with `INVALID_INPUT` before any service call. +A `requestId` the service returns from submit or iterate SHALL also be a UUID, since the CLI prints +it inside a `--resume` command an agent is told to run; any other value SHALL be treated as an +invalid response body and SHALL NOT be printed. + +#### Scenario: A resumed request is not resubmitted + +- **WHEN** a user runs `taskless rule create --resume ` after a run that gave up on it +- **THEN** the CLI SHALL poll `GET /cli/api/v2/request/{requestId}` and write the produced rules +- **AND** SHALL NOT call `POST /cli/api/v2/request` + +#### Scenario: Improve resumes without iterating again + +- **WHEN** a user runs `taskless rule improve --resume ` +- **THEN** the CLI SHALL NOT call `POST /cli/api/v2/rule/{ruleId}/iterate` + +#### Scenario: --resume with --from is refused + +- **WHEN** a user runs `taskless rule create --resume --from req.json` +- **THEN** the CLI SHALL fail with `INVALID_INPUT` without calling the service + +#### Scenario: A server-authored request id is never put on a command line + +- **WHEN** submit answers `200` with a `requestId` that is not a UUID, such as `x; rm -rf ~` +- **THEN** the CLI SHALL fail with `NETWORK_ERROR` as an invalid response body +- **AND** SHALL NOT print that value or poll it + +#### Scenario: A rule id is not a request id + +- **WHEN** a user runs `taskless rule improve --resume no-eval-3fa9c21b` +- **THEN** the CLI SHALL fail with `INVALID_INPUT` without calling the service + ### Requirement: Rules improve reads request from file `taskless rule improve` SHALL accept a `--from ` flag specifying a JSON file containing the diff --git a/packages/cli/src/agent/create-remote-rule.md b/packages/cli/src/agent/create-remote-rule.md index 18518ccf..e4aee6f5 100644 --- a/packages/cli/src/agent/create-remote-rule.md +++ b/packages/cli/src/agent/create-remote-rule.md @@ -1,4 +1,4 @@ -# Topic: create-remote-rule (CLI v%(CLI_VERSION)s / topic v4) +# Topic: create-remote-rule (CLI v%(CLI_VERSION)s / topic v5) ## You are here This is `create-remote-rule`. It helps you have the Taskless service @@ -137,8 +137,9 @@ Two ways to legitimately be here: the id is the rule's directory name (for example `no-eval-3fa9c21b`). It is what `rule improve`, `rule restore`, and `rule rollback` take, and it is on disk, so nothing needs recording. - `requestId` names the generation request only; no command takes it - back, and passing it to `rule improve` fails with `RULE_NOT_FOUND`. + `requestId` names the generation request only. The one command that + takes it back is `rule create --resume ` (see Errors); + passing it to `rule improve` as a rule id fails with `RULE_NOT_FOUND`. **Read `notices` if it is present.** It is an optional array of advisory messages about a delivery that was written anyway. A rule @@ -181,7 +182,7 @@ With `--json`, failures emit `{ ok: false, code, message }`: | `NO_ORIGIN_REMOTE` | git repository, no `origin` | route to a local recipe; `auth login` cannot fix it | | `UNSUPPORTED_REMOTE_HOST`| `origin` is not GitHub | route to a local recipe; `auth login` cannot fix it | | `INVALID_INPUT` | `--from` JSON failed validation | re-read the input schema, fix, retry | -| `NETWORK_ERROR` | submit/poll failed | report and suggest retry | +| `NETWORK_ERROR` | submit/poll failed | the CLI already retried transient failures. If the message names a `--resume` command, run that to keep waiting on the same request; re-running with `--from` submits a NEW request, so confirm with the user first | | `RULE_GENERATION_FAILED` | the service failed to generate | report the message; suggest enriching prompt | | `RULE_UNSUPPORTED` | plan lacks this generation type | tell the user to enable it; do not retry | diff --git a/packages/cli/src/agent/improve-rule.md b/packages/cli/src/agent/improve-rule.md index ea74a225..13af89aa 100644 --- a/packages/cli/src/agent/improve-rule.md +++ b/packages/cli/src/agent/improve-rule.md @@ -1,4 +1,4 @@ -# Topic: improve-rule (CLI v%(CLI_VERSION)s / topic v6) +# Topic: improve-rule (CLI v%(CLI_VERSION)s / topic v7) ## Goal Iterate on an existing Taskless rule. The CLI submits the user's @@ -128,7 +128,7 @@ When `--json` is set, failures emit `{ ok: false, code, message }`: | `UNSUPPORTED_REMOTE_HOST`| `origin` is not GitHub | tell the user; `auth login` cannot fix it | | `INVALID_INPUT` | `--from` JSON failed validation | re-read input schema, fix, retry | | `RULE_NOT_FOUND` | the service did not issue this rule | re-check the directory name; a local or pre-0.12.0 rule needs the anonymous flow | -| `NETWORK_ERROR` | API submit/poll failed | report and suggest retry | +| `NETWORK_ERROR` | API submit/poll failed | the CLI already retried transient failures. If the message names a `--resume` command, run that to keep waiting on the same request; re-running with `--from` submits a NEW request, so confirm with the user first | | `RULE_GENERATION_FAILED` | API returned a generation failure | report; suggest enriching guidance/references | | `RULE_UNSUPPORTED` | plan lacks this generation type | tell the user to enable it; do not retry | diff --git a/packages/cli/src/api/v2.ts b/packages/cli/src/api/v2.ts index 877c2b38..17d35628 100644 --- a/packages/cli/src/api/v2.ts +++ b/packages/cli/src/api/v2.ts @@ -1,4 +1,5 @@ import createClient from "openapi-fetch"; +import { z } from "zod"; import type { paths } from "../generated/api-v2"; import { getApiBaseUrl } from "./config"; @@ -431,6 +432,33 @@ export type RequestBody = NonNullable< export type RequestAccepted = OkBody<"/cli/api/v2/request", "post">; +/** + * Whether `value` has the shape of a request id, which the service documents + * as a UUID. `guid`, not `uuid`: the shape is what matters, not the RFC + * version bits. + * + * A request id is printed inside a `--resume` command that an agent is told + * to run, so one that is anything else is refused rather than passed on: it + * would put server-authored text on a command line. + */ +export function isRequestId(value: unknown): value is string { + return z.guid().safeParse(value).success; +} + +/** Accept a submit or iterate body only when it names a well-formed request. */ +function acceptRequest( + data: unknown +): V2Outcome { + if (!isRecord(data) || !isRequestId(data.requestId)) { + return { + status: "unavailable", + reason: "invalid response body", + retryable: false, + }; + } + return { status: "ok", data: data as RequestAccepted }; +} + export type RequestCode = ErrorCode<"/cli/api/v2/request", "post">; const REQUEST_CODES = errorCodes>()([ @@ -448,7 +476,7 @@ export function submitRequest( return settle( () => client.POST("/cli/api/v2/request", { body }), REQUEST_CODES, - acceptObject + acceptRequest ); } @@ -520,7 +548,7 @@ export function iterateRule( body, }), ITERATE_CODES, - acceptObject + acceptRequest ); } diff --git a/packages/cli/src/commands/rules.ts b/packages/cli/src/commands/rules.ts index 7cd8cdd7..5d755bb9 100644 --- a/packages/cli/src/commands/rules.ts +++ b/packages/cli/src/commands/rules.ts @@ -12,6 +12,7 @@ import { } from "../auth/identity"; import { describeRefusal } from "../api/refusal"; import { + isRequestId, iterateRule, retryAdvice, submitRequest, @@ -115,8 +116,56 @@ function describeSubmitFailure( } } +/** + * The request id `--resume` names, or `undefined` when the flag is absent. + * + * A resumed request was already submitted, so there is nothing for `--from` to + * describe: passing both is a mistake about which request is meant, and is + * refused rather than guessed at. + */ +function resumeRequestId( + args: { resume?: string; from?: string }, + subcommand: "create" | "improve", + fail: (message: string, code?: CLIErrorCode) => never +): string | undefined { + if (args.resume === undefined) return undefined; + if (args.from !== undefined) { + fail( + `--resume picks up a request that was already submitted, so it takes no --from file.\n Example: ${getCliPrefix()} rule ${subcommand} --resume `, + "INVALID_INPUT" + ); + } + const requestId = args.resume.trim(); + if (!isRequestId(requestId)) { + fail( + `--resume takes the request id a previous \`rule ${subcommand}\` printed, a UUID. Got "${args.resume}".`, + "INVALID_INPUT" + ); + } + return requestId; +} + +/** + * Resolve identity (orgId from JWT, repositoryUrl from git remote), or fail. + * `resolveIdentity` throws a CLIError carrying its own code. Read the field; + * never re-derive the code from the message. + */ +async function identityOrFail( + cwd: string, + fail: (message: string, code?: CLIErrorCode) => never +): Promise { + try { + return await resolveIdentity(cwd); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + fail(message, identityFailureCode(error)); + } +} + /** How {@link completeRequest} reports, per command. */ interface CompletionOptions { + /** Which command this is, for the `--resume` line a give-up names. */ + subcommand: "create" | "improve"; json: boolean; fail: (message: string, code?: CLIErrorCode) => never; /** "Generated" or "Updated", for human output. */ @@ -145,6 +194,7 @@ async function completeRequest( repositoryUrl: identity.repositoryUrl, orgId: identity.orgSubject, onProgress: (message: string) => console.error(message), + resumeCommand: `${getCliPrefix()} rule ${options.subcommand} --resume ${requestId}`, }; let delivered: Delivered; @@ -179,7 +229,12 @@ async function completeRequest( return 0; } } - delivered = await deliverRevisions(cwd, context, status.revisions); + delivered = await deliverRevisions( + cwd, + context, + requestId, + status.revisions + ); } catch (error) { if (error instanceof CLIError && error.reported) throw error; options.fail( @@ -228,7 +283,12 @@ const createCommand = defineCommand({ from: { type: "string", description: - "Path to a JSON file containing the rule request (required). Example: --from .taskless/.tmp-rule-request.json", + "Path to a JSON file containing the rule request (required unless --resume). Example: --from .taskless/.tmp-rule-request.json", + }, + resume: { + type: "string", + description: + "Keep waiting on a request a previous run submitted, by the request id it printed, instead of submitting a new one", }, anonymous: { type: "boolean", @@ -274,7 +334,32 @@ const createCommand = defineCommand({ // Set to the number of rules written when generation succeeds; drives the // cli_rule_created event in the finally. let createdRuleCount: number | undefined; + /** Poll, then fetch, verify, and write each produced rule. */ + async function complete( + identity: Identity, + requestId: string + ): Promise { + const written = await completeRequest(cwd, identity, requestId, { + subcommand: "create", + json: args.json, + fail, + verb: "Generated", + failedPrefix: "Rule generation failed", + output: (result) => + createOutputSchema.parse({ success: true, requestId, ...result }), + }); + if (written > 0) createdRuleCount = written; + } + try { + const resumed = resumeRequestId(args, "create", fail); + if (resumed !== undefined) { + const identity = await identityOrFail(cwd, fail); + console.error(`Resuming request ${resumed}. Waiting for generation...`); + await complete(identity, resumed); + return; + } + // 1. Read and validate --from file if (!args.from) { fail( @@ -314,16 +399,8 @@ const createCommand = defineCommand({ ); } - // 2. Resolve identity (orgId from JWT, repositoryUrl from git remote) - let identity; - try { - identity = await resolveIdentity(cwd); - } catch (error) { - // resolveIdentity throws a CLIError carrying its own code. Read the - // field; never re-derive the code from the message. - const message = error instanceof Error ? error.message : String(error); - fail(message, identityFailureCode(error)); - } + // 2. Resolve identity + const identity = await identityOrFail(cwd, fail); // 3. Submit the request const submitted = await submitRequest(identity.token, { @@ -341,15 +418,7 @@ const createCommand = defineCommand({ // 4. Poll, then fetch, verify, and write each produced rule console.error(`Rule requested (${requestId}). Waiting for generation...`); - const written = await completeRequest(cwd, identity, requestId, { - json: args.json, - fail, - verb: "Generated", - failedPrefix: "Rule generation failed", - output: (result) => - createOutputSchema.parse({ success: true, requestId, ...result }), - }); - if (written > 0) createdRuleCount = written; + await complete(identity, requestId); } finally { // Concrete state event: a rule was actually generated and written. if (createdRuleCount !== undefined) { @@ -379,7 +448,12 @@ const improveCommand = defineCommand({ from: { type: "string", description: - "Path to a JSON file containing { ruleId, guidance, references? }. Example: --from .taskless/.tmp-iterate-request.json", + "Path to a JSON file containing { ruleId, guidance, references? } (required unless --resume). Example: --from .taskless/.tmp-iterate-request.json", + }, + resume: { + type: "string", + description: + "Keep waiting on a request a previous run submitted, by the request id it printed, instead of submitting a new one", }, anonymous: { type: "boolean", @@ -420,7 +494,33 @@ const improveCommand = defineCommand({ // Set to the number of rules written when iteration succeeds; drives the // cli_rule_improved event in the finally. let improvedRuleCount: number | undefined; + + /** Poll, then fetch, verify, and write the new revision. */ + async function complete( + identity: Identity, + requestId: string + ): Promise { + const written = await completeRequest(cwd, identity, requestId, { + subcommand: "improve", + json: args.json, + fail, + verb: "Updated", + failedPrefix: "Rule iteration failed", + output: (result) => + improveOutputSchema.parse({ success: true, requestId, ...result }), + }); + if (written > 0) improvedRuleCount = written; + } + try { + const resumed = resumeRequestId(args, "improve", fail); + if (resumed !== undefined) { + const identity = await identityOrFail(cwd, fail); + console.error(`Resuming request ${resumed}. Waiting for generation...`); + await complete(identity, resumed); + return; + } + // 1. Read and validate --from file if (!args.from) { fail( @@ -460,15 +560,8 @@ const improveCommand = defineCommand({ ); } - // 2. Resolve identity (orgId from JWT, repositoryUrl from git remote) - let identity; - try { - identity = await resolveIdentity(cwd); - } catch (error) { - // Same contract as `rule create`: the code travels on the error. - const message = error instanceof Error ? error.message : String(error); - fail(message, identityFailureCode(error)); - } + // 2. Resolve identity + const identity = await identityOrFail(cwd, fail); // 3. Submit the iterate request, addressed by the rule's own id const submitted = await iterateRule(identity.token, request.ruleId, { @@ -489,15 +582,7 @@ const improveCommand = defineCommand({ console.error( `Iterate request submitted (${requestId}). Waiting for generation...` ); - const written = await completeRequest(cwd, identity, requestId, { - json: args.json, - fail, - verb: "Updated", - failedPrefix: "Rule iteration failed", - output: (result) => - improveOutputSchema.parse({ success: true, requestId, ...result }), - }); - if (written > 0) improvedRuleCount = written; + await complete(identity, requestId); } finally { // Concrete state event: a rule was actually iterated and rewritten. if (improvedRuleCount !== undefined) { diff --git a/packages/cli/src/rules/generate.ts b/packages/cli/src/rules/generate.ts index e892b9b5..b6375a89 100644 --- a/packages/cli/src/rules/generate.ts +++ b/packages/cli/src/rules/generate.ts @@ -1,7 +1,6 @@ import { fetchRule, getRequestStatus, - retryAdvice, type RequestStatus, type ServedRule, } from "../api/v2"; @@ -33,10 +32,43 @@ export interface GenerationContext { orgId?: string | number; /** Progress lines for a human; never part of `--json` output. */ onProgress: (message: string) => void; + /** + * The command that picks this request up again without submitting a new + * one, named whenever the CLI stops waiting on a request that may still + * finish. + */ + resumeCommand: string; } const POLL_INTERVAL_MS = 15_000; +/** + * How many retryable `unavailable` outcomes in a row a poll or a rule fetch + * absorbs before giving up: about two minutes at `POLL_INTERVAL_MS`, enough to + * ride out a deploy or a rate limit without hammering a service that is + * telling the CLI to back off. Any answer that is not `unavailable` resets the + * count, so it measures one outage, not the request's whole life. + */ +const MAX_CONSECUTIVE_UNAVAILABLE = 8; + +/** Wait between attempts. Tests collapse it by stubbing `setTimeout`. */ +async function pause(): Promise { + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); +} + +/** + * The sentences for a request the CLI stopped watching. Giving up on a poll + * cancels nothing on the service, so the request may still finish, and + * running the command again without `--resume` is a SECOND request rather + * than a retry of this one. + */ +function abandonedRequestAdvice( + context: GenerationContext, + requestId: string +): string { + return ` Request ${requestId} was not cancelled and may still complete on the service. Run \`${context.resumeCommand}\` to keep waiting for it; running the command without \`--resume\` submits a new request.`; +} + /** * The service returns the same 404 `organization_not_found` whether the org * isn't yours or its GitHub App installation doesn't cover this repository (it @@ -72,22 +104,30 @@ export type FinishedRequest = RequestStatus; /** * Poll a request until it stops moving. * + * A retryable `unavailable` (a network failure, `408`, `429`, or a `5xx`) is + * not an answer about the request either, but it is one a later poll may not + * repeat, so polling carries on through up to `MAX_CONSECUTIVE_UNAVAILABLE` of + * them in a row. + * * Throws a `CLIError` for anything that is not an answer about the request: * `request_not_found` (the id will never resolve, reported as `NETWORK_ERROR` - * because the remedy is to resubmit), a rejected token, and an unreachable - * service, each with the code a caller branches on. + * because the remedy is to resubmit), a rejected token, and a service that + * stayed unreachable or answered with something unreadable, each with the + * code a caller branches on. */ export async function awaitRequest( context: GenerationContext, requestId: string ): Promise { + let unavailable = 0; while (true) { - await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); + await pause(); const outcome = await getRequestStatus(context.token, requestId, { repositoryUrl: context.repositoryUrl, ...(context.orgId === undefined ? {} : { orgId: context.orgId }), }); + if (outcome.status !== "unavailable") unavailable = 0; switch (outcome.status) { case "ok": { break; @@ -118,8 +158,15 @@ export async function awaitRequest( ); } case "unavailable": { + unavailable += 1; + if (outcome.retryable && unavailable < MAX_CONSECUTIVE_UNAVAILABLE) { + context.onProgress( + `Status check failed (${outcome.reason}); retrying (${String(unavailable)}/${String(MAX_CONSECUTIVE_UNAVAILABLE - 1)})...` + ); + continue; + } throw new CLIError( - `Polling failed: ${outcome.reason}.${retryAdvice(outcome)}`, + `Polling failed: ${outcome.reason}${outcome.retryable ? `, ${String(unavailable)} times in a row` : ""}.${abandonedRequestAdvice(context, requestId)}`, "NETWORK_ERROR" ); } @@ -180,6 +227,7 @@ export interface Delivered { export async function deliverRevisions( cwd: string, context: GenerationContext, + requestId: string, revisions: FinishedRequest["revisions"] ): Promise { // The contract requires `revisions` on every status, but `getRequestStatus` @@ -198,16 +246,13 @@ export async function deliverRevisions( revisions.map(async ({ ruleId, revisionId }) => ({ ruleId, revisionId, - outcome: await fetchRule(context.token, ruleId, { - repositoryUrl: context.repositoryUrl, - ...(context.orgId === undefined ? {} : { orgId: context.orgId }), - }), + ...(await fetchGenerated(context, ruleId)), })) ); const verified = []; - for (const { ruleId, revisionId, outcome } of fetched) { - const served = servedOrThrow(ruleId, outcome); + for (const { ruleId, revisionId, outcome, attempts } of fetched) { + const served = servedOrThrow(context, ruleId, requestId, outcome, attempts); const verdict = await verifyServedRule(served, { ruleId, revisionId }); if (!verdict.ok) { throw new CLIError( @@ -231,9 +276,43 @@ export async function deliverRevisions( return delivered; } +type FetchOutcome = Awaited>; + +/** + * Fetch a just-generated rule, retrying a retryable `unavailable` the way + * polling does. The rule already exists by now, so giving up on a blip would + * throw away a generation that succeeded. Never throws: the last outcome is + * returned with the number of attempts it took, for `servedOrThrow` to judge. + */ +async function fetchGenerated( + context: GenerationContext, + ruleId: string +): Promise<{ outcome: FetchOutcome; attempts: number }> { + for (let attempts = 1; ; attempts += 1) { + const outcome = await fetchRule(context.token, ruleId, { + repositoryUrl: context.repositoryUrl, + ...(context.orgId === undefined ? {} : { orgId: context.orgId }), + }); + if ( + outcome.status !== "unavailable" || + !outcome.retryable || + attempts >= MAX_CONSECUTIVE_UNAVAILABLE + ) { + return { outcome, attempts }; + } + context.onProgress( + `Fetching rule ${ruleId} failed (${outcome.reason}); retrying (${String(attempts)}/${String(MAX_CONSECUTIVE_UNAVAILABLE - 1)})...` + ); + await pause(); + } +} + function servedOrThrow( + context: GenerationContext, ruleId: string, - outcome: Awaited> + requestId: string, + outcome: FetchOutcome, + attempts: number ): ServedRule { switch (outcome.status) { case "ok": { @@ -261,8 +340,9 @@ function servedOrThrow( ); } case "unavailable": { + // Nothing has been written: every rule is fetched before any is. throw new CLIError( - `Rule ${ruleId} could not be fetched: ${outcome.reason}.${retryAdvice(outcome)}`, + `Rule ${ruleId} was generated by request ${requestId} but could not be fetched: ${outcome.reason}${attempts > 1 ? `, ${String(attempts)} times in a row` : ""}. No rules were written. Run \`${context.resumeCommand}\` to fetch it again without generating it again.`, "NETWORK_ERROR" ); } diff --git a/packages/cli/src/schemas/rules-create.ts b/packages/cli/src/schemas/rules-create.ts index abeb91ac..4bcffbbd 100644 --- a/packages/cli/src/schemas/rules-create.ts +++ b/packages/cli/src/schemas/rules-create.ts @@ -23,7 +23,7 @@ export const outputSchema = z.object({ requestId: z .string() .describe( - "The generation request's id. Not a rule id: nothing takes it back as one" + "The generation request's id. Not a rule id: only `rule create --resume` takes it back" ), rules: z .array(z.string()) diff --git a/packages/cli/src/schemas/rules-improve.ts b/packages/cli/src/schemas/rules-improve.ts index bc62c288..2279cc2e 100644 --- a/packages/cli/src/schemas/rules-improve.ts +++ b/packages/cli/src/schemas/rules-improve.ts @@ -28,7 +28,11 @@ export const inputSchema = z.object({ /** Output schema for `taskless rule improve --json` on success */ export const outputSchema = z.object({ success: z.literal(true), - requestId: z.string().describe("The iterate request's id"), + requestId: z + .string() + .describe( + "The iterate request's id. Not a rule id: only `rule improve --resume` takes it back" + ), rules: z .array(z.string()) .describe("Ids of the rules that were written (their directory names)"), diff --git a/packages/cli/test/api-v2.test.ts b/packages/cli/test/api-v2.test.ts index 8abe8ed7..3fd3ece6 100644 --- a/packages/cli/test/api-v2.test.ts +++ b/packages/cli/test/api-v2.test.ts @@ -152,6 +152,53 @@ describe("v2 client", () => { }); }); + describe("generation requests", () => { + const REQUEST_ID = "6f1c2b9e-4d3a-4e8b-9c7f-0a1b2c3d4e5f"; + + it.each([ + [ + "submit", + () => submitRequest("tok", { repositoryUrl: REPO, prompt: "p" }), + ], + [ + "iterate", + () => iterateRule("tok", "r", { repositoryUrl: REPO, guidance: "g" }), + ], + ])("accepts a UUID request id from %s", async (_, call) => { + respond(200, { requestId: REQUEST_ID, status: "accepted" }); + expect(await call()).toEqual({ + status: "ok", + data: { requestId: REQUEST_ID, status: "accepted" }, + }); + }); + + // The id is printed inside a `--resume` command an agent is told to run, + // so server text that is not a request id never gets that far. + it.each([ + ["shell metacharacters", "x; rm -rf ~"], + ["a substitution", "$(id)"], + ["control characters", `${REQUEST_ID}\u001B[2J`], + ["a non-string", 42], + ["nothing", undefined], + ])( + "refuses a request id carrying %s as an invalid body", + async (_, requestId) => { + const calls = [ + () => submitRequest("tok", { repositoryUrl: REPO, prompt: "p" }), + () => iterateRule("tok", "r", { repositoryUrl: REPO, guidance: "g" }), + ]; + for (const call of calls) { + respond(200, { requestId, status: "accepted" }); + expect(await call()).toEqual({ + status: "unavailable", + reason: "invalid response body", + retryable: false, + }); + } + } + ); + }); + describe("revisions", () => { const LISTING = { ruleId: "no-eval-3fa9c21b", diff --git a/packages/cli/test/rule-poll-transient.test.ts b/packages/cli/test/rule-poll-transient.test.ts new file mode 100644 index 00000000..f31f4a79 --- /dev/null +++ b/packages/cli/test/rule-poll-transient.test.ts @@ -0,0 +1,334 @@ +import { execFileSync } from "node:child_process"; +import { existsSync } from "node:fs"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + vi, + type MockInstance, +} from "vitest"; + +import { runCommand } from "citty"; + +import { ruleCommand } from "../src/commands/rules"; +import { + ITERATE_REQUEST_ID, + REQUEST_ID, + servedBody, + stubV2Server, + type StubFailure, + type StubRule, +} from "./support/v2-server"; + +/** How many calls the stub received for `method` on a path under `prefix`. */ +function calls( + fetchMock: ReturnType, + method: string, + prefix: string +): number { + return fetchMock.mock.calls.filter((call) => { + const input = call[0] as string | URL | Request; + const request = input instanceof Request ? input : new Request(input); + return ( + request.method === method && + new URL(request.url).pathname.startsWith(prefix) + ); + }).length; +} + +/** `count` copies of one failure. */ +function repeat(count: number, failure: StubFailure): StubFailure[] { + return Array.from({ length: count }, () => failure); +} + +/** + * #466: polling a request, and fetching the rules it produced, gave up on the + * first transient failure. Retrying meant running the command again, which + * submitted a SECOND generation request while the first was still running. + * + * A retryable `unavailable` (a network failure, `408`, `429`, `5xx`) is now + * absorbed up to a consecutive-failure budget; anything that is an answer + * still fails at once. These drive the real command, because the budget and + * the give-up message only mean something end to end. + */ +describe("rule create: transient failures while polling and fetching", () => { + let cwd: string; + let logSpy: MockInstance<(...data: unknown[]) => void>; + let errorSpy: MockInstance<(...data: unknown[]) => void>; + + const rule: StubRule = { + id: "poll-test-rule-3fa9c21b", + engine: "sg", + files: [ + { + path: "poll-test-rule-3fa9c21b.yml", + content: + "id: poll-test-rule-3fa9c21b\nlanguage: TypeScript\nrule:\n pattern: foo\n", + }, + { path: ".tests/fail/case.ts", content: "foo();\n" }, + ], + }; + + async function stub(failures: { + poll?: Array; + fetch?: StubFailure[]; + }): Promise> { + return stubV2Server({ + produced: [{ rule, body: await servedBody(rule, "rev-1") }], + failures, + }); + } + + async function create(): Promise { + const requestFile = join(cwd, "request.json"); + await writeFile(requestFile, JSON.stringify({ prompt: "add a rule" })); + await runCommand(ruleCommand, { + rawArgs: ["create", "--from", requestFile, "--json", "-d", cwd], + }); + } + + async function resume( + subcommand: "create" | "improve", + requestId: string, + ...extra: string[] + ): Promise { + await runCommand(ruleCommand, { + rawArgs: [ + subcommand, + "--resume", + requestId, + ...extra, + "--json", + "-d", + cwd, + ], + }); + } + + function lastEnvelope(): Record { + const lastCall = logSpy.mock.calls.at(-1); + if (!lastCall) throw new Error("console.log was never called"); + return JSON.parse(String(lastCall[0])) as Record; + } + + const ruleFile = (): string => + join(cwd, ".taskless", "rules", "sg", rule.id, `${rule.id}.yml`); + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "taskless-rule-poll-")); + await mkdir(join(cwd, ".taskless"), { recursive: true }); + await writeFile( + join(cwd, ".taskless", "taskless.json"), + JSON.stringify({ + version: "2026-03-03", + orgId: 123, + repositoryUrl: "https://github.com/test/test", + }) + ); + execFileSync("git", ["init"], { cwd }); + execFileSync( + "git", + ["remote", "add", "origin", "https://github.com/test/test.git"], + { cwd } + ); + + process.env.TASKLESS_TOKEN = "test-token"; + process.env.TASKLESS_API_URL = "https://example.invalid/cli"; + process.exitCode = undefined; + + logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + // Collapse every wait between attempts to nothing. + vi.stubGlobal( + "setTimeout", + (function_: (...arguments_: unknown[]) => void) => { + function_(); + return 0 as unknown as NodeJS.Timeout; + } + ); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + delete process.env.TASKLESS_TOKEN; + delete process.env.TASKLESS_API_URL; + process.exitCode = undefined; + await rm(cwd, { recursive: true, force: true }); + }); + + it("keeps polling through a run of retryable failures and delivers the rule", async () => { + const fetchMock = await stub({ + poll: ["network", 503, 429, 408, 502, "network", 500], + }); + + await create(); + + expect(process.exitCode).toBeUndefined(); + expect(lastEnvelope()).toMatchObject({ + success: true, + requestId: REQUEST_ID, + rules: [rule.id], + }); + expect(existsSync(ruleFile())).toBe(true); + // One request was submitted, however many polls it took. + expect(calls(fetchMock, "GET", "/cli/api/v2/request/")).toBe(8); + expect(calls(fetchMock, "POST", "/cli/api/v2/request")).toBe(1); + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining("Status check failed (HTTP 503); retrying") + ); + }); + + it("gives up after the budget, naming the request and saying it may still complete", async () => { + const fetchMock = await stub({ poll: repeat(8, 503) }); + + await expect(create()).rejects.toThrow(); + + expect(process.exitCode).toBe(1); + const envelope = lastEnvelope(); + expect(envelope.code).toBe("NETWORK_ERROR"); + expect(envelope.message).toContain("8 times in a row"); + expect(envelope.message).toContain(REQUEST_ID); + expect(envelope.message).toContain("may still complete"); + expect(envelope.message).toContain(`rule create --resume ${REQUEST_ID}`); + expect(calls(fetchMock, "GET", "/cli/api/v2/request/")).toBe(8); + }); + + it("counts consecutive failures only: an answer in between resets the budget", async () => { + // 14 failures in all, but never 8 in a row. + const run = repeat(7, 429); + const fetchMock = await stub({ poll: [...run, "building", ...run] }); + + await create(); + + expect(process.exitCode).toBeUndefined(); + expect(existsSync(ruleFile())).toBe(true); + expect(calls(fetchMock, "GET", "/cli/api/v2/request/")).toBe(16); + }); + + it("fails at once on a non-retryable unavailable, without spending the budget", async () => { + const fetchMock = await stub({ poll: [418] }); + + await expect(create()).rejects.toThrow(); + + const envelope = lastEnvelope(); + expect(envelope.code).toBe("NETWORK_ERROR"); + expect(envelope.message).toContain("HTTP 418"); + expect(envelope.message).not.toContain("times in a row"); + expect(envelope.message).toContain(REQUEST_ID); + expect(calls(fetchMock, "GET", "/cli/api/v2/request/")).toBe(1); + }); + + it("fails at once on a rejected token", async () => { + const fetchMock = await stub({ poll: [401] }); + + await expect(create()).rejects.toThrow(); + + expect(lastEnvelope().code).toBe("AUTH_REQUIRED"); + expect(calls(fetchMock, "GET", "/cli/api/v2/request/")).toBe(1); + }); + + it("retries fetching a generated rule through transient failures", async () => { + const fetchMock = await stub({ fetch: [503, "network", 429] }); + + await create(); + + expect(process.exitCode).toBeUndefined(); + expect(existsSync(ruleFile())).toBe(true); + expect(calls(fetchMock, "GET", "/cli/api/v2/rule/")).toBe(4); + }); + + it("gives up fetching after the budget, naming the rule and the request, and writes nothing", async () => { + const fetchMock = await stub({ fetch: repeat(8, 500) }); + + await expect(create()).rejects.toThrow(); + + const envelope = lastEnvelope(); + expect(envelope.code).toBe("NETWORK_ERROR"); + expect(envelope.message).toContain(rule.id); + expect(envelope.message).toContain(REQUEST_ID); + expect(envelope.message).toContain("8 times in a row"); + expect(envelope.message).toContain("No rules were written"); + expect(envelope.message).toContain(`rule create --resume ${REQUEST_ID}`); + expect(calls(fetchMock, "GET", "/cli/api/v2/rule/")).toBe(8); + expect(existsSync(ruleFile())).toBe(false); + }); + + describe("--resume", () => { + it("picks up a request a give-up abandoned, without submitting another", async () => { + // One stub across both runs: the first spends all 8 failures and gives + // up, the resumed run finds the service answering again. + const fetchMock = await stub({ poll: repeat(8, 503) }); + await expect(create()).rejects.toThrow(); + expect(existsSync(ruleFile())).toBe(false); + process.exitCode = undefined; + + await resume("create", REQUEST_ID); + + expect(process.exitCode).toBeUndefined(); + expect(lastEnvelope()).toMatchObject({ + success: true, + requestId: REQUEST_ID, + rules: [rule.id], + }); + expect(existsSync(ruleFile())).toBe(true); + expect(calls(fetchMock, "POST", "/cli/api/v2/request")).toBe(1); + }); + + it("fetches a generated rule a give-up left unwritten, without generating it again", async () => { + const fetchMock = await stub({ fetch: repeat(8, 500) }); + await expect(create()).rejects.toThrow(); + process.exitCode = undefined; + + await resume("create", REQUEST_ID); + + expect(existsSync(ruleFile())).toBe(true); + expect(calls(fetchMock, "POST", "/cli/api/v2/request")).toBe(1); + }); + + it("rule improve --resume polls the iterate request and submits no iteration", async () => { + const fetchMock = await stub({}); + + await resume("improve", ITERATE_REQUEST_ID); + + expect(process.exitCode).toBeUndefined(); + expect(lastEnvelope()).toMatchObject({ + success: true, + requestId: ITERATE_REQUEST_ID, + rules: [rule.id], + }); + expect(existsSync(ruleFile())).toBe(true); + expect(calls(fetchMock, "POST", "/cli/api/v2/rule/")).toBe(0); + expect( + calls(fetchMock, "GET", `/cli/api/v2/request/${ITERATE_REQUEST_ID}`) + ).toBe(1); + }); + + it("refuses --resume with --from: a resumed request has nothing to submit", async () => { + const fetchMock = await stub({}); + + await expect( + resume("create", REQUEST_ID, "--from", "request.json") + ).rejects.toThrow(); + + expect(lastEnvelope().code).toBe("INVALID_INPUT"); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("refuses a request id that is not a UUID before calling the service", async () => { + const fetchMock = await stub({}); + + await expect(resume("improve", "no-eval-3fa9c21b")).rejects.toThrow(); + + const envelope = lastEnvelope(); + expect(envelope.code).toBe("INVALID_INPUT"); + expect(envelope.message).toContain("no-eval-3fa9c21b"); + expect(fetchMock).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/cli/test/support/v2-server.ts b/packages/cli/test/support/v2-server.ts index f6058612..29431c52 100644 --- a/packages/cli/test/support/v2-server.ts +++ b/packages/cli/test/support/v2-server.ts @@ -65,6 +65,31 @@ export interface StubServerOptions { status?: string; /** `error` on the terminal status, for `failed` / `unsupported`. */ error?: string; + /** + * Failures to answer with before the real response, consumed one per call + * in order: an HTTP status, or `"network"` for a fetch that throws the way + * Node's does on a transport failure. `poll` answers status checks, and may + * also hold `"building"`, a real in-progress answer; `fetch` answers + * `GET rule/{ruleId}` across all rules. + */ + failures?: { + poll?: Array; + fetch?: StubFailure[]; + }; +} + +/** One injected failure: an HTTP status, or a transport failure. */ +export type StubFailure = number | "network"; + +function failWith(failure: StubFailure): Response { + if (failure === "network") { + throw new TypeError("fetch failed", { + cause: Object.assign(new Error("connect ECONNREFUSED"), { + code: "ECONNREFUSED", + }), + }); + } + return Response.json({}, { status: failure }); } export const REQUEST_ID = "11111111-1111-1111-1111-111111111111"; @@ -74,6 +99,8 @@ export const ITERATE_REQUEST_ID = "22222222-2222-2222-2222-222222222222"; export function stubV2Server( options: StubServerOptions ): ReturnType { + const pollFailures = [...(options.failures?.poll ?? [])]; + const fetchFailures = [...(options.failures?.fetch ?? [])]; const fetchMock = vi.fn((input: string | URL | Request): Response => { const request = input instanceof Request ? input : new Request(input); const url = new URL(request.url); @@ -94,6 +121,15 @@ export function stubV2Server( }); } if (method === "GET" && pathname.startsWith("/cli/api/v2/request/")) { + const failure = pollFailures.shift(); + if (failure === "building") { + return Response.json({ + requestId: pathname.split("/").at(-1), + status: "building", + revisions: [], + }); + } + if (failure !== undefined) return failWith(failure); return Response.json({ requestId: pathname.split("/").at(-1), status: options.status ?? "generated", @@ -105,6 +141,8 @@ export function stubV2Server( }); } if (method === "GET" && pathname.startsWith("/cli/api/v2/rule/")) { + const failure = fetchFailures.shift(); + if (failure !== undefined) return failWith(failure); const ruleId = decodeURIComponent(pathname.split("/").at(-1) ?? ""); const match = options.produced.find(({ rule }) => rule.id === ruleId); return match === undefined