From b8c7eec7b8a8b4ccc3b40e9642ec095becfb07f0 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 8 Sep 2026 11:36:52 -0700 Subject: [PATCH 1/2] fix(cli): route the file-set/tests guard in rule create+improve through fail() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard refusing a file-set rule that also carries a stray `tests` field threw a bare `CLIError` from inside the command's own `try`, never touching the command's `fail()` helper. Under `--json` that produced no envelope at all: stdout empty, prose on stderr, exit 1 — indistinguishable from a crash, and RULE_GENERATION_FAILED (which the create-remote-rule recipe documents as a branch target) was never actually reachable through this path. Both call sites (rules.ts create and improve) now call fail(), which writes the { ok: false, code, message } envelope under --json and marks the error reported. The duplicated guard is also consolidated into a single fileSetTestsFieldError() helper, since two unmaintained copies is how they drift. Rules already written to disk earlier in the same delivery loop are still not named in the failure envelope — the published envelope shape has no field for a partial file list, and extending it is a schema change out of scope here. Fixes #280 --- .changeset/rule-guard-json-envelope.md | 20 ++ packages/cli/src/commands/rules.ts | 94 ++++---- .../cli/test/rule-guard-json-envelope.test.ts | 200 ++++++++++++++++++ 3 files changed, 276 insertions(+), 38 deletions(-) create mode 100644 .changeset/rule-guard-json-envelope.md create mode 100644 packages/cli/test/rule-guard-json-envelope.test.ts diff --git a/.changeset/rule-guard-json-envelope.md b/.changeset/rule-guard-json-envelope.md new file mode 100644 index 00000000..4338c6ff --- /dev/null +++ b/.changeset/rule-guard-json-envelope.md @@ -0,0 +1,20 @@ +--- +"@taskless/cli": patch +--- + +`rule create --json` and `rule improve --json` now emit the standard `{ ok: +false, code, message }` envelope on stdout when a file-set rule arrives with +a stray `tests` field, instead of throwing a bare, unreported `CLIError`. +Previously the guard threw from inside the command's own `try` without going +through the command's `fail()` helper, so under `--json` nothing was written +to stdout at all — prose landed on stderr and the process exited 1, +indistinguishable from a crash, and the `RULE_GENERATION_FAILED` code the +`create-remote-rule` recipe documents as a branch target was never actually +reachable for this guard. Both call sites now route through `fail()`, and the +duplicated guard itself was consolidated into one shared check so the two +copies cannot drift again silently. + +Not addressed here: rules written to disk earlier in the same delivery loop +(before the guard fires) are still not named in the failure envelope. The +published envelope shape (`CLIErrorEnvelope`) has no field for a partial file +list, and adding one is a schema change out of scope for this fix. diff --git a/packages/cli/src/commands/rules.ts b/packages/cli/src/commands/rules.ts index a0ff6da2..f0c59e1f 100644 --- a/packages/cli/src/commands/rules.ts +++ b/packages/cli/src/commands/rules.ts @@ -10,6 +10,7 @@ import { pollRuleStatus, iterateRule, isSingleContentRule, + type GeneratedRule, } from "../api/rules"; import { writeRuleFile, @@ -44,6 +45,35 @@ function getTimestamp(): string { const POLL_INTERVAL_MS = 15_000; +/** + * A file set rule carries its fixtures as ordinary files under `.tests/`, + * already written by `writeRuleFile`. Only the single-content envelope has a + * separate `tests` field 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 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. + * + * `create` and `improve` both run this guard over the same + * `GeneratedRule` shape with the same remedy, so it is shared here rather + * than copied: two unmaintained copies is how they drift, and neither had a + * test before this one did. + */ +function fileSetTestsFieldError(rule: GeneratedRule): string | undefined { + if ( + !isSingleContentRule(rule) && + (rule as { tests?: unknown }).tests !== undefined + ) { + return ( + `Rule "${rule.id}" was delivered as a file set and also carries \`tests\`; ` + + `a file set's fixtures belong in its own \`.tests/\` files.` + ); + } + return undefined; +} + const createCommand = defineCommand({ meta: { name: "create", @@ -246,25 +276,19 @@ const createCommand = defineCommand({ }); writtenFiles.push(ruleFile); - // 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" - ); + // See `fileSetTestsFieldError` for why this is a guard rather + // than a silent drop. + const strayTestsError = fileSetTestsFieldError(rule); + if (strayTestsError !== undefined) { + // Route through `fail()`, not a bare throw: this is inside + // the command's own `try`, and a throw here that never + // touches `fail()` skips the `--json` envelope entirely (see + // #280). `writtenFiles` already holds every rule file written + // earlier in this loop, but the envelope shape this command + // publishes has no field to carry a partial file list on + // failure — extending it is a schema change, out of scope + // here (see the PR description). + fail(strayTestsError, "RULE_GENERATION_FAILED"); } if (isSingleContentRule(rule) && rule.tests) { const testFile = await writeRuleTestFile(cwd, rule, timestamp); @@ -523,25 +547,19 @@ const improveCommand = defineCommand({ }); writtenFiles.push(ruleFile); - // 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" - ); + // See `fileSetTestsFieldError` for why this is a guard rather + // than a silent drop. + const strayTestsError = fileSetTestsFieldError(rule); + if (strayTestsError !== undefined) { + // Route through `fail()`, not a bare throw: this is inside + // the command's own `try`, and a throw here that never + // touches `fail()` skips the `--json` envelope entirely (see + // #280). `writtenFiles` already holds every rule file written + // earlier in this loop, but the envelope shape this command + // publishes has no field to carry a partial file list on + // failure — extending it is a schema change, out of scope + // here (see the PR description). + fail(strayTestsError, "RULE_GENERATION_FAILED"); } if (isSingleContentRule(rule) && rule.tests) { const testFile = await writeRuleTestFile(cwd, rule, timestamp); diff --git a/packages/cli/test/rule-guard-json-envelope.test.ts b/packages/cli/test/rule-guard-json-envelope.test.ts new file mode 100644 index 00000000..4c2d3726 --- /dev/null +++ b/packages/cli/test/rule-guard-json-envelope.test.ts @@ -0,0 +1,200 @@ +import { execFileSync } from "node:child_process"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + vi, + type MockInstance, +} from "vitest"; + +import { runCommand } from "citty"; + +import { ruleCommand } from "../src/commands/rules"; + +/** + * #280: the guard refusing a file-set rule that also carries a stray `tests` + * field threw a bare `CLIError` from inside the command's own `try`, never + * touching `fail()`. Under `--json` that skipped the envelope entirely — + * stdout empty, prose on stderr, exit 1, indistinguishable from a crash. + * + * These tests drive the ACTUAL command (via citty's own `runCommand`, which + * parses argv exactly like the built CLI does) rather than the guard function + * in isolation, because a unit test proving the function throws correctly + * says nothing about whether the command reports it correctly — that is + * exactly the seam #280 slipped through. + */ +describe("rule create/improve --json: file-set rule with a stray `tests` field", () => { + let cwd: string; + let logSpy: MockInstance<(...data: unknown[]) => void>; + + const requestId = "11111111-1111-1111-1111-111111111111"; + const iterateRequestId = "22222222-2222-2222-2222-222222222222"; + + // A minimal ast-grep file-set delivery for engine "sg": one file at + // `.yml` (the only file `ENGINE_LAYOUTS.sg` requires), plus the stray + // `tests` field the schema says a file set must never carry. + const badRule = { + id: "guard-test-rule", + engine: "sg", + files: [ + { + path: "guard-test-rule.yml", + content: + "id: guard-test-rule\nlanguage: TypeScript\nrule:\n pattern: foo\n", + }, + ], + tests: { valid: ["const x = 1;"], invalid: ["foo();"] }, + }; + + function stubFetch(pollRequestId: string): void { + const fetchMock = vi.fn( + (input: string | URL | Request, init?: RequestInit) => { + const url = new URL( + typeof input === "string" + ? input + : input instanceof URL + ? input.href + : input.url + ); + const method = ( + init?.method ?? (input instanceof Request ? input.method : "GET") + ).toUpperCase(); + const { pathname } = url; + + if (pathname === "/cli/api/whoami") { + // Swallowed by fetchWhoami; resolveOrgSubject falls back to the + // token-claim path. Not what this test is about. + return Response.json({}, { status: 500 }); + } + if (method === "POST" && pathname === "/cli/api/request") { + return Response.json({ requestId }, { status: 200 }); + } + if ( + method === "GET" && + pathname === `/cli/api/request/${pollRequestId}` + ) { + return Response.json( + { status: "generated", rules: [badRule] }, + { status: 200 } + ); + } + if ( + method === "POST" && + pathname === "/cli/api/request/guard-test-rule/iterate" + ) { + return Response.json( + { requestId: iterateRequestId }, + { status: 200 } + ); + } + throw new Error(`unexpected ${method} ${pathname}`); + } + ); + vi.stubGlobal("fetch", fetchMock); + } + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "taskless-rule-guard-")); + await mkdir(join(cwd, ".taskless"), { recursive: true }); + await writeFile( + join(cwd, ".taskless", "taskless.json"), + JSON.stringify({ + version: "2026-03-03", + orgId: 123, + repositoryUrl: "https://github.com/test/test", + }) + ); + // resolveRepositoryUrl shells out to `git`; give it a real repo to read + // rather than mocking the module, so the test exercises the same path + // the built CLI does. + execFileSync("git", ["init"], { cwd }); + execFileSync( + "git", + ["remote", "add", "origin", "https://github.com/test/test.git"], + { cwd } + ); + + process.env.TASKLESS_TOKEN = "test-token"; + process.env.TASKLESS_API_URL = "https://example.invalid/cli"; + + logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); + // The command's poll loop always waits `POLL_INTERVAL_MS` (15s) before + // its first status check, real or mocked. Stubbing `setTimeout` to fire + // immediately collapses that wait to nothing without needing to reach + // into (or export) the command's private constant. + vi.stubGlobal( + "setTimeout", + (function_: (...arguments_: unknown[]) => void) => { + function_(); + return 0 as unknown as NodeJS.Timeout; + } + ); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + delete process.env.TASKLESS_TOKEN; + delete process.env.TASKLESS_API_URL; + await rm(cwd, { recursive: true, force: true }); + }); + + /** Envelope is the JSON blob most recently written to stdout via console.log. */ + function lastEnvelope(): { ok: boolean; code?: string; message?: string } { + const calls = logSpy.mock.calls; + const lastCall = calls.at(-1); + if (!lastCall) throw new Error("console.log was never called"); + return JSON.parse(String(lastCall[0])) as { + ok: boolean; + code?: string; + message?: string; + }; + } + + it("rule create --json: reports RULE_GENERATION_FAILED as an envelope on stdout, not a bare throw", async () => { + stubFetch(requestId); + const requestFile = join(cwd, "request.json"); + await writeFile(requestFile, JSON.stringify({ prompt: "add a rule" })); + + const runPromise = runCommand(ruleCommand, { + rawArgs: ["create", "--from", requestFile, "--json", "-d", cwd], + }); + + await expect(runPromise).rejects.toThrow(); + expect(process.exitCode).toBe(1); + + const envelope = lastEnvelope(); + expect(envelope.ok).toBe(false); + expect(envelope.code).toBe("RULE_GENERATION_FAILED"); + expect(envelope.message).toContain("guard-test-rule"); + expect(envelope.message).toContain("tests"); + }); + + it("rule improve --json: reports RULE_GENERATION_FAILED as an envelope on stdout, not a bare throw", async () => { + stubFetch(iterateRequestId); + const requestFile = join(cwd, "improve-request.json"); + await writeFile( + requestFile, + JSON.stringify({ ruleId: "guard-test-rule", guidance: "tighten it" }) + ); + + const runPromise = runCommand(ruleCommand, { + rawArgs: ["improve", "--from", requestFile, "--json", "-d", cwd], + }); + + await expect(runPromise).rejects.toThrow(); + expect(process.exitCode).toBe(1); + + const envelope = lastEnvelope(); + expect(envelope.ok).toBe(false); + expect(envelope.code).toBe("RULE_GENERATION_FAILED"); + expect(envelope.message).toContain("guard-test-rule"); + expect(envelope.message).toContain("tests"); + }); +}); From f2bfb6364b6754da9ef6970913aaa0b56a4b0c64 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 8 Sep 2026 12:26:44 -0700 Subject: [PATCH 2/2] docs(cli): explain why rule delete --json stays silent on success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A coordinator review flagged the delete branch's silence under --json as the same silent-success shape as #280's guard bug, and asked me to add a success envelope. Before landing that, I found test/error-envelope.test.ts already asserts the opposite as correct ("is silent on stdout when a real rule is deleted in --json mode"), introduced deliberately in 07c0d3c. The cli-check and cli-auth OpenSpec specs both say the standardized envelope applies "when ... exits with an error" — it is an error-only envelope. create/improve/ meta print on success because they have a payload to return; delete (like auth logout) does not, so silence is correct, not a bug. No behavior change. Adds a comment at the call site recording why, so the next reader (or reviewer) doesn't have to re-derive it. --- packages/cli/src/commands/rules.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/cli/src/commands/rules.ts b/packages/cli/src/commands/rules.ts index f0c59e1f..a4cb2df2 100644 --- a/packages/cli/src/commands/rules.ts +++ b/packages/cli/src/commands/rules.ts @@ -749,6 +749,16 @@ const deleteCommand = defineCommand({ try { const result = await deleteRuleFiles(cwd, id); if (result.outcome === "deleted") { + // Silent on stdout under `--json`, deliberately, not an oversight. + // The spec's `{ ok:false, code, message }` envelope is documented as + // an ERROR envelope ("... exits with an error" — see + // openspec/specs/cli-check/spec.md and cli-auth/spec.md), not a + // general success/failure wrapper: `create`/`improve`/`meta` print on + // success because they have a payload to hand back (generated rules, + // metadata), and `delete` does not. `auth logout` is the same shape + // for the same reason. Introduced this way in 07c0d3c; see + // test/error-envelope.test.ts's "is silent on stdout when a real rule + // is deleted in --json mode". if (!args.json) { console.log(`Deleted rule "${id}" and associated test files.`); }