[jayden1711]

Closes #952

What changed

• New src/git-remote.ts: one check, gitRemote(name). It returns the name only when it matches ^[A-Za-z0-9][A-Za-z0-9._-]*$, the pattern the issue gives, and throws otherwise.
• It runs once, where each module takes a remote from its caller, before any git call. Every git fetch / ls-remote / push in these modules uses only the checked value:
  • CoordinatorBase (this.remote). This covers the 5 merge-flow.ts calls the issue names, and the other this.remote fetches in coordinator.ts, landing, learnings, pr-hygiene, fix-runs, gardener, optimize, checks-from-feedback, delegation and decisions.
  • The PrBatcher and PrTestRuns constructors.
  • repoGit (main watch), which also covers acceptGit's ls-remote because it builds repoGit first.
  • gateGit (deploy gate).
  • LiveConfig and startConfig.
  • readClaim, claim, renew and release.
• A changelog fragment under Guardrail changes.

Why this shape

The issue asks for the check in #927's git runner. #927 is still open, and the runner's env module is being changed under #1001, so this PR doesn't touch it. Checking where the remote comes in covers about 30 fetch call sites with 9 checks and leaves the call sites unchanged (inferred, from rg "'fetch'" src). I didn't add --end-of-options at each site: once the name is checked, it adds nothing, and it would mean patching each site separately. If #927 lands, gitRemote can move into its runner unchanged (inferred).

Evidence

• The attack (measured, git 2.43.0, scratch clone): git fetch -q "--upload-pack=touch $M #" main, the refspec form with the same remote, and git ls-remote each created the marker file. test/git-remote.test.ts test 2 shows the same thing in the suite.

• Fail-before (measured): I restored the 7 call-site files to origin/main with git checkout origin/main -- <files>, kept the helper so the test compiles, ran npm run build, then ran node --import ./dist/test/isolate.js --test dist/test/git-remote.test.js. Result: 2 pass, 5 fail. The failures are tests 3–7 (main-watch, deploy gate, live config, claims, constructors), with "Missing expected exception" or the git error instead of the refusal.

• Pass-after (measured): the same command with the fix gives 7/7 pass.

• Neighbouring tests (measured): git-remote, claims, live-config, main-watch, main-watch-coordinator, deploy-gate, pr-batch, pr-batch-coordinator, testrun-runs and brand: 186/186 pass.

• Typecheck and denylist (measured): npm run typecheck is clean, and npm run check:denylist reports "1659 files clean".

• Full suite (measured): flock … npm test ran once on this head: 5721 pass, 17 fail, 40 todo. I believe all 17 come from the local sandbox and none from this change (inferred, not re-run on main):

  • 1 in coordinator.test sees the sandbox's own pip cache variable;
  • 1 in desktop-removed.test sees yaml resolve through the worktree's symlinked node_modules;
  • 15 in machine-sudoers-* compare a message that the long TMPDIR path clips.

  CI is authoritative.

Critical set: yes

Measured: the engine's computeLevel on these files gives L3, with self-modification: src/coordinator/base.ts, src/coordinator/pr-batch.ts, src/coordinator/test-runs.ts. The change is also coordinator git hardening. It needs two independent reviews.

Not done (follow-ups)

• checkedPush (src/push-check.ts, the push gate) takes a remote too. Every engine caller passes origin or the coordinator's already-checked remote (inferred). I left it alone because it is a guard outside this task's ownership.
• remoteTip (src/auto-merge-resume.ts, ls-remote) is only reached with the coordinator's checked remote (inferred).
• src/harness-evals/replay.ts fetches from o.source(t), which is a path or URL rather than a remote name, so this pattern doesn't fit it. --end-of-options there would be a separate change.
• scripts/ use a hardcoded origin (measured, grep).

[jayden1711]

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

VERDICT: PASS @0bdcc993033d0be77b3c863687fcae682d05d64b (adversarial)

Reviewer r87b, critical set. Head confirmed with gh pr view 1087 --json headRefOid (prefix 0bdcc99 matches; unchanged at the end).

Findings

• No engine path reaches git with an unchecked remote (measured + inferred). rg over src shows that every engine caller passes 'origin' (service.ts, dashboard.ts) or the coordinator's checked this.remote. No config field names a remote: rg remote src/config/schema.ts finds only runner: 'remote-machine'. CoordinatorBase.remote is a copy taken once. LiveConfig builds this.ref in its constructor. repoGit/gateGit close over a checked parameter, and claims check on every call. So mutating the options afterwards doesn't affect any of these (inferred, from reading the code).
• Unicode look-alike hyphens and URL/path forms are refused (measured, dist/probe-r87b.mjs). Refused: U+2010, U+2011, U+2212, U+FF0D, en and em dashes, a:b, ext::sh, a/b, ../x, origin@{0}, a trailing space or tab, a combining accent, non-strings, and String objects. Accepted: ORIGIN, up-stream, a.b.c, Origin_2.
• Fail-before and pass-after reproduced (measured). I ran git checkout origin/main -- on the 7 call-site files, kept the helper, ran npm run build, then node --import ./dist/test/isolate.js --test dist/test/git-remote.test.js. Result: 2 pass, 5 fail (tests 3–7). At the head: 7/7 pass.
• Mutation testing (measured, dist/mutate-r87b.sh, compiled JS mutated one line at a time). 16 of 17 mutants were killed, including dropping gitRemote at each of the 13 call sites (each claims function separately), allowing a leading -, removing either anchor, allowing /:, and skipping the throw. The one survivor removes the typeof guard; types already block it, and every non-string probe still gets refused or would be harmless.
• Neighbouring tests and typecheck (measured). claims, live-config, main-watch, deploy-gate, pr-batch, testrun-runs and brand pass 126/126. npm run typecheck exits 0.
• CI (measured, gh pr checks 1087). 9 pass, 8 pending (test shards), 7 skipping, 0 failing at the time of writing.
• The changelog's "nothing changes for an install" is true (inferred, from the grep above).

Follow-ups (not FAILs: no engine path, not claimed)

1. PrBatcher and PrTestRuns ignore gitRemote's return value and re-read this.d.remote at fetch time (measured). A caller that mutates its deps object after construction reaches git: bd.remote = '--upload-pack=…' and then fetch([]) created the marker, and so did PrTestRuns.fetchPr with a no-space upload-pack script. Both engine constructors pass a fresh literal that nothing else keeps (inferred), so no engine path can do this. The new file's header comment ("its git calls use only what that returned") is slightly stronger than the code. Fix: store the checked value (private readonly remote = gitRemote(d.remote)) and use it.
2. A plain name that isn't a configured remote is still read as a relative path (measured). git ls-remote a..b, run with a bare repo in ./a..b, succeeded. This is not option injection, and the pattern can't prevent it. --end-of-options or remote get-url validation would be a separate change.
3. Possible false refusal of names with / (measured: git check-ref-format refs/remotes/team/fork/x accepts it). Names like team/fork are legal in git but now refused. No engine caller passes one, so this isn't ordinary use today; worth documenting if a remote ever becomes configurable.
4. checkedPush, remoteTip and replay remain unchecked. The PR names them as out of scope and doesn't claim them; each is reached only with 'origin' or a checked remote (inferred).

Ran

gh pr view/checks, npm run build, targeted node --test, npm run typecheck, dist/probe-r87b.mjs (scratch repos under $TMPDIR, marker files only, no network), dist/mutate-r87b.sh, and the fail-before checkout described above, after which the worktree was restored to HEAD.

[jayden1711]

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

VERDICT: PASS @0bdcc993033d0be77b3c863687fcae682d05d64b (final check)

Reviewer r87a, PR #1087 (issue #952). Critical set: yes (L3 self-modification). This is one of the two reviews it needs. Head confirmed with gh pr view 1087 --json headRefOid both before and after the review: unchanged.

Findings

• Coverage (measured, rg "'(fetch|ls-remote|push|remote)'" src plus a caller grep): every git fetch, ls-remote or push in src/ that takes a non-literal remote now gets it checked. The check sits in one of three places: on CoordinatorBase.this.remote, which is readonly and assigned only at base.ts:243; on PrBatcher's or PrTestRuns' d.remote; or in repoGit/acceptGit, gateGit, LiveConfig/startConfig or claims. Three sites are left uncovered, and each is justified:

  • checkedPush gets either the coordinator's this.remote or origin (in tune.ts, where no caller sets remote, and in config-change.ts).
  • remoteTip/mainNow is reached only from merge-flow's this.remote.
  • direct-land localRemote runs remote get-url, which is not an upload-pack path, and gets origin or this.remote.

  harness-evals/replay takes a path or URL rather than a remote name. The PR body lists it as a follow-up.

• No false refusals (measured, rg -no "remote: *[^,})]+" test src): every literal remote in src/ and test/ is 'origin'. The config schema has no remote option. Every default is ?? 'origin', and service.ts passes 'origin'. Names with hyphens, underscores and dots that start with a letter or digit pass (test 1).

• Fail-before / pass-after (measured): with the 7 call-site files restored to origin/main, then npm run build and node --import ./dist/test/isolate.js --test dist/test/git-remote.test.js, tests 3–7 fail and 1–2 pass. On the head, all 7 pass. The neighbouring tests (pr-batch, testrun-runs, claims, live-config, main-watch, deploy-gate) give 131/131 pass.

• Mutants caught (measured): dropping the check from renew alone fails test 6, because git prints its option usage. Dropping it from the PrTestRuns constructor alone fails test 7 with "Missing expected exception".

• Typecheck (measured): npm run typecheck is clean.

• CI (measured, gh pr checks 1087): every check passes on the head: all 8 test shards (Node 22 and 24), denylist, dco, agentshield, push-gate and scope. The real-systemd jobs are skipped. This confirms the builder's 17 local failures came from the host, not this change.

• Main drift (measured): origin/main is 6 commits past the merge base. git diff d5650da origin/main -- src adds no new fetch, ls-remote, push or remote sites.

• Changelog and docs (inferred, from reading them): the fragment is accurate. "Every engine caller passes origin" matches the grep.

Follow-ups (not FAILs)

• The pattern refuses names git accepts, such as _x, .x and a/b. The engine never uses them (inferred). If a user-set remote is ever added, document the restriction there.
• checkedPush and remoteTip take an unchecked remote. That is safe today only because of who calls them (inferred). Moving gitRemote into the git: one runner for every engine git call, with replace refs and grafts off (#807) [PROTECTED: owner merges] #927 runner, or adding a check in checkedPush, would make this structural.
• harness-evals/replay fetch: add --end-of-options, as the PR says.

Ran

The gh pr view and gh pr checks calls above; a detached worktree at the head; npm run build; the targeted node --test runs; a fail-before run with the 7 files checked out from origin/main; one mutant build; npm run typecheck; and the rg call-site greps. The worktree was restored to the head afterwards.