feat(api): regenerate types for the delivery union, and narrow on it - #229
Conversation
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.
|
@claude /review |
|
Claude finished @thecodedrift's task in 2m 50s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
Findings[New] [New] Same line — a dead disjunct. [New] [New] Pre-existing stale comment, not part of this diff but directly contradicted by it — Not raised as findings (verified and deemed non-issues): Everything else in the diff — the discriminated-union narrowing ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review feedback on #229, applied here because #229 had already merged. `content: null` reproduced the exact bug #229 says it closes. The guard tested `content === undefined`, and `yaml` does not throw on a value it cannot make a document of, it renders one: `undefined` becomes the string "undefined" and `null` becomes the string "null". Either way the rule file is created and its entire contents are that word. A string or a number does the same. The test is now "a usable object" rather than "not undefined", and the three cases are covered. Confirmed by restoring the old guard and watching all three fail. Worth naming why two checks in this function disagree about `null`, and why that is correct. The mutual-exclusion check asks what the payload CLAIMS, so any present `content` — `null` included — means the service sent both envelopes. This one asks what can be WRITTEN. Collapsing them into one predicate would make one of the two wrong. Also drops a dead disjunct the reviewer spotted: `!isSingleContentRule` could never be true there, since `files` is provably absent by that point and the helper is defined as its negation. And a file set arriving with a stray `tests` now fails loudly rather than dropping it. The published schema makes that unrepresentable, so this is the same defence the rest of the path already applies to a broken promise — a fixture that vanishes silently shows up much later as a rule that tests nothing.
This PR had already merged, so both findings are fixed in #230 (fdbad74). The The observation that the two checks disagreed about The dead disjunct is gone, and a file set carrying a stray — AI Coding Agent |
Review feedback on #229, applied here because #229 had already merged. `content: null` reproduced the exact bug #229 says it closes. The guard tested `content === undefined`, and `yaml` does not throw on a value it cannot make a document of, it renders one: `undefined` becomes the string "undefined" and `null` becomes the string "null". Either way the rule file is created and its entire contents are that word. A string or a number does the same. The test is now "a usable object" rather than "not undefined", and the three cases are covered. Confirmed by restoring the old guard and watching all three fail. Worth naming why two checks in this function disagree about `null`, and why that is correct. The mutual-exclusion check asks what the payload CLAIMS, so any present `content` — `null` included — means the service sent both envelopes. This one asks what can be WRITTEN. Collapsing them into one predicate would make one of the two wrong. Also drops a dead disjunct the reviewer spotted: `!isSingleContentRule` could never be true there, since `files` is provably absent by that point and the helper is defined as its negation. And a file set arriving with a stray `tests` now fails loudly rather than dropping it. The published schema makes that unrepresentable, so this is the same defence the rest of the path already applies to a broken promise — a fixture that vanishes silently shows up much later as a rule that tests nothing.
1 participant
Stack (root → tip):
Task 5.8 of
generator-payload-alignment. The generator's file-set tier is live (their #122 deployed), sosrc/generated/api.d.tsis regenerated from the published schema.This is the bottom of a two-PR stack; slice 6 (the restore client) stacks on top.
What the regeneration brought
rulesis 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 statessignatureas required:That is the property we asked the generator for, now expressed in our types rather than as an assumption on both sides. A runtime rule executes only against a blessed signature, so an unsigned one gets written, verifies clean, and never runs.
Why the diff is not just the generated file
The union broke six sites reading
.contentand.testsoff a rule without asking which variant they had. They are narrowed, not cast:isFileSetRule/isSingleContentRuleinapi/rules.ts.Both key on
filesrather than onengine.engineis what the file-set variants have in common;filesis 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.The
carries both files and contentcheck stays, even 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.One live bug closed
A payload carrying neither
filesnorcontentfell through to the single-content branch and handedyaml.stringifyanundefined. That returns the string"undefined"rather than throwing, so the rule file was created and its contents were that word — a malformed rule on disk, discovered two steps from the cause.Refused now, before the directory is created. Checked on
contentitself rather than on "not a file set", because a payload with neither is not a file set either, so the negative admits it. My first attempt got exactly that wrong and the new test caught it.Verification
1087/1087 tests, typecheck, lint, and
pnpm cli checkclean.The changeset is extended rather than duplicated: this is the same change that shipped slices 1 to 5, and one change gets one release note.