Skip to content

Commit d4401f7

Browse files
claude[bot]claude
andauthored
fix(tooling): isolate the throwaway git repos in the two hook/driver-invoked scripts (#16811)
Git exports GIT_DIR / GIT_WORK_TREE / GIT_INDEX_FILE into every child it runs and those outrank `cwd`, so a `git init` / `git config` / `git add` aimed at a temp directory lands on the REAL repository whenever these two files run under a hook or as the merge driver. Both are exactly that: git-merge-regen.mjs IS the `merge=os-regen` driver, and check-regen-pending.mjs is what .githooks/pre-commit and .githooks/pre-push invoke. Every spawn site is classified rather than blanket-stripped. Children whose subject is a repository their own `cwd` and arguments name take `gitFreeEnv()` from scripts/git-env.mjs; the three whose subject is the merge git is currently performing keep the ambient environment and carry a note saying why stripping them would be a behaviour change in the dangerous direction. Neither file spawns a network child, measured with a control. A new self-test battery in each file proves it behaviourally: a disposable stand-in repository is created, the three variables are pointed at it, and the file's whole --self-test is re-entered under that environment. The stand-in must come back byte-identical in config, index, refs and HEAD, and a firing control asserts the leaked environment actually reached the child first. Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU Co-authored-by: Claude <noreply@anthropic.com>
1 parent 68fd85a commit d4401f7

2 files changed

Lines changed: 407 additions & 27 deletions

File tree

‎scripts/check-regen-pending.mjs‎

Lines changed: 194 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -58,20 +58,40 @@
5858
* the build is newer than the sources, because a phantom "breaking removal" has
5959
* cost real triage time before (#4687, and the trap is recorded in AGENTS.md).
6060
*
61+
* ## Git environment isolation (#16753)
62+
*
63+
* `.githooks/pre-commit` and `.githooks/pre-push` invoke this file, so git
64+
* exports `GIT_DIR` / `GIT_WORK_TREE` / `GIT_INDEX_FILE` into it -- the exact
65+
* path #16624 travelled -- and those outrank `cwd` in every `git` child it
66+
* spawns. Each spawn site is classified against the boundary
67+
* `scripts/git-env.mjs` carries:
68+
*
69+
* - a child whose subject is a repository its OWN `cwd` and arguments name --
70+
* the throwaway fixture, and the read against this checkout's index -- is
71+
* spawned with `env: gitFreeEnv()`, as is the node child `runHook` spawns
72+
* INSIDE the fixture, which runs git children of its own.
73+
* - `gitDirPath()` and `mergeInProgress()`'s `rev-parse HEAD` keep the ambient
74+
* environment: their subject is the commit or push git is performing right
75+
* now, so the exported variables ARE the question. Each carries a ⛔ note.
76+
* - a NETWORK child would keep the ambient environment too (`GIT_CONFIG_*`
77+
* and `GIT_SSL_CAINFO` carry its transport). This file spawns none:
78+
* measured with a control on #16753.
79+
*
6180
* Usage:
6281
* node scripts/check-regen-pending.mjs # pre-commit
6382
* node scripts/check-regen-pending.mjs --pre-push # pre-push: never defers
6483
* node scripts/check-regen-pending.mjs --self-test # fixtures only, no repo state
6584
*/
6685

6786
import { execFileSync, execSync, spawnSync } from 'node:child_process';
68-
import { appendFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, statSync, writeFileSync } from 'node:fs';
87+
import { appendFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, realpathSync, rmSync, statSync, writeFileSync } from 'node:fs';
6988
import { tmpdir } from 'node:os';
7089
import { dirname, join, resolve } from 'node:path';
7190
import { fileURLToPath } from 'node:url';
7291

7392
import { PENDING_MARKER, entryForPath, ownerDir, ownerOf, ownerRunCommand } from './regen-artifacts.mjs';
7493
import { inspectBuildStamp, inspectDeclarationStamp } from './build-input-hash.mjs';
94+
import { gitFreeEnv } from './git-env.mjs';
7595
import { isEntrypoint } from './invoked-as.mjs';
7696
import {
7797
EXIT_PREREQUISITE_NOT_MET,
@@ -353,6 +373,10 @@ export function bundlesAreStale(specDir = SPEC_DIR) {
353373
* two agents merging in parallel cannot collect each other's debt.
354374
*/
355375
function gitDirPath() {
376+
// ⛔ AMBIENT ENVIRONMENT ON PURPOSE -- never `gitFreeEnv()` here (#16753).
377+
// This runs from a hook, and the repository git is committing or pushing is
378+
// the one it exported; the marker belongs in THAT git dir. Deriving it from
379+
// `cwd` instead would put the deferral's record somewhere nothing collects it.
356380
return execFileSync('git', ['rev-parse', '--absolute-git-dir'], { encoding: 'utf8' }).trim();
357381
}
358382

@@ -389,6 +413,8 @@ export function mergeInProgress(gitDir) {
389413
const mergeHeads = readFileSync(file, 'utf8').split('\n').map((l) => l.trim()).filter(Boolean);
390414
let head = '';
391415
try {
416+
// ⛔ Ambient, for `gitDirPath()`'s reason: the HEAD being recorded is the one
417+
// of the merge git is completing right now (#16753).
392418
head = execFileSync('git', ['rev-parse', 'HEAD'], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }).trim();
393419
} catch {
394420
head = '(root)'; // A merge as the first commit has no HEAD. Vanishingly rare, not an error.
@@ -825,7 +851,7 @@ const invokedDirectly = isEntrypoint(import.meta.url);
825851
// -- Why the CALLEE NAME is the battery --
826852
//
827853
// This file has no `selfTest()` entry function: the `--self-test` dispatch at
828-
// the bottom invokes THREE named callees, each printing its own section and
854+
// the bottom invokes FOUR named callees, each printing its own section and
829855
// returning a boolean. So the roster's unit is the CALLEE, and its label is the
830856
// one the SOURCE ALREADY CARRIES -- the function's own name. Nothing is invented
831857
// and nothing is judged per comment, and a set difference names WHICH sub-check
@@ -835,15 +861,15 @@ const invokedDirectly = isEntrypoint(import.meta.url);
835861
// is why each floor is 1 rather than the number of assertions the callee
836862
// happens to contain.
837863
//
838-
// STOP -- the three callees' own inner sinks are NOT batteries here, and they
864+
// STOP -- the four callees' own inner sinks are NOT batteries here, and they
839865
// are worth naming because all three are different shapes: `fixtureSelfTest`'s
840866
// `check()` helper, `decisionTableSelfTest`'s literal table with its driving
841867
// loop, and `prePushIsArmedSelfTest`'s bare boolean. Recipe A (PR #15271,
842868
// `check-sdui-manifest`) does make a table row a battery -- for a file whose
843869
// SELF-TEST *is* the table: one literal table, one driving loop over it, one
844870
// sink. Here the table is a local of ONE callee among three, and that callee
845871
// already reduces its rows to a single returned verdict of its own. Flooring
846-
// those rows would floor one callee's internals while the other two stayed at
872+
// those rows would floor one callee's internals while the other three stayed at
847873
// callee granularity -- a roster whose unit changes per entry. The rule: the
848874
// battery is the unit the DISPATCH names.
849875
//
@@ -867,6 +893,7 @@ const SELF_TEST_BATTERIES = Object.freeze({
867893
prePushIsArmedSelfTest: 1,
868894
decisionTableSelfTest: 1,
869895
fixtureSelfTest: 1,
896+
ambientGitEnvIsolation: 1,
870897
});
871898

872899
// DELETING an entry silences that battery's floor exactly as effectively as
@@ -875,7 +902,7 @@ const SELF_TEST_BATTERIES = Object.freeze({
875902
// key in the literal above, so the roster falls below this number; the
876903
// roster/dispatch cross-check in `batteryFloorFailures()` is the other half, and
877904
// it names WHICH callee was listed twice.
878-
const SELF_TEST_BATTERY_FLOOR = 3;
905+
const SELF_TEST_BATTERY_FLOOR = 4;
879906

880907
// The key a registration is filed under when a callee registers no name at all.
881908
// It is not a declared battery, so it reds by the same set difference rather
@@ -885,7 +912,7 @@ const UNATTRIBUTED_BATTERY = '(no callee named)';
885912
// The battery ledger, read by `batteryFloorFailures()` from the dispatch block
886913
// at the very bottom of this file. It is MODULE-level rather than local to a
887914
// self-test body because this file HAS no self-test body: the registrations
888-
// happen inside three separate callees and the floor is read at the dispatch's
915+
// happen inside four separate callees and the floor is read at the dispatch's
889916
// verdict site, so the ledger has to outlive every one of those frames.
890917
//
891918
// Named for the roster's role, deliberately WITHOUT a self-test spelling:
@@ -899,7 +926,7 @@ const batterySeen = new Map();
899926
/**
900927
* Record that a self-test callee RAN.
901928
*
902-
* Called as the FIRST statement of each of the three callees the `--self-test`
929+
* Called as the FIRST statement of each of the four callees the `--self-test`
903930
* dispatch invokes -- above any early return, so a callee that bails out early
904931
* still reports that it ran, and the floor is never met by a frame that returned
905932
* before doing anything.
@@ -912,7 +939,7 @@ function registerCase(name) {
912939
/**
913940
* The floor: every declared callee RAN (#13799).
914941
*
915-
* Evaluated at the dispatch's verdict site -- after all three callees have had
942+
* Evaluated at the dispatch's verdict site -- after all four callees have had
916943
* their chance and immediately before the success line -- and reached only from
917944
* the `--self-test` branch, so a production `pre-commit` / `pre-push` run never
918945
* reads the ledger at all.
@@ -1006,7 +1033,7 @@ function fixtureSelfTest() {
10061033
registerCase('fixtureSelfTest');
10071034
const dir = mkdtempSync(join(tmpdir(), 'os-regen-defer-'));
10081035
const git = (args, opts = {}) =>
1009-
execFileSync('git', args, { cwd: dir, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], ...opts });
1036+
execFileSync('git', args, { cwd: dir, encoding: 'utf8', env: gitFreeEnv(), stdio: ['ignore', 'pipe', 'pipe'], ...opts });
10101037
const results = [];
10111038
const check = (label, cond) => {
10121039
results.push(cond);
@@ -1086,10 +1113,17 @@ function fixtureSelfTest() {
10861113
// `spawnSync`, not `execFileSync`: every message this script prints goes to
10871114
// STDERR, which execFileSync returns only on the failure path — capturing the
10881115
// accept-path wording is half of what is under test here.
1116+
// ⚠️ `gitFreeEnv()`, not `process.env` (#16753). This child is not a `git`
1117+
// child, but it RE-ENTERS THIS FILE inside the fixture and spawns git
1118+
// children of its own -- `gitDirPath()` first among them, which is ambient
1119+
// by design. Handing it an inherited `GIT_DIR` therefore points the whole
1120+
// deferral state machine at the REAL clone: the marker is read, appended to
1121+
// and deleted in the shared git dir, from a case whose entire subject is a
1122+
// throwaway repository.
10891123
const r = spawnSync(process.execPath, [fileURLToPath(import.meta.url), ...args], {
10901124
cwd: dir,
10911125
encoding: 'utf8',
1092-
env: { ...process.env, OS_REGEN_GATE_CWD: dir },
1126+
env: { ...gitFreeEnv(), OS_REGEN_GATE_CWD: dir },
10931127
});
10941128
return { code: r.status ?? 1, out: `${r.stdout ?? ''}${r.stderr ?? ''}` };
10951129
};
@@ -1294,6 +1328,7 @@ function prePushIsArmedSelfTest() {
12941328
mode = execFileSync('git', ['ls-files', '-s', '.githooks/pre-push'], {
12951329
cwd: REPO_ROOT,
12961330
encoding: 'utf8',
1331+
env: gitFreeEnv(),
12971332
stdio: ['ignore', 'pipe', 'pipe'],
12981333
}).trim().split(/\s+/)[0] ?? '';
12991334
} catch (err) {
@@ -1338,6 +1373,153 @@ function decisionTableSelfTest() {
13381373
return ok;
13391374
}
13401375

1376+
/* The recursion fuse for `ambientGitEnvIsolation` below. The probe re-enters
1377+
* this file's own `--self-test`, and the inner run must not re-enter it again.
1378+
*
1379+
* ⛔ Deliberately NOT a `GIT_`-prefixed name: `gitFreeEnv()` strips every one of
1380+
* those, so a `GIT_`-spelled fuse would be blown open by the very call this
1381+
* battery exists to verify -- and the probe would recurse until the box ran out.
1382+
*/
1383+
const AMBIENT_ISOLATION_FUSE = 'OS_REGEN_AMBIENT_ISOLATION_INNER';
1384+
1385+
/**
1386+
* ⭐ The throwaway repository `fixtureSelfTest` builds must be isolated from the
1387+
* AMBIENT one (#16753 -- Tier A of #16644; the incident is #16624).
1388+
*
1389+
* `.githooks/pre-commit` and `.githooks/pre-push` invoke this file, so git
1390+
* exports `GIT_DIR` / `GIT_WORK_TREE` / `GIT_INDEX_FILE` into it -- and those
1391+
* outrank `cwd` in every `git` child it spawns. A `git init` / `git config` /
1392+
* `git add` aimed at the fixture's temp directory therefore lands on the REAL
1393+
* repository. Measured, on this box: 8,190 paths staged as deleted in the shared
1394+
* index and `core.bare = true` written into the `.git/config` every linked
1395+
* worktree reads, from a self-test that printed `ok` throughout.
1396+
*
1397+
* ⛔ THE STAND-IN IS DISPOSABLE AND MUST STAY THAT WAY. Pointing these variables
1398+
* at the checkout this file lives in would not TEST the incident, it would BE
1399+
* the incident -- for every agent sharing the clone. A throwaway repository
1400+
* demonstrates the property just as well: what is under test is that an ambient
1401+
* `GIT_DIR` outranks `cwd`, and any real repository shows that.
1402+
*
1403+
* The probe re-enters the WHOLE `--self-test` rather than one helper, because
1404+
* the property is a property of every spawn site at once -- including the node
1405+
* child `runHook` spawns, which re-enters this file INSIDE the fixture and runs
1406+
* git children of its own. A per-site assertion is exactly the thing a new spawn
1407+
* site can be added without.
1408+
*
1409+
* ⚠️ MEASURED, and the reason the fingerprint is not the index alone: with all
1410+
* three variables set, git resolves the work tree to the stand-in, so `git add
1411+
* -A` re-adds the stand-in's OWN files and the index comes back unchanged --
1412+
* `scripts/git-env.mjs` records the same asymmetry. What moves under all three
1413+
* is the CONFIG, and then the fixture's commits move refs and HEAD. So the
1414+
* fingerprint is config + index + refs + HEAD, and a leak has to move none.
1415+
*/
1416+
function ambientGitEnvIsolation() {
1417+
registerCase('ambientGitEnvIsolation');
1418+
if (process.env[AMBIENT_ISOLATION_FUSE]) {
1419+
console.log(' ✓ inner run -- the outer run is the probe');
1420+
return true;
1421+
}
1422+
1423+
const standIn = mkdtempSync(join(tmpdir(), 'os-regen-standin-'));
1424+
const elsewhere = mkdtempSync(join(tmpdir(), 'os-regen-elsewhere-'));
1425+
const same = (a, b) => {
1426+
try {
1427+
return realpathSync(a) === realpathSync(b);
1428+
} catch {
1429+
return a === b;
1430+
}
1431+
};
1432+
try {
1433+
const standInGit = (...args) => execFileSync('git', args, {
1434+
cwd: standIn,
1435+
encoding: 'utf8',
1436+
env: gitFreeEnv(),
1437+
stdio: ['ignore', 'pipe', 'pipe'],
1438+
});
1439+
standInGit('init', '-q', '-b', 'main', '.');
1440+
standInGit('config', 'user.email', 'stand-in@objectstack.invalid');
1441+
standInGit('config', 'user.name', 'os-regen stand-in');
1442+
writeFileSync(join(standIn, 'tracked.txt'), 'the file the stand-in tracks\n');
1443+
standInGit('add', '-A');
1444+
standInGit('commit', '-qm', 'the commit the stand-in would lose');
1445+
const standInGitDir = standInGit('rev-parse', '--absolute-git-dir').trim();
1446+
1447+
const leaky = {
1448+
...process.env,
1449+
GIT_DIR: standInGitDir,
1450+
GIT_WORK_TREE: standIn,
1451+
GIT_INDEX_FILE: join(standInGitDir, 'index'),
1452+
[AMBIENT_ISOLATION_FUSE]: '1',
1453+
};
1454+
1455+
/* -- The firing control, and it is the case that matters --
1456+
* "The stand-in is untouched" is also what an environment that never reached
1457+
* the child produces. So before any verdict is read out of the fingerprint,
1458+
* a plain `git` child spawned in an UNRELATED directory has to answer with
1459+
* the stand-in's git dir -- i.e. the leak is live, here, now. */
1460+
const seen = execFileSync('git', ['rev-parse', '--absolute-git-dir'], {
1461+
cwd: elsewhere,
1462+
encoding: 'utf8',
1463+
env: leaky,
1464+
stdio: ['ignore', 'pipe', 'pipe'],
1465+
}).trim();
1466+
if (!same(seen, standInGitDir)) {
1467+
console.log(
1468+
' ✗ the leaked git environment never reached the child, so "the stand-in is untouched"\n'
1469+
+ ` would prove nothing: a git child in ${elsewhere} answered ${seen}`,
1470+
);
1471+
return false;
1472+
}
1473+
1474+
const fingerprint = () => JSON.stringify({
1475+
config: readFileSync(join(standInGitDir, 'config'), 'utf8'),
1476+
index: standInGit('ls-files'),
1477+
refs: standInGit('for-each-ref', '--format=%(refname) %(objectname)'),
1478+
head: readFileSync(join(standInGitDir, 'HEAD'), 'utf8'),
1479+
}, null, 1);
1480+
1481+
const before = fingerprint();
1482+
const inner = spawnSync(process.execPath, [fileURLToPath(import.meta.url), '--self-test'], {
1483+
cwd: REPO_ROOT,
1484+
encoding: 'utf8',
1485+
env: leaky,
1486+
});
1487+
const after = fingerprint();
1488+
1489+
if (after !== before) {
1490+
console.log(
1491+
' ✗ ⛔ a `--self-test` run under an ambient GIT_DIR / GIT_WORK_TREE / GIT_INDEX_FILE WROTE\n'
1492+
+ ' THE REPOSITORY THOSE VARIABLES NAME. On the shared checkout that is #16624: 8,190\n'
1493+
+ ' paths staged as deleted and core.bare=true in the config every worktree reads.\n'
1494+
+ ' Every throwaway-repo `git` child owes `gitFreeEnv()` from scripts/git-env.mjs.\n'
1495+
+ ` stand-in BEFORE: ${before}\n`
1496+
+ ` stand-in AFTER: ${after}`,
1497+
);
1498+
return false;
1499+
}
1500+
if (inner.status !== 0) {
1501+
console.log(
1502+
` ✗ the inner --self-test exited ${inner.status} under a leaked git environment. The stand-in\n`
1503+
+ ' is byte-identical, so this is not the leak -- it is the run itself failing.\n'
1504+
+ ` ${`${inner.stdout ?? ''}${inner.stderr ?? ''}`.trim().split('\n').slice(-14).join('\n ')}`,
1505+
);
1506+
return false;
1507+
}
1508+
1509+
console.log(
1510+
' ✓ a full --self-test under a leaked GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE left the disposable'
1511+
+ '\n stand-in repository byte-identical (config, index, refs, HEAD)',
1512+
);
1513+
return true;
1514+
} catch (err) {
1515+
console.log(` ✗ ambient-git-env isolation could not run: ${err?.stderr?.toString() || err?.message || err}`);
1516+
return false;
1517+
} finally {
1518+
rmSync(standIn, { recursive: true, force: true });
1519+
rmSync(elsewhere, { recursive: true, force: true });
1520+
}
1521+
}
1522+
13411523
if (invokedDirectly) {
13421524
if (process.argv.includes('--self-test')) {
13431525
// Touches no repo state: the interesting logic is the staleness rules, and
@@ -1353,7 +1535,7 @@ if (invokedDirectly) {
13531535
const noTree = schemaTreeIsStale(join(REPO_ROOT, 'scripts')) === true;
13541536
console.log(`${noTree ? '✓' : '✗'} a directory with no json-schema/ reads as STALE (conservative default)`);
13551537

1356-
// The three callees as a literal LIST rather than three bare calls, so the
1538+
// The four callees as a literal LIST rather than four bare calls, so the
13571539
// names this block invokes are data the floor below can cross-check the
13581540
// roster against, in both directions. The names are read off the function
13591541
// declarations themselves (`fn.name`), so a renamed callee moves this list
@@ -1372,6 +1554,7 @@ if (invokedDirectly) {
13721554
['\ndeferred-merge collection point:', prePushIsArmedSelfTest],
13731555
['\ndeferred-merge decision table:', decisionTableSelfTest],
13741556
['\ndeferred-merge sequence, replayed on a throwaway repo:', fixtureSelfTest],
1557+
['\nambient git environment isolation, on a disposable stand-in:', ambientGitEnvIsolation],
13751558
];
13761559
const results = callees.map(([banner, run]) => {
13771560
console.log(banner);

0 commit comments

Comments
 (0)