Skip to content

fix(cli): engage the gh stubs on Windows instead of calling the real API - #245

Open
ophiocus wants to merge 5 commits into
theam:mainfrom
ophiocus:fix/windows-posix-only-cli-suite
Open

fix(cli): engage the gh stubs on Windows instead of calling the real API#245
ophiocus wants to merge 5 commits into
theam:mainfrom
ophiocus:fix/windows-posix-only-cli-suite

Conversation

@ophiocus

Copy link
Copy Markdown
Contributor

Closes #241. The suite's gh stubs never engaged on Windows — the fixture PATH was joined with a hard-coded : and the stub is an extensionless script — so eight tests resolved the developer's real gh and called api.github.com, including the one named "deterministic GitHub fixtures". This PR makes the stubs engage on every platform, adds a belt so a future fallthrough can never reach the real API, and stands up the verify-windows CI job so the whole Windows-only defect class fails in CI instead of on contributors' machines.

Why a seam, not a shim

The obvious fix — a gh.cmd wrapper beside the stub — cannot work: Node refuses to execute .cmd/.bat through execFile without a shell by design since CVE-2024-27980 (the same refusal behind #182's spawn EINVAL). No PATH arrangement makes a script stub executable there. So the templates gain a minimal, shell-free seam:

const GH_BIN = process.env.FACILITY_GH_BIN ?? "gh";
const GH_ARGS = process.env.FACILITY_GH_ARGS ? JSON.parse(process.env.FACILITY_GH_ARGS) : [];

Production behavior is byte-identical (default "gh", no args, no shell ever). Tests set FACILITY_GH_BIN=process.execPath and pass the stub script via FACILITY_GH_ARGS — works identically on POSIX and Windows.

The changes

What Where
Seam in the two gh-calling templates templates/delivery/verify.mjs, templates/doctor/resolve.mjs
Tests drive the seam; PATH joined with path.delimiter test/delivery.test.mjs, test/doctor-policy.test.mjs
Fail-offline belt: stub env pins GH_HOST=gh-stub.invalid + dummy token, so if a stub ever falls through again the suite dies against an unresolvable host, never against the real API both test files
POSIX 0600 asserts scoped to platforms with mode bits (NTFS has none); the secret-not-logged assert still runs everywhere test/platform.test.mjs
init e2e skipped on win32 with #230 as the recorded unskip criterion (its symlink bug, not this PR's subject) test/init.test.mjs
verify-windows job: CLI suite + guards on windows-latest, same pinned action SHAs as verify .github/workflows/ci.yml

Why the CI job belongs here

Ten currently-open issues are Windows-only defects invisible to ubuntu-latest (#182 #183 #167 #191 #217 #227 #228 #230 #240 #241 — inventory in #241). Every one shipped because nothing red ran on Windows. The job covers the pure-Node surfaces (CLI suite, guards) in ~3 minutes — no Docker, no Postgres — and this very PR is its first proof: before it, that job fails 11 tests; with it, green.

Verified

Found while running the suite on a real Windows machine — happy to iterate on any of it, and the Windows arc continues in #240/#227.

…alling the real API

The gh stubs never engaged on Windows: the fixture PATH was joined with a
hard-coded colon and the stub is an extensionless script, so eight tests
resolved the developer's real gh and called api.github.com — including the
one named "deterministic GitHub fixtures". Node refuses .cmd files without
a shell (CVE-2024-27980), so no PATH arrangement can fix this; the templates
now expose a shell-free seam (FACILITY_GH_BIN / FACILITY_GH_ARGS, default
"gh" with no args) and the tests drive it. The stub env also pins
GH_HOST/GH_TOKEN to invalid values so any future fallthrough dies offline.
POSIX mode asserts are scoped to platforms with mode bits; the init e2e is
skipped on win32 with theam#230 as its unskip criterion. A verify-windows CI job
runs the CLI suite and guards on windows-latest so this whole class fails
in CI instead of on contributors' machines.

Closes theam#241

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The hosted image defaults core.autocrlf=true; the CRLF checkout blinded
the newline-anchored watchtower-template assertions on the job's first
run (95/1) while local Windows and Linux pass. LF checkout matches what
every other environment tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@adrian-lorenzo adrian-lorenzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

FACILITY_GH_BIN and FACILITY_GH_ARGS are now trusted from the ambient environment by the shipped delivery verifier. The builder and repository checks run before the delivery steps, and either can persist these variables through $GITHUB_ENV. The verifier would then execute the selected binary with GH_TOKEN, allowing earlier code to replace gh and fake the GitHub evidence or run inside the privileged finalization step.

Please keep the test seam at the module boundary—for example, by injecting the GitHub runner into an exported entry point—while the production entry point always invokes the fixed executable. Add a regression test showing that hostile ambient variables cannot redirect it.

The Windows CI coverage and offline safeguard are worth keeping. Once this boundary is sealed, this will be a strong improvement.

Review round two: FACILITY_GH_BIN/FACILITY_GH_ARGS were readable from
the ambient environment by the shipped verifier and resolver, so an
earlier workflow step persisting variables through $GITHUB_ENV could
swap the executable they run while holding GH_TOKEN. The seam now lives
where the review asked: verify.mjs exports runDelivery(mode, { gh })
and resolve.mjs exports main({ gh }); the script entry paths construct
the fixed "gh" executable with nothing about the invocation readable
from env. The delivery and resolver integration tests inject the runner
through those exported entry points in-process, and two spawn-path
regression tests prove a hostile FACILITY_GH_BIN pointing at a
marker-writing binary is never executed - the fixed gh is attempted and
dies offline against the poisoned GH_HOST instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ophiocus

ophiocus commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Sealed in 48e510a — and thank you for spelling out the attack path; $GITHUB_ENV persistence from an earlier step is exactly the kind of thing I should have modeled before reaching for env vars at all.

The seam now lives where you pointed:

  • verify.mjs exports runDelivery(mode, { gh }) and resolve.mjs exports main({ gh }) — the injectable runner exists only as a parameter on those entry points. The script paths construct the fixed "gh" executable inline; nothing about the invocation is readable from the environment anymore.
  • The delivery tests and the resolver integration tests now inject the runner in-process through the exported entry points (the doctor tests got simpler for it — the fixture map became a closure instead of a stub binary).
  • Two spawn-path regression tests prove the boundary holds: each launches the real script with a hostile FACILITY_GH_BIN pointing at a marker-writing binary plus GH_HOST=gh-stub.invalid — the marker is never written, and the fixed gh dies offline instead of reaching the API. If anyone ever reintroduces an ambient override, those two go red.

Verified: Linux 99 pass / 0 fail, Windows 98 pass / 0 fail on the same head; the verify-windows job you liked stays in and re-proves it on the hosted runner.

ophiocus and others added 2 commits September 1, 2026 12:05
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nly-cli-suite

# Conflicts:
#	.github/workflows/ci.yml
@ophiocus ophiocus changed the title test(cli): engage the gh stubs on Windows instead of calling the real API fix(cli): engage the gh stubs on Windows instead of calling the real API Sep 1, 2026
@ophiocus

ophiocus commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Status note: verify-windows and minimum-node are green on the merged head; the verify red is not this PR — two browserslist advisories published ~18:00Z today fail the audit step of every run repo-wide. #267 carries the one-line floor (house override pattern) that clears it for all open PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(cli): suite is POSIX-only — gh stubs never engage on Windows, so eight tests call the real GitHub API

2 participants