fix(test-harness): run the built CLI, not a packaged binary - #48
Merged
Conversation
Option 3 from #35. The harness spawned packages/cli/binaries/spectral — the standalone binary produced by @yao-pkg/pkg — so a per-commit test job was gated on producing a distributable artifact. When packaging broke, the whole harness was unrunnable, and it read as a test failure rather than a packaging one. That is exactly what happened on Windows, and the root cause is narrower than #35 first described. The two branches of build-binary disagree about the version format. The Unix branch normalizes to the major version: NODE_VERSION=$(echo "18.20.8" | awk -F "." '{ print $1 }') # node18-linux The Windows branch interpolates the input raw: --targets node${{ inputs.node-version }}-windows # node18.20.8-windows pkg publishes bases per major version, so it reported "No available node version satisfies 'v18.20.8'". Note it complained about the version, not the platform. Inherited, and long invisible: test-windows only ran on direct develop pushes upstream, with its harness step gated on a matrix key that did not exist. The harness now runs the built CLI. Two things this required, both verified locally rather than assumed: - process.execPath, not a bare `node`. Scenarios are spawned with a scrubbed environment containing no PATH — which is precisely why an absolute, self-contained binary worked here before. A bare `node` gives "/bin/sh: node: command not found" on all 66 suites. - A PowerShell call operator on Windows. A statement beginning with a quoted string is parsed as a string literal, not a command, so {bin} as a quoted interpreter path plus script path would be echoed instead of executed. SPECTRAL_BIN overrides the default, so the real binary can still be exercised at release time by pointing it at packages/cli/binaries/spectral. Decoupling the per-commit job from packaging does not mean the packaged artifact stops being testable. The build-binary step is removed from both harness actions, since yarn build has already produced dist in every job that runs them. Verified locally on macOS, Node 25.2.1: 65 of 66 harness suites pass. The 66th, help-no-document, fails because its own faketty.sh helper calls `script -qfec`, which is util-linux syntax that BSD script does not accept — it fails identically with a trivial command and is unrelated to this change. Windows correctness is NOT verified locally and cannot be. CI is the proof. Closes #35 Refs #3, #10, #11, #29 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kinlane
commented
Aug 3, 2026
kinlane
left a comment
kinlane
left a comment
Contributor
Author
There was a problem hiding this comment.
Not 100% sure, but going for it.
kinlane
added a commit
that referenced
this pull request
Aug 3, 2026
…ges (#49) * fix(test-harness): invalidate generated tests when the generator changes main is red. #48 passed locally and failed in CI, and this is why. The harness generates a .test.js per scenario, baking the {bin} command into the generated file at generation time. Generation is incremental: fileEntryCache tracks the scenario files and the generated outputs, and only regenerates scenarios whose inputs changed. Nothing tracked the generator itself. So #48 changed which command the tests run, no scenario file moved, and CI restored a previously generated tests/ directory together with the cache that declared it current. Result: the suites kept executing the old packaged-binary path, which no longer exists: /bin/sh: 1: /__w/.../packages/cli/binaries/spectral: not found Two layers, because either alone leaves a hole. generate-tests.ts now tracks __filename alongside the scenarios. If the generator changed, every scenario is regenerated rather than only those with changed inputs. The CI cache key now includes hashFiles('test-harness/**') — and, critically, so does the restore-keys PREFIX. The prefix was the actual hole: an exact-key miss fell back to test-harness-<os>-<node>-, which happily restored tests generated by an older generator. Verified by reproducing the failure locally before fixing it. The subtlety is that it only appears at steady state: after a run with an empty tests/ directory, the next run sees the generated files as newly tracked and regenerates anyway. It takes three runs to reach the state CI restores into. control, no fix, steady-state cache, generator changed -> dist/index.js (stale) with fix, same cache, generator changed -> dist/FIXED.js (correct) Harness still passes 65/66 locally. The 66th is help-no-document, whose own faketty.sh uses util-linux `script -qfec` syntax that BSD script rejects; it fails identically with a trivial command and is unrelated. Refs #35, #29, #33 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(test-harness): quote the bin command for PowerShell on Windows Second Windows attempt, and the first one was wrong for a reason worth recording. win.ts wraps every command in: powershell -Command "& { cd '<cwd>';<command>;echo LASTEXITCODE=$LASTEXITCODE }" The whole thing is already inside double quotes. So emitting {bin} as "C:\Program Files\nodejs\node.exe" "...\dist\index.js" terminated that wrapper, and PowerShell saw an unquoted path that split on its space: & : The term 'C:\Program' is not recognized as the name of a cmdlet... The call operator I added was firing correctly; the quoting around it was not surviving. This never came up before because the old {bin} was a single path with no spaces (binaries/spectral.exe), so it needed no quoting at all. Quoting is now platform-aware: PowerShell single quotes are literal and pass through the wrapper intact, while POSIX shells keep double quotes. win.ts recognises either quote character when deciding to prefix the call operator. Also fixes the prettier error the lint job caught on the previous commit. Local state unchanged: 65/66 on macOS, the 66th being help-no-document's BSD `script` incompatibility. Windows remains unverified locally and cannot be verified locally. CI is the only proof, and this is the second attempt at it — stated plainly rather than dressed up as confidence. Refs #35, #29 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Closes #35 — option 3, per your decision. Refs #3, #10, #11, #29.
You asked me to verify rather than assume. Here is what I actually ran.
First: the diagnosis in #35 was too generous to the toolchain
It is not that
pkglacks a Windows target. The two branches ofbuild-binarydisagree about the version format.pkgpublishes bases per major version, henceNo available node version satisfies 'v18.20.8'. Note it complained about the version, not the platform. Inherited and long invisible, becausetest-windowsonly ran on directdeveloppushes upstream with its harness step gated on a matrix key that did not exist.What I verified locally
Install and build both succeed on Node 25.2.1 — itself a data point for the open criterion on #3.
Then, pointing the harness at
dist/index.js:bin: 'node <dist>'/bin/sh: node: command not foundbin: '"<process.execPath>" "<dist>"'That first result is the finding worth keeping: scenarios are spawned with a scrubbed environment that has no
PATH— which is precisely why an absolute, self-contained binary worked here before. Anything relying onPATHsilently fails everywhere at once.The one remaining failure is
help-no-document, and it is not this change. Its ownfaketty.shcallsscript -qfec, util-linux syntax that BSDscriptrejects:It fails identically with a trivial command and no
{bin}involved. macOS-local artifact.What I changed
generate-tests.ts—binis nowprocess.execPathplus the built entry point, overridable viaSPECTRAL_BINso the real binary can still be exercised at release time. Decoupling the per-commit job from packaging should not make the packaged artifact untestable.win.ts— PowerShell parses a statement beginning with a quoted string as a string literal, so it would echo the path rather than run it. Added the call operator when{bin}is a quoted interpreter path.build-binarystep is removed.yarn buildalready runs in every job that reaches the harness.Lint caught a prettier error in my first draft before merge, which is #33 and #37 doing their job — the first time today that happened in the right order.
What I have not verified, and cannot
Windows. The PowerShell call-operator fix is reasoned, not tested — I have no Windows machine. CI is the only proof, and given the day's record I would rather say that plainly than imply confidence I do not have. If
test-windowsfails on this PR, the fix is wrong and I will say so.This is also why the PR matters beyond #35:
test-windowscurrently reddens every pull request, which blocks turning on required status checks in #10.