diff --git a/.changeset/merry-cranes-frolic.md b/.changeset/merry-cranes-frolic.md new file mode 100644 index 000000000..3c0c4b0fa --- /dev/null +++ b/.changeset/merry-cranes-frolic.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4643 +--- +**Windows path-confinement is now actually verified** — the external-descriptor write-confinement check resolved paths through the ambient `path` module, so its Windows semantics (drive letters, UNC paths, separator handling) were only ever exercised when the suite happened to run on Windows, and never with Windows-specific inputs. A Windows-only escape was therefore unverified on every platform. The check now accepts an optional path implementation, and drive-letter, UNC, traversal and prefix-boundary escapes are covered deterministically. (#4641) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 119564105..0bf2bef1c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -120,7 +120,6 @@ jobs: full_matrix: ${{ steps.scope.outputs.full_matrix }} product_changed: ${{ steps.scope.outputs.product_changed }} targeted_tests: ${{ steps.scope.outputs.targeted_tests }} - windows_tests: ${{ steps.scope.outputs.windows_tests }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: @@ -143,7 +142,6 @@ jobs: echo "product_changed=true" echo "full_matrix=true" echo "targeted_tests=" - echo "windows_tests=" } >> "$GITHUB_OUTPUT" { echo "## Test scope" @@ -164,7 +162,6 @@ jobs: console.log(`- code_changed: \`${result.code_changed}\``); console.log(`- full_matrix: \`${result.full_matrix}\``); console.log(`- targeted_tests: \`${result.targeted_tests.length}\``); - console.log(`- windows_tests: \`${result.windows_tests.length}\``); if (result.reasons.length > 0) { console.log(''); console.log('### Reasons'); @@ -262,25 +259,21 @@ jobs: # that failure mode, it is re-basing the SAME headroom policy on a number # that had gone stale. # - # The `scope: windows` lane is sharded three ways for the same reason - # (see #3057): on PR #3094 it reached 15m05s against a 15-minute cap and - # was CANCELLED, four shas in a row — a change to tests/helpers.cjs scoped - # in the install-heavy suites and pushed the single Windows lane over the - # top. Per #869, a timeout bump only moves that cliff; sharding removes - # it. That lane does not run any aux suite on its own shard 1 (the - # aux-suite `if:` conditions below are gated on `scope == 'full'` - # specifically), so it needs no reserve and is unaffected by this cap - # change beyond sharing the same job-level `timeout-minutes`. + # #4641: this job's `scope: windows` lane (three shards, #3057) is deleted + # — test-conformance is now the sole Windows selector. See + # docs/adr/4641-windows-selector-consolidation.md. # tests/ci-test-job-timeout-budget.test.cjs holds every lane here to a # headroom factor over its own measured cost. timeout-minutes: 32 env: GSD_PLUGIN_ROOT: .ci-gsd-plugin-root-disabled - # #2665: a live-config leak fails the run on Linux/macOS lanes. Windows - # stays report-only: the guard's first run found PRE-EXISTING USERPROFILE - # leaks there (~190 test sites sandbox HOME alone), a documented separate - # class — promote once that sweep lands (see live-config-guard.cjs SEVERITY). - GSD_STRICT_LIVE_CONFIG_GUARD: ${{ matrix.os != 'windows-latest' && '1' || '' }} + # #2665 / #4641: this job's matrix is ubuntu-only as of #4641 (the + # `scope: windows` rows moved to test-conformance, see the timeout + # comment above), so the live-config leak guard is strict unconditionally + # here. The Windows report-only carve-out (PRE-EXISTING USERPROFILE leaks, + # ~190 test sites sandbox HOME alone) now lives solely on jobs.test-conformance + # — promote it there once that sweep lands (see live-config-guard.cjs SEVERITY). + GSD_STRICT_LIVE_CONFIG_GUARD: '1' # #2854: pin the emitted gate's baseline to the SAME commit the tree was merged # with. "Rebase check" merges `pull_request.base.sha` (pinned by #2472 so all 12 # matrix jobs agree on one tree), but `resolveBase()` otherwise falls through to @@ -337,12 +330,10 @@ jobs: # below reserves that fixed cost out of shard 1's LPT unit-test share, so # shard 1 gets fewer unit-test files rather than more aux-suite runs. # - # #3057: the `scope: windows` lane is now sharded three ways too, the - # same fix applied to the same cliff (#869's stated durable follow-up). - # It hit exactly 15m05s and was CANCELLED on PR #3094, four shas - # straight, after a change to tests/helpers.cjs scoped in the - # install-heavy suites. Its shards use the same `--shard i/n` - # flag on scripts/run-tests.cjs, applied AFTER scope selection. + # #4641: the `scope: windows` lane (three shards, #3057) that used to + # be listed here is deleted — test-conformance is now the sole + # Windows selector. See + # docs/adr/4641-windows-selector-consolidation.md. - os: ubuntu-latest node-version: 24 scope: targeted @@ -358,18 +349,6 @@ jobs: node-version: 24 scope: full shard: 3/3 - - os: windows-latest - node-version: 24 - scope: windows - shard: 1/3 - - os: windows-latest - node-version: 24 - scope: windows - shard: 2/3 - - os: windows-latest - node-version: 24 - scope: windows - shard: 3/3 steps: # Windows lane on checkout v5.0.1 (drops includeIf; no auth flake, uses Node 24 natively). @@ -447,7 +426,6 @@ jobs: env: TEST_SCOPE: ${{ matrix.scope }} TARGETED_TESTS: ${{ needs.changes.outputs.targeted_tests }} - WINDOWS_TESTS: ${{ needs.changes.outputs.windows_tests }} run: node scripts/ci-prepare-test-scope.cjs - name: Run scoped tests @@ -476,7 +454,7 @@ jobs: # invocations identically — each recomputes the whole 3-way partition # independently and must agree on it (see the `sig` cross-job # fingerprint diagnostic further down in run-tests.cjs). The - # `scope: windows` lane runs no aux suite on its own shard 1, so this + # `scope: targeted` lane runs no aux suite on its own, so this # must stay empty there. tests/ci-full-lane-sharding.test.cjs pins both # halves of this contract. Reserve-value derivation: # .gsd/bug/fix-4070-shard1-aux-suite-budget/10-diagnosis.md. @@ -590,7 +568,6 @@ jobs: env: TEST_SCOPE: targeted TARGETED_TESTS: ${{ needs.changes.outputs.targeted_tests }} - WINDOWS_TESTS: ${{ needs.changes.outputs.windows_tests }} run: node scripts/ci-prepare-test-scope.cjs - name: Run scoped tests run: node scripts/run-tests.cjs --files-from .ci-selected-tests.txt diff --git a/docs/TESTING-SUITES.md b/docs/TESTING-SUITES.md index 054f0d7c2..987bd463d 100644 --- a/docs/TESTING-SUITES.md +++ b/docs/TESTING-SUITES.md @@ -466,9 +466,9 @@ only supported runtime. | Job | Lanes | Gated on | Purpose | |---|---|---|---| -| `test` | `ubuntu-latest` (1 targeted + 3-shard full) + `windows-latest` (3-shard, Windows/path/shell-scoped) | `product_changed == 'true'` | The default, always-scoped PR signal — the full `unit`/`integration`/`security` suites run once, sharded, on Linux; Windows runs the Windows-sensitive subset plus every changed test file | +| `test` | `ubuntu-latest` (1 targeted + 3-shard full) | `product_changed == 'true'` | The default, always-scoped PR signal — the full `unit`/`integration`/`security` suites run once, sharded, on Linux. **Linux only**: its three `scope: windows` shards were deleted in #4641 (ADR-4641), which found them a second, redundant Windows selector alongside `test-conformance` | | `test-inert` | `ubuntu-latest` | `code_changed == 'true' && product_changed != 'true'` | A lightweight lane for PRs that touch only administrative/policy workflow files (code changed, but nothing that needs the real matrix) | -| `test-conformance` | `windows-latest` (3-shard) + `macos-latest` (unsharded) | `code_changed == 'true' && full_matrix == 'true'` | Runs only the **platform-conformance-tier** file list (`scripts/lib/platform-conformance-tier.generated.cjs`, epic #4589 Phase 2/#4591) on real Windows/macOS — the sole gating signal for real-OS coverage. Retired the parallel legacy full-suite matrix in #4603. | +| `test-conformance` | `windows-latest` (3-shard) + `macos-latest` (unsharded) | `code_changed == 'true' && full_matrix == 'true'` | Runs only the **platform-conformance-tier** file list (`scripts/lib/platform-conformance-tier.generated.cjs`, epic #4589 Phase 2/#4591) on real Windows/macOS — since #4641 the **sole** Windows and macOS selector in CI, not merely the sole gating one. Retired the parallel legacy full-suite matrix in #4603; #4641 removed the second Windows selector in `test` and narrowed the tier from 548 to 266 of 932 eligible unit-suite files (58.8% → 28.5%, measured 2026-09-11; the absolute counts track `next`'s test count, the percentages are what the ceiling test binds on). | | `coverage-gate` | `ubuntu-latest` | `product_changed == 'true' && test.result == 'success'` | Merges every `test` shard's coverage dumps and evaluates the threshold once (sharding moved this out of the `test` job itself — #2952) | | `qa-loop-walk` | `ubuntu-latest` | `product_changed == 'true'` | The QA smell-ratchet scenario walk (see "The QA smell ratchet" below) | | `required-tests` | `ubuntu-latest` | `always()` | Aggregates every job above into the one branch-protection-required check | diff --git a/docs/adr/4593-macos-conformance-tier-architecture.md b/docs/adr/4593-macos-conformance-tier-architecture.md index 359cd3742..a07a3a8e8 100644 --- a/docs/adr/4593-macos-conformance-tier-architecture.md +++ b/docs/adr/4593-macos-conformance-tier-architecture.md @@ -7,6 +7,14 @@ applies ADR-1703's evidence-first, static-classifier discipline to a second, macOS-specific surface, rather than reusing ADR-1703's Windows-oriented signal set unmodified. +> **Amendment (2026-09-11, [ADR-4641](./4641-windows-selector-consolidation.md)):** the general / +> Windows-oriented tier this ADR compares against was **546 files** when this ADR was written, and +> every figure below is accurate as of that date. ADR-4641 has since removed two over-broad +> detectors from `CATEGORIES`, taking it to **254**. Nothing in this ADR's decision changes: +> `MACOS_CATEGORIES` is a separate array, `chmod-mode-bit` and `symlink-keyword` keep the +> definitions and rationale recorded here, and the macOS tier remains 196 files. The historical +> figures below are deliberately left unedited. + ## Context `test-conformance`'s `macos-latest` CI leg (#4591, epic #4589 Phase 2) runs only the diff --git a/docs/adr/4641-windows-selector-consolidation.md b/docs/adr/4641-windows-selector-consolidation.md new file mode 100644 index 000000000..953a635a6 --- /dev/null +++ b/docs/adr/4641-windows-selector-consolidation.md @@ -0,0 +1,358 @@ +# ADR-4641: One Windows test selector, and a proportional ceiling on the conformance tier + +- **Status:** Accepted +- **Date:** 2026-09-11 +- **Issue:** [#4641](https://github.com/open-gsd/gsd-core/issues/4641) +- **Twin of:** [ADR-4593](./4593-macos-conformance-tier-architecture.md) (macOS conformance tier) — that ADR + applied evidence-first sizing to the macOS signal set; this one applies the same discipline to the + Windows signal set and to the *number of selectors*, which ADR-4593 did not cover. +- **Closes a gap in:** epic [#4589](https://github.com/open-gsd/gsd-core/issues/4589), whose goal — + "the OS-agnostic majority of the suite runs once, on Linux, while a small and explicitly-scoped + platform-conformance tier covers genuinely OS-specific behavior" — was not achieved by its five + merged phases. + +## Context + +Epic #4589 closed 2026-09-10 with every acceptance box checked. Measured on PR #4640 +([run 34618834118](https://github.com/open-gsd/gsd-core/actions/runs/34618834118)), a routine +`tests/**`-touching PR, two things were true that the goal text forbids. + +### Two independent Windows selectors + +| selector | gate | what it runs | +|---|---|---| +| `test` job, three `scope: windows` rows | `product_changed` | `windows_tests` = **every** changed `tests/*.test.cjs`, unconditionally | +| `test-conformance`, three windows shards | `code_changed && full_matrix` | the generated conformance tier | + +The epic replaced `test-full` with `test-conformance` and never touched the first selector, which +predates it (#494, sharded in #3057). On PR #4640, **5 of 7 changed test files ran on a real Windows +runner twice.** Non-Linux job count was **7**, not the 4 the epic's closeout reported — that figure +compared `test-full` (6) against `test-conformance` (4) and omitted the three always-on `scope: +windows` shards from both sides. Counting every non-Linux job, the epic moved 9 → 7, not 6 → 4. + +### The tier was 59% of the suite + +`node scripts/gen-platform-conformance-tier.cjs` reported **546 of 930** eligible unit-suite files +(58.7%) when this was diagnosed. Measured on the tree this PR actually ships against (**932** eligible, +after #4253 and #4644 landed on `next` mid-flight) the same comparison is **548 → 257** by detector +removal alone. Absolute counts drift every time `next` gains a test file; the **percentages did not +move at all** across three rebases (58.8% → 28.5%), which is the whole reason the ceilings are +ratios. Measured per-category contribution, where `UNIQUE` is the count of files for which that +category is the **sole** signal — i.e. the marginal cost of keeping it: + +| category | total | UNIQUE | +|---|---:|---:| +| `process-seam-subprocess` | 335 | **118** | +| `hardcoded-path-vs-path-call` | 328 | **108** | +| `raw-child-process` | 96 | 19 | +| `symlink-keyword` | 86 | 6 | +| `win32-darwin-literal` | 77 | 1 | +| `process-platform` | 73 | 1 | +| `chmod-mode-bit` | 73 | 4 | +| `windows-env-var` | 69 | 6 | +| `windows-shell-token` | 23 | 4 | +| `os-platform` | 2 | 0 | + +Two categories carried 226 of the tier's sole-signal membership; the other eight carried 41 combined. + +- **`process-seam-subprocess`** matches `runNode(` / `runGit(` / `runHook(` / `runGsdTools(` / + `gitOrThrow(` — the repo's own `tests/helpers.cjs` entry points, used by nearly every CLI test. + For the overwhelming majority of them, going *through* the seam is the opposite of a platform + signal: `src/shell-command-projection.cts` takes `platform` as an injected parameter, and + `tests/shell-command-projection-dispatch.test.cjs` already exercises PowerShell/cmd.exe/PATHEXT + in-process on Linux by passing `platform: 'win32'` as data. That is epic #4589's own argument for + why the cutover was safe, applied against itself. **But see the Consequences section: this + detector was 99% noise wrapping a real signal — the ~9 tests that spawn a real shell via + `runHook`'s `interpreter` option — and that signal is preserved by a narrow replacement category + rather than lost with the blanket one.** +- **`hardcoded-path-vs-path-call`** requires a `path.join|resolve|…(` call *anywhere* in the file AND + a quoted `'/…'` literal *anywhere* in the file, with no proximity. In a Node test suite both are + universal. The defect class it gestures at is already enforced by Linux-runnable ESLint rules + under ADR-1703 (`no-hardcoded-tmp`, `no-path-literal-in-assert`). + +**The repo had already reached this conclusion for one consumer and not the other.** +`gen-platform-conformance-tier.cjs` carried: + +```js +// Two CATEGORIES entries precise enough for TEST-file classification (this +// module's own purpose) but far too broad for SOURCE-file reachability +const NOISY_FOR_SOURCE_REACHABILITY = new Set(['hardcoded-path-vs-path-call', 'symlink-keyword']); +``` + +Measured there: `classifyContent` over `src/` flagged 100 of 235 files; excluding these two narrowed +it to 28, "all verified to carry a genuine platform-conditional branch." The same over-breadth +verdict was reached, recorded, and then not applied to the tier itself. + +### Why no gate caught it + +Phase 2's acceptance criterion was *"conformance-tier file list exists as a single source of truth."* +It checked that the list **exists**. No phase asserted it was **small**, and no test would have +failed if the classifier had put all 930 files in. The #4591 per-file parity-baseline diff that +would have caught it was explicitly never performed — disclosed in the generator's own header as a +KNOWN LIMIT — and Phase 5 then retired `test-full`, the safety net that disclosure named as its +compensating control. + +## Decision + +### 1. Delete the `scope: windows` lane; port its residue to reachability + +The alternative — *gating* `windows.add(file)` on `reachesConformanceTierOrSeam` — was evaluated and +**rejected as provably redundant**. For a test file that predicate is literally: + +```js +if (file.startsWith('tests/') && file.endsWith('.test.cjs')) { + const { CONFORMANCE_TIER_FILES } = loadConformanceTier(); + return CONFORMANCE_TIER_FILES.includes(file); +} +``` + +and the same predicate is what sets `full_matrix`, which is what turns `test-conformance` on. So +every file a gated lane would run is (a) already in the tier and (b) has already caused the +conformance lane to run the whole tier in the same workflow run. Gating does not reduce the +duplication; it makes it total. + +The lane's one non-redundant contribution is the `isWindowsHint` arm — tests pulled in by a path +RULE whose *filename* contains `windows`/`win32`/`shell`/`path`. That is a filename substring +heuristic, precisely the kind of unproven heuristic #4592 replaced with reachability. It is +therefore **ported into `reachesConformanceTierOrSeam`**: such a RULE-pulled test now sets +`full_matrix = true`, and the conformance lane covers it. The signal is preserved; the parallel lane +is not. + +**The escalation is tier-backed, and that condition is load-bearing.** Three variants were measured +over the 16 entries of `RULES`: + +| variant | predicate | rules firing | verdict | +|---|---|---:|---| +| A | `isWindowsHint(t)` | 6/16 | Can fire on a test that is **not** in the tier — `full_matrix` goes true, the conformance lane runs, and the hinted test still never runs on Windows. Cost without coverage. | +| **B (shipped)** | `isWindowsHint(t) && reachesConformanceTierOrSeam(t)` | 6/16 | Identical firing set to A *today*, so no behavior change — but correct by construction: it can only escalate when the conformance lane will actually run the file. | +| C | `reachesConformanceTierOrSeam(t)` alone | **14/16** | Rejected as over-broad. Would newly escalate most ordinary product-code PRs (`src/`, `agents/`, `commands/`, `hooks/`, `skills/`, config paths) — tier membership alone is too weak a trigger. | + +A and B coincide only because every windows-hint test currently pulled in by a rule happens to be in +the tier except one (`tests/normalize-path-in-content.rule.test.cjs`, which has zero signals). B is +shipped because that coincidence is not an invariant. + +Live effect is deliberately small: of the six rules that fire, four already set `fullMatrix: true` +(no-op), `inert CI`'s escalation is overridden downstream by the inert-CI reset, and exactly one — +`portability lint rules (ADR-1703)` — genuinely changes behavior. + +`test-conformance` becomes the **sole** Windows selector, matching how it already is the sole macOS +selector. + +### 2. Remove the two house-idiom detectors, and add one narrow replacement + +`process-seam-subprocess` and `hardcoded-path-vs-path-call` are deleted from `CATEGORIES`. +`hardcoded-path-vs-path-call` leaves `NOISY_FOR_SOURCE_REACHABILITY` with it (the set now holds +`symlink-keyword` alone). Tier, all measured on one tree (932 eligible): **548 → 257 (58.8% → 27.6%)** +by removing the two detectors, then **257 → 266 (28.5%)** once the narrow `shell-interpreter-spawn` +replacement added 9 genuinely shell-spawning tests back. Net: **282 files removed, 9 restored**. +`ALWAYS_REAL_OS` currently adds **0** — see Consequences for why it is still there. + +Measured, the change is surgical: `src/` reachability is **28 → 28, zero files change status**, +because `hardcoded-path-vs-path-call` was already excluded there and no `src/` file matches the +test-helper regexes. Phase 3's classifier behavior for `src/` diffs is provably unchanged. + +### 3. A proportional ceiling, asserted failing-first + +The missing Phase 2 gate is added as a test, and it is expressed as a **ratio against a live +denominator**, not a count: + +| tier | measured | ceiling | +|---|---:|---:| +| Windows (`CONFORMANCE_TIER_FILES`) | 28.5% | **33%** | +| macOS (`MACOS_CONFORMANCE_TIER_FILES`) | 21.2% | **25%** | + +An absolute count goes stale as the suite grows and silently stops binding; the property that +matters — "a tier, not the suite" — is inherently proportional. The ceiling is deliberately *not* +today's emitted value, which #4641 rules out explicitly as a non-bound. + +## Consequences + +- Non-Linux jobs on a `full_matrix` PR: **7 → 4**. Against the true pre-epic baseline of 9, epic + #4589 plus this ADR deliver **9 → 4 (-56%)**, versus the -33% its closeout claimed against a + denominator that excluded this lane. + + **Measured, not computed** — read off real job lists rather than derived from the workflow file, + which is the verification epic #4589's own closeout skipped: + + | | PR #4640 (the trigger) | PR #4643 (this change) | + |---|---:|---:| + | jobs in the completed `test.yml` run | 21 | **17** | + | non-Linux jobs | 7 | **4** | + | `test` job | 4 ubuntu + 3 windows | 4 ubuntu, **0 windows** | + | conformance tier size | 548 files | **266 files** | + + Both job totals are counted the same way — every job in the *completed* run, which includes the + post-test `Coverage gate` and baseline-publisher jobs. An earlier draft of this table compared + #4640's completed total against this run's count at matrix-expansion time, before those trailing + jobs exist; that is an apples-to-oranges comparison and the kind of error this ADR is otherwise + about, so it is called out rather than quietly corrected. + + One caveat stated rather than glossed: a PR's *total check count* is not a clean before/after, + because many gates are path-scoped and this change touches a broader path set than #4640. The + like-for-like figure is the `test.yml` job count and its non-Linux portion, which is what the + epic's goal was about. + + **Wall-clock, measured on both runs — and the honest read is that this is a correctness win more + than a speed one:** + + | conformance job | #4640 (548-file tier) | #4643 (266-file tier) | | + |---|---:|---:|---| + | windows shard 1/3 | 29m47s | **21m12s** | -29% | + | windows shard 2/3 | 29m00s | **26m21s** | -9% | + | windows shard 3/3 | **40m24s** | **31m27s** | -22% | + | macOS | 17m48s | 21m02s | +18% | + + File count fell 52% but wall-clock only 9-29%, because the files removed were the *cheap static* + ones — the tier that remains is concentrated in genuinely expensive spawn-heavy work, which is + exactly what it should contain. Do not expect a future narrowing to buy time proportional to file + count. The macOS figure moved the wrong way while its tier was **unchanged by this PR** (198 files; + it tracks `next`'s test count, not this change), which + fixes it as runner variance rather than an effect of this change, and is a caution against reading + any single duration as signal. + + The load-bearing number is shard 3/3: it ran at **40m24s against a 45-minute cap**, 90% of the + cliff that #869 and #3057 were both filed about. Pulling it to 31m27s restores real headroom. +- **282 test files leave real-OS Windows execution** — 291 dropped when the two detectors were + removed, 9 restored by the narrow `shell-interpreter-spawn` replacement. + This is a real coverage change, not a refactor. It is defensible because every file that stays out + does so by losing a signal that was never a platform signal — each remains covered by the Linux + run, and the files that genuinely spawn a real binary are untouched or restored + (`raw-child-process`, 96 files; `shell-interpreter-spawn`, 33). + + The drop-out set was audited rather than assumed. Of those initially dropped, **14** had a filename suggesting + platform relevance (`/windows|win32|shell|path|platform|posix|crlf|symlink|exec|spawn|subprocess/i`), + and each was inspected. Six carry an explicit `allow-test-rule: source-text-is-the-product` or + `structural-regression-guard` marker; the rest were read individually. + + **That audit initially reached the wrong conclusion, and the correction is the most important + thing in this ADR.** Its first pass concluded all 14 were static analyses or seam-mediated CLI + tests. An adversarial review found a counterexample by reading *call semantics* rather than + filenames: `tests/execute-phase-worktree-guard.test.cjs` calls + + ```js + runHook('-c', [guardScript()], { interpreter: 'bash', cwd: dir, … }) + ``` + + and `tests/helpers/process-seam.cjs`'s `runHook` spawns `options.interpreter` through a real + `spawnSync`. With `interpreter: 'bash'` that is a **real bash binary** executing a shell script + extracted from workflow markdown, doing real git plumbing — bash availability, quoting, and git + output parsing all differ on Windows. No injected-`platform` unit test stands in for that. + + **The seam argument therefore needs a boundary it did not originally state.** "Going through the + seam is not a platform signal" is true of `src/shell-command-projection.cts`, which takes + `platform` as an injected parameter. It is **not** true of `tests/helpers/process-seam.cjs`, whose + `runHook`/`runGit` spawn real binaries. Conflating the two is what made the original + `process-seam-subprocess` detector look purely noisy: it was 99% noise wrapping a real signal. + + **On `ALWAYS_REAL_OS` adding zero today — disclosed, not hidden.** The allowlist holds one entry, + `tests/external-descriptor-confinement.test.cjs`, and it currently contributes **0 files**, because + this PR's own win32 test cases introduced the literal `win32` into that file and it now classifies + in on content via `win32-darwin-literal`. A future reader measuring the allowlist's marginal + contribution will get zero and may conclude the mechanism is dead. It is not, and the entry stays: + the file's real-OS need is a property of the CODE UNDER TEST — `isPathConfined` reads the ambient + `path` module — not of the test's text, and the text that currently saves it is incidental. Rewrite + those cases to use a helper without the literal and the file drops out silently. The pin exists + precisely for that, and the tests assert every entry names a file that exists so a stale entry + fails loudly rather than rotting. + + The fix is a narrow replacement category rather than restoring the blanket one: + + ```js + { name: 'shell-interpreter-spawn', + test: (c) => /interpreter:\s*['"`](bash|sh|zsh|dash|pwsh|powershell|cmd)['"`]/.test(c) } + ``` + + Measured 2026-09-11: 33 eligible files match, **9** of them were outside the tier and are added + back, taking it from 257 to **266 of 932 (27.6% → 28.5%)**, which is the committed total. Still under the 33% ceiling. Every one + of the 9 was confirmed by reading the matching source line — all are live `interpreter:` options on + real `runHook`/`runHookSeam` calls, zero comment or fixture matches. Two narrower alternatives + (`runGit(` alone; non-node `spawnSeam(`) were measured and rejected: each adds 9 files but **misses + the counterexample entirely**, because it spawns through `runHook`'s `interpreter` option rather + than through `runGit`. + + The lesson is recorded deliberately: an audit that selects candidates by filename inherits exactly + the defect this ADR is fixing in the classifier. The 14-file filename sweep was the right first cut + and the wrong last word. + + The worked example is `tests/windows-robustness.test.cjs`, which was on this ADR's own first-draft + "must remain in the tier" list **because of its filename**. It does not spawn anything: it reads + other files' source text and asserts on it (`assert.match(region, /windowsHide:\s*true/)`), and its + apparent `spawnSync(` / `execFileSync(` occurrences are string literals used as *search anchors* + into those other files. It is fully Linux-runnable and correctly drops out. Selecting it by name + would have been the same error the classifier makes — and a test now pins that it drops out, with + the reason, so nobody "fixes" it back in. + + The clearest statement of this ADR's thesis is one the repo already wrote. `tests/hardcoded-paths.test.cjs`, + itself a drop-out, opens: *"Statically scans source files to catch hardcoded platform-specific + paths… Catches issues that previously required a real Windows runner to detect."* +- `classify()` no longer returns a `windows_tests` key; `ci-prepare-test-scope.cjs` no longer + accepts a `windows` scope. Both are removed rather than left inert, so a future reader cannot + mistake a dead output for a live one. +- ADR-4593's **decision is unaffected**: `MACOS_CATEGORIES` is a separate array, `chmod-mode-bit` + and `symlink-keyword` keep their recorded rationale and their definitions, and the macOS tier + is unchanged — the regenerated `macos-conformance-tier.generated.cjs` is byte-identical to the one + on `next` (`git diff` reports zero changed lines). + Its five prose citations of the 546 figure are **left as written**: they were accurate on + 2026-09-10 and an ADR is a dated record, not a live reference page. ADR-4593 instead carries a + short amendment note pointing here, so a reader who arrives at the 546 figure learns it has since + moved without the original reasoning being rewritten underneath them. + + Neither tier's size is asserted as a literal count anywhere in the test suite: the ceilings are + ratios against a live denominator, and the macOS list is pinned by comparing the committed file to + a fresh classification of the live tree. A count hardcoded in a test is a failure scheduled for + whenever the suite next grows — which is exactly how the first draft of this work broke. + +### Risk accepted, and why it is not a rerun of #962 + +#962 narrowed Windows coverage and was rescinded (#4421) after a macOS-only failure merged green. +That failure was root-caused to a rendered-text-length assertion sensitive to tmpdir path length — +a violation of ADR-456's typed-surface mandate that nothing enforced. Epic #4589 **Phase 1 shipped +that enforcement** (`local/no-rendered-text-length-assert`, error from the moment it landed). The +specific defect class that made the last narrowing unsafe is now statically prevented, which is the +condition #4589 itself named as the precondition for narrowing. That is the difference, and it is +why this narrowing rests on an enforced invariant rather than on optimism. + +## Rejected alternatives + +- **Gate the lane instead of deleting it.** Rejected: provably redundant, shown above. Gating would + have produced a lane whose main arm duplicates the conformance lane exactly and whose residual arm + is a filename heuristic. +- **Keep the lane ungated as deliberate belt-and-braces.** Rejected: it does not function as a + safety net for the files it duplicates, and for the files it does not duplicate it selects them by + filename substring. Paying three Windows runners per PR for that is not a trade-off, it is an + accident preserved. +- **Narrow `hardcoded-path-vs-path-call` to same-line proximity rather than deleting it.** Rejected: + ADR-1703's Linux-runnable rules already enforce the class, so real-OS execution buys nothing. +- **Also drop `symlink-keyword`** (measured at the time as 228 rather than 254, before the + `shell-interpreter-spawn` replacement took the tier to its final 266). + Rejected: worth 6 unique files, and ADR-4593 reuses it in `MACOS_CATEGORIES` with recorded + rationale. +- **Narrow `chmod-mode-bit`'s bare-octal arm.** #4641's text named this as a co-driver. Measurement + says otherwise: 51 files match only via the bare-octal arm, but for **4** is `chmod-mode-bit` the + sole signal. Changing it would invalidate ADR-4593's measured macOS table for a 4-file benefit. + Rejected on evidence; the issue's claim is corrected here. +- **An absolute file-count ceiling.** Rejected: goes stale under suite growth and stops binding + without anyone noticing — the same failure shape as Phase 2's "the list exists" criterion. +- **A companion "sole-signal concentration" ceiling** — no single category may be the sole signal for + more than N% of the tier. Proposed because the ratio ceiling has a real Goodhart weakness: a ratio + can be satisfied by inflating the *denominator*, so adding OS-agnostic tests loosens it without + narrowing the tier. Concentration looked like the harder-to-fake companion, since the original + defect was precisely one detector carrying half the tier. **Measured, and rejected on the numbers.** + Post-fix the peak sole-signal share is `raw-child-process` at ~53/266 = **~20%**, against the two + historic offenders at 21.6% (`process-seam-subprocess`) and 19.8% (`hardcoded-path-vs-path-call`). + Any threshold above 20% would have missed the original defect; any threshold below it fails today + on a category that is entirely legitimate — a test that spawns a real subprocess genuinely needs a + real OS. Concentration cannot separate "a big honest category" from "a big dishonest one"; the + discriminator is whether the signal is platform-meaningful, which is a judgement no threshold + encodes. The ratio ceiling stands alone, with its denominator-inflation weakness disclosed rather + than papered over by a second gate that does not actually bind. +- **Relaxing `raw-child-process` to drop its `content.includes('child_process')` precondition.** + Investigated and rejected on measurement. The narrowing appeared to unmask a false negative: + `tests/windows-robustness.test.cjs` contains `spawnSync(` and `execFileSync(` yet does not match + `raw-child-process`, which looked like the precondition being over-tight (its stated rationale — + stopping a local identifier such as `spawnResult` from matching — is already served by the + trailing paren in `\bspawnSync\(`). Relaxing it was measured to add **13** files, and reading the + matching line in each showed **all 13 are false positives**: comment text, jsdoc prose describing + a return shape, template-literal code fixtures fed to an ESLint rule under test, and the + search-anchor string literals described above. There is no false negative. The precondition stays + exactly as written, and this paragraph exists so the same apparent bug is not "fixed" next time. diff --git a/docs/adr/README.md b/docs/adr/README.md index 06155ea96..eabf677e6 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -269,6 +269,7 @@ These govern the system as it stands. Cite these. | [ADR-3806](3806-review-dispositions-ledger.md) | Review Dispositions Ledger canonizes where and how reviews-mode records incorporate/defer decisions in PLAN.md | Accepted | — | | [ADR-4139](4139-compact-content-seam.md) | The compact-content seam — shrink the eager window, never the guarantee | Accepted | — | | [ADR-4593](4593-macos-conformance-tier-architecture.md) | A macOS-specific conformance-tier classifier, separate from the Windows-oriented one | Accepted | — | +| [ADR-4641](4641-windows-selector-consolidation.md) | One Windows test selector, and a proportional ceiling on the conformance tier | Accepted | — | ### Proposed diff --git a/scripts/ci-prepare-test-scope.cjs b/scripts/ci-prepare-test-scope.cjs index 90d5aef50..a841c8b80 100644 --- a/scripts/ci-prepare-test-scope.cjs +++ b/scripts/ci-prepare-test-scope.cjs @@ -4,9 +4,9 @@ // Shell-agnostic: invoked as `node scripts/ci-prepare-test-scope.cjs` from any shell. // // Required environment variables (set by the workflow step's `env:` block): -// TEST_SCOPE — "windows" | "targeted" +// TEST_SCOPE — "targeted" (the `windows` scope was retired by #4641: +// test-conformance is now the sole Windows selector) // TARGETED_TESTS — space-separated test file list (from ci-test-scope.cjs output) -// WINDOWS_TESTS — space-separated test file list for the windows lane // // Writes: .ci-selected-tests.txt (one file per line, no blanks) // Exit 0 = success; exit 1 = unknown scope. @@ -46,13 +46,14 @@ function isResolvable(entry, root) { // Resolve the scoped test selection for the lane. Pure (no I/O beyond the // existence probe under `root`) so it can be unit-tested directly. -function resolveSelection({ scope, targeted, windows, root }) { +function resolveSelection({ scope, targeted, root }) { let selected; - if (scope === 'windows') { - selected = windows; - } else if (scope === 'targeted') { + if (scope === 'targeted') { selected = targeted; } else { + // #4641: the `windows` scope was retired (test-conformance is now the + // sole Windows selector) — an unknown/retired scope must fail loudly + // rather than silently selecting nothing. throw new ExitError(1, `::error::Unknown test scope: ${scope}`); } @@ -75,7 +76,6 @@ function main() { const lines = resolveSelection({ scope: process.env.TEST_SCOPE || '', targeted: process.env.TARGETED_TESTS || '', - windows: process.env.WINDOWS_TESTS || '', root, }); diff --git a/scripts/ci-test-scope.cjs b/scripts/ci-test-scope.cjs index f0548c9da..f1857b864 100644 --- a/scripts/ci-test-scope.cjs +++ b/scripts/ci-test-scope.cjs @@ -485,6 +485,15 @@ function addAll(set, values) { // fullMatrix=true, so the full Windows lane already runs when those paths // change. The old six-hint list pulled 102 of ~633 test files into the scoped // windows lane, turning it into a ~10-minute job on every PR. +// #4641: the scoped windows lane itself is gone. These hints now drive +// full_matrix instead — a matched RULE whose tests[] includes a +// windows-hint filename AND that hinted file is itself in the conformance +// tier (per reachesConformanceTierOrSeam) escalates straight to full_matrix +// (routed to test-conformance, the sole Windows selector) rather than +// feeding a side lane. Tier-backed on purpose: full_matrix only ever runs +// CONFORMANCE_TIER_FILES, so a hint on a non-tier file would cost 4 CI jobs +// with zero Windows coverage of that file. See +// docs/adr/4641-windows-selector-consolidation.md. const WINDOWS_HINTS = ['windows', 'win32', 'shell', 'path']; const isWindowsHint = s => WINDOWS_HINTS.some(k => s.toLowerCase().includes(k)); @@ -538,7 +547,6 @@ function reachesConformanceTierOrSeam(file, deps = {}) { // the real, committed generated file. function classify(files, reachabilityDeps = {}) { const targeted = new Set(); - const windows = new Set(); const reasons = []; let productOrPipelineChanged = false; // product/pipeline code (excludes docs) let inertCiChanged = false; // inert workflow files @@ -575,7 +583,6 @@ function classify(files, reachabilityDeps = {}) { if (file.startsWith('tests/') && file.endsWith('.test.cjs')) { targeted.add(file); - windows.add(file); // #494 originally narrowed this to skip full_matrix for changed test // files, on the theory that ubuntu targeted_tests + the scoped windows // lane already covered them. Rescinded per #4421: PR #4384 landed a @@ -612,6 +619,32 @@ function classify(files, reachabilityDeps = {}) { addAll(targeted, rule.tests); reasons.push(`${file}: ${rule.name}`); if (rule.fullMatrix) fullMatrix = true; + // #4641: a rule that pulls in a test file matching a Windows-sensitive + // filename hint is the one non-redundant residue of the deleted + // scoped windows lane — escalate to full_matrix (test-conformance) + // instead of feeding a side lane. TIER-BACKED (measured): full_matrix + // routes to test-conformance, which runs ONLY CONFORMANCE_TIER_FILES — + // escalating on the filename hint alone can fire on a hinted test that + // isn't in that tier, costing 4 CI jobs while never actually running it + // on Windows. The predicate below requires BOTH the hint AND tier + // membership (via reachesConformanceTierOrSeam, the same fail-safe + // reachability helper used elsewhere in this file, so error/uncertainty + // behavior stays identical). Measured: over the 16 RULES entries this + // narrowed form fires on the same rules as the un-narrowed form today + // (no behavior change now, correct-by-construction going forward). The + // broader alternative — escalate on ANY tier member a rule pulls in, + // ignoring the filename hint — was measured and rejected: it fires on + // 14 of 16 rules, newly escalating most ordinary product-code PRs + // (src/, agents/, commands/, hooks/, skills/, config paths). Only one + // rule's escalation is live today: 'portability lint rules (ADR-1703)'. + // Four others already had fullMatrix: true (no-op here), and 'inert + // CI''s escalation is overridden downstream by the inert-CI reset. + // Distinct, greppable reason so the conformance-lane coverage for it + // is traceable per rule. + if (rule.tests.some(t => isWindowsHint(t) && reachesConformanceTierOrSeam(t, reachabilityDeps))) { + fullMatrix = true; + reasons.push(`${file}: ${rule.name} (windows-hint rule test, #4641)`); + } } } } @@ -625,7 +658,7 @@ function classify(files, reachabilityDeps = {}) { // covered by .github/workflows/install-smoke.yml 'tests/release-tarball-smoke.install.test.cjs', ]); - for (const f of SCOPED_LANE_EXCLUDE) { targeted.delete(f); windows.delete(f); } + for (const f of SCOPED_LANE_EXCLUDE) { targeted.delete(f); } // code_changed: true when product/pipeline OR inert CI changed. // Docs-only PRs (neither flag set) get code_changed=false → full matrix skip. @@ -639,8 +672,6 @@ function classify(files, reachabilityDeps = {}) { targetedTests.push('unit'); } - const windowsTests = existingTests([...new Set([...windows, ...targetedTests.filter(isWindowsHint)])].sort()); - // Inert-CI-only: full_matrix must be false (override any RULES that fired). if (inertCiChanged && !productOrPipelineChanged) { fullMatrix = false; @@ -649,12 +680,11 @@ function classify(files, reachabilityDeps = {}) { // Normalize: when code_changed is false, the output must be self-consistent. // A docs file can coincidentally match a coarse content RULE (e.g. docs/installer-migrations.md // matches the installer rule via path.includes('install')), leaving full_matrix=true and - // non-empty targeted_tests/windows_tests. The workflow skips correctly (gated on code_changed) + // non-empty targeted_tests. The workflow skips correctly (gated on code_changed) // but the output object would be self-contradictory. Force a clean "nothing to run" result. if (!codeChanged) { fullMatrix = false; targetedTests.length = 0; - windowsTests.length = 0; } return { @@ -662,7 +692,6 @@ function classify(files, reachabilityDeps = {}) { product_changed: productOrPipelineChanged, full_matrix: fullMatrix, targeted_tests: targetedTests, - windows_tests: windowsTests, reasons: [...new Set(reasons)].sort(), }; } @@ -674,7 +703,6 @@ function writeOutputs(result) { `product_changed=${result.product_changed}`, `full_matrix=${result.full_matrix}`, `targeted_tests=${result.targeted_tests.join(' ')}`, - `windows_tests=${result.windows_tests.join(' ')}`, ]; appendFileSync(process.env.GITHUB_OUTPUT, `${lines.join('\n')}\n`); } diff --git a/scripts/docs-guard-registry.cjs b/scripts/docs-guard-registry.cjs index a1f206e30..9dba40c1b 100644 --- a/scripts/docs-guard-registry.cjs +++ b/scripts/docs-guard-registry.cjs @@ -9,8 +9,12 @@ * * A prior version of this PR added a `docs guards` RULE to `RULES` in * scripts/ci-test-scope.cjs, on the theory that classify()'s `!codeChanged` - * normalization (which zeroes fullMatrix/targeted_tests/windows_tests for - * docs-only diffs) made the RULE inert to classify()'s scope decision. + * normalization (which at the time zeroed fullMatrix/targeted_tests/ + * windows_tests for docs-only diffs) made the RULE inert to classify()'s + * scope decision. (#4641 later removed `windows_tests` from classify()'s + * output entirely; the normalization today only zeroes fullMatrix and + * targeted_tests. This paragraph is historical narration of a rejected + * design, not a description of current behaviour.) * * That is true for docs-ONLY diffs and FALSE for MIXED docs+code diffs: * `codeChanged` is true whenever ANY changed file is product/pipeline code, diff --git a/scripts/gen-platform-conformance-tier.cjs b/scripts/gen-platform-conformance-tier.cjs index 7c8940b81..d72d2b78f 100644 --- a/scripts/gen-platform-conformance-tier.cjs +++ b/scripts/gen-platform-conformance-tier.cjs @@ -35,6 +35,27 @@ * is the safe direction — a false positive costs one extra test running on a * real OS; a false negative silently drops real-OS coverage). * + * That stance is a correct per-file tiebreak, but proved wrong in aggregate + * (#4641): applied to two categories that matched the house test idiom + * rather than a genuine platform signal, it produced a Windows "tier" of + * 547 of 931 eligible unit-suite files (58.8%, measured 2026-09-11) — most + * of the suite. Measured per-category UNIQUE (sole-signal, i.e. the file + * would have been excluded without it) contribution as of that same + * measurement: `process-seam-subprocess` 118 files, `hardcoded-path-vs- + * path-call` 108 files, every other category 41 files COMBINED. Both were + * removed outright from CATEGORIES; the same-tree, same-day recount put + * the tier at 255 of 931 (27.4%) — exactly 292 entries removed from the + * committed list, none added. The macOS tier was unaffected by this change + * (a diff of macos-conformance-tier.generated.cjs across the same removal + * showed zero changed lines). None of these counts is asserted as a + * literal anywhere in the test suite: the ceilings this generator enforces + * are ratios against a live denominator (the current eligible-file count), + * and the committed lists are pinned by comparing against a fresh + * classification of the live tree, not against a hardcoded number — + * deliberately, since a hardcoded count in a test is a failure scheduled + * for the next time the suite grows. See + * docs/adr/4641-windows-selector-consolidation.md for the full rationale. + * * KNOWN LIMIT, disclosed deliberately: this is a STATIC content classifier, * not a real per-file, per-OS behavioral diff. Epic #4589's issue #4591 asked * for the cutover to be validated by "running the existing full matrix one @@ -129,10 +150,6 @@ const CATEGORIES = [ name: 'windows-env-var', test: (content) => /\bPATHEXT\b|\bUSERPROFILE\b|\bHOMEDRIVE\b|\bHOMEPATH\b/.test(content), }, - { - name: 'process-seam-subprocess', - test: (content) => /\brunNode\(|\brunGit\(|\brunHook\(|\brunGsdTools\(|\bgitOrThrow\(/.test(content), - }, { name: 'raw-child-process', // Requires BOTH the child_process import/reference token AND one of the @@ -145,6 +162,32 @@ const CATEGORIES = [ return /\bspawnSync\(|\bexecSync\(|\bexecFileSync\(/.test(content); }, }, + { + name: 'shell-interpreter-spawn', + // Added after an adversarial review (#4641) caught a REAL false negative + // introduced by removing 'process-seam-subprocess' above: that removal + // also dropped the only coverage for tests/execute-phase-worktree-guard. + // test.cjs, which calls tests/helpers/process-seam.cjs's `runHook(..., + // { interpreter: 'bash', ... })`. `runHook` spawns `options.interpreter` + // via a real `spawnSync`, so `interpreter: 'bash'` is a genuine real-shell + // invocation — bash availability, quoting, and git output parsing all + // differ across OSes. This is deliberately narrower than (and does not + // reintroduce) 'process-seam-subprocess': the rationale that going + // through the injected-`platform`-parameter seam in + // src/shell-command-projection.cts is NOT a platform signal (because the + // caller supplies `platform` itself) holds for THAT seam only — it does + // not hold for `runHook`'s `interpreter` option, which spawns a real + // interpreter binary rather than taking platform as injected data. + // Measured 2026-09-11: 33 eligible files match this pattern; 9 of them + // were outside the committed Windows tier and are added back by this + // change, taking the tier from 255 to 264 of 931 eligible files (27.4% -> + // 28.4%), still under the 33% ceiling. All 9 additions were verified by + // reading the matching source line: 0 false positives, every match is a + // live `interpreter:` option on a real `runHook`/`runHookSeam` call. As + // with all counts in this file, these are dated point-in-time + // measurements, not standing facts. + test: (content) => /interpreter:\s*['"`](bash|sh|zsh|dash|pwsh|powershell|cmd)['"`]/.test(content), + }, { name: 'symlink-keyword', // A leading `\b` with no trailing one, case-insensitive: this is @@ -155,25 +198,65 @@ const CATEGORIES = [ // alone still excludes a mid-word embedding like "presymlink". test: (content) => /\bsymlink/i.test(content), }, - { - name: 'hardcoded-path-vs-path-call', - // Intentionally coarse (design doc: over-inclusion is the safe - // direction): a path.* call ANYWHERE in the file plus a quoted - // forward-slash-leading string literal ANYWHERE in the file, with no - // attempt at proximity/scoping. - test: (content) => - /path\.(join|resolve|dirname|basename|normalize|relative)\(/.test(content) && - /['"`]\/[\w.\-/]*['"`]/.test(content), - }, ]; -// Two CATEGORIES entries precise enough for TEST-file classification (this -// module's own purpose) but far too broad for SOURCE-file reachability -// (scripts/ci-test-scope.cjs's #4592 use). Empirically verified: applying -// classifyContent to every file under src/ (235 files) flags 100 of them, -// driven almost entirely by these two categories; excluding them narrows it -// to 28 files, all verified to carry a genuine platform-conditional branch. -const NOISY_FOR_SOURCE_REACHABILITY = new Set(['hardcoded-path-vs-path-call', 'symlink-keyword']); +// A CATEGORIES entry precise enough for TEST-file classification (this +// module's own purpose) but too broad for SOURCE-file reachability +// (scripts/ci-test-scope.cjs's #4592 use). This set used to hold a second +// member, 'hardcoded-path-vs-path-call', alongside 'symlink-keyword'; that +// category was removed outright from CATEGORIES (#4641 — measured to be the +// single largest driver of Windows-tier over-inclusion, a universal Node +// test-suite idiom rather than a platform signal), not merely exempted here, +// because it was over-broad for BOTH consumers (this module's own Windows +// tier AND source reachability), not source-reachability alone. Only +// 'symlink-keyword' remains: still precise enough for test-file +// classification but, per the same empirical pass described above, too noisy +// for source reachability. +const NOISY_FOR_SOURCE_REACHABILITY = new Set(['symlink-keyword']); + +/** + * Escape hatch, WINDOWS TIER ONLY (union'd into `classifyTree`, never into + * `classifyMacosTree`/`MACOS_CATEGORIES` — those stay untouched by this map). + * + * `classifyContent` above is a STATIC CONTENT classifier: it can only see + * text in the test file itself. Some files need real-OS coverage for a + * reason that lives in the CODE UNDER TEST, not in the test's own text — no + * regex over the test file can ever detect that, because the signal simply + * isn't there to find. Rather than chase that gap with ever-more-specific + * content heuristics (the exact failure mode #4641 measured and rolled + * back — see the header comment above), this map is the single, centrally- + * enumerated source of truth for those cases, matching ADR-1703's + * `portability-vocab.cjs` stance and epic #4589 Phase 2's explicit + * requirement that such overrides be "centrally-enumerated, not a naming + * convention". It is deliberately NOT a heuristic: it is a `Map` (path -> + * reason) precisely so every entry is forced to carry a recorded, + * human-reviewed reason at the call site — an entry without one is + * impossible by construction (there is no positional/array form that would + * let a path be added without a paired reason string). + * + * Adding an entry requires a recorded reason and should be rare: prefer + * fixing the classifier (a new CATEGORIES signal) when the real-OS need IS + * expressible as content; reach for this map only when it structurally is + * not. + * + * Current entries: + * - tests/external-descriptor-confinement.test.cjs: exercises `isPathConfined` + * (src/external-descriptor-trust.cts:41-49), which calls the AMBIENT + * `path` module directly — `path.resolve(root, target)` and `path.sep` — + * with no platform/path injection seam. Its win32 semantics (drive + * letters, UNC paths, `\` separator) are therefore only reachable by + * actually running on Windows; the win32 branch is unreachable on Linux. + * This is a security-relevant write-confinement gate, so a silent gap + * here is a security regression, not a coverage nit (#4641). + */ +const ALWAYS_REAL_OS = new Map([ + [ + 'tests/external-descriptor-confinement.test.cjs', + 'Exercises isPathConfined (src/external-descriptor-trust.cts:41-49), which uses the ambient ' + + 'path module (path.resolve/path.sep) with no platform injection; its win32 branch (drive ' + + 'letters, UNC paths, \\ separator) is unreachable on Linux. Security-relevant write-confinement gate.', + ], +]); /** * macOS-specific detection categories (#4593, design doc @@ -274,15 +357,20 @@ function classifyTree(testsDir) { const unitFiles = absoluteFiles.filter((absPath) => suiteOf(absPath) === null); const flagged = []; for (const absPath of unitFiles) { + const rel = 'tests/' + path.relative(testsDir, absPath).replace(/\\/g, '/'); const content = fs.readFileSync(absPath, 'utf8'); const { needsRealOs } = classifyContent(content); - if (needsRealOs) { - const rel = path.relative(testsDir, absPath).replace(/\\/g, '/'); - flagged.push('tests/' + rel); + // The ALWAYS_REAL_OS escape hatch (Windows tier only — see its doc + // comment) is unioned in HERE, keyed off a file that this walk actually + // found, rather than blindly appended regardless of `testsDir` — that + // keeps the escape hatch from leaking a real-repo path into an unrelated + // temp-fixture-tree classification (e.g. this module's own tests). + if (needsRealOs || ALWAYS_REAL_OS.has(rel)) { + flagged.push(rel); } } - flagged.sort(); - return { total: absoluteFiles.length, files: flagged }; + const result = [...new Set(flagged)].sort(); + return { total: absoluteFiles.length, files: result }; } /** @@ -458,6 +546,7 @@ module.exports = { classifyContent, CATEGORIES, NOISY_FOR_SOURCE_REACHABILITY, + ALWAYS_REAL_OS, walkTestFiles, classifyTree, renderGeneratedFile, diff --git a/scripts/lib/platform-conformance-tier.generated.cjs b/scripts/lib/platform-conformance-tier.generated.cjs index d727f6b32..7b3139c92 100644 --- a/scripts/lib/platform-conformance-tier.generated.cjs +++ b/scripts/lib/platform-conformance-tier.generated.cjs @@ -3,41 +3,19 @@ module.exports = { CONFORMANCE_TIER_FILES: [ - "tests/active-workstream-store.test.cjs", - "tests/active-workstream-store.unit.test.cjs", - "tests/adr-15-progress-converge.test.cjs", - "tests/adr-22-plan-drift-guard.test.cjs", - "tests/adr-612-bracket-coherence.test.cjs", - "tests/adr-612-bracket-phase-counting.test.cjs", - "tests/adr-612-bracket-read-tolerance.test.cjs", "tests/adr-index-gate.test.cjs", "tests/adr857-core-without-capabilities.test.cjs", - "tests/advance-plan-ambiguous-phase.test.cjs", - "tests/agent-frontmatter.test.cjs", - "tests/agent-hint-routing-1689.test.cjs", "tests/agent-install-check.test.cjs", "tests/agent-install-validation.test.cjs", "tests/agent-skills.test.cjs", - "tests/ai-evals.test.cjs", "tests/antigravity-upgrades.test.cjs", "tests/api-coverage-gate-e2e.test.cjs", "tests/api-coverage.test.cjs", "tests/assumption-delta-checkpoint-e2e.test.cjs", "tests/assumption-delta.test.cjs", "tests/audit-command-cutover.test.cjs", - "tests/audit-open-remainder-count.test.cjs", - "tests/audit-uat-acknowledged.test.cjs", - "tests/audit-uat-summary-segmentation.test.cjs", - "tests/audit-workstream-layouts.test.cjs", "tests/augment-upgrades.test.cjs", - "tests/autonomous-converge.test.cjs", - "tests/autonomous-interactive.test.cjs", - "tests/benchmark-compact-content.test.cjs", - "tests/branch-no-track-guard.test.cjs", - "tests/broken-windows-description.test.cjs", "tests/broken-windows.test.cjs", - "tests/bugs-1656-1657.test.cjs", - "tests/canary-version-leak-lint.test.cjs", "tests/capability-cli.test.cjs", "tests/capability-command-dispatch.test.cjs", "tests/capability-consent.test.cjs", @@ -45,152 +23,79 @@ module.exports = { "tests/capability-lifecycle.test.cjs", "tests/capability-loader.test.cjs", "tests/capability-lock-mkdir-failure-3987.test.cjs", - "tests/capability-precedence-parity.test.cjs", "tests/capability-probe-fallback.test.cjs", - "tests/capability-registry.test.cjs", "tests/capability-source.test.cjs", "tests/capability-state.test.cjs", - "tests/capability-trust.test.cjs", - "tests/capability-writer.test.cjs", - "tests/changeset-cli.test.cjs", - "tests/changeset-github-release-notes.test.cjs", - "tests/changeset-lint.test.cjs", "tests/changeset-new.test.cjs", - "tests/check-contract-drift.test.cjs", "tests/check-env.test.cjs", "tests/check-gap-analysis-plan-post-e2e.test.cjs", "tests/check-glossary-refs.test.cjs", "tests/check-tdd-review-checkpoint-e2e.test.cjs", - "tests/check-ui-plan-gate.test.cjs", "tests/check-ui-safety-gate.test.cjs", "tests/check-update-config-dir.test.cjs", "tests/chunked-planning-parallel.test.cjs", "tests/ci-docs-guard-registry.test.cjs", - "tests/ci-next-health.test.cjs", - "tests/ci-pr-mergeability.test.cjs", "tests/ci-rebase-check.test.cjs", "tests/ci-test-scope.test.cjs", "tests/cjs-command-router-adapter.test.cjs", - "tests/claude-md-path.test.cjs", "tests/claude-md.test.cjs", - "tests/claude-orchestration-command-router.test.cjs", - "tests/claude-orchestration.test.cjs", - "tests/claude-skills-migration.test.cjs", - "tests/cli-exit.test.cjs", "tests/cline-install.test.cjs", - "tests/clock-seam.test.cjs", - "tests/clock.test.cjs", "tests/close-phase-todos-padded-resolves.test.cjs", - "tests/code-review-command.test.cjs", - "tests/code-review-depth.test.cjs", - "tests/code-review-fix-pipeline-regression.test.cjs", "tests/code-review-pipeline-regression.test.cjs", "tests/code-review-tier3-files-override-scoping.test.cjs", "tests/code-review.test.cjs", - "tests/codebuddy-install.test.cjs", - "tests/codebuddy-upgrades.test.cjs", "tests/codex-config-agents.test.cjs", "tests/codex-config-hooks.test.cjs", "tests/codex-config-install.test.cjs", "tests/codex-config.test.cjs", - "tests/codex-declarative-reference.test.cjs", "tests/codex-inherit-smoke.test.cjs", - "tests/command-contract.test.cjs", "tests/command-routing-hub.test.cjs", "tests/commands.test.cjs", - "tests/commit-docs-bypass.test.cjs", "tests/commit-files-deletion.test.cjs", "tests/commit-files-pathspec.test.cjs", "tests/commonjs-marker.test.cjs", "tests/compact-content-4139.test.cjs", - "tests/compact-content-partition-guard.test.cjs", - "tests/completion-predicate-drift-guard.test.cjs", "tests/completion-ratio-scope-withholding.test.cjs", - "tests/completion-ratio-single-owner.test.cjs", - "tests/concurrency-safety.test.cjs", "tests/config-defaults-runtime-exclusion.test.cjs", - "tests/config-field-docs.test.cjs", "tests/config-get-default.test.cjs", "tests/config-loader.test.cjs", "tests/config-schema.property.test.cjs", "tests/config.test.cjs", - "tests/configuration-migrate-config.test.cjs", - "tests/context-drift.test.cjs", - "tests/context-predicates-query.test.cjs", "tests/copilot-install.test.cjs", "tests/copilot-upgrades.test.cjs", "tests/core-utils.test.cjs", - "tests/coverage-metadata-parser.test.cjs", - "tests/coverage-uat-routing.test.cjs", "tests/cursor-hook-workspace-roots.test.cjs", "tests/cursor-hooks.test.cjs", - "tests/cursor-reviewer.test.cjs", "tests/cursor-subagent-isolation.test.cjs", - "tests/debug-session-manager-commit.test.cjs", - "tests/decisions.test.cjs", - "tests/declarative-reference-antigravity.test.cjs", - "tests/declarative-reference-augment.test.cjs", - "tests/declarative-reference-codebuddy.test.cjs", - "tests/declarative-reference-copilot.test.cjs", - "tests/declarative-reference-windsurf.test.cjs", - "tests/declarative-reference-zcode.test.cjs", - "tests/default-flip-documentation-lint.test.cjs", "tests/dispatcher.test.cjs", - "tests/docs-hooks-table-parity.test.cjs", - "tests/docs-parity-live-registry.test.cjs", - "tests/docs-update.test.cjs", "tests/drift-detection.test.cjs", - "tests/edge-probe-spec-phase-contract.test.cjs", - "tests/edge-probe.test.cjs", - "tests/edit-phase-milestone-scope-guard.test.cjs", - "tests/edit-phase.test.cjs", "tests/effort-surface-axis.test.cjs", "tests/effort-sync-installed-runtime.test.cjs", "tests/emitted-ack-trailer.test.cjs", "tests/emitted-attribution.test.cjs", - "tests/emitted-caps-gate.test.cjs", "tests/emitted-provenance.test.cjs", - "tests/emitted-sizes.test.cjs", "tests/ensure-runtime-build.test.cjs", - "tests/enumeration-single-owner.test.cjs", - "tests/eslint-glob-coverage.test.cjs", "tests/eslint-rules.test.cjs", - "tests/estimate-calibrate.test.cjs", - "tests/estimate-loop-convergence.test.cjs", - "tests/execute-mvp-tdd-gate.test.cjs", "tests/execute-phase-decimal-arithmetic.test.cjs", - "tests/execute-phase-wave.test.cjs", "tests/execute-phase-worktree-guard.test.cjs", "tests/execute-plan-update-codebase-map-diff-base.test.cjs", "tests/execute-wave-post-gate-pipeline-e2e.test.cjs", "tests/executed-plan.test.cjs", "tests/executor-mvp-tdd-section.test.cjs", - "tests/exit-code-registry.test.cjs", - "tests/explore-command.test.cjs", "tests/external-descriptor-confinement.test.cjs", "tests/failing-direction.test.cjs", "tests/fallow-runner.test.cjs", "tests/faulty-deps.test.cjs", - "tests/feat-2296-provider-escalation.test.cjs", "tests/feat-2483-review-claude-mds-guard.test.cjs", - "tests/feat-2646-deferred-items-audit-scanner.test.cjs", - "tests/feat-3881-yaml-parser-consequences.test.cjs", "tests/features-index-gate.test.cjs", - "tests/fixture-builder.test.cjs", - "tests/frontmatter-cli.test.cjs", "tests/frontmatter.test.cjs", "tests/gap-checker.property.test.cjs", - "tests/gate-predicate-evaluator-missing.test.cjs", "tests/gemini-runtime-removed.test.cjs", "tests/gen-context-index.test.cjs", "tests/gen-health-docs.test.cjs", - "tests/gen-registry.test.cjs", "tests/gen-section-manifest.test.cjs", - "tests/gen-state-md-docs.test.cjs", "tests/git-base-branch.test.cjs", - "tests/git-fixture.test.cjs", "tests/golden-install-tree.test.cjs", - "tests/graphify-command-cutover.test.cjs", "tests/graphify-graph-path.test.cjs", "tests/graphify-visualization.test.cjs", "tests/graphify.test.cjs", @@ -198,26 +103,12 @@ module.exports = { "tests/gsd-check-update-worker-atomic-cache.test.cjs", "tests/gsd-check-update-worker-platform-gate.test.cjs", "tests/gsd-mcp-server-bin.test.cjs", - "tests/gsd-quick-batch-merge-integration.test.cjs", - "tests/gsd-quick-batch-workflow.test.cjs", - "tests/gsd-secret-read-guard.test.cjs", - "tests/gsd-settings-advanced.test.cjs", "tests/gsd-statusline.test.cjs", - "tests/gsd-tools-path-refs.test.cjs", "tests/gsd-validate-commit-crash-policy.test.cjs", - "tests/gsd-write-guard.property.test.cjs", "tests/gsd-write-guard.test.cjs", - "tests/gsd2-import.test.cjs", - "tests/hardcoded-paths.test.cjs", - "tests/health-diagnostic-rules/agent-install.test.cjs", "tests/health-diagnostic-rules/config-validation.test.cjs", - "tests/health-diagnostic-rules/consistency.test.cjs", - "tests/health-diagnostic-rules/phase-structure.test.cjs", - "tests/health-diagnostic-rules/root-existence.test.cjs", - "tests/health-diagnostic-rules/state-consistency.test.cjs", "tests/health-diagnostic-rules/worktree-health.test.cjs", "tests/health-diagnostic.test.cjs", - "tests/health-validation.test.cjs", "tests/helpers-cleanup.test.cjs", "tests/helpers-process-isolation.test.cjs", "tests/hermes-skills-migration.test.cjs", @@ -225,141 +116,71 @@ module.exports = { "tests/hooks-crash-policy.test.cjs", "tests/hooks-opt-in.test.cjs", "tests/host-integration.test.cjs", - "tests/host-runtime-detection.test.cjs", - "tests/ingest-docs.test.cjs", - "tests/init-debug.test.cjs", "tests/init-manager.test.cjs", "tests/init.test.cjs", - "tests/injection-blocking-config.test.cjs", - "tests/inline-plan-threshold.test.cjs", - "tests/install-fs-adapter-seam.test.cjs", "tests/install-minimal-hooks.test.cjs", "tests/install-nested-layout.test.cjs", "tests/install-path-detection.test.cjs", "tests/install-regressions.test.cjs", "tests/install-runtime-artifacts.test.cjs", - "tests/install-scope.test.cjs", "tests/install-write-confinement.test.cjs", "tests/install.test.cjs", - "tests/installed-surface-resolver.test.cjs", "tests/installer-migration-antigravity-retire-confighome-artifacts.test.cjs", - "tests/installer-migration-authoring.test.cjs", "tests/installer-migration-config-root-marker.test.cjs", "tests/installer-migration-pi-retire-hooks-dir.test.cjs", "tests/installer-migration-prune-stale-pristine.test.cjs", "tests/installer-migration-rename-gsd-core.test.cjs", - "tests/installer-migration-report.test.cjs", - "tests/installer-migrations-manifest-schema.test.cjs", "tests/installer-migrations.test.cjs", - "tests/intel-command-cutover.test.cjs", "tests/intel.test.cjs", - "tests/inventory-manifest-sync.test.cjs", "tests/inventory-nested-families.test.cjs", "tests/io.test.cjs", "tests/isolation-sentinel.test.cjs", "tests/kilo-upgrades.test.cjs", "tests/kimi-agent-converter.test.cjs", - "tests/kimi-normalize-payload.property.test.cjs", "tests/kimi-upgrades.test.cjs", "tests/kimi-variant-disambiguation.test.cjs", - "tests/learnings.test.cjs", - "tests/legacy-cleanup-config-dir.test.cjs", - "tests/lint-allow-test-rule-refs.test.cjs", - "tests/lint-compiled-artifact-sync.test.cjs", - "tests/lint-docs-command-form.test.cjs", - "tests/lint-frontmatter-scalar-broad-grep.test.cjs", - "tests/lint-hooks-runtime-build-seam.test.cjs", - "tests/lint-legacy-dir-name.test.cjs", - "tests/lint-planning-artifact-writer-drift.test.cjs", - "tests/lint-pr-check-project-dir.test.cjs", - "tests/lint-regression-test-names.test.cjs", - "tests/lint-seam-enforcement.test.cjs", - "tests/lint-skill-deps.test.cjs", - "tests/lint-test-file-count.test.cjs", "tests/lint-workflow-shellcheck-fetch.test.cjs", - "tests/list-seeds.test.cjs", "tests/live-config-guard.test.cjs", - "tests/live-dom-uat.test.cjs", "tests/lockfile-cve-audit.test.cjs", "tests/locking-bugs-1909-1916-1925-1927.test.cjs", "tests/loop-hooks-empty-points-e2e.test.cjs", "tests/loop-hooks-ship-pre-e2e.test.cjs", "tests/loop-hooks-verify-post-e2e.test.cjs", - "tests/loop-render-hooks.test.cjs", - "tests/managed-hooks.test.cjs", - "tests/manifest-version-sync.test.cjs", - "tests/markdown-table.test.cjs", "tests/mcp-catalog.property.test.cjs", "tests/mcp-catalog.test.cjs", "tests/milestone-archive.test.cjs", - "tests/milestone-lock.test.cjs", - "tests/milestone-prefixed-convention.test.cjs", - "tests/milestone-window-drift-guard.test.cjs", "tests/milestone-window-single-owner.test.cjs", - "tests/milestone.test.cjs", - "tests/model-omit-when-inherit-guard.test.cjs", - "tests/model-profiles.test.cjs", "tests/model-resolver.test.cjs", - "tests/mutation-matrix-ratchet.test.cjs", - "tests/mutation-score-ratchet.test.cjs", "tests/mutation-workflow-base-ref.test.cjs", - "tests/mvp-phase-integration.test.cjs", "tests/new-milestone-clear-phases.test.cjs", - "tests/next-decimal-roadmap-scan.test.cjs", - "tests/next-up-clear-order.test.cjs", - "tests/no-bare-gsd-tools-command-position.test.cjs", "tests/no-bare-npm-exec.rule.test.cjs", - "tests/no-dead-sdk-refs.test.cjs", "tests/no-exact-case-env-access.rule.test.cjs", - "tests/no-hardcoded-home-gsd-tools.test.cjs", - "tests/no-hardcoded-tmp.rule.test.cjs", "tests/no-path-literal-in-assert.rule.test.cjs", "tests/no-pending-3212-markers.test.cjs", "tests/no-phantom-issue-refs.test.cjs", "tests/no-posix-mode-bit-assert.rule.test.cjs", "tests/no-private-binary-resolution.rule.test.cjs", - "tests/no-rendered-text-length-assert.rule.test.cjs", - "tests/no-swallowed-precondition.rule.test.cjs", "tests/no-unbounded-dirname-walk.rule.test.cjs", - "tests/no-unbounded-spawn-allowlist.test.cjs", "tests/no-unbounded-spawn.test.cjs", "tests/no-unguarded-nonportable-exec.rule.test.cjs", - "tests/normalize-path-in-content.rule.test.cjs", - "tests/normalize-test-command.test.cjs", "tests/npm-audit-baseline.test.cjs", "tests/npm-integrity-gate.test.cjs", "tests/onboard-command.test.cjs", "tests/opencode-command-dir-plural.test.cjs", - "tests/opencode-permissions.test.cjs", "tests/opencode-plugin-adapter.test.cjs", - "tests/orphan-worktree-detection.test.cjs", - "tests/orphaned-hooks.test.cjs", "tests/overlay-repo-helpers.test.cjs", - "tests/package-name-single-source.test.cjs", "tests/packaging-shipped-scripts-require-only-shipped.test.cjs", - "tests/parallel-dependent-plans.test.cjs", "tests/path-replacement.test.cjs", - "tests/pattern-mapper.test.cjs", - "tests/pattern.test.cjs", "tests/pause-work-context-detection.test.cjs", + "tests/pause-work-improvements.test.cjs", "tests/perf-317-context-monitor-fs.test.cjs", - "tests/phase-command-router.test.cjs", "tests/phase-completion-single-owner.test.cjs", "tests/phase-estimation.test.cjs", - "tests/phase-id-drift-guard.test.cjs", "tests/phase-locator.test.cjs", - "tests/phase-resolution-parity.test.cjs", - "tests/phase-tdd-applicable.test.cjs", "tests/phase.test.cjs", "tests/phase6-capstone-conformance.test.cjs", - "tests/phases-command-router.test.cjs", "tests/pi-config-dir-env-override.test.cjs", - "tests/pi-extension-reachability.test.cjs", - "tests/pick-flag.test.cjs", - "tests/plan-bounce.test.cjs", "tests/plan-count-single-owner.test.cjs", - "tests/plan-phase-drift-guard.test.cjs", - "tests/plan-phase-mvp-flag.test.cjs", "tests/plan-phase-stall-detection.test.cjs", "tests/plan-pre-hook-e2e.test.cjs", "tests/plan-review-convergence.test.cjs", @@ -367,187 +188,85 @@ module.exports = { "tests/planning-inspect.unit.test.cjs", "tests/planning-lock-mkdir-failure-1884.test.cjs", "tests/planning-prompt-drift.test.cjs", - "tests/planning-snapshot-bypass-drift.test.cjs", "tests/planning-snapshot.test.cjs", "tests/planning-workspace.test.cjs", "tests/platform-conformance-tier.test.cjs", "tests/platform-guard.unit.test.cjs", - "tests/playwright-ui-verify.test.cjs", "tests/plugin-manifest.test.cjs", "tests/policy-160-route0-resume.test.cjs", "tests/policy-shell-pinning.test.cjs", "tests/portability-rule-disable-ban.test.cjs", - "tests/portability-vocab-drift.test.cjs", - "tests/post-planning-gaps-2493.test.cjs", "tests/pr-branch-planning-filter.test.cjs", "tests/precommit-alias-drift-hook.test.cjs", - "tests/precondition-element.test.cjs", "tests/prepush-enterprise-email-hook.test.cjs", - "tests/probe-core.test.cjs", "tests/process-seam.test.cjs", "tests/profile-output.test.cjs", "tests/profile-pipeline.test.cjs", "tests/prohibition-enforcement.test.cjs", - "tests/prohibition-probe.verify-tier.test.cjs", - "tests/project-instruction-file-parity.test.cjs", "tests/project-root.test.cjs", - "tests/prompt-budget-cli.test.cjs", - "tests/prune-orphaned-worktrees.test.cjs", - "tests/quick-batch-command-router.test.cjs", "tests/quick-batch.test.cjs", "tests/quick-branching.test.cjs", "tests/quick-research.test.cjs", "tests/quick-review-scope-tip-bound.test.cjs", - "tests/qwen-upgrades.test.cjs", "tests/read-guard.test.cjs", - "tests/read-injection-scanner.property.test.cjs", - "tests/reapply-patches.test.cjs", "tests/reapply-verify-hunks.test.cjs", - "tests/refactor-trigger-cli.test.cjs", - "tests/registry-reviewer-parity.test.cjs", - "tests/release-hotfix-empty-cherry-pick.test.cjs", - "tests/removed-but-needed-lint.test.cjs", - "tests/repo-invariants.test.cjs", "tests/repo-layout.test.cjs", "tests/representative-corpus.test.cjs", "tests/require-fs-op-fallback.rule.test.cjs", "tests/require-full-tmpdir-triad.rule.test.cjs", - "tests/require-issue-link-policy.test.cjs", "tests/require-userprofile-with-home.rule.test.cjs", - "tests/research-cli.test.cjs", - "tests/research-store.test.cjs", - "tests/resolve-execution-dynamic-routing.test.cjs", - "tests/resolver-hoist-guard.test.cjs", "tests/response-language-coverage.test.cjs", "tests/retired-artifact-cleanup.test.cjs", - "tests/reversibility-tagging.test.cjs", "tests/review-build-prompt-optional-sections.test.cjs", "tests/review-default-reviewers-config.test.cjs", - "tests/review-default-reviewers-workflow.test.cjs", - "tests/review-lane-descriptor.test.cjs", - "tests/review-lane-invocation.test.cjs", "tests/review-lane-runner.test.cjs", "tests/review-lane-windows-spawn-resolution.test.cjs", "tests/review-model-config.test.cjs", "tests/review-parallel-lanes.test.cjs", "tests/review-plan-coverage-manifest.test.cjs", - "tests/review-reviewer-instances-config.test.cjs", - "tests/reviewer-config-federation.test.cjs", - "tests/reviewer-docs-parity.test.cjs", - "tests/reviewer-lane-declarations.test.cjs", "tests/reviewer-manifest-body.test.cjs", "tests/reviewer-step-dispatch.test.cjs", - "tests/reviewer-trust-disclosure.test.cjs", "tests/revision-remediation-binding.test.cjs", - "tests/roadmap-mode-field.test.cjs", "tests/roadmap-parser.test.cjs", - "tests/roadmap-phase-fallback.test.cjs", - "tests/roadmap-upgrade.test.cjs", "tests/roadmap.test.cjs", - "tests/roadmapper-granularity.test.cjs", "tests/run-tests-harness.test.cjs", "tests/run-tests-temp-root.test.cjs", "tests/run-with-timeout.test.cjs", - "tests/runtime-artifact-install-plan.test.cjs", - "tests/runtime-artifact-layout-descriptor-drive.test.cjs", - "tests/runtime-artifact-layout-install-profiles.test.cjs", "tests/runtime-artifact-layout-surface.test.cjs", "tests/runtime-artifact-layout.test.cjs", "tests/runtime-converters.test.cjs", "tests/runtime-homes-descriptor-drive.test.cjs", - "tests/runtime-homes.property.test.cjs", "tests/runtime-identity.test.cjs", "tests/runtime-launcher-parity.test.cjs", - "tests/runtime-name-policy.test.cjs", - "tests/safe-resume-gate-anchoring.test.cjs", - "tests/schema-drift.test.cjs", - "tests/sdk-removal-query-family-dispatch.test.cjs", - "tests/section-manifest-init-facts.test.cjs", - "tests/section-manifest.test.cjs", "tests/security.test.cjs", - "tests/settings-integrations.test.cjs", "tests/settings-jsonc.test.cjs", "tests/sh-hook-paths.test.cjs", - "tests/shadow-report.test.cjs", "tests/shared-hooks-dir-resolution.test.cjs", "tests/shell-command-projection-dispatch.test.cjs", "tests/shell-command-projection-path-sep.test.cjs", "tests/ship-notes-wedged-pr.test.cjs", - "tests/shipped-reference-cites.test.cjs", - "tests/skill-frontmatter-contract.test.cjs", "tests/skill-manifest.test.cjs", - "tests/slash-command-namespace.test.cjs", "tests/slug-derivation-drift-guard.test.cjs", - "tests/smart-entry.unit.test.cjs", - "tests/spec-section.test.cjs", - "tests/stale-bake-guard.test.cjs", - "tests/state-command-cutover.test.cjs", - "tests/state-contract.test.cjs", - "tests/state-document.test.cjs", - "tests/state-field-drift.test.cjs", - "tests/state-io.test.cjs", - "tests/state-prune.test.cjs", - "tests/state-rebuild-cli.test.cjs", "tests/state-todos-render.test.cjs", - "tests/state-transition.test.cjs", - "tests/state-write-path-drift-guard.test.cjs", "tests/state.test.cjs", - "tests/stats-mvp-display.test.cjs", - "tests/stats-phase-id-shape.test.cjs", - "tests/subagent-timeout.test.cjs", - "tests/summary-status-blocked-3345.test.cjs", - "tests/surface-md-paths.regression.test.cjs", - "tests/sync-skills-cross-runtime-refuse.test.cjs", - "tests/table-schema-drift-lint.test.cjs", - "tests/tdd-backend-wiring.test.cjs", - "tests/tdd-mode.test.cjs", - "tests/tdd-red-evidence.test.cjs", - "tests/tdd-single-statement.test.cjs", "tests/teams-status.test.cjs", - "tests/template.test.cjs", - "tests/thinking-partner.test.cjs", - "tests/todos-done-rename-guard.test.cjs", "tests/todos-workstream-scope.test.cjs", - "tests/tracer-bullet.test.cjs", - "tests/trae-imperative-reference.test.cjs", - "tests/tsconfig-noemit.test.cjs", - "tests/uat-predicate.test.cjs", - "tests/uat.test.cjs", - "tests/ui-spec-inventory-provenance.test.cjs", - "tests/ultraplan-phase.test.cjs", "tests/unreachable-guard-drift.test.cjs", "tests/unreachable-shell-guard.test.cjs", "tests/unusable-input.test.cjs", - "tests/update-context.test.cjs", "tests/update-custom-backup.test.cjs", - "tests/update-workflow.test.cjs", "tests/user-artifact-staging.test.cjs", - "tests/validate-context.test.cjs", - "tests/validate-registry.test.cjs", "tests/verification-status.test.cjs", "tests/verify-archive-dirs-live-path.test.cjs", "tests/verify-command-grounding.test.cjs", - "tests/verify-health.test.cjs", "tests/verify.test.cjs", - "tests/vscode-browser-no-node-api.test.cjs", - "tests/vscode-extension-reachability.test.cjs", - "tests/windows-robustness.test.cjs", - "tests/windsurf-conversion.test.cjs", "tests/windsurf-hooks-bridge.test.cjs", - "tests/windsurf-install.test.cjs", - "tests/workflow-compat.test.cjs", - "tests/workflow-fragments.test.cjs", "tests/workflow-guard.test.cjs", - "tests/workflow-maintainer-skip.test.cjs", "tests/workflow-shell-pinning.test.cjs", - "tests/workspace.test.cjs", "tests/workstream-inventory.test.cjs", "tests/workstream-scoped-paths.test.cjs", "tests/workstream.test.cjs", - "tests/worktree-base-ref.test.cjs", - "tests/worktree-baseref-install.test.cjs", "tests/worktree-cleanup.test.cjs", - "tests/worktree-safety-reap.test.cjs", "tests/worktree-safety.test.cjs", "tests/worktree.test.cjs", ], diff --git a/scripts/lint-docs-guard-registration.exempt-baseline.cjs b/scripts/lint-docs-guard-registration.exempt-baseline.cjs index dff484b6e..51bb3ed74 100644 --- a/scripts/lint-docs-guard-registration.exempt-baseline.cjs +++ b/scripts/lint-docs-guard-registration.exempt-baseline.cjs @@ -125,8 +125,12 @@ const DOCS_GUARD_EXEMPT_DOCS_PATHS = { 'docs/how-to/some-unrelated-guide.md', 'docs/how-to/x.md', 'docs/some-unrelated-file.md', 'docs/totally-unrelated.md', ], + // #4641: cites docs/adr/4641-windows-selector-consolidation.md in an + // explanatory comment describing why the retired `windows` scope must not + // be restored; the file never reads that (or any) docs/ file. 'ci-test-scope.test.cjs': [ - 'docs/a.md', 'docs/adr', 'docs/adr/22-plan-drift-guard.md', 'docs/how-to/configure-model-profiles.md', + 'docs/a.md', 'docs/adr', 'docs/adr/22-plan-drift-guard.md', + 'docs/adr/4641-windows-selector-consolidation.md', 'docs/how-to/configure-model-profiles.md', 'docs/installer-migrations.md', 'docs/ja-JP', 'docs/ja-JP/USAGE.md', 'docs/usage.md', 'docs/x.md', ], 'cline-install.test.cjs': ['docs/guide.md'], diff --git a/src/external-descriptor-trust.cts b/src/external-descriptor-trust.cts index 5557fa2de..4bd346361 100644 --- a/src/external-descriptor-trust.cts +++ b/src/external-descriptor-trust.cts @@ -37,14 +37,29 @@ import path from 'node:path'; * currently keeps every caller of this function's callers symlink-safe: they * reject symlinks upstream, before a target ever reaches a lexical-only check * like this one. + * + * `opts.pathImpl` (default: the ambient `path` module) lets a caller inject + * `path.win32` or `path.posix`. This is security-relevant: the win32 branch + * (drive letters, UNC paths, `\` separator) is otherwise only reachable by + * actually running this process on a Windows host, so without injection a + * win32-specific confinement escape would be unverified on every other + * platform. This mirrors the platform-injection seam already used elsewhere + * in this repo, e.g. src/shell-command-projection.cts's `opts.platform` + * (#4641). All existing 2-arg callers are unaffected: the default resolves to + * the ambient `path`, preserving byte-identical behaviour. */ -export function isPathConfined(target: string, root: string): boolean { +export function isPathConfined( + target: string, + root: string, + opts: { pathImpl?: typeof path } = {}, +): boolean { if (typeof target !== 'string' || typeof root !== 'string' || target.length === 0 || root.length === 0) { return false; } - const rootResolved = path.resolve(root); - const targetResolved = path.resolve(root, target); - const prefix = rootResolved + path.sep; + const p = opts.pathImpl ?? path; + const rootResolved = p.resolve(root); + const targetResolved = p.resolve(root, target); + const prefix = rootResolved + p.sep; return targetResolved === rootResolved || targetResolved.startsWith(prefix); } diff --git a/tests/ci-full-lane-sharding.test.cjs b/tests/ci-full-lane-sharding.test.cjs index 1c8acccb6..4ff19f695 100644 --- a/tests/ci-full-lane-sharding.test.cjs +++ b/tests/ci-full-lane-sharding.test.cjs @@ -73,14 +73,65 @@ function isCompleteShardSet(specs) { return seen.size === total && [...seen].every((i) => i >= 1 && i <= total); } +test('the test job has no windows lane; test-conformance is the sole Windows selector (#4641)', () => { + const workflow = loadWorkflow('test.yml'); + const testInclude = workflow.jobs.test.strategy.matrix.include; + const testWindowsEntries = testInclude.filter((e) => e.os === 'windows-latest'); + assert.equal( + testWindowsEntries.length, 0, + `the \`test\` job's matrix.include still has ${testWindowsEntries.length} windows-latest ` + + `entr${testWindowsEntries.length === 1 ? 'y' : 'ies'} (${JSON.stringify(testWindowsEntries)}). ` + + '#4641 deletes the `test` job\'s windows lane; test-conformance is the sole Windows selector.', + ); + + const conformanceInclude = workflow.jobs['test-conformance'].strategy.matrix.include; + const conformanceWindowsEntries = conformanceInclude.filter((e) => e.os === 'windows-latest'); + const conformanceMacosEntries = conformanceInclude.filter((e) => e.os === 'macos-latest'); + + // Derived, not pinned: windows-latest must be a non-empty, COMPLETE shard + // set (same denominator, numerators 1..N — see isCompleteShardSet above), + // whatever N the workflow currently declares. A bare `.length === 3` here + // would break the moment the workflow is rebalanced to a different shard + // count without a wiring regression, exactly the shape of bug this sweep + // exists to remove (the sibling macOS-tier-count magic number already did + // this once). + assert.ok( + conformanceWindowsEntries.length > 0, + `test-conformance declares no windows-latest entries: ${JSON.stringify(conformanceInclude)}`, + ); + assert.ok( + isCompleteShardSet(conformanceWindowsEntries.map((e) => e.shard)), + 'the windows-latest entries in test-conformance are not a complete shard set: ' + + JSON.stringify(conformanceWindowsEntries), + ); + + // macos-latest is, by design, a single UNSHARDED entry (see the workflow's + // "macos-latest stays unsharded; it has real headroom" comment above the + // `test-conformance` job) — unlike the windows/full shard counts, this "1" + // is not a fact about the live tree that grows with the suite, it is the + // structural claim the test exists to pin: more than one entry here would + // silently duplicate full macOS runs, and a `shard` key would mean the + // workflow started partitioning a lane the run-tests.cjs invocation below + // does not expect to be partitioned. + assert.equal( + conformanceMacosEntries.length, 1, + `expected a single unsharded macos-latest entry in test-conformance, got ${conformanceMacosEntries.length}: ` + + JSON.stringify(conformanceMacosEntries), + ); + assert.equal( + conformanceMacosEntries[0] && conformanceMacosEntries[0].shard, undefined, + 'the macos-latest entry in test-conformance declares a shard, contradicting the ' + + `workflow's "macos-latest stays unsharded" design: ${JSON.stringify(conformanceMacosEntries)}`, + ); +}); + test('the full test lane is sharded and complete (#2952)', async (t) => { const workflow = loadWorkflow('test.yml'); const include = workflow.jobs.test.strategy.matrix.include; const fullLanes = include.filter((e) => e.scope === 'full'); - const windowsLanes = include.filter((e) => e.scope === 'windows'); - // The only lane in this job with no shard is `scope: targeted` — the fast, - // single-runner default lane. Both `full` and `windows` are sharded. - const shardedScopes = { full: fullLanes, windows: windowsLanes }; + // #4641: the `test` job's `scope: windows` lane is deleted — there is no + // longer a second sharded scope in this job to pin alongside `full`. + const shardedScopes = { full: fullLanes }; for (const [scope, lanes] of Object.entries(shardedScopes)) { await t.test(`the \`scope: ${scope}\` lane is actually sharded, not a single runner`, () => { @@ -112,12 +163,13 @@ test('the full test lane is sharded and complete (#2952)', async (t) => { } await t.test('no other lane is sharded', () => { - for (const lane of include.filter((e) => e.scope !== 'full' && e.scope !== 'windows')) { + // #4641: the `test` job's `scope: windows` lane is deleted, so `full` is + // the only sharded scope left in this job's matrix. + for (const lane of include.filter((e) => e.scope !== 'full')) { assert.equal( lane.shard, undefined, - `lane ${JSON.stringify(lane)} declares a shard but is neither \`scope: full\` ` - + 'nor `scope: windows` — the targeted lane runs a selected file list, not ' - + 'a partition.', + `lane ${JSON.stringify(lane)} declares a shard but is not \`scope: full\` — ` + + 'the targeted lane runs a selected file list, not a partition.', ); } }); diff --git a/tests/ci-test-scope.test.cjs b/tests/ci-test-scope.test.cjs index 089be3c15..7152071f4 100644 --- a/tests/ci-test-scope.test.cjs +++ b/tests/ci-test-scope.test.cjs @@ -56,7 +56,11 @@ describe('ci-test-scope.cjs', () => { assert.strictEqual(result.full_matrix, true); assert.ok(result.targeted_tests.includes('tests/workflow-shell-pinning.test.cjs')); assert.ok(result.targeted_tests.includes('tests/release-tarball-smoke-workflow.test.cjs')); - assert.ok(result.windows_tests.includes('tests/workflow-shell-pinning.test.cjs')); + // #4641: the `test` job's windows lane is deleted; full_matrix (already + // asserted true above) is the only Windows signal left, and classify() + // emits no windows_tests key at all. + assert.strictEqual(Object.hasOwn(result, 'windows_tests'), false, + `expected no windows_tests property, got keys: ${JSON.stringify(Object.keys(result))}`); }); test('pipeline workflow (install-smoke.yml) — product_changed true, full_matrix true', () => { @@ -307,18 +311,21 @@ describe('ci-test-scope superset invariant (#494, rescinded by #4421)', () => { `expected full_matrix=true for a tests/**-only change (rescinded #494 carve-out, see #4421), got: ${JSON.stringify(result)}`); assert.ok(result.targeted_tests.includes('tests/perf-317-context-monitor-fs.test.cjs'), `expected the changed test in targeted_tests, got: ${JSON.stringify(result.targeted_tests)}`); - assert.ok(result.windows_tests.includes('tests/perf-317-context-monitor-fs.test.cjs'), - `expected the changed test in windows_tests, got: ${JSON.stringify(result.windows_tests)}`); + // #4641: the `test` job's windows lane is deleted; full_matrix (already + // asserted true above) is the only Windows signal left. + assert.strictEqual(Object.hasOwn(result, 'windows_tests'), false, + `expected no windows_tests property, got keys: ${JSON.stringify(Object.keys(result))}`); }); - test('A2: a changed test file with no windows hint still joins the windows lane and triggers full_matrix', () => { - // commands.test.cjs matches none of the WINDOWS_HINTS substrings — the - // unconditional changed-test → windows lane rule must include it anyway. + test('A2: a changed test file with no windows hint still triggers full_matrix, with no side lane (#4641)', () => { + // commands.test.cjs matches none of the WINDOWS_HINTS substrings — under + // the old #494/#4421 behavior it still joined the windows lane. Post-#4641 + // there is no windows lane to join; full_matrix is the sole signal. const result = scopeFor(['tests/commands.test.cjs']); assert.strictEqual(result.full_matrix, true, `expected full_matrix=true (rescinded #494 carve-out, see #4421), got: ${JSON.stringify(result)}`); - assert.ok(result.windows_tests.includes('tests/commands.test.cjs'), - `expected hint-less changed test in windows_tests, got: ${JSON.stringify(result.windows_tests)}`); + assert.strictEqual(Object.hasOwn(result, 'windows_tests'), false, + `expected no windows_tests property, got keys: ${JSON.stringify(Object.keys(result))}`); }); test('A3: a deleted/nonexistent test path falls back to the unit token; full_matrix now depends on conformance-tier reachability (#4592)', () => { @@ -1029,6 +1036,66 @@ describe('#4592 reachability-based full_matrix classifier', () => { }); }); +describe('#4641 windows lane removal: test-conformance becomes the sole Windows selector', () => { + const { classify } = require('../scripts/ci-test-scope.cjs'); + + // Case C: a changed test file that is not tagged in Phase 2's + // CONFORMANCE_TIER_FILES no longer needs to force a Windows run of its own — + // once the `test` job's `scope: windows` lane is deleted, the only Windows + // signal left is full_matrix (routed to test-conformance). + test('a non-tier test file no longer forces a Windows run (#4641)', () => { + const { CONFORMANCE_TIER_FILES } = require('../scripts/lib/platform-conformance-tier.generated.cjs'); + const file = 'tests/some-brand-new-non-tier-4641.test.cjs'; + assert.ok(!CONFORMANCE_TIER_FILES.includes(file), 'precondition: fixture path must not be tier-tagged'); + const result = classify([file]); + assert.strictEqual(result.full_matrix, false, + `expected full_matrix=false for a non-conformance-tier test file, got: ${JSON.stringify(result)}`); + }); + + // Case D: post-#4641, classify() must not emit a `windows_tests` key at all — + // "absent" and "empty array" are different claims, and only the former + // matches a workflow with no windows_tests output to consume. + test('windows_tests is gone, not merely empty (#4641)', () => { + const result = classify(['tests/some-brand-new-non-tier-4641.test.cjs']); + assert.strictEqual( + Object.hasOwn(result, 'windows_tests'), false, + `expected classify() to return no windows_tests property at all, got keys: ${JSON.stringify(Object.keys(result))}`, + ); + }); + + // Case E: the one non-redundant residue of the deleted windows lane — a + // RULE whose tests[] includes a filename matching isWindowsHint — must be + // preserved by escalating to full_matrix instead of a side lane. + // 'portability lint rules (ADR-1703)' pulls in + // tests/no-path-literal-in-assert.rule.test.cjs and + // tests/normalize-path-in-content.rule.test.cjs, both matching the 'path' + // hint in WINDOWS_HINTS, and today carries no fullMatrix of its own. + test('RULE-pulled windows-hint tests force full_matrix, not a side lane (#4641)', () => { + const file = 'eslint-rules/no-path-literal-in-assert.cjs'; + const result = classify([file]); + assert.strictEqual(result.full_matrix, true, + `expected full_matrix=true because the matched rule pulls in a windows-hint test, got: ${JSON.stringify(result)}`); + assert.ok( + result.reasons.some(r => r.startsWith(`${file}: portability lint rules (ADR-1703)`)), + `expected reasons to name the windows-hint rule, got: ${JSON.stringify(result.reasons)}`, + ); + }); + + // Case F: MUST-PASS pin, already covered by the "full_matrix true for a + // conformance-tier test file" test in the '#4592 reachability-based + // full_matrix classifier' describe block above — a changed test file that + // IS in CONFORMANCE_TIER_FILES still yields full_matrix=true. Restated here + // so the #4641 removal's regression surface is pinned in one place too. + test('a conformance-tier test file still yields full_matrix=true (#4641 pin)', () => { + const { CONFORMANCE_TIER_FILES } = require('../scripts/lib/platform-conformance-tier.generated.cjs'); + assert.ok(CONFORMANCE_TIER_FILES.length > 0, 'precondition: committed tier list must be non-empty'); + const file = CONFORMANCE_TIER_FILES[0]; + const result = classify([file]); + assert.strictEqual(result.full_matrix, true, + `expected full_matrix=true for conformance-tier file ${file}, got: ${JSON.stringify(result)}`); + }); +}); + // ──────────────────────────────────────────────────────────────────────── // Folded from tests/bug-641-files-from-suite-token.test.cjs — consolidation epic #1969 (B6 #1975) // ──────────────────────────────────────────────────────────────────────── @@ -1250,7 +1317,7 @@ describe('bug #1329 — ci-prepare-test-scope fallback never emits a deleted fil fs.writeFileSync(path.join(tmpDir, f), PASS_BODY, 'utf8'); } - const lines = resolveSelection({ scope: 'targeted', targeted: '', windows: '', root: tmpDir }); + const lines = resolveSelection({ scope: 'targeted', targeted: '', root: tmpDir }); assert.ok(!lines.includes(absent), `absent file "${absent}" must be filtered out, got: ${lines.join(', ')}`); for (const f of present) { @@ -1260,17 +1327,30 @@ describe('bug #1329 — ci-prepare-test-scope fallback never emits a deleted fil test('empty detection with no surviving fallback files falls back to the unit sentinel', () => { // tmpDir/tests exists but contains none of the FALLBACK files. - const lines = resolveSelection({ scope: 'windows', targeted: '', windows: '', root: tmpDir }); + // #4641: this used to run under scope: 'windows'; that scope was retired + // with the windows CI lane. Re-pointed at 'targeted' (still empty-detected) + // to keep the fallback-sentinel contract covered. + const lines = resolveSelection({ scope: 'targeted', targeted: '', root: tmpDir }); assert.deepStrictEqual(lines, [FALLBACK_SENTINEL]); }); + test('resolveSelection rejects the retired windows scope (#4641)', () => { + // #4641: the `test` job's `scope: windows` lane was deleted (see + // docs/adr/4641-windows-selector-consolidation.md). Do not restore this + // scope — resolveSelection must now fail loudly for it rather than + // silently falling back. + assert.throws( + () => resolveSelection({ scope: 'windows', targeted: '', root: tmpDir }), + /Unknown test scope: windows/, + ); + }); + test('detected list passes through verbatim — files and suite sentinels preserved, not existence-filtered', () => { // The detected list is already filtered by affected-tests-lib and may carry // a suite sentinel; ci-prepare-test-scope must not touch it. const lines = resolveSelection({ scope: 'targeted', targeted: 'tests/does-not-exist.test.cjs unit', - windows: '', root: tmpDir, }); assert.deepStrictEqual(lines, ['tests/does-not-exist.test.cjs', 'unit']); @@ -1288,7 +1368,7 @@ describe('bug #1329 — ci-prepare-test-scope fallback never emits a deleted fil [path.join(REPO_ROOT, 'scripts', 'ci-prepare-test-scope.cjs')], { cwd: tmpDir, - env: { ...process.env, TEST_SCOPE: 'targeted', TARGETED_TESTS: '', WINDOWS_TESTS: '' }, + env: { ...process.env, TEST_SCOPE: 'targeted', TARGETED_TESTS: '' }, timeoutMs: PROBE_TIMEOUT_MS, }, ); diff --git a/tests/external-descriptor-confinement.test.cjs b/tests/external-descriptor-confinement.test.cjs index 913bdb6c9..106f08087 100644 --- a/tests/external-descriptor-confinement.test.cjs +++ b/tests/external-descriptor-confinement.test.cjs @@ -25,6 +25,53 @@ test('isPathConfined: confined paths are true, escapes are false', () => { assert.ok(!isPathConfined('skills', ''), 'empty root is NOT confined'); }); +test('isPathConfined: win32 injection — confined and escape cases', () => { + const win32 = path.win32; + const root = 'C:\\Users\\me\\.gsd'; + assert.ok( + isPathConfined('sub\\file.md', root, { pathImpl: win32 }), + 'win32 target under root is confined', + ); + assert.ok( + !isPathConfined('D:\\evil', root, { pathImpl: win32 }), + 'a different drive letter is NOT confined', + ); + assert.ok( + !isPathConfined('C:\\Windows\\system32', root, { pathImpl: win32 }), + 'an absolute path on the same drive outside root is NOT confined', + ); + assert.ok( + !isPathConfined('..\\..\\evil', root, { pathImpl: win32 }), + 'a backslash traversal is NOT confined', + ); + assert.ok( + !isPathConfined('\\\\server\\share\\x', root, { pathImpl: win32 }), + 'a UNC path is NOT confined', + ); + assert.ok( + !isPathConfined('../../x', root, { pathImpl: win32 }), + 'a forward-slash traversal is NOT confined (win32 accepts / too)', + ); +}); + +test('isPathConfined: win32 prefix-boundary — sibling with root as a string prefix is refused', () => { + const win32 = path.win32; + const root = 'C:\\Users\\me\\.gsd'; + assert.ok( + !isPathConfined('..\\.gsdEVIL', root, { pathImpl: win32 }), + 'C:\\Users\\me\\.gsdEVIL must be refused despite sharing the "C:\\Users\\me\\.gsd" string prefix', + ); +}); + +test('isPathConfined: posix prefix-boundary — sibling with root as a string prefix is refused', () => { + const posix = path.posix; + const root = '/home/me/.gsd'; + assert.ok( + !isPathConfined('../.gsdEVIL', root, { pathImpl: posix }), + '/home/me/.gsdEVIL must be refused despite sharing the "/home/me/.gsd" string prefix', + ); +}); + test('assertDescriptorConfined: a benign descriptor (all destSubpaths under configHome) passes', () => { const desc = { id: 'community-host', diff --git a/tests/platform-conformance-tier.test.cjs b/tests/platform-conformance-tier.test.cjs index 654b5bdda..c14b51452 100644 --- a/tests/platform-conformance-tier.test.cjs +++ b/tests/platform-conformance-tier.test.cjs @@ -28,6 +28,10 @@ const { NOISY_FOR_SOURCE_REACHABILITY, classifyMacosContent, classifyMacosTree, + CATEGORIES, + MACOS_CATEGORIES, + walkTestFiles, + ALWAYS_REAL_OS, } = require('../scripts/gen-platform-conformance-tier.cjs'); const ROOT = path.resolve(__dirname, '..'); @@ -35,6 +39,21 @@ const SCRIPT = path.join(ROOT, 'scripts', 'gen-platform-conformance-tier.cjs'); const GENERATED_PATH = path.join(ROOT, 'scripts', 'lib', 'platform-conformance-tier.generated.cjs'); const MACOS_GENERATED_PATH = path.join(ROOT, 'scripts', 'lib', 'macos-conformance-tier.generated.cjs'); +// Policy ceilings, not derived facts — a bound like this has to be a number +// somewhere, so it is hoisted here once (module scope, shared by every case +// below that needs it) rather than left as a bare literal inside an +// assertion. Measured at authoring time (2026-09-11): the Windows tier sat at +// 264/931 eligible unit-suite files (~28.4%), the macOS tier at 197/931 +// (~21.2%). Each ceiling below leaves headroom over that measurement — enough +// to absorb ordinary suite growth (new test files that happen to touch a real +// platform signal) without going so loose that a regression toward +// re-matching a removed house idiom (process-seam calls, path-call-plus- +// slash-literal) would slip back under the ceiling undetected. If the +// measured ratio moves, update the ratio in this comment and re-justify the +// ceiling — do not just raise the number to make a red test green. +const WINDOWS_TIER_RATIO_CEILING = 0.33; +const MACOS_TIER_RATIO_CEILING = 0.25; + // ─── Rows 1-15: classifyContent, pure fixtures ──────────────────────────────── describe('classifyContent — happy-path signals', () => { @@ -88,16 +107,25 @@ describe('classifyContent — happy-path signals', () => { } }); - test('flags process-seam subprocess helpers', () => { + // Replaces the former 'flags process-seam subprocess helpers' case: that + // category ('process-seam-subprocess') was removed outright from CATEGORIES + // (#4641 — it matched the house test idiom of calling the process seam at + // all, not a genuine platform signal). Its narrower, evidence-backed + // replacement is 'shell-interpreter-spawn', which keys on a REAL shell + // binary name passed as the process-seam helpers' `interpreter` option + // (tests/helpers/process-seam.cjs's `runHook`/`runHookSeam`) — genuinely + // platform-dependent (bash/zsh/cmd availability, quoting, output parsing + // all differ across OSes), unlike the removed category's over-broad "any + // process-seam call" signal. + test('flags shell-interpreter-spawn (real interpreter option on a process-seam helper)', () => { for (const fixture of [ - "runNode(['--check'])", - "runGit(['status'])", - "runHook(HOOK_PATH, [])", - "runGsdTools(['state', 'show'])", + "runHook(HOOK_PATH, [], { interpreter: 'bash' })", + "runHookSeam(HOOK_PATH, [], { interpreter: 'zsh' })", + "runHook(HOOK_PATH, [], { interpreter: 'cmd' })", ]) { const { needsRealOs, signals } = classifyContent(fixture); assert.equal(needsRealOs, true, fixture); - assert.ok(signals.includes('process-seam-subprocess'), fixture); + assert.ok(signals.includes('shell-interpreter-spawn'), fixture); } }); @@ -114,12 +142,18 @@ describe('classifyContent — happy-path signals', () => { assert.ok(signals.includes('symlink-keyword')); }); - test('flags hardcoded path literal vs path.* call', () => { - const fixture = "const p = path.join(root, 'x');\nassert.equal(rendered, '/etc/passwd');"; - const { needsRealOs, signals } = classifyContent(fixture); - assert.equal(needsRealOs, true); - assert.ok(signals.includes('hardcoded-path-vs-path-call')); - }); + // The former 'flags hardcoded path literal vs path.* call' case asserted + // the 'hardcoded-path-vs-path-call' category, which #4641 removed outright + // from CATEGORIES (measured to be, alongside process-seam-subprocess, the + // largest driver of Windows-tier over-inclusion — a universal Node + // test-suite idiom, not a platform signal). Unlike process-seam-subprocess + // above, this category has no narrower evidence-backed replacement: no + // still-existing CATEGORIES entry keys on "a hardcoded path literal + // alongside a path.* call". Every other CATEGORIES entry already has + // dedicated happy-path coverage elsewhere in this describe block, so there + // is genuinely nothing left for a rewritten fixture here to assert; the + // case is retired rather than kept as dead weight around a deleted + // detector. }); describe('classifyContent — negative / hostile inputs', () => { @@ -154,13 +188,23 @@ describe('classifyContent — negative / hostile inputs', () => { // ─── Row 15 (#4592): NOISY_FOR_SOURCE_REACHABILITY drift guard ──────────────── describe('NOISY_FOR_SOURCE_REACHABILITY (#4592)', () => { - test('NOISY_FOR_SOURCE_REACHABILITY exports exactly the two noisy categories', () => { + // #4641 removed 'hardcoded-path-vs-path-call' from CATEGORIES outright (it + // was the single largest driver of Windows-tier over-inclusion, a house + // test idiom rather than a genuine platform signal) — not merely from this + // exemption set. NOISY_FOR_SOURCE_REACHABILITY therefore now holds exactly + // one member, 'symlink-keyword': still precise enough for test-file + // classification but too noisy for source reachability. + test('NOISY_FOR_SOURCE_REACHABILITY exports exactly the one remaining noisy category', () => { assert.ok(NOISY_FOR_SOURCE_REACHABILITY instanceof Set, `expected a Set, got: ${typeof NOISY_FOR_SOURCE_REACHABILITY}`); assert.deepEqual( [...NOISY_FOR_SOURCE_REACHABILITY].sort(), - ['hardcoded-path-vs-path-call', 'symlink-keyword'].sort(), - `expected exactly the two named noisy categories, got: ${JSON.stringify([...NOISY_FOR_SOURCE_REACHABILITY])}`, + ['symlink-keyword'], + `expected exactly the one remaining noisy category, got: ${JSON.stringify([...NOISY_FOR_SOURCE_REACHABILITY])}`, + ); + assert.ok( + !CATEGORIES.map((c) => c.name).includes('hardcoded-path-vs-path-call'), + 'hardcoded-path-vs-path-call was removed from CATEGORIES outright (#4641), not merely exempted here', ); }); }); @@ -283,13 +327,19 @@ describe('gen-platform-conformance-tier.cjs — real repo tree (regression)', () fresh = classifyTree(realTestsDir); }, 'a full sweep of the real tests/ tree must complete without throwing'); - // Measured 546/952 at authoring time (#4591, post suite-exclusion fix) — - // the range below is a sanity ballpark with headroom for organic - // test-suite growth in either direction, not a brittle exact-match on - // that literal. + // Post-#4641 the tier is a ratio-bounded MINORITY of eligible unit-suite + // files (WINDOWS_TIER_RATIO_CEILING, same policy ceiling the #4641 block + // below enforces), not a brittle absolute-count range against a literal + // that goes stale every time the category set or the suite's file count + // changes (a hardcoded exact-count sanity range already broke once in + // this PR). Derived from the live tree, not a hardcoded number. + const { suiteOf } = require('../scripts/lib/suite-detection.cjs'); + const eligibleCount = walkTestFiles(realTestsDir).filter((absPath) => suiteOf(absPath) === null).length; + const ratio = fresh.files.length / eligibleCount; assert.ok( - fresh.files.length >= 450 && fresh.files.length <= 700, - `expected a real, current, sanity-checked count in [450, 700], got ${fresh.files.length}`, + ratio > 0 && ratio <= WINDOWS_TIER_RATIO_CEILING, + `expected a nonzero conformance tier within the #4641 ratio ceiling (<=${WINDOWS_TIER_RATIO_CEILING * 100}%), ` + + `got ${fresh.files.length}/${eligibleCount} (${(ratio * 100).toFixed(1)}%)`, ); delete require.cache[require.resolve(GENERATED_PATH)]; @@ -474,11 +524,19 @@ describe('gen-platform-conformance-tier.cjs — real repo tree, macOS target (re fresh = classifyMacosTree(realTestsDir); }, 'a full sweep of the real tests/ tree must complete without throwing'); - // Measured 196/930 at authoring time (#4593 design doc). Sanity ballpark - // with headroom for organic test-suite growth, not a brittle exact match. + // Post-#4641 the tier is a ratio-bounded MINORITY of eligible unit-suite + // files (MACOS_TIER_RATIO_CEILING, same policy ceiling the #4641 block + // below enforces), not a brittle absolute-count range against a literal + // that goes stale every time the category set or the suite's file count + // changes (a hardcoded exact-count sanity range already broke once in + // this PR). Derived from the live tree, not a hardcoded number. + const { suiteOf } = require('../scripts/lib/suite-detection.cjs'); + const eligibleCount = walkTestFiles(realTestsDir).filter((absPath) => suiteOf(absPath) === null).length; + const ratio = fresh.files.length / eligibleCount; assert.ok( - fresh.files.length >= 100 && fresh.files.length <= 350, - `expected a real, current, sanity-checked count in [100, 350], got ${fresh.files.length}`, + ratio > 0 && ratio <= MACOS_TIER_RATIO_CEILING, + `expected a nonzero macOS conformance tier within the #4641 ratio ceiling (<=${MACOS_TIER_RATIO_CEILING * 100}%), ` + + `got ${fresh.files.length}/${eligibleCount} (${(ratio * 100).toFixed(1)}%)`, ); delete require.cache[require.resolve(MACOS_GENERATED_PATH)]; @@ -511,3 +569,280 @@ describe('gen-platform-conformance-tier.cjs — real repo tree, macOS target (re ); }); }); + +// ─── #4641: narrow the Windows conformance tier away from house-idiom noise ─── + +describe('conformance tier narrowing (#4641)', () => { + // WINDOWS_TIER_RATIO_CEILING / MACOS_TIER_RATIO_CEILING are hoisted to + // module scope above (shared with the real-repo-tree regression case + // further down, which needs the same ceiling rather than a second + // independently-drifting copy of it). + + test('conformance tier stays a tier, not the suite (#4641)', () => { + const realTestsDir = path.join(ROOT, 'tests'); + const { suiteOf } = require('../scripts/lib/suite-detection.cjs'); + + const absoluteFiles = walkTestFiles(realTestsDir); + const eligibleFiles = absoluteFiles.filter((absPath) => suiteOf(absPath) === null); + const eligibleCount = eligibleFiles.length; + + let tierCount = 0; + for (const absPath of eligibleFiles) { + const content = fs.readFileSync(absPath, 'utf8'); + const { needsRealOs } = classifyContent(content); + if (needsRealOs) tierCount++; + } + + const ratio = tierCount / eligibleCount; + assert.ok( + ratio <= WINDOWS_TIER_RATIO_CEILING, + `expected the conformance tier to be at most ${WINDOWS_TIER_RATIO_CEILING * 100}% of eligible unit-suite files, ` + + `got ${tierCount}/${eligibleCount} (${(ratio * 100).toFixed(1)}%)`, + ); + }); + + test('macos conformance tier stays within its evidence-backed ceiling (#4641)', () => { + const realTestsDir = path.join(ROOT, 'tests'); + const { suiteOf } = require('../scripts/lib/suite-detection.cjs'); + + const absoluteFiles = walkTestFiles(realTestsDir); + const eligibleFiles = absoluteFiles.filter((absPath) => suiteOf(absPath) === null); + const eligibleCount = eligibleFiles.length; + + let tierCount = 0; + for (const absPath of eligibleFiles) { + const content = fs.readFileSync(absPath, 'utf8'); + const { needsRealOs } = classifyMacosContent(content); + if (needsRealOs) tierCount++; + } + + const ratio = tierCount / eligibleCount; + assert.ok( + ratio <= MACOS_TIER_RATIO_CEILING, + `expected the macOS conformance tier to be at most ${MACOS_TIER_RATIO_CEILING * 100}% of eligible unit-suite files, ` + + `got ${tierCount}/${eligibleCount} (${(ratio * 100).toFixed(1)}%)`, + ); + }); + + test('seam-helper calls are not a platform signal (#4641)', () => { + for (const call of ['runNode(', 'runGit(', 'runHook(', 'runGsdTools(', 'gitOrThrow(']) { + const fixture = `${call}args);\nassert.equal(result.exitCode, 0);\n`; + const { needsRealOs, signals } = classifyContent(fixture); + assert.equal(needsRealOs, false, call); + assert.deepEqual(signals, [], call); + } + }); + + test('a path call plus a slash literal is not a platform signal (#4641)', () => { + const fixture = "const p = path.join(dir, 'sub');\nassert.equal(p, '/tmp/fixture');\n"; + const { needsRealOs, signals } = classifyContent(fixture); + assert.equal(needsRealOs, false); + assert.deepEqual(signals, []); + }); + + test('the two house-idiom detectors are gone (#4641)', () => { + const names = CATEGORIES.map((c) => c.name); + assert.ok(!names.includes('process-seam-subprocess'), `CATEGORIES still contains process-seam-subprocess: ${JSON.stringify(names)}`); + assert.ok(!names.includes('hardcoded-path-vs-path-call'), `CATEGORIES still contains hardcoded-path-vs-path-call: ${JSON.stringify(names)}`); + }); + + test('genuine platform-conditional content still classifies IN (#4641)', () => { + const { needsRealOs, signals } = classifyContent("if (process.platform === 'win32') { doThing(); }"); + assert.equal(needsRealOs, true); + assert.ok(signals.includes('process-platform')); + }); + + test('seam-BYPASSING spawn still classifies IN (#4641)', () => { + const fixture = "const { spawnSync } = require('node:child_process');\nspawnSync('ls', []);"; + const { needsRealOs, signals } = classifyContent(fixture); + assert.equal(needsRealOs, true); + assert.ok(signals.includes('raw-child-process')); + }); + + test('chmodSync content still classifies IN via chmod-mode-bit (#4641)', () => { + const { needsRealOs, signals } = classifyContent('fs.chmodSync(target, mode);'); + assert.equal(needsRealOs, true); + assert.ok(signals.includes('chmod-mode-bit')); + }); + + test('symlinkSync content still classifies IN via symlink-keyword (#4641)', () => { + const { needsRealOs, signals } = classifyContent('fs.symlinkSync(target, link);'); + assert.equal(needsRealOs, true); + assert.ok(signals.includes('symlink-keyword')); + }); + + test('macOS signal set is untouched by the Windows narrowing (#4641)', () => { + assert.deepEqual( + MACOS_CATEGORIES.map((c) => c.name), + ['darwin-literal', 'zsh-dispatch', 'case-sensitivity', 'chmod-mode-bit', 'symlink-keyword'], + ); + }); + + test('macOS generated tier matches a fresh classification of the live tests/ tree (#4641)', () => { + const realTestsDir = path.join(ROOT, 'tests'); + const fresh = classifyMacosTree(realTestsDir); + + delete require.cache[require.resolve(MACOS_GENERATED_PATH)]; + const { MACOS_CONFORMANCE_TIER_FILES } = require(MACOS_GENERATED_PATH); + + assert.deepEqual( + MACOS_CONFORMANCE_TIER_FILES.slice().sort(), + fresh.files.slice().sort(), + 'the committed macOS generated file must be fresh — run ' + + '`node scripts/gen-platform-conformance-tier.cjs --target macos --write`', + ); + }); + + test('named probe files still classify IN on a genuine platform signal, not by filename (#4641)', () => { + // These three files are examples, not the assertion. Each was picked + // because it probes a DIFFERENT genuine platform-signal family, so the + // three together exercise the narrowed classifier's breadth, not just its + // presence: shell-command-projection-dispatch pulls in raw + // spawn/shell-dispatch signals, review-lane-windows-spawn-resolution pulls + // in Windows path/spawn-resolution signals, and prohibition-enforcement + // pulls in process.platform/os.platform conditionals. Asserting by + // FILENAME membership is exactly the classifier bug #4641 fixes — a file + // being in this literal list proves nothing about why it is in the tier. + // So the assertion here is on the SIGNAL: each file must still classify + // needsRealOs === true, and its surviving `signals` must be non-empty and + // drawn from the real (non-house-idiom) category names still present in + // CATEGORIES after the #4641 narrowing. + const files = [ + 'tests/shell-command-projection-dispatch.test.cjs', + 'tests/review-lane-windows-spawn-resolution.test.cjs', + 'tests/prohibition-enforcement.test.cjs', + ]; + const validSignalNames = new Set(CATEGORIES.map((c) => c.name)); + for (const rel of files) { + const absPath = path.join(ROOT, rel); + const content = fs.readFileSync(absPath, 'utf8'); + const { needsRealOs, signals } = classifyContent(content); + assert.equal(needsRealOs, true, rel); + assert.ok(signals.length > 0, `${rel}: expected a non-empty signal set, got none`); + for (const signal of signals) { + assert.ok( + validSignalNames.has(signal), + `${rel}: signal "${signal}" is not a genuine platform category name (${JSON.stringify([...validSignalNames])})`, + ); + } + } + }); + + test('CATEGORIES contains the shell-interpreter-spawn detector (#4641)', () => { + const names = CATEGORIES.map((c) => c.name); + assert.ok( + names.includes('shell-interpreter-spawn'), + `CATEGORIES must contain shell-interpreter-spawn: ${JSON.stringify(names)}`, + ); + }); + + test('a real interpreter: bash spawn (the adversarial-review finding) still classifies IN (#4641)', () => { + // tests/execute-phase-worktree-guard.test.cjs calls tests/helpers/ + // process-seam.cjs's runHook(..., { interpreter: 'bash', ... }), which + // spawns a REAL bash binary via spawnSync. That is a genuine + // platform-dependent signal (bash availability, quoting, git output + // parsing all differ across OSes) that the removed + // 'process-seam-subprocess' category used to catch incidentally, and + // which silently dropped out of the tier when that category was removed + // — an adversarial review caught this as a real false negative (#4641). + // Assert directly against the real file's content so nobody can + // "fix" a regression here by re-editing a hand-written fixture string. + const absPath = path.join(ROOT, 'tests/execute-phase-worktree-guard.test.cjs'); + const content = fs.readFileSync(absPath, 'utf8'); + const { needsRealOs, signals } = classifyContent(content); + assert.equal(needsRealOs, true, 'tests/execute-phase-worktree-guard.test.cjs'); + assert.ok( + signals.includes('shell-interpreter-spawn'), + `expected shell-interpreter-spawn among signals, got: ${JSON.stringify(signals)}`, + ); + }); + + test('runHook without an interpreter option does not match shell-interpreter-spawn (#4641)', () => { + // The detector must key on a real shell name being passed as the + // `interpreter` option, not on the mere presence of `runHook(...)` — + // the default (no `interpreter:` option) spawns node, not a real shell, + // and is not a platform signal. + const fixture = "runHook(HOOK_PATH, [], { cwd: dir });\nassert.equal(result.exitCode, 0);\n"; + const { needsRealOs, signals } = classifyContent(fixture); + assert.equal(needsRealOs, false); + assert.ok(!signals.includes('shell-interpreter-spawn'), JSON.stringify(signals)); + }); + + test('a source-text-analysis test drops out of the tier even when its filename says windows (#4641)', () => { + // tests/windows-robustness.test.cjs carries `// allow-test-rule: + // source-text-is-the-product` and only ever reads OTHER files' source + // text and asserts on it (e.g. `assert.match(region, /windowsHide:\s*true/)`). + // Its apparent `spawnSync(` / `execFileSync(` hits are string-literal + // search anchors into other files' source, not real subprocess calls — + // it spawns nothing itself and is fully Linux-runnable. Despite the + // filename, it must classify OUT of the real-OS tier. Do not "fix" this + // by re-adding the file to the four-named-files case above. + const absPath = path.join(ROOT, 'tests/windows-robustness.test.cjs'); + const content = fs.readFileSync(absPath, 'utf8'); + const { needsRealOs } = classifyContent(content); + assert.equal(needsRealOs, false, 'tests/windows-robustness.test.cjs'); + }); +}); + +// ─── #4641: ALWAYS_REAL_OS escape hatch for code-under-test-only signals ─── + +describe('ALWAYS_REAL_OS escape hatch (#4641)', () => { + test('is a Map, so every entry is forced to carry a reason', () => { + assert.ok(ALWAYS_REAL_OS instanceof Map, 'ALWAYS_REAL_OS must be a Map'); + }); + + test('every key is present in the committed Windows CONFORMANCE_TIER_FILES', () => { + delete require.cache[require.resolve(GENERATED_PATH)]; + const { CONFORMANCE_TIER_FILES } = require(GENERATED_PATH); + const committedSet = new Set(CONFORMANCE_TIER_FILES); + for (const relPath of ALWAYS_REAL_OS.keys()) { + assert.ok( + committedSet.has(relPath), + `${relPath} is in ALWAYS_REAL_OS but missing from the committed Windows tier — ` + + 'run `node scripts/gen-platform-conformance-tier.cjs --write`', + ); + } + }); + + test('every entry has a non-empty recorded reason', () => { + for (const [relPath, reason] of ALWAYS_REAL_OS.entries()) { + assert.equal(typeof reason, 'string', `${relPath}: reason must be a string`); + assert.ok(reason.trim().length > 0, `${relPath}: reason must be non-empty`); + } + }); + + test('every key names a file that actually exists on disk', () => { + for (const relPath of ALWAYS_REAL_OS.keys()) { + const absPath = path.join(ROOT, relPath); + assert.ok( + fs.existsSync(absPath), + `${relPath} is enumerated in ALWAYS_REAL_OS but does not exist on disk — stale allowlist entry ` + + '(silent rot: the file was likely deleted or renamed)', + ); + } + }); + + test('tests/external-descriptor-confinement.test.cjs is enumerated (#4641)', () => { + // It exercises isPathConfined (src/external-descriptor-trust.cts:41-49), + // which uses the AMBIENT path module (path.resolve/path.sep) with no + // platform/path injection — its win32 branch (drive letters, UNC paths, + // \ separator) is only reachable by actually running on Windows. A + // security-relevant write-confinement gate; do not remove this entry to + // "clean up" the allowlist. + assert.ok( + ALWAYS_REAL_OS.has('tests/external-descriptor-confinement.test.cjs'), + 'tests/external-descriptor-confinement.test.cjs must stay in ALWAYS_REAL_OS', + ); + }); + + test('does not leak into the macOS tier — macOS is POSIX, the win32 concern does not apply', () => { + delete require.cache[require.resolve(MACOS_GENERATED_PATH)]; + const { MACOS_CONFORMANCE_TIER_FILES } = require(MACOS_GENERATED_PATH); + const macosSet = new Set(MACOS_CONFORMANCE_TIER_FILES); + assert.ok( + !macosSet.has('tests/external-descriptor-confinement.test.cjs'), + 'tests/external-descriptor-confinement.test.cjs must be absent from MACOS_CONFORMANCE_TIER_FILES ' + + '(the ALWAYS_REAL_OS entry is Windows-only)', + ); + }); +}); diff --git a/tests/release-tarball-smoke.install.test.cjs b/tests/release-tarball-smoke.install.test.cjs index f33ead312..ae6f8aa73 100644 --- a/tests/release-tarball-smoke.install.test.cjs +++ b/tests/release-tarball-smoke.install.test.cjs @@ -511,25 +511,42 @@ function safeRealpath(p) { describe('bug-131: runNpm isolates HOME from the caller environment', () => { // ── Test 1 — runNpm works with an unwritable HOME ──────────────────────── - // Spawn a child Node process that sets HOME to a chmod-0500 directory, then - // invokes runNpm(['--version']). Without the fix, npm tries to read/write - // HOME/.npmrc and HOME/.npm, fails with EACCES, and runNpm throws. - // With the fix, runNpm injects its own isolated HOME and npm succeeds. + // Spawn a child Node process that sets HOME to an unwritable directory, then + // invokes runNpm(['cache', 'verify']). `npm --version` performs zero + // filesystem I/O against HOME/.npm or HOME/.npmrc on modern npm, and even + // `npm config get cache` only *resolves* the cache path as a string without + // touching disk — both stay green even without HOME isolation, making the + // assertion vacuous. `npm cache verify` genuinely creates/reads/writes the + // cache directory under HOME (mkdir _cacache, write logs), so without the + // fix it fails with ENOTDIR against the unwritable HOME, and with the fix + // runNpm's injected isolated HOME lets it succeed. (Proven empirically: with + // this exact probe, neutralising runNpm()'s isolation flips this test from + // green to red, whereas `npm config get cache` stayed green either way.) test('runNpm succeeds even when process HOME is unwritable', () => { - // Create an unwritable dir to serve as a poisoned HOME. - const poisonedHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-bug131-poison-')); + // Simulate an unwritable HOME with a mechanism that holds for every uid, + // including root (as gsd-test Docker benches run). chmod 0o500 is + // insufficient because root bypasses mode bits entirely, silently making + // this assertion vacuous under root — see CLAUDE.md section 4. Instead, + // make the PARENT of "HOME" a regular file rather than a directory: any + // attempt to create or write an entry under a non-directory parent fails + // with ENOTDIR at the filesystem/VFS level, a property that has nothing + // to do with permission bits and therefore cannot be bypassed by root. + const poisonedHomeBlocker = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-bug131-poison-')); + const blockerFile = path.join(poisonedHomeBlocker, 'blocker'); + fs.writeFileSync(blockerFile, ''); // regular file, not a directory + const poisonedHome = path.join(blockerFile, 'home'); // parent is a file → ENOTDIR try { - fs.chmodSync(poisonedHome, 0o500); // r-x only — not writable // We exercise the real runNpm() path by running a tiny inline Node script - // that requires helpers.cjs and calls runNpm(['--version']) with HOME set - // to the unwritable dir. The script exits 0 on success, non-zero on throw. + // that requires helpers.cjs and calls runNpm(['cache', 'verify']) with + // HOME set to the unwritable dir. The script exits 0 on success, non-zero + // on throw. const script = ` process.env.HOME = ${JSON.stringify(poisonedHome)}; process.env.USERPROFILE = ${JSON.stringify(poisonedHome)}; const { runNpm } = require(${JSON.stringify(path.join(__dirname, 'helpers.cjs'))}); try { - const out = runNpm(['--version']); + const out = runNpm(['cache', 'verify']); if (!out || out.trim() === '') process.exit(2); // vacuous success guard process.stdout.write(out); process.exit(0); @@ -558,16 +575,16 @@ describe('bug-131: runNpm isolates HOME from the caller environment', () => { 0, `runNpm should succeed with an unwritable HOME but exited ${exitCode}. stderr: ${stderr}`, ); - // npm --version returns something like "10.x.y" + // npm cache verify reports what it found/fixed in the cache directory. assert.match( - stdout.trim(), - /^\d+\.\d+/, - `expected semver output from npm --version, got: ${stdout}`, + stdout, + /cache verified|content verified/i, + `expected npm cache verify output, got: ${stdout}`, ); } finally { - // Restore write permission before cleanup so the directory can be deleted. - try { fs.chmodSync(poisonedHome, 0o700); } catch (_) { /* best-effort */ } - cleanup(poisonedHome); + // poisonedHome itself was never created (its parent is a file), so only + // the directory holding the blocker file needs cleanup. + cleanup(poisonedHomeBlocker); } });