fix(api): name the cause of rule service failures and say when to retry

[theCodeDrift]

When the rule service can't be reached, or answers with a status the schema doesn't document, the user saw only fetch failed or HTTP 503, with nothing saying whether to try again. Some paths also called a rejected request an outage, and two refusal paths relied on the service writing the upgrade link into its message.

Changes

api/v2.ts

• An unavailable outcome now carries retryable. It is true for a network failure, 408, 429, or a 5xx, and false for an undocumented 4xx or a malformed body.
• A network failure is named by its cause, which is where Node's fetch puts it: network error: getaddrinfo ENOTFOUND …, not fetch failed. When the cause has an empty message (an AggregateError from a dual-stack connection refusal), its code is used.
• A success response whose body isn't JSON is reported as invalid response body, not as a network error.
• retryAdvice(outcome) returns the "try again" sentence. Every message built from an unavailable outcome appends it: submit, poll, fetch, recovery, and check.

api/refusal.ts

• describeRefusal() moves the upgrade-link handling out of recover.ts. commands/rules.ts and rules/generate.ts use it too, so every refusal path shows the link exactly once.

rules/plan-check.ts

• check builds its failure clause in reconcileFailureCause(). A documented code such as validation_error now reads as "rejected the verification request (code: details)", no longer "unavailable".

rules/recover.ts

• validation_error is reported as INVALID_INPUT with the service's details, where it was NETWORK_ERROR with the details dropped. This changes the --json error code, and the changeset says so.

Not in this PR

Polling still aborts on the first transient failure. Retrying means submitting the request again, since there is no way to resume polling one. Having the poll loop ride out a few transient failures would be better, but it changes behavior, so it is left for a follow-up.

Testing

• pnpm lint, pnpm typecheck, and the full CLI suite pass (114 files, 2027 tests).
• New cases cover:
  • the network cause, and the empty-message fallback to code
  • which statuses are marked retryable
  • an unparseable body
  • describeRefusal
  • rollback answering 503 and validation_error
  • check against a 503 and a validation_error reconcile

Fixes #454

[theCodeDrift]

@claude /review

[claude]

Claude finished @theCodeDrift's task in 6m 21s —— View job

---

Review mode: incremental — read 0 prior review thread(s) before reviewing.

.prior-review.json held no review threads, no reviews, and no comments beyond the triggering @claude /review. This is the first review of this PR, so everything below is assessed against the whole diff.

• Read .prior-review.json (no prior findings to classify)
• Fetch PR diff and metadata
• Read changed files in full context (api/v2.ts, api/refusal.ts, rules/plan-check.ts, rules/recover.ts, rules/generate.ts, commands/rules.ts, plus the new tests)
• Review for correctness, security, performance, test adequacy, clarity
• Post inline comments for concrete issues
• Post final summary

Summary

The change is well-scoped and the control flow checks out: retryable is computed consistently (network failure / 408 / 429 / 5xx → true; malformed body / undocumented 4xx → false), retryAdvice is appended only where the outcome is actually "unavailable", and reconcileFailureCause correctly stops calling a rejected request (error status, e.g. validation_error) an "unavailable" outage. describeRefusal correctly dedupes the upgrade link against the service's own message. The describeNetworkError cause/.code fallback logic matches the documented Node fetch behavior (plain Error cause → its message; empty-message AggregateError → its .code), and is exercised by the new tests. The INVALID_INPUT reclassification for validation_error in recover.ts matches what the changeset describes as a --json breaking change. Test coverage for the new branches (retryable-by-status, cause-naming, the AggregateError fallback, describeRefusal, and the recovery/check validation_error paths) looks thorough.

[New] One finding worth a look: both inline comments flag that outcome.details — free-form text from the rule service's response body — is interpolated into user-facing messages in three places this PR adds or extends (rules/recover.ts's new validation_error case and its default case, and rules/plan-check.ts's new reconcileFailureCause) without going through stripControlCharacters. The codebase already applies that stripping to refusal.message and to the generation-request error text specifically because they're server-authored strings "written to a terminal from across the network," and a terminal can interpret escape sequences in them. details is the same kind of untrusted value and reaches the terminal the same way, but isn't covered. A similar gap already exists at commands/rules.ts:81 (not touched by this PR), so this isn't a regression this PR introduces wholesale, but it does propagate the same unsanitized pattern to new call sites rather than closing it. Worth a decision either way (fix now, or file a follow-up), given how much care the rest of this PR puts into exactly this class of "what reaches the user" detail.

No other correctness, performance, or clarity issues found. Didn't run pnpm lint/typecheck/tests myself per review instructions — the PR body reports all three passing (114 files, 2027 tests), and CI reports status separately on the PR.

[theCodeDrift]

Re: @claude[bot] — "Claude finished @theCodeDrift's task in 6m 21s…"
#462 (comment)

The details finding is fixed in fce6b72. The control characters are stripped once, in settle(), so every message that interpolates details gets clean text, including the pre-existing commands/rules.ts:81 site.

— AI Coding Agent