fix(harness): bind pushes to gated repository trees - #2164
coreplane-switchboard[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Changes requested: The push guard can certify an incomplete formatting check, so it does not enforce that the pushed changed set passed formatting.
Warning
Changes requested · head 87878d3 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — agent-coding.md item 13: arbitrary Prettier commands certify the changed set | src/core/harness/pi/toolRules.ts:271 |
Full review
F1 — The guard treats any successful command containing npx prettier --check as proof for the entire generation. After a rebase changes both a source and test file, checking only the test file—or even running a command that merely contains that text—certifies the generation and permits an unformatted source file to be pushed. Verify that the formatter covered the complete changed set, or run the canonical changed-set check at push time before recording the receipt.
87878d3 to
f79a553
Compare
There was a problem hiding this comment.
LGTM: The prior finding is resolved: only the canonical complete changed-set formatter against the protected base can certify the pushed tree.
Note
Approved · head f79a553 · no findings
Full review
F1 is resolved; the canonical changed-set formatter now exclusively certifies the pushed tree.
4dffa9b to
ce8a962
Compare
There was a problem hiding this comment.
LGTM: The stale fixtures now establish canonical formatting receipts before their simulated pushes, resolving both prior CI failures without weakening the guard.
Note
Approved · head ce8a962 · no findings
Full review
Both prior CI failures are resolved; the updated fixtures now establish canonical formatting receipts before their simulated pushes.
There was a problem hiding this comment.
Changes requested: The formatting receipt can remain valid across an untracked HEAD change or a later failed formatter.
Warning
Changes requested · head ce8a962 · 2 findings: 1 major, 1 minor
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — agent-coding.md item 13: indirect HEAD changes retain the old formatting receipt | src/core/harness/pi/toolRules.ts:279 |
| minor | F2 Spec contradiction — agent-coding.md item 13: a failed formatter leaves an earlier receipt valid | src/core/harness/pi/toolRules.ts:302 |
Full review
F1 — The generation advances only for a fixed list of literal Git subcommands. A command such as npm version patch can modify files and create a commit without matching this regex, so a formatting receipt from the previous HEAD still authorizes pushing the new tree. Compare the formatted tree’s actual identity before push, or conservatively invalidate the receipt for commands not proven read-only.
F2 — When a canonical formatting check fails, recordToolResult deletes the pending entry but preserves formattedGeneration. A prior successful receipt for the same generation therefore still authorizes the push, contradicting the requirement that a failed formatter proves nothing. Clear the matching receipt on failure and cover pass → fail → push with a regression test.
ce8a962 to
57812d3
Compare
There was a problem hiding this comment.
Changes requested: The receipt can still authorize a tree changed inside the push command, and dirty receipts do not bind the files Prettier checked.
Warning
Changes requested · head 57812d3 · 2 findings: 2 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — docs/reference/specs/agent-coding.md item 13: a compound push can mutate the tree after it is checked | src/core/harness/pi/toolRules.ts:329 |
| major | F2 Dirty receipts do not identify the contents Prettier checked | src/core/harness/pi/toolRules.ts:290 |
Full review
F1 (major, high confidence): judgeToolCallWithTree inspects the checkout before the entire shell command executes. A command such as npm version patch && git push origin HEAD therefore passes against the old receipt, changes and commits the tree, then pushes the unverified tree from the same authorized call. Refuse compound pushes that can mutate before git push, or move the comparison to the actual push execution boundary.
F2 (major, high confidence): Every dirty worktree is represented only as <HEAD-tree>:dirty. A dirty correction can make Prettier pass while masking an unformatted committed HEAD, after which the push sends that unformatted commit; later dirty edits also leave the fingerprint unchanged. Only clean worktrees should earn receipts, or the fingerprint must bind the complete index and working-tree contents that Prettier checked.
57812d3 to
785cccc
Compare
There was a problem hiding this comment.
Changes requested: The prior findings are resolved, but an explicit refspec can still push a tree other than the one that earned the receipt.
Warning
Changes requested · head 785cccc · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F3 Push receipts do not bind the refspec's source tree | src/core/harness/pi/toolRules.ts:392 |
Full review
F3 — The guard compares the receipt with the checkout’s current HEAD, but judgePush validates only the destination side of an explicit refspec. After earning a receipt on tree A, git push origin unformatted-ref:plan/fix-issue-2152-the-codin-af9451/u1 can send tree B while the comparison still sees tree A. Restrict guarded pushes to a source proven to be the current HEAD, or resolve the source ref’s tree and compare that tree with the receipt; add a regression test for an alternate-source refspec.
785cccc to
7d9719f
Compare
There was a problem hiding this comment.
Changes requested: The alternate-source fix checks only the first refspec, so one push can still publish additional unformatted trees.
Warning
Changes requested · head 7d9719f · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F3 Push receipts still do not bind every refspec source tree | src/core/harness/pi/toolRules.ts:407 |
Full review
F3: The fix resolves only the first refspec. git push accepts multiple refspecs, so after HEAD earns a receipt, git push origin HEAD:<own-branch> stale-ref:refs/tags/stale validates only HEAD while also publishing the unformatted stale-ref. A branchless run can similarly place a protected branch second. Validate every refspec’s destination and source tree, or refuse multi-refspec and bulk forms such as --all and --mirror.
7d9719f to
ff0b416
Compare
There was a problem hiding this comment.
Changes requested: Explicit multi-refspec forms are fixed, but configured refspecs still let an omitted-refspec push publish unchecked trees.
Warning
Changes requested · head ff0b416 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F3 Push receipts still do not bind every refspec source tree | src/core/harness/pi/toolRules.ts:420 |
Full review
F3: The explicit multi-refspec cases are fixed, but an omitted refspec can still resolve to multiple configured refspecs. After earning a receipt, a run can add remote.origin.push entries for HEAD:<own-branch> and stale-ref:refs/tags/stale, then execute git push origin. Line 420 validates only the checkout tree while Git publishes both configured sources. Resolve the effective configured refspecs or refuse omitted-refspec pushes unless configuration proves they select only the checked-out branch.
ff0b416 to
cd30118
Compare
There was a problem hiding this comment.
Changes requested: Configured bare-push destinations can bypass the run's branch and protected-branch policy.
Warning
Changes requested · head cd30118 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — agent-coding.md item 13: configured destinations bypass branch policy | src/core/harness/pi/toolRules.ts:347 |
Full review
F1: inspectGitPushSources discards each configured refspec’s destination. For example, remote.origin.push=HEAD:main followed by git push origin validates the formatted HEAD tree and is allowed without applying the own-branch/protected-branch checks to main. Preserve and validate every configured destination—or fail closed—before authorizing the push.
cd30118 to
12328f9
Compare
There was a problem hiding this comment.
Changes requested: Configured push URLs still let literal origin publish the gated tree outside the run's repository.
Warning
Changes requested · head 12328f9 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F2 Spec contradiction — agent-coding.md item 13: pushurl redirects literal origin | src/core/harness/pi/toolRules.ts:347 |
Full review
F2 — inspectGitPushSources checks refspecs, mirror mode, follow-tags, and push.default, but never checks remote.origin.pushurl. After earning a receipt, a run can execute git config remote.origin.pushurl <other-repository> followed by git push origin; the guard allows it because the tree and literal remote name still match, while Git publishes to another repository. Refuse configured push URLs or bind the resolved push endpoint to the run’s repository, and add the missing pushurl row to the requested table test.
ee75d43 to
faa1f00
Compare
There was a problem hiding this comment.
Changes requested: The pushurl fix protects only bare pushes; explicit refspec pushes can still be redirected to another repository.
Warning
Changes requested · head faa1f00 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F2 Spec contradiction — agent-coding.md item 13: pushurl redirects literal origin | src/core/harness/pi/toolRules.ts:470 |
Full review
F2 — Major, high confidence. The fix checks remote.origin.pushurl only through inspectPushSources, which line 470 calls only when the refspec is omitted. With remote.origin.pushurl configured, git push origin HEAD:fix/the-pr-head follows the explicit-refspec path, passes the tree and destination checks, then publishes to the redirected repository. This contradicts agent-coding item 13’s promise that configured push URLs are refused. Inspect the remote endpoint for every push, and add an explicit-refspec pushurl case to the invariant table.
faa1f00 to
7b8734a
Compare
There was a problem hiding this comment.
Changes requested: The pushurl path is fixed, but origin's ordinary URL can still be redirected after the receipt is earned.
Warning
Changes requested · head 7b8734a · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F3 The ordinary remote URL is not bound to the run repository | src/core/harness/pi/toolRules.ts:345 |
Full review
F3 — inspectGitPushRemote treats the absence of remote.origin.pushurl as proof that origin still targets the run repository, without inspecting remote.origin.url. After earning a formatting receipt, a model can run git remote set-url origin <other-repository> and then git push origin HEAD:<run-branch>; the worktree and receipt remain unchanged, so the guard authorizes a push to the other repository. Resolve Git’s effective push endpoint and compare it with an immutable run-repository endpoint before allowing the push.
7b8734a to
863394c
Compare
There was a problem hiding this comment.
Changes requested: A backgrounded or piped push can still race a mutation after the guard's tree inspection.
Warning
Changes requested · head 863394c · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — docs/reference/specs/agent-coding.md item 13: backgrounded pushes can race a post-inspection mutation | src/core/harness/pi/toolRules.ts:298 |
Full review
F1 — pushCanFollowAnotherCommand checks only text before the push. A command such as git push origin HEAD:<run-ref> & npm version patch passes authorization, then runs the push concurrently with a tree-changing command, allowing Git to resolve the unformatted commit after the guard’s inspection. Reject pushes participating in background jobs or pipelines, and add regressions for both & and | compound forms.
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
863394c to
c8bd53f
Compare
There was a problem hiding this comment.
Changes requested: The push guard still permits racing compound forms and does not carry its validated destination into PR publication.
Warning
Changes requested · head c8bd53f · 2 findings: 2 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — docs/reference/specs/agent-coding.md item 13: pipeline and background pushes remain outside the receipt proof | src/core/harness/pi/toolRules.ts:494 |
| major | F2 Validated push destinations are discarded before PR publication | src/core/harness/pi/toolRules.ts:538 |
Full review
F1 — Major, high confidence. pushCanFollowAnotherCommand() checks only commands before the push. Because GIT_PUSH truncates at | and control &, commands such as git push origin HEAD:topic | npm version patch or git push origin HEAD:topic & npm version patch are allowed. The mutation can race after authorization but before Git resolves HEAD, publishing a tree that never earned the formatting receipt. Reject pipelines, background jobs, and unsupported compound push commands entirely.
F2 — Major, high confidence. The guard resolves and validates the effective destination in pushResolution, but that destination never reaches trackPushedBranch(). The PR post-step still infers the branch from capped command output and otherwise falls back to the checkout branch. An explicit or configured refspec can therefore push the gated tree to one allowed destination while the post-step opens, updates, or refuses a PR based on another branch. Publish the validated destination with the successful push receipt and use it as the authoritative PR branch.
There was a problem hiding this comment.
Changes requested: The push guard can approve an unqualified destination that Git resolves outside the run-owned branch namespace.
Warning
Changes requested · head c8bd53f · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — agent-coding.md item 13: unqualified refspecs can resolve outside the run-owned branch | src/core/harness/pi/toolRules.ts:452 |
Full review
F1 — Git infers the namespace of an unqualified destination from the source ref, but the guard treats that destination as a branch name. For example, on run branch topic, git push origin refs/tags/release:topic can update refs/tags/topic while the guard approves topic as the run-owned branch when the tag resolves to the formatted tree. Resolve the effective fully qualified destination, or reject ambiguous unqualified refspecs.
A push publishes only the gated tree, to the run’s own ref, on the run’s remote. Every Git-resolved source, destination, and effective endpoint is checked before execution.
Why: Issue #2152 showed rebases could reshape a head after gates passed, wasting CI and review rounds. Record 0071 sets the ordering; sibling #2158 handles conflict recovery, while this change enforces the push boundary.
Where to look
Feedback wanted: Check that no Git source, destination, endpoint, default mode, or force form bypasses the unified resolution path, especially same-tree branch switches.
Risk: This is a 1,449-line cross-harness change at the push boundary. Errors either block coding pushes or publish to a wrong ref/repository; rollback the single commit. Splitting policy from wiring/tests would weaken the invariant’s proof.
Verified: 1,039 focused tests, root TypeScript, ESLint, changed-set Prettier, hygiene, specs, and title checks passed on rebased 1.260.3; CI remains the final gate.
Decisions (3)
Validation (5 criteria)
For agents
Force-pushed one coherent commit 863394c after rebasing onto origin/main b43af4a. Review the push boundary as one invariant rather than by individual Git syntax. The table’s config column drives the fake resolver; direct resolver tests cover NUL-delimited config parsing and repository URL binding.
Requested by @justinhelmer in slack:C0BRRHKFLCB
🤖 Generated with Claude Code