fix: fail fast and explain 401s on group writes

README.md

@@ -66,6 +66,8 @@ tdc auth login
 
 This opens Todoist OAuth in your browser. The default grant can read Comms data and create/update content and messages. It does not include delete, channel management, or user/workspace write scopes; use `--read-only` for read-only access or `--full-access` when needed.
 
+All group management — `groups create`, `rename`, `delete`, `add-user`, `remove-user` — needs the `workspaces:write` scope, so it requires `tdc auth login --full-access`. Group reads (`groups`, `groups view`) work on a default login.
+
 Once approved, the token is stored in your OS credential manager:
 
 - macOS: Keychain

skills/comms-cli/SKILL.md

@@ -46,7 +46,7 @@ tdc changelog                     # Show recent changelog entries
 tdc migrate urls <urls>           # Translate old twist.com URLs to Comms URLs (needs a Twist token)
 ```
 
-OAuth login uses Todoist OAuth for Comms access. The default grant can read Comms data and create/update content or messages. It does not include delete, channel management, or user/workspace write scopes; use `tdc auth login --full-access` only when needed. Stored auth uses the system credential manager when available. If secure storage is unavailable, `tdc` warns and falls back to `~/.config/comms-cli/config.json`. `COMMS_API_TOKEN` always takes priority over the stored token.
+OAuth login uses Todoist OAuth for Comms access. The default grant can read Comms data and create/update content or messages. It does not include delete, channel management, or user/workspace write scopes; use `tdc auth login --full-access` only when needed (all `tdc groups` writes require it). Stored auth uses the system credential manager when available. If secure storage is unavailable, `tdc` warns and falls back to `~/.config/comms-cli/config.json`. `COMMS_API_TOKEN` always takes priority over the stored token.
 
 In read-only mode (`tdc auth login --read-only`), commands that modify Comms data (reply, archive, react, delete, etc.) are blocked by the CLI. Externally provided tokens (`COMMS_API_TOKEN` or `tdc auth token`) are treated as unknown scope and assumed write-capable.
 
@@ -284,6 +284,8 @@ tdc groups remove-user <group-ref> user1 user2  # Remove users from a group
 tdc groups remove-user <ref> id:123,id:456      # Comma-separated ID refs
 ```
 
+All group *writes* (`groups create`, `rename`, `delete`, `add-user`, `remove-user`) need the `workspaces:write` scope, which only `tdc auth login --full-access` grants. Group *reads* (`groups`, `groups view`) work on a default login.
+
 If a channel is not found in `tdc channels`, widen with broader listings such as `tdc channels --scope public`, then `tdc channels --scope public --state all`. Check `tdc channels --help` for other available filters.
 
 `tdc channel threads` returns every thread in the channel; pagination filters (`--limit`, `--cursor`, `--since`, `--until`, `--unread`) are applied client-side after fetch. `--archive-filter` is applied server-side. Results are sorted newest-first by last activity. In `--json` / `--ndjson`, the response includes a `nextCursor` string (opaque) you can pass via `--cursor` to fetch the next page; NDJSON emits the cursor as a final `{ "_meta": true, "nextCursor": "..." }` line.

src/index.ts

@@ -71,7 +71,7 @@ const commands: Record<string, [string, () => Promise<(p: Command) => void>]> =
     changelog: ['Show recent changelog entries', loadChangelogCommand],
     doctor: ['Diagnose common CLI setup and environment issues', loadDoctorCommand],
     groups: [
-        'Group operations (list, view, create, rename, delete, add-user, remove-user)',
+        'Group operations (list, view, create, rename, delete, add-user, remove-user); writes need --full-access',
         loadGroupsCommand,
     ],
     config: ['Manage CLI configuration', loadConfigCommand],

src/lib/api.test.ts

@@ -7,12 +7,14 @@ const sdkMocks = vi.hoisted(() => ({
     createClient: vi.fn(),
     deleteChannel: vi.fn(),
     uploadAttachment: vi.fn(),
+    addGroupUsers: vi.fn(),
 }))
 
 vi.mock('@doist/comms-sdk', () => {
     class CommsApi {
         channels = { deleteChannel: sdkMocks.deleteChannel }
         attachments = { upload: sdkMocks.uploadAttachment }
+        groups = { addUsers: sdkMocks.addGroupUsers }
         workspaceUsers = { getWorkspaceUsers: getWorkspaceUsersMock }
         constructor(token?: string, options?: unknown) {
             sdkMocks.createClient(token, options)
@@ -50,9 +52,12 @@ vi.mock('./auth.js', () => ({
 // exercise `channels.deleteChannel` and `attachments.upload` as mutating;
 // reads (getWorkspaceUsers) stay off the write path.
 const permMocks = vi.hoisted(() => ({
-    ensureWriteAllowed: vi.fn().mockResolvedValue(undefined),
+    ensureMutationAllowed: vi.fn().mockResolvedValue(undefined),
     isMutatingMethod: vi.fn(
-        (path: string) => path === 'channels.deleteChannel' || path === 'attachments.upload',
+        (path: string) =>
+            path === 'channels.deleteChannel' ||
+            path === 'attachments.upload' ||
+            path === 'groups.addUsers',
     ),
 }))
 vi.mock('./permissions.js', () => permMocks)
@@ -125,7 +130,8 @@ describe('wrapResult — central 403 translation', () => {
         sdkMocks.createClient.mockReset()
         sdkMocks.deleteChannel.mockReset()
         sdkMocks.uploadAttachment.mockReset()
-        permMocks.ensureWriteAllowed.mockReset().mockResolvedValue(undefined)
+        sdkMocks.addGroupUsers.mockReset()
+        permMocks.ensureMutationAllowed.mockReset().mockResolvedValue(undefined)
     })
 
     it('uses the explicit base URL when creating the wrapped SDK client', () => {
@@ -179,7 +185,7 @@ describe('wrapResult — central 403 translation', () => {
     })
 
     it('translates an attachments.upload scope 403 into INSUFFICIENT_SCOPE (re-login prompt)', async () => {
-        permMocks.ensureWriteAllowed.mockResolvedValue(undefined)
+        permMocks.ensureMutationAllowed.mockResolvedValue(undefined)
         sdkMocks.uploadAttachment.mockRejectedValueOnce(
             new CommsRequestError('Request failed with status 403', 403, {
                 error_string: 'Insufficient scope provided: attachments:write',
@@ -198,11 +204,11 @@ describe('wrapResult — central 403 translation', () => {
             ],
         })
         // Confirms upload runs through the mutating write-guard.
-        expect(permMocks.ensureWriteAllowed).toHaveBeenCalled()
+        expect(permMocks.ensureMutationAllowed).toHaveBeenCalled()
     })
 
     it('routes attachments.upload through the write-guard, blocking it in read-only mode', async () => {
-        permMocks.ensureWriteAllowed.mockRejectedValueOnce(new Error('READ_ONLY'))
+        permMocks.ensureMutationAllowed.mockRejectedValueOnce(new Error('READ_ONLY'))
         const client = createWrappedCommsClient('test-token')
 
         await expect(
@@ -212,6 +218,48 @@ describe('wrapResult — central 403 translation', () => {
         expect(sdkMocks.uploadAttachment).not.toHaveBeenCalled()
     })
 
+    it('routes group membership writes through the mutation guard', async () => {
+        permMocks.ensureMutationAllowed.mockRejectedValueOnce(new Error('INSUFFICIENT_SCOPE'))
+        const client = createWrappedCommsClient('test-token')
+
+        await expect(
+            client.groups.addUsers({ id: 'G1', workspaceId: 69, userIds: [1] }),
+        ).rejects.toThrow('INSUFFICIENT_SCOPE')
+        // The guard runs before the request, so nothing hits the network.
+        expect(sdkMocks.addGroupUsers).not.toHaveBeenCalled()
+        expect(permMocks.ensureMutationAllowed).toHaveBeenCalledWith('groups.addUsers')
+    })
+
+    it('translates a 401 into INVALID_TOKEN with re-auth guidance', async () => {
+        sdkMocks.addGroupUsers.mockRejectedValueOnce(
+            new CommsRequestError('Request failed with status 401', 401, {
+                error_string: 'Invalid token',
+                error_code: 200,
+            }),
+        )
+        const client = createWrappedCommsClient('test-token')
+
+        await expect(
+            client.groups.addUsers({ id: 'G1', workspaceId: 69, userIds: [1] }),
+        ).rejects.toMatchObject({
+            code: 'INVALID_TOKEN',
+            message: 'Comms rejected the token: 401.',
+            hints: ['Re-authenticate with `tdc auth login`, then check `tdc auth status`'],
+        })
+    })
+
+    it('gives the same 401 guidance on reads, which also route through wrapResult', async () => {
+        sdkMocks.deleteChannel.mockRejectedValueOnce(
+            new CommsRequestError('Request failed with status 401', 401, {}),
+        )
+        const client = createWrappedCommsClient('test-token')
+
+        await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({
+            code: 'INVALID_TOKEN',
+            hints: ['Re-authenticate with `tdc auth login`, then check `tdc auth status`'],
+        })
+    })
+
     it('passes non-403 errors through untranslated', async () => {
         const originalError = new CommsRequestError('Request failed with status 500', 500, {})
         sdkMocks.deleteChannel.mockRejectedValueOnce(originalError)

src/lib/api.ts

@@ -7,8 +7,8 @@ import {
 } from '@doist/comms-sdk'
 import { getApiTokenSnapshot } from './auth.js'
 import { getConfig, updateConfig } from './config.js'
-import { CliError, isForbidden, isInsufficientScope } from './errors.js'
-import { ensureWriteAllowed, isMutatingMethod } from './permissions.js'
+import { CliError, isForbidden, isInsufficientScope, isInvalidToken } from './errors.js'
+import { ensureMutationAllowed, isMutatingMethod } from './permissions.js'
 import { getProgressTracker } from './progress.js'
 import { withSpinner } from './spinner.js'
 
@@ -158,7 +158,7 @@ function createNestedSpinnerProxy<T extends object>(obj: T, basePath: string): T
 
                 // For mutating methods, check permissions before calling the API
                 if (shouldCheckPermissions) {
-                    return ensureWriteAllowed().then(() => {
+                    return ensureMutationAllowed(fullPath).then(() => {
                         const result = originalMethod.apply(target, args)
                         return wrapResult(result, progressTracker, spinnerConfig)
                     })
@@ -208,6 +208,14 @@ function wrapResult(
                     'Contact your workspace admin, or re-authenticate with `tdc auth login` if your token looks wrong',
                 ])
             }
+            if (isInvalidToken(error)) {
+                // A 401 means the token itself is bad or expired. An
+                // under-scoped grant is a 403 `Insufficient scope`, handled
+                // above — so re-authenticating is the whole fix here.
+                throw new CliError('INVALID_TOKEN', 'Comms rejected the token: 401.', [
+                    'Re-authenticate with `tdc auth login`, then check `tdc auth status`',
+                ])
+            }
             throw error
         })
 

src/lib/auth-provider.ts

@@ -25,6 +25,7 @@ import {
 } from './config.js'
 import { CliError } from './errors.js'
 import { parseRef } from './refs.js'
+import { splitScopeString } from './scopes.js'
 import { createCommsUserRecordStore, getDefaultUserRecord } from './user-records.js'
 
 const DEFAULT_TODOIST_AUTH_BASE_URL = 'https://todoist.com'
@@ -517,14 +518,6 @@ function normalizeScopeString(scope: string): string {
     return splitScopeString(scope).join(' ')
 }
 
-function splitScopeString(scope: string): string[] {
-    return scope
-        .replaceAll(',', ' ')
-        .split(/\s+/)
-        .map((part) => part.trim())
-        .filter(Boolean)
-}
-
 /**
  * Accepts `42`, `id:42`, and case-insensitive labels — `parseRef` normalises
  * the numeric forms. Broader than cli-core's default strict-equality matcher.

src/lib/errors.test.ts

@@ -1,7 +1,7 @@
 import { CommsRequestError } from '@doist/comms-sdk'
 import { describe, expect, it } from 'vitest'
 
-import { isForbidden, isInsufficientScope } from './errors.js'
+import { isForbidden, isInsufficientScope, isInvalidToken } from './errors.js'
 
 describe('isInsufficientScope', () => {
     it('returns true for a 403 with "Insufficient scope" error_string', () => {
@@ -83,3 +83,38 @@ describe('isForbidden', () => {
         expect(isForbidden(error)).toBe(false)
     })
 })
+
+describe('isInvalidToken', () => {
+    it('returns true for a 401 regardless of body', () => {
+        expect(
+            isInvalidToken(new CommsRequestError('Request failed with status 401', 401, {})),
+        ).toBe(true)
+        expect(
+            isInvalidToken(
+                new CommsRequestError('Request failed with status 401', 401, {
+                    error_code: 200,
+                    error_string: 'Invalid token',
+                }),
+            ),
+        ).toBe(true)
+    })
+
+    it('returns false for non-401 status codes', () => {
+        expect(
+            isInvalidToken(new CommsRequestError('Request failed with status 403', 403, {})),
+        ).toBe(false)
+        expect(
+            isInvalidToken(new CommsRequestError('Request failed with status 404', 404, {})),
+        ).toBe(false)
+        expect(
+            isInvalidToken(new CommsRequestError('Request failed with status 500', 500, {})),
+        ).toBe(false)
+    })
+
+    it('returns false for plain errors and non-object values', () => {
+        expect(isInvalidToken(new Error('something'))).toBe(false)
+        expect(isInvalidToken(null)).toBe(false)
+        expect(isInvalidToken(undefined)).toBe(false)
+        expect(isInvalidToken('string')).toBe(false)
+    })
+})

src/lib/errors.ts

@@ -106,6 +106,17 @@ export function isForbidden(error: unknown): boolean {
     return hasCommsStatusCode(error, 403) && !isInsufficientScope(error)
 }
 
+/**
+ * Check whether an error is a Comms API 401. Comms returns this both for a
+ * genuinely bad token and — because `_raise_todoist_rest_error` maps an
+ * upstream Todoist `UNAUTHORIZED` onto `INVALID_TOKEN` — for a valid token that
+ * lacks the scope a proxied workspace/group write needs. The two are
+ * indistinguishable on the wire, so the hint covers both.
+ */
+export function isInvalidToken(error: unknown): boolean {
+    return hasCommsStatusCode(error, 401)
+}
+
 /**
  * Comms-flavoured CliError that preserves the historical positional
  * `(code, message, hints?, type?)` signature used across hundreds of call