Skip to content

feat: adopt cli-core 1.4 capabilities - #56

Merged
gnapse merged 2 commits into
mainfrom
ernesto/adopt-cli-core-1-4
Aug 28, 2026
Merged

feat: adopt cli-core 1.4 capabilities#56
gnapse merged 2 commits into
mainfrom
ernesto/adopt-cli-core-1-4

Conversation

@gnapse

@gnapse gnapse commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Upgrade @doist/cli-core from 0.26.2 to 1.4.0 and adopt its latest shared capabilities:

  • add --ids-only to list commands with one clear entity ID
  • expose credential storage policies for OAuth and manual-token login
  • use cli-core directly for output-mode validation, ID output, and empty states
  • update tests, README, command help, and the bundled agent skill

Examples

Pipe-friendly ID output

# Workspace and user IDs
tdc workspaces --ids-only
tdc users --ids-only

# Thread and conversation IDs
tdc inbox --ids-only
tdc channel threads general --ids-only
tdc conversation unread --ids-only
tdc conversation list --ids-only

# Channel, member-user, and group IDs
tdc channels --ids-only
tdc channel members general --ids-only
tdc groups --ids-only

--ids-only prints one stable ID per line. Empty results print nothing. It is mutually exclusive with --json and --ndjson. Pagination notices go to stderr so stdout stays safe for pipes.

Credential storage policies

# Keep the current behavior: use the system credential manager,
# then warn and fall back to plaintext config storage if unavailable.
tdc auth login --credential-store fallback

# Require the system credential manager; do not fall back to plaintext.
tdc auth login --credential-store system

# Explicitly store the credential in the config file.
tdc auth login --credential-store plaintext

# The same policy is available for manual token entry.
tdc auth token --credential-store system

The default remains fallback for backward compatibility. The three values are also available through shell completion.

Upstream cli-core changes

@doistbot

doistbot commented Aug 27, 2026

Copy link
Copy Markdown
Member

⚠️ PR size is large: Review quality may be affected

👋 @gnapse This PR is large enough that Doistbot's review may miss details.

Reviewable diff: 367 review-load lines across 34 files (+367 / -68), excluding low-signal files. I will still run the review, but this would be easier for your colleagues to review as smaller PRs or a PR stack 😅

ℹ️ To make it easier to review, the recommended diff size is < 750 review-load lines and < 25 files changed

To be mindful of their time I would suggest you split this PR

🪄 Suggested slicing plan 👇

Split the PR into a 3-slice stack: first upgrade @doist/cli-core to 1.4.0 and add credential storage policies for auth commands, second introduce the core --ids-only infrastructure and adopt it for channel and conversation list commands, and third adopt --ids-only across remaining list commands while syncing documentation and bundled agent skills. This keeps all slices under the 25 changed files limit while preserving testability and functional boundaries.

PR order

  1. slice-1-chore-upgrade-doist-cli-core-to-1-4-0-an → base main
  2. slice-2-feat-add-ids-only-output-support-to-core → base slice-1-chore-upgrade-doist-cli-core-to-1-4-0-an
  3. slice-3-feat-adopt-ids-only-across-remaining-com → base slice-2-feat-add-ids-only-output-support-to-core

PR 1 chore: upgrade @doist/cli-core to 1.4.0 and support credential storage policies

Upgrades cli-core to 1.4.0 and wires the new CredentialStore policy options through comms token store and auth login/token commands, along with unit tests.

Files (8):

  • package-lock.json
  • package.json
  • src/commands/auth/auth.test.ts
  • src/commands/auth/index.ts
  • src/commands/auth/login.ts
  • src/commands/auth/token.ts
  • src/lib/auth-provider.test.ts
  • src/lib/auth-provider.ts

PR 2 feat: add --ids-only output support to core output infra, channel, and conversation commands

Implements core CLI support for --ids-only output mode and adopts it across channel and conversation listing subcommands.

Files (16):

  • src/commands/channel/channel.test.ts
  • src/commands/channel/index.ts
  • src/commands/channel/list.ts
  • src/commands/channel/members.ts
  • src/commands/channel/threads.ts
  • src/commands/conversation/conversation.test.ts
  • src/commands/conversation/helpers.ts
  • src/commands/conversation/index.ts
  • src/commands/conversation/list.ts
  • src/commands/conversation/unread.ts
  • src/index.ts
  • src/lib/global-args.test.ts
  • src/lib/global-args.ts
  • src/lib/options.ts
  • src/lib/output.test.ts
  • src/lib/output.ts

PR 3 feat: adopt --ids-only across remaining commands and update agent skill docs

Completes --ids-only adoption across groups, inbox, users, and workspaces list commands, and updates the README and bundled agent skill content.

Files (11):

  • README.md
  • skills/comms-cli/SKILL.md
  • src/commands/groups/groups.test.ts
  • src/commands/groups/index.ts
  • src/commands/groups/list.ts
  • src/commands/inbox.test.ts
  • src/commands/inbox.ts
  • src/commands/user.test.ts
  • src/commands/user.ts
  • src/commands/workspace.ts
  • src/lib/skills/content.ts

This plan is based on the current PR head. Keep each slice buildable and move tests with the behavior they cover.

You can use your agent of choice (Codex/Claude etc) to help you split this PR 😊 Just copy the link to this comment and ask them Can you please create a PR stack based on the suggestions in this comment

@doistbot
doistbot requested a review from frankieyan August 27, 2026 18:27
@gnapse gnapse self-assigned this Aug 27, 2026
@gnapse gnapse added the 👀 Show PR PR must be reviewed before or after merging label Aug 27, 2026

@doistbot doistbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid adoption of cli-core 1.4: list commands gain --ids-only output with early conflict validation, and OAuth/token login now expose the shared credential-store policy, with tests and docs kept in sync. Few things worth tightening:

  • In findConversationWithUser, validate output-mode conflicts up front — currently --json --ndjson only errors after API calls and session loading, so a bad flag combo can trigger requests or surface an unrelated error first.
  • Skip the workspace-wide unread fetch (and thread decoration) when using --ids-only without --unread in the thread list, since that data isn't emitted on this path and the extra request runs on every invocation.
  • The new INVALID_CREDENTIAL_STORE error code isn't registered in the ErrorCode union in src/lib/errors.ts — AGENTS.md requires new codes to be added there.

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

Optional follow-up notes (9)
  • P3 src/lib/auth-provider.ts:109: New error code INVALID_CREDENTIAL_STORE should be added to the ErrorCode union in src/lib/errors.ts (under the Auth & permissions category) per the repo's error-code convention. Because CliError accepts any string, type-check won't catch the omission, so the code only surfaces via grep.
  • P3 src/lib/output.ts:255: printEmpty is now a thin pass-through to cli-core's printEmptyCore — the only thing this wrapper adds is a required type argument that every call site must still pass (type: 'thread', etc.) and that is immediately discarded via void type. Consider dropping the wrapper and calling printEmptyCore({ options, message }) directly from the ~10 command call sites, which removes the dead parameter and keeps src/lib/output.ts to the helpers that actually add local behavior (formatJson/formatNdjson essential-field filtering, formatError, etc.).
  • P3 src/commands/auth/index.ts:30: Register the credential-store values with the existing withUnvalidatedChoices helper (and mirror it on auth login). The custom parser preserves validation, but this .option() leaves Option.argChoices unset, so tdc completion cannot suggest fallback, system, or plaintext after --credential-store. Build an Option with that helper and retain parseCredentialStore as its argument parser.
  • P3 src/commands/conversation/list.ts:41: resolveOutputMode(options) is called here only for its side effect (throwing on conflicting flags), and its return value is discarded; renderConversationList (helpers.ts:192) resolves the same options again and actually uses the result. Every other list command does const outputMode = resolveOutputMode(options). The intent is fail-fast before the network fetch, which is fine, but a one-line comment would prevent a future reader from deleting this as a no-op or double-validating unnecessarily.
  • P3 src/commands/inbox.ts:123: With --ids-only, the output only needs thread IDs, but by this point the command has already fetched the name of every channel via client.channels.getChannel(id) to build channelMap — wasted API calls unless --channel filtering is in play. The other ids-only paths in this PR skip their enrichment deliberately (e.g. conversation list asserts getWorkspaceUsers is not called, unread skips getConversation), so inbox is the odd one out. Could emit IDs before the channel-name fetch when options.channel is unset.
  • P3 src/commands/channel/members.ts:15: resolveChannelRef and getWorkspaceGroups were previously fetched in parallel (Promise.all), but the restructure now awaits resolveChannelRef first, then runs getWorkspaceGroups in parallel with fetchUsersByIds. For the default (non---ids-only) path this serializes two independent API calls and adds resolveChannelRef's latency to the critical path. You can keep the --ids-only short-circuit and still overlap the channel lookup with the groups fetch by starting getWorkspaceGroups eagerly only when not in ids-only mode, e.g. const groupsPromise = outputMode === 'ids-only' ? undefined : getWorkspaceGroups(workspaceId) before awaiting the channel, then Promise.all([groupsPromise, fetchUsersByIds(...)]).
  • P3 src/commands/channel/members.ts:18: The --ids-only early return skips getWorkspaceGroups and fetchUsersByIds, but members.test.ts has no test for it. This mirrors the conversation unread --ids-only test, which explicitly asserts getConversation/getWorkspaceUsers are not called — the same reliability guarantee applies here (ids-only should still work when a workspace contains deleted users that would fail fetchUsersByIds). Add a test asserting the member IDs are printed and neither getWorkspaceGroups nor getWorkspaceUsers is called.
  • P3 src/commands/channel/threads.ts:122: The pagination notice (More threads available. Use --cursor …) is a documented user-visible contract (README/SKILL say it goes to stderr), but threads.test.ts has no --ids-only test. A regression that routes this notice to stdout would corrupt the one-ID-per-line stream for scripting. Add a test covering the paginated case and asserting the notice lands on stderr while stdout stays clean.
  • P3 src/commands/auth/auth.test.ts:242: The invalid-value branch of parseCredentialStore (INVALID_CREDENTIAL_STORE) has no coverage, unlike sibling parse helpers (INVALID_SCOPE, INVALID_STATE, INVALID_DATE) which are all tested. tdc auth login --credential-store=typo and tdc auth token --credential-store=typo are both user-facing paths through this branch. A one-liner next to the plaintext test — asserting parseAsync rejects with code INVALID_CREDENTIAL_STORE — would cover it.

Share FeedbackReview Logs

Comment thread src/commands/conversation/helpers.ts
Comment thread src/commands/channel/threads.ts
@gnapse
gnapse merged commit 441e69c into main Aug 28, 2026
8 checks passed
@gnapse
gnapse deleted the ernesto/adopt-cli-core-1-4 branch August 28, 2026 00:55
doist-release-bot Bot added a commit that referenced this pull request Aug 28, 2026
## [3.1.0](v3.0.0...v3.1.0) (2026-08-28)

### Features

* adopt cli-core 1.4 capabilities ([#56](#56)) ([441e69c](441e69c))
@doist-release-bot

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 3.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@frankieyan frankieyan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released 👀 Show PR PR must be reviewed before or after merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants