refactor(errors): centralize 403 FORBIDDEN translation in wrapResult#247
refactor(errors): centralize 403 FORBIDDEN translation in wrapResult#247lmjabreu wants to merge 1 commit into
Conversation
Lift the local isForbidden helper from the channel delete command up into wrapResult so every SDK call gets uniform 403 → CliError(FORBIDDEN) translation, not just channel deletion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
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.
| ['Run `tw auth login` to re-authenticate with the required scopes'], | ||
| ) | ||
| } | ||
| if (isForbidden(error)) { |
There was a problem hiding this comment.
[P2] This only translates 403s for rejected promise-returning methods that pass through wrapResult. client.batch(...) doesn’t: it’s a top-level TwistApi method, and batched 403s are still surfaced later by assertBatchData() / getOptionalBatchData() as API_ERROR or BATCH_FAILED. Since commands like thread view, inbox, comment delete, and msg delete rely on batch, the new "central" FORBIDDEN/INSUFFICIENT_SCOPE translation is still incomplete. Either wrap batch on the root proxy too, or teach the batch helpers to map 403 responses into the same CliErrors.
|
|
||
| it('translates a plain 403 into a FORBIDDEN CliError', async () => { | ||
| sdkMocks.deleteChannel.mockRejectedValueOnce( | ||
| Object.assign(new Error('Request failed with status 403'), { |
There was a problem hiding this comment.
[P2] The vi.mock block at the top of this file explicitly provides a TwistRequestError mock implementation, but these tests ignore it and manually reconstruct the same error shape using Object.assign. You can import { TwistRequestError } from '@doist/twist-sdk' and use new TwistRequestError('Request failed with status 403', 403, {}) here and in the tests below. This removes the duplicate error assembly and aligns with how errors.test.ts handles SDK errors.
| * 403s keep their dedicated `INSUFFICIENT_SCOPE` code and hints; `isForbidden` | ||
| * is the catch-all fallback for plain workspace-permission 403s. | ||
| */ | ||
| export function isForbidden(error: unknown): boolean { |
There was a problem hiding this comment.
[P2] isForbidden() duplicates the same Twist-request shape/status narrowing that isInsufficientScope() already does. Please extract the common httpStatusCode check into a tiny shared helper (or build one predicate on top of the other) so these classifiers don't drift as more API-status helpers get added.
| } | ||
| if (isForbidden(error)) { | ||
| throw new CliError('FORBIDDEN', 'Twist refused this action: 403 Forbidden.', [ | ||
| 'You may not have permission for this action — workspace admins can perform it', |
There was a problem hiding this comment.
[P2] This hint now shows up for every SDK method that goes through wrapResult, including read-only calls like getChannel or getWorkspaces. On those paths, “workspace admins can perform it” is often irrelevant or misleading. Either pass method/mutation context into wrapResult and tailor the hint, or keep the centralized 403 copy fully generic.
| const { createWrappedTwistClient } = await import('./api.js') | ||
| const client = createWrappedTwistClient('test-token') | ||
|
|
||
| await expect(client.channels.deleteChannel(500)).rejects.toMatchObject({ |
There was a problem hiding this comment.
[P2] This only pins the code. The change here is the centralized friendly translation, so the 403 cases should also assert the user-facing message (and ideally hints). As written, a regression to the wrong text would still pass. The same gap exists in the insufficient-scope case below.
| const { createWrappedTwistClient } = await import('./api.js') | ||
| const client = createWrappedTwistClient('test-token') | ||
|
|
||
| await expect(client.channels.deleteChannel(500)).rejects.toThrow( |
There was a problem hiding this comment.
[P2] toThrow('Request failed with status 500') doesn't actually prove non-403 errors are passed through untouched. A wrapped CliError with the same message would still satisfy this. Capture the original error object and assert the rejection is that same instance, or at least assert it's not a CliError.
2 participants
Summary
isForbiddenhelper fromsrc/commands/channel/delete.ts(introduced in feat(channel): add create, delete, archive, unarchive subcommands #246) into the centralwrapResulthandler insrc/lib/api.ts, so every SDK call gets uniform 403 →CliError('FORBIDDEN')translation — not just channel deletion. Future commands (kick user, group delete, etc.) inherit the friendly message automatically.isForbidden(error)predicate insrc/lib/errors.tssits next toisInsufficientScope. JSDoc documents the precedence rule: callers must testisInsufficientScopefirst so OAuth-scope 403s keep their dedicatedINSUFFICIENT_SCOPEcode and hints;isForbiddenis the catch-all.channel.test.ts(where it was at the wrong layer after the lift) into a newsrc/lib/api.test.tsthat exercises the real proxy +wrapResultflow by mocking@doist/twist-sdk. Covers FORBIDDEN translation, INSUFFICIENT_SCOPE precedence, and non-403 pass-through.Stacked on #246 — that PR introduces the local helper this one removes, so branching off
mainwould make the cleanup unexpressible. Merge after #246.Tradeoff: the central message is generic (
Twist refused this action: 403 Forbidden.) where #246's local message included the channel name (Twist refused to delete "<name>"…). That's the deliberate cost of uniform coverage — 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.Test plan
npm run type-checknpm run lintnpm test— 674 passedtw channel delete id:837190 --yesagainst the parked admin-only channel surfaces the new friendly FORBIDDEN message (verified aftertw channel unarchive, then re-archived).🤖 Generated with Claude Code