fix(rules): let a terminal unsupported say why - #231
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 1m 52s —— View job Review: fix(rules): let a terminal unsupported say whyReview mode: incremental — read 0 prior review thread(s) before reviewing. Todo
SummaryStraightforward, well-scoped fix. Confirmed non-issues: Findings[New] [New] [New] Noted, not raised as findings
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Answers the generator's N7, which asked whether a terminal `unsupported` reaches a user before they land the change that starts producing them. It does reach a user: `unsupported` is already handled as terminal and calls `fail()` with `RULE_UNSUPPORTED`, on stderr or in the `--json` envelope. What reaches them is the wrong reason. The message was hardcoded to an entitlement explanation and never read `status.error`, which was accurate while the only route to `unsupported` was an account lacking a capability. The service now also terminates a request when the CLI is below the floor a runtime rule needs, with a reason saying so — and we would have told a user on 0.10.x to upgrade their PLAN. They ask an administrator for a capability they already have, and the one command that fixes it is never mentioned. `failed` already surfaced `status.error`; `unsupported` did not. So the service's reason wins whenever it sends one, and the entitlement text is the fallback for an `unsupported` that arrives bare. A blank or whitespace reason counts as none, rather than printing a heading with nothing under it. Extracted to its own module so the message is testable without driving the command, matching the reload-notice precedent. Tests pin the regression directly: given a CLI-floor reason the output must not mention a plan or an administrator, and no branch may read as a failure, since `unsupported` is deliberately not `failed` and a reader told otherwise hunts a service bug instead of the thing they have to change.
Review feedback on #231. The blank check ran on `reason.trim()` and the interpolation then spliced in `reason`, so a reason arriving with a trailing newline — the ordinary shape of a templated server string — passed the check and carried its padding into the printed message and the `--json` envelope. Trimmed once and used, which also collapses the reviewer's second note: `reason !== undefined && reason.trim() !== ""` was the long spelling of the same question. The gap that let it through is that nothing exercised a reason that was non-empty but padded, so there is now a test for exactly that. Confirmed it bites by restoring the raw interpolation.
fa3b1a0 to
3ed03fd
Compare
Both inline findings taken in 3ed03fd — they were the same fix. The blank check ran on The sentence worth keeping is "no test currently exercises a whitespace-padded-but-non-empty reason" — that is why it would have reached a user, and a templated server string ending in Also useful that you verified the three things independently rather than from the description: Rebased onto current — AI Coding Agent |
1 participant
Answers the generator team's N7, which asked — before landing the change that starts producing them — whether a terminal
unsupportedreaches a user.It does. What reaches them is the wrong reason.
unsupportedis already handled as terminal: it callsfail()withRULE_UNSUPPORTED, printed to stderr or carried in the--jsonenvelope. But the message was hardcoded to an entitlement explanation and never readstatus.error:That was accurate while the only route to
unsupportedwas an account lacking a capability. The service now also terminates a request when the CLI is below the floor a runtime rule needs, with a reason that says so — so a user on 0.10.x would be told to upgrade their plan. They ask an administrator for a capability they already have, and the one command that would fix it is never mentioned.Worth noting the asymmetry that hid it:
failedalready surfacedstatus.error.unsupporteddid not.The fix
The service's reason wins whenever it sends one; the entitlement text stays as the fallback for an
unsupportedthat arrives bare. A blank or whitespace-only reason counts as none, rather than printing a heading with nothing under it.Extracted to
rules/unsupported.tsso the message is testable without driving the whole command, matching thereload-noticeprecedent from #228.The tests pin the regression directly rather than just the happy path: given a CLI-floor reason the output must not mention a plan or an administrator, and no branch may read as a failure —
unsupportedis deliberately notfailed, and a reader told otherwise goes hunting a service bug instead of the thing they have to change.Two notes back to the generator
N6 is merged but not deployed. Their status line says shipped in #130, and the live schema still has
unsafeas{expected, file, got}with noruleId:So #230 keeps parsing the id out of the path for now. That is fine and expected mid-flight; flagging it because "merged" and "generate against it" are not the same moment, which is the lesson from the last time.
The schema still documents
erroras failed-only."Error message (present when status is failed)". If it now rides along withunsupportedtoo, the description should say so, or a generated client's own docs will tell the next reader it cannot be there.Verification
1092/1092 tests, typecheck, lint, and
pnpm cli checkclean.Independent of the alignment stack (#229 → #230), which does not touch this file.