Skip to content

refactor: Start integrating @doist/cli-core#66

Merged
scottlovegrove merged 5 commits into
mainfrom
scottl/cli-core
May 11, 2026
Merged

refactor: Start integrating @doist/cli-core#66
scottlovegrove merged 5 commits into
mainfrom
scottl/cli-core

Conversation

@scottlovegrove
Copy link
Copy Markdown
Contributor

Summary

First consumer-side integration of @doist/cli-core 0.9.0 into outline-cli. Mirrors twist-cli #208 (errors / config / spinner / output / markdown helpers) on this codebase. PR 1 of a 3-PR stack:

  1. This PR — cli-core wiring (errors, config consolidation, spinner, output, markdown)
  2. Next — global-args + ViewOptions + empty-output test helper (mirror of twist-cli #209)
  3. Next — delegate ol update to @doist/cli-core/commands (mirror of twist-cli #214)

The migration step from twist-cli #211 (updateChannelupdate_channel) is not needed: outline-cli already stores the channel under update_channel.

What's wired through cli-core now

Module Was Now
src/lib/errors.ts n/a (plain Errors) new module — local CliError subclass over cli-core's CliError<ErrorCode>. Keeps the historical positional (code, message, hints?, type?) signature. code accepts ErrorCode | CliErrorCode so cli-core's canonical codes type-check at call sites.
src/lib/config.ts n/a — config reads were split across auth.ts (api_token / base_url / oauth_client_id) and update-config.ts (update_channel), both writing the same file with their own sync fs calls new module — thin wrappers over cli-core's readConfig / writeConfig / updateConfig with 'outline-cli' app name. Single source of truth for the config schema.
src/lib/auth.ts sync fs reads + writes, racing update-config.ts on the same file async wrappers over config.ts. Public API preserved (getApiToken / getBaseUrl / getOAuthClientId / getTokenSource / saveConfig / clearConfig) — now returns promises.
src/lib/update-config.ts independent sync fs reader/writer hitting the same config file async wrappers over config.ts.
src/lib/spinner.ts local 70-line impl with yocto-spinner, manual chalk.green('✓ …') / chalk.red('✗ …') doubling, ad-hoc shouldDisableSpinner const spinner = createSpinner({ isDisabled: shouldDisableSpinner }) + destructured exports. The CLI-specific bit (OL_SPINNER opt-out + --json / --ndjson / --no-spinner fan-out, CI opt-out aware: CI='false' is treated as not-CI) is wired in as isDisabled.
src/lib/output.ts inline JSON.stringify(_, null, 2) / one log per ndjson item delegates serialization to cli-core's formatJson / formatNdjson. formatError and the essential-keys filtering layer stay local.
src/lib/markdown.ts marked@15 + marked-terminal@7 with sync marked.parse() cli-core's preloadMarkdown / renderMarkdown (async). renderMarkdown call site in document.ts now awaits.

Dependency bumps

  • Add @doist/cli-core 0.9.0
  • marked 15.0.12 → 18.0.3 (cli-core peer requires >=18)
  • Replace marked-terminal 7.3.0 (caps at marked@<16) with marked-terminal-renderer 2.2.0 — same fork that cli-core itself uses, no marked version cap
  • Drop yocto-spinner from direct deps (still resolved transitively via cli-core)
  • vitest 4.0.18 → 4.1.5 (cli-core peer requires >=4.1)

Behaviour changes worth flagging

  • yocto-spinner.success() and .error() already prepend their own / glyphs. The old spinner.ts added a manual chalk.green('✓ ') / chalk.red('✗ ') on top, which produced doubled symbols in real terminals (e.g. ✔ ✓ Done). cli-core 0.3+ drops the manual prefixes — so the success / fail line now renders as a single ✔ Done instead of ✔ ✓ Done.
  • updateConfig now uses cli-core's strict-read semantics: a broken config file throws instead of being silently overwritten (the previous lenient getUpdateChannel + setUpdateChannel composition could clobber data the user might have recovered by hand-fixing the file).
  • auth.ts and update-config.ts no longer race on writes — both go through the single config.ts wrapper.
  • CI='false' is now honoured as an opt-out (the spinner runs).

vitest config

Adds server.deps.inline: ['@doist/cli-core'] to a new vitest.config.ts. Without it, vitest treats the real-installed cli-core as external and vi.mock('@doist/cli-core', …) doesn't reach its compiled imports.

Tests

  • src/__tests__/spinner.test.ts trimmed to a single smoke test that mocks @doist/cli-core#createSpinner and asserts the kit is built with a function isDisabled. The previous ~200 lines of behavioural coverage now live in cli-core's own spinner suite.
  • src/__tests__/auth.test.ts updated for the async API; same coverage.
  • src/__tests__/output.test.ts adjusts the ndjson expectation to match cli-core's formatNdjson (single console.log with \n-joined entries rather than one log per item).
  • src/__tests__/commands.test.ts mocks auth.js with async functions; the document-get rendering assertion drops the # prefix (marked-terminal-renderer renders headings differently from the old marked-terminal).

Test plan

  • npm run type-check
  • npm run lint:check — 0 warnings, 0 errors
  • npm test118 / 118 pass
  • npm run build
  • npm run check:skill-sync
  • npm ls yocto-spinner confirms cli-core still resolves it transitively
  • Manually exercise ol auth status, ol auth login, ol update --channel against a real config
  • Run a long-running command (ol search foo) in a TTY to confirm the spinner appears and no doubled ✔ ✓ glyphs on success
  • CI=1 ol search foo, ol search foo --json, OL_SPINNER=false ol search foo to confirm spinner suppression

🤖 Generated with Claude Code

First consumer-side integration of `@doist/cli-core` 0.9.0 into outline-cli.
Mirrors twist-cli #208 — adds `errors.ts`, consolidates config, and wires
spinner / output / markdown through cli-core.

What's wired through cli-core now:

| Module | Was | Now |
| --- | --- | --- |
| `src/lib/errors.ts` | n/a (plain `Error`s) | new module — local `CliError` subclass over cli-core's `CliError<ErrorCode>`. Keeps the historical positional `(code, message, hints?, type?)` signature. `code` accepts `ErrorCode | CliErrorCode` so cli-core's canonical codes (`CONFIG_*`, etc.) type-check at call sites. |
| `src/lib/config.ts` | n/a — config reads were split across `auth.ts` (api_token / base_url / oauth_client_id) and `update-config.ts` (update_channel), both writing the same file with their own sync `fs` calls | new module — thin wrappers over cli-core's `readConfig` / `writeConfig` / `updateConfig` with `'outline-cli'` app name. Single source of truth for the config schema. |
| `src/lib/auth.ts` | sync `fs` reads + writes, two parallel writers racing `update-config.ts` | async wrappers over `config.ts`. Public API preserved (`getApiToken` / `getBaseUrl` / `getOAuthClientId` / `getTokenSource` / `saveConfig` / `clearConfig`) — now returns promises. |
| `src/lib/update-config.ts` | independent sync `fs` reader/writer hitting the same config file | async wrappers over `config.ts`. |
| `src/lib/spinner.ts` | local 70-line impl with yocto-spinner, manual `chalk.green('✓ …')` / `chalk.red('✗ …')` doubling, ad-hoc `shouldDisableSpinner` | `const spinner = createSpinner({ isDisabled: shouldDisableSpinner })` + destructured exports. The CLI-specific bit (`OL_SPINNER` opt-out + `--json` / `--ndjson` / `--no-spinner` fan-out, `CI` opt-out aware: `CI='false'` is treated as not-CI) is wired in as `isDisabled`. |
| `src/lib/output.ts` | inline `JSON.stringify(_, null, 2)` / one log per ndjson item | delegates serialization to cli-core's `formatJson` / `formatNdjson`. `formatError` and the essential-keys filtering layer stay local. |
| `src/lib/markdown.ts` | marked@15 + marked-terminal@7 with sync `marked.parse()` | cli-core's `preloadMarkdown` / `renderMarkdown` (async). `renderMarkdown` call site in `document.ts` now awaits. |

## Dependency bumps

- Add `@doist/cli-core` 0.9.0
- `marked` 15.0.12 → 18.0.3 (cli-core peer requires `>=18`)
- Replace `marked-terminal` 7.3.0 (caps at `marked@<16`) with `marked-terminal-renderer` 2.2.0 — same author-spirit fork, no marked version cap, what cli-core itself uses
- Drop `yocto-spinner` from direct deps (still resolved transitively via cli-core)
- `vitest` 4.0.18 → 4.1.5 (cli-core peer requires `>=4.1`)

## Behaviour changes worth flagging

`yocto-spinner.success()` and `.error()` already prepend their own `✔` / `✖` glyphs. The old `spinner.ts` added a manual `chalk.green('✓ ')` / `chalk.red('✗ ')` on top, which produced doubled symbols in real terminals (e.g. `✔ ✓ Done`). cli-core 0.3+ drops the manual prefixes — so the success / fail line now renders as a single `✔ Done` instead of `✔ ✓ Done`.

`updateConfig` now uses cli-core's strict-read semantics: a broken config file throws instead of being silently overwritten (the previous lenient `getUpdateChannel` + `setUpdateChannel` composition could clobber data the user might have recovered by hand-fixing the file).

`auth.ts` and `update-config.ts` no longer race on writes — both go through the single `config.ts` wrapper.

`CI='false'` is now honoured as an opt-out (the spinner runs).

## vitest config

Adds `server.deps.inline: ['@doist/cli-core']` to a new `vitest.config.ts`. Without it, vitest treats the real-installed cli-core as external and `vi.mock('@doist/cli-core', …)` doesn't reach its compiled imports.

## Tests

- `src/__tests__/spinner.test.ts` trimmed to a single smoke test that mocks `@doist/cli-core#createSpinner` and asserts the kit is built with a function `isDisabled`. The previous ~200 lines of behavioural coverage now live in cli-core's own spinner suite.
- `src/__tests__/auth.test.ts` updated for the async API; same coverage.
- `src/__tests__/output.test.ts` adjusts the ndjson expectation to match cli-core's `formatNdjson` (single `console.log` with `\n`-joined entries rather than one log per item).
- `src/__tests__/commands.test.ts` mocks `auth.js` with async functions.

## Test plan

- [x] `npm run type-check`
- [x] `npm run lint:check` — 0 warnings, 0 errors
- [x] `npm test` — 118 / 118 pass
- [x] `npm run build`
- [x] `npm run check:skill-sync`
- [x] `npm ls yocto-spinner` confirms cli-core still resolves it transitively
- [ ] Manually exercise `ol auth status`, `ol auth login`, `ol update --channel` against a real config
- [ ] Run a long-running command (`ol search foo`) in a TTY to confirm the spinner appears and **no doubled `✔ ✓` glyphs** on success
- [ ] `CI=1 ol search foo`, `ol search foo --json`, `OL_SPINNER=false ol search foo` to confirm spinner suppression

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@scottlovegrove scottlovegrove self-assigned this May 11, 2026
scottlovegrove and others added 3 commits May 11, 2026 14:53
Caching at module load pinned the path to the unmocked `node:os.homedir`
before vitest's `vi.mock('node:os', …)` hoist took effect in a fresh test
file. Local runs hit module isolation per file; CI's worker pool re-used
modules across files and surfaced the bug.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`vi.mock('node:os')` did not reliably reach cli-core's compiled `homedir()`
under CI's worker pool — locally the mock landed, in CI two tests read
the real `~/.config/outline-cli/config.json`. cli-core's `getConfigPath`
already honours `XDG_CONFIG_HOME`, so point it at a tmpdir per test
instead.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Local Claude Code harness state — accidentally landed in the previous commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@scottlovegrove
Copy link
Copy Markdown
Contributor Author

2500 of the 2700 LoC changes were package-lock.json, do not fear the pr.

Copy link
Copy Markdown
Member

@doistbot doistbot left a comment

Choose a reason for hiding this comment

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

This PR lays a solid foundation by wiring up the new @doist/cli-core module to standardize error handling, configuration, output formatting, and spinners across the CLI. Migrating these utilities significantly improves maintainability and aligns the project with broader ecosystem standards. A few areas need slight adjustments, specifically regarding configuration parsing strictness, preserving update channels when clearing auth, keeping NDJSON output memory-efficient, removing secret exposure via CLI flags, and refining some type safety and test assertions.

Share FeedbackReview Logs

Comment thread src/lib/config.ts
Comment thread src/lib/output.ts Outdated
Comment thread src/lib/auth.ts Outdated
Comment thread src/lib/auth.ts Outdated
Comment thread src/commands/document.ts
Comment thread src/__tests__/spinner.test.ts Outdated
Comment thread src/__tests__/output.test.ts Outdated
Comment thread src/__tests__/spinner.test.ts Outdated
Comment thread src/commands/auth.ts
Comment thread src/lib/auth.ts Outdated
- auth: route writes through `updateConfig`; `clearConfig` now unsets only
  auth keys instead of unlinking — preserves `update_channel` and any other
  shared config.
- errors: drop `(string & {})` from `ErrorCode`; restore closed union, rely
  on `CliError`'s `TCode | CliErrorCode` constructor for cli-core canonical
  codes.
- search: skip `getBaseUrl()` when `--json` / `--ndjson` (machine path
  doesn't render the human formatter).
- output: stream NDJSON item-by-item — restores per-item `console.log` and
  drops the full-array buffer.
- spinner test: restore env / argv coverage by invoking the captured
  `isDisabled` under each scenario; switch to `expect.objectContaining`
  with `expect.any(Function)`.
- output test: assert parsed records, not the exact `console.log`
  call shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@scottlovegrove scottlovegrove requested review from a team and gnapse and removed request for a team May 11, 2026 14:14
@scottlovegrove scottlovegrove added the 👀 Show PR PR must be reviewed before or after merging label May 11, 2026
@scottlovegrove scottlovegrove merged commit 60f3707 into main May 11, 2026
4 checks passed
@scottlovegrove scottlovegrove deleted the scottl/cli-core branch May 11, 2026 14:14
scottlovegrove added a commit that referenced this pull request May 11, 2026
…@doist/cli-core` (#67)

## Summary

Mirrors [twist-cli #209](Doist/twist-cli#209) on
top of outline's cli-core 0.9.0 baseline (PR #66). Both CLIs now share
one source of truth for global-flag parsing, env-var gating, the
`ViewOptions` shape, and the empty-machine-output test contract.

PR 2 of a 3-PR stack:
1. ✅ #66 — cli-core wiring (errors, config, spinner, output, markdown)
2. **This PR** — global-args + ViewOptions + empty-output test helper
3. Next — delegate `ol update` to `@doist/cli-core/commands` (mirror of
twist-cli #214)

## What's wired through cli-core now

| Module | Was | Now |
| --- | --- | --- |
| `src/lib/global-args.ts` | n/a | new module —
`createGlobalArgsStore()` + `createAccessibleGate({ envVar:
'OL_ACCESSIBLE' })` + `createSpinnerGate({ envVar: 'OL_SPINNER' })`. |
| `src/lib/spinner.ts` | inline `shouldDisableSpinner` reading
`OL_SPINNER`/`CI`/argv | imports `shouldDisableSpinner` from
`global-args.js`. |
| `src/lib/output.ts` | local `OutputOptions = { json?, ndjson?, full?
}` | `CoreViewOptions & { full?, raw? }`. Adds a thin
`printEmpty(message, opts)` wrapper around cli-core's `printEmpty`. |
| `src/commands/document.ts` | `ol document list` printed nothing on an
empty result | calls `printEmpty('No documents found.', outputOpts)` so
machine consumers get `[]\n` / nothing for `--json` / `--ndjson` and
humans get the message. |
| `src/__tests__/empty-output.test.ts` | n/a | new — uses
`describeEmptyMachineOutput` from `@doist/cli-core/testing` to lock in
the `ol document list` empty contract. |

## Behaviour worth flagging

- `--accessible` flag and `OL_ACCESSIBLE=1` env var are now recognised
globally (no consumer reads `isAccessible` yet — wired for parity with
twist + todoist; reserves the seam).
- `ol document list` against an empty result now prints `No documents
found.` in human mode (was silent). `--json` / `--ndjson` behaviour
matches the canonical empty contract.
- Spinner gate now lives in cli-core's `createSpinnerGate` — adds
`--progress-jsonl` and `--verbose` as additional disable triggers
(forward-compatible; no consumer in outline today).

## Test plan

- [x] `npm run type-check`
- [x] `npm run lint:check` — 0 warnings, 0 errors
- [x] `npm test` — **127 / 127** pass
- [x] `npm run build`
- [x] `npm run check:skill-sync`
- [ ] Smoke (gates):
- `OL_SPINNER=false ol search foo`, `ol --no-spinner search foo` →
spinner off
- `CI=1 ol search foo` → spinner off; `CI=false ol search foo` → spinner
ON
- [ ] Smoke (empty list contract, against an empty collection):
  - `ol document list --collection <empty>` → `No documents found.`
  - `ol document list --collection <empty> --json` → `[]`
  - `ol document list --collection <empty> --ndjson | wc -c` → `0`

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
scottlovegrove added a commit that referenced this pull request May 11, 2026
## Summary

Mirrors [twist-cli #214](Doist/twist-cli#214).
cli-core 0.9.0 owns the registry fetch, package-manager detection,
install spawn, channel switch, and `--json`/`--ndjson` envelopes.
outline provides the package name, version, config path, changelog hint,
and spinner runner.

PR 3 of a 3-PR stack — **stacked on #67**:
1. ✅ #66 — cli-core wiring (errors, config, spinner, output, markdown)
2. ✅ #67 — global-args + ViewOptions + empty-output test helper
3. **This PR** — delegate `ol update` to cli-core/commands

## Changes

- `src/commands/update/index.ts` now forwards to cli-core's
`registerUpdateCommand` with `{ packageName, currentVersion, configPath,
changelogCommandName: 'ol changelog', withSpinner }`.
- Delete `src/commands/update/action.ts` (203 lines) and
`src/commands/update/switch.ts` (37 lines) — cli-core owns both.
- Delete `src/lib/update-config.ts` (12 lines) — cli-core reads/writes
`update_channel` directly via the config path we hand it.
- `src/__tests__/update.test.ts` slims to a wrapper test (option
forwarding). ~400 lines of behaviour tests removed — covered upstream by
cli-core's update suite.
- `src/lib/skills/content.ts` documents the new `--json` flag on `ol
update --check`; `skills/outline-cli/SKILL.md` regenerated.

Net: **37 insertions / 710 deletions across 7 files.**

## Behaviour changes (mirroring twist-cli #214)

- Invalid `update_channel` on disk surfaces as `INVALID_UPDATE_CHANNEL`
from `ol update` (was silently coerced to stable).
- Errors emit canonical cli-core codes (`INVALID_FLAGS`,
`UPDATE_CHECK_FAILED`, `UPDATE_INSTALL_FAILED`).
- Both subcommands accept `--json` / `--ndjson` envelopes.

## Test plan

- [x] `npm run type-check`
- [x] `npm run lint:check` — 0 warnings, 0 errors
- [x] `npm test` — 103 tests pass
- [x] `npm run build && npm run sync:skill && npm run check:skill-sync`
— SKILL.md in sync
- [ ] Smoke `ol update --channel` — reports `stable`
- [ ] Smoke `ol update --check` — reports current vs latest with channel
line
- [ ] Smoke `ol update --check --json` — single-record JSON envelope
- [ ] Manual `ol update switch --pre-release` then `ol update switch
--stable` — flips on-disk `update_channel`

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@doist-release-bot
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 1.5.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

2 participants