golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

enhance(#3618): fold fallow-runner onto the canonical binary resolver (epic #3411 Phase 2) (#3633)

Browse Source 
* chore(#3618): fold fallow-runner onto the canonical binary resolver

Epic #3411 Phase 2. src/fallow-runner.cts was the fourth divergent
implementation of Windows binary resolution the epic enumerated —
candidateNames, isExecutableFile, findInPath, findInNodeModules, 40 lines.
All four are deleted; resolveFallowBinary is one seam call.

Two OPT-IN options were added to resolveExecutableBinary to make the fold
behavior-preserving, both defaulting off so Phase 1's callers are byte-identical:

  prependPaths      dirs searched before env.PATH, in order, through the
                    identical per-directory candidate logic. This expresses
                    node_modules/.bin-first precedence without env surgery —
                    the rejected alternative re-introduced the
                    spread-loses-the-proxy hazard the Windows lane caught in
                    Phase 1, at every future call site instead of once.
  requireExecutable POSIX-only accessSync(X_OK); a no-op on win32 where mode
                    bits do not mean execute. Opt-in rather than default
                    because unconditional X_OK breaks #3445's suite, which
                    stages candidates with plain writeFileSync and never sets
                    an exec bit — the repo bans chmod in tests — so every one
                    would resolve to null on POSIX.

Deliberate behavior change on Windows: fallow's prior candidate list ended in a
BARE fallow. The seam never tries a bare name there, so an extensionless file
beside fallow.cmd is no longer resolved. That is the fix, not a regression — the
extensionless file is npm's POSIX sh shim, which CreateProcess cannot run
(#3275). Rows 7 and 8 of the design record it.

Defect found while working, fixed inline: the resolution order was documented
BACKWARDS as PATH-then-.bin in structural-pre-pass.md, docs/INVENTORY.md and
four INVENTORY translations. The code has always been .bin first, and .bin first
is correct — a project-local tool should beat a global one. The archived
changeset is left alone as a historical record.

fallow-runner had no test file at all. tests/fallow-runner.test.cjs is new
(F1-F15) and the seam options are pinned by S1-S12 folded into the existing
dispatch suite. RED proven by execution: with both source files stashed and
build:lib re-run, 7 of 27 probe cases failed.

Refs #3411

* chore(#3618): backfill changeset pr number 3633

* fix(#3618): assert both platform contracts in F4 instead of a POSIX-only premise

Windows CI on #3633 failed F4. The test monkeypatched accessSync to throw and
asserted resolveFallowBinary returned null — but that premise, that the X_OK
check is consulted at all, is POSIX-only by design. requireExecutable is a
deliberate no-op on win32 because Windows mode bits do not mean execute, so the
staged fixture correctly resolved there.

40-design.md's negative-space section already states this carve-out verbatim.
The test contradicted the design it was written from: fixtures were made
platform-adaptive in the previous commit, and this assertion was left
platform-blind.

F4 now asserts BOTH contracts — null on POSIX, resolves on win32 — rather than
skipping either. A t.skip on one lane would have been green and would have left
the win32 carve-out unpinned by fallow's own entry point.

Audited every other row for the same class. F1-F3, F5, F6, F11-F15 hold on both
platforms; F7-F10 and S1-S12 inject platform explicitly and are unaffected. F4
was the only row with a single-platform premise.

The local probe runs on one platform and structurally cannot catch this, which
is why it was green — that limitation is now stated at the top of the probe so a
green probe is not mistaken for platform coverage. The win32 branch was proven
by injecting platform:'win32' with accessSync throwing and asserting it still
resolves.

Refs #3411

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-18 15:32:57 -04:00
committed by GitHub
parent 46f14c621e
commit ac1b6d679f
12 changed files with 614 additions and 59 deletions
Download Patch File Download Diff File Expand all files Collapse all files

60
src/fallow-runner.cts
View File

@@ -7,51 +7,18 @@
*
* Parses the real fallow `audit --format json` schema (schema_version 3
* envelope, nested dead_code/duplication sections). See fallow 2.70.0+.
*
* #3411 Phase 2 (#3618): binary resolution no longer lives here — it delegates
* to the platform seam's `resolveExecutableBinary` (shell-command-projection.cts).
* This file's prior private resolver deliberately included an extensionless
* `fallow` as a win32 candidate; the seam does not, and that is a fix, not a
* regression — an extensionless file sitting beside `fallow.cmd` is npm's POSIX
* `sh` shim, which `CreateProcess` cannot run (#3275).
*/
import fs from 'node:fs';
import path from 'node:path';
function candidateNames(): string[] {
return process.platform === 'win32'
? ['fallow.exe', 'fallow.cmd', 'fallow.bat', 'fallow']
: ['fallow'];
}
function isExecutableFile(filePath: string): boolean {
try {
const stat = fs.statSync(filePath);
if (!stat.isFile()) return false;
if (process.platform === 'win32') return true;
fs.accessSync(filePath, fs.constants.X_OK);
return true;
} catch {
return false;
}
}
function findInPath(envPath: string | undefined): string | null {
if (!envPath) return null;
const names = candidateNames();
const segments = envPath.split(path.delimiter).filter(Boolean);
for (const segment of segments) {
for (const name of names) {
const candidate = path.join(segment, name);
if (isExecutableFile(candidate)) return candidate;
}
}
return null;
}
function findInNodeModules(cwd: string): string | null {
const names = candidateNames();
const binDir = path.join(cwd, 'node_modules', '.bin');
for (const name of names) {
const candidate = path.join(binDir, name);
if (isExecutableFile(candidate)) return candidate;
}
return null;
}
import { resolveExecutableBinary } from './shell-command-projection.cjs';
export interface ResolveFallowOpts {
cwd: string;
@@ -59,7 +26,16 @@ export interface ResolveFallowOpts {
}
export function resolveFallowBinary({ cwd, envPath = process.env['PATH'] ?? '' }: ResolveFallowOpts): string | null {
return findInNodeModules(cwd) || findInPath(envPath) || null;
return resolveExecutableBinary('fallow', {
prependPaths: [path.join(cwd, 'node_modules', '.bin')],
// PATHEXT is passed through (not spread from the rest of process.env) so
// win32 resolution still works, while avoiding the spread-loses-the-
// case-insensitive-proxy hazard the Windows lane caught in Phase 1. `?? ''`
// is intentional: the seam falls back to its own DEFAULT_PATHEXT when this
// is empty/absent, so an empty string here is a safe "let the seam decide".
env: { PATH: envPath, PATHEXT: process.env['PATHEXT'] ?? '' },
requireExecutable: true,
});
}
export function requireFallowBinary({ cwd, envPath = process.env['PATH'] ?? '' }: ResolveFallowOpts): string {

43
src/shell-command-projection.cts
View File

@@ -653,11 +653,14 @@ const DEFAULT_PATHEXT = '.EXE;.CMD;.BAT;.COM';
/** Windows extensions that must be mediated through cmd.exe rather than spawned. */
const CMD_MEDIATED_EXT = /\.(cmd|bat)$/i;
function _isFile(candidate: string): boolean {
function _isFile(candidate: string, requireExecutable = false, platform: string = process.platform): boolean {
try {
return fs.statSync(candidate).isFile();
if (!fs.statSync(candidate).isFile()) return false;
if (requireExecutable && platform !== 'win32') fs.accessSync(candidate, fs.constants.X_OK);
return true;
} catch {
// Missing, unreadable (EACCES), or a broken link — all mean "not this one".
// Missing, unreadable (EACCES), a broken link, or (when requireExecutable
// is set) not executable — all mean "not this one".
return false;
}
}
@@ -733,25 +736,45 @@ function _buildVerbatimCmdLine(target: string, args: string[]): string {
* Path-like names (any `/` or `\`) bypass the PATH scan: the name is already an
* address, so it passes through when it names an existing file.
*
* **`opts.prependPaths`** — directories searched BEFORE `env.PATH`, in array
* order (e.g. a project-local `node_modules/.bin`). Defaults to `[]`, so
* Phase 1's callers (`execTool`, and `gsd-tools.cjs`'s `resolveSpawnBinary` /
* `deps.spawn` / `hasBinary`), which set neither new option, are byte-identical
* to today. The existing per-directory candidate logic (win32 as-is-then-append-
* PATHEXT; POSIX bare name) applies to prepended directories exactly as it does
* to `PATH` segments — there is no special-casing.
*
* **`opts.requireExecutable`** — when `true` and the platform is not `win32`,
* a candidate must additionally pass `fs.accessSync(candidate, fs.constants.X_OK)`
* to count as a match. On `win32` this is a no-op (mode bits do not mean
* execute on Windows — the same carve-out `fallow-runner`'s prior private
* resolver already had). Defaults to `false`, so `accessSync` is never called
* unless a caller opts in. It is opt-in rather than the default because making
* `X_OK` unconditional would break #3445's suite: those tests stage candidates
* with plain `fs.writeFileSync` and never set an exec bit (the repo bans
* `chmod` in tests), so every one of them would resolve to `null` on POSIX.
*
* @returns the resolved path, or `null` when nothing matched. Callers fall back to
* the declared name on `null` so a genuine ENOENT still surfaces (#3086).
*/
export function resolveExecutableBinary(
name: string | null | undefined,
opts: { platform?: string; env?: NodeJS.ProcessEnv } = {},
opts: { platform?: string; env?: NodeJS.ProcessEnv; prependPaths?: string[]; requireExecutable?: boolean } = {},
): string | null {
if (!name) return null;
const requireExecutable = opts.requireExecutable ?? false;
const platform = opts.platform ?? process.platform;
if (name.includes('/') || name.includes('\\')) {
return _isFile(name) ? name : null;
return _isFile(name, requireExecutable, platform) ? name : null;
}
const env = opts.env ?? process.env;
const platform = opts.platform ?? process.platform;
const segments = String(_envGet(env, 'PATH') || '').split(path.delimiter).filter(Boolean);
const pathSegments = String(_envGet(env, 'PATH') || '').split(path.delimiter).filter(Boolean);
const segments = [...(opts.prependPaths ?? []), ...pathSegments];
if (platform !== 'win32') {
for (const dir of segments) {
const candidate = path.join(dir, name);
if (_isFile(candidate)) return candidate;
if (_isFile(candidate, requireExecutable, platform)) return candidate;
}
return null;
}
@@ -766,11 +789,11 @@ export function resolveExecutableBinary(
for (const dir of segments) {
if (carriesKnownExt) {
const asIs = path.join(dir, name);
if (_isFile(asIs)) return asIs;
if (_isFile(asIs, requireExecutable, platform)) return asIs;
}
for (const ext of exts) {
const candidate = path.join(dir, name + ext);
if (_isFile(candidate)) return candidate;
if (_isFile(candidate, requireExecutable, platform)) return candidate;
}
}
return null;