diff --git a/.changeset/lucky-pans-repeat.md b/.changeset/lucky-pans-repeat.md new file mode 100644 index 00000000..91a2d2b1 --- /dev/null +++ b/.changeset/lucky-pans-repeat.md @@ -0,0 +1,7 @@ +--- +"@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. + +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/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..97fd3fd1 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -251,11 +251,18 @@ async function repairWithheldRules( ); } - const { targets, unidentifiable } = repairTargets(input.result); - for (const entry of unidentifiable) { + 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} drifted from the blessed rule, and its rule id could not ` + - `be read from its path, so it was not restored.` + `${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.` ); } diff --git a/packages/cli/src/rules/runtime/repair.ts b/packages/cli/src/rules/runtime/repair.ts index a287546a..6627c651 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 @@ -40,63 +36,83 @@ 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 inside a reported `check.ts` path. + * The rule id an entry names, or `undefined` if it did not name one. * - * `.taskless/rules/runtime//check.ts`, matched from the RIGHT so a repo - * nested under a similarly-named directory cannot shift the segment. + * 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. * - * 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. + * 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. */ -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; +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. */ +/** + * Repair targets for the buckets that have something to fetch. + * + * 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. + * + * 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[]; -}): { targets: RepairTarget[]; unidentifiable: UnsafeEntry[] } { +}): RepairPlan { const targets: RepairTarget[] = []; - const unidentifiable: UnsafeEntry[] = []; + const unidentified: UnidentifiedEntry[] = []; for (const entry of buckets.unsafe) { - const ruleId = ruleIdFromCheckPath(entry.file); + const ruleId = namedRuleId(entry); if (ruleId === undefined) { - unidentifiable.push(entry); + unidentified.push({ file: entry.file }); 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, - }); + 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, unidentifiable }; + + 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 1e44cc5f..07f518a6 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: () => ({ @@ -348,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 aa75c89a..a97621d4 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"; @@ -33,6 +32,7 @@ describe("which reconcile verdicts can be repaired", () => { 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,113 @@ 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. + 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: [], - missing: [ + 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); + expect(targets).toHaveLength(1); + expect(targets[0]?.ruleId).toBe("logs-abc12345"); }); -}); -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); - }); + // 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([ - ["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(); + 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" }]); }); });