From 8584c7865a0ef6b5e45674ad098086d2c56e3358 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 31 Aug 2026 21:59:50 -0700 Subject: [PATCH 1/3] feat(check): repair a drifted rule instead of only reporting it Slice 6. `check` parsed reconcile's `unsafe`, `unknown` and `missing` verdicts and read none of them. THE TASK SAID "unsafe / 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. The first two are repairable; the third has nothing on the server to fetch, which is exactly why its entry carries no rule id. `missing` was omitted from the task despite being the only bucket that already carries the id restore is keyed on. So `unsafe` and `missing` route to restore, and `unknown` gets an explanation, since "on your disk, never issued by the service" is ordinary (hand-written, or another org or install) and read as an unexplained skip. Restored bytes are verified against the signature reconcile ALREADY sent, not the one the restore response carries. Verifying a response against itself proves the service is internally consistent, which it would also be if it returned a NEWER generation of the rule: an upgrade wearing a repair's clothes, arriving mid-check, reviewed by nobody. Because `unsafe.expected` is in hand there is exactly one acceptable answer, which is what makes "restore never returns newer bytes" a test rather than a promise we relay. 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 and the next `check` gets it blessed through the ordinary path. Fetching code and executing it in the same pass that discovered the drift would move the gate, and the gate is the point. A repair that cannot happen is a notice, never a failed `check`: an unrepaired rule stays withheld, which is already safe. The rule id for an `unsafe` entry is parsed out of `.taskless/rules/runtime//check.ts`, anchored on the full prefix because `rules/runtime/` alone is common enough that a wrong id would be a 404 reading as "the service lost your rule". That parse is the weak link and is asked for as N6: the layout has moved twice, and a move breaks repair silently. Delete it when the entry carries the id. Also corrects tasks.md, which had drifted several items behind reality. --- .changeset/generator-payload-alignment.md | 24 +++ .../generator-payload-alignment/tasks.md | 25 ++- packages/cli/src/api/restore.ts | 92 ++++++++ packages/cli/src/commands/check.ts | 103 ++++++++- packages/cli/src/rules/runtime/repair.ts | 173 +++++++++++++++ packages/cli/test/repair.test.ts | 201 ++++++++++++++++++ 6 files changed, 608 insertions(+), 10 deletions(-) create mode 100644 packages/cli/src/api/restore.ts create mode 100644 packages/cli/src/rules/runtime/repair.ts create mode 100644 packages/cli/test/repair.test.ts 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..844b599e 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -16,6 +16,11 @@ 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 type { GeneratedRule } from "../api/rules"; import { discoverRuntimeRules, type RuntimeRule, @@ -194,6 +199,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 +218,96 @@ 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.` + ); + } + + for (const target of targets) { + const outcome = await restoreRule(token, { + ruleId: target.ruleId, + repositoryUrl: input.repositoryUrl, + }); + 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 as unknown as GeneratedRule); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + notices.push(`${target.file} could not be written (${message}).`); + continue; + } + notices.push( + `${target.file} was restored to 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; diff --git a/packages/cli/src/rules/runtime/repair.ts b/packages/cli/src/rules/runtime/repair.ts new file mode 100644 index 00000000..acf6bc8b --- /dev/null +++ b/packages/cli/src/rules/runtime/repair.ts @@ -0,0 +1,173 @@ +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 { + const files = (rule as { files?: { path: string; content: string }[] }).files; + if (!Array.isArray(files)) return undefined; + const matches = 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`", + }; + } + + const claimed = (rule as { signature?: string }).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/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); + }); +}); From fdbad745f9e08448e61bd5dbeb62f65d5599f5a4 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 31 Aug 2026 23:37:21 -0700 Subject: [PATCH 2/3] fix(rules): refuse content that yaml renders instead of rejecting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #229, applied here because #229 had already merged. `content: null` reproduced the exact bug #229 says it closes. The guard tested `content === undefined`, and `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". Either way the rule file is created and its entire contents are that word. A string or a number does the same. The test is now "a usable object" rather than "not undefined", and the three cases are covered. Confirmed by restoring the old guard and watching all three fail. Worth naming why two checks in this function disagree about `null`, and why that is correct. The mutual-exclusion check asks what the payload CLAIMS, so any present `content` — `null` included — means the service sent both envelopes. This one asks what can be WRITTEN. Collapsing them into one predicate would make one of the two wrong. Also drops a dead disjunct the reviewer spotted: `!isSingleContentRule` could never be true there, since `files` is provably absent by that point and the helper is defined as its negation. And a file set arriving with a stray `tests` now fails loudly rather than dropping it. The published schema makes that unrepresentable, so this is the same defence the rest of the path already applies to a broken promise — a fixture that vanishes silently shows up much later as a rule that tests nothing. --- packages/cli/src/commands/rules.ts | 34 ++++++++++++++++++++++++++ packages/cli/src/rules/files.ts | 39 ++++++++++++++++++++++++------ packages/cli/test/deliver.test.ts | 21 +++++++++++++++- 3 files changed, 85 insertions(+), 9 deletions(-) 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/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 From 11df67d242efdda3f83ff49a2725cc51ee229f5e Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 31 Aug 2026 23:45:22 -0700 Subject: [PATCH 3/3] fix(check): put repair notices on the --json envelope, and test the wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #230. THE FEATURE'S ENTIRE OUTPUT WAS INVISIBLE TO CI. Repair notices reached `warn()` only, which is a no-op under `--json`, and the envelope merged `dispatched.notices` alone. So the one channel a CI run reads dropped every "was restored" and "could not be restored" message — from the feature whose whole purpose is explaining why a rule did not run. The reviewer's other point is the reason it survived: `repair.test.ts` covers the decisions as pure functions and cannot see the wiring. `repair-integration.test.ts` exercises the real path against a mock serving both endpoints, and all four cases fail against the old envelope code, so the coverage gap and the defect were the same thing. Writing that test found something real. A restore returning only `check.ts` is refused, because `writeRuleFile` enforces the delivery contract and a runtime rule needs its captures. That is correct — the schema calls `files` "every file the rule directory must contain" — so the fixtures now return the complete set, which is what the service sends. Also from review: - `restoreRule` calls run concurrently. Each target is a different rule under the same token, so they do not order against each other. The WRITES stay sequential, because a half-applied set is the state this path exists to avoid. - Two casts removed in favour of the generated types. `rule.files` and `rule.signature` are on the union already, and re-declaring them inline would absorb a schema change instead of failing the build. The `as unknown as GeneratedRule` was unnecessary: `RestoredRule` is directly assignable, which the double cast was hiding. - The success notice said the rule "was restored", which overstated it. The delivered set is written over the directory without removing files it does not mention, and only `check.ts` is signed, so nothing here can vouch for the rest. It now says the check was rewritten with the blessed bytes. The gap itself is #233, since it is shared with `rule create` and `rule iterate` and wants one answer for all three. --- packages/cli/src/commands/check.ts | 45 ++- packages/cli/src/rules/runtime/repair.ts | 12 +- packages/cli/test/repair-integration.test.ts | 283 +++++++++++++++++++ 3 files changed, 324 insertions(+), 16 deletions(-) create mode 100644 packages/cli/test/repair-integration.test.ts diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 844b599e..8824423b 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -20,7 +20,6 @@ import type { ReconcileResponse } from "../api/reconcile"; import { restoreRule } from "../api/restore"; import { repairTargets, verifyRestoredCheck } from "../rules/runtime/repair"; import { writeRuleFile } from "../rules/files"; -import type { GeneratedRule } from "../api/rules"; import { discoverRuntimeRules, type RuntimeRule, @@ -258,11 +257,23 @@ async function repairWithheldRules( ); } - for (const target of targets) { - const outcome = await restoreRule(token, { - ruleId: target.ruleId, - repositoryUrl: input.repositoryUrl, - }); + // 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 (${ @@ -292,15 +303,21 @@ async function repairWithheldRules( } try { - await writeRuleFile(cwd, rule as unknown as GeneratedRule); + 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 restored to the bytes the service blessed. It does ` + - `not run in this pass; the next \`check\` reports the repaired ` + + `${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.` ); } @@ -500,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; @@ -524,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/rules/runtime/repair.ts b/packages/cli/src/rules/runtime/repair.ts index acf6bc8b..a287546a 100644 --- a/packages/cli/src/rules/runtime/repair.ts +++ b/packages/cli/src/rules/runtime/repair.ts @@ -106,9 +106,10 @@ export type RepairVerdict = /** The `check.ts` entry of a restored file set, if it carries exactly one. */ function restoredCheck(rule: RestoredRule): string | undefined { - const files = (rule as { files?: { path: string; content: string }[] }).files; - if (!Array.isArray(files)) return undefined; - const matches = files.filter((file) => file.path === "check.ts"); + // `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; } @@ -143,7 +144,10 @@ export async function verifyRestoredCheck( }; } - const claimed = (rule as { signature?: string }).signature; + // 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 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(); + } + }); +});