fix: take an unsafe entry's rule id from the entry, not its path

.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.

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;

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.`
     );
   }
 

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/<id>/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/<RULES_DIRECTORY>/
-  // 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. */

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();
+    }
+  });
 });