From 3234f040ff0d7152602b32ad7647978802983dd2 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 31 Aug 2026 21:47:04 -0700 Subject: [PATCH] feat(api): regenerate types for the delivery union, and narrow on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 5.8. The generator's file-set tier is live, so `src/generated/api.d.ts` is regenerated from the published schema. `rules` is now a discriminated union — single-content, plus one file-set variant per engine — where it used to be one shape with optional fields. The runtime variant states `signature` as REQUIRED, which is the change we asked for: a runtime rule executes only against a blessed signature, and an unsigned one is a rule that will be written, verify clean, and never run. That union broke six sites reading `.content` and `.tests` off a rule without asking which variant it was. They are narrowed rather than cast: `isFileSetRule` and `isSingleContentRule` key on `files`, because `engine` is what the file-set variants have in common and `files` is what separates them from the single-content one. The "carries both `files` and `content`" check stays, though the union now makes that unrepresentable. The type states what the service promises; the check defends against it breaking that promise, which is the only reason a client validates a payload at all. Fixes one case that reached the filesystem. A payload carrying NEITHER `files` nor `content` fell through to the single-content branch and handed `yaml.stringify` an `undefined`, which returns the string "undefined" rather than throwing — so the rule file was written and what it contained was that word. Refused now, before the directory is created, checked on `content` itself rather than on "not a file set", since a payload with neither is not a file set either. --- .changeset/generator-payload-alignment.md | 15 ++ packages/cli/src/api/rules.ts | 35 +++ packages/cli/src/commands/rules.ts | 17 +- packages/cli/src/generated/api.d.ts | 281 +++++++++++++++++----- packages/cli/src/rules/files.ts | 25 +- packages/cli/test/deliver.test.ts | 14 ++ 6 files changed, 321 insertions(+), 66 deletions(-) diff --git a/.changeset/generator-payload-alignment.md b/.changeset/generator-payload-alignment.md index 7b2c2a03..bf263856 100644 --- a/.changeset/generator-payload-alignment.md +++ b/.changeset/generator-payload-alignment.md @@ -95,3 +95,18 @@ differing only in case, which are one file on a case-insensitive filesystem), and one path being an ancestor of another. The whole set is assessed as a unit, so a refused delivery leaves no directory behind rather than a half-written rule that verifies as broken two steps from the cause. + +The generated API types now carry the delivery union, and the client narrows +on it. + +`rules` is published as `SingleContent | Sg | Vale | Runtime` rather than one +shape with optional fields, so a runtime file set states `signature` as +required. Reading `content` or `tests` off a rule no longer type-checks +without asking which variant arrived, which is the property doing its job: +the client cannot treat an unsigned runtime rule as deliverable. + +It also closes a case that reached the filesystem. A payload carrying neither +`files` nor `content` fell through to the single-content branch and handed +`yaml.stringify` an `undefined`, which returns the string `"undefined"` +rather than throwing. The rule file was created and its contents were that +word. It is now refused before the directory exists. diff --git a/packages/cli/src/api/rules.ts b/packages/cli/src/api/rules.ts index 5ec2c840..56adafab 100644 --- a/packages/cli/src/api/rules.ts +++ b/packages/cli/src/api/rules.ts @@ -13,6 +13,41 @@ export type GeneratedRule = NonNullable[number]; /** Sidecar metadata keyed by rule filename */ export type RuleMetadata = NonNullable; +/** + * A rule delivered as a file set, discriminated on `engine`. + * + * The service now publishes `rules` as a union rather than one shape with + * optional fields, so `rule.content` no longer type-checks without asking + * which variant this is. That is the schema working: a runtime file set + * REQUIRES a signature, and a type that let every field be read off any + * variant could not express that. + */ +export type DeliveredFileSetRule = Extract; + +/** A rule delivered as one `content` object, the pre-file-set envelope. */ +export type SingleContentRule = Exclude; + +/** + * Whether this rule arrived as a file set. + * + * Keyed on `files` rather than on `engine`, because `engine` is what the + * file-set variants have in COMMON and `files` is what separates them from the + * single-content one. Narrowing on the wrong field reads as equivalent and + * silently admits a shape the branch cannot handle. + */ +export function isFileSetRule( + rule: GeneratedRule +): rule is DeliveredFileSetRule { + return (rule as { files?: unknown }).files !== undefined; +} + +/** Whether this rule arrived as a single `content` object. */ +export function isSingleContentRule( + rule: GeneratedRule +): rule is SingleContentRule { + return !isFileSetRule(rule); +} + // --- Helpers --- /** Extract error details from an untyped error response body */ diff --git a/packages/cli/src/commands/rules.ts b/packages/cli/src/commands/rules.ts index 7c7d4420..0b4f2852 100644 --- a/packages/cli/src/commands/rules.ts +++ b/packages/cli/src/commands/rules.ts @@ -5,7 +5,12 @@ import { defineCommand } from "citty"; import { ZodError } from "zod"; import { identityFailureCode, resolveIdentity } from "../auth/identity"; -import { submitRule, pollRuleStatus, iterateRule } from "../api/rules"; +import { + submitRule, + pollRuleStatus, + iterateRule, + isSingleContentRule, +} from "../api/rules"; import { writeRuleFile, writeRuleTestFile, @@ -238,7 +243,10 @@ const createCommand = defineCommand({ const ruleFile = await writeRuleFile(cwd, rule); writtenFiles.push(ruleFile); - if (rule.tests) { + // 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. + if (isSingleContentRule(rule) && rule.tests) { const testFile = await writeRuleTestFile(cwd, rule, timestamp); writtenFiles.push(testFile); } @@ -475,7 +483,10 @@ const improveCommand = defineCommand({ const ruleFile = await writeRuleFile(cwd, rule); writtenFiles.push(ruleFile); - if (rule.tests) { + // 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. + if (isSingleContentRule(rule) && rule.tests) { const testFile = await writeRuleTestFile(cwd, rule, timestamp); writtenFiles.push(testFile); } diff --git a/packages/cli/src/generated/api.d.ts b/packages/cli/src/generated/api.d.ts index 40e558e0..1958119b 100644 --- a/packages/cli/src/generated/api.d.ts +++ b/packages/cli/src/generated/api.d.ts @@ -39,8 +39,6 @@ export interface paths { id: string; /** @description Organization name */ name: string; - /** @description GitHub App installation ID */ - installationId: number; /** * @description Identity provider * @constant @@ -158,66 +156,123 @@ export interface paths { | "merged" | "closed" | "unsupported"; - /** @description Generated rules (present when status is generated) */ - rules?: { - /** @description Rule identifier (matches content.id) */ - id: string; - /** @description The ast-grep rule definition */ - content: { - /** @description Unique rule identifier, e.g. no-unused-variable */ - id: string; - /** @description Language to parse, e.g. typescript */ - language: string; - /** @description Rule object to find matching AST nodes */ - rule: { - [key: string]: unknown; - }; - /** - * @description Severity level - * @enum {string} - */ - severity?: "hint" | "info" | "warning" | "error" | "off"; - /** @description Message explaining why the rule fired */ - message?: string; - /** @description Additional notes to elaborate the message */ - note?: string; - /** @description Auto-fix pattern or object */ - fix?: - | string - | { + /** @description Generated rules (present when status is generated). Clients below the `file-set` floor receive `content`; clients at or above it receive `files`. */ + rules?: + | { + /** @description Rule identifier (matches content.id) */ + id: string; + /** @description The ast-grep rule definition */ + content: { + /** @description Unique rule identifier, e.g. no-unused-variable */ + id: string; + /** @description Language to parse, e.g. typescript */ + language: string; + /** @description Rule object to find matching AST nodes */ + rule: { [key: string]: unknown; }; - /** @description Meta variable constraints */ - constraints?: { - [key: string]: unknown; - }; - /** @description Reusable utility rules */ - utils?: { - [key: string]: unknown; - }; - /** @description Meta variable transformations */ - transform?: { - [key: string]: unknown; - }; - /** @description Extra rule metadata */ - metadata?: { - [key: string]: unknown; - }; - /** @description Glob patterns the rule applies to */ - files?: string[]; - /** @description Glob patterns to exclude */ - ignores?: string[]; - /** @description Documentation link */ - url?: string; - }; - /** @description Test cases for the rule */ - tests?: { - /** @description Code that should NOT trigger the rule */ - valid: string[]; - /** @description Code that SHOULD trigger the rule */ - invalid: string[]; - }; - }[]; + /** + * @description Severity level + * @enum {string} + */ + severity?: "hint" | "info" | "warning" | "error" | "off"; + /** @description Message explaining why the rule fired */ + message?: string; + /** @description Additional notes to elaborate the message */ + note?: string; + /** @description Auto-fix pattern or object */ + fix?: + | string + | { + [key: string]: unknown; + }; + /** @description Meta variable constraints */ + constraints?: { + [key: string]: unknown; + }; + /** @description Reusable utility rules */ + utils?: { + [key: string]: unknown; + }; + /** @description Meta variable transformations */ + transform?: { + [key: string]: unknown; + }; + /** @description Extra rule metadata */ + metadata?: { + [key: string]: unknown; + }; + /** @description Glob patterns the rule applies to */ + files?: string[]; + /** @description Glob patterns to exclude */ + ignores?: string[]; + /** @description Documentation link */ + url?: string; + }; + /** @description Test cases for the rule */ + tests?: { + /** @description Code that should NOT trigger the rule */ + valid: string[]; + /** @description Code that SHOULD trigger the rule */ + invalid: string[]; + }; + }[] + | ( + | { + /** @description The rule directory name under .taskless/rules// */ + id: string; + /** @description Every file the rule directory must contain */ + files: { + /** @description Path relative to .taskless/rules/// */ + path: string; + /** @description The file’s exact bytes */ + content: string; + }[]; + /** + * @description ast-grep — inert declarative rules + * @constant + */ + engine: "sg"; + /** @description Blessed canonical signature, when one was registered */ + signature?: string; + } + | { + /** @description The rule directory name under .taskless/rules// */ + id: string; + /** @description Every file the rule directory must contain */ + files: { + /** @description Path relative to .taskless/rules/// */ + path: string; + /** @description The file’s exact bytes */ + content: string; + }[]; + /** + * @description Vale — inert prose/markup rules + * @constant + */ + engine: "vale"; + /** @description Blessed canonical signature, when one was registered */ + signature?: string; + } + | { + /** @description The rule directory name under .taskless/rules// */ + id: string; + /** @description Every file the rule directory must contain */ + files: { + /** @description Path relative to .taskless/rules/// */ + path: string; + /** @description The file’s exact bytes */ + content: string; + }[]; + /** + * @description Executable — check.ts runs against a file tree + * @constant + */ + engine: "runtime"; + /** @description REQUIRED. Execution is gated on this signature, so a runtime rule without one could never run. */ + signature: string; + } + )[]; /** @description Sidecar metadata keyed by rule filename (present when rules are present) */ meta?: { [key: string]: { @@ -307,6 +362,112 @@ export interface paths { patch?: never; trace?: never; }; + "/cli/api/rule/{ruleId}/restore": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + /** Restore the complete on-disk file set for a rule the caller’s organization owns */ + post: { + parameters: { + query?: never; + header?: never; + path: { + ruleId: string; + }; + cookie?: never; + }; + /** @description OK */ + requestBody?: { + content: { + "application/json": { + /** @description Full repository URL the rule was generated for */ + repositoryUrl: string; + }; + }; + }; + responses: { + /** @description OK */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": { + /** @description The ticket id the rules were generated under */ + ruleId: string; + /** @description The complete file set for each rule the ticket produced */ + rules: ( + | { + /** @description The rule directory name under .taskless/rules// */ + id: string; + /** @description Every file the rule directory must contain */ + files: { + /** @description Path relative to .taskless/rules/// */ + path: string; + /** @description The file’s exact bytes */ + content: string; + }[]; + /** + * @description ast-grep — inert declarative rules + * @constant + */ + engine: "sg"; + /** @description Blessed canonical signature, when one was registered */ + signature?: string; + } + | { + /** @description The rule directory name under .taskless/rules// */ + id: string; + /** @description Every file the rule directory must contain */ + files: { + /** @description Path relative to .taskless/rules/// */ + path: string; + /** @description The file’s exact bytes */ + content: string; + }[]; + /** + * @description Vale — inert prose/markup rules + * @constant + */ + engine: "vale"; + /** @description Blessed canonical signature, when one was registered */ + signature?: string; + } + | { + /** @description The rule directory name under .taskless/rules// */ + id: string; + /** @description Every file the rule directory must contain */ + files: { + /** @description Path relative to .taskless/rules/// */ + path: string; + /** @description The file’s exact bytes */ + content: string; + }[]; + /** + * @description Executable — check.ts runs against a file tree + * @constant + */ + engine: "runtime"; + /** @description REQUIRED. Execution is gated on this signature, so a runtime rule without one could never run. */ + signature: string; + } + )[]; + }; + }; + }; + }; + }; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/cli/api/reconcile": { parameters: { query?: never; diff --git a/packages/cli/src/rules/files.ts b/packages/cli/src/rules/files.ts index 045eb9f9..38cbcdd1 100644 --- a/packages/cli/src/rules/files.ts +++ b/packages/cli/src/rules/files.ts @@ -4,6 +4,7 @@ import { basename, join, resolve } from "node:path"; import { parse, stringify } from "yaml"; import { ensureTasklessDirectory } from "../filesystem/directory"; +import { isSingleContentRule } from "../api/rules"; import type { GeneratedRule, RuleMetadata } from "../api/rules"; import { resolveIngestEngine, @@ -43,7 +44,11 @@ export async function writeRuleFile( throw new Error(`Rule "${rule.id}" ${delivered.reason}.`); } if (delivered.kind === "present") { - if (rule.content !== undefined) { + // The published union makes this unrepresentable, and the check stays. + // The type states what the service PROMISES; this defends against it + // breaking that promise, which is the only reason the client validates a + // payload at all. + if ((rule as { content?: unknown }).content !== undefined) { throw new Error( `Rule "${rule.id}" carries both \`files\` and \`content\`; they are mutually exclusive.` ); @@ -63,6 +68,17 @@ export async function writeRuleFile( return ruleFilePath(cwd, engine, rule.id); } + // `files` is absent past the branch above, so this must be the single-content + // envelope. Checked BEFORE anything is created, and checked on `content` + // itself rather than on "not a file set": a payload carrying neither is not a + // file set either, so the negative would admit it. It used to, and `yaml` + // renders `undefined` as the STRING "undefined" instead of throwing, so the + // rule file was written and what it contained was that word. + if (!isSingleContentRule(rule) || rule.content === undefined) { + throw new Error( + `Rule "${rule.id}" carries neither \`files\` nor \`content\`.` + ); + } await ensureTasklessDirectory(cwd); await mkdir(ruleDirectory(cwd, engine, rule.id), { recursive: true }); const filePath = ruleFilePath(cwd, engine, rule.id); @@ -87,10 +103,13 @@ export async function writeRuleTestFile( const directory = ruleTestsDirectory(cwd, engine, rule.id); await mkdir(directory, { recursive: true }); const filePath = join(directory, `${rule.id}-${timestamp}-test.yml`); + // Only the single-content envelope carries `tests`; a file set delivers its + // fixtures as ordinary files under `.tests/`. + const tests = isSingleContentRule(rule) ? rule.tests : undefined; const content = { id: rule.id, - valid: rule.tests?.valid ?? [], - invalid: rule.tests?.invalid ?? [], + valid: tests?.valid ?? [], + invalid: tests?.invalid ?? [], }; await writeFile(filePath, stringify(content, { lineWidth: 0 }), "utf8"); return filePath; diff --git a/packages/cli/test/deliver.test.ts b/packages/cli/test/deliver.test.ts index 3b46a147..11f36a5f 100644 --- a/packages/cli/test/deliver.test.ts +++ b/packages/cli/test/deliver.test.ts @@ -250,6 +250,20 @@ describe("delivering a rule as a file set", () => { ); }); + it("refuses a payload carrying neither files nor content", async () => { + // Before the published union forced the variants apart, this fell through + // to the single-content branch and handed `stringify` an `undefined`, + // which returns the STRING "undefined" rather than throwing. The rule file + // was written, and what it contained was the word undefined. + const rule = { id: "no-eval-abc12345" } as unknown as GeneratedRule; + await expect(writeRuleFile(cwd, rule)).rejects.toThrow( + /neither `files` nor `content`/ + ); + expect(existsSync(ruleDirectory(cwd, "sg", "no-eval-abc12345"))).toBe( + false + ); + }); + it("still writes a legacy single-content payload", async () => { // The envelope every published CLI receives, and will keep receiving. const rule = {