From 1698998f12b84a0586b2d5614265f3ae2fd9f473 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 1 Sep 2026 18:34:03 -0700 Subject: [PATCH 1/2] fix: take an unsafe entry's rule id from the entry, not its path `repairTargets` parsed the rule id out of a reported `check.ts` path because reconcile's `unsafe` entries did not carry `ruleId`. They do: the deployed `GET /cli/api/__schema` requires `[expected, file, got, ruleId]` on `unsafe`. Our hand-written `UnsafeEntry` never declared the field, so it arrived on the wire and stayed invisible. The parse tied repair to a filesystem layout that has moved twice. A further move breaks it with nothing anyone sees: a wrong id, a 404 from restore, and a rule left unrepaired and unexecuted, which reads exactly like a pass. Deletes `ruleIdFromCheckPath`, the `unidentifiable` bucket it fed, and the notice that reported it. `repairTargets` now returns a plain array, since an entry can no longer fail to identify itself. `unknown` still routes nowhere, and `verifyRestoredCheck` is untouched. Fixes #240 --- .changeset/lucky-pans-repeat.md | 5 ++ packages/cli/src/api/reconcile.ts | 1 + packages/cli/src/commands/check.ts | 8 +- packages/cli/src/rules/runtime/repair.ts | 78 ++++++-------------- packages/cli/test/repair-integration.test.ts | 71 ++++++++++++++++-- packages/cli/test/repair.test.ts | 77 ++++++++++--------- 6 files changed, 130 insertions(+), 110 deletions(-) create mode 100644 .changeset/lucky-pans-repeat.md diff --git a/.changeset/lucky-pans-repeat.md b/.changeset/lucky-pans-repeat.md new file mode 100644 index 00000000..f78b8955 --- /dev/null +++ b/.changeset/lucky-pans-repeat.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +Rule repair now takes a drifted rule's id from the reconcile entry that reports it, rather than parsing it out of the reported `check.ts` path. The server sends `ruleId` on every `unsafe` entry, so the parse and its fallback are gone. The parse tied repair to a filesystem layout that has moved twice, and a further move would have broken it silently: a wrong id, a 404 from restore, and a rule left unrepaired and unexecuted with nothing reported. diff --git a/packages/cli/src/api/reconcile.ts b/packages/cli/src/api/reconcile.ts index 08309102..66806741 100644 --- a/packages/cli/src/api/reconcile.ts +++ b/packages/cli/src/api/reconcile.ts @@ -35,6 +35,7 @@ export interface RunEntry { /** A held rule whose content differs from what the server blessed. */ export interface UnsafeEntry { + ruleId: string; file: string; expected: string; got: string; diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index a0c37a79..e2b84c3f 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -251,13 +251,7 @@ async function repairWithheldRules( ); } - 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.` - ); - } + const targets = repairTargets(input.result); // 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 diff --git a/packages/cli/src/rules/runtime/repair.ts b/packages/cli/src/rules/runtime/repair.ts index a287546a..043c3631 100644 --- a/packages/cli/src/rules/runtime/repair.ts +++ b/packages/cli/src/rules/runtime/repair.ts @@ -1,22 +1,18 @@ 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. + * - `unsafe` `{ ruleId, file, expected, got }` — we hold bytes that drifted + * from what the server blessed. Repairable: the server has the real ones, + * and the entry names which rule to ask for. * - `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. + * reported. Repairable, and it names its rule the same way. * - `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 @@ -41,62 +37,32 @@ export interface RepairTarget { } /** - * 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. + * Repair targets for the buckets that have something to fetch. * - * 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. + * Both repairable verdicts carry `ruleId`, so the id restore is keyed on is + * read straight off the entry. Nothing is derived from the reported path: the + * rules layout has moved twice, and a parse of it would break silently on the + * next move, requesting a rule id the service never issued and leaving a + * drifted rule unrepaired and unexecuted with nothing to read. */ -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({ +}): RepairTarget[] { + return [ + ...buckets.unsafe.map((entry) => ({ ruleId: entry.ruleId, file: entry.file, + expected: entry.expected, + })), + ...buckets.missing.map((entry) => ({ + ruleId: entry.ruleId, + file: entry.file, + // A missing rule is one we do not hold, so there is no prior signature + // to hold the restored bytes to. expected: undefined, - }); - } - return { targets, unidentifiable }; + })), + ]; } /** Why a restored rule was refused, in words a `check` reader can act on. */ diff --git a/packages/cli/test/repair-integration.test.ts b/packages/cli/test/repair-integration.test.ts index 1e44cc5f..c7e2d83d 100644 --- a/packages/cli/test/repair-integration.test.ts +++ b/packages/cli/test/repair-integration.test.ts @@ -127,13 +127,22 @@ 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: [], -}); +/** + * Reconcile reporting the reported check as drifted from `expected`. + * + * `ruleId` is what restore is keyed on. It defaults to the rule directory's + * name only so the ordinary fixtures read naturally; nothing derives it from + * `REPORTED`, and the disagreeing-id test below passes one that does not match + * the path at all. + */ +const driftedReconcile = + (expected: string, ruleId = "demo") => + () => ({ + run: [], + unsafe: [{ ruleId, file: REPORTED, expected, got: "1;h=sha-256;d=stale" }], + unknown: [], + missing: [], + }); describe("repairing a drifted runtime rule, end to end", () => { let directory: string; @@ -325,6 +334,54 @@ describe("repairing a drifted runtime rule, end to end", () => { } }); + it("asks for the id the unsafe entry carries, not the one in its path", async () => { + // #240. The entry names `logs-abc12345` while its path's rule directory is + // `demo`, and the entry wins. This is the assertion a path-parsing + // implementation fails: it would ask for `demo`, and after a layout move it + // would ask for something the service never issued, take a 404, and leave + // the rule unrepaired and unexecuted with nothing said on any channel. + const blessed = await canonicalHash(BLESSED); + const mock = await startMock({ + reconcile: driftedReconcile(blessed, "logs-abc12345"), + restore: (ruleId) => ({ + statusCode: 200, + body: { + ruleId, + rules: [ + { + id: ruleId, + engine: "runtime", + files: [ + { path: "check.ts", content: BLESSED }, + { path: "captures/logs.yml", content: CAPTURE }, + ], + signature: blessed, + }, + ], + }, + }), + }); + try { + await runCli(["check", "-d", directory, "--json"], { + TASKLESS_TOKEN: "fake.token", + TASKLESS_API_URL: mock.apiUrl, + }); + expect(mock.restoreCalls).toEqual(["logs-abc12345"]); + // And the repair completed, so the entry's id was what reached restore + // rather than the request merely landing somewhere harmless. The bytes + // land under the id's own directory, since the delivered set decides + // where a rule lives and the reported path never did. + await expect( + readFile( + join(directory, ".taskless/rules/runtime/logs-abc12345/check.ts"), + "utf8" + ) + ).resolves.toBe(BLESSED); + } finally { + await mock.close(); + } + }); + it("says why an unknown file cannot be restored, and asks for nothing", async () => { const mock = await startMock({ reconcile: () => ({ diff --git a/packages/cli/test/repair.test.ts b/packages/cli/test/repair.test.ts index aa75c89a..1e959edd 100644 --- a/packages/cli/test/repair.test.ts +++ b/packages/cli/test/repair.test.ts @@ -3,7 +3,6 @@ import { describe, expect, it } from "vitest"; import { canonicalHash } from "../src/rules/rule-hash"; import { repairTargets, - ruleIdFromCheckPath, verifyRestoredCheck, type RepairTarget, } from "../src/rules/runtime/repair"; @@ -30,9 +29,10 @@ describe("which reconcile verdicts can be repaired", () => { // `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({ + const targets = repairTargets({ unsafe: [ { + ruleId: "logs-abc12345", file: ".taskless/rules/runtime/logs-abc12345/check.ts", expected: "1;h=sha-256;d=aaa", got: "1;h=sha-256;d=bbb", @@ -54,49 +54,46 @@ describe("which reconcile verdicts can be repaired", () => { 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: [ + it.each([["unsafe" as const], ["missing" as const]])( + "takes a %s entry's id rather than parsing its path", + (bucket) => { + // The path parses to `x`; the entry says `evals-def67890`. The entry wins, + // and nothing reads the path at all. A path-parsing implementation asks + // restore for `x`, gets a 404, and leaves the rule unrepaired in silence, + // which is the failure this assertion exists to catch. + const entry = { + ruleId: "evals-def67890", + file: ".taskless/rules/runtime/x/check.ts", + }; + const targets = repairTargets({ + unsafe: + bucket === "unsafe" + ? [{ ...entry, expected: "1;h=sha-256;d=aaa", got: "b" }] + : [], + missing: bucket === "missing" ? [entry] : [], + }); + expect(targets).toHaveLength(1); + expect(targets[0]?.ruleId).toBe("evals-def67890"); + } + ); + + it("identifies an unsafe entry whose file sits outside the rules tree", () => { + // Nothing about the reported path decides anything now, so a rule held + // somewhere the layout does not describe is still repairable. Under path + // parsing this entry was unidentifiable and dropped. + const targets = repairTargets({ + unsafe: [ { - ruleId: "evals-def67890", - file: ".taskless/rules/runtime/x/check.ts", + ruleId: "logs-abc12345", + file: "src/check.ts", + expected: "1;h=sha-256;d=a", + got: "b", }, ], - }); - 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(); + expect(targets).toHaveLength(1); + expect(targets[0]?.ruleId).toBe("logs-abc12345"); }); }); From dd2a9911c916d35823db2429c86aa004738cdbcf Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 1 Sep 2026 19:42:38 -0700 Subject: [PATCH 2/2] fix(check): skip a reconcile entry that names no rule, and say so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `repairTargets` read `ruleId` off the entry unconditionally, and `reconcile` decodes the response with a cast rather than a schema. The deployed schema requires the field on both repairable verdicts, so the `string` type is the service's promise and not a fact about the bytes that arrived. A rollback, a canary, or a schema regression put `undefined` into it and `restoreRule` interpolated it straight into `/cli/api/request/undefined/restore` — a call that cannot succeed, made for a rule nobody named, failing where nobody reads it. Entries without a usable id are now partitioned out and reported as a notice, the way the deleted `unidentifiable` path did. Covers `missing` as well as `unsafe`, since the gap predates the entry-id change on that bucket. Whitespace-only ids are rejected for the same reason an absent one is: they encode into an equally meaningless request. Nothing is guessed. Path parsing stays deleted; the answer to a missing id is to report it. The rule stays withheld, which is already the safe state, and the notice reaches the `--json` envelope so a CI run sees it rather than reading a clean pass. This echoes `writeRuleFile`, which keeps a runtime check for a payload carrying both `files` and `content` even though the published union makes it unrepresentable. --- .changeset/lucky-pans-repeat.md | 2 + packages/cli/src/commands/check.ts | 15 +++- packages/cli/src/rules/runtime/repair.ts | 80 ++++++++++++++++---- packages/cli/test/repair-integration.test.ts | 44 +++++++++++ packages/cli/test/repair.test.ts | 73 +++++++++++++++++- 5 files changed, 195 insertions(+), 19 deletions(-) diff --git a/.changeset/lucky-pans-repeat.md b/.changeset/lucky-pans-repeat.md index f78b8955..91a2d2b1 100644 --- a/.changeset/lucky-pans-repeat.md +++ b/.changeset/lucky-pans-repeat.md @@ -3,3 +3,5 @@ --- Rule repair now takes a drifted rule's id from the reconcile entry that reports it, rather than parsing it out of the reported `check.ts` path. The server sends `ruleId` on every `unsafe` entry, so the parse and its fallback are gone. The parse tied repair to a filesystem layout that has moved twice, and a further move would have broken it silently: a wrong id, a 404 from restore, and a rule left unrepaired and unexecuted with nothing reported. + +A reconcile entry that arrives without a usable `ruleId` is now skipped with a notice instead of becoming a request. The service's schema requires the field, but the CLI decodes the response with a cast rather than a schema, so a rollback or a regression could previously send the CLI to `/cli/api/request/undefined/restore`. This covers `missing` as well as `unsafe`; the rule stays withheld, which is already the safe state, and the notice reaches the `--json` envelope so a CI run can see it. diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index e2b84c3f..97fd3fd1 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -251,7 +251,20 @@ async function repairWithheldRules( ); } - const targets = repairTargets(input.result); + const { targets, unidentified } = repairTargets(input.result); + + // A repairable entry that named no rule. The service's own schema requires + // one, so reaching here means it broke that contract — and the entry is + // skipped rather than guessed at, because a rule left unrepaired stays + // withheld, which is safe, while a request built from a missing id asks for + // a rule nobody named and fails in a way nobody reads. + for (const entry of unidentified) { + notices.push( + `${entry.file} needs to be restored, but the rule service did not say ` + + `which rule it belongs to, so it could not be requested and will not ` + + `run.` + ); + } // 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 diff --git a/packages/cli/src/rules/runtime/repair.ts b/packages/cli/src/rules/runtime/repair.ts index 043c3631..6627c651 100644 --- a/packages/cli/src/rules/runtime/repair.ts +++ b/packages/cli/src/rules/runtime/repair.ts @@ -36,6 +36,41 @@ export interface RepairTarget { expected: string | undefined; } +/** A repairable entry that did not name the rule to ask for. */ +export interface UnidentifiedEntry { + file: string; +} + +/** What the repairable buckets resolved to: what to fetch, and what cannot be. */ +export interface RepairPlan { + targets: RepairTarget[]; + /** + * Entries the published schema says must carry a `ruleId` and did not. There + * is no request to make for these, only something to say. + */ + unidentified: UnidentifiedEntry[]; +} + +/** + * The rule id an entry names, or `undefined` if it did not name one. + * + * The deployed schema requires `ruleId` on both repairable verdicts and the + * type here says `string`, but `reconcile` decodes the response with a cast + * rather than a schema, so that type is the service's PROMISE and not a fact + * about the bytes that arrived. This defends against it breaking that promise, + * which is the only reason a client validates a payload at all. + * + * Without it a rollback, a canary, or a schema regression puts `undefined` into + * a `string` and `restoreRule` asks for `/cli/api/request/undefined/restore` — + * a request that cannot succeed, made on behalf of a rule nobody can name. + * Whitespace is rejected for the same reason: it URL-encodes into an equally + * meaningless request rather than failing where it can be read. + */ +function namedRuleId(entry: { ruleId: string }): string | undefined { + const id: unknown = entry.ruleId; + return typeof id === "string" && id.trim() !== "" ? id : undefined; +} + /** * Repair targets for the buckets that have something to fetch. * @@ -44,25 +79,40 @@ export interface RepairTarget { * rules layout has moved twice, and a parse of it would break silently on the * next move, requesting a rule id the service never issued and leaving a * drifted rule unrepaired and unexecuted with nothing to read. + * + * An entry that names no rule is separated out rather than guessed at. The + * answer to a missing id is to report it — a rule that is not repaired stays + * withheld, which is already the safe state, and a notice is the difference + * between that and a clean-looking pass. */ export function repairTargets(buckets: { unsafe: UnsafeEntry[]; missing: MissingEntry[]; -}): RepairTarget[] { - return [ - ...buckets.unsafe.map((entry) => ({ - ruleId: entry.ruleId, - file: entry.file, - expected: entry.expected, - })), - ...buckets.missing.map((entry) => ({ - ruleId: entry.ruleId, - file: entry.file, - // A missing rule is one we do not hold, so there is no prior signature - // to hold the restored bytes to. - expected: undefined, - })), - ]; +}): RepairPlan { + const targets: RepairTarget[] = []; + const unidentified: UnidentifiedEntry[] = []; + + for (const entry of buckets.unsafe) { + const ruleId = namedRuleId(entry); + if (ruleId === undefined) { + unidentified.push({ file: entry.file }); + continue; + } + targets.push({ ruleId, file: entry.file, expected: entry.expected }); + } + + for (const entry of buckets.missing) { + const ruleId = namedRuleId(entry); + if (ruleId === undefined) { + unidentified.push({ file: entry.file }); + continue; + } + // A missing rule is one we do not hold, so there is no prior signature to + // hold the restored bytes to. + targets.push({ ruleId, file: entry.file, expected: undefined }); + } + + return { targets, unidentified }; } /** Why a restored rule was refused, in words a `check` reader can act on. */ diff --git a/packages/cli/test/repair-integration.test.ts b/packages/cli/test/repair-integration.test.ts index c7e2d83d..07f518a6 100644 --- a/packages/cli/test/repair-integration.test.ts +++ b/packages/cli/test/repair-integration.test.ts @@ -405,4 +405,48 @@ describe("repairing a drifted runtime rule, end to end", () => { await mock.close(); } }); + + it("asks for nothing when an unsafe entry carries no rule id, and says why", async () => { + // The deployed schema requires `ruleId` on `unsafe`, and `reconcile` + // decodes with a cast rather than a schema — so a rollback, a canary, or a + // regression can put an entry like this on the wire despite the `string` + // type. Without a guard `restoreRule` interpolates `undefined` and the CLI + // requests `/cli/api/request/undefined/restore`: a call that cannot + // succeed, made for a rule nobody named. + const mock = await startMock({ + reconcile: () => ({ + run: [], + unsafe: [ + { + file: REPORTED, + expected: "1;h=sha-256;d=a", + got: "1;h=sha-256;d=b", + }, + ], + unknown: [], + 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"); + + // The whole point: no request at all, and in particular not the literal + // `undefined` the unguarded path would have sent. + expect(mock.restoreCalls).toEqual([]); + // Not a silent skip. `--json` is what CI reads, so the notice has to be + // here and it has to name the file. + expect(notices).toContain(REPORTED); + expect(notices).toMatch(/did not say which rule it belongs to/); + // Withheld, not repaired: the drifted bytes are untouched, which is + // already the safe state for a rule that could not be verified. + await expect(readFile(checkFile, "utf8")).resolves.toBe(DRIFTED); + } finally { + await mock.close(); + } + }); }); diff --git a/packages/cli/test/repair.test.ts b/packages/cli/test/repair.test.ts index 1e959edd..a97621d4 100644 --- a/packages/cli/test/repair.test.ts +++ b/packages/cli/test/repair.test.ts @@ -29,7 +29,7 @@ describe("which reconcile verdicts can be repaired", () => { // `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({ + const { targets } = repairTargets({ unsafe: [ { ruleId: "logs-abc12345", @@ -65,7 +65,7 @@ describe("which reconcile verdicts can be repaired", () => { ruleId: "evals-def67890", file: ".taskless/rules/runtime/x/check.ts", }; - const targets = repairTargets({ + const { targets } = repairTargets({ unsafe: bucket === "unsafe" ? [{ ...entry, expected: "1;h=sha-256;d=aaa", got: "b" }] @@ -81,7 +81,7 @@ describe("which reconcile verdicts can be repaired", () => { // Nothing about the reported path decides anything now, so a rule held // somewhere the layout does not describe is still repairable. Under path // parsing this entry was unidentifiable and dropped. - const targets = repairTargets({ + const { targets } = repairTargets({ unsafe: [ { ruleId: "logs-abc12345", @@ -95,6 +95,73 @@ describe("which reconcile verdicts can be repaired", () => { expect(targets).toHaveLength(1); expect(targets[0]?.ruleId).toBe("logs-abc12345"); }); + + // The published schema requires `ruleId` on both repairable verdicts, and + // `reconcile` decodes with a cast rather than a schema, so the `string` type + // is what the service PROMISES rather than what arrived. A rollback or a + // canary that breaks that promise must not become a request: without the + // guard `restoreRule` is handed `undefined` and asks the service for + // `/cli/api/request/undefined/restore`, which cannot succeed and says + // nothing when it fails. + describe.each([["unsafe" as const], ["missing" as const]])( + "a %s entry that names no rule", + (bucket) => { + const plan = (ruleId?: unknown) => { + const entry = { ruleId, file: `.taskless/rules/runtime/x/check.ts` }; + return repairTargets({ + unsafe: + bucket === "unsafe" + ? [{ ...entry, expected: "1;h=sha-256;d=a", got: "b" }] + : [], + missing: bucket === "missing" ? [entry] : [], + } as unknown as Parameters[0]); + }; + + it.each([ + ["absent", undefined], + ["empty", ""], + ["whitespace", " "], + ])("is not requested when its id is %s", (_label, ruleId) => { + const { targets, unidentified } = plan(ruleId); + // Nothing to ask for: no target means no request built from a rule id + // nobody supplied. + expect(targets).toEqual([]); + // And it is said out loud, so the skip is not a silent pass. + expect(unidentified).toEqual([ + { file: ".taskless/rules/runtime/x/check.ts" }, + ]); + }); + + it("is not rescued by parsing its path", () => { + // The path contains `x`, which the deleted fallback would have taken. + // The id is reported as absent, not invented. + const { targets } = plan(); + expect(targets).toEqual([]); + }); + } + ); + + it("keeps identified entries when another names no rule", () => { + // One bad entry withholds itself, not the rules beside it. + const { targets, unidentified } = repairTargets({ + unsafe: [ + { + ruleId: "logs-abc12345", + file: ".taskless/rules/runtime/logs-abc12345/check.ts", + expected: "1;h=sha-256;d=a", + got: "b", + }, + { file: "src/orphan.ts", expected: "1;h=sha-256;d=c", got: "d" }, + ], + missing: [{ ruleId: "evals-def67890", file: "src/present.ts" }], + } as unknown as Parameters[0]); + + expect(targets.map((target) => target.ruleId)).toEqual([ + "logs-abc12345", + "evals-def67890", + ]); + expect(unidentified).toEqual([{ file: "src/orphan.ts" }]); + }); }); describe("verifying restored bytes", () => {