refactor(errors): centralize 403 FORBIDDEN translation in wrapResult

[lmjabreu]

Summary

• Lift the local isForbidden helper from src/commands/channel/delete.ts into the central wrapResult handler in src/lib/api.ts. Every SDK call now gets uniform 403 → CliError('FORBIDDEN') translation, not just channel deletion. Future commands (kick user, group delete, etc.) inherit the friendly message for free.
• New isForbidden(error) predicate in src/lib/errors.ts sits next to isInsufficientScope. Both delegate to a shared hasTwistStatusCode(error, status) type predicate so the shape narrowing lives in one place, with no as casts. JSDoc documents the precedence rule: callers must test isInsufficientScope first so OAuth-scope 403s keep their dedicated INSUFFICIENT_SCOPE code and hints; isForbidden is the catch-all.
• FORBIDDEN end-to-end translation test in src/lib/api.test.ts exercises the real proxy + wrapResult flow by mocking @doist/twist-sdk. Covers FORBIDDEN translation, INSUFFICIENT_SCOPE precedence, and non-403 pass-through (asserted by object identity, not just message).

Hint copy is call-agnostic. The central message reads Twist refused this action: 403 Forbidden. with hints You may not have permission for this action and Contact your workspace admin, or re-authenticate with \tw auth login` if your token looks wrong`. #246's local message included the channel name; uniform coverage is the deliberate tradeoff. If per-command flavour matters later, we can plumb the spinner config's command name into the central message.

Follow-ups (not this PR): the same central-translation pattern should eventually cover 404 (NOT_FOUND-style with resource hint), 429 (rate-limit with retry-after), and 5xx (transient retry with backoff). Each warrants its own PR with its own behaviour decision. Batch responses (assertBatchData / getOptionalBatchData) also bypass wrapResult and would need a separate 403 check; tracked for the same follow-up.

Test plan

• npm run type-check
• npm run lint
• npm test (710 passed)
• Manual smoke: tw channel delete id:837190 --yes against the parked admin-only channel surfaces the new friendly FORBIDDEN message (verified after tw channel unarchive, then re-archived).

[doistbot]

This PR successfully centralizes 403 Forbidden error handling within the API proxy, establishing a solid pattern for unified error translation. The refactoring simplifies command-level logic and sets a great foundation for standardizing other API responses. There are a few areas that need further refinement, specifically extending the translation coverage to batch API methods, adapting the error hints to remain accurate for read-only calls, extracting shared logic in the error predicates and test mocks, and strengthening the test assertions to ensure exact messages and object instances are validated.

_{Share Feedback • Review Logs}

[gnapse]

What if instead of relying on a comment here we redesign the error types system to avoid this? here's what I suggest:

• create a getErrorType(error: unknown) function
• the return type is a restricted string union type with the possible answers
• then isForbidden and isInsufficientScope can be implemented like this:

  function isForbidden(error) {
      return getErrorType(error) === 'forbidden'
  }
  function isInsufficientScope(error) {
      return getErrorType(error) === 'insufficient-scope'
  }

• Maybe we do not even need these functions, and we just rely on exporting getErrorType as the main way to check if an error is of a certain type
• getErrorType would have embedded in it the workflow of first checking if it's insufficient scope before returning if it's forbidden.

[lmjabreu]

The fragility you're flagging is real. The precedence rule lives in a doc comment that nothing enforces, so a future caller could test isForbidden first and get the wrong code without anything noticing. A getErrorType discriminator that bakes the precedence into the implementation would catch that statically.

I'd rather do that as a follow-up PR. This one is scoped to the 403 centralization in wrapResult, and the redesign touches the public shape of errors.ts (whether isForbidden/isInsufficientScope stay as wrappers or get deprecated). It would also be the right place to extend the type union for 404/429/5xx, which is on the follow-up list. Doing all the API status helpers in one pass beats sprinkling the change across two PRs.

Does that work, or would you rather block this on the redesign?

[gnapse]

That's ok, addressing it in a follow-up refactoring PR.

[doist-release-bot]

🎉 This PR is included in version 2.45.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀