Skip to content

Commit 48f4e92

Browse files
committed
fix(check): put repair notices on the --json envelope, and test the wiring
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.
1 parent b87d591 commit 48f4e92

3 files changed

Lines changed: 324 additions & 16 deletions

File tree

‎packages/cli/src/commands/check.ts‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import type { ReconcileResponse } from "../api/reconcile";
2020
import { restoreRule } from "../api/restore";
2121
import { repairTargets, verifyRestoredCheck } from "../rules/runtime/repair";
2222
import { writeRuleFile } from "../rules/files";
23-
import type { GeneratedRule } from "../api/rules";
2423
import {
2524
discoverRuntimeRules,
2625
type RuntimeRule,
@@ -258,11 +257,23 @@ async function repairWithheldRules(
258257
);
259258
}
260259

261-
for (const target of targets) {
262-
const outcome = await restoreRule(token, {
263-
ruleId: target.ruleId,
264-
repositoryUrl: input.repositoryUrl,
265-
});
260+
// Fetched concurrently: each target is a different rule id under the same
261+
// token and repository, so they do not order against each other, and a repo
262+
// with several drifted rules would otherwise pay one round trip per rule on
263+
// every `check` until they reconverge. The WRITES stay sequential below,
264+
// because two rules can share a directory prefix and a half-applied set is
265+
// the state this whole path exists to avoid.
266+
const fetched = await Promise.all(
267+
targets.map(async (target) => ({
268+
target,
269+
outcome: await restoreRule(token, {
270+
ruleId: target.ruleId,
271+
repositoryUrl: input.repositoryUrl,
272+
}),
273+
}))
274+
);
275+
276+
for (const { target, outcome } of fetched) {
266277
if (outcome.status !== "ok") {
267278
notices.push(
268279
`${target.file} could not be restored (${
@@ -292,15 +303,21 @@ async function repairWithheldRules(
292303
}
293304

294305
try {
295-
await writeRuleFile(cwd, rule as unknown as GeneratedRule);
306+
await writeRuleFile(cwd, rule);
296307
} catch (error) {
297308
const message = error instanceof Error ? error.message : String(error);
298309
notices.push(`${target.file} could not be written (${message}).`);
299310
continue;
300311
}
312+
// Says what was written, not what the directory now contains. The
313+
// delivered set is written over whatever is there; it does not remove a
314+
// file the set does not mention, so a stray capture left beside the rule
315+
// survives the repair. Claiming the rule "was restored" would overstate
316+
// that, and only `check.ts` is signed, so nothing here can vouch for the
317+
// rest of the directory. Tracked as #233.
301318
notices.push(
302-
`${target.file} was restored to the bytes the service blessed. It does ` +
303-
`not run in this pass; the next \`check\` reports the repaired ` +
319+
`${target.file} was rewritten with the bytes the service blessed. It ` +
320+
`does not run in this pass; the next \`check\` reports the repaired ` +
304321
`signature and is blessed through the ordinary path.`
305322
);
306323
}
@@ -500,6 +517,7 @@ export const checkCommand = defineCommand({
500517
const results = dispatched.results;
501518

502519
for (const notice of dispatched.notices) warn(`Notice: ${notice}`);
520+
const runNotices = [...plan.notices, ...dispatched.notices];
503521
for (const failure of dispatched.failures) warn(`Error: ${failure}`);
504522

505523
let errorCount = 0;
@@ -524,9 +542,12 @@ export const checkCommand = defineCommand({
524542
...(dispatched.failures.length > 0
525543
? { failures: dispatched.failures }
526544
: {}),
527-
...(dispatched.notices.length > 0
528-
? { notices: dispatched.notices }
529-
: {}),
545+
// BOTH sources. `plan.notices` carries the repair diagnostics —
546+
// what was restored, what could not be, and why — and they used to
547+
// reach only `warn()`, which is a no-op under `--json`. So the one
548+
// channel a CI run reads dropped the entire output of the feature
549+
// whose whole purpose is explaining a rule that did not run.
550+
...(runNotices.length > 0 ? { notices: runNotices } : {}),
530551
});
531552
console.log(JSON.stringify(output));
532553
} else {

‎packages/cli/src/rules/runtime/repair.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -106,9 +106,10 @@ export type RepairVerdict =
106106

107107
/** The `check.ts` entry of a restored file set, if it carries exactly one. */
108108
function restoredCheck(rule: RestoredRule): string | undefined {
109-
const files = (rule as { files?: { path: string; content: string }[] }).files;
110-
if (!Array.isArray(files)) return undefined;
111-
const matches = files.filter((file) => file.path === "check.ts");
109+
// `rule.files` directly: every variant of the union declares it, so a cast
110+
// here would re-declare a shape the generated types already know and absorb
111+
// a future schema change instead of failing the build.
112+
const matches = rule.files.filter((file) => file.path === "check.ts");
112113
return matches.length === 1 ? matches[0]?.content : undefined;
113114
}
114115

@@ -143,7 +144,10 @@ export async function verifyRestoredCheck(
143144
};
144145
}
145146

146-
const claimed = (rule as { signature?: string }).signature;
147+
// Read through the union rather than cast: `signature` is optional on the
148+
// `sg`/`vale` variants and required on `runtime`, which is the distinction
149+
// this function exists to enforce, so it must come from the types.
150+
const claimed = rule.signature;
147151
if (typeof claimed !== "string" || claimed === "") {
148152
// The published schema requires this on a runtime rule, so reaching here
149153
// means the service broke its own contract. Refuse rather than write bytes
Lines changed: 283 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,283 @@
1+
import { execFile } from "node:child_process";
2+
import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
3+
import { createServer, type Server } from "node:http";
4+
import { tmpdir } from "node:os";
5+
import { join, resolve } from "node:path";
6+
import { promisify } from "node:util";
7+
8+
import { afterEach, beforeEach, describe, expect, it } from "vitest";
9+
10+
import { canonicalHash } from "../src/rules/rule-hash";
11+
12+
const execFileAsync = promisify(execFile);
13+
const binPath = resolve(import.meta.dirname, "../dist/index.js");
14+
15+
/**
16+
* The repair path end to end: reconcile reports drift, restore answers, the
17+
* file is rewritten, and the run says so.
18+
*
19+
* `repair.test.ts` covers the decisions as pure functions. What it cannot see
20+
* is the wiring, and the wiring is where this feature failed review: every
21+
* repair notice reached `warn()` only, which is a no-op under `--json`, so the
22+
* one channel a CI run reads dropped the entire output of the thing whose
23+
* purpose is explaining a rule that did not run. A test at this level is what
24+
* would have caught it, so it is the first assertion below.
25+
*/
26+
27+
/** A mock serving both endpoints the repair path uses. */
28+
interface Mock {
29+
apiUrl: string;
30+
restoreCalls: string[];
31+
close: () => Promise<void>;
32+
}
33+
34+
function startMock(handlers: {
35+
reconcile: (body: {
36+
files: { file: string; signature: string }[];
37+
}) => unknown;
38+
restore: (ruleId: string) => { statusCode: number; body: unknown };
39+
}): Promise<Mock> {
40+
const restoreCalls: string[] = [];
41+
const server: Server = createServer((request, response) => {
42+
let raw = "";
43+
request.on("data", (chunk: Buffer) => (raw += chunk.toString()));
44+
request.on("end", () => {
45+
const url = request.url ?? "";
46+
if (url === "/cli/api/reconcile") {
47+
const body = handlers.reconcile(
48+
JSON.parse(raw) as { files: { file: string; signature: string }[] }
49+
);
50+
response.writeHead(200, { "content-type": "application/json" });
51+
response.end(JSON.stringify(body));
52+
return;
53+
}
54+
const restore = /^\/cli\/api\/rule\/([^/]+)\/restore$/.exec(url);
55+
if (restore) {
56+
const ruleId = decodeURIComponent(restore[1] ?? "");
57+
restoreCalls.push(ruleId);
58+
const { statusCode, body } = handlers.restore(ruleId);
59+
response.writeHead(statusCode, { "content-type": "application/json" });
60+
response.end(JSON.stringify(body));
61+
return;
62+
}
63+
response.writeHead(404).end("{}");
64+
});
65+
});
66+
return new Promise((resolvePromise) => {
67+
server.listen(0, "127.0.0.1", () => {
68+
const address = server.address();
69+
const port = typeof address === "object" && address ? address.port : 0;
70+
resolvePromise({
71+
apiUrl: `http://127.0.0.1:${String(port)}/cli`,
72+
restoreCalls,
73+
close: () => new Promise((done) => server.close(() => done())),
74+
});
75+
});
76+
});
77+
}
78+
79+
async function runCli(
80+
args: string[],
81+
env: Record<string, string>
82+
): Promise<{ stdout: string; exitCode: number }> {
83+
try {
84+
const { stdout } = await execFileAsync("node", [binPath, ...args], {
85+
env: { ...process.env, ...env },
86+
});
87+
return { stdout, exitCode: 0 };
88+
} catch (error) {
89+
const failure = error as { stdout: string; code: number };
90+
return { stdout: failure.stdout ?? "", exitCode: failure.code };
91+
}
92+
}
93+
94+
/** The `--json` envelope, ignoring any migration chatter before it. */
95+
function envelope(stdout: string): { notices?: string[] } {
96+
const line = stdout
97+
.trim()
98+
.split("\n")
99+
.findLast((l) => l.trim().startsWith("{"));
100+
return JSON.parse(line ?? "{}") as { notices?: string[] };
101+
}
102+
103+
const CAPTURE = [
104+
"id: logs-abc12345",
105+
"language: typescript",
106+
"rule:",
107+
" pattern: console.log($A)",
108+
"metadata:",
109+
" taskless:",
110+
" version: 1",
111+
" kind: runtime",
112+
" name: logs",
113+
" check: check.ts",
114+
" match: anchor",
115+
"",
116+
].join("\n");
117+
118+
const DRIFTED = "export default async () => [];\n";
119+
const BLESSED = "export default async function () {\n return [];\n}\n";
120+
const REPORTED = ".taskless/rules/runtime/demo/check.ts";
121+
122+
/** Reconcile reporting the reported check as drifted from `expected`. */
123+
const driftedReconcile = (expected: string) => () => ({
124+
run: [],
125+
unsafe: [{ file: REPORTED, expected, got: "1;h=sha-256;d=stale" }],
126+
unknown: [],
127+
missing: [],
128+
});
129+
130+
describe("repairing a drifted runtime rule, end to end", () => {
131+
let directory: string;
132+
let checkFile: string;
133+
134+
beforeEach(async () => {
135+
directory = await mkdtemp(join(tmpdir(), "tskl-repair-"));
136+
const rule = join(directory, ".taskless", "rules", "runtime", "demo");
137+
await mkdir(join(rule, "captures"), { recursive: true });
138+
await writeFile(join(rule, "captures", "logs.yml"), CAPTURE, "utf8");
139+
checkFile = join(rule, "check.ts");
140+
await writeFile(checkFile, DRIFTED, "utf8");
141+
await writeFile(join(directory, "src.ts"), 'console.log("hi");\n', "utf8");
142+
await execFileAsync("git", ["init"], { cwd: directory });
143+
await execFileAsync(
144+
"git",
145+
["remote", "add", "origin", "https://github.com/acme/widgets.git"],
146+
{ cwd: directory }
147+
);
148+
});
149+
150+
afterEach(async () => {
151+
await rm(directory, { recursive: true, force: true });
152+
});
153+
154+
it("reports the repair on the --json envelope, not only on stderr", async () => {
155+
// The regression this test exists for. `--json` is what CI reads, and the
156+
// repair notices used to reach `warn()` alone.
157+
const blessed = await canonicalHash(BLESSED);
158+
const mock = await startMock({
159+
reconcile: driftedReconcile(blessed),
160+
restore: () => ({
161+
statusCode: 200,
162+
body: {
163+
ruleId: "demo",
164+
rules: [
165+
{
166+
id: "demo",
167+
engine: "runtime",
168+
// The COMPLETE set. The schema calls `files` "every file the
169+
// rule directory must contain", and `writeRuleFile` enforces
170+
// that: a runtime rule restored without its captures would be
171+
// written, verify as incomplete, and never fire.
172+
files: [
173+
{ path: "check.ts", content: BLESSED },
174+
{ path: "captures/logs.yml", content: CAPTURE },
175+
],
176+
signature: blessed,
177+
},
178+
],
179+
},
180+
}),
181+
});
182+
try {
183+
const { stdout } = await runCli(["check", "-d", directory, "--json"], {
184+
TASKLESS_TOKEN: "fake.token",
185+
TASKLESS_API_URL: mock.apiUrl,
186+
});
187+
188+
const notices = envelope(stdout).notices ?? [];
189+
expect(notices.join("\n")).toContain(REPORTED);
190+
expect(notices.join("\n")).toMatch(/blessed/);
191+
expect(mock.restoreCalls).toEqual(["demo"]);
192+
193+
// And the bytes actually landed.
194+
await expect(readFile(checkFile, "utf8")).resolves.toBe(BLESSED);
195+
} finally {
196+
await mock.close();
197+
}
198+
});
199+
200+
it("refuses bytes that are not the ones reconcile blessed, and says so", async () => {
201+
// The service answers consistently — it signs exactly what it sends — and
202+
// sends a NEWER rule than the one we were owed. Nothing is written.
203+
const owed = await canonicalHash(BLESSED);
204+
const newer = "export default async () => [{ file: 'x' }];\n";
205+
const newerSignature = await canonicalHash(newer);
206+
const mock = await startMock({
207+
reconcile: driftedReconcile(owed),
208+
restore: () => ({
209+
statusCode: 200,
210+
body: {
211+
ruleId: "demo",
212+
rules: [
213+
{
214+
id: "demo",
215+
engine: "runtime",
216+
files: [
217+
{ path: "check.ts", content: newer },
218+
{ path: "captures/logs.yml", content: CAPTURE },
219+
],
220+
signature: newerSignature,
221+
},
222+
],
223+
},
224+
}),
225+
});
226+
try {
227+
const { stdout } = await runCli(["check", "-d", directory, "--json"], {
228+
TASKLESS_TOKEN: "fake.token",
229+
TASKLESS_API_URL: mock.apiUrl,
230+
});
231+
const notices = (envelope(stdout).notices ?? []).join("\n");
232+
expect(notices).toMatch(/was not restored/);
233+
expect(notices).toMatch(/does not upgrade/);
234+
await expect(readFile(checkFile, "utf8")).resolves.toBe(DRIFTED);
235+
} finally {
236+
await mock.close();
237+
}
238+
});
239+
240+
it("explains a rule the service will not return", async () => {
241+
const mock = await startMock({
242+
reconcile: driftedReconcile(await canonicalHash(BLESSED)),
243+
restore: () => ({ statusCode: 404, body: {} }),
244+
});
245+
try {
246+
const { stdout } = await runCli(["check", "-d", directory, "--json"], {
247+
TASKLESS_TOKEN: "fake.token",
248+
TASKLESS_API_URL: mock.apiUrl,
249+
});
250+
const notices = (envelope(stdout).notices ?? []).join("\n");
251+
expect(notices).toMatch(/could not be restored/);
252+
// A repair that cannot happen is a notice, never a failed run: the rule
253+
// stays withheld, which is already the safe state.
254+
await expect(readFile(checkFile, "utf8")).resolves.toBe(DRIFTED);
255+
} finally {
256+
await mock.close();
257+
}
258+
});
259+
260+
it("says why an unknown file cannot be restored, and asks for nothing", async () => {
261+
const mock = await startMock({
262+
reconcile: () => ({
263+
run: [],
264+
unsafe: [],
265+
unknown: [{ file: REPORTED }],
266+
missing: [],
267+
}),
268+
restore: () => ({ statusCode: 500, body: {} }),
269+
});
270+
try {
271+
const { stdout } = await runCli(["check", "-d", directory, "--json"], {
272+
TASKLESS_TOKEN: "fake.token",
273+
TASKLESS_API_URL: mock.apiUrl,
274+
});
275+
const notices = (envelope(stdout).notices ?? []).join("\n");
276+
expect(notices).toMatch(/was not issued by the rule service/);
277+
// Nothing on the server to ask for, so nothing is asked.
278+
expect(mock.restoreCalls).toEqual([]);
279+
} finally {
280+
await mock.close();
281+
}
282+
});
283+
});

0 commit comments

Comments
 (0)