From 2a153e45abc0b005fe0caee1ca5ba1cee864408a Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 28 Sep 2026 20:55:40 -0700 Subject: [PATCH] feat(rules): generate and improve rules through the v2 API, verifying every served file --- openspec/changes/cli-v2-rule-api/tasks.md | 29 +- packages/cli/src/agent/create-remote-rule.md | 27 +- packages/cli/src/agent/improve-rule.md | 45 +- packages/cli/src/agent/rule-meta.md | 19 +- packages/cli/src/agent/rule.md | 5 +- packages/cli/src/api/rules.ts | 160 ----- packages/cli/src/commands/rules.ts | 577 ++++++------------ packages/cli/src/rules/deliver.ts | 61 +- packages/cli/src/rules/files.ts | 46 +- packages/cli/src/rules/generate.ts | 255 ++++++++ packages/cli/src/rules/verify-delivery.ts | 138 +++++ packages/cli/src/schemas/rules-create.ts | 12 +- packages/cli/src/schemas/rules-improve.ts | 10 +- packages/cli/test/api-rule-errors.test.ts | 59 -- packages/cli/test/deliver.test.ts | 29 +- packages/cli/test/repair-integration.test.ts | 8 +- .../cli/test/rule-create-entitlement.test.ts | 185 ++++-- packages/cli/test/rule-from.test.ts | 7 +- .../cli/test/rule-guard-json-envelope.test.ts | 121 ++-- packages/cli/test/support/v2-server.ts | 118 ++++ packages/cli/test/verify-delivery.test.ts | 148 +++++ 21 files changed, 1207 insertions(+), 852 deletions(-) create mode 100644 packages/cli/src/rules/generate.ts create mode 100644 packages/cli/src/rules/verify-delivery.ts delete mode 100644 packages/cli/test/api-rule-errors.test.ts create mode 100644 packages/cli/test/support/v2-server.ts create mode 100644 packages/cli/test/verify-delivery.test.ts diff --git a/openspec/changes/cli-v2-rule-api/tasks.md b/openspec/changes/cli-v2-rule-api/tasks.md index 45632c37..be01330f 100644 --- a/openspec/changes/cli-v2-rule-api/tasks.md +++ b/openspec/changes/cli-v2-rule-api/tasks.md @@ -49,28 +49,29 @@ upgradeUrl }`, strip C0/C1 control characters except newline from ## 3. Generation on v2 (slice 2) -- [ ] 3.1 Add `rules/verify-delivery.ts`: verify a served file set against its +- [x] 3.1 Add `rules/verify-delivery.ts`: verify a served file set against its `signatures` (every signature names a file, every non-`.tests/` file has one, each hash matches, runtime `signature` equals the `check.ts` entry) and that `rules` holds exactly one set whose `id` is the requested id. Unit tests for each refusal. -- [ ] 3.2 Make `writeDeliveredFileSet` the only write path for a served rule and - make it replace the directory (purge files the set lacks, `.tests/` - included; create each file's parent directories). - Drop the legacy single-`content` branch from `deliver.ts` and - `files.ts`. `deliver.test.ts` covers a local extra capture being removed. -- [ ] 3.3 Move `rule create` to v2: submit, poll, fetch each produced +- [x] 3.2 Make `writeDeliveredFileSet` the only write path for a served rule + (`writeServedRule`) and make it replace the directory (purge files the + set lacks, `.tests/` included; create each file's parent directories). + `deliver.test.ts` covers a stale fixture and a local extra capture being + removed. The legacy single-`content` branch still has a caller in the v1 + repair path until 5.3, so it is dropped in 8.1. +- [x] 3.3 Move `rule create` to v2: submit, poll, fetch each produced `{ ruleId, revisionId }` head in parallel without `revision`, confirm `revisionId`, verify, write. Print `error` verbatim (sanitized) on `failed` / `unsupported`. `--json` prints `requestId` and `rules`, no `ruleId`; update `schemas/rules-create.ts`. Tests use a stubbed v2 server. -- [ ] 3.4 Move `rule improve` to `POST v2/rule/{ruleId}/iterate`, with the input +- [x] 3.4 Move `rule improve` to `POST v2/rule/{ruleId}/iterate`, with the input `ruleId` meaning the directory name; `404 rule_not_found` → `RULE_NOT_FOUND`. Tests cover success and the not-found code. -- [ ] 3.5 Keep the write-time entitlement warning for runtime sets served with +- [x] 3.5 Keep the write-time entitlement warning for runtime sets served with `runtimeSignatures: false`; `rule-create-entitlement.test.ts` passes against v2 fixtures. -- [ ] 3.6 Update the `create-remote-rule`, `improve-rule`, and `rule-meta` +- [x] 3.6 Update the `create-remote-rule`, `improve-rule`, and `rule-meta` recipes: record the rule ids from `rules`, pass a directory name to `improve`, never the request id. `recipe-cross-references.test.ts` passes. @@ -156,8 +157,12 @@ upgradeUrl }`, strip C0/C1 control characters except newline from ## 8. Retire v1 (slice 5) -- [ ] 8.1 Delete `api/rules.ts`, `api/reconcile.ts`, `api/restore.ts`, the - frozen v1 `api.schema.json` / `api.d.ts`, and every v1 type use; move `auth/whoami.ts` and `auth/org.ts` to v2 whoami. Verify +- [ ] 8.1 Delete `api/rules.ts` (its v1 request, poll, and iterate calls went + in slice 2), `api/reconcile.ts`, `api/restore.ts`, the frozen v1 + `api.schema.json` / `api.d.ts`, the legacy single-`content` path in + `files.ts` / `deliver.ts` (`writeRuleFile`, `writeRuleTestFile`, + `writeRuleMetaFiles`, `deliveredFiles`, `resolveIngestEngine`), and + every v1 type use; move `auth/whoami.ts` and `auth/org.ts` to v2 whoami. Verify `grep -rn "/cli/api/" packages/cli/src` finds only `/cli/api/v2/` paths. - [ ] 8.2 Add a vite build check (per the code style guide, not a test that scans output) that fails the build if the bundle contains a `/cli/api/` diff --git a/packages/cli/src/agent/create-remote-rule.md b/packages/cli/src/agent/create-remote-rule.md index 82720e5c..18518ccf 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 v3) +# Topic: create-remote-rule (CLI v%(CLI_VERSION)s / topic v4) ## You are here This is `create-remote-rule`. It helps you have the Taskless service @@ -125,17 +125,20 @@ Two ways to legitimately be here: 8. **Clean up.** Delete `.taskless/.tmp-rule-request.json` whether the call succeeded or failed. -9. **Report.** The service writes the rule to - `.taskless/rules/sg//.yml` and its tests to - `.taskless/rules/sg//.tests/-YYYYMMDD-test.yml`. These are - the same paths and the same shape a locally authored rule uses, so - `check`, `improve-rule`, `verify`, and `test` treat them - identically. Nothing is written under `.taskless/rule-metadata/`. - Show the user the paths and suggest `%(TASKLESS_CLI)s agent check`. - - **Record the `ruleId` from the `--json` output.** It is the ticket - id the iterate endpoint is addressed by, `improve-rule` asks for it, - and no file on disk carries it. +9. **Report.** The CLI writes each generated rule, fixtures included, + to `.taskless/rules///`, replacing anything already in + that directory. These are the same paths and the same shape a + locally authored rule uses, so `check`, `improve-rule`, `verify`, and + `test` treat them identically. Nothing is written under + `.taskless/rule-metadata/`. Show the user the paths and suggest + `%(TASKLESS_CLI)s agent check`. + + **`rules` in the `--json` output lists each written rule's id**, and + 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`. **Read `notices` if it is present.** It is an optional array of advisory messages about a delivery that was written anyway. A rule diff --git a/packages/cli/src/agent/improve-rule.md b/packages/cli/src/agent/improve-rule.md index 449de825..ea74a225 100644 --- a/packages/cli/src/agent/improve-rule.md +++ b/packages/cli/src/agent/improve-rule.md @@ -1,10 +1,10 @@ -# Topic: improve-rule (CLI v%(CLI_VERSION)s / topic v5) +# Topic: improve-rule (CLI v%(CLI_VERSION)s / topic v6) ## Goal Iterate on an existing Taskless rule. The CLI submits the user's guidance to the Taskless API iterate endpoint, which returns an updated rule that overwrites the original on disk. The agent's job -is to gather the right ruleId + guidance + supporting references and +is to gather the right rule id + guidance + supporting references and to report the result. If the user wants the local-only flow (no API call), fetch @@ -18,12 +18,12 @@ If the user wants the local-only flow (no API call), fetch `[unknown]`, stop and say the tier is unavailable rather than submitting. `auth login` does not fix it, no GitHub owner is a property of the project, not the session. -- The target rule exists at `.taskless/rules/sg//.yml`. -- You have the rule's **ticket id**: the value `%(TASKLESS_CLI)s rule - create --json` printed as `ruleId` when the rule was generated. The - iterate endpoint is addressed by that id. Nothing on disk holds it, - so it comes from the create output or from the user. Without it, - fetch the anonymous variant instead. +- The target rule exists at `.taskless/rules///`, and the + Taskless service issued it. Its **rule id is its directory name** + (for example `no-eval-3fa9c21b`), which is what the iterate endpoint + is addressed by. A rule you wrote locally, or one generated before + CLI 0.12.0, is not known to the service: improving it fails with + `RULE_NOT_FOUND`, so fetch the anonymous variant instead. ## Steps @@ -31,20 +31,15 @@ If the user wants the local-only flow (no API call), fetch `loggedIn`. If false, fetch `%(TASKLESS_CLI)s agent auth`. 2. **Identify the rule to improve.** If the user named one, use it. - Otherwise, list rules in `.taskless/rules/sg/` and ask which one. - Read the existing rule file so you can summarize what it does. + Otherwise, list the rule directories under `.taskless/rules//` + and ask which one. Read the existing rule files so you can summarize + what the rule does. -3. **Get the ticket id.** This is the id the iterate endpoint is - addressed by, and it is the `ruleId` field from that rule's - `%(TASKLESS_CLI)s rule create --json` output. Take it from the - session that created the rule, or ask the user for it. - - **Do not run `%(TASKLESS_CLI)s rule meta ` to get it.** That - command reads `.taskless/rule-metadata/.yml`, a sidecar this CLI - never writes, so it exits 1 with `RULE_META_UNAVAILABLE` for every - rule. If no one has the ticket id, fetch - `%(TASKLESS_CLI)s agent improve-rule --anonymous` and iterate - locally. +3. **Take the rule id from the directory name.** It is the `` in + `.taskless/rules///`, and the same value + `%(TASKLESS_CLI)s rule create --json` listed in `rules`. It is never + the `requestId` that command printed. You do not need + `%(TASKLESS_CLI)s rule meta` for it; that command has nothing to read. 4. **Gather improvement guidance.** Ask the user what should change: - Are there false positives we need to exclude? @@ -116,9 +111,9 @@ The `--from` JSON file conforms to: %(INPUT_SCHEMA)s ``` -`ruleId` is the original rule's ticket ID, printed as `ruleId` by -`%(TASKLESS_CLI)s rule create --json`. It is not the YAML file name, -and it is not readable from anything under `.taskless/`. +`ruleId` is the rule's directory name under `.taskless/rules//`, +as `%(TASKLESS_CLI)s rule create --json` lists it in `rules`. It is not +the `requestId` from that output. ## Errors @@ -132,7 +127,7 @@ When `--json` is set, failures emit `{ ok: false, code, message }`: | `NO_ORIGIN_REMOTE` | git repository, no `origin` | tell the user; `auth login` cannot fix it | | `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 has no such ticket id | re-check the id from `rule create --json` | +| `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 | | `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/agent/rule-meta.md b/packages/cli/src/agent/rule-meta.md index d0441bf8..50e3dfa9 100644 --- a/packages/cli/src/agent/rule-meta.md +++ b/packages/cli/src/agent/rule-meta.md @@ -1,4 +1,4 @@ -# Topic: rule-meta (CLI v%(CLI_VERSION)s / topic v3) +# Topic: rule-meta (CLI v%(CLI_VERSION)s / topic v4) ## Goal Report what `%(TASKLESS_CLI)s rule meta` does today, so no recipe and no @@ -18,22 +18,19 @@ retry with a different id; there is no id that works. ## What to do instead -`rule improve` needs the ticket id, and that id comes from the machine -that created the rule, not from disk: - -- `%(TASKLESS_CLI)s rule create --json` prints it as `ruleId` on - success. Record it when you create a rule you expect to iterate on. -- If the id was not recorded, ask the user for it. -- If nobody has it, iterate locally: fetch - `%(TASKLESS_CLI)s agent improve-rule --anonymous`. +Nothing you would have used it for needs it. `rule improve`, +`rule restore`, and `rule rollback` take a rule's id, and the id is the +rule's directory name under `.taskless/rules//`, which is on +disk. `%(TASKLESS_CLI)s rule create --json` lists the same ids in +`rules`. ## Errors | code | meaning | fix | |-------------------------|------------------------------------------|---------------------------------------| -| `RULE_META_UNAVAILABLE` | no sidecar exists, and none is written | Use the ticket id from `rule create` | +| `RULE_META_UNAVAILABLE` | no sidecar exists, and none is written | Use the rule's directory name | | `INVALID_INPUT` | a sidecar exists and is malformed | Delete it; nothing here depends on it | ## See Also -- `%(TASKLESS_CLI)s agent improve-rule`: how the ticket id is actually sourced +- `%(TASKLESS_CLI)s agent improve-rule`: iterating a rule by its id diff --git a/packages/cli/src/agent/rule.md b/packages/cli/src/agent/rule.md index f42c5f3e..16ddc791 100644 --- a/packages/cli/src/agent/rule.md +++ b/packages/cli/src/agent/rule.md @@ -1,4 +1,4 @@ -# Topic: rule (CLI v%(CLI_VERSION)s / topic v2) +# Topic: rule (CLI v%(CLI_VERSION)s / topic v3) ## Goal Umbrella for rule operations. Fetch the topic for the action you want. @@ -15,7 +15,8 @@ Umbrella for rule operations. Fetch the topic for the action you want. `rule meta` reads a sidecar this CLI never writes, so it fails for every rule. Fetch `rule-meta` only to learn what to do instead. `improve-rule` -takes the ticket id from `rule create --json`. +takes the rule's id, which is its directory name under +`.taskless/rules//`. `route` is the entry point for authoring: it reads the request and names the `create-*-rule` topic that fits, so you do not pick an engine diff --git a/packages/cli/src/api/rules.ts b/packages/cli/src/api/rules.ts index 43c303b3..9bab50c4 100644 --- a/packages/cli/src/api/rules.ts +++ b/packages/cli/src/api/rules.ts @@ -1,7 +1,4 @@ import type { paths } from "../generated/api"; -import { createApiClient } from "./client"; -import { CLIError } from "../util/cli-error"; -import { getCliPrefix } from "../util/package-manager"; // --- Types extracted from the generated schema --- @@ -48,160 +45,3 @@ export function isSingleContentRule( ): rule is SingleContentRule { return !isFileSetRule(rule); } - -// --- Helpers --- - -/** Extract error details from an untyped error response body */ -function parseErrorBody(rawError: unknown): Record { - if (rawError && typeof rawError === "object") { - return rawError as Record; - } - return {}; -} - -/** - * The server returns the same 404 `organization_not_found` whether the org - * isn't yours or its GitHub App installation doesn't cover this repository (it - * deliberately doesn't distinguish, to avoid leaking org existence), so the - * message names both causes — coverage first, since a resolved org subject - * makes membership the less likely one. - */ -function orgNotFoundMessage(): string { - return [ - "Taskless could not act on this repository for your organization.", - "", - "Most often the organization's Taskless GitHub App installation does not cover this repository. It can also mean your login no longer has access to the organization.", - "", - "- Confirm the Taskless app is installed on this repository's owner and includes this repository.", - `- If access recently changed, re-authenticate with \`${getCliPrefix()} auth login\`.`, - ].join("\n"); -} - -// --- API functions --- - -/** Submit a new rule generation request */ -export async function submitRule( - token: string, - request: { - /** Org subject: Taskless UUID (preferred) or numeric GitHub org id. */ - orgId: string | number; - repositoryUrl: string; - prompt: string; - successCases?: string[]; - failureCases?: string[]; - } -) { - const client = createApiClient(token); - const { data, error, response } = await client.POST("/cli/api/request", { - body: request, - }); - - if (!data) { - const errorData = parseErrorBody(error); - if (response.status === 400 && errorData.error === "validation_error") { - const details = (errorData.details as string[]) ?? []; - throw new Error(`Validation error: ${details.join(", ")}`); - } - if ( - response.status === 403 && - errorData.error === "repository_not_accessible" - ) { - throw new Error( - [ - "Repository is not accessible to this organization.", - "", - "- Verify that your local `origin` remote points to the intended GitHub repository.", - "- Confirm that your GitHub user/organization has access to that repository.", - `- If you recently changed access or remotes, try re-authenticating with \`${getCliPrefix()} auth login\`.`, - ].join("\n") - ); - } - if ( - response.status === 404 && - errorData.error === "organization_not_found" - ) { - throw new Error(orgNotFoundMessage()); - } - throw new Error( - `Request submission failed (HTTP ${String(response.status)})` - ); - } - - return data; -} - -/** Poll for rule generation status */ -export async function pollRuleStatus(token: string, requestId: string) { - const client = createApiClient(token); - const { data, error, response } = await client.GET( - "/cli/api/request/{requestId}", - { - params: { path: { requestId } }, - } - ); - - if (!data) { - const errorData = parseErrorBody(error); - if (response.status === 403 && errorData.error === "access_denied") { - throw new Error("Access denied to this request."); - } - if (response.status === 404 && errorData.error === "request_not_found") { - throw new Error("Request not found. It may have expired."); - } - throw new Error(`Status polling failed (HTTP ${String(response.status)})`); - } - - return data; -} - -/** Submit an improve/iterate request for an existing rule */ -export async function iterateRule( - token: string, - requestId: string, - request: { - /** Org subject: Taskless UUID (preferred) or numeric GitHub org id. */ - orgId: string | number; - guidance: string; - references?: Array<{ filename: string; content: string }>; - } -) { - const client = createApiClient(token); - const { data, error, response } = await client.POST( - "/cli/api/request/{requestId}/iterate", - { - params: { path: { requestId } }, - body: request, - } - ); - - if (!data) { - const errorData = parseErrorBody(error); - if (response.status === 400 && errorData.error === "validation_error") { - const details = (errorData.details as string[]) ?? []; - throw new Error(`Validation error: ${details.join(", ")}`); - } - if (response.status === 403 && errorData.error === "access_denied") { - throw new Error("Access denied to this request."); - } - if (response.status === 404 && errorData.error === "request_not_found") { - // The caller supplied this ticket id (from `rule create --json`, or from - // the user), so "no such id" is a state they can act on: re-check the id. - // The code travels on the error so `improveCommand` reports - // RULE_NOT_FOUND rather than folding it into NETWORK_ERROR, which would - // tell an agent to retry an id that will never resolve. - throw new CLIError( - "Rule not found. It may have expired.", - "RULE_NOT_FOUND" - ); - } - if ( - response.status === 404 && - errorData.error === "organization_not_found" - ) { - throw new Error(orgNotFoundMessage()); - } - throw new Error(`Iterate request failed (HTTP ${String(response.status)})`); - } - - return data; -} diff --git a/packages/cli/src/commands/rules.ts b/packages/cli/src/commands/rules.ts index 540f8b79..84367913 100644 --- a/packages/cli/src/commands/rules.ts +++ b/packages/cli/src/commands/rules.ts @@ -6,26 +6,16 @@ import { defineCommand } from "citty"; import { ZodError } from "zod"; import { identityFailureCode, resolveIdentity } from "../auth/identity"; +import { iterateRule, submitRequest, type V2Outcome } from "../api/v2"; +import type { Identity } from "../auth/identity"; +import { readRuleMetaFile, deleteRuleFiles } from "../rules/files"; import { - submitRule, - pollRuleStatus, - iterateRule, - isSingleContentRule, - type GeneratedRule, -} from "../api/rules"; -import { - notRunOnPlanSentence, - parseEntitlement, - type MayCarryEntitlement, -} from "../api/entitlement"; -import { - writeRuleFile, - writeRuleTestFile, - writeRuleMetaFiles, - readRuleMetaFile, - deleteRuleFiles, -} from "../rules/files"; -import { resolveIngestEngine } from "../rules/engines"; + awaitRequest, + deliverRevisions, + orgNotFoundMessage, + requestErrorText, + type Delivered, +} from "../rules/generate"; import { RULES_DIRECTORY } from "../rules/layout"; import { unsupportedMessage } from "../rules/unsupported"; import { @@ -41,63 +31,158 @@ import { getTelemetry } from "../telemetry"; import { CLIError } from "../util/cli-error"; import { type CLIErrorCode, writeJsonError } from "../types/errors"; -/** - * The warning for a runtime rule written under a plan that will not run it, or - * `undefined` when there is nothing to say. - * - * The rule is still written: it is the organization's rule, and it runs again - * the moment the plan allows. What must not happen is the author finishing - * `rule create` believing it is live. Static rules never warn. - */ -function notRunOnPlanNotice( - status: MayCarryEntitlement, - rule: unknown, - ruleFile: string -): string | undefined { - const entitlement = parseEntitlement(status.entitlement); - if (entitlement === undefined) return undefined; - if (resolveIngestEngine(rule) !== "runtime") return undefined; - return `${ruleFile} was written. ${notRunOnPlanSentence(entitlement)}`; +/** Why submitting a request or an iteration failed, as a message and a code. */ +function describeSubmitFailure( + outcome: Exclude, { status: "ok" }>, + ruleId?: string +): { message: string; code: CLIErrorCode } { + switch (outcome.status) { + case "unauthorized": { + return { + message: "Authentication was rejected. Log in again.", + code: "AUTH_REQUIRED", + }; + } + case "unavailable": { + return { + message: `Request submission failed: ${outcome.reason}.`, + code: "NETWORK_ERROR", + }; + } + case "refused": { + return { message: outcome.refusal.message, code: "NETWORK_ERROR" }; + } + case "error": { + switch (outcome.code) { + case "validation_error": { + return { + message: `Validation error: ${(outcome.details ?? []).join(", ")}`, + code: "INVALID_INPUT", + }; + } + case "organization_not_found": { + return { message: orgNotFoundMessage(), code: "NETWORK_ERROR" }; + } + case "rule_not_found": { + // The caller supplied this id, so "no such rule" is a state they can + // act on: re-check it. It is a rule's directory name, never the + // request id `rule create` prints. + return { + message: + `Rule ${ruleId ?? ""} was not found for this repository. ` + + `\`ruleId\` is the rule's directory name under \`.taskless/rules//\`.`, + code: "RULE_NOT_FOUND", + }; + } + case "enqueue_failed": { + return { + message: + "The request was recorded but could not be queued. Try again.", + code: "NETWORK_ERROR", + }; + } + default: { + return { + message: `Request submission failed (${outcome.code}).`, + code: "NETWORK_ERROR", + }; + } + } + } + } } -/** Format today's date as YYYYMMDD */ -function getTimestamp(): string { - const now = new Date(); - const year = String(now.getFullYear()); - const month = String(now.getMonth() + 1).padStart(2, "0"); - const day = String(now.getDate()).padStart(2, "0"); - return `${year}${month}${day}`; +/** How {@link completeRequest} reports, per command. */ +interface CompletionOptions { + json: boolean; + fail: (message: string, code?: CLIErrorCode) => never; + /** "Generated" or "Updated", for human output. */ + verb: string; + /** The prefix for a request that ended `failed`. */ + failedPrefix: string; + /** The command's `--json` success envelope for what was written. */ + output: ( + result: Omit & { notices?: string[] } + ) => unknown; } -const POLL_INTERVAL_MS = 15_000; - /** - * A file set rule carries its fixtures as ordinary files under `.tests/`, - * already written by `writeRuleFile`. Only the single-content envelope has a - * separate `tests` field to write. - * - * A file set arriving WITH a stray `tests` is unrepresentable in the - * published schema, and if the service ever sent one it would be dropped in - * silence. Named rather than ignored, because everything else on this path - * fails loudly when the contract is broken, and a fixture that vanishes is - * exactly the kind of loss that shows up later as a rule which tests nothing. - * - * `create` and `improve` both run this guard over the same - * `GeneratedRule` shape with the same remedy, so it is shared here rather - * than copied: two unmaintained copies is how they drift, and neither had a - * test before this one did. + * Poll a submitted request to its end and deliver what it produced. Shared by + * `create` and `improve`, whose only differences are wording and the envelope. + * Returns how many rules were written. */ -function fileSetTestsFieldError(rule: GeneratedRule): string | undefined { - if ( - !isSingleContentRule(rule) && - (rule as { tests?: unknown }).tests !== undefined - ) { - return ( - `Rule "${rule.id}" was delivered as a file set and also carries \`tests\`; ` + - `a file set's fixtures belong in its own \`.tests/\` files.` +async function completeRequest( + cwd: string, + identity: Identity, + requestId: string, + options: CompletionOptions +): Promise { + const context = { + token: identity.token, + repositoryUrl: identity.repositoryUrl, + orgId: identity.orgSubject, + onProgress: (message: string) => console.error(message), + }; + + let delivered: Delivered; + try { + const status = await awaitRequest(context, requestId); + switch (status.status) { + case "unsupported": { + options.fail( + unsupportedMessage(requestErrorText(status)), + "RULE_UNSUPPORTED" + ); + break; + } + case "failed": { + options.fail( + `${options.failedPrefix}: ${requestErrorText(status) ?? "no reason was given"}`, + "RULE_GENERATION_FAILED" + ); + break; + } + case "generated": { + break; + } + default: { + // `pr`, `merged`, `closed`: states a CLI request does not reach. Report + // the state rather than inventing a delivery. + if (options.json) { + console.log(JSON.stringify(options.output({ rules: [], files: [] }))); + } else { + console.log(`Request ${requestId} is in state "${status.status}".`); + } + return 0; + } + } + delivered = await deliverRevisions(cwd, context, status.revisions); + } catch (error) { + if (error instanceof CLIError && error.reported) throw error; + options.fail( + error instanceof Error ? error.message : String(error), + error instanceof CLIError && error.code ? error.code : "INTERNAL_ERROR" ); } - return undefined; + + if (options.json) { + console.log( + JSON.stringify( + options.output({ + rules: delivered.rules, + files: delivered.files, + ...(delivered.notices.length > 0 + ? { notices: delivered.notices } + : {}), + }) + ) + ); + } else { + for (const notice of delivered.notices) console.error(`Warning: ${notice}`); + console.log(`${options.verb} ${String(delivered.rules.length)} rule(s):\n`); + for (const filePath of delivered.files) console.log(` ${filePath}`); + } + return delivered.rules.length; } const createCommand = defineCommand({ @@ -218,163 +303,31 @@ const createCommand = defineCommand({ fail(message, identityFailureCode(error)); } - // 3. Submit rule to API - let ruleId: string; - try { - const response = await submitRule(identity.token, { - orgId: identity.orgSubject, - repositoryUrl: identity.repositoryUrl, - prompt: request.prompt, - successCases: request.successCases, - failureCases: request.failureCases, - }); - // The canonical field. `ruleId` is still on the response, carrying the - // same value, and is marked deprecated: it never named a rule. The - // local keeps its name because it feeds this command's own `--json` - // `ruleId` field, which is OUR published contract and a separate - // decision from the service's — renaming that one moves the ground - // under every recipe and agent that reads it. - ruleId = response.requestId; - } catch (error) { - fail( - error instanceof Error ? error.message : String(error), - "NETWORK_ERROR" - ); - } - - // 4. Poll for results - console.error(`Rule submitted (${ruleId}). Waiting for generation...`); - - while (true) { - await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); - - let status; - try { - status = await pollRuleStatus(identity.token, ruleId); - } catch (error) { - fail( - `Polling failed: ${error instanceof Error ? error.message : String(error)}`, - "NETWORK_ERROR" - ); - } - - switch (status.status) { - case "accepted": { - console.error("Status: accepted — waiting for processing..."); - break; - } - case "classifying": { - console.error("Status: classifying — analyzing your request..."); - break; - } - case "building": { - console.error("Status: building — generating rules..."); - break; - } - case "unsupported": { - fail(unsupportedMessage(status.error), "RULE_UNSUPPORTED"); - break; - } - case "failed": { - fail( - `Rule generation failed: ${status.error}`, - "RULE_GENERATION_FAILED" - ); - break; - } - case "generated": { - // 6. Write files - const timestamp = getTimestamp(); - const writtenFiles: string[] = []; - const notices: string[] = []; - const rules = status.rules ?? []; - - for (const rule of rules) { - // Collected AND printed, and neither alone is enough. Under - // `--json` the prose is suppressed and the message rides in the - // envelope instead, because a machine consumer cannot read - // stderr — and an unattended caller is the one most likely to - // act on a fixture-less delivery. `check` already settles this - // trade the same way. The rule is written either way. - const ruleFile = await writeRuleFile(cwd, rule, (message) => { - notices.push(message); - if (!args.json) console.error(`Warning: ${message}`); - }); - writtenFiles.push(ruleFile); - const planWarning = notRunOnPlanNotice(status, rule, ruleFile); - if (planWarning !== undefined) { - notices.push(planWarning); - if (!args.json) console.error(`Warning: ${planWarning}`); - } - - // See `fileSetTestsFieldError` for why this is a guard rather - // than a silent drop. - const strayTestsError = fileSetTestsFieldError(rule); - if (strayTestsError !== undefined) { - // Route through `fail()`, not a bare throw: this is inside - // the command's own `try`, and a throw here that never - // touches `fail()` skips the `--json` envelope entirely (see - // #280). `writtenFiles` already holds every rule file written - // earlier in this loop, but the envelope shape this command - // publishes has no field to carry a partial file list on - // failure — extending it is a schema change, out of scope - // here (see the PR description). - fail(strayTestsError, "RULE_GENERATION_FAILED"); - } - if (isSingleContentRule(rule) && rule.tests) { - const testFile = await writeRuleTestFile(cwd, rule, timestamp); - writtenFiles.push(testFile); - } - } - - // Dead in practice, and kept deliberately. The service does not - // populate `meta` on a status response, so no sidecar has ever - // been written from here; the branch is what would start writing - // one the day it does. `rule meta` says as much when asked, rather - // than the code pretending the file is merely absent. - if (status.meta) { - const metaFiles = await writeRuleMetaFiles(cwd, status.meta); - writtenFiles.push(...metaFiles); - } - - // 7. Output results - if (args.json) { - const output = createOutputSchema.parse({ - success: true, - ruleId, - rules: rules.map((r) => r.id), - files: writtenFiles, - ...(notices.length > 0 ? { notices } : {}), - }); - console.log(JSON.stringify(output)); - } else { - console.log(`Generated ${String(rules.length)} rule(s):\n`); - for (const filePath of writtenFiles) { - console.log(` ${filePath}`); - } - } - if (rules.length > 0) createdRuleCount = rules.length; - return; - } - case "pr": - case "merged": - case "closed": { - // Terminal states beyond generation — treat as done without files - if (args.json) { - const output = createOutputSchema.parse({ - success: true, - ruleId, - rules: [], - files: [], - }); - console.log(JSON.stringify(output)); - } else { - console.log(`Rule ${ruleId} is in state "${status.status}".`); - } - return; - } - } + // 3. Submit the request + const submitted = await submitRequest(identity.token, { + orgId: identity.orgSubject, + repositoryUrl: identity.repositoryUrl, + prompt: request.prompt, + successCases: request.successCases, + failureCases: request.failureCases, + }); + if (submitted.status !== "ok") { + const failure = describeSubmitFailure(submitted); + fail(failure.message, failure.code); } + const requestId = submitted.data.requestId; + + // 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; } finally { // Concrete state event: a rule was actually generated and written. if (createdRuleCount !== undefined) { @@ -496,162 +449,34 @@ const improveCommand = defineCommand({ fail(message, identityFailureCode(error)); } - // 3. Submit iterate request to API - let requestId: string; - try { - const response = await iterateRule(identity.token, request.ruleId, { - orgId: identity.orgSubject, - guidance: request.guidance, - references: request.references, - }); - requestId = response.requestId; - } catch (error) { - // `iterateRule` marks the failures the caller can act on by throwing a - // CLIError carrying the code (a 404 on the supplied ticket id is - // RULE_NOT_FOUND, not something to retry). Everything else really is - // an unclassified transport/API failure. - fail( - error instanceof Error ? error.message : String(error), - error instanceof CLIError && error.code ? error.code : "NETWORK_ERROR" - ); + // 3. Submit the iterate request, addressed by the rule's own id + const submitted = await iterateRule(identity.token, request.ruleId, { + orgId: identity.orgSubject, + repositoryUrl: identity.repositoryUrl, + guidance: request.guidance, + ...(request.references === undefined + ? {} + : { references: request.references }), + }); + if (submitted.status !== "ok") { + const failure = describeSubmitFailure(submitted, request.ruleId); + fail(failure.message, failure.code); } + const requestId = submitted.data.requestId; - // 4. Poll for results using the requestId + // 4. Poll, then fetch, verify, and write the new revision console.error( `Iterate request submitted (${requestId}). Waiting for generation...` ); - - while (true) { - await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); - - let status; - try { - status = await pollRuleStatus(identity.token, requestId); - } catch (error) { - fail( - `Polling failed: ${error instanceof Error ? error.message : String(error)}`, - "NETWORK_ERROR" - ); - } - - switch (status.status) { - case "accepted": { - console.error("Status: accepted — waiting for processing..."); - break; - } - case "classifying": { - console.error("Status: classifying — analyzing your request..."); - break; - } - case "building": { - console.error("Status: building — generating rules..."); - break; - } - case "unsupported": { - fail(unsupportedMessage(status.error), "RULE_UNSUPPORTED"); - break; - } - case "failed": { - fail( - `Rule iteration failed: ${status.error}`, - "RULE_GENERATION_FAILED" - ); - break; - } - case "generated": { - // 6. Write files (overwrites existing rule files) - const timestamp = getTimestamp(); - const writtenFiles: string[] = []; - const notices: string[] = []; - const rules = status.rules ?? []; - - for (const rule of rules) { - // Collected AND printed, and neither alone is enough. Under - // `--json` the prose is suppressed and the message rides in the - // envelope instead, because a machine consumer cannot read - // stderr — and an unattended caller is the one most likely to - // act on a fixture-less delivery. `check` already settles this - // trade the same way. The rule is written either way. - const ruleFile = await writeRuleFile(cwd, rule, (message) => { - notices.push(message); - if (!args.json) console.error(`Warning: ${message}`); - }); - writtenFiles.push(ruleFile); - const planWarning = notRunOnPlanNotice(status, rule, ruleFile); - if (planWarning !== undefined) { - notices.push(planWarning); - if (!args.json) console.error(`Warning: ${planWarning}`); - } - - // See `fileSetTestsFieldError` for why this is a guard rather - // than a silent drop. - const strayTestsError = fileSetTestsFieldError(rule); - if (strayTestsError !== undefined) { - // Route through `fail()`, not a bare throw: this is inside - // the command's own `try`, and a throw here that never - // touches `fail()` skips the `--json` envelope entirely (see - // #280). `writtenFiles` already holds every rule file written - // earlier in this loop, but the envelope shape this command - // publishes has no field to carry a partial file list on - // failure — extending it is a schema change, out of scope - // here (see the PR description). - fail(strayTestsError, "RULE_GENERATION_FAILED"); - } - if (isSingleContentRule(rule) && rule.tests) { - const testFile = await writeRuleTestFile(cwd, rule, timestamp); - writtenFiles.push(testFile); - } - } - - // Dead in practice, and kept deliberately. The service does not - // populate `meta` on a status response, so no sidecar has ever - // been written from here; the branch is what would start writing - // one the day it does. `rule meta` says as much when asked, rather - // than the code pretending the file is merely absent. - if (status.meta) { - const metaFiles = await writeRuleMetaFiles(cwd, status.meta); - writtenFiles.push(...metaFiles); - } - - // 7. Output results - if (args.json) { - const output = improveOutputSchema.parse({ - success: true, - requestId, - rules: rules.map((r) => r.id), - files: writtenFiles, - ...(notices.length > 0 ? { notices } : {}), - }); - console.log(JSON.stringify(output)); - } else { - console.log(`Updated ${String(rules.length)} rule(s):\n`); - for (const filePath of writtenFiles) { - console.log(` ${filePath}`); - } - } - if (rules.length > 0) improvedRuleCount = rules.length; - return; - } - case "pr": - case "merged": - case "closed": { - if (args.json) { - const output = improveOutputSchema.parse({ - success: true, - requestId, - rules: [], - files: [], - }); - console.log(JSON.stringify(output)); - } else { - console.log( - `Request ${requestId} is in state "${status.status}".` - ); - } - return; - } - } - } + 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; } finally { // Concrete state event: a rule was actually iterated and rewritten. if (improvedRuleCount !== undefined) { @@ -719,7 +544,7 @@ const metaCommand = defineCommand({ `No metadata sidecar exists for rule "${args.id}", and this version of the CLI never writes one: ` + `the rule service does not return the metadata block that ` + `.taskless/rule-metadata/ is written from. This is not specific to "${args.id}". ` + - `To iterate on a rule, pass the ticket id returned as "ruleId" by \`taskless rule create --json\` ` + + `To iterate on a rule, pass its id (the rule's directory name under .taskless/rules//) ` + `to \`taskless rule improve --from \`, or use the local-only improve flow.`, "RULE_META_UNAVAILABLE" ); diff --git a/packages/cli/src/rules/deliver.ts b/packages/cli/src/rules/deliver.ts index 15a2fae6..b5d08232 100644 --- a/packages/cli/src/rules/deliver.ts +++ b/packages/cli/src/rules/deliver.ts @@ -193,16 +193,6 @@ export function describeIncompleteSet( * policed: what makes a fixture meaningful differs per engine, every engine's * own runner already judges it, and a second opinion here would be a worse one * computed from less. - * - * **This answers a question about the DELIVERY, and the caller must not report - * it as a question about the rule.** `.tests/` is in {@link PRESERVED_SUBTREES}, - * so fixtures already on disk survive a delivery that never mentions them — - * which is the normal case rather than an edge one, since `writeRuleTestFile` - * accumulates fixtures locally that no later file set names by construction. - * A rule can therefore be delivered with no fixtures and still be exercised by - * every one of them. {@link writeRuleFile} pairs this with what is actually on - * disk before saying anything, and the message here is written to be true only - * under that pairing. */ export function describeMissingFixtures( files: readonly DeliveredFile[] @@ -294,42 +284,6 @@ export function assessDelivery( return { ok: true, files }; } -/** - * The one subtree inside a rule directory a delivered set does not govern. - * - * **`.tests/` SURVIVES A DELIVERY THAT DOES NOT MENTION IT, DELIBERATELY.** - * Everything else under the rule directory is removed when the set omits it - * (see {@link writeDeliveredFileSet}). Stating the exception here rather than - * deciding it silently is the point: a fixture that vanishes surfaces later as - * a rule that tests nothing, and nobody connects that to a delivery weeks - * earlier. - * - * Two reasons, and the first is the one that matters: - * - * - **Nothing under `.tests/` reaches an engine.** The dot is what makes - * ast-grep skip the directory during rule discovery (see - * {@link RULE_TESTS_DIRECTORY}), `strayModules` already exempts it for the - * same reason, and runtime capture discovery skips it by name. A stale - * fixture therefore cannot change what a rule matches, which is the entire - * harm this purge exists to prevent. A stale fixture fails a test run loudly, - * in front of someone already looking at that rule. - * - **This CLI writes files there that no delivered set will ever name.** - * `writeRuleTestFile` writes `--test.yml` on every - * single-content create and iterate, so fixtures accumulate locally and are - * absent from a later file-set delivery by construction. Purging `.tests/` - * would delete a rule's whole local test history the first time it was - * redelivered as a file set. - * - * Only the top-level `.tests/` is exempt. `RULE_TESTS_DIRECTORY` is defined - * relative to the rule directory, so a nested `captures/.tests/` is not a test - * directory — it is a file an engine reads, and it is purged like any other. - * - * A delivered set may still WRITE into `.tests/`; that is how a file set ships - * its own fixtures. Exempt means "not deleted for going unmentioned", not "off - * limits". - */ -const PRESERVED_SUBTREES = new Set([RULE_TESTS_DIRECTORY]); - /** What a rule directory holds, as paths relative to it. */ interface RuleDirectoryContents { /** Every file and symlink the purge may consider. */ @@ -339,8 +293,14 @@ interface RuleDirectoryContents { } /** - * Enumerate a rule directory, skipping the subtrees a delivered set does not - * govern. + * Enumerate a rule directory, every subtree included. + * + * `.tests/` is enumerated like everything else. It used to be spared, because + * the v1 single-content envelope wrote fixtures locally that no later file set + * would name. v2 serves a rule's fixtures in every file set (confirmed with the + * rules team, 2026-09-29), so a fixture the set does not name is stale, and a + * stale fixture makes `taskless test` judge the rule against cases nobody + * issued. * * Symlinks are listed as files rather than descended into. `readdir` does not * follow them, so `isDirectory()` is false for a link to a directory, and @@ -369,7 +329,6 @@ async function readRuleDirectory( } for (const entry of entries) { const path = prefix === "" ? entry.name : `${prefix}/${entry.name}`; - if (PRESERVED_SUBTREES.has(path)) continue; if (entry.isDirectory()) { directories.push(path); await walk(join(absolute, entry.name), path); @@ -522,8 +481,8 @@ async function purgeUndeliveredFiles( // Deepest first, so a directory emptied by pruning its children is prunable // in the same pass. `rmdir` rather than a recursive `rm` precisely because it - // REFUSES a non-empty directory: one still holding a delivered file, or a - // preserved `.tests/`, must survive, and the filesystem answering "not empty" + // REFUSES a non-empty directory: one still holding a delivered file must + // survive, and the filesystem answering "not empty" // is a stronger guarantee of that than bookkeeping kept correct by hand. const deepestFirst = contents.directories.toSorted( (a, b) => b.split("/").length - a.split("/").length diff --git a/packages/cli/src/rules/files.ts b/packages/cli/src/rules/files.ts index 69379f97..7b72fc7d 100644 --- a/packages/cli/src/rules/files.ts +++ b/packages/cli/src/rules/files.ts @@ -6,6 +6,7 @@ import { parse, stringify } from "yaml"; import { ensureTasklessDirectory } from "../filesystem/directory"; import { isSingleContentRule } from "../api/rules"; import type { GeneratedRule, RuleMetadata } from "../api/rules"; +import type { ServedFileSet } from "../api/v2"; import { resolveIngestEngine, ruleDirectory, @@ -13,7 +14,7 @@ import { ruleTestsDirectory, findRuleEngines, } from "./engines"; -import type { EngineName } from "./layout"; +import { isKnownEngine, type EngineName } from "./layout"; import { describeRuleIdCollision, findRuleIdCollision } from "./id-uniqueness"; import { isValidRuleId } from "./validate-id"; import { @@ -160,6 +161,49 @@ export async function writeRuleFile( return filePath; } +/** + * Write a rule served by the v2 API into `.taskless/rules///`. + * + * The caller must already have passed the set through `verifyServedRule`: + * this function checks that the set is a complete, writable rule, and trusts + * that its bytes are the issued ones. The engine is the set's own `engine`, + * which v2 always sends, so nothing is inferred from the payload's shape. + * + * The set IS the directory: whatever is on disk that the set does not name is + * removed, `.tests/` included, since v2 serves a rule's fixtures with it. + */ +export async function writeServedRule( + cwd: string, + fileSet: ServedFileSet, + onWarning?: (message: string) => void +): Promise { + if (!isValidRuleId(fileSet.id)) { + throw new Error(`Invalid rule ID "${fileSet.id}"`); + } + const engine: string = fileSet.engine; + if (!isKnownEngine(engine)) { + throw new Error( + `Rule "${fileSet.id}" is a ${engine} rule, which this version of the CLI does not support. Upgrade the Taskless CLI and try again.` + ); + } + const assessment = assessDelivery(cwd, engine, fileSet.id, fileSet.files); + if (!assessment.ok) { + throw new Error(`Rule "${fileSet.id}" ${assessment.reason}.`); + } + await ensureTasklessDirectory(cwd); + await mkdir(ruleDirectory(cwd, engine, fileSet.id), { recursive: true }); + await writeDeliveredFileSet(cwd, engine, fileSet.id, assessment); + // After the write, and asked of the set alone: the purge made the directory + // equal to the set, so "the delivery carried no fixtures" and "the rule has + // none" are now the same statement. + const missingFixtures = describeMissingFixtures(assessment.files); + if (missingFixtures !== undefined) { + onWarning?.(`Rule "${fileSet.id}" ${missingFixtures}.`); + } + await warnOnIdCollision(cwd, fileSet.id, onWarning); + return ruleFilePath(cwd, engine, fileSet.id); +} + /** * Say so when the rule just written shares its id with another engine's. * diff --git a/packages/cli/src/rules/generate.ts b/packages/cli/src/rules/generate.ts new file mode 100644 index 00000000..b8955940 --- /dev/null +++ b/packages/cli/src/rules/generate.ts @@ -0,0 +1,255 @@ +import { + fetchRule, + getRequestStatus, + type RequestStatus, + type ServedRule, +} from "../api/v2"; +import { notRunOnPlanSentence, parseEntitlementV2 } from "../api/entitlement"; +import { stripControlCharacters } from "../api/refusal"; +import { CLIError } from "../util/cli-error"; +import { getCliPrefix } from "../util/package-manager"; +import { writeServedRule } from "./files"; +import { verifyServedRule } from "./verify-delivery"; + +/** + * The v2 generation flow `rule create` and `rule improve` share: poll a + * request to an end state, fetch each rule it produced by its own id, verify + * the bytes, and write. + * + * v2 polling returns `{ ruleId, revisionId }` pairs and never content, which is + * what makes a rule addressable after the request that produced it is + * forgotten. The content comes from `GET rule/{ruleId}`, fetched WITHOUT + * `revision`: a just-generated rule is its rule's head, and the head is served + * on every plan, whereas naming a revision is a recovery read that a Free + * organization is refused. The `revisionId` from polling is then how the CLI + * knows the head it got is the revision it asked for. + */ + +/** Where a generation request is, and how to reach it. */ +export interface GenerationContext { + token: string; + repositoryUrl: string; + orgId?: string | number; + /** Progress lines for a human; never part of `--json` output. */ + onProgress: (message: string) => void; +} + +const POLL_INTERVAL_MS = 15_000; + +/** + * 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 + * deliberately doesn't distinguish, to avoid leaking org existence), so the + * message names both causes — coverage first, since a resolved org subject + * makes membership the less likely one. + */ +export function orgNotFoundMessage(): string { + return [ + "Taskless could not act on this repository for your organization.", + "", + "Most often the organization's Taskless GitHub App installation does not cover this repository. It can also mean your login no longer has access to the organization.", + "", + "- Confirm the Taskless app is installed on this repository's owner and includes this repository.", + `- If access recently changed, re-authenticate with \`${getCliPrefix()} auth login\`.`, + ].join("\n"); +} + +/** A request's terminal status. */ +export type FinishedRequest = RequestStatus; + +/** + * Poll a request until it stops moving. + * + * Throws a `CLIError` for anything that is not an answer about the request: + * `request_not_found` (the id will never resolve), a rejected token, and an + * unreachable service, each with the code a caller branches on. + */ +export async function awaitRequest( + context: GenerationContext, + requestId: string +): Promise { + while (true) { + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); + + const outcome = await getRequestStatus(context.token, requestId, { + repositoryUrl: context.repositoryUrl, + ...(context.orgId === undefined ? {} : { orgId: context.orgId }), + }); + switch (outcome.status) { + case "ok": { + break; + } + case "unauthorized": { + throw new CLIError( + "Polling failed: authentication was rejected. Log in again.", + "AUTH_REQUIRED" + ); + } + case "error": { + throw new CLIError( + outcome.code === "request_not_found" + ? `Request ${requestId} was not found for this repository.` + : `Polling failed (${outcome.code}).`, + outcome.code === "request_not_found" + ? "RULE_NOT_FOUND" + : "NETWORK_ERROR" + ); + } + case "refused": + case "unavailable": { + throw new CLIError( + `Polling failed: ${outcome.status === "unavailable" ? outcome.reason : "unexpected refusal"}.`, + "NETWORK_ERROR" + ); + } + } + + const status = outcome.data; + switch (status.status) { + case "accepted": { + context.onProgress("Status: accepted — waiting for processing..."); + continue; + } + case "classifying": { + context.onProgress("Status: classifying — analyzing your request..."); + continue; + } + case "building": { + context.onProgress("Status: building — generating rules..."); + continue; + } + default: { + return status; + } + } + } +} + +/** + * The server-authored reason a request ended without rules, safe to print. + * + * `error` carries plan refusals (`REMOTE_GENERATION_NOT_IN_PLAN`) and, later, + * `CLI_UPGRADE_REQUIRED`. It is printed as given, minus control characters, + * because it is written to a terminal from across the network. + */ +export function requestErrorText(status: FinishedRequest): string | undefined { + const text = status.error?.trim(); + return text === undefined || text === "" + ? undefined + : stripControlCharacters(text); +} + +/** What delivering a finished request wrote. */ +export interface Delivered { + /** Rule ids written, in the order the request listed them. */ + rules: string[]; + /** The rule file of each rule written. */ + files: string[]; + /** Advisory messages about rules that were still written. */ + notices: string[]; +} + +/** + * Fetch, verify, then write every rule a request produced. + * + * All rules are fetched and verified before ANY is written, so a request whose + * second rule fails verification leaves the tree exactly as it was rather than + * holding half a delivery that already reported nothing. + */ +export async function deliverRevisions( + cwd: string, + context: GenerationContext, + revisions: FinishedRequest["revisions"] +): Promise { + const fetched = await Promise.all( + revisions.map(async ({ ruleId, revisionId }) => ({ + ruleId, + revisionId, + outcome: await fetchRule(context.token, ruleId, { + repositoryUrl: context.repositoryUrl, + ...(context.orgId === undefined ? {} : { orgId: context.orgId }), + }), + })) + ); + + const verified = []; + for (const { ruleId, revisionId, outcome } of fetched) { + const served = servedOrThrow(ruleId, outcome); + const verdict = await verifyServedRule(served, { ruleId, revisionId }); + if (!verdict.ok) { + throw new CLIError( + `The generated rule could not be delivered: ${verdict.reason}.`, + "RULE_GENERATION_FAILED" + ); + } + verified.push({ served, fileSet: verdict.fileSet }); + } + + const delivered: Delivered = { rules: [], files: [], notices: [] }; + for (const { served, fileSet } of verified) { + const ruleFile = await writeServedRule(cwd, fileSet, (message) => { + delivered.notices.push(message); + }); + delivered.rules.push(fileSet.id); + delivered.files.push(ruleFile); + const planNotice = notRunOnPlanNotice(served, fileSet.engine, ruleFile); + if (planNotice !== undefined) delivered.notices.push(planNotice); + } + return delivered; +} + +function servedOrThrow( + ruleId: string, + outcome: Awaited> +): ServedRule { + switch (outcome.status) { + case "ok": { + return outcome.data; + } + case "refused": { + // A head is served on every plan, so this means the service answered for + // 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_GENERATION_FAILED" + ); + } + case "error": { + throw new CLIError( + `Rule ${ruleId} was generated but could not be fetched (${outcome.code}).`, + "RULE_GENERATION_FAILED" + ); + } + case "unauthorized": { + throw new CLIError( + `Rule ${ruleId} could not be fetched: authentication was rejected. Log in again.`, + "AUTH_REQUIRED" + ); + } + case "unavailable": { + throw new CLIError( + `Rule ${ruleId} could not be fetched: ${outcome.reason}.`, + "NETWORK_ERROR" + ); + } + } +} + +/** + * The warning for a runtime rule written under a plan that will not run it. + * + * The rule is still written: it is the organization's rule, and it runs the + * moment the plan allows. What must not happen is the author finishing + * `rule create` believing it is live. Static rules never warn. + */ +function notRunOnPlanNotice( + served: ServedRule, + engine: string, + ruleFile: string +): string | undefined { + if (engine !== "runtime") return undefined; + const entitlement = parseEntitlementV2(served.entitlement); + if (entitlement === undefined) return undefined; + return `${ruleFile} was written. ${notRunOnPlanSentence(entitlement)}`; +} diff --git a/packages/cli/src/rules/verify-delivery.ts b/packages/cli/src/rules/verify-delivery.ts new file mode 100644 index 00000000..38bdce50 --- /dev/null +++ b/packages/cli/src/rules/verify-delivery.ts @@ -0,0 +1,138 @@ +import type { ServedFileSet, ServedRule } from "../api/v2"; +import { canonicalHash } from "./rule-hash"; +import { RULE_TESTS_DIRECTORY } from "./layout"; + +/** + * Checking a served rule against the signatures it arrived with, before any + * byte of it reaches the disk. + * + * v2 serves every file set with `signatures`: one per file of the rule, except + * its fixtures under `.tests/`, each the algoVersion-1 envelope of that file's + * content. A signature is a record of what was issued, not permission to run + * anything (only a reconcile `run` verdict grants that), so what this checks is + * integrity: that the bytes about to be written are the bytes the service says + * it issued. A set that fails is refused whole, because a rule directory with + * one wrong file reconciles as `unsafe` on the next `check` and fails a run + * two steps from the cause. + * + * The two directions are both required. A file with no signature is bytes + * nothing vouches for; a signature with no file is a rule missing a piece the + * service thinks it has, which is the inert-rule outcome that exits 0. + */ + +/** A served rule that verified, narrowed to its one file set. */ +export type VerifiedDelivery = + | { ok: true; fileSet: ServedFileSet; revisionId: string } + | { ok: false; reason: string }; + +/** What the caller already knows the served rule must be. */ +export interface DeliveryExpectation { + /** The rule id that was asked for. */ + ruleId: string; + /** + * The revision that must be served, when the caller knows it: the one a + * generation request produced, or the one a rollback asked for. + */ + revisionId?: string; +} + +function isFixture(path: string): boolean { + return path.startsWith(`${RULE_TESTS_DIRECTORY}/`); +} + +/** + * Verify a served rule. Never throws for a payload problem; every refusal is a + * reason naming the rule and what was wrong. + */ +export async function verifyServedRule( + served: ServedRule, + expected: DeliveryExpectation +): Promise { + const { ruleId } = expected; + + if (served.ruleId !== ruleId) { + return { + ok: false, + reason: `the service answered for rule ${served.ruleId}, not ${ruleId}`, + }; + } + if ( + expected.revisionId !== undefined && + served.revisionId !== expected.revisionId + ) { + return { + ok: false, + reason: `rule ${ruleId} was served at revision ${served.revisionId}, not ${expected.revisionId}`, + }; + } + if (served.rules.length !== 1) { + return { + ok: false, + reason: `rule ${ruleId} was served as ${String(served.rules.length)} file sets; exactly one is expected`, + }; + } + const fileSet = served.rules[0] as ServedFileSet; + if (fileSet.id !== ruleId) { + return { + ok: false, + reason: `rule ${ruleId} was served with a file set for ${fileSet.id}`, + }; + } + + const signatures = new Map(); + for (const entry of fileSet.signatures) { + if (signatures.has(entry.path)) { + return { + ok: false, + reason: `rule ${ruleId} carries two signatures for ${entry.path}`, + }; + } + signatures.set(entry.path, entry.signature); + } + + const delivered = new Set(fileSet.files.map((file) => file.path)); + for (const path of signatures.keys()) { + if (!delivered.has(path)) { + return { + ok: false, + reason: `rule ${ruleId} carries a signature for ${path} but no such file`, + }; + } + if (isFixture(path)) { + return { + ok: false, + reason: `rule ${ruleId} carries a signature for the fixture ${path}; fixtures are never signed`, + }; + } + } + + for (const file of fileSet.files) { + if (isFixture(file.path)) continue; + const claimed = signatures.get(file.path); + if (claimed === undefined) { + return { + ok: false, + reason: `rule ${ruleId} carries ${file.path} with no signature`, + }; + } + const actual = await canonicalHash(file.content); + if (actual !== claimed) { + return { + ok: false, + reason: `rule ${ruleId} carries ${file.path} whose bytes do not match its signature (claimed ${claimed}, got ${actual})`, + }; + } + } + + if (fileSet.engine === "runtime") { + const checkSignature = signatures.get("check.ts"); + if (checkSignature === undefined || fileSet.signature !== checkSignature) { + return { + ok: false, + reason: `runtime rule ${ruleId}'s signature does not equal its check.ts entry`, + }; + } + } + + return { ok: true, fileSet, revisionId: served.revisionId }; +} diff --git a/packages/cli/src/schemas/rules-create.ts b/packages/cli/src/schemas/rules-create.ts index 54ba9040..abeb91ac 100644 --- a/packages/cli/src/schemas/rules-create.ts +++ b/packages/cli/src/schemas/rules-create.ts @@ -20,8 +20,16 @@ export const inputSchema = z.object({ /** Output schema for `taskless rule create --json` on success */ export const outputSchema = z.object({ success: z.literal(true), - ruleId: z.string().describe("UUID of the generated rule job"), - rules: z.array(z.string()).describe("Rule IDs that were generated"), + requestId: z + .string() + .describe( + "The generation request's id. Not a rule id: nothing takes it back as one" + ), + rules: z + .array(z.string()) + .describe( + "Ids of the rules that were written: each rule's directory name under `.taskless/rules//`, which is what `rule improve`, `rule restore`, and `rule rollback` take" + ), files: z.array(z.string()).describe("File paths that were written"), notices: z .array(z.string()) diff --git a/packages/cli/src/schemas/rules-improve.ts b/packages/cli/src/schemas/rules-improve.ts index c4bb333f..bc62c288 100644 --- a/packages/cli/src/schemas/rules-improve.ts +++ b/packages/cli/src/schemas/rules-improve.ts @@ -6,7 +6,9 @@ export const inputSchema = z.object({ .string() .trim() .min(1, "ruleId must be a non-empty string") - .describe("ID of the rule to improve"), + .describe( + "Id of the rule to improve: its directory name under `.taskless/rules//`, as `rule create --json` lists it in `rules`" + ), guidance: z .string() .trim() @@ -26,8 +28,10 @@ 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 request ID for polling status"), - rules: z.array(z.string()).describe("Rule IDs that were updated"), + requestId: z.string().describe("The iterate request's id"), + rules: z + .array(z.string()) + .describe("Ids of the rules that were written (their directory names)"), files: z.array(z.string()).describe("File paths that were written"), notices: z .array(z.string()) diff --git a/packages/cli/test/api-rule-errors.test.ts b/packages/cli/test/api-rule-errors.test.ts deleted file mode 100644 index 449c450e..00000000 --- a/packages/cli/test/api-rule-errors.test.ts +++ /dev/null @@ -1,59 +0,0 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; - -import { iterateRule } from "../src/api/rules"; -import { CLIError } from "../src/util/cli-error"; - -function stubResponse(status: number, body: unknown): void { - vi.stubGlobal( - "fetch", - vi.fn().mockResolvedValue(Response.json(body, { status })) - ); -} - -/** - * `rule improve` takes a ticket id from the caller (the `ruleId` field of - * `rule create --json`, or from the user), so a 404 on that id is a state the - * caller can act on. The code has to travel on the error, because the command - * otherwise reports every `iterateRule` failure as NETWORK_ERROR — which tells - * an agent to retry an id that will never resolve. - */ -describe("iterateRule error codes", () => { - const originalUrl = process.env.TASKLESS_API_URL; - - beforeEach(() => { - process.env.TASKLESS_API_URL = "https://example.invalid/cli"; - }); - - afterEach(() => { - if (originalUrl === undefined) { - delete process.env.TASKLESS_API_URL; - } else { - process.env.TASKLESS_API_URL = originalUrl; - } - vi.unstubAllGlobals(); - }); - - it("carries RULE_NOT_FOUND on a 404 for the supplied ticket id", async () => { - stubResponse(404, { error: "request_not_found" }); - - const error_ = await iterateRule("token", "missing-id", { - orgId: "org", - guidance: "tighten it", - }).catch((error_: unknown) => error_); - - expect(error_).toBeInstanceOf(CLIError); - expect((error_ as CLIError).code).toBe("RULE_NOT_FOUND"); - }); - - it("leaves an unclassified failure without a code, so it reports as NETWORK_ERROR", async () => { - stubResponse(500, { error: "internal" }); - - const error_ = await iterateRule("token", "some-id", { - orgId: "org", - guidance: "tighten it", - }).catch((error_: unknown) => error_); - - expect(error_).toBeInstanceOf(Error); - expect((error_ as CLIError).code).toBeUndefined(); - }); -}); diff --git a/packages/cli/test/deliver.test.ts b/packages/cli/test/deliver.test.ts index b14c35f3..aa42c001 100644 --- a/packages/cli/test/deliver.test.ts +++ b/packages/cli/test/deliver.test.ts @@ -374,11 +374,10 @@ describe("a delivered set defines what the rule directory contains", () => { expect(existsSync(join(directory, "captures", "logs.yml"))).toBe(true); }); - it("keeps .tests/ fixtures the set does not mention", async () => { - // The stated exception. Nothing under `.tests/` reaches an engine (the dot - // is what makes ast-grep skip it), and this CLI writes timestamped - // fixtures there itself that no delivered set will ever name. Purging them - // would delete a rule's local test history on its first file-set delivery. + it("removes .tests/ fixtures the set does not mention", async () => { + // v2 serves a rule's fixtures in every file set, so a fixture the set does + // not name is stale. Left in place it would make `taskless test` judge + // the rule against cases nobody issued. const directory = await writeComplete(); await mkdir(join(directory, ".tests", "valid"), { recursive: true }); await writeFile( @@ -396,9 +395,9 @@ describe("a delivered set defines what the rule directory contains", () => { expect( existsSync(join(directory, ".tests", "logs-abc12345-1970-test.yml")) - ).toBe(true); + ).toBe(false); expect(existsSync(join(directory, ".tests", "valid", "sample.ts"))).toBe( - true + false ); }); @@ -651,13 +650,9 @@ describe("a delivery that carries no fixtures", () => { expect(warnings[0]).toContain("nothing exercises it"); }); - it("stays silent when the purge preserved fixtures the set never mentioned", async () => { - // The case the delivered-set check alone gets wrong, and it is the ordinary - // one rather than an edge: `.tests/` survives a set that does not name it, - // and `writeRuleTestFile` accumulates local fixtures no later set will - // name by construction. Warning here tells the holder of a rule they have - // tested that nothing proves it, while the proof sits in the directory - // just written. + it("warns when the set carries no fixtures, since the purge removed the old ones", async () => { + // The set is the directory, `.tests/` included, so a set with no + // fixtures leaves a rule with none, and the warning is true of the rule. const testsDirectory = join( ruleDirectory(cwd, "sg", "no-eval-abc12345"), ".tests" @@ -678,12 +673,10 @@ describe("a delivery that carries no fixtures", () => { (message) => warnings.push(message) ); - expect(warnings).toEqual([]); - // And the fixture is still there — the silence is because the purge kept - // it, not because the warning was dropped along with it. + expect(warnings).toHaveLength(1); expect( existsSync(join(testsDirectory, "no-eval-abc12345-20260101-test.yml")) - ).toBe(true); + ).toBe(false); }); it("stays silent when fixtures are present", async () => { diff --git a/packages/cli/test/repair-integration.test.ts b/packages/cli/test/repair-integration.test.ts index 926b4207..fad492c2 100644 --- a/packages/cli/test/repair-integration.test.ts +++ b/packages/cli/test/repair-integration.test.ts @@ -310,7 +310,7 @@ describe("repairing a drifted runtime rule, end to end", () => { } }); - it("leaves the rule directory holding exactly the blessed set, minus fixtures", async () => { + it("leaves the rule directory holding exactly the blessed set", async () => { // #233. Repair runs BECAUSE the directory's trustworthiness is in // question, and only `check.ts` is signed — so a stray capture beside the // rule is never reported by reconcile, was never replaced by the repair, @@ -356,7 +356,11 @@ describe("repairing a drifted runtime rule, end to end", () => { }); expect(existsSync(join(rule, "captures", "stray.yml"))).toBe(false); - expect(existsSync(join(rule, ".tests", "demo-1970-test.yml"))).toBe(true); + // A served set is the whole directory, fixtures included, so a fixture + // it does not name is stale and goes with the rest. + expect(existsSync(join(rule, ".tests", "demo-1970-test.yml"))).toBe( + false + ); // The rule is whole afterwards. A repair that leaves it inert would be // the bug, not a trade-off. await expect(readFile(checkFile, "utf8")).resolves.toBe(BLESSED); diff --git a/packages/cli/test/rule-create-entitlement.test.ts b/packages/cli/test/rule-create-entitlement.test.ts index dcd00cd0..52299def 100644 --- a/packages/cli/test/rule-create-entitlement.test.ts +++ b/packages/cli/test/rule-create-entitlement.test.ts @@ -16,6 +16,12 @@ import { import { runCommand } from "citty"; import { ruleCommand } from "../src/commands/rules"; +import { + REQUEST_ID, + servedBody, + stubV2Server, + type StubRule, +} from "./support/v2-server"; /** * A runtime rule written under a plan without runtime signatures is still @@ -41,19 +47,21 @@ const CAPTURE = [ "", ].join("\n"); -const runtimeRule = { +const runtimeRule: StubRule = { id: "plan-runtime-rule", engine: "runtime", files: [ + { path: ".tests/fail/case.ts", content: "console.log(1);\n" }, { path: "check.ts", content: "export default async () => [];\n" }, { path: "captures/logs.yml", content: CAPTURE }, ], }; -const staticRule = { +const staticRule: StubRule = { id: "plan-static-rule", engine: "sg", files: [ + { path: ".tests/fail/case.ts", content: "foo();\n" }, { path: "plan-static-rule.yml", content: @@ -62,45 +70,20 @@ const staticRule = { ], }; +/** Serve `rule` from a request, with `extra` on its served body. */ +async function stubFetch( + rule: StubRule, + extra: Record = {} +): Promise { + stubV2Server({ + produced: [{ rule, body: await servedBody(rule, "rev-1", extra) }], + }); +} + describe("rule create/improve: a runtime rule the plan will not run", () => { let cwd: string; let logSpy: MockInstance<(...data: unknown[]) => void>; - const requestId = "33333333-3333-3333-3333-333333333333"; - const iterateRequestId = "44444444-4444-4444-4444-444444444444"; - - function stubFetch(status: Record): void { - vi.stubGlobal( - "fetch", - vi.fn((input: string | URL | Request, init?: RequestInit) => { - const url = new URL( - typeof input === "string" - ? input - : input instanceof URL - ? input.href - : input.url - ); - const method = ( - init?.method ?? (input instanceof Request ? input.method : "GET") - ).toUpperCase(); - const { pathname } = url; - if (pathname === "/cli/api/whoami") { - return Response.json({}, { status: 500 }); - } - if (method === "POST" && pathname === "/cli/api/request") { - return Response.json({ requestId }, { status: 200 }); - } - if (method === "POST" && pathname.endsWith("/iterate")) { - return Response.json({ requestId: iterateRequestId }); - } - if (method === "GET" && pathname.startsWith("/cli/api/request/")) { - return Response.json({ status: "generated", ...status }); - } - throw new Error(`unexpected ${method} ${pathname}`); - }) - ); - } - beforeEach(async () => { cwd = await mkdtemp(join(tmpdir(), "taskless-rule-plan-")); await mkdir(join(cwd, ".taskless"), { recursive: true }); @@ -156,8 +139,7 @@ describe("rule create/improve: a runtime rule the plan will not run", () => { } it("rule create writes the rule and warns under --json", async () => { - stubFetch({ - rules: [runtimeRule], + await stubFetch(runtimeRule, { entitlement: { runtimeSignatures: false, upgradeUrl: UPGRADE }, }); await create(); @@ -174,8 +156,7 @@ describe("rule create/improve: a runtime rule the plan will not run", () => { }); it("rule improve warns the same way", async () => { - stubFetch({ - rules: [runtimeRule], + await stubFetch(runtimeRule, { entitlement: { runtimeSignatures: false }, }); const requestFile = join(cwd, "improve.json"); @@ -191,18 +172,134 @@ describe("rule create/improve: a runtime rule the plan will not run", () => { it("an entitled or legacy response does not warn", async () => { for (const entitlement of [undefined, { runtimeSignatures: true }]) { - stubFetch({ rules: [runtimeRule], entitlement }); + await stubFetch( + runtimeRule, + entitlement === undefined ? {} : { entitlement } + ); await create(); expect(planWarnings()).toEqual([]); } }); it("a static rule never warns, whatever the plan", async () => { - stubFetch({ - rules: [staticRule], + await stubFetch(staticRule, { entitlement: { runtimeSignatures: false, upgradeUrl: UPGRADE }, }); await create(); expect(planWarnings()).toEqual([]); }); + + it("rule create --json names the request and the written rules, and no ruleId", async () => { + await stubFetch(staticRule); + await create(); + const envelope = JSON.parse( + String(logSpy.mock.calls.at(-1)?.[0]) + ) as Record; + expect(envelope.requestId).toBe(REQUEST_ID); + expect(envelope.rules).toEqual(["plan-static-rule"]); + expect(envelope).not.toHaveProperty("ruleId"); + }); + + it("replaces the rule directory, fixtures included, and creates nested paths", async () => { + const directory = join(cwd, ".taskless", "rules", "sg", "plan-static-rule"); + await mkdir(join(directory, ".tests", "stale"), { recursive: true }); + await writeFile(join(directory, ".tests", "stale", "old.ts"), "old\n"); + await writeFile(join(directory, "stray.yml"), "id: stray\n"); + + await stubFetch(staticRule); + await create(); + + expect(existsSync(join(directory, ".tests", "fail", "case.ts"))).toBe(true); + expect(existsSync(join(directory, ".tests", "stale", "old.ts"))).toBe( + false + ); + expect(existsSync(join(directory, "stray.yml"))).toBe(false); + }); + + it("refuses a served head whose revision is not the one the request produced", async () => { + stubV2Server({ + produced: [ + { + rule: staticRule, + body: { + ...(await servedBody(staticRule, "rev-2")), + // Polling reports the revision the stub's body names, so pin the + // poll to rev-1 by serving a mismatched body under a rev-1 claim. + revisionId: "rev-2", + }, + }, + ], + }); + // Polling reads `body.revisionId`; make it disagree with what is served. + const fetchMock = globalThis.fetch as unknown as { + getMockImplementation: () => (input: Request) => Promise; + mockImplementation: (f: (input: Request) => Promise) => void; + }; + const original = fetchMock.getMockImplementation(); + fetchMock.mockImplementation(async (input: Request) => { + const url = new URL(input.url); + if (url.pathname.startsWith("/cli/api/v2/request/")) { + return Response.json({ + requestId: REQUEST_ID, + status: "generated", + revisions: [{ ruleId: "plan-static-rule", revisionId: "rev-1" }], + }); + } + return original(input); + }); + + await expect(create()).rejects.toThrow(); + const envelope = JSON.parse(String(logSpy.mock.calls.at(-1)?.[0])) as { + code?: string; + message?: string; + }; + expect(envelope.code).toBe("RULE_GENERATION_FAILED"); + expect(envelope.message).toContain("rev-2"); + expect( + existsSync(join(cwd, ".taskless", "rules", "sg", "plan-static-rule")) + ).toBe(false); + }); + + it("prints a failed request's error as given", async () => { + stubV2Server({ + produced: [], + status: "failed", + error: "REMOTE_GENERATION_NOT_IN_PLAN: \u001B[31mupgrade\u001B[0m", + }); + await expect(create()).rejects.toThrow(); + const envelope = JSON.parse(String(logSpy.mock.calls.at(-1)?.[0])) as { + code?: string; + message?: string; + }; + expect(envelope.code).toBe("RULE_GENERATION_FAILED"); + expect(envelope.message).toContain("REMOTE_GENERATION_NOT_IN_PLAN"); + expect(envelope.message).not.toContain("\u001B"); + }); + + it("rule improve reports an unknown rule id as RULE_NOT_FOUND", async () => { + vi.stubGlobal( + "fetch", + vi.fn((input: Request) => { + const { pathname } = new URL(input.url); + if (pathname === "/cli/api/whoami") { + return Response.json({}, { status: 500 }); + } + return Response.json({ error: "rule_not_found" }, { status: 404 }); + }) + ); + const requestFile = join(cwd, "improve.json"); + await writeFile( + requestFile, + JSON.stringify({ ruleId: "gone-00000000", guidance: "tighten" }) + ); + await expect( + runCommand(ruleCommand, { + rawArgs: ["improve", "--from", requestFile, "--json", "-d", cwd], + }) + ).rejects.toThrow(); + const envelope = JSON.parse(String(logSpy.mock.calls.at(-1)?.[0])) as { + code?: string; + }; + expect(envelope.code).toBe("RULE_NOT_FOUND"); + }); }); diff --git a/packages/cli/test/rule-from.test.ts b/packages/cli/test/rule-from.test.ts index 4c333edf..e4f475ea 100644 --- a/packages/cli/test/rule-from.test.ts +++ b/packages/cli/test/rule-from.test.ts @@ -110,7 +110,12 @@ describe("rules create --from", () => { }); describe("the create and improve --json envelopes carry delivery notices", () => { - const CREATE = { success: true, ruleId: "req-1", rules: ["a"], files: ["f"] }; + const CREATE = { + success: true, + requestId: "req-1", + rules: ["a"], + files: ["f"], + }; const IMPROVE = { success: true, requestId: "req-1", diff --git a/packages/cli/test/rule-guard-json-envelope.test.ts b/packages/cli/test/rule-guard-json-envelope.test.ts index 4c2d3726..c2e9443e 100644 --- a/packages/cli/test/rule-guard-json-envelope.test.ts +++ b/packages/cli/test/rule-guard-json-envelope.test.ts @@ -1,4 +1,5 @@ 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"; @@ -15,87 +16,53 @@ import { import { runCommand } from "citty"; import { ruleCommand } from "../src/commands/rules"; +import { + servedBody, + stubV2Server, + type StubFile, + type StubRule, +} from "./support/v2-server"; /** - * #280: the guard refusing a file-set rule that also carries a stray `tests` - * field threw a bare `CLIError` from inside the command's own `try`, never - * touching `fail()`. Under `--json` that skipped the envelope entirely — - * stdout empty, prose on stderr, exit 1, indistinguishable from a crash. + * #280: a guard refusing a delivered rule from inside the command's own `try` + * threw a bare `CLIError` that never touched `fail()`. Under `--json` that + * skipped the envelope entirely — stdout empty, prose on stderr, exit 1, + * indistinguishable from a crash. * - * These tests drive the ACTUAL command (via citty's own `runCommand`, which - * parses argv exactly like the built CLI does) rather than the guard function - * in isolation, because a unit test proving the function throws correctly - * says nothing about whether the command reports it correctly — that is - * exactly the seam #280 slipped through. + * The guard is now signature verification: a served rule whose bytes do not + * match its signatures is refused before anything is written. These tests + * drive the ACTUAL command (via citty's own `runCommand`, which parses argv + * exactly like the built CLI does), because a unit test proving the verifier + * refuses says nothing about whether the command reports it — that is exactly + * the seam #280 slipped through. */ -describe("rule create/improve --json: file-set rule with a stray `tests` field", () => { +describe("rule create/improve --json: a served rule that fails verification", () => { let cwd: string; let logSpy: MockInstance<(...data: unknown[]) => void>; - const requestId = "11111111-1111-1111-1111-111111111111"; - const iterateRequestId = "22222222-2222-2222-2222-222222222222"; - - // A minimal ast-grep file-set delivery for engine "sg": one file at - // `.yml` (the only file `ENGINE_LAYOUTS.sg` requires), plus the stray - // `tests` field the schema says a file set must never carry. - const badRule = { - id: "guard-test-rule", + const rule: StubRule = { + id: "guard-test-rule-3fa9c21b", engine: "sg", files: [ { - path: "guard-test-rule.yml", + path: "guard-test-rule-3fa9c21b.yml", content: - "id: guard-test-rule\nlanguage: TypeScript\nrule:\n pattern: foo\n", + "id: guard-test-rule-3fa9c21b\nlanguage: TypeScript\nrule:\n pattern: foo\n", }, + { path: ".tests/fail/case.ts", content: "foo();\n" }, ], - tests: { valid: ["const x = 1;"], invalid: ["foo();"] }, }; - function stubFetch(pollRequestId: string): void { - const fetchMock = vi.fn( - (input: string | URL | Request, init?: RequestInit) => { - const url = new URL( - typeof input === "string" - ? input - : input instanceof URL - ? input.href - : input.url - ); - const method = ( - init?.method ?? (input instanceof Request ? input.method : "GET") - ).toUpperCase(); - const { pathname } = url; - - if (pathname === "/cli/api/whoami") { - // Swallowed by fetchWhoami; resolveOrgSubject falls back to the - // token-claim path. Not what this test is about. - return Response.json({}, { status: 500 }); - } - if (method === "POST" && pathname === "/cli/api/request") { - return Response.json({ requestId }, { status: 200 }); - } - if ( - method === "GET" && - pathname === `/cli/api/request/${pollRequestId}` - ) { - return Response.json( - { status: "generated", rules: [badRule] }, - { status: 200 } - ); - } - if ( - method === "POST" && - pathname === "/cli/api/request/guard-test-rule/iterate" - ) { - return Response.json( - { requestId: iterateRequestId }, - { status: 200 } - ); - } - throw new Error(`unexpected ${method} ${pathname}`); - } - ); - vi.stubGlobal("fetch", fetchMock); + /** Serve the rule with its file edited AFTER signing. */ + async function stubTampered(): Promise { + const body = await servedBody(rule, "rev-1"); + const [fileSet] = body.rules as Array<{ files: StubFile[] }>; + fileSet!.files[0] = { + path: "guard-test-rule-3fa9c21b.yml", + content: + "id: guard-test-rule-3fa9c21b\nlanguage: TypeScript\nrule:\n pattern: bar\n", + }; + stubV2Server({ produced: [{ rule, body }] }); } beforeEach(async () => { @@ -158,7 +125,7 @@ describe("rule create/improve --json: file-set rule with a stray `tests` field", } it("rule create --json: reports RULE_GENERATION_FAILED as an envelope on stdout, not a bare throw", async () => { - stubFetch(requestId); + await stubTampered(); const requestFile = join(cwd, "request.json"); await writeFile(requestFile, JSON.stringify({ prompt: "add a rule" })); @@ -172,16 +139,20 @@ describe("rule create/improve --json: file-set rule with a stray `tests` field", const envelope = lastEnvelope(); expect(envelope.ok).toBe(false); expect(envelope.code).toBe("RULE_GENERATION_FAILED"); - expect(envelope.message).toContain("guard-test-rule"); - expect(envelope.message).toContain("tests"); + expect(envelope.message).toContain(rule.id); + expect(envelope.message).toContain("signature"); + // Refused before anything was written. + expect(existsSync(join(cwd, ".taskless", "rules", "sg", rule.id))).toBe( + false + ); }); it("rule improve --json: reports RULE_GENERATION_FAILED as an envelope on stdout, not a bare throw", async () => { - stubFetch(iterateRequestId); + await stubTampered(); const requestFile = join(cwd, "improve-request.json"); await writeFile( requestFile, - JSON.stringify({ ruleId: "guard-test-rule", guidance: "tighten it" }) + JSON.stringify({ ruleId: rule.id, guidance: "tighten it" }) ); const runPromise = runCommand(ruleCommand, { @@ -194,7 +165,11 @@ describe("rule create/improve --json: file-set rule with a stray `tests` field", const envelope = lastEnvelope(); expect(envelope.ok).toBe(false); expect(envelope.code).toBe("RULE_GENERATION_FAILED"); - expect(envelope.message).toContain("guard-test-rule"); - expect(envelope.message).toContain("tests"); + expect(envelope.message).toContain(rule.id); + expect(envelope.message).toContain("signature"); + // Refused before anything was written. + expect(existsSync(join(cwd, ".taskless", "rules", "sg", rule.id))).toBe( + false + ); }); }); diff --git a/packages/cli/test/support/v2-server.ts b/packages/cli/test/support/v2-server.ts new file mode 100644 index 00000000..f6058612 --- /dev/null +++ b/packages/cli/test/support/v2-server.ts @@ -0,0 +1,118 @@ +import { vi } from "vitest"; + +import { canonicalHash } from "../../src/rules/rule-hash"; + +/** + * A stubbed v2 rule API for driving the real `rule create` / `rule improve` + * commands: submit and iterate hand back a request id, polling reports the + * produced `{ ruleId, revisionId }` pairs, and `GET rule/{ruleId}` serves each + * rule's file set. + * + * Served sets are SIGNED here with the CLI's own `canonicalHash`, the way the + * service signs them, so a test that wants a bad signature has to break one + * on purpose rather than inherit a set that was never valid. + */ + +export interface StubFile { + path: string; + content: string; +} + +export interface StubRule { + id: string; + engine: "sg" | "vale" | "runtime"; + files: StubFile[]; +} + +/** A served rule body: one signed file set plus its revision. */ +export async function servedBody( + rule: StubRule, + revisionId: string, + extra: Record = {} +): Promise> { + const signatures = await Promise.all( + rule.files + .filter((file) => !file.path.startsWith(".tests/")) + .map(async (file) => ({ + path: file.path, + signature: await canonicalHash(file.content), + })) + ); + const check = signatures.find((entry) => entry.path === "check.ts"); + return { + ruleId: rule.id, + revisionId, + rules: [ + { + ...rule, + // Copied, so a test that tampers with the served set cannot reach + // back into the rule every later test signs. + files: rule.files.map((file) => ({ ...file })), + signatures, + ...(rule.engine === "runtime" && check !== undefined + ? { signature: check.signature } + : {}), + }, + ], + ...extra, + }; +} + +export interface StubServerOptions { + /** Rules the request produces, each with the body `GET rule/{id}` serves. */ + produced: Array<{ rule: StubRule; body: Record }>; + /** Terminal request status. Defaults to `generated`. */ + status?: string; + /** `error` on the terminal status, for `failed` / `unsupported`. */ + error?: string; +} + +export const REQUEST_ID = "11111111-1111-1111-1111-111111111111"; +export const ITERATE_REQUEST_ID = "22222222-2222-2222-2222-222222222222"; + +/** Install the stub as the global `fetch`. Returns the mock for assertions. */ +export function stubV2Server( + options: StubServerOptions +): ReturnType { + const fetchMock = vi.fn((input: string | URL | Request): Response => { + const request = input instanceof Request ? input : new Request(input); + const url = new URL(request.url); + const { pathname } = url; + const method = request.method.toUpperCase(); + + if (pathname === "/cli/api/whoami") { + // Swallowed by the org lookup, which falls back to the token. + return Response.json({}, { status: 500 }); + } + if (method === "POST" && pathname === "/cli/api/v2/request") { + return Response.json({ requestId: REQUEST_ID, status: "accepted" }); + } + if (method === "POST" && pathname.endsWith("/iterate")) { + return Response.json({ + requestId: ITERATE_REQUEST_ID, + status: "accepted", + }); + } + if (method === "GET" && pathname.startsWith("/cli/api/v2/request/")) { + return Response.json({ + requestId: pathname.split("/").at(-1), + status: options.status ?? "generated", + revisions: options.produced.map(({ rule, body }) => ({ + ruleId: rule.id, + revisionId: body.revisionId, + })), + ...(options.error === undefined ? {} : { error: options.error }), + }); + } + if (method === "GET" && pathname.startsWith("/cli/api/v2/rule/")) { + const ruleId = decodeURIComponent(pathname.split("/").at(-1) ?? ""); + const match = options.produced.find(({ rule }) => rule.id === ruleId); + return match === undefined + ? Response.json({ error: "rule_not_found" }, { status: 404 }) + : Response.json(match.body); + } + throw new Error(`unexpected ${method} ${pathname}`); + }); + vi.stubGlobal("fetch", fetchMock); + return fetchMock; +} diff --git a/packages/cli/test/verify-delivery.test.ts b/packages/cli/test/verify-delivery.test.ts new file mode 100644 index 00000000..cc4d17a9 --- /dev/null +++ b/packages/cli/test/verify-delivery.test.ts @@ -0,0 +1,148 @@ +import { describe, expect, it } from "vitest"; + +import type { ServedRule } from "../src/api/v2"; +import { verifyServedRule } from "../src/rules/verify-delivery"; +import { servedBody, type StubRule } from "./support/v2-server"; + +const SG: StubRule = { + id: "no-eval-3fa9c21b", + engine: "sg", + files: [ + { path: "no-eval-3fa9c21b.yml", content: "id: no-eval-3fa9c21b\n" }, + { path: ".tests/fail/case.ts", content: "eval(x);\n" }, + ], +}; + +const RUNTIME: StubRule = { + id: "no-env-leak-00000000", + engine: "runtime", + files: [ + { path: "check.ts", content: "export default async () => [];\n" }, + { path: "captures/env.yml", content: "id: env\n" }, + ], +}; + +async function served( + rule: StubRule, + mutate: (body: { + ruleId: string; + revisionId: string; + rules: Array<{ + id: string; + files: Array<{ path: string; content: string }>; + signatures: Array<{ path: string; signature: string }>; + signature?: string; + }>; + }) => void = () => {} +): Promise { + const body = (await servedBody(rule, "rev-1")) as Parameters< + typeof mutate + >[0]; + mutate(body); + return body as unknown as ServedRule; +} + +describe("verifyServedRule", () => { + it("accepts a correctly signed set, fixtures unsigned", async () => { + const verdict = await verifyServedRule(await served(SG), { + ruleId: SG.id, + revisionId: "rev-1", + }); + expect(verdict).toMatchObject({ ok: true, revisionId: "rev-1" }); + }); + + it("accepts a runtime set whose signature equals its check.ts entry", async () => { + const verdict = await verifyServedRule(await served(RUNTIME), { + ruleId: RUNTIME.id, + }); + expect(verdict.ok).toBe(true); + }); + + it.each([ + [ + "a file whose bytes do not match its signature", + (body: Parameters[1] & object>[0]) => { + body.rules[0]!.files[0]!.content = "id: tampered\n"; + }, + "do not match its signature", + ], + [ + "a file with no signature", + (body: Parameters[1] & object>[0]) => { + body.rules[0]!.signatures = []; + }, + "with no signature", + ], + [ + "a signature naming no file", + (body: Parameters[1] & object>[0]) => { + body.rules[0]!.signatures.push({ path: "ghost.yml", signature: "x" }); + }, + "no such file", + ], + [ + "a signed fixture", + (body: Parameters[1] & object>[0]) => { + body.rules[0]!.signatures.push({ + path: ".tests/fail/case.ts", + signature: "x", + }); + }, + "fixtures are never signed", + ], + [ + "two signatures for one path", + (body: Parameters[1] & object>[0]) => { + body.rules[0]!.signatures.push({ ...body.rules[0]!.signatures[0]! }); + }, + "two signatures", + ], + [ + "more than one file set", + (body: Parameters[1] & object>[0]) => { + body.rules.push({ ...body.rules[0]! }); + }, + "exactly one is expected", + ], + [ + "a file set for another rule", + (body: Parameters[1] & object>[0]) => { + body.rules[0]!.id = "someone-else-00000000"; + }, + "file set for someone-else", + ], + [ + "an answer for another rule", + (body: Parameters[1] & object>[0]) => { + body.ruleId = "someone-else-00000000"; + }, + "answered for rule someone-else", + ], + ])("refuses %s", async (_, mutate, reason) => { + const verdict = await verifyServedRule(await served(SG, mutate), { + ruleId: SG.id, + }); + expect(verdict.ok).toBe(false); + expect(verdict.ok ? "" : verdict.reason).toContain(reason); + }); + + it("refuses a revision other than the one expected", async () => { + const verdict = await verifyServedRule(await served(SG), { + ruleId: SG.id, + revisionId: "rev-0", + }); + expect(verdict.ok ? "" : verdict.reason).toContain("not rev-0"); + }); + + it("refuses a runtime set whose signature is not its check.ts entry", async () => { + const verdict = await verifyServedRule( + await served(RUNTIME, (body) => { + body.rules[0]!.signature = body.rules[0]!.signatures.find( + (entry) => entry.path === "captures/env.yml" + )!.signature; + }), + { ruleId: RUNTIME.id } + ); + expect(verdict.ok ? "" : verdict.reason).toContain("check.ts entry"); + }); +});