[jayden1711]

What changed

src/push-check.ts (the push limits, pushProblems and checkedPush):

• Refuse, never throw. Every read of the range runs inside one boundary. A git call that fails (a repo config broken between calls, a branch deleted, a crafted tree git won't read, a name that isn't a commit) becomes one named refusal: "the push limits couldn't read the range (<git's fatal/error line>): nothing is judged clean, so nothing is pushed". It used to throw. Every caller already failed closed (push check: refuse, never throw, when the repo changes between git calls #981 measured 0 pushed in 230 runs), but a throw in PR mode ended that landing tick.
• Read by commit id. The range's ends are resolved once (rev-parse --verify --end-of-options <r>^{commit}). Every later call reads those ids, never the names, so a branch moved or deleted between the check's git calls changes nothing it reads.
• Push what was judged. checkedPush pushes the head commit id the limits judged, not o.head's name. On main, a head passed by name that moved after the check was pushed unchecked (measured, test 5 below). Every engine caller passes a commit id today (inferred from reading the callers: each passes rev-parse HEAD, commit-tree output or a recorded head). So this closes a route for future callers, not a live leak.

Closes #981, for main as it is. See "Not done" for the snapshot half, which depends on #927.

Critical set: yes

This is the push check, under "locks and the tool guard" and the push limits. It also edits two locked tests, test/push-check.test.ts and test/push-names-plumbing.test.ts (engine-guards). It needs two independent reviews.

One assertion changed in a locked test. In test/push-names-plumbing.test.ts, the crafted empty-name tree case expected pushProblems to throw /empty filename in tree entry/. It now expects exactly one refusal that names that same git error and says nothing is pushed. This tightens the test rather than weakening it: the range is still never judged clean, and the caller now gets a refusal instead of an exception. Reviewers should confirm they agree.

Evidence

The new tests live in test/push-check.test.ts. A git stand-in placed first on PATH makes one change to the repo at a chosen git call, then runs the real git. That makes the race deterministic.

1. A repo config broken right before the size check's diff -U0 gives a named refusal ("bad config"), not a throw.
2. checkedPush with the config broken at rev-list --objects gives a refusal, and the remote has no task branch.
3. A head or base that names no commit gives a refusal, not a throw.
4. A head branch deleted at rev-list --objects changes nothing: the refused path is still reported.
5. A head branch moved to an unchecked commit right before git push: the checked commit is pushed.

Fail-before (measured). I added the tests on unfixed origin/main (d5650da) src, then ran npx tsc -p tsconfig.json && node --import ./dist/test/isolate.js --test --test-timeout=180000 dist/test/push-check.test.js. Result: tests 20, pass 15, fail 5. The 5 failures are exactly the new tests:

• 1 threw fatal: bad config line 14 in file .git/config;
• 2 and 3 threw from execFileSync;
• 4 threw fatal: ambiguous argument '<base>..topic': unknown revision;
• 5 found the moved branch's unchecked commit on the remote.

Pass-after (measured). Same commands at this head:

Test file	Pass / total
push-check	20/20
push-names-plumbing	8/8
push-skill-links	4/4
push-skill-names	4/4
change-files-renames	63/63
push-guard	5/5

Other checks (measured):

• npm run typecheck: clean.
• npm run check:denylist: 1658 files clean.
• changelog.test.js: 27/27.
• Full suite (flock /var/tmp/wl-coord/fulltest.lock nice -n 15 ionice -c3 npm test, run once at de6922d): 5845 tests, 5552 pass, 134 fail, 119 skipped, 40 todo.
  • None of the 134 failures are in a push test. Every push test passed, the five new ones included.
  • The failures are in signed-receipts, machine-sudoers, merge-vacuity, project-command-home and a few other files.
  • The errors I read point at this machine's sandbox, not at this change (inferred from them): a shared /var/tmp parent the signer refuses, EINVAL on chown, the sandbox's pip and proxy variables, and the node_modules symlink.
  • merge-vacuity-r6, one of the failing files, passes 11/11 alone at this head, and 11/11 with main's src/push-check.ts (measured).
  • CI is the authoritative run.

Not done

• Moving every call into git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927's snapshot. The issue's design ("every git call the push check makes runs against the snapshot") needs git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927's gitSnapshot. git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927 is still open, and it is not on main. This PR doesn't stack on it. When git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927 lands, its push-check hunks will conflict with this PR's. They compose like this (inferred):
  • git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927's commitIds and its rangeNames/blobsByName calls run inside this boundary, so a throw there becomes a refusal too. git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927's own commitIds comment says it "Throws as git does on a bad name", which is one of the throw sites push check: refuse, never throw, when the repo changes between git calls #981 names.
  • git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927's late shallowCut before the push needs the same boundary. Without it, that call can still throw.
• The size check's config is read live (guess: only matters after git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927). The rename pairing (sizeFiles with renames set to "configured") reads diff.renames from the repo's config on its own call. A config flipped (not broken) between calls changes the line count without any error. Pinning it means reading the config once, or reading it through the snapshot. That is git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927's design; this PR only makes a broken config refuse.

Follow-ups noticed for #966 (not done here; not this PR's issue)

• push check: refuse a non-commit range end; glob ? and astral characters; NFC/NFD #966 item 1 (a head or base that isn't a commit) is incidentally covered. ^{commit} makes a tree end of the range a refusal: "expected commit type, but the object dereferences to tree type" (measured: built dist at this head, then pushProblems(repo, base, '<head>^{tree} id', DEFAULT_PUSH_LIMITS) in a scratch repo). This PR adds no test for it. push check: refuse a non-commit range end; glob ? and astral characters; NFC/NFD #966's builder should add its test with a tree head, then confirm and close that item. The refusal's last sentence ("if something changed the repository...") doesn't fit that case. push check: refuse a non-commit range end; glob ? and astral characters; NFC/NFD #966 may want its own wording.
• push check: refuse a non-commit range end; glob ? and astral characters; NFC/NFD #966 items 2 and 3 (glob ? with astral characters, and NFC/NFD) are untouched.

[jayden1711]

Independent final check (R88a), posted by the coordinator.

VERDICT: PASS @de6922dc7d9c20d2bb788377c3ce38dd5063dda1 (final check)

PR #1088 (issue #981). Reviewed in a detached worktree at the head. The head was re-read with gh pr view 1088 --json headRefOid at the end and had not moved.

Findings

• Fail-before and pass-after hold (measured). I swapped in main's src/push-check.ts (d5650da) and kept the head's tests, then ran npx tsc -p tsconfig.json && node --import ./dist/test/isolate.js --test dist/test/push-check.test.js dist/test/push-names-plumbing.test.js. Result: 28 tests, 6 failed. The failures were the 5 new push check: refuse, never throw, when the repo changes between git calls #981 tests and the changed locked empty-name test. At the head, push-check, push-names-plumbing, push-guard, push-skill-links and push-skill-names gave 41/41.

• The locked test change is equivalent or stronger, not a relaxation (measured and inferred). On main, pushProblems threw empty filename in tree entry. Now it returns exactly one problem that names that same git error and says "nothing is pushed". checkedPush refuses on any non-empty problem list, or when no head was judged, so the push is still blocked (inferred from the code). The coordinator's change-stage caller turns the same problem into a rejection. The test now pins the count and the error text, which is tighter than "throws".

• The pushed ref is still correct for every caller (measured). All 11 checkedPush callers in src/ pass ref as refs/heads/... (rg -n "checkedPush\(" src). A probe (<tmp>) ran against the head's and main's built push-check.js. The head and main gave identical results for these shapes:

  • a new branch with no force, head passed by name (HEAD);
  • a fast-forward by name;
  • a non-fast-forward with no force (rejected, remote unchanged);
  • a non-fast-forward with force (replaced);
  • the default-branch guard.

  The existing lease test (expect) passes. The lease is built from o.ref and o.expect, not from the source, so it is unaffected.

• Mutation testing (measured). Two mutants were each caught by exactly one test:

  • pushing ${o.head} again was caught by test 18;
  • judging by names instead of resolved ids was caught by test 17.

• CI is green on the head (measured). gh pr checks 1088: every test shard on Node 22 and 24, push-gate, denylist, dco, scope, scan, agentshield and new-deps passed. Only the real-systemd jobs were skipped. This supports the builder's view that the 134 local failures came from the sandbox (inferred).

• Other checks (measured). npm run typecheck is clean. npm run check:denylist reports 1658 files clean. changelog.test.js passed 27/27. The changelog fragment is present and accurate. Nothing in src/ is brand-specific.

• The PR body's claims match the diff (inferred). It honestly marks the snapshot half (git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927) as not done.

Follow-ups (not FAILs)

• A short ref now fails (measured, no caller affected). On main, a short ref (n2) with a named head created the branch. At the head it fails with git's "destination is not a full refname", because the source is now a commit id. This fails closed and no caller does it. Consider asserting ref starts with refs/ in checkedPush.
• Minimum git version (guess). rev-parse --verify --end-of-options works on git 2.43.0 here. I believe older git (2.40 to 2.42, still allowed by the 2.40 floor for check-attr) may not accept --end-of-options in rev-parse. If so, every push there would be refused, which fails closed. Worth checking against git's release notes.
• Transient races now reject the worker's change (inferred). At the change stage (coordinator.ts around line 1495), a git failure from a race used to throw and end the tick. Now it is a rejection sent back to the worker. That fails closed, but the worker may be told to redo a change that was fine.

What I ran

git worktree add --detach, tsc, the targeted node --test runs above, the main-src swap, two sed mutants (reverted, and git status was clean afterwards), the probe script, npm run typecheck, npm run check:denylist and gh pr checks/gh pr view. I made no GitHub writes.

[jayden1711]

Independent adversarial review (R88b), posted by the coordinator.

VERDICT: PASS @de6922dc7d9c20d2bb788377c3ce38dd5063dda1 (adversarial)

Findings

• Fail-before / pass-after holds (measured). I put main's src/push-check.ts into my worktree with the head's tests, then ran npx tsc -p tsconfig.json && node --import ./dist/test/isolate.js --test dist/test/push-check.test.js dist/test/push-names-plumbing.test.js. Result: 6 of 28 fail, namely the 5 new tests plus the edited empty-name assertion. At the head: 28/28. npm run typecheck is clean. Every CI check on the head is green (gh pr checks 1088).
• No unchecked commit pushed (measured). I ran probe.mjs head|main against scratch repos with a local bare remote. Each of these was either refused or pushed exactly the commit that was checked:
  • an annotated tag of a commit that passes the limits, and one of a commit that doesn't;
  • a tag of a tree, a tree id as head, a blob as base;
  • a symbolic ref as head;
  • an ambiguous name (tag x on a bad commit, branch x on a good one);
  • git replace of a good commit by a bad one, and of a 3 MB blob by a small one (the remote got the original objects);
  • a branch packed into packed-refs, then overridden by a loose ref to a bad commit;
  • a corrupt loose ref.
• Fail-open on main that the head closes (measured). On main, pushProblems with a tree id as head returned [] (judged clean); only the remote's "non-commit object" rejection stopped the push. The head refuses it.
• Throws become refusals (measured). Main throws and the head refuses for each of these: a missing blob, a missing head commit, a corrupt ref, a blob as base, a name starting with --, and git killed with SIGKILL at rev-list --objects. With SIGKILL, git leaves no stderr, so the reason shown is "Command failed: git ...", which is readable.
• No false refusal on ordinary use (measured). These behave the same on head and main: head given as HEAD, as a short sha or as a branch name; base given as origin/main; an empty range; a push with expect (lease); a plain fast-forward push.
• Behaviour changes no engine caller reaches (measured; not FAILs):
  1. A short destination ref with the same name as the source branch (head: 'feat', ref: 'feat') pushes on main. At the head, git errors with "destination ... not a full refname", because the source is now a raw commit id. Every caller in the repo passes refs/heads/... (measured: rg "checkedPush\("), and the error pushes nothing.
  2. :/msg as head is refused at the head, because git reads :/c^{commit} as a message search. Main pushed it.
• Mutants (measured, mutate.sh). Each mutant edits the compiled push-check.js once; then I run the two push test files and restore the file.
  • Caught: pushing o.head (test 18), judge reading names (test 17), rethrowing (tests 14, 15, 16, 23), dropping --verify (25 tests).
  • Survived: dropping the ^{commit} peel, and dropping --end-of-options.
• Locked test edit (inferred). The edited push-names-plumbing assertion is stricter than before: exactly one refusal that names git's error. I agree.

What I ran

• Detached worktree at the head and TMPDIR=<tmp>.
• The fail-before swap described above.
• probe.mjs and lease.mjs against both the head build and main's compiled push-check.js.
• mutate.sh, npm run typecheck and gh pr checks 1088.
• I re-checked that the head had not moved at the end.

Follow-ups (not FAILs)

1. Add a test with a tree or a tag of a tree as head, to kill the no-peel mutant. This is push check: refuse a non-commit range end; glob ? and astral characters; NFC/NFD #966 item 1, as the PR already notes.
2. Add a test that a head named --... is never read as an option, to kill the --end-of-options mutant.
3. checkedPush with a short destination ref no longer works. Either require refs/ in the type or the code, or document it.
4. The git push call doesn't pass --no-replace-objects. Replaced objects aren't sent today (measured), but pinning it would match the other git calls (inferred).

[jayden1711]

Weakening scan decision for head de6922dc7d9c20d2bb788377c3ce38dd5063dda1, recorded by the coordinator under the owner's delegation.

The owner's instruction (verbatim): "Stp asking me quesgtions and just take recommended option each time". Applied only because both independent reviewers of this exact head (R88a, R88b) classified every flag as a strengthening or a limit, not a relaxation.

The weakening scan of base d5650da8 to head de6922dc reports 3 flags (measured):

1. Changed assertion in test/push-names-plumbing.test.ts: strengthening. The old test accepted any throw naming the git error. The new one needs exactly one refusal that names it and says nothing is pushed. It fails against main's src/push-check.ts and passes at the head (measured, both reviewers).
2. Changed guard lines in src/push-check.ts: strengthening. checkedPush still refuses on any problem, also refuses when no head was judged, and pushes the judged commit id rather than the branch name. Mutants that revert either change fail tests 17 and 18 (measured, both reviewers).
3. Added skip mark in test/push-check.test.ts: limit. It skips only on Windows and marks only the 4 new tests that use a sh git stand-in. On Linux the run reports skipped 0 (measured, both reviewers). The same pattern is already used across the test suite.

Decision: proceed (recommended option). This covers only this head; any new push needs a fresh scan and reviews.