Commit 34e522a
feat(cli): tell the user when their sim is out of date (#7420)
* feat(cli): tell the user when their sim is out of date
`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.
* fix(cli): close the update-notifier findings from pre-landing review
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.
* fix(cli): correct the update-notifier privacy claim and prerelease parsing
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.
* fix(cli): make the update-notifier docs match what the code actually 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.
* docs(cli): name the update cache path for relocated config dirs
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.
* fix(cli): harden and simplify update checks
* test(cli): isolate update checks from CI markers
* fix(cli): tighten update check eligibility
---------
Co-authored-by: Waleed Latif <walif6@gmail.com>1 parent c261ac1 commit 34e522a
9 files changed
Lines changed: 1586 additions & 7 deletions
File tree
- apps/docs/content/docs/cli
- packages/sim-cli
- src
- config
- update
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
114 | 114 | | |
115 | 115 | | |
116 | 116 | | |
117 | | - | |
| 117 | + | |
118 | 118 | | |
119 | 119 | | |
120 | 120 | | |
121 | 121 | | |
| 122 | + | |
122 | 123 | | |
123 | | - | |
124 | | - | |
125 | | - | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
126 | 191 | | |
127 | 192 | | |
128 | 193 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
| 6 | + | |
| 7 | + | |
6 | 8 | | |
7 | 9 | | |
8 | 10 | | |
| |||
85 | 87 | | |
86 | 88 | | |
87 | 89 | | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
88 | 134 | | |
89 | 135 | | |
90 | 136 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
256 | 256 | | |
257 | 257 | | |
258 | 258 | | |
259 | | - | |
| 259 | + | |
260 | 260 | | |
261 | 261 | | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
262 | 279 | | |
263 | 280 | | |
264 | 281 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | 3 | | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
4 | 7 | | |
5 | 8 | | |
6 | 9 | | |
7 | 10 | | |
8 | 11 | | |
9 | 12 | | |
10 | | - | |
11 | | - | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
12 | 17 | | |
13 | 18 | | |
14 | 19 | | |
| |||
159 | 164 | | |
160 | 165 | | |
161 | 166 | | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
13 | 14 | | |
14 | 15 | | |
15 | 16 | | |
| |||
151 | 152 | | |
152 | 153 | | |
153 | 154 | | |
| 155 | + | |
| 156 | + | |
154 | 157 | | |
155 | 158 | | |
156 | 159 | | |
| |||
0 commit comments