fix(scripts): make the alias check fail closed, and correct three stale docs (#208)

The alias check was a negative match on an allowlist of English error strings, which returned True for empty output, for $null, and for "exit status 1: Access is denied." — so an alias switch producing nothing, or failing on the symlink permission error this file's own header warns about, was reported as success while the old version kept running. That is the bug #198 was filed about, narrowed rather than removed, and it also broke whenever nvm reworded an error. It is now a positive match on "Now using node v<what is actually running>". The floor check moves ahead of the switch and reads the constant rather than the result. Where it sat, $major was always whatever NODE_VERSION says, so it validated the switch it had just made instead of the pin it exists to guard, and could never fire. Use-NodeLatest is now Use-PinnedNode. In a change whose whole subject is that "latest" means something people do not expect, the name was an avoidable trap. The restore default moves beside NODE_VERSION. It deliberately is not a param default: a param block runs before the dot-source, so $script:DEFAULT_NODE_VERSION is still $null there and the restore would have quietly restored nothing — leaving the machine on the pinned version, which is the exact failure the restore exists to prevent. It is resolved after the dot-source instead, and an explicit -DefaultNodeVersion still wins. Three documents described behaviour the code no longer has: README's "both scripts run nvm use latest", run-tests.ps1's .DESCRIPTION, and project-context.md's instruction to agents. All corrected, and project-context.md now also says not to run these scripts from an agent shell, which is how this machine once ended up with no Node at all. Part 4 of the issue is partly stale: backend/package.json already declares engines >=20.9.0. frontend now matches it. The larger question — whether local should be pinned to the Node 20 that CI and the production image actually run — is a decision rather than an oversight and is left open on the issue. Verified by parsing all three scripts with the PowerShell AST parser, which does not execute them. They are deliberately never run from an agent shell. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-02 09:33:09 -05:00
parent f52976dec9
commit 06933aec75
6 changed files with 77 additions and 22 deletions
@@ -270,7 +270,7 @@ Neither failure mentions the Node version as the cause, and the first one reads
 export PATH="/c/Users/tlamb/AppData/Local/nvm/v24.13.1:$PATH"
 ```

-Thom is fine with switching the active version for a test run — `nvm use latest`, then **`nvm use 18.16.1` when finished**, which is not optional since the app's own tooling expects 18.
+Thom is fine with the scripts switching the active version for a test run: they use the pinned `NODE_VERSION` (26.7.0) in `scripts/NodeVersion.ps1` and restore 18.16.1 when finished, including on failure. Do not run those scripts from an agent shell — they prompt for elevation and can leave the machine with no Node at all.

 Unit tests and `tsc` run fine on 18, so a green `npm test` says nothing about whether the other two suites can even start.

@@ -33,13 +33,17 @@ Commands below are shown for **PowerShell** (Windows). A bash equivalent is note
 .\scripts\start-local.ps1 -Fresh     # ...from an empty database
 .\scripts\start-local.ps1 -Stop      # stop everything

-.\scripts
un-tests.ps1 -Suite unit
-.\scripts
un-tests.ps1 -Suite integration
-.\scripts
un-tests.ps1 -Suite e2e
-.\scripts
un-tests.ps1 -Suite all
+.\scripts
+un-tests.ps1 -Suite unit
+.\scripts
+un-tests.ps1 -Suite integration
+.\scripts
+un-tests.ps1 -Suite e2e
+.\scripts
+un-tests.ps1 -Suite all
 ```

-Both scripts run `nvm use latest` first and verify the result is Node 20 or newer, then put the machine back to 18.16.1 when they finish — including when they fail partway, so an interrupted run does not leave the version switched. **`nvm use` rewrites a machine-global symlink, so this changes the Node version for every terminal on the machine while a script is running, not only the one you ran it in.** Both scripts say so as they do it.
+Both scripts switch to the pinned Node 26.7.0 (`NODE_VERSION` in `scripts/NodeVersion.ps1`) and verify that is what actually ends up running, then put the machine back to 18.16.1 when they finish — including when they fail partway, so an interrupted run does not leave the version switched. **`nvm use` rewrites a machine-global symlink, so this changes the Node version for every terminal on the machine while a script is running, not only the one you ran it in.** Both scripts say so as they do it.

 The Node 20 floor is not arbitrary: `node-pg-migrate` pulls in an `lru-cache` that calls `diagnostics_channel.tracingChannel()`, which does not exist before Node 19.9. On Node 18 migrations die inside minified library code with `(0 , U.tracingChannel) is not a function`, which says nothing about versions.

@@ -2,6 +2,9 @@
  "name": "redefined-designs-frontend",
  "version": "1.0.0",
  "private": true,
+  "engines": {
+    "node": ">=20.9.0"
+  },
  "scripts": {
    "dev": "vite",
    "build": "tsc && tsc -p tsconfig.test.json --noEmit && vite build",
@@ -69,8 +69,17 @@ function Use-Node {
            # nvm wanted is not installed" into a claim that the newest install
            # was too old, which sent the reader to `nvm install` holding a list
            # that already had newer versions on it.
+            # A POSITIVE match on what success looks like, not a negative
+            # one on an allowlist of English error strings. The negative form
+            # returned True for empty output, for $null, and for
+            # 'exit status 1: Access is denied.' — so an alias switch that
+            # produced nothing, or failed on the symlink permission error this
+            # file's own header warns about, was reported as success while the
+            # old version kept running. That is the bug #198 was filed about,
+            # narrowed rather than removed. It also broke whenever nvm reworded
+            # an error. See #208.
            $switched = if ($Version -in @('latest', 'lts', 'newest')) {
-                $output -notmatch 'activation error|not installed'
+                $output -match ('Now using node v' + [regex]::Escape($raw.TrimStart('v')))
            }
            else {
                $raw.TrimStart('v') -eq $Version.TrimStart('v')
@@ -100,9 +109,10 @@ nvm said:
 $output

 nvm-windows rewrites a symlink at $env:NVM_SYMLINK, and can report success
-without having rewritten it. Check the version is installed, and that this shell
-can write that link:
+without having rewritten it. If that version is not installed, install it; if it
+is, check this shell can write that link:

+    nvm install $Version
    nvm list
 "@
 }
@@ -114,6 +124,20 @@ can write that link:
 # the specification.
 $script:NODE_VERSION = '26.7.0'

+# What the machine is put back to afterwards. Beside NODE_VERSION rather than
+# duplicated as a parameter default in each script, because two copies of a
+# version drift and the half that drifts is the half nobody runs. Both scripts
+# still take -DefaultNodeVersion to override it.
+$script:DEFAULT_NODE_VERSION = '18.16.1'
+
+# Local runs are three major lines ahead of CI (Node 20 in every workflow) and of
+# the production image (node:20-bookworm-slim). That divergence is deliberate but
+# not free: post-20 syntax and node: APIs pass here and fail in the pipeline, and
+# Node 26 ships an npm that can touch the lockfile in ways CI's npm reads
+# differently. The `engines` field in both package.json files records the floor
+# machine-readably; nothing yet catches "too new for where this ships". See #208
+# part 4, which is an open decision rather than an oversight.
+
 <#
    Switches to the pinned version and insists it clears the floor.

@@ -130,20 +154,24 @@ $script:NODE_VERSION = '26.7.0'
    A version that is not installed is Use-Node's error to report, and it now
    reports nvm's own reason, so there is nothing to say about it here.
 #>
-function Use-NodeLatest {
+function Use-PinnedNode {
    param([scriptblock]$Step, [scriptblock]$Note)

-    Use-Node -Version $script:NODE_VERSION -Why 'this project needs Node 20 or newer' -Step $Step -Note $Note | Out-Null
-
-    $major = Get-NodeMajor
-    if ($major -lt 20) {
+    # Checked BEFORE the switch, against the constant rather than against what
+    # ends up running. After the switch $major is whatever NODE_VERSION says,
+    # so the old placement could never fire — it validated the switch it had
+    # just made instead of the pin it exists to guard. See #208.
+    $pinnedMajor = [int](($script:NODE_VERSION -replace '^v', '') -split '\.')[0]
+    if ($pinnedMajor -lt 20) {
        throw @"
-Node is v$major after switching to $($script:NODE_VERSION), which is below the 20 this project needs.
+NODE_VERSION in scripts/NodeVersion.ps1 is $($script:NODE_VERSION), which is below the 20 this project needs.

-NODE_VERSION in scripts/NodeVersion.ps1 is pinned to a version that is too old.
-node-pg-migrate, ts-jest and Playwright all need 20 or newer.
+node-pg-migrate, ts-jest and Playwright all need 20 or newer. Nothing was
+switched.
 "@
    }
+
+    Use-Node -Version $script:NODE_VERSION -Why 'this project needs Node 20 or newer' -Step $Step -Note $Note | Out-Null
 }

 <#
@@ -8,7 +8,7 @@
    database bring-up and the TEST_PGPORT handling are shared by more than one
    suite and would otherwise be copied around and drift apart.

-    Switches Node to the latest installed version for the run and puts the
+    Switches Node to the pinned version in scripts/NodeVersion.ps1 for the run and puts the
    machine default back afterwards, the same way start-local.ps1 does — and for
    the same reason, since ts-jest and Playwright are subject to the same Node
    20 floor as the migrations.
@@ -47,7 +47,12 @@ param(
    [int]$TestDbPort = 55432,
    [switch]$KeepTestDb,
    [string]$Filter,
-    [string]$DefaultNodeVersion = '18.16.1'
+    # Resolved after NodeVersion.ps1 is dot-sourced below, not here. A param
+    # block runs before anything else in the script, so $script:DEFAULT_NODE_VERSION
+    # is still $null at this point and using it as the default would silently
+    # restore nothing — leaving the machine on the pinned version, which is the
+    # exact failure the restore exists to prevent. See #208.
+    [string]$DefaultNodeVersion = ''
 )

 $ErrorActionPreference = 'Stop'
@@ -74,6 +79,11 @@ function Invoke-Checked {

 . (Join-Path $PSScriptRoot 'NodeVersion.ps1')

+# The one home for this value is NodeVersion.ps1, beside NODE_VERSION. It cannot
+# be a param default (see the note there), so it is filled in here instead, and
+# an explicit -DefaultNodeVersion still wins.
+if (-not $DefaultNodeVersion) { $DefaultNodeVersion = $script:DEFAULT_NODE_VERSION }
+
 # Bound once so the shared switcher reports in this script's output style.
 $NodeOut = @{ Step = ${function:Write-Step}; Note = ${function:Write-Note} }

@@ -160,7 +170,7 @@ running stack. Start it first:
    finally { Pop-Location }
 }

-Use-NodeLatest @NodeOut
+Use-PinnedNode @NodeOut

 try {
    switch ($Suite) {
@@ -39,7 +39,12 @@ param(
    # What -Stop puts the machine back to. nvm's default here is 18.16.1, which
    # is too old to run this project's tooling but is what everything else on
    # the machine expects.
-    [string]$DefaultNodeVersion = '18.16.1'
+    # Resolved after NodeVersion.ps1 is dot-sourced below, not here. A param
+    # block runs before anything else in the script, so $script:DEFAULT_NODE_VERSION
+    # is still $null at this point and using it as the default would silently
+    # restore nothing — leaving the machine on the pinned version, which is the
+    # exact failure the restore exists to prevent. See #208.
+    [string]$DefaultNodeVersion = ''
 )

 $ErrorActionPreference = 'Stop'
@@ -78,6 +83,11 @@ function Assert-Docker {

 . (Join-Path $PSScriptRoot 'NodeVersion.ps1')

+# The one home for this value is NodeVersion.ps1, beside NODE_VERSION. It cannot
+# be a param default (see the note there), so it is filled in here instead, and
+# an explicit -DefaultNodeVersion still wins.
+if (-not $DefaultNodeVersion) { $DefaultNodeVersion = $script:DEFAULT_NODE_VERSION }
+
 # Bound once so the shared switcher reports through this script's own output
 # style rather than printing in a voice of its own.
 $NodeOut = @{ Step = ${function:Write-Step}; Note = ${function:Write-Note} }
@@ -270,7 +280,7 @@ if ($Stop) {
    return
 }

-Use-NodeLatest @NodeOut
+Use-PinnedNode @NodeOut

 # Anything after the switch reverts on the way out of a failure. Without this a
 # run that dies in migrations leaves the machine on the new version with nothing