fix(vscode): run a private CLI copy and never replace the user's - #154
Merged
Merged
Conversation
…s (1/3) The extension replaced a newer CLI in ~/.specter/bin with its own older one on every activation. That auto-update on version mismatch has been in extension.ts since v0.6.5 and no constraint ever specified it. After the CLI-only v0.15.1 release, with the Marketplace extension held at 0.15.0, a 0.15.1 installed there reverted to 0.15.0 with no error. The shell PATH command had put ~/.specter/bin first on PATH, so the pre-push hook ran the downgraded binary, and the release that fixed the hook could not reach hook users. C-34 states the separation. The extension keeps a private copy per version under ~/.specter/cli and may do what it likes there. It never writes ~/.specter/bin/specter, a PATH binary, or specter.binaryPath; the shell PATH command may create the user copy once, when nothing is there. It declares a supported CLI range in package.json and uses any PATH or user-dir binary inside that range as is. One outside it is skipped, named in the Output channel, and left untouched. The range keeps C-27's concern rather than dropping it: deviation is allowed only within the versions the extension's wire types were built against, and the type parity tests are what make a patch release safe to accept. C-01's order now ends at the private copy. C-27's default-version rule now governs that copy. AC-80 binds the gate on both sides and that a skipped candidate is untouched. AC-81 binds that nothing automatic writes the user's copy, including Re-download and the shell PATH command on an existing file. AC-82 binds the range form and that the repository's VERSION satisfies it. spec-vscode 5.1.1 to 6.0.0, major: a conforming implementation's auto-update is now forbidden. No spec depends on spec-vscode. Dogfood drops transiently, three criteria with no annotated test until commit 2 of 3.
…e (2/3) Sixteen runtime assertions red, one vacuous. The new functions are reached through require() rather than a typed import, because ts-jest runs strict and a typed import of a missing symbol is a build failure, which is not a red test. tsc --noEmit passes on this tree; every red is a runtime TypeError or a failed expectation naming what is missing. AC-80 binds the range gate as a pure plan: a PATH or user-dir binary inside the declared range is used with no download and no write; one outside it is skipped, named with its version and the range, and appears in no write; the user dir reached through PATH counts once; an unreadable version is not a candidate; an existing private copy needs no download. AC-81 binds that nothing automatic writes ~/.specter/bin/specter: the download target with no candidate anywhere, the Re-download plan, the .exe suffix on Windows, and installUserCopy against a real temp dir, which creates the user copy when absent and leaves an existing one byte for byte. AC-82 binds satisfiesRange on both bounds, numeric compare, a pre-release suffix, and malformed ranges, and reads package.json and the repository VERSION so the shipped extension is proven to accept the shipped CLI. The malformed-range case passes today for the wrong reason: calling a missing function throws too. It is kept because it binds real behavior once the function exists, and the twelve other AC-82 and AC-80 assertions in the same file cannot pass that way. The other 319 extension tests pass unchanged. Committed with --no-verify: the hook stops on the red tests, the intended state between commits 2 and 3 of an SDD cycle.
The extension no longer replaces a binary it did not put there. Resolution is a pure plan in binaryDiscovery.ts, and extension.ts only carries it out. planBinaryResolution: the workspace setting is used as is, because it is the user's explicit choice. PATH and ~/.specter/bin/specter are candidates only while their --version satisfies the range package.json declares under specterCli.range; a candidate outside the range is recorded as skipped, named in the Output channel with its version and the range, and appears in no write. The user dir reached through PATH counts once. A file that does not answer --version is not a candidate. The private copy at ~/.specter/cli/specter-<version> is used when present and downloaded when not, and the plan's writes list can only ever name that path. satisfiesRange accepts exactly ">=A.B.C <X.Y.Z", inclusive below and exclusive above, numeric per component, pre-release suffix ignored, and throws on any other form so a malformed declaration fails the build rather than disabling the gate. package.json declares >=0.15.0 <0.16.0, and AC-82 binds that the repository VERSION satisfies it. The auto-update on version mismatch, in extension.ts since v0.6.5 and never specified, is deleted. Re-download refreshes the private copy only. The shell PATH command is the one permitted write to the user's copy: it calls installUserCopy, which creates ~/.specter/bin/specter from the private copy when nothing is there and leaves an existing file byte for byte, and it refuses to run at all when the extension is using a CLI the user already has on PATH, since prepending ~/.specter/bin would shadow it. The activation prompt to add the dir to PATH is gated the same way. resolveBinaryPath is retired so the decision has one owner; its five AC-02 cases move onto planBinaryResolution with the same expectations, the old 'cache' source now reading as 'user-dir' and 'needs-download' as a planned download of the private copy. Gates: tsc clean, eslint zero errors, jest 336 of 336, extension builds, dogfood-strict rc=0. spec-vscode is 82 of 82 under annotation counting and 79 of 82 outcome-verified; the three uncovered under strict are AC-43, AC-44, and AC-46, unchanged from before this cycle. AC-80 to AC-82 are covered on both gates. The wrapper in extension.ts cannot be unit-tested without VS Code; the plan it executes is, and the Marketplace gate covers the wrapper before publish.
An independent review of the docs found that the private copy described in C-34 is never produced on Linux or macOS. extractBinary's tar branch extracts the archive member as <dir>/specter and then chmods <dir>/specter-<version>, which does not exist, so every private download fails with ENOENT and activation stops with a modal error. The old target was ~/.specter/bin/specter, whose basename matched the member, which is why the branch never failed before. The plan's tests could not see this: they prove the plan targets the versioned path, not that the extractor writes it. This test feeds extractBinary a real tar.gz whose only member is an executable named specter, as goreleaser produces, and asks for a versioned target. Red on arrival, both cases, with the reviewer's exact ENOENT. tsc is clean, so the red is a runtime failure. Committed with --no-verify: the hook stops on the red test.
… CLI Three defects in the C-34 implementation, all found by the independent documentation review checking claims against the code. The private download never landed on Linux or macOS. extractBinary's tar branch wrote the member as <dir>/specter and chmod'ed <dir>/specter-<version>, which did not exist, so every download failed with ENOENT and activation stopped with a modal error. The branch now extracts into a scratch directory and renames the member to the versioned target. The red test from 8e29f48 is green, against a real tar.gz built the way goreleaser builds it, including a second version extracting beside the first. The shell PATH command could still change what the user's shell runs. A PATH CLI outside the range set the plan's source to private, and the command would then have created ~/.specter/bin/specter and put that directory ahead on PATH, so the shell's `specter` moved from the user's 0.16.0 to the extension's 0.15.0. "Never replaces a CLI you installed" stayed literally true and the effect was the same. shellInstallDecision now decides from the whole plan: no install and no PATH edit when the extension is using a PATH or setting CLI, or when any PATH CLI was skipped for its version; PATH edit only when the user copy already exists; install and edit only when the extension runs its own copy and nothing of the user's is on PATH. The activation prompt is gated by the same decision, and its wording no longer claims a CLI is installed at ~/.specter/bin when nothing is. Two extension commands typed a bare `specter` into a terminal, Run Reverse Compiler and View Diff, which worked before only because the old download landed on the terminal's PATH. They now invoke the binary the extension resolved, through terminalInvocation, so they work with no shell setup, and AC-45's promise that runReverse is usable in an empty workspace holds. shellInstallDecision and terminalInvocation carry unit tables, under AC-81 and AC-45. One of those tables caught a fourth defect while it was being written: an existing user copy skipped for its version was reported as an install target. Fixed in the decision, not the test. Gates: tsc clean, eslint zero errors, jest 347 of 347, extension builds.
The activation prompt chose its wording from whether the extension was on its private copy, and the shell PATH command decides from the whole plan. With a user copy present but skipped for its version, the prompt offered to install a copy the command would then refuse to write. The prompt now takes decision.install, so it offers an install only when one will happen. Found by the second independent review; tsc clean, jest 347 of 347 across five runs.
Changelog entry under Unreleased, the extension README settings and command rows and its terminal section, the root README install paragraph, the CLI reference install sentence, and QUICKSTART's install line. Each says the same thing: the extension uses a supported CLI already on PATH, otherwise downloads its own copy under ~/.specter/cli, and never changes a CLI the user installed; the shell PATH command creates ~/.specter/bin/specter when nothing is there and leaves an existing file alone. Two independent reviews against the code shaped these. The first found a blocking defect in the implementation, fixed in b1ef1c5, and five gaps here: a specter.binaryPath CLI is not range-checked; the shell PATH command does nothing when the user has a working CLI on PATH; typing specter in the integrated terminal needs the user copy when the user has no CLI of their own, while the extension's own terminal commands do not; the root README gave the same instruction twice; and QUICKSTART still said the extension auto-downloads the CLI. The second found four wording overstatements, all corrected: "no version check" is now "not checked against the supported range", "a CLI on PATH" is "a working CLI on PATH", the terminal claim is qualified for users with their own CLI, and a moved sentence about external terminals sits beside the sentence it qualifies again. QUICKSTART carried seven em dashes from before this change. The ratchet makes an edited file the editor's to bring to standard, so they are rewritten by context; none was inside a fenced block. doc-style clean on all five files.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged terminalInvocation on the pull request: it escaped double quotes inside a double-quoted path and nothing else, so a backslash, a dollar sign, or a backtick in the resolved path would have reached the user's shell live. The second review had noted the same gap. A path made only of characters no shell interprets is used bare. Anything else is single-quoted, which makes every character literal in POSIX shells and in PowerShell alike. An embedded single quote is written as '\\'' on POSIX and as '' in PowerShell, and PowerShell gets the call operator it needs to run a quoted path. The platform is a parameter, defaulting to the running one, so the table can cover both. The AC-45 table now feeds a path carrying a backslash, a dollar sign, a backtick, and a double quote and expects it back untouched inside single quotes, plus the embedded-quote form on each platform. tsc clean, jest 354 of 354.
The Unreleased entry described the private copy, the range, the Output channel, and the shell command's rules. Those are reference material and stay in the extension README and the CLI reference. The entry now states the user-visible change and the two actions: update the extension, and run the shell PATH command if you have no shell CLI.
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.
What
The VS Code extension replaced a CLI it did not install. Since v0.6.5,
extension.tsre-downloaded whenever the resolved CLI version differed from the extension's own, withspecter.autoDownloaddefaulting to true andspecter.versiondefaulting to the extension version. After the CLI-only v0.15.1 release, with the Marketplace extension held at 0.15.0, a 0.15.1 in~/.specter/bin/specterreverted to 0.15.0 on every activation with no error. The shell PATH command had put that directory first on PATH, so the pre-push hook ran the downgraded binary, and the release that fixed the hook could not reach hook users. No constraint had ever specified the auto-update.The extension now keeps the CLI it runs separate from the CLI the user's shell runs:
~/.specter/cli/specter-<version>, one file per version, and that is the only place it writes automatically.package.jsondeclares the CLI range it supports,specterCli.range, as>=0.15.0 <0.16.0. A CLI on PATH or at~/.specter/bin/specterinside the range is used as is, with no download. One outside it is skipped, named in the Output channel, and left untouched.~/.specter/bin/specteris the user's.Specter: Add CLI to Shell PATHcreates it from the private copy when nothing is there, as an explicit action, and leaves an existing file byte for byte.Specter: Re-download CLIrefreshes only the private copy.binaryDiscovery.tswhosewriteslist can only ever name the private copy;extension.tsexecutes it.The range keeps C-27's concern rather than dropping it: deviation is allowed only within the versions the extension's wire types were built against, and the existing type parity tests are what make a patch release safe to accept. A test binds that the repository
VERSIONsatisfies the declared range, so the extension that ships accepts the CLI that ships with it.How it was built
Contract first, three commits in order, then docs.
70927b3spec: spec-vscode 6.0.0, constraint C-34, amended C-01 and C-27, criteria AC-80 to AC-82. Major, because a conforming implementation's auto-update is now forbidden.db6cb7ared tests: sixteen runtime assertions red, reached throughrequireso the red is not a build failure.3a82413implementation, with the five AC-02 cases moved onto the one resolution owner.8e29f48andb1ef1c5: the first independent docs review found the private download never landed on Linux or macOS, because the extractor wrote the tar member asspecterand chmod'edspecter-<version>. Red test against a real archive, then the fix, plus a shell-command decision that refuses to shadow a user's out-of-range CLI, and terminal commands that name the resolved binary.9cae731prompt wording,ee77ff1single-quote terminal quoting after CodeQL flagged the double-quote escape as incomplete, and the docs: changelog, extension README, root README, CLI reference, QUICKSTART, after two independent reviews against the code.Verification
tscclean,eslintzero errors, jest 350 of 350, extension builds. (Theee77ff1commit message says 354; the measured count is 350. The message was written before the run finished and is left as it is rather than rewriting pushed history.)tsc, jest,go build, andspecter checkall pass at3a82413.What this does not do
It cannot repair the 0.15.0 extension already installed anywhere. Until a new extension ships, the only thing that holds on such a machine is
specter.versionset in user settings. The wrapper inextension.tsis not unit-testable without VS Code; the plan it executes is, and the Marketplace gate covers the wrapper before publish. No release is cut here; the extension publish is the maintainer's call through the VS Code gate.🤖 Generated with Claude Code
https://claude.ai/code/session_01J3X8T9AMWkTdq75Wi27Nfn