feat(check): repair a drifted rule instead of only reporting it - #230
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 6m 35s —— View job Review of #230 — repair a drifted rule instead of only reporting itReview mode: incremental — read 0 prior review thread(s) before reviewing. I read all 6 changed files plus the full bodies of 1. [New] Repair notices never reach
for (const notice of plan.notices) warn(notice);and ...(dispatched.notices.length > 0 ? { notices: dispatched.notices } : {}),
Failure scenario: 2. [New] Restoring a drifted rule doesn't purge files that aren't part of the server's blessed set — a "restored" directory can still contain content the repair never touched.
3. [New] for (const target of targets) {
const outcome = await restoreRule(token, { ... });
...
}Each target is independent (different rule ids, same token/repositoryUrl), so this could be 4. [New] Casts bypass the generated types instead of using them, which is exactly what STYLEGUIDE-CODE.md's "Prefer Library Types Over Custom Definitions" warns against.
const files = (rule as { files?: { path: string; content: string }[] }).files;
await writeRuleFile(cwd, rule as unknown as GeneratedRule);
5. [New]
Nothing else stood out — the |
Slice 6. `check` parsed reconcile's `unsafe`, `unknown` and `missing`
verdicts and read none of them.
THE TASK SAID "unsafe / unknown" AND THAT WAS WRONG. Read as a set the
entry shapes settle it: `unsafe` {file, expected, got} means the server
holds bytes we drifted from, `missing` {ruleId, file} means it expected
a rule we never reported, and `unknown` {file} means we hold a file it
never issued. The first two are repairable; the third has nothing on the
server to fetch, which is exactly why its entry carries no rule id.
`missing` was omitted from the task despite being the only bucket that
already carries the id restore is keyed on.
So `unsafe` and `missing` route to restore, and `unknown` gets an
explanation, since "on your disk, never issued by the service" is
ordinary (hand-written, or another org or install) and read as an
unexplained skip.
Restored bytes are verified against the signature reconcile ALREADY
sent, not the one the restore response carries. Verifying a response
against itself proves the service is internally consistent, which it
would also be if it returned a NEWER generation of the rule: an upgrade
wearing a repair's clothes, arriving mid-check, reviewed by nobody.
Because `unsafe.expected` is in hand there is exactly one acceptable
answer, which is what makes "restore never returns newer bytes" a test
rather than a promise we relay.
Nothing repaired runs in the pass that repaired it. Restore rewrites the
working tree and promotes nothing into the current run, so an `unsafe`
rule stays withheld and the next `check` gets it blessed through the
ordinary path. Fetching code and executing it in the same pass that
discovered the drift would move the gate, and the gate is the point. A
repair that cannot happen is a notice, never a failed `check`: an
unrepaired rule stays withheld, which is already safe.
The rule id for an `unsafe` entry is parsed out of
`.taskless/rules/runtime/<id>/check.ts`, anchored on the full prefix
because `rules/runtime/` alone is common enough that a wrong id would be
a 404 reading as "the service lost your rule". That parse is the weak
link and is asked for as N6: the layout has moved twice, and a move
breaks repair silently. Delete it when the entry carries the id.
Also corrects tasks.md, which had drifted several items behind reality.
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.
37b9503 to
fdbad74
Compare
…iring 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.
All five addressed in 11df67d. Finding 1 was the important one and it was worse than "low". 1 — repair notices never reached 5 — and your reason it survived is right. Writing that test found something the review could not: a restore returning only 4 — both casts were hiding something. 3 — restores now run concurrently. The writes stay sequential, since a half-applied set is the state this path exists to avoid. 2 — the notice was overclaiming, and that part is fixed here. It now says the check was rewritten with the blessed bytes rather than that the rule was restored, because the delivered set is written over the directory without removing files it does not mention, and only 1110/1110 tests, typecheck, lint, and — AI Coding Agent |
#230 merged while this was in flight, and its integration fixture writes a current-layout tree with no `taskless.json`. A tree without a manifest reads as version 0, so `check` refuses it, and every repair notice the test asserts on went missing. The refusal is right and stays. Nothing can tell a current tree with no manifest from a pre-`0004` project by looking, and the safe answer to "I cannot confirm this is current" is to say so rather than to read it and report whatever falls out — which in that fixture's case was an empty envelope. So the fixture completes its scaffold, the way every other suite here now does.
#230 merged while this was in flight, and its integration fixture writes a current-layout tree with no `taskless.json`. A tree without a manifest reads as version 0, so `check` refuses it, and every repair notice the test asserts on went missing. The refusal is right and stays. Nothing can tell a current tree with no manifest from a pre-`0004` project by looking, and the safe answer to "I cannot confirm this is current" is to say so rather than to read it and report whatever falls out — which in that fixture's case was an empty envelope. So the fixture completes its scaffold, the way every other suite here now does.
#230 merged while this was in flight, and its integration fixture writes a current-layout tree with no `taskless.json`. A tree without a manifest reads as version 0, so `check` refuses it, and every repair notice the test asserts on went missing. The refusal is right and stays. Nothing can tell a current tree with no manifest from a pre-`0004` project by looking, and the safe answer to "I cannot confirm this is current" is to say so rather than to read it and report whatever falls out — which in that fixture's case was an empty envelope. So the fixture completes its scaffold, the way every other suite here now does.
1 participant
Stack (root → tip):
Slice 6 of
generator-payload-alignment. Stacked on #229 (task 5.8), which regenerates the types this builds against.checkhas been parsing reconcile'sunsafe,unknownandmissingverdicts and reading none of them.The task said "unsafe / unknown", and that was wrong
Read as a set, the entry shapes settle it:
unsafe{file, expected, got}missing{ruleId, file}unknown{file}unknownhas nothing on the server to fetch, which is exactly why its entry carries no rule id. Andmissing— omitted from the task — is the only bucket that already carries the id restore is keyed on.So
unsafeandmissingroute to restore.unknowngets an explanation instead, because "on your disk, never issued by the service" is an ordinary situation (hand-written, or belonging to another organization or installation) that currently reads as an unexplained skip.Verification is against the signature reconcile already sent
Not the one the restore response carries. That distinction is the whole guarantee.
Verifying a response against its own
signaturefield proves the service is internally consistent — which it would also be if it returned a newer generation of the rule. That is an upgrade wearing a repair's clothes, arriving mid-check, reviewed by nobody. Becauseunsafe.expectedis already in hand there is exactly one acceptable answer, and "the service sent something newer" is refused by the same comparison that catches a corrupted transfer.That is what makes task 6.4's "re-fetch never returns newer bytes" a test rather than a promise we relay. The case is covered explicitly, with an internally-consistent-but-newer payload.
A
missingrule has no prior expectation, so it falls back to the response's own signature. Genuinely weaker, and the best available: we are fetching a rule we do not hold, so there is nothing local to disagree with. Stated in the module rather than left as a silent inconsistency.Nothing repaired runs in the pass that repaired it
Restore rewrites the working tree and promotes nothing into the current run. An
unsaferule stays withheld whether or not its bytes were just restored, amissingrule was never a local candidate, and anunknownrule never runs. The nextcheckreports the repaired signature and is blessed through the ordinary path.Fetching code and executing it in the same pass that discovered the drift would move the gate, and the gate is the only reason any of this exists.
A repair that cannot happen is a notice, never a failed
check— a rule that was not repaired stays withheld, which is already the safe state.The weak link, and the ask behind it
An
unsafeentry does not carryruleId, so it is parsed out of.taskless/rules/runtime/<id>/check.ts. Anchored on the full prefix rather than onrules/runtime/, which is common enough that a wrong id would 404 and read as "the service lost your rule".That parse makes repair depend on a layout that has already moved twice, and a move breaks it silently: wrong id, 404, rule stays unrepaired, nobody sees an error. Raised with the generator as N6 (
ruleIdonunsafe), on the same "don't make the client infer what the server knows" argument that settled D2. Tracked as task 6.7; the parse goes when the field arrives.Also
tasks.mdhad drifted several items behind reality — 1.5, 2.5, 3.6, 5.9 were done and unchecked. Corrected here, along with the 6.2 rewrite above.Verification
1103/1103 tests, typecheck, lint, and
pnpm cli checkclean.Built on top of #229
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.