fix: accept digit-free opaque ids and map 404/409 to CliError

[lmjabreu]

Closes #65.

Short description

looksLikeOpaqueCommsId in refs.ts used to require a Cb prefix, so a bare id with no digit and a different prefix (about 3% of ids) fell through to the name branch and was rejected. It now checks that the token base58-decodes to 16 bytes, which is the server's own rule. The check is only used by the resolvers that have no name fallback (thread, comment, conversation, message). A 21-character base58 name like EngineeringDiscussion also decodes to 16 bytes, so the channel path keeps names first: getDirectChannelId never treats a bare digit-free token as an id, and resolveChannelRef tries getChannel only when nothing matches by name, keeping CHANNEL_NOT_FOUND if that probe 404s or 409s. One consequence: a bare digit-free channel id now needs a workspace (default or --workspace) like a name does, and is checked against it; id:<id> and URL forms stay workspace-agnostic. The id-only resolveChannelId (search --channel, thread create) applies the decode check directly, since it has no name to protect.

The wrapped client in api.ts only mapped 403 and 401. It now maps a 404 to NOT_FOUND, the malformed-id 409 (error_code 217, which the server sends with two different messages) to INVALID_REF, and any other 409 to a new CONFLICT code carrying the server's error_string. 400 and 5xx still pass through unchanged. resolveGroupRef re-wraps the new NOT_FOUND as GROUP_NOT_FOUND so its tdc groups hint survives.

Test plan

Against main, each of these prints the failure from #65. Against this branch:

1. tdc conversation done CDMDzXhBNCgyQZjkDnqwG --dry-run

   • Prints [dry-run] Would archive conversation, no INVALID_REF

2. tdc thread view id:nope

   • Prints Error: INVALID_REF with the server's "must decode to 16 bytes" message, no stack trace

3. tdc thread view <any-thread-id> --comment id:nope

   • Same as 2

4. tdc conversation done CDMDzXhBNCgyQZjkDnqw1 --dry-run (well-formed id that doesn't exist)

   • Prints Error: NOT_FOUND with the "check the id" hint

5. tdc thread view id:nope --json

   • {"error":{"code":"INVALID_REF",...}} rather than INTERNAL_ERROR

6. tdc channel threads EngineeringDiscussion (21-character base58 name, no such channel)

   • Error: CHANNEL_NOT_FOUND, not an id error

7. tdc groups view id:CDMDzXhBNCgyQZjkDnqw1

   • Error: GROUP_NOT_FOUND with the tdc groups hint

8. tdc search x --channel CbjxNkWHJBwcaVkoTCRgM (any bare digit-free channel id)

   • Reaches the API (results or none), no INVALID_REF

Controls: a thread id with a digit still resolves bare, and a 500 still passes through untranslated (existing test).

🤖 Generated with Claude Code

[lmjabreu]

@doistbot /review

[doistbot]

Replaces the Cb-prefix opaque-ID heuristic with a base58-decodes-to-16-bytes check (the server's own rule) and extends the wrapped client's error mapping from 401/403 to also translate 404 and 409 responses into typed CliErrors. Well-tested and fixes all five scenarios from #65.

Few things worth tightening:

• The decode-based check now misclassifies 21–22 character base58-only channel names (e.g. EngineeringDiscussion) as IDs in direct-channel paths, so tdc channel update/delete/archive skips name lookup — consider restricting the heuristic to resolvers without a name fallback or having the channel path disambiguate via the API.
• Translating all 404s to NOT_FOUND before resolveGroupRef sees them bypasses its existing catch, so missing group ids surface as generic NOT_FOUND instead of GROUP_NOT_FOUND with the tdc groups hint — re-wrap there or drop the now-dead path.
• isMalformedId relies on prose matching rather than the stable error_code (217) for identifying the server's malformed-id 409.
• Small dedup opportunities: the error_string extractor duplicates logic in isInsufficientScope, and the base58 alphabet appears twice in refs.ts.

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

Optional follow-up notes (5)

• src/lib/errors.ts:143: isMalformedId keys off the free-form error_string instead of the stable error_code 217 that identifies this server response. If the message is reworded or absent, a malformed id 409 falls through to CONFLICT; conversely, an unrelated 409 whose message happens to contain "must decode to" would be misclassified as INVALID_REF. Match error_code === 217 (or include it) rather than prose.
• src/lib/errors.ts:131: getCommsErrorString duplicates the responseData.error_string extraction already inlined in isInsufficientScope (lines 89–97). Reuse the new helper there, e.g. return hasCommsStatusCode(error, 403) && (getCommsErrorString(error)?.includes('Insufficient scope') ?? false), so the shape checks have a single source of truth.
• src/lib/refs.ts:83: The base58 alphabet is now specified twice and must be kept in sync by hand: the BASE58_ALPHABET constant and the /^[1-9A-HJ-NP-Za-km-z]{21,22}$/ character class. Reuse the constant for validation instead — e.g. check ref.length is 21–22, then return false inside the loop when BASE58_ALPHABET.indexOf(char) === -1.
• src/lib/refs.test.ts:379: This assertion doesn't exercise the base58-alphabet rejection the comment describes: ProductOperationsIOlead is 23 characters, so the {21,22} length check already rejects it before any character test. Use a 21/22-character name containing O/I/l/0 (or adjust the comment) to actually pin down that behavior.
• src/lib/refs.ts:83: The {21,22} length cap contradicts the pure-decoding rule the comment describes: a 16-byte id whose first byte is 0 base58-encodes to 23 characters (one leading 1 plus 21–22 body chars), and such an id (~1/256 of random ids) would be rejected here and misrouted to name lookup. Widen the quantifier to {21,23} — the byte-length check below already rejects anything that doesn't decode to exactly 16 bytes.

_{Share Feedback • Review Logs}

[lmjabreu]

isMalformedId relies on prose matching rather than the stable error_code (217)

Fixed in 92e7772. Keys on error_code === 217 now. Good call: the server sends 217 with a second message ("id must be UUIDv7 (version nibble mismatch)"), which the prose match would have missed.

the error_string extractor duplicates logic in isInsufficientScope, and the base58 alphabet appears twice

Done. isInsufficientScope reuses getCommsErrorString, and the validator walks the alphabet constant instead of a second character class.

ProductOperationsIOlead is 23 characters, so the {21,22} length check already rejects it

Good catch. Replaced with a 21-character name containing a capital O, on the resolver that still uses the check.

a 16-byte id whose first byte is 0 base58-encodes to 23 characters

Keeping as-is. 16 × 0xFF encodes to 22 characters and 0x00 + 15 × 0xFF also to 22, since 58²² > 2¹²⁸. Added a test encoding both so the bound is pinned rather than argued.

[lmjabreu]

@doistbot /review

[doistbot]

This PR fixes the digit-free opaque-id rejection from #65 by switching to a base58-decode check and mapping 404/409 responses to proper CliErrors, with the channel path keeping names-first resolution. One regression is worth addressing before merge:

Few things worth tightening:

• The removal of the opaque-id fallback from getDirectChannelId breaks resolveChannelId (used by tdc search --channel), which has no name fallback — a bare digit-free channel id like CbjxNkWHJBwcaVkoTCRgM that resolved before now throws INVALID_REF. Consider restoring the fallback in this path or routing search channels through resolveChannelRef, which already has the workspace id.

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

Optional follow-up notes (4)

• src/lib/refs.test.ts:1068: The 17-byte case doesn't exercise what the comment claims: base58([0x01, ...16 × 0xFF]) is 2^129 − 1, which exceeds 58^22 (≈ 2^128.87), so it base58-encodes to 23 characters and the 21–22 length check rejects it before the byte-length check runs. To pin the decode check at the same length, use a 17-byte value below 58^22 — e.g. base58([0x01, ...Array(16).fill(0x00)]) (2^128, exactly 22 characters), which only the byte-length check can reject.
• src/lib/refs.test.ts:1043: The base58 test helper re-declares the base58 alphabet that already lives in refs.ts as BASE58_ALPHABET, so the encoder can drift from the decoder it is meant to mirror. Export BASE58_ALPHABET from refs.ts (like looksLikeOpaqueCommsId already is) and reference it here, keeping a single source of truth for the alphabet.
• src/lib/api.ts:237: getCommsErrorString is typed string | null and isMalformedId only checks error_code === 217, so this template renders Comms rejected the id: null if a 217 response ever lacks error_string. The CONFLICT branch just below already guards this with ?? '409 Conflict'; give this branch the same fallback for consistency.
• src/lib/refs.test.ts:379: CustomerSuccessLeadership is 25 characters, so it can't exercise the decode-to-16-bytes path this test documents — looksLikeOpaqueCommsId bails on the length check before decoding. Since getDirectChannelId now returns null for every bare digit-free token, this line only restates the existing 'Engineering' name case and adds nothing beside the 21-character EngineeringDiscussion case. Drop it, or replace it with another 21/22-character token that actually decodes to 16 bytes.

_{Share Feedback • Review Logs}

[lmjabreu]

The 17-byte case doesn't exercise what the comment claims

Fixed in 0d1aca0. Now [0x01, 0×16] = 2¹²⁸, which is 22 characters, so only the decode can reject it.

Export BASE58_ALPHABET from refs.ts and reference it here

Done.

this template renders Comms rejected the id: null if a 217 response ever lacks error_string

Fixed. Falls back to "it does not decode to a Comms id (409)", with a test for the missing-string case.

CustomerSuccessLeadership is 25 characters ... Drop it

Done.

[lmjabreu]

@doistbot /review

[doistbot]

This PR fixes opaque-id detection in refs.ts to use the server's own base58-decodes-to-16-bytes rule instead of requiring a Cb prefix, so digit-free ids with other prefixes resolve correctly, while keeping names-first resolution on the channel path and the workspace check intact. It also maps 404 and 409 responses (including the malformed-id 409) to typed CliErrors with a new CONFLICT code, and re-wraps group lookups to preserve the existing hints. No inline issues were flagged; the changes align with the repo's error-handling and resolver conventions, the workspace/tenant guardrails are preserved on the new channel fallback, and the base58 implementation doesn't duplicate any existing dependency.

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

Optional follow-up notes (4)

• src/lib/refs.ts:273: The new isChannelNotFound and the inline instanceof CliError && error.code … checks in resolveChannelRef (L348) and resolveGroupRef (L542) are three copies of the same predicate. Extract one shared helper, e.g. isCliErrorCode(error, ...codes) in errors.ts next to isNotFound/isConflict, and reuse it: isChannelNotFound(error) becomes isCliErrorCode(error, 'CHANNEL_NOT_FOUND'), and the two catch branches read from the same predicate.
• src/lib/refs.ts:369: getDirectChannelId(ref) on line 364 already calls parseRef(ref) internally, so this second parseRef(ref) re-runs the trim/regex work for every name-typed ref — exactly the new digit-free-id path this line handles. Parse once and reuse the result (e.g. have getDirectChannelId accept an already-parsed ParsedRef, or inline the id/url handling here) to avoid the duplicate work.
• src/lib/refs.test.ts:604: These cases assert only the final CHANNEL_NOT_FOUND, never that mockGetChannel was called. Since resolveChannelRef already throws CHANNEL_NOT_FOUND from matchByName when the name lists are empty, the test would still pass if the id fallback (and its NOT_FOUND/INVALID_REF → CHANNEL_NOT_FOUND mapping) were removed entirely. Add expect(mockGetChannel).toHaveBeenCalledWith('EngineeringDiscussion') so the test actually pins the fallback path it describes.
• src/lib/refs.test.ts:893: The error.code !== 'NOT_FOUND' rethrow branch is not pinned by a test. A non-NOT_FOUND CliError (e.g. FORBIDDEN or INVALID_TOKEN from a scoped token) should pass through unchanged, but the existing workspace-mismatch test throws GROUP_NOT_FOUND inside the try, so it would still pass even if the catch were simplified to re-wrap every CliError. Add a companion test that rejects getGroup with new CliError('FORBIDDEN', ...) and asserts that code is preserved, mirroring resolveChannelRef's "lets any other id-fallback failure through" case.

_{Share Feedback • Review Logs}

[lmjabreu]

The new isChannelNotFound and the inline instanceof CliError && error.code … checks ... are three copies of the same predicate

Done in aade3f4. isCliErrorCode(error, ...codes) in errors.ts, used at all three sites.

getDirectChannelId(ref) on line 364 already calls parseRef(ref) internally, so this second parseRef(ref) re-runs the trim/regex work

Keeping as-is. It is a trim and two regexes on a short string, once per command; changing getDirectChannelId's signature for that is not worth the churn.

These cases assert only the final CHANNEL_NOT_FOUND, never that mockGetChannel was called

Good catch. Added the toHaveBeenCalledWith assertion; deleting the fallback now fails four tests where before it failed two.

The error.code !== 'NOT_FOUND' rethrow branch is not pinned by a test

Done. FORBIDDEN from getGroup passes through unchanged.

[scottlovegrove]

I wonder if some of these helper functions should have gone in the SDK instead, this way the MCP can also make use of them for checking IDs.

[doist-release-bot]

🎉 This PR is included in version 3.4.1 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[lmjabreu]

I wonder if some of these helper functions should have gone in the SDK instead, this way the MCP can also make use of them for checking IDs.

Yes they should have, and they actually are in the SDK 🙈

The SDK one has an additional check this one missed, the v7 version nibble. So EngineeringDiscussion passes the check I wrote and fails the SDK's.

There are a few error helpers that don't exist in the SDK though (isNotFound, isConflict, isMalformedId).

I've opened a draft PR that removes some of this code (#67) and I can open another one against the SDK. Let me know if you have any preferences for how the helpers should be added, otherwise I'll read the pattern there and replicate it.