Skip to content

feat(extensions): discover installed extensions - #522

Merged
scottlovegrove merged 2 commits into
feat/extensions-1-corefrom
feat/extensions-2-discovery
Sep 11, 2026
Merged

feat(extensions): discover installed extensions#522
scottlovegrove merged 2 commits into
feat/extensions-1-corefrom
feat/extensions-2-discovery

Conversation

@scottlovegrove

@scottlovegrove scottlovegrove commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Slice 2 of 7, on top of slice 1.

Discovery walks the extensions directory and works out what each entry is: a symlink or path file is a local install, a .git directory is a clone, anything else is a release binary. It runs on every invocation of the CLI, including the common case of no extensions at all, so it costs one readdir plus a couple of small file reads and never spawns a process or touches the network. The git remote is read out of .git/config rather than by asking git.

Also adds the test fixture helpers, which build real extension directories in a temporary location so later slices can install and run something genuine rather than a mock.

Stack: #521 core · #522 discovery · #523 dispatch · #524 tools · #525 install · #526 upgrade · #527 manager

@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.

This slice adds extension discovery that classifies each entry in the extensions directory (local install via symlink/path file, git clone via .git, or release binary) with a lightweight, read-only scan that runs on every CLI invocation, plus test fixture helpers for building real extension directories. The implementation reuses the existing slice-1 helpers well and covers the main cases. Few things worth tightening:

  • Resolve relative targets in path files against the path file's own directory (resolve(dirname(entryPath), target)) instead of process.cwd(), so local installs behave deterministically regardless of where td is invoked.
  • Check that .git is a directory (e.g. via the existing isDirectory() helper), not just that it exists, so binary installs containing a .git file aren't misclassified as clones.
  • Only treat a missing extensions directory (ENOENT) as "no extensions" and rethrow other readdir failures, so permission/I/O errors don't silently masquerade as an empty install.
  • Bound the concurrency of per-entry discovery instead of fanning out with Promise.all, and in the Node fixture prefer process.exitCode (or a sync write) over process.exit so the JSON report isn't truncated on pipes.

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

Optional follow-up notes (3)
  • P3 src/lib/extensions/discover.test.ts:101: This test unconditionally creates a directory symlink, which fails on Windows without elevated rights or Developer Mode — the exact limitation the Windows-style path-file test works around. Guard it with it.skipIf(process.platform === 'win32') (or another POSIX-only guard) so the suite stays runnable on Windows.
  • P3 src/test-support/extension-fixture.ts:73: This fixture re-derives the extension naming conventions that are already the source of truth in src/lib/extensions/source.ts: ${binName}-${name} (line 73), ${binName}-extension.json (line 84), and .${binName}-manifest.json (line 90). Import and use toDirName, authoredManifestFileName, and manifestFileName instead, so the fixture cannot drift from the real on-disk contract.
  • P3 src/lib/extensions/discover.ts:113: For git installs, readGitRemote runs after the Promise.all that already reads the authored manifest and state, so the .git/config read is serialized behind those independent reads. Add it as a fourth entry in that Promise.all (resolving only for kind === 'git') so all per-extension file reads complete concurrently.

Share FeedbackReview Logs

Comment thread src/lib/extensions/discover.ts Outdated
Comment thread src/test-support/extension-fixture.ts Outdated
Comment thread src/lib/extensions/discover.ts Outdated
Comment thread src/lib/extensions/discover.ts Outdated
Comment thread src/lib/extensions/discover.ts Outdated
@scottlovegrove
scottlovegrove added this pull request to stack #528 September 10, 2026 16:03
@scottlovegrove
scottlovegrove force-pushed the feat/extensions-2-discovery branch 2 times, most recently from 4625112 to 19be1db Compare September 10, 2026 16:18
@scottlovegrove
scottlovegrove force-pushed the feat/extensions-2-discovery branch from 19be1db to ef817c2 Compare September 10, 2026 16:51
@scottlovegrove
scottlovegrove force-pushed the feat/extensions-2-discovery branch from ef817c2 to 3d0fca8 Compare September 10, 2026 16:56
@scottlovegrove
scottlovegrove removed this pull request from stack #528 September 10, 2026 16:57
@scottlovegrove
scottlovegrove added this pull request to stack #529 September 10, 2026 16:58
@scottlovegrove
scottlovegrove removed this pull request from stack #529 September 10, 2026 16:59
@scottlovegrove
scottlovegrove added this pull request to stack #530 September 10, 2026 16:59
Discovery walks the extensions directory and works out what each entry is:
a symlink or path file is a local install, a .git directory is a clone,
anything else is a release binary.

It runs on every invocation of the CLI, including the common case of no
extensions at all, so it costs one readdir plus a couple of small file
reads and never spawns a process or touches the network. The git remote is
read out of .git/config rather than by asking git.

Also adds the test fixture helpers, which build real extension
directories in a temporary location so later slices can install and run
something genuine rather than a mock.
Review feedback on the discovery slice:

- resolve a relative path-file target against the path file's own
  directory, so a local install points at one directory regardless of
  where the CLI was run from
- require .git to be a directory, so a release binary that ships a file
  by that name is not misread as a clone
- only treat a missing extensions directory as "none installed", and let
  a permission or I/O failure surface instead of looking like an empty
  install
- bound how many extensions are inspected at once, and read each one's
  manifests, state and git remote together rather than in sequence
- add findExtension, which describes only the entry asked for, so
  dispatch and remove stop inspecting every installed extension
- carry the pinned ref through discovery, so callers do not read the
  state file a second time to learn it

The fixture now uses the naming helpers rather than repeating the
conventions, and sets exitCode instead of calling process.exit, which
could truncate a report written to a pipe.
@scottlovegrove
scottlovegrove force-pushed the feat/extensions-2-discovery branch from 3d0fca8 to 2aff7ec Compare September 11, 2026 09:43
@scottlovegrove
scottlovegrove merged commit b6906dc into next Sep 11, 2026
6 checks passed
@scottlovegrove
scottlovegrove deleted the feat/extensions-2-discovery branch September 11, 2026 10:03
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
scottlovegrove added a commit that referenced this pull request Sep 11, 2026
Slice 3 of 7, on top of slice 2.

The execution half of the contract: arguments reach the extension
exactly as the user typed them, the three standard streams are the
host's own, and the host exits with whatever the extension exited with.

Node-shebang scripts are launched with the host's own Node, which makes
them work on Windows and pins them to the Node version the CLI already
requires. Other scripts run directly on POSIX and through `sh` on
Windows.

The environment adds the documented `TD_*` contract. No credential is
ever injected: an extension that needs a token runs `td auth token
view`. A token the user exported themselves is inherited like any other
variable, which the spec records as a deliberate exception, and there is
now a test pinning that behaviour so it cannot change by accident.

**Stack:** #521 core · #522 discovery · #523 dispatch · #524 tools ·
#525 install · #526 upgrade · #527 manager
scottlovegrove added a commit that referenced this pull request Sep 11, 2026
Slice 4 of 7, on top of slice 3.

The outside world an install has to talk to, plus the one operation that
needs no installer.

- A subprocess wrapper that captures a bounded tail of output, since npm
runs extension lifecycle scripts that could otherwise print without end.
- Git operations, with working-tree state reported as clean, dirty or
unknown, so that "could not tell" is never mistaken for "safe to
delete".
- npm dependency installation, run without the CLI's own credentials,
invoked through the command interpreter on Windows because a `.cmd` shim
cannot be spawned directly on supported Node versions, and without
writing a lockfile into a clone that has none.
- The GitHub client: releases, asset downloads and checksum
verification.
- Removal, which deletes only the link for a local install and refuses a
clone with uncommitted or unreadable state unless forced.

**Stack:** #521 core · #522 discovery · #523 dispatch · #524 tools ·
#525 install · #526 upgrade · #527 manager
scottlovegrove added a commit that referenced this pull request Sep 11, 2026
Slice 5 of 7, on top of slice 4.

Installs from a release binary, a git clone, or a local directory.

Everything is assembled in a staging directory and moved into place only
once complete, with the previous install moved aside rather than
deleted, so a failure part-way through leaves the working extension
recoverable. The trust warning is printed before anything is downloaded,
cloned or executed.

Release assets are matched by platform and architecture, verified
against the release's checksums when it publishes any, and recorded in a
manifest that carries the author's description and version requirement.

**Stack:** #521 core · #522 discovery · #523 dispatch · #524 tools ·
#525 install · #526 upgrade · #527 manager
scottlovegrove added a commit that referenced this pull request Sep 11, 2026
Slice 6 of 7, on top of slice 5.

Clones fast-forward, release binaries are re-downloaded when the tag
moves, and local installs are left alone because the user's own
directory is already the source of truth.

Pins are honoured unless forced, and a forced upgrade clears the pin it
just moved past rather than leaving a stale one to skip every later
upgrade. Dependencies are reinstalled only when `package.json` or the
lockfile changed. Upgrading many extensions bounds how many talk to
GitHub at once, and warns about trust once for the whole run rather than
once per extension.

**Stack:** #521 core · #522 discovery · #523 dispatch · #524 tools ·
#525 install · #526 upgrade · #527 manager
scottlovegrove added a commit that referenced this pull request Sep 11, 2026
Slice 7 of 7, on top of slice 6. This completes the library half of
phase 1.

Ties the slices together behind `createExtensionManager`, which is the
only thing a host CLI needs to call.

Includes the version-range check behind an extension's `requires` field.
A range the running CLI does not satisfy is a warning rather than a
refusal: upgrading the CLI should not silently break an extension that
still works.

Also records the subsystem in `CODEBASE.md`, including why nothing in
the directory imports from the rest of the repo.

Next, in a separate PR: the `td extension` command group, the
`src/index.ts` wiring, the command-token lookup change, `--user`
scoping, doctor checks and `SKILL_CONTENT`.

**Stack:** #521 core · #522 discovery · #523 dispatch · #524 tools ·
#525 install · #526 upgrade · #527 manager
@doist-release-bot

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 5.4.0-next.1 🎉

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants