Skip to content

arch-docs: run git without a shell so ^{commit} survives Windows - #1969

Merged
Birfy merged 2 commits into
mainfrom
dev/1954-arch-docs-gate-no-shell
Nov 1, 2026
Merged

Birfy merged 2 commits into
mainfrom
dev/1954-arch-docs-gate-no-shell

Conversation

@Birfy

@Birfy Birfy commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #1954

Owner: Chief
Reviewer: Reviewer
Written-by: Claim 2f6d
Branch: dev/1954-arch-docs-gate-no-shell
Base: beba27a
Tip SHA: c722893
Docs-not-needed: no — docs/subsystems/install.md owns core/scripts/ and core/test/check-arch-docs.test.js, so its "Key files" row is updated in this tree.

The arch-docs gate built four git command lines as strings and ran them through a shell. On Windows that shell is cmd.exe, where ^ escapes the next character: git rev-parse --verify origin/main^{commit} reached git as origin/main{commit}, rev-parse failed, and assertBaseRef's catch reported cannot diff against origin/main on a checkout that had origin/main (Triage re-read the log: * [new ref] 88aaf3c7… -> origin/main and git rev-parse refs/remotes/origin/main → 88aaf3c7… in the same job). Every scheduled windows-latest row failed its Lint step that way from 2026-10-17 on, while npm run lint itself reported 0 errors.

The gate now hands git its arguments as a list — execFileSync('git', ['rev-parse', '--verify', ${ref}^{commit}]) and the same for diff --name-only <ref>...HEAD, diff --name-only <ref>...HEAD -- <file> and log <ref>..HEAD --pretty=%B — so ^{commit} is one argv element and no shell interprets it. The ref is still taken from ARCH_DOCS_BASE / origin/main unchanged and the fail-closed path is untouched: a genuinely missing base ref still throws cannot diff against <ref>.

What changed

Step Before After
base ref check execSync('git rev-parse --verify ' + ref + '^{commit}') execFileSync('git', ['rev-parse','--verify', ref + '^{commit}'])
changed files execSync('git diff --name-only <ref>...HEAD') execFileSync('git', ['diff','--name-only', '<ref>...HEAD'])
one file execSync('git diff --name-only <ref>...HEAD -- <file>') execFileSync('git', ['diff','--name-only', '<ref>...HEAD', '--', file])
trailer execSync('git log <ref>..HEAD --pretty=%B') execFileSync('git', ['log', '<ref>..HEAD', '--pretty=%B'])

No output format, exit code, message or call site changes; execSync is gone from the file and the import is now execFileSync.

Verification at the tip

Tip SHA: c722893
Ran: cd core && node --test test/check-arch-docs.test.js → 7/7 pass (the six existing cases, including invalid base ref fails closed and empty diff passes, plus the new one); node scripts/check-arch-docs.mjs → arch-docs: OK exit 0; node scripts/ds-bot-patches.mjs check → ok 10 patches apply cleanly in order / ok core/ equals ds-bot/ + patches/ds-bot/; node .github/scripts/changelog.mjs check → 6 fragments well formed.

The two patches/ds-bot/files/ entries are byte copies of the tip core/ files (both are files upstream ds-bot does not have, so generate stores them verbatim); this machine has no git, so generate could not run and the copies were made by hand and then proved by check above. npm run verify was not run here for the same reason (generate/git absent); CI runs it.
Not run (and why): the scheduled Compatibility run itself — that needs a dispatch after merge, see next.

Red first

The new case the gate never runs git through a shell puts a fake git first on PATH in a temp dir. The fake records the name of its parent process and exits non-zero when that parent is a shell (sh/dash/bash/zsh/ksh/cmd*); otherwise it answers rev-parse with a sha and diff/log with nothing. The test then runs the real gate with spawnSync(process.execPath, ['scripts/check-arch-docs.mjs']) and asserts the parent was not a shell and the gate exited 0.

Against the base blob (core/scripts/check-arch-docs.mjs from beba27a77e9ca453a1c357fff497c6a500d4be0e, this branch's test file):

not ok 7 - the gate never runs git through a shell
  error: 'git was started by a shell (sh), so ^{commit} is mangled on Windows'
# pass 6
# fail 1

Against this tip, same command: # pass 7 / # fail 0. The case pins the class — "git was started by a shell" — not one Windows symptom, so it stays red for any future execSync that creeps back in.

Checkpoint

{
  "pr": 1969,
  "claim": "Chief",
  "issue": 1954,
  "summary": "The arch-docs gate ran four git commands through a shell; on Windows cmd.exe ate the ^ in origin/main^{commit}, so rev-parse failed and every scheduled windows-latest row reported cannot diff against origin/main. It now passes git its arguments as a list with the ref verbatim, so no shell interprets ^{commit}. Fail-closed on a genuinely missing base ref is unchanged. install.md records the rule and a new test pins it.",
  "files": ["core/scripts/check-arch-docs.mjs", "core/test/check-arch-docs.test.js", "patches/ds-bot/files/scripts/check-arch-docs.mjs", "patches/ds-bot/files/test/check-arch-docs.test.js", "docs/subsystems/install.md", "changelog.d/1954-arch-docs-no-shell.fixed.md"],
  "checks": { "ran": "cd core && node --test test/check-arch-docs.test.js && node scripts/check-arch-docs.mjs; node scripts/ds-bot-patches.mjs check; node .github/scripts/changelog.mjs check", "result": "green", "at": "c722893f6967ec93d42f2623fe8415fb395ae05b" },
  "next": "After merge, dispatch nightly.yml on main and confirm the windows-latest · Node 24 row's Lint step is green — that is the only place the cmd.exe path runs."
}

Birfy added 2 commits November 1, 2026 10:34
core/scripts/check-arch-docs.mjs built four git commands as strings and ran
them through the shell (execSync). On Windows that shell is cmd.exe, where
`^` escapes the next character, so origin/main^{commit} arrived at git as
origin/main{commit}; rev-parse failed and the gate reported
"cannot diff against origin/main" on a checkout where origin/main existed —
every scheduled windows-latest row failed the Lint step from 2026-10-17 on.

Run git with its arguments as a list (execFileSync) instead, the ref verbatim
as one argument. The fail-closed behaviour on a genuinely missing base ref is
unchanged: core/test/check-arch-docs.test.js keeps "invalid base ref fails
closed" and gains "the gate never runs git through a shell", which fails on
the old source (git started by sh) and passes on this one.

Fixes #1954
…hell line

Docs-not-needed: updates the owning page docs/subsystems/install.md itself.
@Birfy

Birfy commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at c722893
Written-by: Reviewer
Attestation: c722893 role=Reviewer

Ran at this tip:

$ PATH=$W/gitstub:$PATH TMPDIR=$W/tmp node --test core/test/check-arch-docs.test.js
# tests 7  # pass 7  # fail 0
$ node -e 'restore core/scripts/check-arch-docs.mjs from base blob beba27a77e9c'; PATH=$W/gitstub:$PATH TMPDIR=$W/tmp node --test core/test/check-arch-docs.test.js
# tests 7  # pass 6  # fail 1

Review notes:

  • Red-first (required): with core/scripts/check-arch-docs.mjs swapped back to the base blob (beba27a77e9c, execSync), test 7 the gate never runs git through a shell fails with git was started by a shell (sh), so ^{commit} is mangled on Windows; the other 6 stay green, so the new test pins exactly this fix and nothing else.
  • Fix: execSync("git rev-parse --verify ${ref}^{commit}") → execFileSync('git', ['rev-parse','--verify',${ref}^{commit}]), same for diff/log (the -- separator for the per-file diff is now an argv element, not string-joined). Fail-closed on a bad base ref is unchanged (assertBaseRef still throws cannot diff against <ref>, covered by test 5).
  • Scope vs base: 6 files, all ci: the Compatibility job fails the whole lint step because arch-docs cannot diff against origin/main #1954 — core/scripts/check-arch-docs.mjs +5/−5, core/test/check-arch-docs.test.js +52/−2, the two patches/ds-bot/files/ mirrors (byte-identical to the core files at this tip, verified), docs/subsystems/install.md (the owned doc for this path, so the gate itself is satisfied), and changelog.d/1954-arch-docs-no-shell.fixed.md. No unrelated edits.
  • CI at this tip: tests-ok success, judge/agent-audience/plugins (*)/package/ubuntu-latest · Node 24/reviewed tip/merge-gate success; only enqueue-guard is red, which is expected until a PASS exists at this tip. No other failures.
  • Body metadata: Reviewer: Reviewer (≠ Owner: Chief), label agent-pr present, checkpoint block 1207 chars / summary 421 / files 6 / checks green at this tip — within limits, nothing to patch.
  • Environment honesty: this box has no git binary and /tmp is mounted noexec, so the two git-dependent tests cannot use the real thing here. I put a minimal stub git on PATH ($W/gitstub) that answers rev-parse (exit 128 for nonexistent-review-base), empty diff/log, and ran with TMPDIR on an exec-capable disk. That is enough for tests 4–6 (they assert on the gate's own logic), and test 7 uses its own fake git prepended to PATH, so it is unaffected. The same suite is green on the CI runner with real git (tests-ok above).

@github-actions github-actions Bot added the pass label Nov 1, 2026
@Birfy
Birfy added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit e2ce7e1 Nov 1, 2026
13 of 15 checks passed
@Birfy
Birfy deleted the dev/1954-arch-docs-gate-no-shell branch November 1, 2026 02:56
@github-actions github-actions Bot removed the agent-pr Pull request opened by an agent label Nov 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci: the Compatibility job fails the whole lint step because arch-docs cannot diff against origin/main

1 participant