feat(cli): tell the user when their sim is out of date - #7420
Merged
Conversation
`sim tools execute` shipped in 2.1.5. Someone on 2.1.2 looking for it saw a help listing without it and concluded the CLI could not do it - a missing subcommand is indistinguishable from a feature that was never built, and nothing in the CLI could tell them otherwise. It had no update check, no version negotiation, and no way to learn what "current" is. Once a day, at an interactive terminal, the root `preAction` hook asks `registry.npmjs.org` for the dist-tags of the channel it was installed from and prints one line on stderr when a newer version exists. The request carries the CLI version and nothing else - no key, no workspace, no command - and `SIM_NO_UPDATE_CHECK=1` turns it off. Everything about it fails silently, and it says nothing when stderr is not a terminal, in CI, under `npx`, from a checkout, or to a prerelease install. The last two are not politeness: the repo manifest trails npm permanently by design because the publish workflow bumps the version in-job under `permissions: contents: read` and never commits it back, so without the checkout guard every engineer here would be told daily to upgrade to a version their own tree already contains; and `staging` publishes on every push, so advising a prerelease user would be stale within the hour. Comparison is scoped to one channel, which is what makes "upgrade" to an older stable version structurally impossible rather than merely guarded against. The comparator implements semver precedence including the numeric prerelease rule - `preview.9` precedes `preview.44`, which a string comparison gets backwards. The `preAction` hook is deliberate over a teardown in the entrypoint: commander answers `--help` and `--version` during parsing, so the two latency-sensitive invocations are excluded by construction, and some commands call `process.exit` directly where a `finally` would never run. Timeout is a hard 1s rather than `SIM_TIMEOUT_SECONDS`, which defaults to an hour and governs work the user actually asked for. The check is stamped whether or not it succeeds, so a blackholed registry costs one second a day instead of one per command.
Mutation testing found three tests that could not fail: deleting the `preAction` hook, switching the default writer to stdout, and flipping `comparePrerelease`'s empty-list arm all left the suite green. The stdout one was vacuous because the test helper always injected a writer, so the single safety property this feature claims - never touch stdout - was unprotected. The hook now has a positive test. It asserts registration rather than a resulting request, because the check suppresses itself when running from a checkout, and inside the suite `import.meta.url` IS a checkout: the behavioural path is unreachable there by construction. It is covered directly in check.test.ts and walked against the real registry from a staged global install. Security review: the response body is now read under a 64KB budget instead of buffering whatever a mirror sends, the request refuses to follow redirects, and the registry's answer is parsed before it is persisted, so nothing unvalidated reaches the disk. The reduced User-Agent was a comment; it is now an assertion, so a future "DRY up the user agent" refactor cannot silently start handing npm the user's node version, platform and arch. A configured mirror's own path and query are preserved. `new URL(relative, base)` discards both, so a token-authenticated Artifactory or Nexus base was being rewritten into a request the mirror answers with a 404. Also: one normalisation for every module-path decision (separators AND case, so a Windows or case-insensitive checkout is not read as a global install by one guard and a checkout by the other), the package name is named once rather than spelled in two unrelated places, and `delete process.env.SIM_CONFIG_DIR` in teardown - assigning `undefined` stores the literal string and leaves later tests pointed at a relative `./undefined` directory. Tests: 843 -> 861. Ten mutations applied to verify the new assertions actually fail when the thing they guard is broken; all ten killed. Declined, with reasons: the ~10s lingering-socket exit delay could not be reproduced through the CLI (measured 1.11-1.38s across three runs on node v23.11.0, including a command that only sets exitCode), so no node:https rewrite. `announced` plus `resetUpdateCheck` stays - it is the same shape as the existing resetEnvironmentNotices and resetRenameWarnings seams. The channel type stays rather than collapsing to a boolean, because it is what a decision to notify prerelease users would extend; its docs now say what the code does instead of describing a comparison it never performs.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThe PR adds a cached update notice for eligible globally installed Sim CLI invocations.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/sim-cli/src/update/check.ts | Implements update-check eligibility, cache handling, registry probing, stable-version comparison, and package-manager-specific notices without an accepted follow-up finding. |
| packages/sim-cli/src/program.ts | Registers the update check as a root pre-action hook while leaving parser-handled help and version paths unaffected. |
| packages/sim-cli/src/update/check.process.test.ts | Exercises timeout, response-size, proxy, credential-transport, and redirect behavior across child-process boundaries. |
| packages/sim-cli/src/update/check.test.ts | Covers notice eligibility, cache behavior, registry validation, version comparison, and upgrade-command selection. |
| apps/docs/content/docs/cli/configuration.mdx | Documents update-check behavior and now accurately discloses configured mirror query credentials and intermediary visibility. |
| packages/sim-cli/README.md | Adds concise update-notice configuration, privacy, cache, and suppression guidance. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CLI command] --> B{Eligible invocation?}
B -- No --> C[Run silently]
B -- Yes --> D{Daily cache fresh?}
D -- Yes --> C
D -- No --> E[Write check timestamp]
E --> F[Spawn bounded registry probe]
F --> G{Newer stable release?}
G -- No or probe failure --> C
G -- Yes --> H[Print update notice to stderr]
H --> I[Run requested command]
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/sta..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…rsing Review round 1: five findings, all valid. The privacy statement was too absolute. The request carries no Sim API key, but `npm_config_registry` can point at a private mirror, and a token embedded in that URL is sent with the request - it has to be, or the mirror rejects it. Both docs now say which credentials are involved and where they go: your registry's, to the host you configured, never Sim's. `parseVersion` accepted zero-padded prerelease identifiers. Semver forbids them, and accepting `2.1.3-preview.09` was worse than cosmetic: `09` failed the numeric test and fell through to being an alphanumeric identifier, and alphanumerics outrank every number, so `preview.010` sorted ABOVE `preview.2`. The file's own doc comment already claimed leading zeroes were rejected "the way the specification rejects them" - true of the release triple, not of the prerelease. Now true of both. The `--version`/`--help` test did not hold the guarantee it advertised. It watched for a request and a cache file, but neither ever appears from inside a checkout no matter what runs, because the check suppresses itself there - so it would have passed even if the hook fired, which is the exact regression it claims to prevent. It now swaps a sentinel into commander's registered preAction hooks and asserts the sentinel does not fire while parsing those two, then asserts it DOES fire for a real action command, so the negative assertion means something. No module mocking, which this package bans. The troubleshooting page hardcoded `npm install -g`, which installs a second copy under a different package manager rather than replacing the executable on PATH. It now shows all three, and says the notice already prints the one matching your install - which the notifier has always done. Tests: 861 -> 863. Both new guards mutation-checked: dropping the leading-zero rejection and deleting the hook each fail the suite.
Contributor
Author
Contributor
Author
|
@cubic review |
Contributor
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…does Review round 2. Three findings, all valid. The previous commit's message claimed it had replaced `process.env.SIM_CONFIG_DIR = undefined` with `delete` in the test teardowns. It had not: it added a comment explaining why the assignment is wrong and left the assignment in place, so the teardown still stored the literal string "undefined". Both files now actually delete it. The same pattern exists in profile.test.ts and configure.test.ts, which predate this branch and are left alone. Two documentation claims were stronger than the implementation. "At most once a day" is only true with a writable `~/.sim`. The pace lives in a timestamp file, so a read-only home in a container - or a `~/.sim` left root-owned by an earlier sudo install - means the pace cannot be remembered and the check runs per command. That was already noted in a code comment; it is now in the docs where users read it, along with the fact that it stays bounded by the same one-second timeout. "The tag it was installed from" described behaviour that does not exist. The check only ever queries `latest`, because prerelease installs return before any request. Both docs now say that plainly instead of implying the CLI can ask about the staging or dev channel.
Contributor
Author
Contributor
Author
|
@cubic review |
Contributor
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Review round 3. The cache is derived from `configDir()`, so it moves with `SIM_CONFIG_DIR` like the config and credentials files do - but the docs named only the `~/.sim` default, sending anyone with a relocated config dir to a file that is not there.
Contributor
Author
Contributor
Author
|
@cubic review |
Contributor
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
Collaborator
Collaborator
|
@cubic-dev-ai review this PR |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Collaborator
Collaborator
|
@cubic-dev-ai review this PR |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 9 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Fix all with cubic | Re-trigger cubic
Collaborator
Collaborator
|
@cubic-dev-ai review this PR |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
No issues found across 9 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Re-trigger cubic
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
simCLIs.npm exec/npx, and project-local invocations silent.Simplification
Type of Change
Testing
Checklist