feat(update): offer to upgrade a stale CLI during openspec update - #1470
Merged
Conversation
Instruction files are generated by the installed CLI, so running `openspec update` against an outdated global install printed "All 1 tool(s) up to date (v1.6.0)" while the workflows newer releases ship were never written. Users read that as success and reported the missing workflows as bugs. `openspec update` now checks the npm registry alongside the update and, when the installed CLI is behind, prints the upgrade command instead of leaving the up-to-date line to speak for itself. The check never gets in the way: it runs concurrently with the update, times out after 1.5s, caches the answer for 24h, returns null on any failure, and is skipped in CI, under tests, and whenever OPENSPEC_NO_UPDATE_CHECK is set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI now checks npm for newer published versions during ChangesCLI update check
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant OpenspecUpdate
participant VersionCheck
participant NpmRegistry
participant UpdateCommand
User->>OpenspecUpdate: run openspec update
OpenspecUpdate->>VersionCheck: check for available CLI update
VersionCheck->>NpmRegistry: fetch published version
NpmRegistry-->>VersionCheck: return validated version
OpenspecUpdate->>UpdateCommand: execute project update
VersionCheck-->>OpenspecUpdate: return newer version or null
OpenspecUpdate-->>User: display installation guidance when newer
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying openspec-docs with
|
| Latest commit: |
561647f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1d602bdb.openspec-docs.pages.dev |
| Branch Preview URL: | https://fix-update-flags-stale-cli.openspec-docs.pages.dev |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The hint assumed a global install. A project-local dependency is now pointed at that dependency instead of `npm install -g`, and every hint prints the directory the running CLI was loaded from, so anyone who upgraded but still runs an old pnpm/volta/npx shim can see which copy answered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/update-flags-stale-cli.md:
- Around line 7-11: Add the text language identifier to the fenced output block
in the changeset so its opening delimiter is explicitly marked as text, while
preserving the existing CLI update instructions.
In `@src/core/version-check.ts`:
- Line 24: Update the CI opt-out check in version-check.ts to recognize both
CI=true and CI=1 as enabled values, while preserving the existing early return
behavior. Add or update the test in test/core/version-check.test.ts covering
CI=1 and verify that no registry request is made there.
- Around line 105-107: The version cache is being refreshed when a cached value
is read, creating a sliding TTL. In src/core/version-check.ts lines 105-107,
update the version-check flow to call writeCachedVersion only when
fetchLatestVersion retrieves a fresh registry value, while preserving the
cached-value path. In test/core/version-check.test.ts lines 94-102, advance
mocked time beyond the TTL and assert that a new registry fetch occurs.
- Around line 51-54: The prerelease comparison in src/core/version-check.ts
lines 51-54 must follow SemVer ordering by splitting identifiers, comparing each
component in sequence, and comparing numeric identifiers numerically; preserve
stable-release precedence and lexical comparison for nonnumeric identifiers. Add
coverage in test/core/version-check.test.ts lines 24-28 for beta.10 versus
beta.2 to verify numeric ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: afd3f901-c17b-4aa7-ab55-f30f8eeddace
📒 Files selected for processing (5)
.changeset/update-flags-stale-cli.mddocs/cli.mdsrc/cli/index.tssrc/core/version-check.tstest/core/version-check.test.ts
CodeQL flagged the version-check cache twice: a predictable path in the shared OS temp dir (js/insecure-temporary-file, high) and registry data written to that file (js/http-to-file-access, medium). `openspec update` is a rare, human-run command, so the cache bought little — removing it resolves both alerts outright and deletes the code that needed them. Also from review: CI=1 now opts out alongside CI=true, and prerelease tags compare per SemVer (dot-separated identifiers, numeric compared numerically) so 1.7.0-beta.10 outranks 1.7.0-beta.2. Build metadata is ignored per spec. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/version-check.ts`:
- Line 22: Make the version-check guard in src/core/version-check.ts at lines
22-22 skip the registry check whenever process.env.CI is defined, removing
reliance on the CI_ENABLED_VALUES allowlist. Update
test/core/version-check.test.ts at lines 165-170 so CI=false expects null and
verifies that no fetch occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a8b2b13e-5c0d-4238-aed7-893ddfb989d7
📒 Files selected for processing (3)
.changeset/update-flags-stale-cli.mdsrc/core/version-check.tstest/core/version-check.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/update-flags-stale-cli.md
Adversarial review found the check could never fire: the request sent `accept: application/vnd.npm.install-v1+json`, which npm serves only on the full packument — on `/<pkg>/latest` it answers 406, so every real run returned null. Every test mocked fetch, so nothing caught it. The header is gone, and a new suite exercises the real fetch path against a local HTTP server, including an assertion that we never send that Accept type. Also from review: - Validate the published version against a strict SemVer pattern before printing it. It lands in the terminal beside an install command, so an unvalidated string could smuggle ANSI cursor controls and repaint the surrounding lines. - Honor DO_NOT_TRACK=1 and OPENSPEC_TELEMETRY=0, the opt-outs telemetry already respects, and update SECURITY.md, which promised telemetry was the only network egress. - Anchor project-local detection on the path being updated and its ancestors instead of process.cwd(), so `openspec update <path>` and workspace sub-packages with a hoisted root node_modules are no longer told to install globally. It can no longer throw when the working directory has been deleted. - Send npx/dlx users `npx @fission-ai/openspec@latest update` rather than advice that would create the global install they avoided. - Query npm_config_registry when set, so private mirrors get an answer their own install command can deliver. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/core/version-check.ts (3)
174-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCanonicalize both sides before the containment check.
installDircomes fromrequire.resolve, which returns a realpath, whilediris onlypath.resolve(projectPath). Under a symlinked project root (/tmp→/private/tmpon macOS, symlinked workspace checkouts), the prefix compare misses and a project-local install gets the globalnpm install -ghint. Apath.relative-based containment check also avoids sibling-prefix surprises.As per coding guidelines: "When asserting existing filesystem paths as identities, canonicalize both actual and expected paths first using
FileSystemUtils.canonicalizeExistingPath()in project code".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/version-check.ts` around lines 174 - 184, Update the containment logic in the shown directory-walk block to canonicalize both installDir and the project path using FileSystemUtils.canonicalizeExistingPath() before comparison. Use path.relative-based containment so sibling directories cannot match by prefix, while preserving the existing parent traversal and boolean results.Source: Coding guidelines
212-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
bunx/pnpm dlxusers get annpxcommand.Detection distinguishes
_npx,dlx, and_bunx, but the rendered hint always saysnpx. Returning the matched runner from the detector would let the hint saypnpm dlx …/bunx ….🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/version-check.ts` around lines 212 - 213, Update the ephemeral-runner detection and version-hint generation in the version-check flow so the detector returns the matched runner identity, not only a boolean. Use that result when constructing the command in the isEphemeralRunnerInstall branch, preserving npx for _npx installs while rendering pnpm dlx or bunx for their respective detected runners.
116-124: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDeclaring a supported Node engine is still recommended.
@fission-ai/openspecenforces>=20.19.0, but@fission-ai/openspec-websitehasengines: null, so an environment could run the site script without a compatible Node globally while still using package engines for installs. Add an explicitpackage.json#engines.nodeguard if older runtimes are intended to be unsupported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/version-check.ts` around lines 116 - 124, Declare an explicit engines.node requirement in the openspec-website package.json matching the supported Node baseline of >=20.19.0. Keep the existing package metadata unchanged and ensure the site script inherits this install/runtime compatibility guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/version-check.ts`:
- Around line 174-184: The isProjectLocalInstall path check must canonicalize
both installDir and the resolved project directory, then use path.relative-based
containment rather than raw startsWith; update src/core/version-check.ts lines
174-184 accordingly. In test/core/version-check.test.ts lines 302-319, construct
paths with platform-safe path.join/path.resolve or FileSystemUtils.toPosixPath
and add an alias/symlink regression case covering canonical path identity.
In `@test/core/version-check.test.ts`:
- Around line 187-194: Update the environment setup and restoration around the
version-check tests to also snapshot, clear, and restore
OPENSPEC_NO_UPDATE_CHECK, DO_NOT_TRACK, and OPENSPEC_TELEMETRY alongside
NODE_ENV, CI, and npm_config_registry. Ensure isCheckEnabled() runs without
contributor-specific opt-outs so the mocked request and its headers remain
available.
---
Nitpick comments:
In `@src/core/version-check.ts`:
- Around line 174-184: Update the containment logic in the shown directory-walk
block to canonicalize both installDir and the project path using
FileSystemUtils.canonicalizeExistingPath() before comparison. Use
path.relative-based containment so sibling directories cannot match by prefix,
while preserving the existing parent traversal and boolean results.
- Around line 212-213: Update the ephemeral-runner detection and version-hint
generation in the version-check flow so the detector returns the matched runner
identity, not only a boolean. Use that result when constructing the command in
the isEphemeralRunnerInstall branch, preserving npx for _npx installs while
rendering pnpm dlx or bunx for their respective detected runners.
- Around line 116-124: Declare an explicit engines.node requirement in the
openspec-website package.json matching the supported Node baseline of >=20.19.0.
Keep the existing package metadata unchanged and ensure the site script inherits
this install/runtime compatibility guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: acc12657-a6f6-4b20-ac10-20ef07bf3297
📒 Files selected for processing (6)
.changeset/update-flags-stale-cli.mdSECURITY.mddocs/cli.mdsrc/cli/index.tssrc/core/version-check.tstest/core/version-check.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cli/index.ts
- docs/cli.md
- .changeset/update-flags-stale-cli.md
The new fixtures mixed unresolved POSIX literals with path.join output. On Windows path.resolve adds a drive letter and path.join does not, so the prefix match could never succeed and two assertions failed there. Fixtures now derive from resolved roots. Real installs were unaffected — both sides come from resolved absolute paths — but case and drive-letter casing can still differ between require.resolve and path.resolve on Windows, so the comparison is now case-insensitive on win32. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verification found the 1.5s timeout did not bound the command. Aborting a fetch still completing its TCP handshake — a firewall dropping packets, a captive portal — leaves the connect handle ref'd, so `openspec update` sat for ~10s after printing everything. Measured against an unroutable address: resolved at 1523ms, process exited at 10558ms. The request now uses node:http(s), whose socket the timeout can actually destroy: same probe resolves at 1547ms and exits at 1550ms. Because the client is no longer fetch, the mocked tests would have gone inert and silently reached the real registry. The whole suite now drives the real code path against a local server, which is also the only way to prove an opt-out sent nothing. Added a child-process guard for the teardown itself (no in-process assertion can see it), a case for a non-JSON body — the captive-portal login page — and order-independence fixes: the mock leak between describes made the 406 regression guard the first casualty under --sequence.shuffle. Also: bound the version pattern and the response body so neither can be absurdly long, and narrow the ephemeral-runner match so a user directory named "dlx" is no longer mistaken for a pnpm cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Being told to run a command, then run the update again, is two steps the
CLI can take for you. `openspec update` now asks:
A newer OpenSpec CLI is available (v1.6.0 -> v1.7.0).
Running from: /usr/local/lib/node_modules/@fission-ai/openspec
? Upgrade to v1.7.0 now? (Y/n)
Yes runs `npm install -g` with stdio inherited — so any auth or sudo
prompt reaches the user directly — then re-runs the update with the new
CLI, because this process still holds the old templates and cannot write
the new workflows itself. No prints the command and updates with the CLI
you have.
It asks rather than acting: a CLI that mutates a global install without
consent is the wrong default. Guards:
- Interactive terminals only, via the repo's isInteractive() (no TTY, or
CI set, means the note prints exactly as before).
- Global npm installs only. A project dependency belongs to that
project's package manager, and an npx/dlx cache has nothing to
upgrade; both get the command instead.
- The re-run carries OPENSPEC_NO_UPDATE_CHECK=1, so a PATH that still
resolves to the old binary cannot loop.
- A failed upgrade, a missing openspec on PATH, and Ctrl-C at the prompt
each fall back to the printed command rather than an error.
The check now runs before the update rather than alongside it, so an
accepted upgrade regenerates files with the new templates in one pass.
Verified end to end against a stubbed npm and openspec on PATH, both
answers, plus the unchanged non-interactive path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found `canSelfUpgrade` treated "not a project dependency and not an npx cache" as proof of a global npm install. It is not: a pnpm, bun, yarn or volta global, and a plain git clone, all qualified. Reproduced by running the CLI from this repo — it offered to npm install -g over the checkout, which would have shadowed it with a second copy. The offer now requires npm to own the install, derived from the running node's global root (and APPDATA/npm_config_prefix) rather than by shelling out to `npm prefix -g`. Everything else gets the command that matches how it was installed — `pnpm add -g`, `bun add -g`, `yarn global add`, `volta install` — a project dependency is pointed at its own package manager with no npm command at all, and a source checkout gets no note, since its version is whatever the branch says. Docs corrected where they had drifted from the code: - The check runs before the update, not alongside it; it can delay the update by up to 1.5s. docs/cli.md and the changeset said otherwise. - npm_config_registry is only honored when npm exports it; an .npmrc setting alone is invisible to us. Docs and JSDoc claimed more. - SECURITY.md gains an "Installing software" row: running a package manager on the user's behalf is the most security-relevant behavior here and the table did not mention it. The "Running other programs" row now covers the re-run's path argument and cross-spawn's Windows shim escaping, and the network row lists every opt-out precisely. - troubleshooting.md's "Commands don't show up" — the exact symptom this PR exists to fix — now explains that instruction files come from the installed CLI, and installation.md's Updating section links onward. - The env-var table notes the CI and NODE_ENV skips, and that npm_config_registry must be an http(s) URL. - "the new workflows land in the same command" no longer overpromises: when the upgraded openspec is not on PATH, the CLI now says the files were not regenerated instead of printing a dim aside. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adversarial review found the flow could claim success it had not earned, and could strand a non-interactive caller. All verified by running the CLI, all fixed: - `npm install -g` exits 0 even when it installs nothing, so "✓ Upgraded to vX" was an assertion, not a fact. The version is now read back from the installed binary; when another install earlier on PATH still answers with the old one — the exact silent staleness this feature exists to fix — it says so instead of claiming the upgrade landed. - The prompt hung forever under `openspec update > log.txt`: the question went to the file while the user watched a blank terminal. The offer now requires stdout to be a terminal too. - Ctrl-C at the prompt read as "no thanks" and carried on into the next prompt. It now stops the command with 130. - `--force` never reached the re-run, so `openspec update --force` could regenerate nothing and exit 0. Flags are forwarded, with `--` before the path so a flag-shaped path stays a path. - A signal-killed re-run, and a re-run with no CLI to hand off to, both reported 0. Both now report failure. - `process.exit()` skipped commander's postAction hook, killing the telemetry flush mid-request. The action sets process.exitCode and returns instead. - The check read only npm_config_registry, which npm exports only under `npm run` — so an enterprise user with a mirror in .npmrc got an unannounced call to public npm. It now reads .npmrc too. - Two different CI predicates: `CI=yes` suppressed the prompt but not the request. One predicate now, and it treats any value except an explicit off-value as CI. - A project-local install was offered a global one when updating a different directory; both anchors are checked now. Tests: the re-run had no coverage at all and now has four cases. Mutation testing over nine mutations (406 header, DO_NOT_TRACK, version validation, prerelease ordering, canSelfUpgrade, the anti-loop env guard, the cwd-vs-target anchor, the timeout) — one survived, the anti-loop guard, so it has a test now and the mutation dies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cmd.exe echoes `%*` with every argument quoted, so the Windows job saw `"update" "--force" "--" "--weird-path"` and the substring assertion for `-- --weird-path` failed. The forwarding itself was correct on both platforms; the assertion now splits and unquotes before checking that the separator immediately precedes the path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeQL flagged file data reaching an outbound request, and it has a point: the project `.npmrc` travels with the repository, so honoring it let a cloned repo choose where the version check sends its request. Only `~/.npmrc` is read now — which is where a mirror is configured anyway, since `npm config set registry` writes there — and a test pins that a project `.npmrc` cannot redirect the request. Docs and changeset say so explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
I think telling them to upgrade is fine. I think only issue is if they've used a different package manager or homebrew or something like that. I think ideally we want to automatically run this for them, but we will probably need to do some work to understand their installation and save that somewhere. (similar to however claude code does it) |
TabishB
previously approved these changes
Jul 28, 2026
…path
Adversarial review found the offer never appeared on Homebrew — a
mainstream macOS install — and I reproduced it on this machine:
npm root -g: /opt/homebrew/lib/node_modules
derived roots: /opt/homebrew/Cellar/node/25.8.1_1/lib/node_modules
process.execPath is realpath'd through Homebrew's symlink into the
Cellar, so a root derived from the node binary never matches the prefix
npm installs into. The same mismatch hits Debian-style layouts.
The install's own shape is now the primary signal:
<prefix>/lib/node_modules/<pkg> (POSIX) or <prefix>/node_modules/<pkg>
(Windows), confirmed by the bin directory npm would have written the
shim into. The node-derived roots stay as a fast path.
Also from the same review, each reproduced first:
- volta nests a whole node install, so its packages sit in exactly npm's
layout: we called it npm-owned, ran `npm install -g`, and on failure
told the user to run volta. Ownership is now decided before location.
- upgradedBinPath returned the first prefix that merely had an openspec
in it, preferring a stale one over the prefix npm just wrote to. It
now derives from the running install first.
- readCliVersion took the first version-shaped token anywhere in stdout,
so a wrapper banner ("Node.js v25.8.1 | OpenSpec") was read as the
answer — turning a real upgrade into a false "still reports vX", or
worse, claiming success for a version nobody installed. It now takes
the line that is only a version.
- The probe child could outlive its 5s timeout indefinitely: SIGTERM
with no escalation and no unref, so a signal-trapping wrapper held the
CLI open for as long as it ran.
- "Another install earlier on your PATH is answering first" was a
misdiagnosis whenever we had asked a known binary directly.
- A `registry=${VAR}` or `@scope:registry=` line in .npmrc — both npm's
documented syntax, the latter being how a scoped package is normally
routed to a mirror — silently fell back to the public registry.
- A 3xx from the registry disabled the check permanently and silently.
Redirects are followed, bounded, under one timeout budget.
- An incidental directory named "pnpm" or "yarn" was read as a global
install of one, printing the wrong upgrade command.
Plus the earlier docs-audit round: the npx branch no longer tells users
to run an update they were just handed, the check no longer fires for a
source checkout whose answer is discarded, the offer gate moved into a
tested pure function, and the declined command now prints below the
update output instead of scrolling away above it.
Docs: install-flavor table, CI off-values, empty-value opt-out, the
"no cache" fact in SECURITY.md, and a changeset trimmed to a summary
that points at the CLI reference. The changeset is now `minor` — this
adds a prompt, an env var, an outbound request, and the ability to
install software; that is not a patch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
clay-good
changed the title
CodeQL flagged file data reaching an outbound request (js/file-access-to-http),
and it is right that a file choosing where a request goes is a flow worth
avoiding. The convenience did not earn it: reading ~/.npmrc needed three
follow-up fixes in one review round (project-vs-user precedence, ${VAR}
expansion, scoped registry keys), and none of it is necessary — anyone on a
private mirror can export npm_config_registry, which is still honored, or
turn the check off.
Removes the .npmrc read and its two helpers; a test pins that a
registry= line in a .npmrc cannot steer the request.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
approved these changes
Jul 28, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed exact head 561647f. The registry check is bounded and opt-out aware, self-upgrade is limited to interactive npm-owned global installs, subprocess arguments stay shell-safe, and the installed binary is verified before update is re-run. All 45 focused tests and CI/security checks pass.
This was referenced Jul 28, 2026
This was referenced Aug 5, 2026
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
4 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.
Status: Ready for review.
What was wrong
Instruction files are generated by the installed CLI. Run
openspec updateon a stale install and it reports:Nothing there is false — the files do match the installed CLI — but users read it as "I have the latest OpenSpec," then report the workflows newer releases ship as missing, because their old CLI could never write them. The fix (
npm install -g @fission-ai/openspec@latestfirst, thenopenspec update) was only discoverable by asking.What it does
openspec updateasks the npm registry for the published version. When yours is behind, it offers to fix it before generating anything:Yes runs
npm install -g …@latest, then re-runs the update with the new binary so the new workflows land in the same command. No prints the command and updates with the CLI you have. Ctrl-C stops the command.Success is verified, not assumed:
npm install -gexits 0 even when it installed nothing, so the new binary is asked its own--version. If the copy that answers is still the old one, it says so rather than claiming success.The offer is deliberately narrow — an interactive terminal (stdin and stdout), and only where
npm install -gis the right fix:…@latestcommand, printednpx/dlxcachenpx @fission-ai/openspec@latest updateWhy it can't break anyone
yes. Non-interactive never prompts — a prompt on a redirected stdout is a question nobody seesnode:httprather thanfetchso the timer can destroy the socket; aborting afetchmid-handshake leaves the CLI unable to exit for ~10snulland prints nothing. Redirects are followed, boundednpm_config_registry. No.npmrcis read: letting file contents choose where an outbound request goes is a flow worth avoiding, and a project file would travel with a cloned repoCI,NODE_ENV=test,OPENSPEC_NO_UPDATE_CHECK,DO_NOT_TRACK=1,OPENSPEC_TELEMETRY=0. The re-run carriesOPENSPEC_NO_UPDATE_CHECK=1, so a stalePATHcannot loopProof it works
test/core/version-check.test.ts— 45 tests. Highlights, each pinning a bug this PR went through:Accepttype npm answers 406 for — the original version of this check was a permanent no-op because of it, and every test mockedfetchso nothing caught it. The suite now drives the real client against a local server.pnpm/yarndirectory names, npx caches, project dependencies, source checkouts.readCliVersionreading the version line rather than the first version-shaped token in a banner.--forceforwarded,--before the path, exit code passed through, and the anti-loop env guard (added after mutation testing found it was the one unprotected behavior).Full suite:
Test Files 114 passed · Tests 3302 passed.pnpm lintandtsc --noEmitclean. Both interactive paths verified end to end under a pty with a stubbed npm.Notes
openspec update.initis untouched: it runs moments after an install, so the stale-template failure mode barely exists there.cross-spawnand@inquirer/promptsare already runtime deps.minor: this adds a prompt, an env var, an outbound request, and the ability to install software.openspec updatesection and install matrix in docs/cli.md, two env-var rows, an "Installing software" row in SECURITY.md, and the "Commands don't show up" entry in docs/troubleshooting.md — the symptom this PR exists to fix.🤖 Generated with Claude Code