diff --git a/.changeset/generator-payload-alignment.md b/.changeset/generator-payload-alignment.md index bf263856..130d409f 100644 --- a/.changeset/generator-payload-alignment.md +++ b/.changeset/generator-payload-alignment.md @@ -110,3 +110,27 @@ It also closes a case that reached the filesystem. A payload carrying neither `yaml.stringify` an `undefined`, which returns the string `"undefined"` rather than throwing. The rule file was created and its contents were that word. It is now refused before the directory exists. + +A rule the service blessed is now repaired, instead of only reported. + +`check` used to parse reconcile's `unsafe`, `unknown` and `missing` verdicts +and read none of them. `unsafe` (bytes that drifted from what the server +blessed) and `missing` (a rule the server expected and this disk never had) +are now re-fetched from `POST /cli/api/rule/{ruleId}/restore`. `unknown` is +not, because a file the service never issued has nothing to fetch; it gets an +explanation instead, since "on your disk, never issued" is ordinary and read +as an unexplained skip. + +Restored bytes are verified against the signature reconcile ALREADY sent, +not against the one the restore response carries. Checking a response against +itself proves only that the service is internally consistent, which it would +also be if it returned a newer generation of the rule. Restore repairs a rule; +it does not upgrade one, and that is now a property with a test rather than a +promise. + +Nothing repaired runs in the pass that repaired it. Restore rewrites the +working tree and promotes nothing into the current run, so an `unsafe` rule +stays withheld; the next `check` reports the repaired signature and is blessed +through the ordinary path. A repair that cannot happen is a notice, never a +failed `check`, because a rule that was not repaired stays withheld and that +is already the safe state. diff --git a/openspec/changes/generator-payload-alignment/tasks.md b/openspec/changes/generator-payload-alignment/tasks.md index 8936c8b4..e7e26011 100644 --- a/openspec/changes/generator-payload-alignment/tasks.md +++ b/openspec/changes/generator-payload-alignment/tasks.md @@ -9,7 +9,7 @@ green on its own; none depends on a later one to be correct. - [x] 1.2 Make the `delete` not-found message engine-agnostic - [x] 1.3 Correct the seven stale `.taskless//rules/` comments, leaving the two historical ones in the migrations - [x] 1.4 Test deleting a rule filed under each engine, and an id no engine holds -- [ ] 1.5 Correct the same stale layout in `cli-runtime-rule-execution`'s spec text (delta written; lands with this change) +- [x] 1.5 Correct the same stale layout in `cli-runtime-rule-execution`'s spec text (delta written; lands with this change) ## 2. Publish the layout table (slice 2) — unblocks the generator @@ -17,7 +17,7 @@ green on its own; none depends on a later one to be correct. - [x] 2.2 Add the `@taskless/cli/layout` export to `package.json` and the build - [x] 2.3 Extend the chunk-graph build plugin to cover the new entry, so a host-capability import fails the build - [x] 2.4 Test that the built entry imports cleanly with no filesystem or command-tree reachability -- [ ] 2.5 Publish a nightly and send the generator the version and specifier (answers **N1**) +- [x] 2.5 Publish a nightly and send the generator the version and specifier (answers **N1**) ## 3. Loud diagnostics on the runtime path (slice 3) @@ -25,7 +25,7 @@ green on its own; none depends on a later one to be correct. - [x] 3.2 Report each of the five drops: unreadable directory, unparseable YAML, wrong `kind`, missing `language`/`name`, missing `id` - [x] 3.3 Surface them through `verify`, matching the `match`-mode wording already shipped - [x] 3.4 Read `metadata.taskless.version`, refusing what this build does not implement -- [ ] 3.6 Read `RUNTIME_CHECK_PROTOCOL_VERSION` — deferred to slice 5; nothing on disk declares it until the payload carries it +- [x] 3.6 Read `RUNTIME_CHECK_PROTOCOL_VERSION` — deferred to slice 5; nothing on disk declares it until the payload carries it - [x] 3.5 Test that each drop is both refused and explained ## 4. One executable per runtime rule (slice 4) @@ -42,15 +42,22 @@ green on its own; none depends on a later one to be correct. - [x] 5.5 Test delivery for all three engines, including a Vale rule with its `.vale.ini` - [x] 5.6 Test that a traversing path creates no file or directory - [x] 5.7 Covered already by `engine-dispatch.test.ts` — "keeps signatures identical and reports the moved path" proves the signature survives `0004`/`0005` while the reported path follows the move -- [ ] 5.8 Regenerate `src/generated/api.d.ts` once their file-set tier is live -- [ ] 5.9 Tell the generator the release that ships this (answers **N2**) — only once it is on `main` +- [x] 5.8 Regenerate `src/generated/api.d.ts` once their file-set tier is live — the union forced six call sites to narrow, and closed a payload that wrote the string `"undefined"` as a rule +- [x] 5.9 Tell the generator the release that ships this (answers **N2**) — only once it is on `main` ## 6. Re-fetch a withheld rule (slice 6) -- [ ] 6.1 Agree the request shape with the generator (answers **N3**): rule id plus signature, scoped like reconcile -- [ ] 6.2 Re-fetch on `unsafe` / `unknown` instead of only warning -- [ ] 6.3 Verify the returned bytes against the held signature before writing -- [ ] 6.4 Test that a tampered check is repaired, and that re-fetch never returns newer bytes +- [x] 6.1 Agree the request shape with the generator (superseded by **N5**): `POST /cli/api/rule/{ruleId}/restore` with `{ repositoryUrl }`, which scopes the response to the owning org and install +- [x] 6.2 Re-fetch on `unsafe` / **`missing`** instead of only warning. **The task said `unknown` and that was wrong.** Read as a set, the entry shapes settle it: `unsafe` `{file, expected, got}` means the server holds bytes we drifted from, `missing` `{ruleId, file}` means it expected a rule we never reported, and `unknown` `{file}` means we hold a file it never issued — nothing to fetch, which is why that entry alone carries no rule id. `missing` was omitted despite being the only bucket already carrying the id restore is keyed on +- [x] 6.2a Explain `unknown` instead, since "on your disk, never issued by the service" is an ordinary situation (hand-written, or another org or install) that read as an unexplained skip +- [x] 6.3 Verify the returned bytes against the held signature before writing — against `unsafe.expected`, the signature reconcile ALREADY sent, not the one the restore response carries. Checking the response against itself proves only internal consistency, which a newer generation would also satisfy +- [x] 6.4 Test that a tampered check is repaired, and that re-fetch never returns newer bytes — the second is a real assertion rather than a relayed promise, because 6.3 verifies against `expected` + +## 6b. What slice 6 does NOT do + +- [x] 6.5 Nothing repaired runs in the pass that repaired it. Restore rewrites the working tree and promotes nothing into the current run: an `unsafe` rule stays withheld, a `missing` rule was never a local candidate, and an `unknown` rule never runs. Fetching code and executing it in the same pass that discovered the drift would move the gate +- [x] 6.6 A repair that fails is a notice, never a failed `check`. A rule that could not be repaired stays withheld, which is already the safe state +- [ ] 6.7 Ask the generator for `ruleId` on a reconcile `unsafe` entry (**N6**). Until then the id is parsed out of `.taskless/rules/runtime//check.ts`, which works and makes repair depend on a layout that has already moved twice — silently, since a wrong id is a 404 and an unrepaired rule rather than an error ## 7. Close out diff --git a/packages/cli/src/api/restore.ts b/packages/cli/src/api/restore.ts new file mode 100644 index 00000000..5eb7adfa --- /dev/null +++ b/packages/cli/src/api/restore.ts @@ -0,0 +1,92 @@ +import type { paths } from "../generated/api"; +import { getApiBaseUrl } from "./config"; +import { CLI_VERSION, CLI_VERSION_HEADER } from "../version"; + +/** + * Fetch the blessed bytes for a rule the client holds wrongly, or not at all. + * + * This is the repair path for reconcile's verdicts, and it is deliberately not + * an upgrade path. `unsafe` means the server holds bytes we have drifted from; + * `missing` means it expected a rule we never reported. Both are answered by + * "send me what you blessed". `unknown` is not: a file the service never issued + * has nothing to fetch, which is why its entry carries no rule id. + * + * NOTHING RESTORED RUNS IN THE PASS THAT RESTORED IT. Restore rewrites the + * working tree and promotes nothing into the current run: an `unsafe` rule + * stays unexecuted, a `missing` rule was never a local candidate, and an + * `unknown` rule never runs. The next `check` reports the repaired signature + * and is blessed through the ordinary path. Fetching bytes and executing them + * in the same breath as discovering drift would move the gate, and the gate is + * the only reason any of this exists. + */ + +type RestoreResponse = + paths["/cli/api/rule/{ruleId}/restore"]["post"]["responses"]["200"]["content"]["application/json"]; + +/** A rule as the service restored it, discriminated on `engine`. */ +export type RestoredRule = NonNullable[number]; + +/** + * The result of an attempted restore. + * + * Mirrors `ReconcileOutcome`: expected conditions are values rather than + * thrown errors, because a repair that cannot happen must degrade `check` to + * "this rule was not repaired and did not run" rather than failing the run. A + * rule the service will not return is a rule that stays withheld, which is + * already a safe state. + */ +export type RestoreOutcome = + | { status: "ok"; rules: RestoredRule[] } + | { status: "unauthorized" } + | { status: "unavailable"; reason: string }; + +/** + * Ask the service for a rule's complete file set. + * + * A `POST` carrying `repositoryUrl`, which is what scopes the response to the + * organization and installation that owns the rule rather than to whoever holds + * a rule id. The verb follows that requirement rather than the other way round. + */ +export async function restoreRule( + token: string, + request: { ruleId: string; repositoryUrl: string } +): Promise { + // Schema paths include the /cli/ prefix, so the base URL is the origin. + const baseUrl = getApiBaseUrl().replace(/\/cli\/?$/, ""); + const url = `${baseUrl}/cli/api/rule/${encodeURIComponent(request.ruleId)}/restore`; + + let response: Response; + try { + response = await fetch(url, { + method: "POST", + headers: { + Authorization: `Bearer ${token}`, + "Content-Type": "application/json", + [CLI_VERSION_HEADER]: CLI_VERSION, + }, + body: JSON.stringify({ repositoryUrl: request.repositoryUrl }), + }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return { status: "unavailable", reason: `network error: ${message}` }; + } + + if (response.status === 401) return { status: "unauthorized" }; + if (!response.ok) { + return { status: "unavailable", reason: `HTTP ${String(response.status)}` }; + } + + let body: unknown; + try { + body = await response.json(); + } catch { + return { status: "unavailable", reason: "invalid response body" }; + } + + const data = body as Partial; + const rules = data.rules; + if (!Array.isArray(rules)) { + return { status: "unavailable", reason: "response carried no `rules`" }; + } + return { status: "ok", rules }; +} diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 1fb2d0a3..8824423b 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -16,6 +16,10 @@ import { resolveOrgSubject } from "../auth/org"; import { resolveRepositoryUrl } from "../util/git-remote"; import { getCliPrefix } from "../util/package-manager"; import { reconcile } from "../api/reconcile"; +import type { ReconcileResponse } from "../api/reconcile"; +import { restoreRule } from "../api/restore"; +import { repairTargets, verifyRestoredCheck } from "../rules/runtime/repair"; +import { writeRuleFile } from "../rules/files"; import { discoverRuntimeRules, type RuntimeRule, @@ -194,6 +198,16 @@ async function planRuntime( `runtime rules could not be materialized (${message})` ); } + // Repair the working tree from the server's verdicts. This changes what the + // NEXT run sees and nothing about this one: an `unsafe` rule stays withheld + // below whether or not its bytes were just restored. Fetching code and + // executing it in the same pass that discovered the drift would move the + // gate, and the gate is the point. + const repair = await repairWithheldRules(cwd, token, { + repositoryUrl, + result: outcome.result, + }); + return { execute, skipped: [ @@ -203,10 +217,114 @@ async function planRuntime( reason: "not blessed by the server (unsafe / unknown / drift)", })), ], - notices: [], + notices: repair.notices, }; } +/** + * Act on the verdicts `check` used to parse and discard. + * + * `unsafe` and `missing` are repairable and are fetched; `unknown` is not, and + * gets an explanation instead. Every outcome here is a NOTICE rather than a + * failure: a rule that could not be repaired is a rule that stays withheld, + * which is already the safe state. A repair failing must never be the reason a + * `check` fails. + */ +async function repairWithheldRules( + cwd: string, + token: string, + input: { repositoryUrl: string; result: ReconcileResponse } +): Promise<{ notices: string[] }> { + const notices: string[] = []; + + // A file this disk holds that the service never issued. There is nothing to + // fetch, and saying so is the whole job: it reads as an unexplained skip + // otherwise, and the causes are ordinary (hand-written, or belonging to + // another organization or installation). + for (const entry of input.result.unknown) { + notices.push( + `${entry.file} was not issued by the rule service, so it cannot be ` + + `restored and will not run. It was written by hand, or belongs to a ` + + `different organization or installation.` + ); + } + + const { targets, unidentifiable } = repairTargets(input.result); + for (const entry of unidentifiable) { + notices.push( + `${entry.file} drifted from the blessed rule, and its rule id could not ` + + `be read from its path, so it was not restored.` + ); + } + + // Fetched concurrently: each target is a different rule id under the same + // token and repository, so they do not order against each other, and a repo + // with several drifted rules would otherwise pay one round trip per rule on + // every `check` until they reconverge. The WRITES stay sequential below, + // because two rules can share a directory prefix and a half-applied set is + // the state this whole path exists to avoid. + const fetched = await Promise.all( + targets.map(async (target) => ({ + target, + outcome: await restoreRule(token, { + ruleId: target.ruleId, + repositoryUrl: input.repositoryUrl, + }), + })) + ); + + for (const { target, outcome } of fetched) { + if (outcome.status !== "ok") { + notices.push( + `${target.file} could not be restored (${ + outcome.status === "unauthorized" + ? "authentication was rejected" + : outcome.reason + }).` + ); + continue; + } + + const rule = outcome.rules.find( + (candidate) => candidate.id === target.ruleId + ); + if (rule === undefined) { + notices.push( + `${target.file} could not be restored: the service returned no rule ` + + `called ${target.ruleId}.` + ); + continue; + } + + const verdict = await verifyRestoredCheck(target, rule); + if (!verdict.ok) { + notices.push(`${target.file} was not restored: ${verdict.reason}.`); + continue; + } + + try { + await writeRuleFile(cwd, rule); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + notices.push(`${target.file} could not be written (${message}).`); + continue; + } + // Says what was written, not what the directory now contains. The + // delivered set is written over whatever is there; it does not remove a + // file the set does not mention, so a stray capture left beside the rule + // survives the repair. Claiming the rule "was restored" would overstate + // that, and only `check.ts` is signed, so nothing here can vouch for the + // rest of the directory. Tracked as #233. + notices.push( + `${target.file} was rewritten with the bytes the service blessed. It ` + + `does not run in this pass; the next \`check\` reports the repaired ` + + `signature and is blessed through the ordinary path.` + ); + } + + return { notices }; +} + /** Parse `--timeout ` into milliseconds; invalid/absent → undefined (default). */ function parseTimeoutMs(raw: string | undefined): number | undefined { if (raw === undefined) return undefined; @@ -399,6 +517,7 @@ export const checkCommand = defineCommand({ const results = dispatched.results; for (const notice of dispatched.notices) warn(`Notice: ${notice}`); + const runNotices = [...plan.notices, ...dispatched.notices]; for (const failure of dispatched.failures) warn(`Error: ${failure}`); let errorCount = 0; @@ -423,9 +542,12 @@ export const checkCommand = defineCommand({ ...(dispatched.failures.length > 0 ? { failures: dispatched.failures } : {}), - ...(dispatched.notices.length > 0 - ? { notices: dispatched.notices } - : {}), + // BOTH sources. `plan.notices` carries the repair diagnostics — + // what was restored, what could not be, and why — and they used to + // reach only `warn()`, which is a no-op under `--json`. So the one + // channel a CI run reads dropped the entire output of the feature + // whose whole purpose is explaining a rule that did not run. + ...(runNotices.length > 0 ? { notices: runNotices } : {}), }); console.log(JSON.stringify(output)); } else { diff --git a/packages/cli/src/commands/rules.ts b/packages/cli/src/commands/rules.ts index 0b4f2852..152a061c 100644 --- a/packages/cli/src/commands/rules.ts +++ b/packages/cli/src/commands/rules.ts @@ -246,6 +246,23 @@ const createCommand = defineCommand({ // A file set carries its fixtures as ordinary files under // `.tests/`, already written by `writeRuleFile`. Only the // single-content envelope has a separate `tests` 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 here 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. + if ( + !isSingleContentRule(rule) && + (rule as { tests?: unknown }).tests !== undefined + ) { + throw new CLIError( + `Rule "${rule.id}" was delivered as a file set and also carries \`tests\`; ` + + `a file set's fixtures belong in its own \`.tests/\` files.`, + "RULE_GENERATION_FAILED" + ); + } if (isSingleContentRule(rule) && rule.tests) { const testFile = await writeRuleTestFile(cwd, rule, timestamp); writtenFiles.push(testFile); @@ -486,6 +503,23 @@ const improveCommand = defineCommand({ // A file set carries its fixtures as ordinary files under // `.tests/`, already written by `writeRuleFile`. Only the // single-content envelope has a separate `tests` 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 here 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. + if ( + !isSingleContentRule(rule) && + (rule as { tests?: unknown }).tests !== undefined + ) { + throw new CLIError( + `Rule "${rule.id}" was delivered as a file set and also carries \`tests\`; ` + + `a file set's fixtures belong in its own \`.tests/\` files.`, + "RULE_GENERATION_FAILED" + ); + } if (isSingleContentRule(rule) && rule.tests) { const testFile = await writeRuleTestFile(cwd, rule, timestamp); writtenFiles.push(testFile); diff --git a/packages/cli/src/rules/files.ts b/packages/cli/src/rules/files.ts index 38cbcdd1..558090ea 100644 --- a/packages/cli/src/rules/files.ts +++ b/packages/cli/src/rules/files.ts @@ -68,15 +68,24 @@ export async function writeRuleFile( return ruleFilePath(cwd, engine, rule.id); } - // `files` is absent past the branch above, so this must be the single-content - // envelope. Checked BEFORE anything is created, and checked on `content` - // itself rather than on "not a file set": a payload carrying neither is not a - // file set either, so the negative would admit it. It used to, and `yaml` - // renders `undefined` as the STRING "undefined" instead of throwing, so the - // rule file was written and what it contained was that word. - if (!isSingleContentRule(rule) || rule.content === undefined) { + // `files` is absent past the branch above, so this is the single-content + // envelope, and what remains is whether its `content` can actually be + // written. Checked BEFORE anything is created. + // + // The test is "a usable object", not "not undefined". `yaml` does not throw + // on a value it cannot make a document of, it renders one: `undefined` + // becomes the string "undefined" and `null` becomes the string "null", so + // either way the rule file is created and its entire contents are that word + // — a malformed rule discovered two steps from the cause. + // + // Note this deliberately asks a different question from the mutual-exclusion + // check above, which treats ANY present `content` (including `null`) as the + // service having sent both envelopes. That one is about what the payload + // claims; this one is about what can be written. Reusing a single predicate + // for both would make one of them wrong. + if (!isUsableContent(rule)) { throw new Error( - `Rule "${rule.id}" carries neither \`files\` nor \`content\`.` + `Rule "${rule.id}" carries no usable \`content\`, and no \`files\`.` ); } await ensureTasklessDirectory(cwd); @@ -86,6 +95,20 @@ export async function writeRuleFile( return filePath; } +/** + * Whether a rule's `content` is something a rule file can be written from. + * + * A rule body is a YAML mapping. `undefined`, `null` and any primitive are all + * values `yaml` will happily render as a scalar document, which is how a rule + * file containing nothing but `null` reaches a developer's disk. + */ +function isUsableContent( + rule: GeneratedRule +): rule is GeneratedRule & { content: Record } { + const content = (rule as { content?: unknown }).content; + return typeof content === "object" && content !== null; +} + /** * Write a rule's test cases inside its own rule directory — * `.taskless/rules/sg/{kebab-id}/.tests/{kebab-id}-{timestamp}-test.yml`. diff --git a/packages/cli/src/rules/runtime/repair.ts b/packages/cli/src/rules/runtime/repair.ts new file mode 100644 index 00000000..a287546a --- /dev/null +++ b/packages/cli/src/rules/runtime/repair.ts @@ -0,0 +1,177 @@ +import { canonicalHash } from "../rule-hash"; +import { RULES_DIRECTORY } from "../layout"; +import type { RestoredRule } from "../../api/restore"; +import type { MissingEntry, UnsafeEntry } from "../../api/reconcile"; + +/** The tree every reported `check.ts` lives under. */ +const TASKLESS_DIRECTORY = ".taskless"; + +/** + * Deciding what a reconcile verdict can repair, and proving a repair is one. + * + * Reconcile returns three verdicts and only two of them are repairable, which + * the entry shapes already say if you read them as a set: + * + * - `unsafe` `{ file, expected, got }` — we hold bytes that drifted from what + * the server blessed. Repairable: the server has the real ones. + * - `missing` `{ ruleId, file }` — the server expected a rule we never + * reported. Repairable, and the only entry that already carries the id + * restore is keyed on. + * - `unknown` `{ file }` — we reported a file the service never issued. NOT + * repairable, and correctly so: there is nothing on the server to fetch, + * which is exactly why the entry carries no rule id. What it needs is an + * explanation, not a request. + * + * NOTHING REPAIRED RUNS IN THE PASS THAT REPAIRED IT. These functions rewrite + * the working tree and promote nothing into the current run. + */ + +/** The signature envelope this repair must reproduce, and the rule to ask for. */ +export interface RepairTarget { + ruleId: string; + file: string; + /** + * The signature the SERVER already told us it blessed. + * + * `undefined` for a `missing` rule, which we do not hold and therefore have + * no prior expectation for. Present for `unsafe`, and that is the case worth + * being strict about — see {@link verifyRestoredCheck}. + */ + expected: string | undefined; +} + +/** + * The rule id inside a reported `check.ts` path. + * + * `.taskless/rules/runtime//check.ts`, matched from the RIGHT so a repo + * nested under a similarly-named directory cannot shift the segment. + * + * This exists only because a reconcile `unsafe` entry does not carry `ruleId` + * (asked for as N6). It is the weak link in the repair path: the layout has + * moved twice already, and a move breaks this silently, since a wrong id + * produces a 404 and a rule that stays unrepaired rather than an error anyone + * sees. Delete it the moment the entry carries the id. + */ +export function ruleIdFromCheckPath(file: string): string | undefined { + const segments = file.split("/"); + const checkIndex = segments.lastIndexOf("check.ts"); + if (checkIndex < 1) return undefined; + const id = segments[checkIndex - 1]; + // The three segments above the id must be `.taskless// + // runtime`, or this is some other `check.ts` and guessing at it would request + // a rule the service never issued. `rules/runtime/` alone is not enough: + // plenty of repositories have one, and the id parsed out of it would be a + // 404 that reads as "the service lost your rule". + if ( + segments[checkIndex - 2] !== "runtime" || + segments[checkIndex - 3] !== RULES_DIRECTORY || + segments[checkIndex - 4] !== TASKLESS_DIRECTORY + ) { + return undefined; + } + return id === undefined || id === "" ? undefined : id; +} + +/** Repair targets for the buckets that have something to fetch. */ +export function repairTargets(buckets: { + unsafe: UnsafeEntry[]; + missing: MissingEntry[]; +}): { targets: RepairTarget[]; unidentifiable: UnsafeEntry[] } { + const targets: RepairTarget[] = []; + const unidentifiable: UnsafeEntry[] = []; + + for (const entry of buckets.unsafe) { + const ruleId = ruleIdFromCheckPath(entry.file); + if (ruleId === undefined) { + unidentifiable.push(entry); + continue; + } + targets.push({ ruleId, file: entry.file, expected: entry.expected }); + } + for (const entry of buckets.missing) { + // Already carries the id, so no path parsing and nothing to fail at. + targets.push({ + ruleId: entry.ruleId, + file: entry.file, + expected: undefined, + }); + } + return { targets, unidentifiable }; +} + +/** Why a restored rule was refused, in words a `check` reader can act on. */ +export type RepairVerdict = + | { ok: true; check: string } + | { ok: false; reason: string }; + +/** The `check.ts` entry of a restored file set, if it carries exactly one. */ +function restoredCheck(rule: RestoredRule): string | undefined { + // `rule.files` directly: every variant of the union declares it, so a cast + // here would re-declare a shape the generated types already know and absorb + // a future schema change instead of failing the build. + const matches = rule.files.filter((file) => file.path === "check.ts"); + return matches.length === 1 ? matches[0]?.content : undefined; +} + +/** + * Whether a restored rule is the one we asked for, and the one we were owed. + * + * THE SIGNATURE CHECKED IS THE ONE RECONCILE ALREADY SENT, not the one the + * restore response carries. That distinction is the whole guarantee. Verifying + * the response against its own `signature` field proves only that the service + * is internally consistent, which it would also be if it handed back a NEWER + * generation of the rule. That would be an upgrade wearing a repair's clothes, + * arriving mid-`check`, having never been reviewed by anyone here. + * + * With `expected` in hand there is exactly one acceptable answer, and "the + * service sent something newer" is refused by the same comparison that catches + * a corrupted transfer. + * + * A `missing` rule has no prior expectation, so it falls back to the + * response's own signature. That is a genuinely weaker check and it is the + * best available: we are not repairing a rule we hold, we are fetching one we + * do not have, and there is nothing local to disagree with. + */ +export async function verifyRestoredCheck( + target: RepairTarget, + rule: RestoredRule +): Promise { + const check = restoredCheck(rule); + if (check === undefined) { + return { + ok: false, + reason: "the restored rule carried no single `check.ts`", + }; + } + + // Read through the union rather than cast: `signature` is optional on the + // `sg`/`vale` variants and required on `runtime`, which is the distinction + // this function exists to enforce, so it must come from the types. + const claimed = rule.signature; + if (typeof claimed !== "string" || claimed === "") { + // The published schema requires this on a runtime rule, so reaching here + // means the service broke its own contract. Refuse rather than write bytes + // nothing vouches for. + return { + ok: false, + reason: "the restored runtime rule carried no signature", + }; + } + + const actual = await canonicalHash(check); + if (actual !== claimed) { + return { + ok: false, + reason: `the restored bytes do not match the signature the service sent with them (claimed ${claimed}, got ${actual})`, + }; + } + + if (target.expected !== undefined && actual !== target.expected) { + return { + ok: false, + reason: `the restored bytes are not the ones reconcile blessed — expected ${target.expected}, got ${actual}. Restore repairs a rule, it does not upgrade one`, + }; + } + + return { ok: true, check }; +} diff --git a/packages/cli/test/deliver.test.ts b/packages/cli/test/deliver.test.ts index 11f36a5f..a3c360c1 100644 --- a/packages/cli/test/deliver.test.ts +++ b/packages/cli/test/deliver.test.ts @@ -250,6 +250,25 @@ describe("delivering a rule as a file set", () => { ); }); + it.each([ + ["content is null", { id: "no-eval-abc12345", content: null }], + ["content is a string", { id: "no-eval-abc12345", content: "rule: {}" }], + ["content is a number", { id: "no-eval-abc12345", content: 0 }], + ])("refuses a payload where %s", async (_label, rule) => { + // `yaml` renders every one of these as a scalar document rather than + // throwing, so without this the rule file is created and its entire + // contents are `null`, `rule: {}` or `0`. The mutual-exclusion check above + // treats a present-but-null `content` as "the service sent both", which is + // the right answer to a different question; this asks whether the value can + // be written at all. + await expect( + writeRuleFile(cwd, rule as unknown as GeneratedRule) + ).rejects.toThrow(/no usable `content`/); + expect(existsSync(ruleDirectory(cwd, "sg", "no-eval-abc12345"))).toBe( + false + ); + }); + it("refuses a payload carrying neither files nor content", async () => { // Before the published union forced the variants apart, this fell through // to the single-content branch and handed `stringify` an `undefined`, @@ -257,7 +276,7 @@ describe("delivering a rule as a file set", () => { // was written, and what it contained was the word undefined. const rule = { id: "no-eval-abc12345" } as unknown as GeneratedRule; await expect(writeRuleFile(cwd, rule)).rejects.toThrow( - /neither `files` nor `content`/ + /no usable `content`/ ); expect(existsSync(ruleDirectory(cwd, "sg", "no-eval-abc12345"))).toBe( false diff --git a/packages/cli/test/repair-integration.test.ts b/packages/cli/test/repair-integration.test.ts new file mode 100644 index 00000000..66277919 --- /dev/null +++ b/packages/cli/test/repair-integration.test.ts @@ -0,0 +1,283 @@ +import { execFile } from "node:child_process"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { createServer, type Server } from "node:http"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { promisify } from "node:util"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { canonicalHash } from "../src/rules/rule-hash"; + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); + +/** + * The repair path end to end: reconcile reports drift, restore answers, the + * file is rewritten, and the run says so. + * + * `repair.test.ts` covers the decisions as pure functions. What it cannot see + * is the wiring, and the wiring is where this feature failed review: every + * repair notice reached `warn()` only, which is a no-op under `--json`, so the + * one channel a CI run reads dropped the entire output of the thing whose + * purpose is explaining a rule that did not run. A test at this level is what + * would have caught it, so it is the first assertion below. + */ + +/** A mock serving both endpoints the repair path uses. */ +interface Mock { + apiUrl: string; + restoreCalls: string[]; + close: () => Promise; +} + +function startMock(handlers: { + reconcile: (body: { + files: { file: string; signature: string }[]; + }) => unknown; + restore: (ruleId: string) => { statusCode: number; body: unknown }; +}): Promise { + const restoreCalls: string[] = []; + const server: Server = createServer((request, response) => { + let raw = ""; + request.on("data", (chunk: Buffer) => (raw += chunk.toString())); + request.on("end", () => { + const url = request.url ?? ""; + if (url === "/cli/api/reconcile") { + const body = handlers.reconcile( + JSON.parse(raw) as { files: { file: string; signature: string }[] } + ); + response.writeHead(200, { "content-type": "application/json" }); + response.end(JSON.stringify(body)); + return; + } + const restore = /^\/cli\/api\/rule\/([^/]+)\/restore$/.exec(url); + if (restore) { + const ruleId = decodeURIComponent(restore[1] ?? ""); + restoreCalls.push(ruleId); + const { statusCode, body } = handlers.restore(ruleId); + response.writeHead(statusCode, { "content-type": "application/json" }); + response.end(JSON.stringify(body)); + return; + } + response.writeHead(404).end("{}"); + }); + }); + return new Promise((resolvePromise) => { + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address ? address.port : 0; + resolvePromise({ + apiUrl: `http://127.0.0.1:${String(port)}/cli`, + restoreCalls, + close: () => new Promise((done) => server.close(() => done())), + }); + }); + }); +} + +async function runCli( + args: string[], + env: Record +): Promise<{ stdout: string; exitCode: number }> { + try { + const { stdout } = await execFileAsync("node", [binPath, ...args], { + env: { ...process.env, ...env }, + }); + return { stdout, exitCode: 0 }; + } catch (error) { + const failure = error as { stdout: string; code: number }; + return { stdout: failure.stdout ?? "", exitCode: failure.code }; + } +} + +/** The `--json` envelope, ignoring any migration chatter before it. */ +function envelope(stdout: string): { notices?: string[] } { + const line = stdout + .trim() + .split("\n") + .findLast((l) => l.trim().startsWith("{")); + return JSON.parse(line ?? "{}") as { notices?: string[] }; +} + +const CAPTURE = [ + "id: logs-abc12345", + "language: typescript", + "rule:", + " pattern: console.log($A)", + "metadata:", + " taskless:", + " version: 1", + " kind: runtime", + " name: logs", + " check: check.ts", + " match: anchor", + "", +].join("\n"); + +const DRIFTED = "export default async () => [];\n"; +const BLESSED = "export default async function () {\n return [];\n}\n"; +const REPORTED = ".taskless/rules/runtime/demo/check.ts"; + +/** Reconcile reporting the reported check as drifted from `expected`. */ +const driftedReconcile = (expected: string) => () => ({ + run: [], + unsafe: [{ file: REPORTED, expected, got: "1;h=sha-256;d=stale" }], + unknown: [], + missing: [], +}); + +describe("repairing a drifted runtime rule, end to end", () => { + let directory: string; + let checkFile: string; + + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), "tskl-repair-")); + const rule = join(directory, ".taskless", "rules", "runtime", "demo"); + await mkdir(join(rule, "captures"), { recursive: true }); + await writeFile(join(rule, "captures", "logs.yml"), CAPTURE, "utf8"); + checkFile = join(rule, "check.ts"); + await writeFile(checkFile, DRIFTED, "utf8"); + await writeFile(join(directory, "src.ts"), 'console.log("hi");\n', "utf8"); + await execFileAsync("git", ["init"], { cwd: directory }); + await execFileAsync( + "git", + ["remote", "add", "origin", "https://github.com/acme/widgets.git"], + { cwd: directory } + ); + }); + + afterEach(async () => { + await rm(directory, { recursive: true, force: true }); + }); + + it("reports the repair on the --json envelope, not only on stderr", async () => { + // The regression this test exists for. `--json` is what CI reads, and the + // repair notices used to reach `warn()` alone. + const blessed = await canonicalHash(BLESSED); + const mock = await startMock({ + reconcile: driftedReconcile(blessed), + restore: () => ({ + statusCode: 200, + body: { + ruleId: "demo", + rules: [ + { + id: "demo", + engine: "runtime", + // The COMPLETE set. The schema calls `files` "every file the + // rule directory must contain", and `writeRuleFile` enforces + // that: a runtime rule restored without its captures would be + // written, verify as incomplete, and never fire. + files: [ + { path: "check.ts", content: BLESSED }, + { path: "captures/logs.yml", content: CAPTURE }, + ], + signature: blessed, + }, + ], + }, + }), + }); + try { + const { stdout } = await runCli(["check", "-d", directory, "--json"], { + TASKLESS_TOKEN: "fake.token", + TASKLESS_API_URL: mock.apiUrl, + }); + + const notices = envelope(stdout).notices ?? []; + expect(notices.join("\n")).toContain(REPORTED); + expect(notices.join("\n")).toMatch(/blessed/); + expect(mock.restoreCalls).toEqual(["demo"]); + + // And the bytes actually landed. + await expect(readFile(checkFile, "utf8")).resolves.toBe(BLESSED); + } finally { + await mock.close(); + } + }); + + it("refuses bytes that are not the ones reconcile blessed, and says so", async () => { + // The service answers consistently — it signs exactly what it sends — and + // sends a NEWER rule than the one we were owed. Nothing is written. + const owed = await canonicalHash(BLESSED); + const newer = "export default async () => [{ file: 'x' }];\n"; + const newerSignature = await canonicalHash(newer); + const mock = await startMock({ + reconcile: driftedReconcile(owed), + restore: () => ({ + statusCode: 200, + body: { + ruleId: "demo", + rules: [ + { + id: "demo", + engine: "runtime", + files: [ + { path: "check.ts", content: newer }, + { path: "captures/logs.yml", content: CAPTURE }, + ], + signature: newerSignature, + }, + ], + }, + }), + }); + try { + const { stdout } = await runCli(["check", "-d", directory, "--json"], { + TASKLESS_TOKEN: "fake.token", + TASKLESS_API_URL: mock.apiUrl, + }); + const notices = (envelope(stdout).notices ?? []).join("\n"); + expect(notices).toMatch(/was not restored/); + expect(notices).toMatch(/does not upgrade/); + await expect(readFile(checkFile, "utf8")).resolves.toBe(DRIFTED); + } finally { + await mock.close(); + } + }); + + it("explains a rule the service will not return", async () => { + const mock = await startMock({ + reconcile: driftedReconcile(await canonicalHash(BLESSED)), + restore: () => ({ statusCode: 404, body: {} }), + }); + try { + const { stdout } = await runCli(["check", "-d", directory, "--json"], { + TASKLESS_TOKEN: "fake.token", + TASKLESS_API_URL: mock.apiUrl, + }); + const notices = (envelope(stdout).notices ?? []).join("\n"); + expect(notices).toMatch(/could not be restored/); + // A repair that cannot happen is a notice, never a failed run: the rule + // stays withheld, which is already the safe state. + await expect(readFile(checkFile, "utf8")).resolves.toBe(DRIFTED); + } finally { + await mock.close(); + } + }); + + it("says why an unknown file cannot be restored, and asks for nothing", async () => { + const mock = await startMock({ + reconcile: () => ({ + run: [], + unsafe: [], + unknown: [{ file: REPORTED }], + missing: [], + }), + restore: () => ({ statusCode: 500, body: {} }), + }); + try { + const { stdout } = await runCli(["check", "-d", directory, "--json"], { + TASKLESS_TOKEN: "fake.token", + TASKLESS_API_URL: mock.apiUrl, + }); + const notices = (envelope(stdout).notices ?? []).join("\n"); + expect(notices).toMatch(/was not issued by the rule service/); + // Nothing on the server to ask for, so nothing is asked. + expect(mock.restoreCalls).toEqual([]); + } finally { + await mock.close(); + } + }); +}); diff --git a/packages/cli/test/repair.test.ts b/packages/cli/test/repair.test.ts new file mode 100644 index 00000000..aa75c89a --- /dev/null +++ b/packages/cli/test/repair.test.ts @@ -0,0 +1,201 @@ +import { describe, expect, it } from "vitest"; + +import { canonicalHash } from "../src/rules/rule-hash"; +import { + repairTargets, + ruleIdFromCheckPath, + verifyRestoredCheck, + type RepairTarget, +} from "../src/rules/runtime/repair"; +import type { RestoredRule } from "../src/api/restore"; + +const CHECK = "export default async () => [];\n"; +const TAMPERED = "export default async () => [{ file: 'x' }];\n"; + +/** A restored runtime rule carrying `check.ts` and a signature over it. */ +async function restored( + content: string, + signature?: string +): Promise { + return { + id: "logs-abc12345", + engine: "runtime", + files: [{ path: "check.ts", content }], + signature: signature ?? (await canonicalHash(content)), + } as unknown as RestoredRule; +} + +describe("which reconcile verdicts can be repaired", () => { + it("routes unsafe and missing, and never unknown", () => { + // `unknown` is not in the input type at all, and that is the point: there + // is nothing on the server to fetch for a file it never issued, which is + // why its entry carries no rule id to fetch it by. + const { targets } = repairTargets({ + unsafe: [ + { + file: ".taskless/rules/runtime/logs-abc12345/check.ts", + expected: "1;h=sha-256;d=aaa", + got: "1;h=sha-256;d=bbb", + }, + ], + missing: [ + { + ruleId: "evals-def67890", + file: ".taskless/rules/runtime/x/check.ts", + }, + ], + }); + + expect(targets).toHaveLength(2); + expect(targets[0]?.ruleId).toBe("logs-abc12345"); + expect(targets[0]?.expected).toBe("1;h=sha-256;d=aaa"); + // A missing rule is one we do not hold, so there is no prior expectation. + expect(targets[1]?.ruleId).toBe("evals-def67890"); + expect(targets[1]?.expected).toBeUndefined(); + }); + + it("takes a missing entry's id rather than parsing its path", () => { + // The path here would parse to `x`; the entry says `evals-def67890`, and + // the entry wins. Parsing is the fallback for `unsafe` only. + const { targets } = repairTargets({ + unsafe: [], + missing: [ + { + ruleId: "evals-def67890", + file: ".taskless/rules/runtime/x/check.ts", + }, + ], + }); + expect(targets[0]?.ruleId).toBe("evals-def67890"); + }); + + it("reports an unsafe entry whose path yields no rule id", () => { + // Rather than requesting a guessed id. The service never issued a rule + // called `src`, and asking for one turns a repairable state into a 404. + const { targets, unidentifiable } = repairTargets({ + unsafe: [{ file: "src/check.ts", expected: "1;h=sha-256;d=a", got: "b" }], + missing: [], + }); + expect(targets).toHaveLength(0); + expect(unidentifiable).toHaveLength(1); + }); +}); + +describe("reading a rule id out of a check path", () => { + it.each([ + [".taskless/rules/runtime/logs-abc12345/check.ts", "logs-abc12345"], + // A repo nested under a directory of the same name: matched from the right. + ["vendor/.taskless/rules/runtime/logs-abc12345/check.ts", "logs-abc12345"], + ])("reads %s as %s", (file, expected) => { + expect(ruleIdFromCheckPath(file)).toBe(expected); + }); + + it.each([ + ["a check outside the rules tree", "src/check.ts"], + ["the wrong engine directory", ".taskless/rules/sg/logs-abc/check.ts"], + ["a differently rooted tree", "other/rules/runtime/logs-abc/check.ts"], + ["no check.ts at all", ".taskless/rules/runtime/logs-abc/rule.yml"], + ])("refuses %s", (_label, file) => { + expect(ruleIdFromCheckPath(file)).toBeUndefined(); + }); +}); + +describe("verifying restored bytes", () => { + const unsafeTarget = async (): Promise => ({ + ruleId: "logs-abc12345", + file: ".taskless/rules/runtime/logs-abc12345/check.ts", + expected: await canonicalHash(CHECK), + }); + + it("accepts the bytes reconcile blessed", async () => { + const verdict = await verifyRestoredCheck( + await unsafeTarget(), + await restored(CHECK) + ); + expect(verdict.ok).toBe(true); + }); + + /** + * The property task 6.4 asks for, and the reason `expected` is checked at + * all: restore repairs, it does not upgrade. + */ + it("refuses bytes that are newer than the ones reconcile blessed", async () => { + // Internally consistent — the service signed exactly what it sent — and + // still refused, because it is not what we were owed. Verifying only the + // response against itself would install this silently, mid-`check`. + const newer = await restored(TAMPERED); + const verdict = await verifyRestoredCheck(await unsafeTarget(), newer); + + expect(verdict.ok).toBe(false); + expect(verdict.ok === false && verdict.reason).toMatch( + /not the ones reconcile blessed/ + ); + expect(verdict.ok === false && verdict.reason).toMatch(/does not upgrade/); + }); + + it("refuses bytes that do not match the signature sent with them", async () => { + // A corrupted or substituted transfer: the claim and the content disagree. + const inconsistent = await restored(TAMPERED, await canonicalHash(CHECK)); + const verdict = await verifyRestoredCheck( + await unsafeTarget(), + inconsistent + ); + + expect(verdict.ok).toBe(false); + expect(verdict.ok === false && verdict.reason).toMatch( + /do not match the signature/ + ); + }); + + it("refuses a runtime rule the service returned without a signature", async () => { + // The published schema requires it, so this means the service broke its + // own contract. Writing unvouched-for bytes is not the way to find out. + const unsigned = { + id: "logs-abc12345", + engine: "runtime", + files: [{ path: "check.ts", content: CHECK }], + } as unknown as RestoredRule; + + const verdict = await verifyRestoredCheck(await unsafeTarget(), unsigned); + expect(verdict.ok).toBe(false); + expect(verdict.ok === false && verdict.reason).toMatch(/no signature/); + }); + + it.each([ + ["no check.ts", []], + [ + "two check.ts entries", + [ + { path: "check.ts", content: CHECK }, + { path: "check.ts", content: TAMPERED }, + ], + ], + ])("refuses a restored set with %s", async (_label, files) => { + const rule = { + id: "logs-abc12345", + engine: "runtime", + files, + signature: await canonicalHash(CHECK), + } as unknown as RestoredRule; + + const verdict = await verifyRestoredCheck(await unsafeTarget(), rule); + expect(verdict.ok).toBe(false); + expect(verdict.ok === false && verdict.reason).toMatch( + /single `check\.ts`/ + ); + }); + + it("falls back to the response signature for a missing rule", async () => { + // Nothing local to disagree with, so the weaker check is the only one + // available. Stated in the module rather than left as an inconsistency. + const verdict = await verifyRestoredCheck( + { + ruleId: "evals-def67890", + file: ".taskless/rules/runtime/evals-def67890/check.ts", + expected: undefined, + }, + await restored(TAMPERED) + ); + expect(verdict.ok).toBe(true); + }); +});