feat(extensions): add the extension manager library - #520
Closed
scottlovegrove wants to merge 1 commit into
Closed
scottlovegrove wants to merge 1 commit into
scottlovegrove wants to merge 1 commit into
Conversation
First half of phase 1 of the extensions spec: the host-agnostic library that discovers, installs, upgrades, removes and runs extensions. No user-facing command is wired up yet, so nothing changes for users until the command group lands. Everything lives in src/lib/extensions/ and takes the host CLI's name, directories, version, reserved command names and first-party source through one options object, with no imports from the rest of the repo. That is what makes the later move to @doist/cli-core a file move. What it does: - discovery: one readdir plus a couple of small file reads, no subprocesses and no network, since it runs on every invocation - install: release binaries with checksum verification, git clones with npm dependency installation, and symlinks for local development, all assembled in a staging directory and moved into place only when complete - upgrade: fast-forward for clones, re-download when a release tag moves, pins honoured unless forced, and a worker pool bounding GitHub requests - remove: local installs lose only their link, clones with uncommitted work refuse without --force - dispatch: arguments passed through verbatim, the documented environment contract, node-shebang scripts run under the host's own node, and the extension's exit code returned to the caller The API token is deliberately absent from the child environment; an extension that needs it runs `td auth token view`. Tests cover all of it with real files in temporary directories, a stubbed GitHub API and a fixture extension that reports how it was called.
Member
|
doistbot
reviewed
Sep 10, 2026
doistbot
left a comment
doistbot
left a comment
Member
There was a problem hiding this comment.
Adds the self-contained extension manager library under src/lib/extensions/ — discovery, staged installs, upgrades, removal, and dispatch — with no repo imports, ahead of the user-facing td extension commands.
Few things worth tightening:
- Secrets leak across the process boundary:
buildExtensionEnvpassesTODOIST_API_TOKEN/GH_TOKENthrough to extensions despite the documented contract, and npm lifecycle scripts during dependency installs inherit the full host environment — build from a minimal allowlist and add a test asserting the token is absent. - Upgrade flow gaps: upgrades never print the trust warning the spec requires, a forced git upgrade resets the clone but leaves the stale pin in state so later plain upgrades still skip it (worth asserting in the test), and
upgradeGititself has no coverage. - Removal can destroy uncommitted work:
isDirtytreats git errors as "clean" and the no-lockfile npm fallback generates apackage-lock.jsonthat dirties the clone, soremovewrongly refuses or wrongly deletes — distinguish the error case and avoid writing the lockfile. - Smaller items: Windows spawning of
npm.cmdwithout a shell fails with EINVAL under Node's 2024 security fix, child output capture is unbounded, non-directory local install targets are accepted but can never dispatch, and^0.0.xranges are treated as minor-bounded rather than patch-only.
I also included a few optional follow-up notes in the details below.
Optional follow-up notes (15)
src/lib/extensions/install.ts:122:
moveIntoPlacedeletes the existing install (and the destination) before renaming the staged directory in. If the process dies between thermand therename, or the rename fails — e.g. on Windows a transient file lock/EPERM on the staged directory is realistic — the previously working extension is gone, contradicting the module doc's "never destroys the one already installed" guarantee. Consider renaming the old directory aside to a temp path first, then renaming staged into place, then deleting the old one, so a failure leaves the previous install recoverable.withSpinneris declared onExtensionManagerOptions("Wraps a slow operation in the host's spinner"), butcreateExtensionManagernever reads it, so the option silently does nothing. Either wire it into install/upgrade/dispatch or drop it from the options until a caller uses it.existsis defined identically in three new modules: here,install.ts(line 48), andnpm.ts(line 14). Extract one shared helper (for example, a small fs utility besiderun.ts) and import it everywhere so thestat/catch semantics have a single source of truth.isExecutableduplicates the privateisExecutableindispatch.ts(line 37) almost verbatim, differing only by a comment. Export that function fromdispatch.tsand reuse it here instead of maintaining two copies of the same platform/executable-bit check.parseRemotere-implements git-remote parsing thatparseSourceinsource.tsalso does: both use the same scp-like regex and.git-suffix stripping. Extract one sharedhost/owner/repoparser and call it from both, so the two parsing paths cannot drift._optionsis never referenced indispatchExtension. Drop the parameter and thedispatchOptionsargument forwarded atmanager.ts:219until the function actually consumes it.before = await headSha(...)runs on every non-dry upgrade too, but that branch never readsbefore;pullandresetToRemoteeach compute the same starting SHA themselves. Move the call inside thedryRunbranch to avoid an extragit rev-parsesubprocess per git extension upgrade.ifblock has no body — it only documents that a pinned tag with no release falls through toinstallGit. An empty conditional reads like missing code. Drop theifand keep the comment as a plain statement before the fall-through.upgrade()re-runs full extension discovery for every selector. EachrequireExtensioncallsfind, which callsdiscover()— a readdir plus manifest/state reads for every installed extension — soupgrade(['a','b','c'])performs three complete scans in parallel. Discover once and resolve all selectors against that single snapshot instead.readStateruns for every extension, but its result is only used inside theextension.pinned && !options.forcebranch (for the detail message). Move the call into that branch so unpinned extensions skip the state-file read.statandlstathere on every discovery. Calllstatfirst; it already identifies regular files/directories, and only follow it withstatwhen the entry is a symlink whose target must be inspected.'td--x'appears twice in this reject matrix, so the second entry is a duplicate no-op case. Drop one.TD_*variables, butbuildExtensionEnvstarts from...process.env. IfTD_TOKEN,TD_API_TOKEN,TD_USER, orTD_ACCESSIBLEis set in the environment, the matchingtoBeUndefined()assertion fails spuriously; likewiseexpect(env.TD_EXTENSION).toBe(process.env.TD_EXTENSION)passes even when an ambient value is present. Clear the relevantTD_*keys inbeforeEach(or usevi.stubEnv) so the contract is tested deterministically.if (!hasGit) returnmakes the test pass vacuously when git is missing, and the skip is invisible in the test report — a CI machine without git silently loses all clone-install coverage while everything looks green. remove.test.ts (lines 105, 116, 127) has the same pattern. Vitest reports skips properly withit.skipIf(!hasGit)/describe.skipIf(!hasGit), so use that instead of an early return.src/lib/— a new 24-file subsystem directory qualifies. Add an entry to thesrc/lib/catalog in CODEBASE.md (even a short one noting it's host-agnostic and headed to@doist/cli-core) so the map stays accurate.
Collaborator
Author
|
Closing in favour of a seven-part stack, following the slicing plan suggested above. The content is the same work with this review's feedback applied; each thread above says which slice carries its fix.
Every slice type-checks and passes its own tests on its own branch, and the tip of the stack is byte-identical to what this branch would have been. |
scottlovegrove
added a commit
that referenced
this pull request
Sep 11, 2026
Slice 1 of 7. Replaces #520, which was one 3.7k-line PR; this is the same work split along doistbot's suggested plan, with its review feedback already applied. The vocabulary every later slice builds on: the types describing an installed extension, the naming rules derived from the host binary name (`td` gives `td-<name>` directories, `td-extension.json` and `.td-manifest.json`), one parser shared by install sources and by git remotes read back off disk, manifest reading and writing including the `package.json` fallback for Node extensions, and per-extension state kept outside the extension directory so that deleting that directory by hand is still a clean uninstall. Nothing is wired up yet. See [the spec](https://github.com/Doist/todoist-cli/blob/main/docs/specs/extensions.md) for where this is going. **Stack:** #521 core · #522 discovery · #523 dispatch · #524 tools · #525 install · #526 upgrade · #527 manager
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First half of phase 1 of the extensions spec: the library that discovers, installs, upgrades, removes and runs extensions. There is no user-facing command yet, so this changes nothing for users; the
td extensiongroup and thesrc/index.tswiring follow in the next PR.Everything lives in
src/lib/extensions/and takes the host CLI's name, directories, version, reserved command names and first-party source through one options object, with no imports from the rest of the repo. That is decision 1 in the spec, and it is what makes the eventual move to@doist/cli-corea file move rather than a rewrite.What is here
readdirplus a couple of small file reads, with no subprocesses and no network, because it runs on every invocation oftdincluding the common case of no extensions at all. Kind is inferred from the directory in the spec's order: symlink or path file means local,.gitmeans git,.td-manifest.jsonmeans binary.Testing
112 tests over nine files, using real files in temporary directories rather than a mocked filesystem, a stubbed GitHub API, and a fixture extension in
src/test-support/that reports the arguments and environment it was given. The spec's named cases are covered: a mismatched checksum refuses the install and leaves nothing behind,--pinresolves the tagged release rather than the latest one, a reserved name fails before any request is made, and the trust warning precedes the first fetch.Not in this PR
The command group, root-command wiring, the command-token lookup change,
--userscoping, doctor checks, andSKILL_CONTENT. Those are the user-facing half and land next.