fix(cli): route file-set/tests guard in rule create+improve through fail()

[thecodedrift]

Stack (root → tip):

• fix(cli): correct verify-schema docstrings to the current command form #318
  • ➡️ fix(cli): route file-set/tests guard in rule create+improve through fail() #319 (you are here)
    • test(cli): stop asserting the winner of a 1ms race in the Vale timeout test #320
      • fix(cli): skip an oversized file instead of letting Vale time out on it #323

Summary

rule create/rule improve write files as they process each generated rule, and refuse when a rule arrives as a file set (files) but also carries a stray tests field — that shape is unrepresentable in the published schema. Both guards (duplicated between create and improve, rules.ts ~:253-268 and ~:536-550) 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 the RULE_GENERATION_FAILED code the create-remote-rule recipe documents as a branch target was never actually reachable through this path.

What changed

• Both call sites now route through the command's existing fail(), which writes { ok: false, code, message } to stdout under --json and marks the error reported.
• The duplicated guard is consolidated into one fileSetTestsFieldError() helper shared by create and improve — two unmaintained copies is how they drift, and neither had a test before this one did.

Two things I decided deliberately

The duplication. Both copies check the exact same condition (isFileSetRule(rule) && rule.tests !== undefined) with the exact same remedy message, over the same GeneratedRule type — genuinely shared behavior, not two things that happen to look alike. Extracted to a pure function (fileSetTestsFieldError) that returns the message-or-undefined; each command still calls its own fail() with the result, since fail closes over that command's args.json/process.exitCode and isn't itself shareable without bigger surgery.

Rules written earlier in the loop. By the time the guard fires, writtenFiles already holds every rule file written earlier in this same delivery — the issue is right that the caller currently has no way to learn what landed on disk before the refusal. I judged this out of scope: the published --json error envelope (CLIErrorEnvelope in types/errors.ts — { ok, code, message }) has no field for a partial file list, and adding one is a schema/contract change, not a routing fix. (I also avoided touching packages/cli/src/schemas/ per the task's file boundaries.) Noted explicitly in the changeset so it isn't silently dropped.

Tests

Added packages/cli/test/rule-guard-json-envelope.test.ts, driving the actual command (via citty's own runCommand, argv-parsed exactly like the built CLI) for both rule create --json and rule improve --json against a mocked delivery carrying files + tests, asserting on the parsed envelope's code (RULE_GENERATION_FAILED), not just a non-zero exit — this is the exact seam the issue calls out: "a unit test proving the function throws correctly says nothing about whether the command reports it correctly."

Mutation check (both tests): reverted fail(strayTestsError, "RULE_GENERATION_FAILED") back to a bare throw new CLIError(...) at both call sites → both tests failed (expected undefined to be 1, i.e. process.exitCode was never set and no envelope was ever logged) → restored the fix → both tests passed again.

Verification

• pnpm typecheck — clean
• pnpm exec vitest run (full packages/cli suite) — 1371/1372 passing; the one failure (vale-run.test.ts timeout-blocking test, a 1ms-timeout race) is pre-existing and unrelated — reproduced in isolation and it passes on its own, confirmed not touched by this change
• pnpm lint (build + house-style check) — clean
• Built the CLI (pnpm build) and ran it end-to-end against a local mock API server with a payload carrying both files and tests:

  {"ok":false,"code":"RULE_GENERATION_FAILED","message":"Rule \"guard-test-rule\" was delivered as a file set and also carries `tests`; a file set's fixtures belong in its own `.tests/` files."}

  Confirmed this parses as JSON, exit code 1, and that guard-test-rule.yml was in fact written to .taskless/rules/sg/guard-test-rule/ before the refusal — illustrating the "rules already written" gap called out above.

Changeset

One changeset, patch (pre-1.0, per repo convention).

Fixes #280

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 4m 39s —— View job

---

Review complete

• Read .prior-review.json
• Gather PR diff and context
• Review rules.ts changes
• Review new test file
• Review changeset
• Post final review

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contains no reviewThreads, reviews, or top-level comments beyond the @claude /review trigger itself, so this is the first review of this PR — nothing to classify as already-addressed.

Summary

The core fix is sound. I traced it end to end against src/index.ts's top-level error handler: previously the guard threw a bare CLIError (no reported: true), so under --json writeJsonError was never called — stdout stayed empty while index.ts's catch-all printed prose to stderr and set exitCode = 1, exactly the "indistinguishable from a crash" behavior described. Routing both call sites through the command's own fail() (which calls writeJsonError before throwing a reported: true CLIError) fixes that, and extracting fileSetTestsFieldError() removes the duplicated guard cleanly — same condition, same remedy, no behavior drift. The delete command's diff is comment-only (explaining the deliberate stdout silence on success), no logic change there.

I verified the new test's mocked HTTP surface against the real code paths (iterateRule's /cli/api/request/{requestId}/iterate endpoint, TASKLESS_TOKEN/TASKLESS_API_URL env vars, resolveIdentity) — all correctly wired to the actual implementation, and runCommand + asserting on the parsed envelope's code is a meaningfully stronger test than unit-testing the guard function in isolation, as the PR description argues.

One finding, posted inline: test/rule-guard-json-envelope.test.ts's afterEach doesn't restore process.exitCode after fail() sets it to 1 on the real process. test/agent-routing-telemetry.test.ts already documents and guards against this exact hazard ("it has to be put back or one failing topic would fail the whole run") — this new file doesn't follow that precedent, and could leave the vitest worker's exit code poisoned after these tests run regardless of whether their assertions pass.

Everything else — the changeset wording, the GeneratedRule type import, the guard's semantics being preserved verbatim through the extraction — checked out.
· branch fix/rule-guard-json-envelope

[claude]

[New] process.exitCode is set to 1 by fail() (a real mutation on the actual process object the whole vitest worker runs in, not something stubbed here) and both tests assert expect(process.exitCode).toBe(1), but this afterEach never puts it back.

This codebase already has a named guard for exactly this hazard: test/agent-routing-telemetry.test.ts captures the prior value in beforeEach and restores it in afterEach, with the comment "The command sets process.exitCode on the process the suite runs in, so it has to be put back or one failing topic would fail the whole run." That reasoning applies identically here — after this file's tests run, the real Node process (or its vitest worker) is left with exitCode = 1, which can surface as a "worker exited with a non-zero exit code" failure or a spuriously-failing overall test run, independent of whether every assertion in this file passed.

Suggest capturing process.exitCode in beforeEach and restoring it in afterEach, the same way agent-routing-telemetry.test.ts does.