F2d: push the task head to its branch with a real BranchPusher (#98)

[mchwang]

Closes #98.

What

GitBranchPusher (runner/branch-push.ts) is the real BranchPusher that PullRequestPublisher calls before it opens or refreshes a PR. It pushes the task head from the runner-owned repository to https://<GH_HOST or github.com>/<owner>/<name>.git. It never reads or writes the user's checkout.

• Authentication (decision 1): -c credential.helper= and then !gh auth git-credential. Git gets its hardened environment plus the gh allowlist, and nothing else. No token is in any argument or on disk.
• What it may overwrite (decision 2): it reads the remote branch first.
  • At the head already: nothing to do. A push whose outcome was lost settles this way.
  • Absent, or at a commit the task's ledger records as owned: it pushes with --force-with-lease pinned to the value it read (empty means the branch must not exist).
  • Anything else: BranchPushRefused, and nothing is pushed.
• After the push, it reads the branch back and requires the head.
• It only pushes branches under codeboost/, and only <head>:refs/heads/<branch>: no tags, no hooks, no submodules.
• An abort keeps the caller's reason (for example, shutdown), whether it lands during a Git call or between two. The call settles only after Git has exited.
• Errors are one line. Any token is removed from them. A push that GitHub refuses for lack of the workflow scope says so.

Wiring the pusher into web/cli.ts stays in #91.

Tests

test/runner-branch-push.test.ts (20 tests) uses real Git against a local bare remote:

• Covered cases: first push, the no-op, a non-fast-forward refresh over an owned commit, refusal over a foreign commit, and lease races both ways (the branch moves, and the branch is created, between the read and the push). The test causes each race from inside the ledger read, which is a real step between the two.
• The exact-ref read, input refusals, Git config from the environment that would redirect the push, abort timing and reason, and token redaction.
• A Git wrapper on PATH proves two things: every remote call carries the gh helper with no token in arguments or extra variables, and a push that exits 0 without moving the branch is caught.

I mutation-checked the tests. Each of these changes fails at least one test: plain --force, no ledger check, no read-back, no credential helper, and dropping the abort-reason check.

Local validation (validated head 530018b): tsc --noEmit is clean, and the new test file passes. I could not finish the full suite locally: the Docker daemon on this machine is not responding (docker info hangs), so the Docker-backed agent-* tests time out. CI is the source of truth for those.

Review findings (independent fresh-context review)

#	Finding	Outcome
1	The workflow scope hint matched any output that contains "workflow"	Fixed: it now matches only GitHub's refusal text, and only for push. Tested.
2	An abort during a Git call replaced the caller's reason (ShuttingDownError)	Fixed: signal.throwIfAborted() after every call. Tested at each of the 3 calls.
3	An abort between calls, or a timed-out cat-file, was reported as "no commit" or as a generic error	Fixed: only a cat-file that ran to completion means a missing commit, and an abort keeps its reason.
4	The owned set may not include a head that codeboost pushed but the ledger marks as foreign (a future rebase)	Deferred: the caller supplies the set. Decide it when wiring (#91) and with rebasing (#22).
5	The credential path was not exercised by a real Git call	Fixed: Git-wrapper test.
6	An injected url skips validation	Deferred: production builds the URL from pushUrl. #91 must not pass url.
7	head is not tied to the task	One-off: a wrong head can only create an absent branch or overwrite a commit this task owns.
8	A server-side lock race shows [remote rejected], not BranchPushRefused	One-off: nothing is pushed, and the next publish refuses correctly.
9	A killed call dropped its cause	Fixed: the cause is kept when status is null. Tested.
10–12	No overall deadline, GHES host with a port, localized messages	One-off: the timeout is per call, as in runner-repository.ts. The porcelain verdicts are not translated.

Review findings (Copilot round 1, fixed in dd67abd)

#	Finding	Outcome
C1	Redaction ran after the 400-character cut and missed enterprise tokens of any shape	Fixed: redact before the cut, including the exact configured GH_TOKEN, GITHUB_TOKEN and enterprise token values. The test for a token across the cut also found that the shape pattern's \b boundaries missed a token glued to other text, so they were removed. Tested, and mutation-checked.
C2	cat-file -e exits 128 for a broken repository too, which was reported as "no commit"	Fixed: cat-file --batch-check reports missing explicitly, and any other failure keeps Git's error. Tested.

Review findings (Copilot round 2, fixed in 530018b)

#	Finding	Outcome
C3	The C1 fix put a killed call's cause after Git's output, so 400+ characters of output dropped it again (a regression of finding 9)	Fixed: the cause comes first. Long-output regression test, mutation-checked.

None of these needs a new AGENTS.md rule. C3 is covered by "Each self-review pass rereads every changed function in full against the base" and by "Treat every behavioural claim … as something to verify": the claim held only for short output, and the test did not exercise long output. C1 is covered by "Treat every behavioural claim … as something to verify" and C2 by "Preserve the original … reason through every layer". For the rest:: 2 and 3 are covered by "Preserve the original timeout, cancellation, and shutdown reason", 5 and 9 by "Treat every behavioural claim … as something to verify", and 1 is a one-off.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Credential exposure, incomplete token redaction, and misleading repository errors must be addressed.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Token scrubbing leaks credentials before truncation · New
• Git failures are misreported as missing commits · New

What changed in this PR

Adds the production BranchPusher for safely publishing runner-owned task commits to GitHub.

Changes:

• Implements authenticated, lease-guarded Git pushes with read-back verification.
• Adds input validation, cancellation handling, and sanitized errors.
• Adds integration tests using real local Git repositories.

File	Description
runner/​branch-push.ts	Implements secure branch pushing.
runner/​publish.ts	Documents the real pusher implementation.
test/​runner-branch-push.test.ts	Tests push behavior, races, authentication, and failures.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Credential exposure, truncated failure causes, and a racy cancellation assertion remain unresolved.

Review effort: Balanced
Findings: 1

Open (1)

• Preserve fallback errors when truncating long Git output · New

Resolved since last review (2)

• Token scrubbing leaks credentials before truncation
• Git failures are misreported as missing commits

Previously missed (2)

In code that hasn't changed since last review

Avoid passing credentials to local cat-file subprocesses

runner/​branch-push.ts:141

This supplies the GitHub tokens and keyring/configuration environment to the local cat-file call as well as to authenticated remote calls. That subprocess does not need credentials, contrary to the least-privilege requirement in AGENTS.md:111; select the hardened Git-only environment when remote is false, and update the environment test to require tokens only for ls-remote/push.

Remove racy assertion on push after callback abort

test/​runner-branch-push.test.ts:162

This assertion is racy for the at === 3 iteration: the push process has already spawned when the callback aborts, so it can update the local remote before the parent sends SIGTERM even though the promise ultimately rejects with the caller's reason. Remove this assertion or use a controlled Git wrapper that blocks before performing the push.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Authenticated force-updates of remote branches warrant final human review despite strong safeguards and tests.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Preserve fallback errors when truncating long Git output