fix: make rule meta explain itself, and route improve-rule off it - #238
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 3m 4s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing (
SummaryThis is a well-scoped fix. I traced the core claim end to end: Two things worth a look: [New] Inline comment on [New] Not part of this diff, but surfaced by it: Everything else — the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review follow-up on #236, taking both low items as one change because the second is only worth doing as the first's mechanism. A purge failure became a normal, reachable outcome in the previous commit, and it landed in a catch that reported "could not be written". That is the opposite of true: the blessed bytes ARE on disk and only the cleanup is partial. A reader acting on it re-runs a repair that already succeeded, or concludes the rule is unrepaired and edits it by hand. The error string already said so, wrapped in a sentence contradicting it. `PurgeIncompleteError` carries the surviving entries, so the caller branches on a type rather than matching prose — which is the point: the two failures ask the reader for opposite things ("the rule is not there" versus "the rule is there and something stale is too"), and a call site that could only read the message would get it wrong the first time the wording changed. The repair notice now names what survived and says an engine still reads it. The second item was `DeliveryWrite.written`/`.removed`, computed and never consumed. Resolved by deleting the shape rather than plumbing it to a caller. Plumbing it would mean widening `writeRuleFile`'s return, which changes call sites in `commands/rules.ts` — a file PR #238 owns concurrently — for a value nothing needed once the failure path carries its own detail. The data now lives where it is used. Verified the new test bites by throwing a plain `Error` instead: it fails, and passes again when restored. Five unrelated tests timed out at the limit on one run and did not reproduce across four further runs, including three consecutive runs of the two suites this change touches.
Review follow-up on #236, taking both low items as one change because the second is only worth doing as the first's mechanism. A purge failure became a normal, reachable outcome in the previous commit, and it landed in a catch that reported "could not be written". That is the opposite of true: the blessed bytes ARE on disk and only the cleanup is partial. A reader acting on it re-runs a repair that already succeeded, or concludes the rule is unrepaired and edits it by hand. The error string already said so, wrapped in a sentence contradicting it. `PurgeIncompleteError` carries the surviving entries, so the caller branches on a type rather than matching prose — which is the point: the two failures ask the reader for opposite things ("the rule is not there" versus "the rule is there and something stale is too"), and a call site that could only read the message would get it wrong the first time the wording changed. The repair notice now names what survived and says an engine still reads it. The second item was `DeliveryWrite.written`/`.removed`, computed and never consumed. Resolved by deleting the shape rather than plumbing it to a caller. Plumbing it would mean widening `writeRuleFile`'s return, which changes call sites in `commands/rules.ts` — a file PR #238 owns concurrently — for a value nothing needed once the failure path carries its own detail. The data now lives where it is used. Verified the new test bites by throwing a plain `Error` instead: it fails, and passes again when restored. Five unrelated tests timed out at the limit on one run and did not reproduce across four further runs, including three consecutive runs of the two suites this change touches.
6bc3944 to
a9b08b1
Compare
Both findings verified against the code and addressed in
— AI Coding Agent |
…e through it `.taskless/rule-metadata/<id>.yml` is written from the `meta` block of a rule status response. The service does not populate that block, so the sidecar has never been written for any rule, in any mode, under any tier. `rule meta` read it and failed `RULE_NOT_FOUND` — a code that reads as "try a different id" when no id works, for rules sitting on disk in front of the user. Keep the command, and make its failure true. A new `RULE_META_UNAVAILABLE` code names the actual state and the message points at the ticket id that does drive iteration. The sidecar is still read, so a file that exists is still reported; what changed is what its absence means. The write branches stay with a note that they are dead until the service populates `meta`. The recipes promised more than the CLI can do. `improve-rule` listed the sidecar as a PRECONDITION and made `rule meta` step 3, so an agent following it stalled on a command that cannot succeed while the working mechanism — the `ruleId` from `rule create --json`, which is what the iterate endpoint is addressed by — was documented as the fallback. Step 3 now names that id and says where it comes from. `create-remote-rule` claimed create writes the sidecar; `delete-rule` and `create-sg-rule` described it as ordinarily present. All corrected, topics bumped.
The improve-rule recipe documents RULE_NOT_FOUND as "the service has no such ticket id, re-check the id from `rule create --json`", but no code path could emit it: every iterateRule failure, including the 404 request_not_found that IS a bad ticket id, was caught and reported as NETWORK_ERROR. An agent handed a wrong id was told to retry an id that will never resolve. iterateRule now throws a CLIError carrying RULE_NOT_FOUND for that 404, following the same "the code travels on the error" contract as resolveIdentity, and improveCommand reads the code off the error rather than flattening every failure to NETWORK_ERROR. Also corrects openspec/specs/cli-rules/spec.md, which asserted in a parenthetical that the API-backed branch of `rule create` writes a metadata sidecar. It never has, which is this change's premise.
Review feedback on #239, landed here because #239 carried the regenerated types and this file is on this branch. `submitRule`'s response now carries both `requestId` and a deprecated `ruleId` with the same value. Line 175 read the deprecated one while its sibling at 435 already read `requestId`, so one file gave two answers about the same response. Worth noting the two were never inconsistent through carelessness: iterate has returned `requestId` all along, and submit only offered `ruleId` until the service renamed the resource. The skew appeared the moment the types were regenerated. The local variable KEEPS its name, deliberately. It feeds this command's own `--json` `ruleId` field, which is our published contract, read by `improve-rule` and by any agent following it. Renaming the service's field and renaming ours are separate decisions, and doing the second silently as a side effect of the first would move the ground under every recipe that reads it. No `response.ruleId` reads remain in `src/`.
a9b08b1 to
602d186
Compare
|
We ended up merging #239 first, which made this an easier fix |
1 participant
The command cannot succeed today
.taskless/rule-metadata/<id>.ymlis written from themetablock of a rule status response —ticketId,installationId,generatedAt,schemaVersion, keyed by rule filename. The rule service does not populate that block. The generator team confirmed it:ctx.metais typed "set by the building step", the only assignment anywhere restores it from a serialized context that never set it either, andgeneratedAtappears nowhere in their tree. They are documenting the deadness at their schema rather than filling it in.So in this CLI:
writeRuleMetaFilessits behindif (status.meta)on both the create and improve paths. Never true, so.taskless/rule-metadata/is never written.rule meta <id>reads that sidecar and failedRULE_NOT_FOUNDwhen it was absent. It is always absent.taskless rule metacould only ever fail, and it failed with a code that reads as "try a different id" for a rule sitting on disk in front of the user.What the recipes promised
rule-meta.txtlisted ".taskless/rule-metadata/<id>.ymlexists" as a PRECONDITION and called itself "used internally by therule improverecipe to fetch theticketIdneeded for iteration".improve-rule.txtrepeated that as a precondition and maderule meta <id> --jsonstep 3 of 10.create-remote-rule.txttold the user create writes metadata there.delete-rule.txtandcreate-sg-rule.txtdescribed the sidecar as ordinarily present.Meanwhile the mechanism that works was documented as the fallback:
rule improve --from <file>takes{ ruleId, guidance, references? }, andruleIdis the ticket id the iterate endpoint is addressed by — printed asruleIdbyrule create --json. The caller supplies it; nothing on disk holds it.Direction chosen: keep the surface, make it truthful
Not removal. Three reasons:
rule metais a named part of the user-facing surface inopenspec/specs/cli-rules/spec.md, and appears incli/spec.md(--anonymousno-op) andanalytics/spec.md. Deleting the command falsifies three spec documents and needs an OpenSpec change of its own; making it honest keeps all three accurate.rule meta— from an older recipe, a pinned skill, a habit — gets told exactly why there is no data and where the ticket id actually comes from.command not foundteaches nothing.meta, the read and write paths are intact and start working. The write branches are kept, annotated as dead until then.Concretely:
RULE_META_UNAVAILABLE, distinct fromRULE_NOT_FOUNDprecisely because the remedies differ: there is no id that produces a sidecar. The message says the sidecar is never written by this version, says it is not specific to the id asked for, and names the working path. The sidecar is still read, so a file that exists is still reported; only the meaning of its absence changed.cli/spec.md; nothing was renamed.What an agent following
improve-rulenow doesStep 3 was "run
rule meta <id> --jsonto get theticketId". It is now: take the ticket id from theruleIdfield of that rule'srule create --jsonoutput, or ask the user for it — and do not runrule meta, which fails for every rule. If nobody has the id, fetchimprove-rule --anonymousand iterate locally. The precondition changed from "a file exists on disk" (unsatisfiable) to "you have the ticket id" (satisfiable, and stated where it comes from). The input-schema note and theRULE_NOT_FOUNDrow were corrected to match.create-remote-rulenow says nothing is written under.taskless/rule-metadata/and tells the agent to recordruleIdfrom the create output, which is the only place it appears.rule-metawas rewritten to be a redirect.rule,delete-rule, andcreate-sg-rulewere corrected. Topic versions bumped:improve-rulev5,rule-metav3,create-sg-rulev5,create-remote-rulev3,delete-rulev3,rulev2.Checks
pnpm build,pnpm typecheck,pnpm lint,vitest run(1141 passed),pnpm cli check— clean, with only the 4 pre-existingno-hedgingwarnings inroute.txtandonboard.txt.