fix: fail fast and explain 401s on group writes

[scottlovegrove]

Reported in [CLI] Can't remove/add users to groups — tdc groups add-user / remove-user fail with a bare Request failed with status 401. Tracked as Comms Issues.

Every group write route (add, update, remove, add_users, remove_users) requires the workspaces:write scope, which the CLI only requests under --full-access. On a default login we round-tripped to the API and surfaced whatever the server said, with no hint that the grant was the problem.

Changes

• src/lib/permissions.ts — API_METHOD_SCOPES table + ensureScopeAllowed(). An under-scoped grant now fails immediately, naming the scope and the command that fixes it, instead of after a request. Fails open when the granted scope is unknown (COMMS_API_TOKEN, manually-saved tokens) — those may be session tokens, which bypass scope enforcement server-side, so blocking them would break working setups.
• src/lib/api.ts — guard wired into the client proxy alongside ensureWriteAllowed; new 401 → INVALID_TOKEN branch in wrapResult, so a rejected token gets re-auth guidance rather than the raw SDK message.
• src/lib/errors.ts — isInvalidToken() predicate, alongside the existing isInsufficientScope / isForbidden.
• README, skill content, groups command description — group writes need --full-access; group reads work on a default login.

Channel writes are deliberately absent from the scope table: they already surface a clean 403 from Comms, which wrapResult turns into the same guidance.

Before / after

# before
Error: Request failed with status 401

# after
Error: INVALID_TOKEN
Comms rejected the token: 401.

  - Re-authenticate with `tdc auth login`, then check `tdc auth status`

An under-scoped grant never reaches this path: Comms returns a 403 Insufficient scope (handled by the existing branch), and the pre-flight guard catches it locally before any request goes out.

Note for reviewers

While tracing this I found a separate server-side issue in the group write path and have written it up for the Comms backend team — it's independent of this PR, which stands on its own as the client-side scope handling.

Testing

npm run type-check, npm test (847 pass), npm run lint:check, npm run build all clean. New coverage in permissions.test.ts (blocks/allows, whole-scope matching not substring, fails open on unknown scope, all five group methods) and api.test.ts (guard runs before the request fires; 401 translation).

The 401 mapping is verified against the live API. The scope-guard path is unit-tested only — verifying it end-to-end requires a default-scope login, which I didn't want to force.

🤖 Generated with Claude Code

[doistbot]

This PR adds a client-side scope guard (workspaces:write) for group writes, translates 401 responses into structured INVALID_TOKEN errors with re-auth guidance, and updates documentation across the README, skill content, and groups command.

I also included a few optional follow-up notes in the details below.

Optional follow-up notes (5)

• src/lib/errors.test.ts:116: isInvalidToken has no dedicated unit tests in errors.test.ts. The existing predicates isInsufficientScope and isForbidden both have isolation tests covering true/false cases (correct status, wrong status, plain errors, non-objects). The new predicate is only exercised indirectly through the wrapResult integration test in api.test.ts. Adding parallel tests here would maintain the established convention and catch regressions if the status-code check changes.
• src/lib/permissions.ts:81: Reuse the scope-token parser in auth-provider.ts (export or move splitScopeString) rather than adding a second parser here. The existing parser also normalizes comma delimiters; this version treats workspaces:write,comms:content:write as one token and rejects the grant, so the two scope-handling paths can drift.
• src/lib/api.ts:162: ensureWriteAllowed() and ensureScopeAllowed(fullPath) each independently call getAuthMetadata(), which reads the config file from disk via getConfig() with no caching. Since both run sequentially before the same request, the second read is redundant — for config-file auth (not COMMS_API_TOKEN), every mutating call now does 2 disk reads where it previously did 1. Fetch the metadata once and pass it to both checks (e.g. give ensureScopeAllowed an optional pre-fetched AuthMetadata parameter) to avoid the duplicate I/O on this per-mutating-call path.
• src/lib/api.ts:162: Group writes now call getAuthMetadata() twice in sequence: once through ensureWriteAllowed() and again through ensureScopeAllowed(). For stored credentials, each invocation loads the config via getConfig(). Combine these checks behind one metadata lookup (or pass the first lookup's metadata into the scope check) to avoid duplicate config I/O on every group mutation.
• src/lib/api.test.ts:246: The test title promises "re-auth guidance," and the sibling FORBIDDEN and INSUFFICIENT_SCOPE tests in this same describe block all assert on hints. This one only checks code and message, so removing or garbling the hints (including the known-issue tracking link) would pass silently. Add at least the stable first hint to the toMatchObject, e.g. hints: expect.arrayContaining(['Re-authenticate with \tdc auth login`, then check `tdc auth status`'])` — or assert the full array to match the established pattern.

_{Share Feedback • Review Logs}

[scottlovegrove]

Thanks — all five follow-up notes addressed in 488dd37. The two inline threads have their own replies; these are the ones from the summary block, which have no inline anchor to reply to:

errors.test.ts — no dedicated isInvalidToken tests. Added, matching the shape of the isForbidden / isInsufficientScope blocks: true for a 401 with and without a body, false for 403/404/500, false for plain errors and non-objects.

permissions.ts — second scope parser. Fixed, and you were right about the drift: my split(/\s+/) treated workspaces:write,comms:content:write as one token and would have rejected a valid grant.

I could not import splitScopeString from auth-provider as suggested, though — auth-provider imports api, and api imports permissions, so that would close a cycle. Extracted it to a new src/lib/scopes.ts instead, used by both, with a hasScope helper on top. Added a test for the comma-delimited case.

api.ts — duplicate getAuthMetadata() / getConfig() disk reads (x2 notes). Confirmed: getConfig() has no caching, so every mutating call was doing two reads. Added ensureMutationAllowed(methodPath), which does one lookup and passes the metadata into both checks; api.ts now calls that single guard. Both ensureWriteAllowed and ensureScopeAllowed take an optional pre-fetched AuthMetadata so they stay independently testable.

ensureScopeAllowed also returns before touching auth metadata when the method declares no extra scope, so unscoped methods do zero reads. Test asserts the single-read behaviour.

api.test.ts — 401 test does not assert hints. Fixed, asserting the full array. That hint list also changed via the other thread: the scope hint is now gated to methods that need one, so there are two tests — full hints on groups.addUsers, re-auth hint only on a method with no scope requirement. (The tracking link you mentioned is already gone; that framing was removed before this push.)

npm test 856 pass, type-check, lint:check, and check:skill-sync all green.

[doist-release-bot]

🎉 This PR is included in version 2.0.1 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[nvignola]

LGTM ✅