Repository navigation
git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] - #927
jayden1711 wants to merge 29 commits into
Conversation
…ts off refs/replace/* and $GIT_DIR/info/grafts are repo state every worktree shares, so an agent can write either, and every git the engine ran there read the agent's stand-in: a `git replace` of the pushed commit hid a refused file and a big one from pushProblems (2 problems became 0), and a graft that made the base reach the head emptied the range (2 became 0). `--no-replace-objects` alone still honours grafts (measured on git 2.43). src/os/git.ts is now the one way src/ runs git: every call gets `--no-replace-objects` and GIT_NO_REPLACE_OBJECTS=1, and GIT_GRAFT_FILE names a path that can't exist, set last over the caller's environment. A runner that takes any command (the setup probes' and init's run) gives git the settings by its name (commandSpawnSync). scripts/lib/git.cjs holds the same settings for scripts; the files that run on their own (the push gate and its hook, new-deps, the test-run VM script) write them out, and the hook hands them to the gate's environment. gitleaks, which runs git itself, gets them in its environment, in the engine's scan and in the push gate. Display and prompt readers follow the issue: the owner's decision diff, the fix-forward culprit diff and the patch hash read every file's lines whatever the change's attributes say (--text, no textconv, no external diff). The evaluator's sources line (src/roles.ts) needs the owner's role eval and the no-newline size count needs the owner's formula: both stay for the owner. test/git-runner.test.ts fails on a git call in src/ or scripts/ that doesn't come through a runner (a scan, a backstop next to the runner, with its exceptions registered as a never-grow table), and is locked with the runner itself. Test (fails without): test/git-runner.test.ts: the replace route and the graft route on pushProblems (each 2 problems became 0 on main), the runner scan (81 files bypassed it on main), the gitleaks and push-gate replace routes. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
#883 moved every child_process import in src/ to src/os/child-process.ts. Each conflict was an import line: the git calls keep the git runner, the other calls take #883's module, and src/os/git.ts now starts its processes through src/os/child-process.ts too (test/child-process.test.ts's backstop). The guard's tree lists both modules, the locked tests list both PRs' entries, and the root updater's new ls-remote call is counted in the git runner scan. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…the test header says what it measures (#807) With a type-only import first, tsc dropped auto-merge-resume.ts's leading comment from its build (measured: 1,428 bytes gone from dist/src/auto-merge-resume.js). The value import goes first again. test/git-runner.test.ts's header now gives the counts its tests measure (2 problems became 0, for each route). Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…927) The release note for the git runner: what an upgrade changes (a new patch_hash for the same change, the optional setup:hooks rerun) and the guardrail it adds. Test (fails without): none, release notes only; test/changelog*.test.ts pass with it. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
The same lock, at another place in each list: next to the push tests in src/review.ts and in engine-guards, so it doesn't sit on the lines #927 adds its own entry to (git merge-tree measured a conflict there). Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Evaluator verdict: CHANGES NEEDED (round 1/2)Final check of PR #927 at head a8d9c98 ( The runner itself holds up. Every way I tried to turn its settings back off failed. The blocker is CI: it is red at this head because of this PR. Six test files fail where fixtures or expectations weren't updated for the runner. 1. Replace and graft routes
Fail-without (measured): I wrote 2. Can a caller turn the settings off? (each one probed through
|
CI was red at a8d9c98 on six files that expected the old argv or copied fixed file lists: - intake-coordinator expects `git --no-replace-objects merge-base ...`; - testrun-measure's pinned import list for measure-cli swaps ../os/child-process.js for ../os/git.js (a swap, not a loosening: os/git.ts starts its processes only through os/child-process.ts); - compare-screenshots, eval-spend-gate, engine-release and installed-engine-config copy scripts/lib/git.cjs beside the scripts they copy. Test (fails without): the six files, red in run 39540570972; 59 of 59 pass with this. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
A `shallow` file lists commits whose parents git treats as missing, and any agent can write one into the shared repo. With the head's parent listed, pushProblems read 0 problems for a range that added a refused file; deleted again before the push, the push carried that file to the remote (measured by #927's final check). shallowCut refuses a range such a commit cuts, before and after the walks and once more right before the push in checkedPush; a shallow file it can't read is a refusal too. Only a cut inside the range is refused: the harness-eval replay's clone is shallow at the task's base (measured), and a depth-limited clone pushes changes above its cut as before. Not --shallow-file: git's internal. The git-runner scan's comment lists its measured misses, and the release note names the shallow refusal. Test (fails without): test/push-shallow.test.ts, 3 of 4 fail at a8d9c98 (a hand-written cut: 0 problems; the delete-between race; an unreadable shallow file); the depth-limited clone control passes either way. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
|
Round 2 (head a1ff009), addressing issuecomment-6307895829. 1. CI fixtures (measured). The six files now pass by name, 59 of 59:
2. Shallow fails closed.
3. Advisory. The scan's measured misses are listed in the test comment and the body. The body has a "What remains" line about the hook copy and 4. #928 (0b1b060). Other checks. Main hasn't moved (41f6dd4), and |
Evaluator verdict: CHANGES NEEDED (round 2/2)This is the delta final check of PR #927 at head a1ff009 ( In short: the round-1 findings on the fixtures and the claim are fixed. The "cut inside the range" rule is sound as a rule. The code that applies it has two holes, though, and both were measured: git reads the shallow file more loosely than 1. Round-1 findings
I re-ran the round-1 reviewer's probe ( 2. The deviation: refuse only a cut inside base..headThe rule (inferred, with measured cases). Shallow only removes parent edges, so the base side can only exclude less, never more. Take any commit of base..head that a cut hides. The first shallow commit on the path from head down to it is reachable from head in the cut view, and the base can't reach it. So
Why the deviation is needed: the harness-eval replay's clone is shallow. The builder measured that; I did not re-measure it. I accept the deviation as a rule. Its implementation fails in two ways, both measured: (a) git reads the shallow file more loosely than
(b) The toggle window is real and lands the file on the remote. A child process wrote and removed
The body labels this residual "inferred, not measured". It is measured, and an agent can repeat its push until it wins. The brief's version (refuse whenever Closing it by design (measured, in
Base and head must be resolved to ids first, because the private dir has no refs (inferred). 3. A new same-class route: a forged commit-graph (measured)The repo's commit-graph file sits in the shared objects dir (an agent can write it, inferred) and git trusts its parent links. Setup: base → K (adds the key) → D (drops it) → H. I wrote a real graph, then pointed D's first parent at the base in the graph's CDAT chunk and recomputed the checksum. Results:
The issue's outcome is "no git reader in the engine can be steered by repo-controlled git state", and the commit-graph is that kind of state. It wasn't named in the issue, so it isn't a regression: main has the same hole (inferred). It is the class this PR exists to close (principle 4). 4. Re-measured fail-without (round 2)I wrote
At the head, 4 of 4 pass (measured). 5. Changed existing test lines (weakening gate)All measured from
None is loosened. 6. Body claims vs code
7. Other checks
Inferred: whichever of #893 or #903 lands after this PR fails CI at a1ff009 (runs 39545170872 and 39545170912, not rerun; measured with
Required changes
Advisory
|
A forged parent link in the shared objects/info/commit-graph made a walk skip the commit that added a refused file, and the push then landed it (measured by #927's round-2 check). -c core.commitGraph=false joins the runner's fixed arguments, and gitEnv appends the same pair to GIT_CONFIG_COUNT after the caller's own, for the gits a program runs itself (gitleaks). scripts/lib/git.cjs, the four standalone scripts and the push gate's gitleaks env do the same. Measured cost: about 30 ms more for three walks over this repo's 2,903 commits. Test (fails without): test/push-snapshot.test.ts's commit-graph cases (push limits, checkedPush, gitleaks) fail at a1ff009; test/git-runner.test.ts holds the settings and their copies. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
git reads a shallow file more loosely than round 2's exact match (an uppercase id, text after the id, a line over 1 KB each cut a range), and a shallow file toggled every ~3 ms beat round 2's before-and-after checks (23 of 150 pushProblems runs; 5 of 80 checkedPush calls landed the refused file; both measured by the round-2 check). gitSnapshot (src/os/git.ts) is a private GIT_DIR over the repo's objects (GIT_OBJECT_DIRECTORY) with one copy of its shallow file, read once and parsed strictly: any line that isn't exactly a full lowercase object id is refused. pushProblems resolves base and head in the repo, takes one snapshot, checks the cut and runs every range walk through it; checkedPush takes a fresh one right before the push; scanRange runs gitleaks through one, and a cut inside the range is never "clean". Documented environment only, not --shallow-file. Test (fails without): test/push-snapshot.test.ts, 9 of 9 fail at a1ff009 (four shallow-line forms, the mid-walk toggle for pushProblems and checkedPush, the forged graph for the limits and gitleaks, gitleaks under a cut). Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
cases are normal; an exact-entry suffix host node:test reports a todo whose body passes as `ok # TODO`, never a failure: once #887 and #894 merged, their cases passed on main as todos, so a regression of either would have stayed green, and nothing made anyone flip them. - The #887 and #894 cases are normal cases now (both pass on main after the merge). - The remaining holes are known-hole cases (knownHole): strict expected failures that pass while every attack still gets through and fail the suite, saying "this hole is closed: turn this into a normal case", once any is refused. The fix PR (#927, #928, #921, #893) goes red in its own CI until it flips its case; main never does. A self-test covers the helper: open, closed, partly closed, bad bookkeeping, and a body that can't run. - tool.domain-allowlist gains two hosts that end with an exact allowlist entry (no dot before it, and after another label): the evaluator's E04 mutant (`h === a` read as `h.endsWith(a)`) survived the whole file before. Test (fails without): E04 now fails tool.domain-allowlist (28 pass, 1 fail); without the two lines it left the file green (evaluator, measured). At PR #921's head the #889 known-hole case fails with the flip message. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…it past replace refs (#724) rangeNames read names with `git log -z`, which is porcelain: log.showSignature=true with an ssh-signed commit glued the signature check's text to the first name ("No signature\nsecrets/k.key"), so the refused-path, workflow and skill checks gave 0 where main gave 1 (a regression, measured by #928's adversarial review), and log.showRoot=false hid an orphan root's files under an "ours" merge (main too). It now runs rev-list into `diff-tree --stdin -r --root -m --first-parent --no-renames --name-only --no-commit-id -z`, which reads no log config. A touched path with a ".", ".." or ".git" part (any case) is refused: only a crafted tree has one, and `./secrets/x.pem` matched no `secrets/**` glob (measured). push-check's own git helper turns replace refs off: a replace of a 3 MB blob with a small one got past the size limit (measured); #927 moves the helper onto its runner. test/push-names-plumbing.test.ts is locked with the other push tests. Test (fails without): test/push-names-plumbing.test.ts, 7 of 8 fail at 0286164 (showSignature for all three checks, showRoot=false, the four crafted parts, the replace ref); the empty-name tree, which git refuses, passes either way. The lock case fails there too. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…only (#927) CI at 3955c23 failed test/child-env-sites.test.ts (#846): with GIT_CONFIG_COUNT on Object.prototype, gitEnv read it through the prototype, numbered its pair 1 and left pair 0 unset, so git exited 128. The count is now read only when it is the environment's own key; the same in scripts/lib/git.cjs and the push gate. Test (fails without): test/child-env-sites.test.ts (2 cases, red in run 39549011913) and a polluted-prototype case in test/git-runner.test.ts. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
No textual conflict, but a semantic one in src/decision-detail.ts: #921 (#889) imports only redactCut there and redacts every shown diff before cutting it, while this branch's maxBuffer branch of decisionDiff still called redactString(stdout.slice(0, MAX)), which the merged tree can't compile and which cut a token across the 512 KiB cut before redacting it. That branch now uses redactCut(stdout, MAX) too; redactString is not re-imported. Test (fails without): test/decision-diff-overflow.test.ts, a real diff over the 8 MiB buffer with a token the cut lands four characters past `ghp_` in. At 5d8c98a (slice, then redact) the shown diff ends in "+ghp_0054"; here no part of any token is shown. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
gitEnv read the caller's GIT_CONFIG_COUNT with Number(), which accepts what git's strtoul doesn't: `0x1`, `1e0` and `1 ` became 1 and `abc` became 0, so the runner turned git's "bogus count" refusal into a run, and for the first three into a run that applied the caller's first pair (measured by #927's second review, A8; #1001 item 1). A count is now read as git reads it (optional leading whitespace and `+`, then digits; empty is 0). Any other count is left as it is, with no pair added, so git still refuses it; the fixed arguments still turn the commit-graph off for the runner's own gits. The same in scripts/lib/git.cjs and in the push gate's graphOff (scripts/push-gate.mjs). Test (fails without): test/git-config-count.test.ts. At 5d8c98a, 7 of 12 fail: the seven refused counts (`0x1`, `1e0`, `1 `, `abc`, `-1`, `1.5`, a lone space) got a pair and a new count; the five accepted forms pass either way. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Evaluator verdict (delta final check, round 8): CHANGES NEEDEDHead checked: 7696ed8. It is the merge of main 1cee9bd (#921) at 8bf832c plus the Summary:
1. The merge re-derives: PASS
2.
|
| Count | git 2.43 | Each copy at 7696ed8 | Each copy at 5d8c98a |
|---|---|---|---|
-0, -0, -00, \t-0 |
accepts, as 0 | left as is: git runs, core.commitGraph unset |
count 1, core.commitGraph=false |
-18446744073709551615 |
accepts, as 1 (strtoul negates mod 2^64): x.y=z |
left as is: git runs, x.y=z, core.commitGraph unset |
count 1, graph off (the caller's pair overwritten) |
- The cause:
CONFIG_COUNT = /^[ \t\n\v\f\r]*\+?[0-9]+$/has no-. strtoul takes an optional+or-; the "too many entries" check comes after, on the unsigned value.
Who can reach it:
- Push gate (inferred):
GIT_ENV = graphOff({ ...process.env, … }), and the hook gets the pusher's environment. SoGIT_CONFIG_COUNT=-0 git pushwould hand gitleaks' gits the commit-graph. That is the routegraphOffexists to close, and it was closed at 5d8c98a (measured above, on the function). - Runner (inferred): the runner's own gits keep
-c core.commitGraph=falsefrom the fixed arguments, so for them only sub-gits are affected.
The three copies agree (measured): they mismatch on the same counts.
- inferred:
test/git-config-count.test.tschecks onlysrc/os/git.tsandscripts/lib/git.cjs. Nothing tests the parse ingraphOff; the scan's pins don't cover it (below).
The push gate's pinned scan patterns still match (measured): git-runner.test.ts passes at the head. The pins are { encoding: 'utf8', env: GIT_ENV }, const GIT_ENV = graphOff({ ...process.env, and the [`GIT_CONFIG_KEY_${n}`]: 'core.commitGraph' line.
- inferred: they pin the shape, not the parse.
The builder's fail-without (measured): at 5d8c98a, 7 of 12 fail (the refused counts), and 5 pass (the accepted ones). At the head, 12/12 pass.
4. Runner scan (the round-4 scan.mjs copy): PASS
- measured: fetched first. origin/main is still 1cee9bd.
- At the head:
files 520 unaccounted 0. - On
merge-tree 7696ed8b 1cee9bd4(tree 303aca21, the same as the head's):files 520 unaccounted 0.
- At the head:
5. Registry probe: PASS
measured: work/delta-927-r8/reg.mjs imports each worktree's own built merge-policy-weakening.js.
- Against 1cee9bd: 107 entries against the head's 110, lost 0. The 3 extra are this PR's:
git-runner.test.tsRUNNERS,OUTSIDE_THE_RUNNERand its lint mark. - Against 5d8c98a: 85 against 110, lost 0. The 25 extra are Redaction: one shared list of token shapes, prefixed dashboard keys, no own redactors (#889) [PROTECTED: owner merges] #921's redact tables and marks.
6. Tests at the head: PASS
measured: run in the foreground with a scratch HOME, state, held-out dir, TMPDIR and git config. Recording stand-ins (exit 1) came first on PATH for claude, gh, systemctl, launchctl, loginctl, sudo, docker, tailscale, useradd and id. The stand-in log holds only claude --version twice, and the stand-in answered it.
| Batch | Result |
|---|---|
git-runner, review, plan-decisions, decision-detail, decision-detail-chrome, child-env-sites, decision-diff-overflow, git-config-count |
73 pass, 1 skipped (the real-Chrome case: no Chrome here) |
All 16 redact* / redaction-one-module |
84/84 |
push-check, push-gate, push-gate-hook, push-guard, push-hook-guard, push-names-plumbing, push-shallow, push-shallow-format, push-skill-links, push-skill-names, push-snapshot, direct-push-alert, merge-policy, change-files-renames |
241/241 |
7. Typecheck, denylist, size: PASS
- measured:
npm run typecheckprints no errors. - measured:
check:denylistreports1503 files clean. - measured: size (
npm pack --dry-run --json, local, no web UI built) is 1,533,807 / 5,129,894 at the head against 1,527,486 / 5,112,322 at 1cee9bd. That is +6,321 tarball and +17,572 unpacked, against a 2,050,000 / 6,400,000 budget that is unchanged (test/package-files.test.tshas no diff against main).
8. Merge-tree with origin/main and the open PRs
measured: git merge-tree --write-tree 7696ed8b <ref>, then the scan on each tree.
| Ref | Merge | Scan |
|---|---|---|
| origin/main 1cee9bd | clean | 0 |
| #997 26811fd, #957, #932, #918, #794, #746 | clean | 0 |
| #944 fb913e4 | CONFLICT src/os/git.ts (it was clean at 5d8c98a) |
0 |
| #910 9b4775d | CONFLICT src/doctor.ts; it conflicts with main 1cee9bd too (measured), not from this PR |
0 |
| #979 dd6b301 | CONFLICT change-files.ts, scan/secrets.ts |
2 (hook.ts 1, scan/secrets.ts 5) |
| #903 d2a806d | CONFLICT review.yaml, merge-flow.ts, worktrees.ts |
3 |
| #893 33af999 | CONFLICT config/load.ts, checks-from-feedback.ts, merge-flow.ts, pr-hygiene.ts, worktrees.ts |
5 |
#944:
- measured: the conflict is the doc-comment block above
gitEnv, where git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944 addsGIT_GUARD_*andguardEnv. - measured: in the auto-merged
gitEnvbody, this round'sif (!Number.isSafeInteger(n)) return out;lands ahead of git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944's tailreturn marked(out) ? guardEnv(out) : out;. - inferred: for any count the runner leaves alone, an instance's git would skip git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944's guard entirely: no
GIT_GUARD_PAIRS, noGIT_CONFIG_GLOBAL=/dev/null, no guard mark.- Today that includes
-0, which git accepts and runs. So the guard's config-named commands (credential helper, fsmonitor, sshCommand and the rest) would come back.
- Today that includes
- inferred, for git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944's own review: its
guardEnvhas the same gap on its own./^\d{1,4}$/returns early on-0without the guard pairs, and git accepts-0(measured above). - measured: merged straight into main, git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944 still conflicts in
src/review.tsandtest/review.test.ts, as in round 7.
#979, #903 and #893 are as in round 7 (inferred): their own unrouted calls are theirs to route.
9. CI at 7696ed8: green
- measured:
gh pr checks 927shows 28 pass and 3 skipped (bootstrap,bootstrap (windows-wsl),publish). None is pending or failed. Nothing was rerun.
Other checks
- Weakening gate (measured): this round changes no existing test line. Both of its test files are new, and the rest of the diff is
src/andscripts/. Nothing is loosened. - Body claims (measured): the Round 8 claims for the merge, the overflow test, the fail-withouts, the scan, the registry, typecheck, denylist and CI all match.
- Not true: "gitEnv now reads a count as git does", and the correction under "What this can never do" ("a count git refuses is left as it is"). They miss the minus forms in §3.
Required changes
-
Read the sign the way git does, in all three copies (principle 1; a regression from 5d8c98a). Pick one:
- (a) Emulate strtoul exactly:
- optional whitespace, then an optional
+or-, then digits; - a magnitude over 2^64-1 becomes 2^64-1;
- a
-gives (2^64 - magnitude) mod 2^64; - a result over INT_MAX means git refuses, so leave the count alone.
- optional whitespace, then an optional
- (b) Simpler and the same on any
unsigned longwidth (32-bit on Git for Windows; guess): when the count has a-, the runner makes git refuse. For example, set the count to-1("too many entries" on 32- and 64-bit).-0then fails closed instead of running.
In
test/git-config-count.test.ts:- add
-0,-0,-00and-18446744073709551615, each run through real git; - cover
scripts/push-gate.mjsgraphOfftoo (for example its source fed to the test, as the probe does), so all three copies are pinned.
It must fail at 7696ed8.
- (a) Emulate strtoul exactly:
-
Don't return early from
gitEnvon a refused count (src/os/git.ts, and the same shape inscripts/lib/git.cjs). Skip only the pair append, and fall through to the end. Then git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944'sreturn marked(out) ? guardEnv(out) : out;still runs after the merge, whatever the count. -
Correct the Round 8 text and the "What this can never do" correction to match (principle 5). Re-measure on the new head:
- the count probe (or your own over the same 34 counts), 0 mismatches;
- typecheck and denylist;
- the scan copy at the head and on the merge with the then-current main;
- the registry probe;
git-runner,git-config-count,push-gateandpush-gate-hook.
Scripts: work/delta-927-r8/ (count-probe.mjs, reg.mjs, mt.sh, run.sh).
Second review, round 8: CHANGES NEEDEDHead checked: 7696ed8. It merges cleanly with main 1cee9bd ( Scope: round 8 only:
Setup: every probe ran with a temp HOME and state, in scratch repos. The token and key shapes are fake. Attacks
Required changes
Follow-up, not blocking
"What this can never do" and "Compromised component"
Refusals: none. |
…IT_CONFIG_PARAMETERS (#927) Round 8 read GIT_CONFIG_COUNT the way it took git's strtoul to: optional whitespace and `+`, then digits. It missed the minus sign. git reads `-0`, ` -0` and `-00` as 0 and runs, while round 8 left them alone with no pair, so a git that reads only its environment (gitleaks') ran with the commit-graph on (measured by both round-8 reviews). And git reads GIT_CONFIG_PARAMETERS (what `git -c` hands the gits it runs) after the COUNT pairs, so a caller's `git -c core.commitGraph=true`, which the agent guardrail allows, turned it back on for the push gate's gitleaks (measured). In all three copies (src/os/git.ts gitEnv, scripts/lib/git.cjs gitEnv, the push gate's graphOff): - only a canonical count, a plain decimal 0 to 9999, gets the runner's pair after the caller's. Any other value becomes `refused`, a count git refuses on every platform, so the git fails closed instead of guessing git's parse; - `'core.commitGraph=false'` is appended to GIT_CONFIG_PARAMETERS, which git reads last; - no early return, so a later tail (#944's guardEnv) still runs. test/git-runner.test.ts's two expected gitEnv objects gain the GIT_CONFIG_PARAMETERS entry; nothing else in it changed. Test (fails without): test/git-config-count.test.ts, rewritten to the reviewers' tables and run through real git, with the push gate's graphOff taken from its source. As a new file in a fresh worktree at 7696ed8, 44 of 46 fail: every non-canonical count (36), every canonical one (5, no GIT_CONFIG_PARAMETERS), and the three GIT_CONFIG_PARAMETERS cases. graphOff alone, from 7696ed8's source, is wrong on 37 of 40 cases and on 0 here. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
… view (#927) The decision diff is read with `--text` (#807 F2), so a binary file's bytes are shown. UTF-16 text puts a NUL between ASCII characters, and no redaction rule matched it: a token in a UTF-16 file read whole once the NULs were dropped, where main shows "Binary files differ" (measured by round 8's second review). That reopened something main closed. Both branches of decisionDiff now drop every NUL before redacting and cutting. `--text` stays: a NUL, or a `-diff` attribute, is also how a change hides its lines from review as "binary", which F2 closes. Test (fails without): test/decision-diff-utf16.test.ts, a token in a UTF-16LE and in a UTF-16BE file. As a new file in a fresh worktree at 7696ed8, both fail: the shown diff, NULs dropped, holds `ghp_`. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Second review (verification-scoped, after a classifier stop)Verdict: CHANGES NEEDED on 4e4287e (4e4287e). Scope. This is the one rerun allowed after a classifier stopped the full second review. It does three things only:
No new payloads were built. The one new finding below is inferred from reading the redaction rules. It was not run. 1. Documented attacks: head vs 7696ed8 (all measured)All runs used a scratch HOME, TMPDIR and state under
2. Fail-without (measured)I wrote
3. Required change (inferred from code, not run)Dropping NULs before redaction glues each byte run in a binary file to the next one. That defeats the redaction rules that need a boundary.
4. Claims (Round 9, "What this can never do", "Compromised component")Round 9
Why appending is safe for every value git accepts (inferred, from git's parser):
What this can never do
Compromised component
RefusalsNone. |
Evaluator verdict (delta final check, round 9): CHANGES NEEDEDHead checked: 4e4287e. It is two signed-off commits on 7696ed8 (550323c and 4e4287e) and holds main 1cee9bd, which is still origin/main (fetched first). Summary:
1. Count parsing: PASSI ran The 51 counts include:
2.
|
| Run | Values | Accepted by plain git | Security fails | Accepted-but-refused | List changed |
|---|---|---|---|---|---|
| Head (measured) | 4,054 | 47 | 0 | 0 | 0 |
| Control, at 7696ed8 (measured) | 354 | 30 | 21 | — | 90 |
- inferred, from git's
parse_config_env_list: a caller value that leaves a quote open makes the appended'core.commitGraph=false'end that quote. What follows is then unquoted text, which git reports as "bogus format". So no caller value can make git skip the appended entry and still run: it either parses the entry last or refuses. The fuzz agrees.
3. No early return: PASS
- measured (diff):
gitEnvinsrc/os/git.tsandscripts/lib/git.cjsnow has onereturn outat the end. A refused count only skips the pair append. graphOff(no git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944 tail): it still returns from its refused branch, with...lastincluded.- git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944 (fb913e4), measured:
merge-treestill conflicts insrc/os/git.ts. The conflict block now runs from thegitEnvdoc comment, over this PR'sgitEnvbody, to git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944's guard section and its owngitEnv.- inferred: whoever resolves it keeps this PR's body and replaces its final
return out;with git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944'sreturn marked(out) ? guardEnv(out) : out;. With no early return left, that tail then runs for every count. - For a refused count,
guardEnv's/^\d{1,4}$/returns early without the guard pairs. The count is stillrefused, which git refuses (measured above), so that path fails closed.
- inferred: whoever resolves it keeps this PR's body and replaces its final
- For git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944's own review (inferred, not this PR's):
guardEnvputsGIT_GUARD_PAIRSin the COUNT pairs. A caller'sGIT_CONFIG_PARAMETERS(git -c core.fsmonitor=…) is read after them, which is the same class as A3.
4. The decision diff's NUL drop: UTF-16 fixed; --text still reopens binary content: FAIL
work/delta-927-r9/diff-probe.mjs compares the head's decisionDiff with main's args (git diff --no-color --no-ext-diff) through the same redactCut. It checks for a 16-character middle part of a fake ghp_ token, in the shown text with NULs dropped, also filtered to printable ASCII.
| File | Head | 7696ed8 | main |
|---|---|---|---|
| UTF-16LE, UTF-16BE, LE with BOM, LE at an odd offset | redacted | shown | "Binary files differ" |
| UTF-32LE, UTF-32BE | redacted | shown | binary |
Mixed: ASCII token= then a UTF-16 token |
redacted | shown | binary |
| UTF-16 token next to a non-ASCII character | redacted | shown | binary |
Token characters split by 0x01, in a file with a NUL |
shown | shown | binary (hidden) |
| The same with no NUL (control) | shown | shown | shown |
| ed25519 private key, PKCS#8 DER (48 bytes, has a NUL) | hunk shown: 32 of 48 bytes verbatim, ~17 of the 32 seed bytes | the same, with NULs | binary (hidden) |
| RSA-2048 private key, PKCS#1 DER (1,192 bytes, has a NUL) | hunk shown: 691 of 1,192 bytes verbatim | the same, with NULs | binary (hidden) |
All measured. The keys are generated fresh in the probe. For each key, the shown hunk is exactly the key's UTF-8 decoding with NULs dropped: every byte that isn't part of an invalid sequence, in position, with one U+FFFD per invalid byte.
- What the NUL drop does (measured): for text in UTF-16 and UTF-32, in either byte order, with or without a BOM, at any offset, mixed or not, the drop closes A5. It shows no binary content that 7696ed8 didn't already show; it only removes the NULs.
- Keeping
--text(judged): its goal is right. A NUL, or a-diff/binaryattribute, must not hide a text change from the owner (Git readers: one runner with replace refs off for every engine git call; display/prompt readers ignore attributes; count no-newline files by size (#802 follow-ups) #807 F2). But--textalso puts a real binary file's bytes on the owner's dashboard, and main never showed them. A binary private key is the clearest case.- Principle 3: this is a route main closed, reopened.
- Fix the class: this is A5's class (a binary file's content shown where main showed "Binary files differ"), and the NUL drop fixes only its UTF-16/32 instance.
- inferred: it predates round 9, since it came with
--text. Round 9 is where the class was named and closed for one encoding only.
- The body's Dashboard line is incomplete (principle 5). It says the diff "shows a
-diffor binary-marked file's lines … redacted as a NUL-free view". It doesn't say that a binary file's raw bytes are shown, or that no rule redacts them.
5. Fail-without: PASS
measured: I wrote the head's test/git-config-count.test.ts as a new file, test/git-config-count-r9.test.ts, and the new test/decision-diff-utf16.test.ts, both with the Write tool, into a fresh worktree at 7696ed8. Blob hashes are identical to the head's: c1a7ef3f and 48d06e97.
| Where | Result |
|---|---|
| At 7696ed8 | 46 of 48 fail: all 36 non-canonical counts, all 5 canonical ones, the 3 parameters cases, and both UTF-16 cases. The 2 that pass are the malformed-parameters case and the agreement case, as the body says. |
| At the head | The two files, plus decision-diff-overflow, git-runner, decision-detail and child-env-sites: 69 of 69 pass. |
-
The UTF-16 failure at 7696ed8:
no part of the token: "…+token: ghp_Utf16Tk9Lm3Qr7Ws2Xv5Zb8Nc4Hd6Jf1Gp0a\n…". That is the bug the test is for. -
graphOffmutants (measured,work/delta-927-r9/graphoff-mutant.mjs, in memory, no repo file edited): every patterngit-runner.test.tspins still matches all five mutants below, so only the count test tells them apart. The count test's checks catch each one:Mutant Failing cases No fail-closed line 13 Refused count kept as is 13 Round-8 parse 5 No GIT_CONFIG_PARAMETERSappend17 Parameters prepended instead of appended 1 The unmutated head gives 0. A11 is closed.
6. Changed existing test lines: no loosening
-
test/git-runner.test.ts(measured, 2 lines): both changed lines are expected objects inassert.deepEqual. Each gainsGIT_CONFIG_PARAMETERS: "'core.commitGraph=false'"and keeps every other key. An exact-object compare with one more required key is tighter. -
test/git-config-count.test.ts, rewritten in place (this PR's own file from round 8, not on main; measured diff):- Round 8's refused counts (
0x1,1e0,1,abc,-1,1.5,) are all still there. Each still asserts git exits 128 with the caller's pair not applied, both env-only and through the runner. The only change is the expectedGIT_CONFIG_COUNT, from "left as is" torefused, which is the orchestrator's decision. - Round 8's accepted counts (
01,1,+1, ``) moved from "the pair is added and git readsz" to "refused, git exits 128". That is the decided fail-closed direction: tighter for security, and it costs availability only for counts nothing in the engine writes. 1keeps its positive case.- Two narrowings, both inferred minor:
- the copies' agreement check now compares only the
GIT_CONFIG*keys instead of the whole object (the fixed env is pinned bygit-runner.test.ts); - the canonical cases no longer go through
gitSpawnSync, only throughgitEnvwith an env-only git.
- the copies' agreement check now compares only the
Neither is a check main had.
- Round 8's refused counts (
7. Scan, registry, typecheck, denylist, size, other tests
-
Scan (measured, the round-4
scan.mjscopy):files 520 unaccounted 0at the head, and also on the merge with main 1cee9bd. That merge's tree is f9adb8dd, the same tree the body names. -
Registry (measured,
work/delta-927-r9/reg.mjs):Against Size (vs the head's 110) Lost 7696ed8 110 0 main 1cee9bd 107 0 The 3 extra entries against main are this PR's
RUNNERS,OUTSIDE_THE_RUNNERand the lint mark. -
measured:
npm run typecheckis clean, andcheck:denylistreports1504 files clean. -
Size (measured,
npm pack --dry-run --json): 1,534,158 / 5,130,975 at the head, against 1,527,486 / 5,112,322 at main. That is +6,672 / +18,653, well inside the unchanged budget. -
Tests (measured): scratch HOME, state, held-out dir, TMPDIR and git config, with recording stand-ins first on PATH. The stand-in log stayed empty.
push-gate,push-gate-hook,review,plan-decisions,redact,redaction-one-module: 103 of 104. The one failure wasredact.test.ts"every pattern runs in linear time on 100 KB of hostile input", a timing test.- Re-run alone,
redact.test.tsgives 11 of 11. This PR changes neithersrc/events/redact.tsnor that test (measured: empty diff against main). So the failure is load-related, not this PR's (inferred).
8. Merge-tree with origin/main and the open PRs
measured: git merge-tree --write-tree 4e4287e6 <ref>, then the scan on each tree.
| Ref | Merge | Scan |
|---|---|---|
| origin/main 1cee9bd | clean | 0 |
| #997 fec9384, #957, #932, #918 701c15a, #794, #746 dafd697 | clean | 0 |
| #944 fb913e4 | CONFLICT src/os/git.ts (see §3) |
0 |
| #910 9b4775d | CONFLICT src/doctor.ts (as in round 8: it conflicts with main too) |
0 |
| #979 85a5bbd | CONFLICT change-files.ts, scan/secrets.ts |
2 (their own calls) |
| #903 f4bb6dd | CONFLICT review.yaml, merge-flow.ts, worktrees.ts |
3 (their own calls) |
| #893 1d90008 | CONFLICT 5 files | 5 (their own calls) |
9. CI at 4e4287e: green
- measured:
gh pr checks 927shows 28 pass and 3 skipped (bootstrap,bootstrap (windows-wsl),publish). None is pending or failed. Nothing was rerun.
Body claims
- Match (measured): the Round 9 claims for the counts, the parameters, no early return, both fail-withouts (44/46 and 2/2), the scan, the registry, typecheck, the denylist (1504) and CI.
- Incomplete:
- the Dashboard line (§4);
- "Break an existing install's push". With the decided fail-closed rule, an environment holding a count git accepts but that isn't canonical (
GIT_CONFIG_COUNT=empty,01,+1,-0) now fails every push through the gate, and every engine git. main pushed there (inferred from §1's measurements and the gate's code). That is the intended trade, but the body should name it.
- Adversarial pass: the section has no round-9 entries. The attacks the round ran (the malformed parameters,
graphOffalone) sit only in the Round 9 notes, and nothing tried other encodings or a real binary file, which is how §4 was missed.
Required changes
- Don't show a binary file's raw bytes on the owner's diff (principle 3; A5's class, not only its UTF-16 instance).
- Keep
--text's goal: a NUL or an attribute must not hide a text change. - Stop real binary content from reaching the dashboard. One way, by design: for a file git would call binary without
--text(a NUL on either side), show it as a "strings" view.- Every run of bytes that isn't printable text becomes one marker: an invalid UTF-8 byte, a C0 control other than tab and newline, or a NUL.
- A printable run shorter than about 8 characters is dropped too.
- Then redact and cut as now.
- UTF-16/32 text (NUL-dropped) and a text line hidden behind one NUL still read in full. A DER key shows nothing of itself.
- The builder may pick another design that meets the test.
- Test that fails at 4e4287e: a freshly generated ed25519 PKCS#8 and RSA PKCS#1 DER private key, committed as a file. No 6-byte run of the key's printable bytes appears in the shown diff. The UTF-16 test, and a "text with one NUL" case, still show their lines.
- Keep
- Correct the body (principle 5):
- the Dashboard line (binary bytes, and what is now done about them);
- "Break an existing install's push": name the fail-closed cost for a count git accepts that isn't canonical, in the gate and in the runner;
- add round-9 entries to "Adversarial pass".
- Re-measure on the new head:
work/delta-927-r9/diff-probe.mjs(0 key bytes, 0 token parts shown);- the fail-without of the new test at 4e4287e;
- typecheck and denylist;
- the scan at the head and on the merge with the then-current main;
- the registry probe.
Not required, for the owner's information:
- inferred:
guardEnvin git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944 has the A3 class of its own. A caller'sGIT_CONFIG_PARAMETERSis read afterGIT_GUARD_PAIRS. - git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944's conflict with this PR: whoever resolves it keeps git: an instance's git runs nothing its checkout's config names, and .git/ is the coordinator's (#926) [PROTECTED: owner merges] #944's
guardEnvtail at the end of this PR'sgitEnv.
Scripts are in the session scratchpad, work/delta-927-r9/: count-probe.mjs, params-probe.mjs, hook-probe.mjs, diff-probe.mjs, graphoff-mutant.mjs, reg.mjs, mt.sh and run.sh.
…, its text lines do (#927) Round 9 showed the `--text` diff with its NULs dropped. That showed a binary file's printable bytes (a DER private key's), and it glued byte runs together, so a fixed-length key shape a NUL had ended lost the boundary its redaction rule needs (AWS, Google, 0x hex; and `\b` before `sk-`, Stripe). main shows "Binary files differ" (both round-9 reviews, measured and inferred). diffTextView reads git's bytes (latin1 from the runner, one character per byte). A file whose diff lines hold a NUL, invalid UTF-8 or a control byte text doesn't hold is binary, and each of its lines shows only as text it decodes to: UTF-32 or UTF-16 when its NULs fall as mostly-Latin text's do, else UTF-8 of at least 16 characters, each NUL run as U+2400, a boundary for every rule. Any other line becomes `[binary: N bytes not shown]`. Every other file shows as it is. `--text` stays (#807 F2: a NUL or a `-diff` attribute must not hide a text change from review). Test (fails without): test/decision-diff-binary.test.ts. As a new file in a fresh worktree at 4e4287e, 5 of 7 fail: the DER keys (ed25519 PKCS#8, RSA-2048 PKCS#1; no 6-byte run of their printable bytes may show), an AWS, a Google and a 0x hex key shape between NULs with a letter after (shown in the clear there), and a text file with one NUL (its NUL must show as a mark). The two UTF-32 cases pass there too; they pin that this view still reads UTF-32. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…e header (#927) CI at 633b03d failed test/change-files-renames.test.ts's "one helper is the only way src/ lists changed names or counts" in shard 2/4 on Node 22 and 24: diffTextView split the diff into files at their `diff --git` header lines, which that lint's "patch headers" words find, and src/decision-detail.ts isn't on its never-grow list. The view now decides per hunk, from its `@@ ` line to the first line that doesn't start with `+`, `-`, a space or `\`, and reads no file header. A hunk is one file's, so what it shows is the same. The lint is unchanged. Test (fails without): test/change-files-renames.test.ts's lint, as CI ran it at 633b03d. test/decision-diff-binary and test/decision-diff-utf16 still pass. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Second adversarial review, round 10: CHANGES NEEDED (head 0b1c7da)Independent second review under the owner's round cap. Everything below was run against scratch repos and in-memory inputs, with a temp HOME, state, TMPDIR and git config, on git 2.43. Main is 1cee9bd. Each claim is labelled measured, inferred or guess. Required changesR1 (b) reopened route, and (c) overclaim: the decision diff's hunk headers show a binary file's bytes.
R2 (b) reopened route: the fix-forward culprit diff now sends a binary file's bytes, and a UTF-16 token, into a tracker issue body.
R3 (c) overclaim: unscoped lines in the changelog fragment (
Attacks
Follow-ups (not required)
The body's eight follow-ups stand as listed. |
Evaluator verdict (delta final check, round 10): CHANGES NEEDEDHead checked: 0b1c7da. It is two signed-off commits on 4e4287e (633b03d and 0b1c7da). The round changes Summary:
Round-9 findings
Fail-without: matches the body
Changed or removed test linesNone this round (measured: the only test change is the new file). Required changes
Other checks
Follow-ups (not required)Each is either open on main too, or a disclosed, fail-closed trade.
Scripts are in the session scratchpad, |
No conflict. The runner scan finds no git start outside the runner in the merged tree (git-runner passes), and Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com> #918's src/signed-receipts.ts comment no longer matches it.
…nd the binary view go (#927) The owner, 2026-10-21: "Cut --text, land the runner." Every required change in both round-10 reviews came from `--text` and the view that tried to show a binary file's text lines safely: a UTF-32 line that crashed it, a hunk header's function-context bytes, diff.suppressBlankEmpty ending a hunk early, and the fix-forward culprit diff carrying a binary file's bytes into a tracker issue. - src/decision-detail.ts: decisionDiff runs main's `git diff --no-color --no-ext-diff base..head` through gitExecFile; diffTextView and its decoders are removed. A binary file shows as "Binary files ... differ", as on main, and output past the buffer is no diff, as on main. - src/coordinator/merge-flow.ts: the culprit diff is main's `git diff <sha>^1 <sha>` again, through the runner (gitOrNull), not patchText. patchText (`--text`) stays for the patch hash only, which is hashed, never shown. - The PR's own tests for the removed behaviour go: test/decision-diff-binary, decision-diff-utf16 and decision-diff-overflow (none is on main). - changelog.d/927-git-runner.md: scoped to the JS and TS code the scan reads, the .sh helpers and the old hook named, the strict GIT_CONFIG_COUNT named, and the two diffs said to be as before. Test (fails without): test/binary-diffs-as-main.test.ts. Each diff shows "Binary files /dev/null and b/blob.bin differ" for a binary file and a text file's lines. As a new file in a fresh worktree at 0b1c7da, 2 of 2 fail. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Second adversarial review, after the cut: PASS (head 99b27ec)This re-reviews the owner's cut ("Cut --text, land the runner"), with the round cap's rules. The review used scratch repos and a temp HOME, state, TMPDIR and git config, on git 2.43. Main is 8df2304, which this head already merges. My round-10 required items
Other checks
Required changesNone. Follow-ups (not required)
|
Evaluator verdict (delta final check, the cut): CHANGES NEEDED (body only)Head checked: 99b27ec. It is two signed-off commits on 0b1c7da:
Main is still 8df2304 (fetched first). Judged under the owner's round cap: only a break of main, a reopened route main closed, or an overclaim is REQUIRED. Summary:
Round-10 findings, re-run at the head (all measured, scratch HOME, state, TMPDIR and git config)
Two notes on the probes:
Diff against main
Fail-without: matches the body
Tests on main
Body and changelog claims
Required changes
Other checks
Follow-ups (not required)
Scripts are in the session scratchpad, |
…s comment to the hash (#927) Wording only, no logic change. The round-11 final check: the replace-refs claims hold on git 2.42 and later (measured on 2.43); before 2.42 a config `core.useReplaceRefs=true` may turn replace refs back on (inferred). The changelog fragment now says so. patchText's doc comment said its patch was for a person, a prompt or a hash; since the cut it is for the patch hash only, never shown (both reviewers). Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Second adversarial review, wording commit: CHANGES NEEDED (head 35f873e)Under the owner's round cap. There is one REQUIRED item, and it is a one-line body fix. The code is fine. What holds
Required(c) overclaim, introduced by the trim: the root updater is listed as running "with the settings written out".
Follow-ups (not required)
Refusals: none. |
Evaluator verdict (delta final check, wording round): CHANGES NEEDEDHead checked: 35f873e. It is one signed-off commit on 99b27ec, with no attribution trailers (measured). Judged under the owner's round cap. Summary:
The diff (measured,
|
Conflicts, each resolved to keep both sides: - .worklane/review.yaml: the engine-guards list keeps test/git-runner.test.ts and #903's vacuity tests. - src/coordinator/change-files.ts: the runner's import; main's local SizeFile replaces the pr-rules import. - src/coordinator/merge-flow.ts: the runner's imports with main's randomBytes; isAncestor keeps main's arguments through gitSpawnSync. - src/scan/secrets.ts: main's new content-based scanCommit (#979) whole; the runner's imports for scanRange. - src/worktrees.ts: main's removed() split, with the runner's calls. Every raw git start the merge brought now goes through the runner, with the same arguments and options: merge-flow.ts (the tip fetch, sync and async; merge-base; rev-list --merges; vacuityFor's git; branch -D; the changed-lines spawn), scan/secrets.ts (gitOut, headId, the core.fsmonitor read), worktrees.ts (removeWorktreeAsync), hook.ts (gitQuery) and vacuity.ts (baseTextReader). No entry is added to the scan's exception list. Its src/scan/secrets.ts entry shrinks from 2 starts to 1, since #979 took the commit scan's gitleaks `git` call out; the pin on that call is replaced by one on scanRange's `--log-opts` snapshot range. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
…stop's process (#927) CI at 802150f failed #746's desktop backstop in every shard: "src/os/git.ts runs in this backstop's process, so it must be a guard file" (test/desktop-removed.test.ts). After the merge with main, src/merge-policy-weakening.ts reads changed files through src/coordinator/change-files.ts, which now runs git through the runner, so a changed line of src/os/git.ts could change what a backstop reads. GUARD_FILES gains 'src/os/git.ts' (an addition only), next to src/os/child-process.ts, so such a line waits for the owner. Test (fails without): test/desktop-removed.test.ts's "every module this backstop runs ... is held", as CI ran it at 802150f; it passes here. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Keep the policy snapshot from #1006 and the worktree ownership, locking, and read-denial protections from #893. Route the added git calls through the existing runner without changing arguments or options. No exception-table entry or test assertion changes. Signed-off-by: Jayden Vasquez <jaydenvasquez1711@gmail.com>
Independent second verification review: PASSExact head: The orchestrator records this independent review separately from its own implementation work and the independent final check.
Required changes: none under the owner's break/reopen/overclaim round cap. Existing nonblocking follow-ups remain recorded in the PR body. This PASS is the independent code-and-claims verdict on the exact head above. Current-head required CI and the fresh combined check remain separate landing gates; this comment asserts neither has completed. |
Independent final check: PASSExact head: The orchestrator records the independent final review here. The final reviewer is distinct from the implementation author, orchestrator and second reviewer.
Required changes: none under the owner's break/reopen/overclaim round cap. Existing disclosed limitations remain follow-ups. This is the independent code-and-claims PASS; exact-head required CI and fresh combined CI are separate landing gates. |
Intent: no git that the engine's JS and TS code runs can be steered by
refs/replace/*or$GIT_DIR/info/grafts. That is repo state every worktree shares and any agent can write.src/os/git.ts(andscripts/lib/git.cjsfor scripts), starts every such git with--no-replace-objects,GIT_NO_REPLACE_OBJECTS=1and aGIT_GRAFT_FILEthat can't exist, set last over the caller's environment.core.useReplaceRefs=truemay turn replace refs back on (inferred, not run; follow-up 10).scripts/machine/worklane-update.cjs) runs alone and sets only--no-replace-objectson its verification clone: no graft file, and the commit-graph stays on. That clone is root's own (inferred: no agent can write a graft or commit-graph there). See Exceptions and follow-up 11.test/git-runner.test.tsfails on a git call that bypasses the runner in the JS and TS files undersrc/andscripts/. It is a backstop next to the runner, not the protection..shhelpers aren't scanned and run git outside it. So does an install's old push-gate hook, untilsetup:hooksreruns (What remains).Risk class: protected. It touches:
src/**(engine);scripts/push-gate*andscripts/setup/**(push gate and machine setup locks);src/review.tsand.worklane/review.yaml;test/git-runner.test.ts(Merge policy: a check table or lint test registers itself where it's declared; only removing or loosening one waits (#869) [PROTECTED: owner merges] #886's marks).It also touches
src/merge-policy-weakening.ts, the merge-policy registry. At 2172591 that is one addition toGUARD_FILES,src/os/git.ts(see below). Its check registrations moved to #886's marks at the 7d90d65 merge. The owner merges.Part of #807: F1 (the runner) and, of F2, the patch hash. Left for the owner, as the issue says: the role-prompt line in
src/roles.ts(it needs the owner's role eval with a real model) and F4 (the no-newline size count; the owner picks the formula).Current integration head: 021f6f1
Measured during recovery: this signed-off merge incorporates main
0f3370f9after #1006, #997, #893 and #910 landed. It is now pushed to the PR branch. CI and the two independent PASSes on the earlier2172591bhead do not certify this merge.policy.moneyPaths).work/codex-927-merge/.021f6f18: main's protections are preserved and new git calls retain their arguments/options through the runner. Neither reviewer independently reran the 40 integration tests on this merge; those remain builder-measured.Previous integration head: 2172591
2172591: the git runner is a guard file
test/desktop-removed.test.ts; measured,gh run view 39730685942).src/merge-policy-weakening.tsreads changed files throughsrc/coordinator/change-files.ts, which runs git through the runner. So the runner runs in the backstop's process.GUARD_FILESgains'src/os/git.ts', next tosrc/os/child-process.ts. This is an addition only, and it tightens: a changed line there now waits for the owner.desktop-removed,merge-policy,review,git-runnerandcoordinatorpass (252, 4 skipped). Typecheck is clean.The landing merge (802150f)
802150f merges main 03c0cb8 (#746, #979, #903), signed off. It answers the final check. It is a landing step, not new design.
Conflicts, each resolved to keep both sides:
.worklane/review.yaml: the engine-guards list keepstest/git-runner.test.tsand Merge: hold a PR whose own tests run none of the app code it changes [PROTECTED: owner merges] #903's vacuity tests.src/coordinator/change-files.ts: the runner's import; main's localSizeFilereplaces the old import.src/coordinator/merge-flow.ts: the runner's imports plus main'srandomBytes.isAncestorkeeps main's arguments, throughgitSpawnSync.src/scan/secrets.ts: main's new content-basedscanCommit(guard: the commit scan covers every commit form and checkout the guard allows [PROTECTED: owner merges] #979), whole, plus the runner's imports forscanRange.src/worktrees.ts: main'sremoved()split, with the runner's calls.Raw git starts the merge brought, now through the runner with the same arguments and options:
src/coordinator/merge-flow.tsgitExecFileSync,gitSpawn);merge-base;rev-list --merges;vacuityFor'sgit;branch -D; the changed-lines spawnsrc/scan/secrets.tsgitOut,headId, thecore.fsmonitorreadsrc/worktrees.tsremoveWorktreeAsyncsrc/hook.tsgitQuerysrc/vacuity.tsbaseTextReaderThe scan's exception table: no entry was added. The
src/scan/secrets.tsentry shrinks from 2 starts to 1, because guard: the commit scan covers every commit form and checkout the guard allows [PROTECTED: owner merges] #979 took the commit scan's own gitleaksgitcall out. Its pin on that call is replaced by a pin onscanRange's--log-optssnapshot range; the pin ongitEnv(env)stays.Measured at 802150f:
The scan copy reports
files 527 unaccounted 0.The registry has 131 registrations against main 03c0cb8's 128, and against 35f873e's 110. Lost 0 both ways; the 3 extra over main are this PR's marks.
Tests:
adversarial/commit-scan*tests andcommit-planmerge-vacuity-*,merge-policy*,vacuityandpackage-filestestgit-runner,review,merge-policypush-gate,push-gate-hook,change-files-renames,git-config-count,binary-diffs-as-main,child-process,boundaries,plan-decisions,intake-coordinatorhook,adversarial/tool-guard-writes,guard-reachTypecheck is clean, and
check:denylistreports 1577 files clean.CI at 802150f (measured): red in the desktop backstop only, fixed in 2172591 (above). I didn't rerun it.
CI at 2172591 (measured from GitHub's check-runs API during session recovery): 28 success, 3 skipped (
publish,bootstrap,bootstrap (windows-wsl)), 0 pending or failed. All eight required checks succeeded on this exact head. Nothing was rerun. The saved local test counts above are builder-reported measurements from the interrupted session; they were not rerun during recovery.Before it: the cut and the wording (35f873e)
The owner, 2026-10-21: "Cut --text, land the runner."
--textand the view meant to show a binary file's text lines safely (final check, second review).src/decision-detail.ts:decisionDiffruns main'sgit diff --no-color --no-ext-diff base..head, now throughgitExecFile. Against main, the file differs in the import, the call, and the function's doc comment, which grew from one line to four to quote the owner (measured,git diff origin/main).diffTextViewand its decoders are gone. A binary file shows as "Binary files ... differ", and output past the buffer gives no diff, both as on main.src/coordinator/merge-flow.ts: the fix-forward culprit diff is main'sgit diff <sha>^1 <sha>again, through the runner (gitOrNull).decision-diff-binary,decision-diff-utf16anddecision-diff-overflow.test/binary-diffs-as-main.test.ts: both diffs showBinary files /dev/null and b/blob.bin differand a text file's lines, and neither shows the binary file's printable run. Fail-without (measured): written as a new file into a fresh worktree at 0b1c7da, 2 of 2 fail.changelog.d/927-git-runner.md.patchText's doc comment says its patch is for the hash only, never shown. No logic changed.change-files-renames, the three changelog tests,git-runner,reviewandmerge-policy. Typecheck is clean, andcheck:denylistreports 1534 files clean.files 524 unaccounted 0, and the registry probe loses 0 against main 8df2304.fix-forward-coordinatorfails locally the same way on main 9f9533a, so this machine can't run it. It passes in CI.gh pr checks 927): green. 28 pass, 3 are skipped (publish,bootstrap,bootstrap (windows-wsl)), 0 fail. Nothing was rerun.What changed
src/os/git.ts(new, OS layer):gitSpawnSync,gitExecFileSync,gitSpawn,gitExecFile, andcommandSpawnSyncfor runners that take any command. It starts processes throughsrc/os/child-process.ts(Engine: a spawned child never gets Object.prototype's keys as environment variables (#846) [PROTECTED: owner merges] #883).--no-replace-objects -c core.commitGraph=falselead the argument list.GIT_NO_REPLACE_OBJECTS=1and aGIT_GRAFT_FILEthat can't exist go last over the caller's environment.core.commitGraph=falsealso goes into the environment for gits a program like gitleaks runs: after the caller'sGIT_CONFIG_COUNTpairs, and at the end ofGIT_CONFIG_PARAMETERS, which git reads last.refused, which git 2.31 and later refuses, so git fails closed.src/goes through it. That is 58 files at first, plus the calls that main's push limits: read each changed name as itself (-z); lock the rename and odd-name tests (#724) [PROTECTED: owner merges] #928, Merge policy: a check table or lint test registers itself where it's declared; only removing or loosening one waits (#869) [PROTECTED: owner merges] #886, Bench: bench pull pulls each task's image by pinned digest and puts its base tree, from the image, in the held-out store (#748 slice 4) [PROTECTED: owner merges] #947 and review: the repo's merge process in docs/process.md, locked, outside the decisions index [PROTECTED: owner merges] #920 brought, moved in this branch's merges. gitleaks getsgitEnv().scripts/lib/git.cjs(new): the same settings for scripts. Four files run on their own, so each writes the settings out (pinned by the scan, and tied to the runner's values by a test):scripts/push-gate-hook.mjs,scripts/push-gate.mjs(itsgraphOff),scripts/ci/new-deps.mjsandscripts/ci/testrun-real-systemd.mjs.src/push-check.ts, round 2 on):pushProblemsrefuses a range that a.git/shallowboundary cuts. A cut outside the range, like a depth-limited clone's base, pushes as before.gitSnapshot: a privateGIT_DIRover the repo's objects, with one checked copy of the shallow file. A line must be exactly one lowercase id of the repo's own format, or the file is refused.checkedPushtakes a fresh snapshot right before the push.scanRange(gitleaks) reads through a snapshot too, and a cut givesunavailable, neverclean.src/coordinator.ts):patchText(src/coordinator/change-files.ts) reads every file's lines whatever the attributes say. The hash is never shown.test/git-runner.test.tsis locked (code andengine-guards) and carries Merge policy: a check table or lint test registers itself where it's declared; only removing or loosening one waits (#869) [PROTECTED: owner merges] #886's// lint-testmark.OUTSIDE_THE_RUNNERandRUNNERStables carry// check-table: never-grow.src/os/git.tsjoinsGUARD_TREE.intake-coordinator,testrun-measure(a swap of one pinned import, not a loosening),compare-screenshots,eval-spend-gate,engine-releaseandinstalled-engine-config.How it was verified
Every fail-without was measured by writing the tests as new files into a fresh worktree at the earlier head, then building and running them by name. Each run used a scratch HOME, state, TMPDIR and git config.
git-runner(replace ref, graft, gitleaks, push gate, scan)push-shallow(a cut in the range, the delete-between race)push-snapshot(shallow forms, mid-walk toggle, forged commit-graph, gitleaks)push-shallow-format(64-hex in sha1, 40-hex in sha256, the 1 KB form)git-config-count(strict count,GIT_CONFIG_PARAMETERS, all three copies)binary-diffs-as-main(both diffs as main)The registry probe was run on every main merge. At 35f873e it counts 110 registrations against main 8df2304's 107, and against 0b1c7da's 110: lost 0 both ways, and the 3 extra are this PR's marks (measured). The scan copy reports
files 524 unaccounted 0at 35f873e (measured). CI runs the full suite.Package size
Measured with
npm pack --dry-run --json, no web build.PACKAGE_BUDGET(2,170,000 / 6,750,000) needs no change (inferred: CI adds the web build).What this can never do
--no-replace-objectsis in the argument list, which no environment takes back.core.useReplaceRefs=truein repo config, in a caller'sGIT_CONFIG_PARAMETERS, or by-c;GIT_REPLACE_REF_BASE; through the runner and through an env-only git.core.useReplaceRefs=truemay turn replace refs back on (inferred, not run). The by-design fix is follow-up 10.GIT_CONFIG_COUNTgit refuses or reads as-0, and for a caller'sGIT_CONFIG_PARAMETERS(from a guardrail-allowedgit -c, in the pushing agent's own environment).git-config-count).+1,1,01,-0, 10000 and up) now fails every push through the gate and every git the runner starts.Compromised component
refs/replace/*,info/grafts,.git/shallowand the commit-graph in the shared repo..git/shallowstill does, as on main. Measured by the round-11 final check:gitSpawnSync(['rev-list', '--count', 'HEAD'])gives 5, and 3 with a shallow file at HEAD~2..git/shallow.pushProblemsthrow rather than refuse, by toggling a shallow line git rejects. Nothing is pushed (follow-up 2).--text, and a binary file's bytes reached the owner's page in several forms. The cut ends that (measured,binary-diffs-as-main).Adversarial pass
Every attack ran in a scratch repo or with fixtures, never on real state. Details are in each round's review comment (History).
Replace ref or graft on the pushed range: on main, the problems went from 2 to 0; now they stay 2. gitleaks and the push gate script also read the real commits (measured, tests).
A caller's env turning it back on (
GIT_GRAFT_FILE, an empty env,GIT_CONFIG_*,GIT_CONFIG_PARAMETERS): the runner's settings win (measured)..git/shallowin all its forms:Each is refused, and 0 toggled pushes landed (measured).
A forged commit-graph parent link: it is no longer read, by the push limits or by gitleaks (measured).
36
GIT_CONFIG_COUNTvalues and malformedGIT_CONFIG_PARAMETERS, through real git, in all three copies: none runs with the commit-graph on, and none applies a caller pair git would refuse (measured).Scan bypasses: a name built at run time, a ternary, a shell string, a returned name, and the
.shfiles are not caught (named in the test's comment; follow-up 5).Binary files on the owner's diff and in the tracker: found again and again while
--textwas in place, and closed by cutting it (measured,binary-diffs-as-main).Exceptions this adds (principle 9)
The scan is new, so it removes no check. Its exception table is where a review should look. Each entry says why the file starts git outside a runner:
src/init.ts,src/setup-steps.ts,src/setup-mode.ts,src/setup-actions.ts,src/starter-ci.ts,src/testrun/host.ts, and three CI and dev scripts);scripts/push-gate-hook.mjs,scripts/push-gate.mjs,scripts/ci/new-deps.mjs,scripts/ci/testrun-real-systemd.mjs);scripts/machine/worklane-update.cjs), installed alone. As the scan's entry says (test/git-runner.test.ts:231), its reads of the verification clone carryVERIFY_GIT_ARGS, which is--no-replace-objectsonly: replace refs off, but no graft file, and the commit-graph isn't turned off. That clone is root's own (inferred: no agent can write a graft or commit-graph there). Follow-up 11;src/scan/secrets.ts,src/review.ts,src/plugins/approval.ts,src/updater-setup.ts,src/os/index.ts,scripts/verify-release-tag.mjs).No main merge added an entry: every raw call that came in was moved onto the runner instead.
Files other lanes own
Import-line and call-site changes only: Lane B (
src/setup-*.ts,src/init.ts,src/doctor.ts), Lane E (scripts/bench/*), Lane A (most of the rest).Overlap
All measured by
git merge-tree --write-treeagainst 802150f, with the scan copy run on each merged tree. On a tree that has conflicts, the scan reads the conflict markers too, so its count there is approximate.src/os/git.ts,src/scan/secrets.tssrc/scan/secrets.ts2src/config/load.ts,src/coordinator/checks-from-feedback.ts,src/coordinator/merge-flow.ts,src/coordinator/pr-hygiene.ts,src/worktrees.tschecks-from-feedback.ts,merge-flow.ts,pr-hygiene.ts,read-denials.tsandworktrees.tsguardEnvtail at the end ofgitEnv, and takes guard: the commit scan covers every commit form and checkout the guard allows [PROTECTED: owner merges] #979'sscanCommit.OUTSIDE_THE_RUNNER.What remains
npm run setup:hooksafter this merges. Until then, an install's old copy of the push-gate hook runs its own git calls without the runner's settings, including the one that loads the gate. So a replace ref could make an old hook load a substituted gate (inferred).--no-verify), and the engine's pushes go throughcheckedPush.History
Each round's reviews are linked. All claims above are current, and each correction sits next to the claim it fixed.
GIT_CONFIG_PARAMETERS--textgoes from both shown diffspatchText's commentFollow-ups (not required)
None of these breaks something that works on main, reopens a route main closed, or rests on a claim this body still makes. Each title is a separate issue:
pushProblemsthrow instead of refusing. It fails closed; a fix movescommitIdsandsizeFilesonto the snapshot.--textwas meant to cover). Round 10's reviews list what such a view must handle: function-context headers,diff.suppressBlankEmpty, long UTF-32 lines, and the tracker body.guardEnvhas theGIT_CONFIG_PARAMETERSclass of its own. A caller's parameters are read afterGIT_GUARD_PAIRS(inferred)..shfiles, in shell strings, and in a returned name (return 'git').docs/design.md: name the git runner.-c diff.suppressBlankEmpty=false, assrc/comment-policy.tsdoes. It is the same on main.-c core.useReplaceRefs=falseto the runner's fixed arguments (andGIT_CONFIG_PARAMETERS).--no-replace-objectsalready wins there, or state a minimum git version.scripts/machine/worklane-update.cjs) sets noGIT_GRAFT_FILEand leaves the commit-graph on. Only--no-replace-objectsis set (VERIFY_GIT_ARGS). The clone is root's own bare repo (inferred), so no agent can write either there.VERIFY_GIT_ARGSthe runner's full settings.New dependencies
None.