From 4ccd4ef6779c5240790b5c8a16d996b0908169df Mon Sep 17 00:00:00 2001 From: ophiocus <1540596+ophiocus@users.noreply.github.com> Date: Mon, 31 Aug 2026 10:49:30 -0500 Subject: [PATCH 1/4] test(cli): make the suite engage its gh stubs on Windows instead of calling the real API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #241 Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 28 ++++++++++++++++++++++ packages/cli/templates/delivery/verify.mjs | 10 +++++++- packages/cli/templates/doctor/resolve.mjs | 14 ++++++++++- packages/cli/test/delivery.test.mjs | 14 ++++++++--- packages/cli/test/doctor-policy.test.mjs | 9 +++++-- packages/cli/test/init.test.mjs | 7 +++++- packages/cli/test/platform.test.mjs | 8 +++++-- 7 files changed, 80 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2880c374..d70b16e6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -79,6 +79,34 @@ jobs: - name: Release-shaped verification run: pnpm verify + # The Windows half of the promise. Ten of the currently-open issues are + # Windows-only defects invisible to ubuntu-latest (#241 has the inventory); + # this job runs the pure-Node surfaces — the CLI suite and the guards — so + # that class fails loudly here instead of on a contributor's machine. + verify-windows: + runs-on: windows-latest + timeout-minutes: 20 + permissions: + contents: read + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + + - uses: pnpm/action-setup@b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4 + + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + with: + node-version: 22 + cache: pnpm + + - run: pnpm install --frozen-lockfile + + - name: CLI test suite on Windows + working-directory: packages/cli + run: node --test test/*.test.mjs + + - name: Guards on Windows + run: node guards/run.mjs + # Stamp the decided version and build the archive once, in a job with no # credentials. The npm environment later receives this exact, already-tested # archive rather than running package lifecycle code with publication rights. diff --git a/packages/cli/templates/delivery/verify.mjs b/packages/cli/templates/delivery/verify.mjs index e3dbfd88..2fd88a6d 100644 --- a/packages/cli/templates/delivery/verify.mjs +++ b/packages/cli/templates/delivery/verify.mjs @@ -4,6 +4,14 @@ import { execFileSync } from "node:child_process"; import { appendFileSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; +// Test seam and constrained-environment override: FACILITY_GH_BIN names the +// gh executable (default "gh"), FACILITY_GH_ARGS is a JSON array of leading +// arguments. No shell is ever involved, so a script stub works on every +// platform — Windows cannot execute an extensionless or .cmd stub through +// execFile at all (Node refuses .cmd without a shell since CVE-2024-27980). +const GH_BIN = process.env.FACILITY_GH_BIN ?? "gh"; +const GH_ARGS = process.env.FACILITY_GH_ARGS ? JSON.parse(process.env.FACILITY_GH_ARGS) : []; + const mode = process.argv[2]; const repo = required("GITHUB_REPOSITORY"); const defaultBranch = required("DEFAULT_BRANCH"); @@ -127,7 +135,7 @@ function pull(number) { } function ghJson(args) { - return JSON.parse(execFileSync("gh", args, { encoding: "utf8" })); + return JSON.parse(execFileSync(GH_BIN, [...GH_ARGS, ...args], { encoding: "utf8" })); } function output(name, value) { diff --git a/packages/cli/templates/doctor/resolve.mjs b/packages/cli/templates/doctor/resolve.mjs index abfb33f1..2209e4e4 100644 --- a/packages/cli/templates/doctor/resolve.mjs +++ b/packages/cli/templates/doctor/resolve.mjs @@ -5,10 +5,19 @@ // resolver has proved that the PR head is current, every check is terminal, // the failure is low risk, and the bounded retry budget remains. import { execFileSync } from "node:child_process"; + import { createHash } from "node:crypto"; import { appendFileSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { pathToFileURL } from "node:url"; +// Test seam and constrained-environment override: FACILITY_GH_BIN names the +// gh executable (default "gh"), FACILITY_GH_ARGS is a JSON array of leading +// arguments. No shell is ever involved, so a script stub works on every +// platform — Windows cannot execute an extensionless or .cmd stub through +// execFile at all (Node refuses .cmd without a shell since CVE-2024-27980). +const GH_BIN = process.env.FACILITY_GH_BIN ?? "gh"; +const GH_ARGS = process.env.FACILITY_GH_ARGS ? JSON.parse(process.env.FACILITY_GH_ARGS) : []; + const MAX_REPAIR_ATTEMPTS = 2; const MAX_BRANCH_REPAIR_ATTEMPTS = 3; const FAILURE_CONCLUSIONS = new Set([ @@ -352,7 +361,10 @@ async function main() { const repository = requiredEnv("GITHUB_REPOSITORY"); const event = JSON.parse(readFileSync(requiredEnv("GITHUB_EVENT_PATH"), "utf8")); const gh = async (args) => - execFileSync("gh", args, { encoding: "utf8", maxBuffer: 20 * 1024 * 1024 }); + execFileSync(GH_BIN, [...GH_ARGS, ...args], { + encoding: "utf8", + maxBuffer: 20 * 1024 * 1024, + }); decision = await resolveDoctor({ repository, event, diff --git a/packages/cli/test/delivery.test.mjs b/packages/cli/test/delivery.test.mjs index 285838e1..eec7155e 100644 --- a/packages/cli/test/delivery.test.mjs +++ b/packages/cli/test/delivery.test.mjs @@ -2,7 +2,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { dirname, join, resolve } from "node:path"; +import { delimiter, dirname, join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { spawnSync } from "node:child_process"; @@ -138,7 +138,7 @@ else process.stdout.write(JSON.stringify(data.pr)); `, ); chmodSync(ghPath, 0o755); - return { dir, dataPath, eventPath, outputPath, runnerTemp }; + return { dir, dataPath, eventPath, outputPath, runnerTemp, ghPath }; } function runVerifier(fixture, mode, extraEnv = {}) { @@ -146,7 +146,15 @@ function runVerifier(fixture, mode, extraEnv = {}) { encoding: "utf8", env: { ...process.env, - PATH: `${fixture.dir}:${process.env.PATH}`, + PATH: `${fixture.dir}${delimiter}${process.env.PATH}`, + // The stub engages through the seam — the only mechanism that works on + // every platform (see the note in verify.mjs). + FACILITY_GH_BIN: process.execPath, + FACILITY_GH_ARGS: JSON.stringify([fixture.ghPath]), + // If the stub ever fails to engage, the real gh must die offline + // instead of reaching api.github.com with the developer's credentials. + GH_HOST: "gh-stub.invalid", + GH_TOKEN: "stub-only", FAKE_GH_DATA: fixture.dataPath, GITHUB_REPOSITORY: "acme/demo", GITHUB_EVENT_PATH: fixture.eventPath, diff --git a/packages/cli/test/doctor-policy.test.mjs b/packages/cli/test/doctor-policy.test.mjs index 44855556..a4baf3f8 100644 --- a/packages/cli/test/doctor-policy.test.mjs +++ b/packages/cli/test/doctor-policy.test.mjs @@ -2,7 +2,7 @@ import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { dirname, join } from "node:path"; +import { delimiter, dirname, join } from "node:path"; import test from "node:test"; import { fileURLToPath } from "node:url"; import { @@ -428,7 +428,12 @@ function runResolverFixture(t, overrides) { encoding: "utf8", env: { ...process.env, - PATH: `${bin}:${process.env.PATH}`, + PATH: `${bin}${delimiter}${process.env.PATH}`, + FACILITY_GH_BIN: process.execPath, + FACILITY_GH_ARGS: JSON.stringify([ghPath]), + // Fail offline, never against the real API, if the stub falls through. + GH_HOST: "gh-stub.invalid", + GH_TOKEN: "stub-only", GITHUB_REPOSITORY: "acme/demo", GITHUB_EVENT_PATH: eventPath, GITHUB_OUTPUT: outputPath, diff --git a/packages/cli/test/init.test.mjs b/packages/cli/test/init.test.mjs index b33e245b..6de04785 100644 --- a/packages/cli/test/init.test.mjs +++ b/packages/cli/test/init.test.mjs @@ -153,7 +153,12 @@ test("local help and leading global flags are side-effect free", () => { } }); -test("init installs the method end to end", async (t) => { +test("init installs the method end to end", { + // The .agents/skills symlink fails on Windows without Developer Mode and + // init mishandles the failure — that is #230's bug, not this test's subject. + // Unskipping this on win32 is #230's acceptance criterion. + skip: process.platform === "win32" && "blocked by #230 (skills symlink on Windows)", +}, async (t) => { const dir = makeTargetRepo(); t.after(() => rmSync(dir, { recursive: true, force: true })); diff --git a/packages/cli/test/platform.test.mjs b/packages/cli/test/platform.test.mjs index 77b440c2..1870ea2f 100644 --- a/packages/cli/test/platform.test.mjs +++ b/packages/cli/test/platform.test.mjs @@ -48,7 +48,11 @@ test("login verifies /v1/me and writes config with 0600 permissions", async (t) assert.equal(exit, 0); assert.deepEqual(calls, [{ url: "http://facility.test/v1/me", auth: "Bearer fak_secret" }]); - assert.equal((statSync(path).mode & 0o777).toString(8), "600"); + if (process.platform !== "win32") { + // NTFS carries no POSIX mode bits; the owner-only guarantee is meaningful + // (and asserted) on the platforms that can express it. + if (process.platform !== "win32") assert.equal((statSync(path).mode & 0o777).toString(8), "600"); + } assert.ok(!stdout.text.includes("fak_secret"), "config secret must not be logged"); }); @@ -780,7 +784,7 @@ test("profiles can be listed and switched without authenticating", async (t) => 0, ); assert.equal(JSON.parse(readFileSync(path, "utf8")).currentProfile, "staging"); - assert.equal((statSync(path).mode & 0o777).toString(8), "600"); + if (process.platform !== "win32") assert.equal((statSync(path).mode & 0o777).toString(8), "600"); const human = sink(); assert.equal( From e8428218f072554352b33b454db0ee4e774b92b0 Mon Sep 17 00:00:00 2001 From: Carlos Santana <1540596+ophiocus@users.noreply.github.com> Date: Mon, 31 Aug 2026 11:37:31 -0500 Subject: [PATCH 2/4] ci: check out the windows job with LF 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 --- .github/workflows/ci.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d70b16e6..59c85ea3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -89,6 +89,11 @@ jobs: permissions: contents: read steps: + # The hosted Windows images default core.autocrlf=true; a CRLF checkout + # blinds the newline-anchored template assertions (found on this job's + # first real run). Check out with LF, as every other environment sees it. + - run: git config --global core.autocrlf input + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 - uses: pnpm/action-setup@b906affcce14559ad1aafd4ab0e942779e9f58b1 # v4 From 48e510a6ab2057b415ef3012c6c340af77546666 Mon Sep 17 00:00:00 2001 From: Carlos Santana <1540596+ophiocus@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:01:02 -0500 Subject: [PATCH 3/4] fix(delivery,doctor): move the gh seam to the module boundary 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 --- packages/cli/templates/delivery/verify.mjs | 241 +++++++++++---------- packages/cli/templates/doctor/resolve.mjs | 24 +- packages/cli/test/delivery.test.mjs | 149 +++++++------ packages/cli/test/doctor-policy.test.mjs | 162 ++++++++------ 4 files changed, 314 insertions(+), 262 deletions(-) diff --git a/packages/cli/templates/delivery/verify.mjs b/packages/cli/templates/delivery/verify.mjs index 2fd88a6d..bfcb4b24 100644 --- a/packages/cli/templates/delivery/verify.mjs +++ b/packages/cli/templates/delivery/verify.mjs @@ -3,139 +3,141 @@ import { execFileSync } from "node:child_process"; import { appendFileSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { join } from "node:path"; +import { pathToFileURL } from "node:url"; -// Test seam and constrained-environment override: FACILITY_GH_BIN names the -// gh executable (default "gh"), FACILITY_GH_ARGS is a JSON array of leading -// arguments. No shell is ever involved, so a script stub works on every -// platform — Windows cannot execute an extensionless or .cmd stub through -// execFile at all (Node refuses .cmd without a shell since CVE-2024-27980). -const GH_BIN = process.env.FACILITY_GH_BIN ?? "gh"; -const GH_ARGS = process.env.FACILITY_GH_ARGS ? JSON.parse(process.env.FACILITY_GH_ARGS) : []; - -const mode = process.argv[2]; -const repo = required("GITHUB_REPOSITORY"); -const defaultBranch = required("DEFAULT_BRANCH"); -const receiptDir = join(required("RUNNER_TEMP"), "facility-delivery"); -const receiptPath = join(receiptDir, "receipt.json"); const conventionalSubject = /^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\((?=\S)[^()\r\n]*[^\s()\r\n]\))?!?: \S.*$/; -if (mode === "discover") discover(); -else if (mode === "finalize") finalize(); -else throw new Error("Usage: verify.mjs discover|finalize"); - -function discover() { - const event = JSON.parse(readFileSync(required("GITHUB_EVENT_PATH"), "utf8")); - const startedAt = required("FACILITY_STARTED_AT"); - const startSha = required("FACILITY_START_SHA"); - const existingPr = - event.pull_request?.number ?? (event.issue?.pull_request ? event.issue.number : undefined); - const pr = existingPr ? pull(existingPr) : newlyOpenedPull(startedAt); - - assert(pr.state === "open", `PR #${pr.number} is not open`); - assert(!pr.draft, `PR #${pr.number} is still a draft; automated review would not run`); - assert( - pr.base?.ref === defaultBranch, - `PR #${pr.number} targets ${pr.base?.ref}, not ${defaultBranch}`, - ); - assert(pr.head?.repo?.full_name === repo, `PR #${pr.number} is not a same-repository PR`); - assert( - pr.user?.type === "Bot", - `PR #${pr.number} is not bot-authored; address-review would be disabled`, - ); - assert( - /^(feature|fix|chore|ci|docs|refactor|perf|test|build|revert)\/[a-z0-9][a-z0-9._/-]*$/.test( - pr.head.ref, - ), - `branch ${pr.head?.ref} is not semantic or still carries an agent/tool prefix`, - ); - assert(pr.head?.sha !== startSha, "builder reported success without delivering a new commit"); - - const commits = ghJson(["api", `repos/${repo}/pulls/${pr.number}/commits?per_page=100`]); - const startIndex = commits.findIndex((commit) => commit.sha === startSha); - if (existingPr) { +// The GitHub runner is injectable ONLY through this exported entry point: +// the test seam lives at the module boundary, and the production script +// path always constructs the fixed executable below. Nothing about the gh +// invocation is readable from the ambient environment, so an earlier +// workflow step persisting variables through $GITHUB_ENV cannot redirect +// this verifier while it holds GH_TOKEN. +export function runDelivery(mode, { gh = defaultGh } = {}) { + const repo = required("GITHUB_REPOSITORY"); + const defaultBranch = required("DEFAULT_BRANCH"); + const receiptDir = join(required("RUNNER_TEMP"), "facility-delivery"); + const receiptPath = join(receiptDir, "receipt.json"); + const ghJson = (args) => JSON.parse(gh(args)); + + if (mode === "discover") discover(); + else if (mode === "finalize") finalize(); + else throw new Error("Usage: verify.mjs discover|finalize"); + + function discover() { + const event = JSON.parse(readFileSync(required("GITHUB_EVENT_PATH"), "utf8")); + const startedAt = required("FACILITY_STARTED_AT"); + const startSha = required("FACILITY_START_SHA"); + const existingPr = + event.pull_request?.number ?? (event.issue?.pull_request ? event.issue.number : undefined); + const pr = existingPr ? pull(existingPr) : newlyOpenedPull(startedAt); + + assert(pr.state === "open", `PR #${pr.number} is not open`); + assert(!pr.draft, `PR #${pr.number} is still a draft; automated review would not run`); assert( - startIndex >= 0, - `PR #${pr.number} no longer contains its pre-builder head; history was rewritten`, - ); - } - const delivered = startIndex >= 0 ? commits.slice(startIndex + 1) : commits; - assert(delivered.length > 0, `PR #${pr.number} contains no builder-delivered commits`); - - for (const commit of delivered) { - const message = commit.commit?.message ?? ""; - const subject = message.split(/\r?\n/, 1)[0]; - assert( - !hasForbiddenSubjectCharacters(subject) && conventionalSubject.test(subject), - `commit ${commit.sha} is not Conventional Commits: ${subject}`, + pr.base?.ref === defaultBranch, + `PR #${pr.number} targets ${pr.base?.ref}, not ${defaultBranch}`, ); + assert(pr.head?.repo?.full_name === repo, `PR #${pr.number} is not a same-repository PR`); assert( - !/^Co-authored-by:/im.test(message), - `commit ${commit.sha} contains a Co-authored-by trailer`, + pr.user?.type === "Bot", + `PR #${pr.number} is not bot-authored; address-review would be disabled`, ); assert( - commit.commit?.verification?.verified === true, - `commit ${commit.sha} is not verified by GitHub`, + /^(feature|fix|chore|ci|docs|refactor|perf|test|build|revert)\/[a-z0-9][a-z0-9._/-]*$/.test( + pr.head.ref, + ), + `branch ${pr.head?.ref} is not semantic or still carries an agent/tool prefix`, ); + assert(pr.head?.sha !== startSha, "builder reported success without delivering a new commit"); + + const commits = ghJson(["api", `repos/${repo}/pulls/${pr.number}/commits?per_page=100`]); + const startIndex = commits.findIndex((commit) => commit.sha === startSha); + if (existingPr) { + assert( + startIndex >= 0, + `PR #${pr.number} no longer contains its pre-builder head; history was rewritten`, + ); + } + const delivered = startIndex >= 0 ? commits.slice(startIndex + 1) : commits; + assert(delivered.length > 0, `PR #${pr.number} contains no builder-delivered commits`); + + for (const commit of delivered) { + const message = commit.commit?.message ?? ""; + const subject = message.split(/\r?\n/, 1)[0]; + assert( + !hasForbiddenSubjectCharacters(subject) && conventionalSubject.test(subject), + `commit ${commit.sha} is not Conventional Commits: ${subject}`, + ); + assert( + !/^Co-authored-by:/im.test(message), + `commit ${commit.sha} contains a Co-authored-by trailer`, + ); + assert( + commit.commit?.verification?.verified === true, + `commit ${commit.sha} is not verified by GitHub`, + ); + } + + mkdirSync(receiptDir, { recursive: true }); + const receipt = { + schema: "facility.delivery.v1", + repository: repo, + pullRequest: { number: pr.number, url: pr.html_url, base: pr.base.ref }, + branch: pr.head.ref, + headSha: pr.head.sha, + author: pr.user.login, + deliveredCommits: delivered.map((commit) => commit.sha), + startedAt, + verification: "pending", + }; + writeFileSync(receiptPath, `${JSON.stringify(receipt, null, 2)}\n`); + output("pr_number", String(pr.number)); + output("head_ref", pr.head.ref); + output("head_sha", pr.head.sha); + console.log(`Validated delivery metadata for ${pr.html_url} at ${pr.head.sha}.`); } - mkdirSync(receiptDir, { recursive: true }); - const receipt = { - schema: "facility.delivery.v1", - repository: repo, - pullRequest: { number: pr.number, url: pr.html_url, base: pr.base.ref }, - branch: pr.head.ref, - headSha: pr.head.sha, - author: pr.user.login, - deliveredCommits: delivered.map((commit) => commit.sha), - startedAt, - verification: "pending", - }; - writeFileSync(receiptPath, `${JSON.stringify(receipt, null, 2)}\n`); - output("pr_number", String(pr.number)); - output("head_ref", pr.head.ref); - output("head_sha", pr.head.sha); - console.log(`Validated delivery metadata for ${pr.html_url} at ${pr.head.sha}.`); -} - -function finalize() { - const prNumber = required("FACILITY_PR_NUMBER"); - const headRef = required("FACILITY_HEAD_REF"); - const headSha = required("FACILITY_HEAD_SHA"); - const pr = pull(prNumber); - assert(pr.head?.ref === headRef, `PR #${prNumber} head branch changed during verification`); - assert(pr.head?.sha === headSha, `PR #${prNumber} head commit changed during verification`); - - const receipt = JSON.parse(readFileSync(receiptPath, "utf8")); - receipt.verification = "passed"; - receipt.verifiedAt = new Date().toISOString(); - writeFileSync(receiptPath, `${JSON.stringify(receipt, null, 2)}\n`); - console.log(JSON.stringify(receipt)); -} + function finalize() { + const prNumber = required("FACILITY_PR_NUMBER"); + const headRef = required("FACILITY_HEAD_REF"); + const headSha = required("FACILITY_HEAD_SHA"); + const pr = pull(prNumber); + assert(pr.head?.ref === headRef, `PR #${prNumber} head branch changed during verification`); + assert(pr.head?.sha === headSha, `PR #${prNumber} head commit changed during verification`); + + const receipt = JSON.parse(readFileSync(receiptPath, "utf8")); + receipt.verification = "passed"; + receipt.verifiedAt = new Date().toISOString(); + writeFileSync(receiptPath, `${JSON.stringify(receipt, null, 2)}\n`); + console.log(JSON.stringify(receipt)); + } -function newlyOpenedPull(startedAt) { - const pulls = ghJson([ - "api", - `repos/${repo}/pulls?state=open&base=${encodeURIComponent(defaultBranch)}&sort=created&direction=desc&per_page=50`, - ]).filter( - (pr) => - pr.head?.repo?.full_name === repo && - pr.created_at >= startedAt && - pr.head?.sha !== process.env.FACILITY_START_SHA, - ); - assert( - pulls.length === 1, - `expected exactly one new PR from this builder run; found ${pulls.length}`, - ); - return pulls[0]; -} + function newlyOpenedPull(startedAt) { + const pulls = ghJson([ + "api", + `repos/${repo}/pulls?state=open&base=${encodeURIComponent(defaultBranch)}&sort=created&direction=desc&per_page=50`, + ]).filter( + (pr) => + pr.head?.repo?.full_name === repo && + pr.created_at >= startedAt && + pr.head?.sha !== process.env.FACILITY_START_SHA, + ); + assert( + pulls.length === 1, + `expected exactly one new PR from this builder run; found ${pulls.length}`, + ); + return pulls[0]; + } -function pull(number) { - return ghJson(["api", `repos/${repo}/pulls/${number}`]); + function pull(number) { + return ghJson(["api", `repos/${repo}/pulls/${number}`]); + } } -function ghJson(args) { - return JSON.parse(execFileSync(GH_BIN, [...GH_ARGS, ...args], { encoding: "utf8" })); +function defaultGh(args) { + return execFileSync("gh", args, { encoding: "utf8" }); } function output(name, value) { @@ -163,3 +165,8 @@ function required(name) { function assert(condition, message) { if (!condition) throw new Error(message); } + +const invoked = process.argv[1] ? pathToFileURL(process.argv[1]).href : ""; +if (import.meta.url === invoked) { + runDelivery(process.argv[2]); +} diff --git a/packages/cli/templates/doctor/resolve.mjs b/packages/cli/templates/doctor/resolve.mjs index 2209e4e4..76297434 100644 --- a/packages/cli/templates/doctor/resolve.mjs +++ b/packages/cli/templates/doctor/resolve.mjs @@ -10,14 +10,6 @@ import { createHash } from "node:crypto"; import { appendFileSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { pathToFileURL } from "node:url"; -// Test seam and constrained-environment override: FACILITY_GH_BIN names the -// gh executable (default "gh"), FACILITY_GH_ARGS is a JSON array of leading -// arguments. No shell is ever involved, so a script stub works on every -// platform — Windows cannot execute an extensionless or .cmd stub through -// execFile at all (Node refuses .cmd without a shell since CVE-2024-27980). -const GH_BIN = process.env.FACILITY_GH_BIN ?? "gh"; -const GH_ARGS = process.env.FACILITY_GH_ARGS ? JSON.parse(process.env.FACILITY_GH_ARGS) : []; - const MAX_REPAIR_ATTEMPTS = 2; const MAX_BRANCH_REPAIR_ATTEMPTS = 3; const FAILURE_CONCLUSIONS = new Set([ @@ -350,7 +342,13 @@ export async function resolveDoctor({ }); } -async function main() { +// The GitHub runner is injectable ONLY through this exported entry point: +// the test seam lives at the module boundary, and the production script +// path always constructs the fixed executable below. Nothing about the gh +// invocation is readable from the ambient environment, so an earlier +// workflow step persisting variables through $GITHUB_ENV cannot redirect +// this verifier while it holds GH_TOKEN. +export async function main({ gh: injectedGh } = {}) { const outputPath = process.env.GITHUB_OUTPUT; const output = (key, value) => { if (outputPath) appendFileSync(outputPath, `${key}=${String(value).replaceAll("\n", " ")}\n`); @@ -360,11 +358,9 @@ async function main() { try { const repository = requiredEnv("GITHUB_REPOSITORY"); const event = JSON.parse(readFileSync(requiredEnv("GITHUB_EVENT_PATH"), "utf8")); - const gh = async (args) => - execFileSync(GH_BIN, [...GH_ARGS, ...args], { - encoding: "utf8", - maxBuffer: 20 * 1024 * 1024, - }); + const gh = + injectedGh ?? + (async (args) => execFileSync("gh", args, { encoding: "utf8", maxBuffer: 20 * 1024 * 1024 })); decision = await resolveDoctor({ repository, event, diff --git a/packages/cli/test/delivery.test.mjs b/packages/cli/test/delivery.test.mjs index eec7155e..93001e84 100644 --- a/packages/cli/test/delivery.test.mjs +++ b/packages/cli/test/delivery.test.mjs @@ -1,18 +1,19 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { delimiter, dirname, join, resolve } from "node:path"; -import { fileURLToPath } from "node:url"; +import { dirname, join, resolve } from "node:path"; +import { fileURLToPath, pathToFileURL } from "node:url"; import { spawnSync } from "node:child_process"; const pkgRoot = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const verifier = join(pkgRoot, "templates", "delivery", "verify.mjs"); +const { runDelivery } = await import(pathToFileURL(verifier)); test("delivery verifier emits a passed receipt for a compliant bot PR", (t) => { const fixture = makeFixture(t); const discover = runVerifier(fixture, "discover"); - assert.equal(discover.status, 0, discover.stdout + discover.stderr); + assert.equal(discover.status, 0, discover.stderr); const outputs = readFileSync(fixture.outputPath, "utf8"); assert.ok(outputs.includes("pr_number=7")); assert.ok(outputs.includes("head_ref=feature/incident-triage")); @@ -22,7 +23,7 @@ test("delivery verifier emits a passed receipt for a compliant bot PR", (t) => { FACILITY_HEAD_REF: "feature/incident-triage", FACILITY_HEAD_SHA: "new-sha", }); - assert.equal(finalize.status, 0, finalize.stdout + finalize.stderr); + assert.equal(finalize.status, 0, finalize.stderr); const receipt = JSON.parse( readFileSync(join(fixture.runnerTemp, "facility-delivery", "receipt.json"), "utf8"), ); @@ -52,7 +53,7 @@ test("delivery verifier accepts style commits and punctuation in scopes", (t) => ]) { const fixture = makeFixture(t, { message }); const result = runVerifier(fixture, "discover"); - assert.equal(result.status, 0, `${message}\n${result.stdout}${result.stderr}`); + assert.equal(result.status, 0, `${message}\n${result.stderr}`); } }); @@ -65,8 +66,8 @@ test("delivery verifier rejects unknown types and malformed subjects", (t) => { "fix(api)): require scoped credentials", "fix( ): require scoped credentials", "fix(api\tauth): require scoped credentials", - "fix: render \u001b[31mred output", - "fix: render \u009b31mred output", + "fix: render red output", + "fix: render ›31mred output", "feat: summary starts after an extra space", ]) { const fixture = makeFixture(t, { message }); @@ -83,13 +84,38 @@ test("delivery verifier rejects a rewritten existing PR baseline", (t) => { assert.match(result.stderr, /pre-builder head|history was rewritten/); }); +test("hostile ambient environment cannot redirect the verifier's gh", (t) => { + // Regression for the PR #245 review: an earlier workflow step could persist + // variables through $GITHUB_ENV; the shipped script path must ignore any + // ambient runner override and only ever invoke the fixed executable. + const fixture = makeFixture(t); + const marker = join(fixture.dir, "hostile-executed"); + const hostile = join(fixture.dir, "hostile-gh.mjs"); + writeFileSync( + hostile, + `import { writeFileSync } from "node:fs";\nwriteFileSync(${JSON.stringify(marker)}, "pwned");\nconsole.log("{}");\n`, + ); + const spawned = spawnSync(process.execPath, [verifier, "discover"], { + encoding: "utf8", + env: { + ...process.env, + ...fixture.env, + FACILITY_GH_BIN: process.execPath, + FACILITY_GH_ARGS: JSON.stringify([hostile]), + // If the fixed gh is attempted it must die offline, never on the API. + GH_HOST: "gh-stub.invalid", + GH_TOKEN: "stub-only", + }, + }); + assert.ok(!existsSync(marker), "ambient FACILITY_GH_BIN must never be executed"); + assert.notEqual(spawned.status, 0, "fixed gh cannot succeed against gh-stub.invalid"); +}); + function makeFixture(t, overrides = {}) { const dir = mkdtempSync(join(tmpdir(), "facility-delivery-")); t.after(() => rmSync(dir, { recursive: true, force: true })); - const dataPath = join(dir, "data.json"); const eventPath = join(dir, "event.json"); const outputPath = join(dir, "output.txt"); - const ghPath = join(dir, "gh"); const runnerTemp = join(dir, "runner-temp"); const pr = { number: 7, @@ -105,65 +131,58 @@ function makeFixture(t, overrides = {}) { }, user: { login: "claude[bot]", type: "Bot" }, }; - const data = { - pr, - commits: [ - ...(overrides.omitBaseline - ? [] - : [ - { - sha: "base-sha", - commit: { message: "Initial commit", verification: { verified: false } }, - }, - ]), - { - sha: "new-sha", - commit: { - message: overrides.message ?? "feat: add incident triage", - verification: { verified: true }, - }, + const commits = [ + ...(overrides.omitBaseline + ? [] + : [ + { + sha: "base-sha", + commit: { message: "Initial commit", verification: { verified: false } }, + }, + ]), + { + sha: "new-sha", + commit: { + message: overrides.message ?? "feat: add incident triage", + verification: { verified: true }, }, - ], - }; - writeFileSync(dataPath, JSON.stringify(data)); + }, + ]; writeFileSync(eventPath, JSON.stringify({ pull_request: { number: 7 } })); writeFileSync(outputPath, ""); - writeFileSync( - ghPath, - `#!/usr/bin/env node -const data = JSON.parse(require("node:fs").readFileSync(process.env.FAKE_GH_DATA, "utf8")); -const endpoint = process.argv[3] || ""; -if (endpoint.includes("/commits")) process.stdout.write(JSON.stringify(data.commits)); -else process.stdout.write(JSON.stringify(data.pr)); -`, - ); - chmodSync(ghPath, 0o755); - return { dir, dataPath, eventPath, outputPath, runnerTemp, ghPath }; + const env = { + GITHUB_REPOSITORY: "acme/demo", + GITHUB_EVENT_PATH: eventPath, + GITHUB_OUTPUT: outputPath, + RUNNER_TEMP: runnerTemp, + DEFAULT_BRANCH: "main", + FACILITY_STARTED_AT: "2026-07-15T12:00:00Z", + FACILITY_START_SHA: "base-sha", + }; + // The runner is injected at the module boundary — the only seam there is. + const gh = (args) => { + const endpoint = args.find((arg) => arg.startsWith("repos/")) ?? ""; + return JSON.stringify(endpoint.includes("/commits") ? commits : pr); + }; + return { dir, outputPath, runnerTemp, env, gh }; } function runVerifier(fixture, mode, extraEnv = {}) { - return spawnSync(process.execPath, [verifier, mode], { - encoding: "utf8", - env: { - ...process.env, - PATH: `${fixture.dir}${delimiter}${process.env.PATH}`, - // The stub engages through the seam — the only mechanism that works on - // every platform (see the note in verify.mjs). - FACILITY_GH_BIN: process.execPath, - FACILITY_GH_ARGS: JSON.stringify([fixture.ghPath]), - // If the stub ever fails to engage, the real gh must die offline - // instead of reaching api.github.com with the developer's credentials. - GH_HOST: "gh-stub.invalid", - GH_TOKEN: "stub-only", - FAKE_GH_DATA: fixture.dataPath, - GITHUB_REPOSITORY: "acme/demo", - GITHUB_EVENT_PATH: fixture.eventPath, - GITHUB_OUTPUT: fixture.outputPath, - RUNNER_TEMP: fixture.runnerTemp, - DEFAULT_BRANCH: "main", - FACILITY_START_SHA: "base-sha", - FACILITY_STARTED_AT: "2026-07-15T12:00:00Z", - ...extraEnv, - }, - }); + const applied = { ...fixture.env, ...extraEnv }; + const saved = {}; + for (const key of Object.keys(applied)) { + saved[key] = process.env[key]; + process.env[key] = applied[key]; + } + try { + runDelivery(mode, { gh: fixture.gh }); + return { status: 0, stderr: "" }; + } catch (error) { + return { status: 1, stderr: String(error?.message ?? error) }; + } finally { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } } diff --git a/packages/cli/test/doctor-policy.test.mjs b/packages/cli/test/doctor-policy.test.mjs index a4baf3f8..9860d2f9 100644 --- a/packages/cli/test/doctor-policy.test.mjs +++ b/packages/cli/test/doctor-policy.test.mjs @@ -1,10 +1,12 @@ import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; -import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { appendFileSync, existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { delimiter, dirname, join } from "node:path"; import test from "node:test"; -import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; +const resolverPath = join(dirname(fileURLToPath(import.meta.url)), "../templates/doctor/resolve.mjs"); +const { main: resolveMain } = await import(pathToFileURL(resolverPath)); import { classifyFailure, countBranchAttempts, @@ -309,8 +311,8 @@ test("posts a new sensitive-boundary triage after an earlier repair attempt", () assert.equal(decision.action, "triage"); }); -test("resolver integrates with deterministic GitHub fixtures and emits a repair packet", (t) => { - const result = runResolverFixture(t, {}); +test("resolver integrates with deterministic GitHub fixtures and emits a repair packet", async (t) => { + const result = await runResolverFixture(t, {}); assert.equal(result.status, 0, result.stderr); assert.match(result.output, /^action=repair$/m); assert.match(result.output, new RegExp(`^head_sha=${SHA_A}$`, "m")); @@ -327,8 +329,8 @@ test("resolver integrates with deterministic GitHub fixtures and emits a repair assert.match(readFileSync(result.commentLog, "utf8"), /outcome="started"/); }); -test("resolver integration denies a cross-repository repair", (t) => { - const result = runResolverFixture(t, { +test("resolver integration denies a cross-repository repair", async (t) => { + const result = await runResolverFixture(t, { pull: pullRequest({ draft: true, head: { repo: { full_name: "outside/fork" } } }), }); assert.equal(result.status, 0, result.stderr); @@ -337,8 +339,8 @@ test("resolver integration denies a cross-repository repair", (t) => { assert.match(readFileSync(result.commentLog, "utf8"), /cross-repository/); }); -test("resolver integration rejects stale and replayed evidence", (t) => { - const stale = runResolverFixture(t, { +test("resolver integration rejects stale and replayed evidence", async (t) => { + const stale = await runResolverFixture(t, { pull: pullRequest({ draft: true, head: { sha: SHA_B } }), }); assert.equal(stale.status, 0, stale.stderr); @@ -347,7 +349,7 @@ test("resolver integration rejects stale and replayed evidence", (t) => { const fingerprint = classifyFailure(check()).fingerprint; const marker = ``; - const replayed = runResolverFixture(t, { + const replayed = await runResolverFixture(t, { comments: [{ body: marker }, { body: marker }], }); assert.equal(replayed.status, 0, replayed.stderr); @@ -355,27 +357,23 @@ test("resolver integration rejects stale and replayed evidence", (t) => { assert.ok(!replayed.contextExists); }); -test("resolver integration fails closed on malformed events and unavailable GitHub evidence", (t) => { - const malformed = runResolverFixture(t, { eventText: "{" }); +test("resolver integration fails closed on malformed events and unavailable GitHub evidence", async (t) => { + const malformed = await runResolverFixture(t, { eventText: "{" }); assert.equal(malformed.status, 1, malformed.stderr); assert.match(malformed.output, /^action=none$/m); - const unavailable = runResolverFixture(t, { failGithub: true }); + const unavailable = await runResolverFixture(t, { failGithub: true }); assert.equal(unavailable.status, 1, unavailable.stderr); assert.match(unavailable.output, /^action=none$/m); assert.doesNotMatch(unavailable.stdout, /fixture-secret/); }); -function runResolverFixture(t, overrides) { +async function runResolverFixture(t, overrides = {}) { const directory = mkdtempSync(join(tmpdir(), "facility-doctor-test-")); t.after(() => rmSync(directory, { recursive: true, force: true })); - const bin = join(directory, "bin"); const eventPath = join(directory, "event.json"); - const fixturePath = join(directory, "fixtures.json"); const outputPath = join(directory, "github-output.txt"); const commentLog = join(directory, "comments.txt"); - const ghPath = join(bin, "gh"); - mkdirSync(bin); const event = { workflow_run: { id: 10, @@ -394,61 +392,54 @@ function runResolverFixture(t, overrides) { ], "repos/acme/demo/issues/7/comments?per_page=100": [overrides.comments ?? []], }; - writeFileSync(eventPath, overrides.eventText ?? JSON.stringify(event)); - writeFileSync(fixturePath, JSON.stringify(fixtures)); - writeFileSync( - ghPath, - [ - "#!/usr/bin/env node", - 'import { appendFileSync, readFileSync } from "node:fs";', - "if (process.env.FAKE_GH_FAIL === 'true') {", - ' process.stderr.write("GitHub unavailable: fixture-secret\\n");', - " process.exit(1);", - "}", - "const args = process.argv.slice(2);", - 'const endpoint = args.find((arg) => arg.startsWith("repos/"));', - 'if (args.includes("-f")) {', - ' appendFileSync(process.env.FAKE_GH_COMMENT_LOG, args.find((arg) => arg.startsWith("body=")) ?? "");', - ' process.stdout.write("{\\"id\\":123}");', - " process.exit(0);", - "}", - "const fixtures = JSON.parse(readFileSync(process.env.FAKE_GH_FIXTURES, 'utf8'));", - "if (!(endpoint in fixtures)) process.exit(2);", - "process.stdout.write(JSON.stringify(fixtures[endpoint]));", - ].join("\n"), - ); - chmodSync(ghPath, 0o755); writeFileSync(outputPath, ""); writeFileSync(commentLog, ""); - const resolver = join(dirname(fileURLToPath(import.meta.url)), "../templates/doctor/resolve.mjs"); - const spawned = spawnSync(process.execPath, [resolver], { - cwd: directory, - encoding: "utf8", - env: { - ...process.env, - PATH: `${bin}${delimiter}${process.env.PATH}`, - FACILITY_GH_BIN: process.execPath, - FACILITY_GH_ARGS: JSON.stringify([ghPath]), - // Fail offline, never against the real API, if the stub falls through. - GH_HOST: "gh-stub.invalid", - GH_TOKEN: "stub-only", - GITHUB_REPOSITORY: "acme/demo", - GITHUB_EVENT_PATH: eventPath, - GITHUB_OUTPUT: outputPath, - GITHUB_RUN_ID: "900", - FAKE_GH_FIXTURES: fixturePath, - FAKE_GH_COMMENT_LOG: commentLog, - FAKE_GH_FAIL: overrides.failGithub ? "true" : "false", - FACILITY_BOT_LOGIN: "facility-agent", - }, - }); + // The runner is injected at the exported module boundary — the only seam. + const gh = async (args) => { + if (overrides.failGithub) throw new Error("GitHub unavailable: fixture-secret"); + if (args.includes("-f")) { + appendFileSync(commentLog, args.find((arg) => arg.startsWith("body=")) ?? ""); + return '{"id":123}'; + } + const endpoint = args.find((arg) => arg.startsWith("repos/")); + if (!(endpoint in fixtures)) throw new Error(`unfixtured endpoint: ${endpoint}`); + return JSON.stringify(fixtures[endpoint]); + }; + + const savedEnv = {}; + const applied = { + GITHUB_REPOSITORY: "acme/demo", + GITHUB_EVENT_PATH: eventPath, + GITHUB_OUTPUT: outputPath, + GITHUB_RUN_ID: "900", + FACILITY_BOT_LOGIN: "facility-agent", + }; + for (const key of Object.keys(applied)) { + savedEnv[key] = process.env[key]; + process.env[key] = applied[key]; + } + const savedCwd = process.cwd(); + const savedExit = process.exitCode; + process.exitCode = 0; + process.chdir(directory); + try { + await resolveMain({ gh }); + } finally { + process.chdir(savedCwd); + for (const [key, value] of Object.entries(savedEnv)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } + const status = process.exitCode ? 1 : 0; + process.exitCode = savedExit ?? 0; return { - status: spawned.status, - stdout: spawned.stdout, - stderr: spawned.stderr, + status, + stdout: "", + stderr: "", output: readFileSync(outputPath, "utf8"), directory, commentLog, @@ -462,3 +453,42 @@ function runResolverFixture(t, overrides) { })(), }; } + +test("hostile ambient environment cannot redirect the resolver's gh", (t) => { + // PR #245 review regression: $GITHUB_ENV-persisted variables must not be + // able to swap the executable the shipped resolver runs with GH_TOKEN. + const directory = mkdtempSync(join(tmpdir(), "facility-doctor-hostile-")); + t.after(() => rmSync(directory, { recursive: true, force: true })); + const marker = join(directory, "hostile-executed"); + const hostile = join(directory, "hostile-gh.mjs"); + writeFileSync( + hostile, + `import { writeFileSync } from "node:fs";\nwriteFileSync(${JSON.stringify(marker)}, "pwned");\nconsole.log("{}");\n`, + ); + const eventPath = join(directory, "event.json"); + writeFileSync( + eventPath, + JSON.stringify({ workflow_run: { id: 10, event: "pull_request", head_sha: SHA_A } }), + ); + const outputPath = join(directory, "github-output.txt"); + writeFileSync(outputPath, ""); + const spawned = spawnSync(process.execPath, [resolverPath], { + cwd: directory, + encoding: "utf8", + env: { + ...process.env, + FACILITY_GH_BIN: process.execPath, + FACILITY_GH_ARGS: JSON.stringify([hostile]), + GH_HOST: "gh-stub.invalid", + GH_TOKEN: "stub-only", + GITHUB_REPOSITORY: "acme/demo", + GITHUB_EVENT_PATH: eventPath, + GITHUB_OUTPUT: outputPath, + GITHUB_RUN_ID: "900", + FACILITY_BOT_LOGIN: "facility-agent", + }, + }); + assert.ok(!existsSync(marker), "ambient FACILITY_GH_BIN must never be executed"); + assert.notEqual(spawned.status, 0, "resolver must fail closed without reachable gh"); + assert.match(spawned.stdout + spawned.stderr, /fail(ed)? closed|doctor: none/i); +}); From 135a322790fd6a56d1408b795f4f3c695c0fe990 Mon Sep 17 00:00:00 2001 From: Carlos Santana <1540596+ophiocus@users.noreply.github.com> Date: Tue, 1 Sep 2026 12:05:17 -0500 Subject: [PATCH 4/4] =?UTF-8?q?ci:=20retrigger=20checks=20=E2=80=94=20no?= =?UTF-8?q?=20run=20was=20created=20for=20the=20previous=20head?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5