Repository navigation
feat(npm): make @deepgram/cli the real Deepgram CLI - #146
dg-coreylweathers wants to merge 14 commits into
Conversation
Replaces `@lukeocodes`, who has left Deepgram, with the current DX owners: `@dg-coreylweathers`, `@GregHolmes` and `@deepgram-kiley`. All three are admins on this repo, so the rule takes effect on merge. Kept separate from #146 so that PR isn't waiting on a code owner who's no longer here.
GregHolmes
left a comment
GregHolmes
left a comment
There was a problem hiding this comment.
This PR makes @deepgram/cli a Node launcher for the Python CLI, adds npm/npx-aware update behavior, and publishes npm only after PyPI verification.
Requesting changes for two release-critical issues:
-
The next root release can still resolve published sibling packages that lack
deepctl_core.launcherandInstallMethod.NPM. Raise the sibling dependency floors on the generated release PR, add the plugin-to-update dependency, regenerateuv.lock, and enforce that work before PyPI publishing. -
On Windows, the launcher treats every cache root as trusted before executing cached
deepctl.exe. Validate ownership, ACLs, and reparse points, or disable cache reuse when equivalent validation is unavailable.
I also found first-run signal forwarding, stale llms version, and trailing-whitespace follow-ups.
| // Windows has no uid and a per-user LOCALAPPDATA. | ||
| function rootTrusted(spec, opts = {}) { | ||
| const platform = opts.platform || process.platform; | ||
| if (isWindows(platform)) return true; |
There was a problem hiding this comment.
This treats every Windows root as safe, then the launcher executes cached deepctl.exe from it. The %TEMP%\\deepgram-cli-user fallback is predictable and can be shared or reparse-point-backed. Validate ownership, ACLs, and reparse points before reuse, or disable cache reuse/fallback where equivalent validation is unavailable.
| eggs/ | ||
| .eggs/ | ||
| lib/ | ||
| # The npm launcher's sources, not a build output. |
There was a problem hiding this comment.
git diff --check origin/main...HEAD fails because these added lines carry trailing CR characters. Save them with the file expected line endings so whitespace validation stays clean.
| from enum import Enum | ||
| from pathlib import Path | ||
|
|
||
| from deepctl_core.launcher import get_launcher, launched_via_npm |
There was a problem hiding this comment.
This import makes the next deepctl release require the first published deepctl-core that contains launcher.py, but this package still declares deepctl-core>=0.1.10. PyPI deepctl-core==0.3.0 satisfies the current root floor and raises ModuleNotFoundError for this module. Raise the update and skills floors on the release PR, regenerate uv.lock, and make that dependency update an enforced release gate.
| stderr.write(`Setting up Deepgram CLI ${version} (one-time, ~1 min)...\n`); | ||
| const pip = [layout.python, '-m', 'pip', 'install', '--disable-pip-version-check', '--no-input', '-q']; | ||
| try { | ||
| runStep(python.command, [...python.args, '-m', 'venv', layout.dir], 'Creating the Python venv', VENV_HINT, spawnSync); |
There was a problem hiding this comment.
The signal handlers are installed only after resolveRunner() returns, but this synchronous setup runs inside it. A supervisor-only SIGTERM or SIGHUP during venv creation can terminate dg while the setup child continues and leaves its lock behind. Install forwarding before runner resolution and supervise setup children asynchronously; add a regression test that signals the launcher during each setup step.
Replies to Greg's review on #146installation.py:10 (sibling floors)Fixed in 64ed2cb. launcher.js:298 (Windows cache roots)Fixed in f130238. I went with your option: Windows never reuses a cache. No root is trusted, the launcher.js:625 (signals during setup)Fixed in 161dd87. Signal handlers go in before setup, setup is async (no spawnSync or sleepSync), and each setup step runs in its own session (setsid), so pip's children die with it. On a signal, the launcher removes the partial venv, releases the lock in .gitignore:18 (trailing CR)main's Stale llms version (review summary)
Also merged main, bumped engines to Node >=20, and added a Linux job that runs the launcher tests on Node 20.0.0, 20, 22 and 24. c1e6e87 |
The @deepgram/cli npm package runs deepctl with DEEPCTL_LAUNCHER set. deepctl-core gains deepctl_core.launcher, which reads that variable once and drops it from the environment. deepctl-cmd-update reports the npm install method and gives npm-shaped update advice. deepctl-cmd-plugin installs plugins into its isolated venv, because the launcher's venv is replaced on every npm version bump. deepctl-cmd-skills shows its one-time prompt even when the launcher runs deepctl through uv. Those packages now need the new core API, so their floors move to deepctl-core>=0.3.0, and plugin declares deepctl-cmd-update>=0.2.7 (with a workspace source) for InstallMethod.NPM. uv.lock is regenerated.
A zero-dependency Node launcher (Node >=20) that runs deepctl at the npm package's own version: a cached private venv, then `uv tool run --from deepctl==<version>`, then a venv it builds under a lock, then a clear error. First-run setup is async and supervised. Signal handlers go in before setup starts, and each setup step runs in its own session. A signal kills the step, removes the partial venv, releases the lock and re-raises the signal. The lock's owner.json is always replaced atomically and records the running step's process group, so on Linux a run that breaks a stale lock can stop a pip left behind by a SIGKILLed launcher. Cached venvs are reused only from roots that pass an ownership check. Windows can't verify ACLs or reparse points, so it never reuses a cache: it runs through uv, or builds a one-run venv and deletes it afterwards. The README documents one known limitation: Node resets inherited SIG_IGN before any JS runs, so `nohup dg` and `trap '' INT` can't protect deepctl. .gitignore stops ignoring npm/lib, and .gitattributes accepts that file's CRLF line endings so `git diff --check` stays clean.
…n bumps release-please now bumps npm/package.json with the root version. After PyPI serves that exact version, a publish-npm job publishes the package through trusted publishing. CI runs the launcher's tests on Node 20.0.0, 20, 22 and 24, and a cross-OS smoke test runs on Node 22. check_dependency_floors.py gains SIBLING_FLOORS: update and skills pin deepctl-core, and plugin pins deepctl-cmd-update, to the workspace version, and --fix raises them. Rule 2 now evaluates whole specifiers. Floors only hold if every changed package is actually released. So check_release_bumps.py fails when a package's src/ or non-workspace dependencies changed since its last release tag without a version bump. On the release PR it reports as the release-bump-gate check: sync-release-pr posts it as a commit status on the bot's commits, and a test.yml job covers commits people push. It runs again in the build job before PyPI. verify-published installs from PyPI alone, then runs deepctl and imports the launcher.
The README and llms-full.txt document `npm install -g @deepgram/cli` and `npx @deepgram/cli`. llms-full.txt still said 0.2.4, so it moves to web/src/data/ and a prerendered endpoint now builds it with the version from pyproject.toml, the same way index.astro already reads it. The build fails if that version can't be read. The doc-command test follows the file to its new path.
c1e6e87 to
e90e1dd
Compare
Replies to Greg's review on #146Top-level noteHeads up: I squashed the branch to 4 commits on top of current main. The final code is identical to what you reviewed, there's no merge commit, and each commit builds and passes on its own:
installation.py:10 (sibling floors)Fixed in 3a5ea7d (the floors pins themselves are in dbd3a33). launcher.js:298 (Windows cache roots)Fixed in 4e8a7c0. I went with your option: Windows never reuses a cache. No root is trusted, the launcher.js:625 (signals during setup)Fixed in 4e8a7c0. Signal handlers go in before setup, setup is async (no spawnSync or sleepSync), and each setup step runs in its own session (setsid), so pip's children die with it. On a signal, the launcher removes the partial venv, releases the lock in .gitignore:18 (trailing CR)main's Stale llms version (review summary)
Also bumped engines to Node >=20 and added a Linux job that runs the launcher tests on Node 20.0.0, 20, 22 and 24. 3a5ea7d |
Launcher failures printed as `dg: <message>` with the hint on a bare second line. They now print as `ERROR: <message>` and `HINT: <hint>`, the same two-line form the CLI uses, so a script or a person can tell the problem from the fix. That covers setup failures, an interrupted setup, a deepctl that can't start, and a deepctl that crashed.
…jobs The launcher and release jobs this branch adds pin actions/checkout to a commit whose tag is v7.0.1, but their comments said v4. They now name the tag the SHA points at, and the jobs that install uv pin its version (0.13.6) instead of taking the newest release.
The launcher used to install deepctl==<version> and let pip or uv resolve everything else from the index. Each release now ships requirements-<version>.txt in the npm package: deepctl and its workspace packages with the hashes of the files PyPI serves for them (read from its JSON API after the release is verified there), and every other dependency pinned with uv.lock's hashes. The release's publish-npm job writes it with npm/scripts/write_requirements.py, cross-checks it against the files that run built, and checks that the tarball contains it. Setup installs only from that file, with --require-hashes, --no-deps and --only-binary :all:, so a download that differs from the release is refused, nothing unlisted is installed, and no build backend runs. `uv tool run --with-requirements` ignores hashes, so with uv the launcher builds the same cached venv with `uv venv` and `uv pip install`, which check them. Setup runs in a directory the launcher owns, uv skips project configuration discovery (--no-config, with the user's or the machine's uv.toml passed back explicitly) and Python runs with -I, so running dg inside a repository can't change what gets installed, while pip.conf, uv.toml and PIP_*/UV_* mirror settings still apply. The pip fallback no longer upgrades pip first, since that download would be unhashed. Python is limited to 3.10 to 3.14, the versions the lock has wheels for. A package without its requirements file installs nothing. DEEPCTL_NPM_PYPI_SPEC, which CI uses to test against another deepctl, keeps the unhashed path. CI gains a Windows and macOS job that runs the hash-locked install from a local index (npm/scripts/local_index.py).
…rols A cache root is reused only when rootTrusted shows it is ours: a real directory, not a symlink, owned by this uid and not writable by group or others. When no root passes, the launcher used to stop with "No writable cache directory". It now builds a venv for that run in a fresh 0700 directory under the system temp directory, whose ancestors get the same ownership check, deletes it afterwards, and says on stderr which directory it would not trust and how to fix it. Nothing is written into the untrusted root. uv installs the files it extracted into its cache without hashing them again, and its default cache ($XDG_CACHE_HOME/uv) can sit in a directory others can write to. So uv always gets a launcher-owned cache: next to the venvs under the trusted root, inside the run's own directory (removed after setup, with pip running cacheless) when no root is trusted, and next to the one-run venvs on Windows. An existing cache directory that another user owns, or that group or others can write to, is not used: that run gets a one-run venv, and the hint says to remove the directory. A UV_CACHE_DIR the user set is left alone. Windows never reuses a venv. Its one-run venvs go under %LOCALAPPDATA%\deepgram-cli\venvs, and the directories the launcher creates there must be plain directories, not symlinks or junctions, before anything is built in them.
deepctl_core.launcher becomes deepctl_core._launcher, with no public re-export, so plugins don't build on it. deepctl-cmd-update, deepctl-cmd-skills, deepctl.main and the release's post-publish import check use the new name. capture_launcher() now takes the value from deepctl.main when that module has already captured DEEPCTL_LAUNCHER at startup (LAUNCHER_CAPTURED), and reads and removes the variable itself otherwise, so an entry point that captures it first keeps working.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Run the two real-filesystem tests for the Linux cache path only on Linux and macOS, as the other real-filesystem tests already are, and compare package paths against the realpath of the temp directory, which macOS reports under /private/var. Add a Windows test with a short-name (8.3) LOCALAPPDATA to show only link and junction flags decide whether a directory is safe.
… tests The launcher now tries python3.14 first, but the end-to-end and signal tests only installed fake python3.13 to python3 executables. On a machine with a real python3.14 on PATH, such as GitHub's ubuntu runners, the launcher found that one instead, built a real venv and ran a real pip, so those tests failed or timed out waiting for the fake step. The fakes now cover every name the launcher probes, from one list. Regular-expression escaping in the tests also goes through one helper that escapes backslashes and every other special character.
|
Thanks Greg. I've pushed seven commits on top; nothing you already reviewed was rewritten.
|
GregHolmes
left a comment
GregHolmes
left a comment
There was a problem hiding this comment.
This PR makes @deepgram/cli a Node launcher for the Python CLI and adds npm publishing after matching PyPI verification.
Requesting changes for three release-critical issues:
- the POSIX cache can trust a group-writable ancestor based only on matching numeric UID/GID;
- Windows still reuses an unverified uv cache;
- the release gate does not watch the npm package files it publishes.
I verified the earlier sibling-floor and cached-venv fixes, and ran 109 Node launcher tests plus 204 targeted Python tests in Docker; they pass. All GitHub checks are green, but the current tests encode these remaining unsafe paths rather than rejecting them.
| const link = st.isSymbolicLink ? st.isSymbolicLink() : false; | ||
| if (link ? !allowLinks : !st.isDirectory()) return false; | ||
| if (st.uid !== 0 && st.uid !== uid) return false; | ||
| const privateGroup = st.uid === uid && st.gid === gid && gid === uid; |
There was a problem hiding this comment.
[B1] Numeric UID/GID equality does not prove a group-writable cache is private
A cached deepctl must run only from a directory no other account can modify. This accepts a group-writable ancestor whenever its numeric GID equals the current UID, but POSIX does not guarantee that group has only one member; an administrator can add another account to it, which can replace the cache after this check.
Reject every group-writable cache ancestor unless group membership can be established safely. The minimal safe path is to use the existing one-run private-venv fallback, with a regression test for a matching numeric GID that includes another user.
|
|
||
| @property | ||
| def src(self) -> str: | ||
| return "src" if self.path == "." else f"{self.path}/src" |
There was a problem hiding this comment.
[B3] The release gate ignores the npm package’s published code
Every file shipped by @deepgram/cli must trigger a root version bump before publication. The root package checks only src/, but the npm package ships npm/bin/ and npm/lib/; a future chore: or refactor: to the launcher passes this gate without a release.
Model the root package’s published paths explicitly, including the npm tarball inputs, and add a fixture changing npm/lib/launcher.js that fails the root release-bump gate.
| dir = fs.mkdtempSync(p.join(base, `${prefix}${key}-`)); | ||
| // Windows: a uv cache kept beside the one-run venvs, held to the same | ||
| // plain-directory check (oneRunBase), so runs after the first are quick. | ||
| if (!untrusted && installer.uv) uvCache = ownedDir(p.join(p.dirname(base), 'uv-cache'), opts); |
There was a problem hiding this comment.
[B2] Windows reuses an unverified uv cache
Windows must not reuse a default uv cache unless the launcher validates its owner, ACLs, and reparse-point ancestors. ownedDir() skips those checks on Windows, so this persistent cache can contain a package artifact written by another account; uv then reuses extracted cache entries without re-hashing them.
Disable the default persistent uv cache on Windows until equivalent ACL validation exists. Use no-cache mode or a per-run cache that is deleted after setup, and add a Windows regression test proving that a pre-existing cache is never reused.
A group id equal to our uid (the user-private-group convention) does not prove the group has no other members: an administrator can add one, and Node cannot list them reliably. Any ancestor writable by group or others now has to be sticky, like /tmp. A umask-002 ~/.cache (0775) therefore fails, and the venvs go to the $TMPDIR/deepgram-cli-<uid> root, which is held to the same checks and still caches across runs. The npm README says where the cache goes in that case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
uv installs files it extracted into its cache without hashing them again, so a cache that outlives the run has to be as trusted as a venv. Windows cannot check owners, ACLs or reparse points from Node, so it no longer keeps %LOCALAPPDATA%\deepgram-cli\uv-cache: uv gets a cache inside the run's own directory, removed once setup is done. pip keeps its own cache, since with --require-hashes it hashes every file it installs. A UV_CACHE_DIR in the environment no longer switches the launcher's cache off: tools set it without the user choosing to (astral-sh/setup-uv exports it on GitHub's runners), so the launcher always passes --cache-dir, which uv puts ahead of UV_CACHE_DIR. The hash-locked Windows CI job plants a uv cache where the launcher used to keep it and fails if uv writes to it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The release bump gate watched only each package's src/. The root package also publishes @deepgram/cli, so a chore: or refactor: to npm/lib/ or npm/bin/ passed without a release. The root now also watches every entry in npm/package.json's `files`, read from HEAD so new entries are covered, with a glob matching at any depth as npm does, and package.json itself. npm/README.md and npm/LICENSE stay out, like the packages' READMEs: a docs change should not force a release. pyproject.toml is now compared on the metadata that reaches the wheel, not just dependencies: the [project] table without its version, [build-system], [tool.setuptools], and [tool.hatch]'s build and metadata tables. An entry point or requires-python change without a bump is caught too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
7a7e08b to
ea0f37f
Compare
Why
@deepgram/clion npm is the obvious name for "the Deepgram CLI", and it's broken:npm i @deepgram/clifails withnotarget. The only versions are two old betas (0.3.0-beta.0,0.4.0-beta.0) fromlukeocodes/deepgram-cli.npm i @deepgram/cli@0.4.0-beta.0installs, then crashes on start:MODULE_NOT_FOUND …/src/hooks/init/log.Coding agents try npm first. In our build-journey evals, agents asked to "use the Deepgram CLI" went to
@deepgram/cli, hit both failures, and gave up on the CLI.This PR makes
@deepgram/clithe real CLI.What it does
New
npm/package.@deepgram/clihas no dependencies and no install scripts. It providesdganddeepgram, and runs the Pythondeepctlat the same version as the npm package:~/.cache/deepgram-cli/venvs/<V>) if one is already built. This is fast and works offline.uv tool run --from deepctl==<V>. No local Python is needed.venvgets a private venv, built in place under a lock. All bootstrap output goes to stderr, sodg -o jsonanddg mcpstdout stay clean on the first run.python3-venv, or useinstall.sh/brew.DEEPCTL_NPM_RUNNER=uvx|venv|pathforces a strategy.pathuses adeepctlalready on PATH, and only when asked.Python side.
InstallMethod.NPMis detected throughDEEPCTL_LAUNCHER, whichdeepctl_core.launcherreads once and removes from the environment so it doesn't leak to child processes. With it:dg updateand the update banner tell npm and npx users the npm command.dg.Release.
npm/package.jsonalong with the root package (extra-files).publish-npmjob runs afterverify-published, so npm never points at a version PyPI can't install.npm-publish, npm pinned to 11.21.0) and sets provenance.latest, it publishes underv<maj>.<min>-maintinstead of movinglatest.CI. A new
launcherjob on Linux, macOS and Windows, added toall-checks:cmp LICENSE npm/LICENSE.Docs. npm/npx install lines in
README.mdandweb/public/llms-full.txt. The latter also fixes thedeepgram/tap/deepctl→deepgram/tap/deepgramtypo.The first npm version will be 0.4.1, from the next release PR.
0.4.1is newer than0.4.0-beta.0, solatestmoves and plainnpm i @deepgram/cliworks.Hardening
This went through five rounds of independent review, each followed by a fix round. Every finding was reproduced in Docker before it was fixed, and each fix was checked against its repro. Highlights:
Cache security.
gid == uid)./tmpfallback is0700and owner-checked.These checks block another user from planting or swapping a cached venv. Tested by pre-creating the cache, with a symlink, and with a 150-iteration swap loop.
Locking.
mkdirlock with owner, pid and host.Signals and stdio.
dg mcpover stdio,ffmpeg | dg listen, and similar pipes.Self-repair and pruning.
Testing
Docker (linux/arm64; the uvx, venv, signal and stdin cases also on amd64): 43/43 cases pass. They cover:
python3-venv(PEP 668);npx ./tgz;initializesent 1s late.Unit and lint:
check_dependency_floors.pyand actionlint are clean.Not yet run: real macOS and Windows runs, which the new
launcherjob does on this PR, and thepublish-npmjob, which runs on the first release now that the owner setup below is in place.Before the first release, owner steps (done 2026-10-23)
@deepgram/cli(set asdeepgramai): GitHub Actions, repodeepgram/cli, workflowrelease.yml, environmentnpm-publish, permission publish. Note for anyone redoing this: npm now separatespublishfromstage publish; the first attempt from the web UI granted onlystage publish, which a plainnpm publishis rejected under, so it was revoked and re-added withnpm trust github @deepgram/cli --repo deepgram/cli --file release.yml --env npm-publish --allow-publish. Verify withnpm trust list @deepgram/cli.npm-publishcreated on this repo, deployment branches limited tomain, no required reviewer.Unsupported 2023 beta. Install the Deepgram CLI with: npm i -g @deepgram/cli@latest (0.4.1 or newer). (npm Support had deprecated them on 2026-09-11 with its generic "no longer supported" text; the flag is per version and does not block publishing.)npm view @deepgram/cli dist-tags.latestis0.4.1, then turn on "Require 2FA and disallow tokens" for the package.Until the first publish,
npx @deepgram/clistill runs the old 2023 beta (now with the pointer message), so the README line goes live slightly ahead of the package.Release-PR checklist (floors can't be raised on this branch)
No published
deepctl-corecontainsdeepctl_core.launcheryet, and the floors check rejects a floor above the workspace version. Before merging the release PR that ships this:packages/deepctl-cmd-update/pyproject.toml: changedeepctl-core>=0.1.10todeepctl-core>=<new core version>.packages/deepctl-cmd-skills/pyproject.toml: changedeepctl-core>=0.2.19todeepctl-core>=<new core version>.packages/deepctl-cmd-plugin/pyproject.toml: adddeepctl-cmd-update>=<new cmd-update version>. It importsInstallMethod, includingNPM, but has never declared the dependency.uv lockandscripts/check_dependency_floors.py.A normal
pip install -U deepctlis already safe, because the root floors pin everything.Known limits and follow-ups (not in this PR)
Accepted limits:
pip install deepctlinto the same prefix overwrites npm'sdg, and brew and npm both wantdg. The npm README says to use one install method.staff, SUSEusers) fall back to a private temp dir. Any user-private-group member could interfere, which is a limit of that convention.nohup dg …doesn't survive SIGHUP the waynohup deepctl …does, because of how Node handles it. Usesetsidor deepctl directly for detached runs.Existing bugs found along the way:
dg updatetells Homebrew users to runbrew upgrade deepctl, but the formula isdeepgram.PermissionErroron~/.deepctl/plugins/venv).actions/checkoutpins are commented# v4but resolve to v7.0.1.Phase 2: PyInstaller per-platform binaries through
optionalDependencies, so npm installs need no Python or uv at all.The history includes a manual re-publish path that was added and later removed, so squash on merge.