bermudalamb/redefined-designs
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects 3 Releases Wiki Activity

fix(scripts): the Node pin left three docs stale, and the alias check still fails open #208

New Issue
Closed
opened 2026-08-28 11:54:46 -05:00 by bermudalamb · 1 comment
bermudalamb commented 2026-08-28 11:54:46 -05:00
Owner
Copy Link
Copy Source

Follow-ups from the code review of #198 (95325c7). The diagnosis and the exact-match verification in that change are right; these are the loose ends.

1. The alias check returns success on empty output and on failures it does not recognise

scripts/NodeVersion.ps1 verifies an alias switch with $output -notmatch 'activation error|not installed'. Verified in PowerShell 7.6.3:

  • '' -notmatch '...' → True
  • $null -notmatch '...' → True
  • 'exit status 1: Access is denied.' -notmatch '...' → True

So an alias switch producing no output, or failing on a symlink permission error — a failure mode the file's own docstring calls out — is reported as success while the old version keeps running. That is the exact shape of the bug #198 was filed about, narrowed rather than removed. Negative matching on an allowlist of English error strings is also brittle across nvm-windows releases.

Fix: invert to a positive match on what success actually looks like. nvm prints Now using node v26.7.0 (64-bit), and $raw is already in hand:

$output -match ('Now using node v' + [regex]::Escape($raw.TrimStart('v')))

Fails closed, and survives nvm rewording its errors.

The branch is currently unreachable — after the pin, both Use-NodeLatest and Restore-Node pass concrete versions — which is why this is not urgent. But the stated reason for keeping it is that the next person reaching for an alias gets a truthful failure, and as written they would get a confidently wrong one instead. Either fix it or delete it; leaving it as-is is the one option that is not defensible.

2. Three documents describe behaviour the code no longer has

  • README.md:42 — "Both scripts run nvm use latest first and verify the result is Node 20 or newer". False on both halves, and it is the same claim about latest that caused #198.
  • scripts/run-tests.ps1:11 — .DESCRIPTION says "Switches Node to the latest installed version for the run." Wrong before #198 and wrong differently now.
  • .claude/project-context.md:273 — still tells agents "Thom is fine with switching the active version for a test run — nvm use latest".

3. The failure the pin makes most likely no longer says what to run

For a machine without 26.7.0, Use-Node now throws truthfully and quotes nvm, which is a real improvement. But the diff removed the only nvm install <version> guidance in the repo, and the pin makes "you do not have this exact version" the default experience for anyone but the author. The message says to check rather than what to run. Adding nvm install $Version to the throw closes it — valid for concrete versions and aliases alike.

4. Local Node is three major lines ahead of CI and production, undocumented

Where Node
Local (NODE_VERSION) 26.7.0
.gitea/workflows/{lint,backend-integration,sonarqube}.yml 20
Dockerfile (prod image) node:20-bookworm-slim
engines field, .nvmrc neither exists

Node 26 ships a much newer npm than Node 20's, so local installs can touch the lockfile in ways CI's npm reads differently; and any post-20 syntax or node: API passes locally and fails in the pipeline or the production image, with a stack trace that says nothing about versions — the class of failure NodeVersion.ps1's own header docstring exists to warn about. The floor check catches "too old"; nothing catches "too new for where this ships".

Not a challenge to the pin — it is that the pin's value ("two machines run the same Node") is half-realised while the machines that matter most run something else. Options: pin to the line actually shipped on; or keep 26.7.0 and document the divergence at the constant, following the convention sonar-project.properties:24 already sets for hand-coupled constants; or at minimum add "engines": { "node": ">=20" } so the requirement exists somewhere machine-readable.

5. Smaller

  • Use-NodeLatest is now misnamed — it switches to a pin, not to "latest". In a change whose subject is that "latest" means something people do not expect, the name is an avoidable trap. Two call sites.
  • The floor check sits after the switch, where $major is always 26 and it cannot fire. As a guard against pinning NODE_VERSION too low it should validate the constant before paying for a machine-global symlink rewrite.
  • $DefaultNodeVersion (18.16.1) is duplicated in start-local.ps1:42 and run-tests.ps1:50; it could sit beside NODE_VERSION with the parameter as an override.

Found by

Code review of 95325c7 (#198).

Follow-ups from the code review of #198 (`95325c7`). The diagnosis and the exact-match verification in that change are right; these are the loose ends. ## 1. The alias check returns success on empty output and on failures it does not recognise `scripts/NodeVersion.ps1` verifies an alias switch with `$output -notmatch 'activation error|not installed'`. Verified in PowerShell 7.6.3: - `'' -notmatch '...'` → `True` - `$null -notmatch '...'` → `True` - `'exit status 1: Access is denied.' -notmatch '...'` → `True` So an alias switch producing no output, or failing on a symlink permission error — a failure mode the file's own docstring calls out — is reported as **success** while the old version keeps running. That is the exact shape of the bug #198 was filed about, narrowed rather than removed. Negative matching on an allowlist of English error strings is also brittle across nvm-windows releases. Fix: invert to a positive match on what success actually looks like. nvm prints `Now using node v26.7.0 (64-bit)`, and `$raw` is already in hand: ```powershell $output -match ('Now using node v' + [regex]::Escape($raw.TrimStart('v'))) ``` Fails closed, and survives nvm rewording its errors. The branch is currently unreachable — after the pin, both `Use-NodeLatest` and `Restore-Node` pass concrete versions — which is why this is not urgent. But the stated reason for keeping it is that the next person reaching for an alias gets a truthful failure, and as written they would get a confidently wrong one instead. Either fix it or delete it; leaving it as-is is the one option that is not defensible. ## 2. Three documents describe behaviour the code no longer has - `README.md:42` — "Both scripts run `nvm use latest` first and verify the result is Node 20 or newer". False on both halves, and it is the same claim about `latest` that caused #198. - `scripts/run-tests.ps1:11` — `.DESCRIPTION` says "Switches Node to the latest installed version for the run." Wrong before #198 and wrong differently now. - `.claude/project-context.md:273` — still tells agents "Thom is fine with switching the active version for a test run — `nvm use latest`". ## 3. The failure the pin makes most likely no longer says what to run For a machine without 26.7.0, `Use-Node` now throws truthfully and quotes nvm, which is a real improvement. But the diff removed the only `nvm install <version>` guidance in the repo, and the pin makes "you do not have this exact version" the default experience for anyone but the author. The message says to *check* rather than what to run. Adding `nvm install $Version` to the throw closes it — valid for concrete versions and aliases alike. ## 4. Local Node is three major lines ahead of CI and production, undocumented | Where | Node | |---|---| | Local (`NODE_VERSION`) | **26.7.0** | | `.gitea/workflows/{lint,backend-integration,sonarqube}.yml` | `20` | | `Dockerfile` (prod image) | `node:20-bookworm-slim` | | `engines` field, `.nvmrc` | neither exists | Node 26 ships a much newer npm than Node 20's, so local installs can touch the lockfile in ways CI's npm reads differently; and any post-20 syntax or `node:` API passes locally and fails in the pipeline or the production image, with a stack trace that says nothing about versions — the class of failure `NodeVersion.ps1`'s own header docstring exists to warn about. The floor check catches "too old"; nothing catches "too new for where this ships". Not a challenge to the pin — it is that the pin's value ("two machines run the same Node") is half-realised while the machines that matter most run something else. Options: pin to the line actually shipped on; or keep 26.7.0 and document the divergence at the constant, following the convention `sonar-project.properties:24` already sets for hand-coupled constants; or at minimum add `"engines": { "node": ">=20" }` so the requirement exists somewhere machine-readable. ## 5. Smaller - `Use-NodeLatest` is now misnamed — it switches to a pin, not to "latest". In a change whose subject is that "latest" means something people do not expect, the name is an avoidable trap. Two call sites. - The floor check sits *after* the switch, where `$major` is always 26 and it cannot fire. As a guard against pinning `NODE_VERSION` too low it should validate the constant before paying for a machine-global symlink rewrite. - `$DefaultNodeVersion` (`18.16.1`) is duplicated in `start-local.ps1:42` and `run-tests.ps1:50`; it could sit beside `NODE_VERSION` with the parameter as an override. ## Found by Code review of `95325c7` (#198).
bermudalamb commented 2026-09-04 19:48:42 -05:00
Author
Owner
Copy Link
Copy Source

All five items are resolved. Four were already done and the issue simply stayed open; the fifth needed a decision rather than a patch, and it has now been made.

1. The alias check returning success on empty output. Done. Use-Node matches positively on what success looks like rather than negatively on an allowlist of English error strings, so it fails closed and survives nvm rewording its messages.

2. Three stale documents. Done in 06933ae — README.md, run-tests.ps1's .DESCRIPTION and .claude/project-context.md no longer describe nvm use latest behaviour the code does not have.

3. The failure that no longer said what to run. Done. The throw now quotes nvm install $Version and nvm list alongside nvm's own reason, so the most likely failure — a machine without the pinned version — ends with the command that fixes it.

5. The smaller three. All done. Use-NodeLatest is gone, renamed to Use-PinnedNode since it switches to a pin and not to "latest". DEFAULT_NODE_VERSION sits beside NODE_VERSION in NodeVersion.ps1 rather than being duplicated as a parameter default in both scripts. And the floor check now runs before the switch, against the constant — its comment records that the old placement "validated the switch it had just made instead of the pin it exists to guard", which is why it could never fire.

4. The version divergence. Decided: the pin stays at 26.7.0.

Local runs stay three major lines ahead of CI and the production image, and CI on Node 20 is the backstop. engines: >=20.9.0 in both package.json files records the floor machine-readably, and .nvmrc is not added.

The cost is now written at the constant rather than left implied: passing locally does not mean it ships, because a post-20 syntax or node: API is caught after a push rather than before one. That is acceptable because it is known — the failure this file's own docstring warns about is the one nobody realises they are exposed to. The comment previously described this as "an open decision rather than an oversight", which would have read to the next person as something still to settle; it now says what was settled and what it costs.

Branch fix/208-node-pin-decision, commit 936dcb6.

All five items are resolved. Four were already done and the issue simply stayed open; the fifth needed a decision rather than a patch, and it has now been made. **1. The alias check returning success on empty output.** Done. `Use-Node` matches positively on what success looks like rather than negatively on an allowlist of English error strings, so it fails closed and survives nvm rewording its messages. **2. Three stale documents.** Done in `06933ae` — `README.md`, `run-tests.ps1`'s `.DESCRIPTION` and `.claude/project-context.md` no longer describe `nvm use latest` behaviour the code does not have. **3. The failure that no longer said what to run.** Done. The throw now quotes `nvm install $Version` and `nvm list` alongside nvm's own reason, so the most likely failure — a machine without the pinned version — ends with the command that fixes it. **5. The smaller three.** All done. `Use-NodeLatest` is gone, renamed to `Use-PinnedNode` since it switches to a pin and not to "latest". `DEFAULT_NODE_VERSION` sits beside `NODE_VERSION` in `NodeVersion.ps1` rather than being duplicated as a parameter default in both scripts. And the floor check now runs **before** the switch, against the constant — its comment records that the old placement "validated the switch it had just made instead of the pin it exists to guard", which is why it could never fire. **4. The version divergence. Decided: the pin stays at 26.7.0.** Local runs stay three major lines ahead of CI and the production image, and CI on Node 20 is the backstop. `engines: >=20.9.0` in both `package.json` files records the floor machine-readably, and `.nvmrc` is not added. The cost is now written at the constant rather than left implied: passing locally does not mean it ships, because a post-20 syntax or `node:` API is caught after a push rather than before one. That is acceptable because it is known — the failure this file's own docstring warns about is the one nobody realises they are exposed to. The comment previously described this as "an open decision rather than an oversight", which would have read to the next person as something still to settle; it now says what was settled and what it costs. Branch `fix/208-node-pin-decision`, commit `936dcb6`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
calamb (Chelsea Lamb) bermudalamb (Thom Lamb)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#208