Skip to content

fix(harness): bind pushes to gated repository trees - #2164

Open
coreplane-switchboard[bot] wants to merge 1 commit into
mainfrom
plan/fix-issue-2152-the-codin-af9451/u1
Open

coreplane-switchboard[bot] wants to merge 1 commit into
mainfrom
plan/fix-issue-2152-the-codin-af9451/u1

Conversation

@coreplane-switchboard

@coreplane-switchboard coreplane-switchboard Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

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

  1. Unified push resolution Resolves configured or explicit refspecs through one path and derives omitted destinations from the checked-out ref. ⚠ A resolution mismatch could publish a different source or destination.
  2. Effective endpoint inspection Applies pushurl-over-url precedence, rejects URL rewrites and fan-out settings, and binds the result to the run repository. ⚠ Failing open could redirect a guarded push to another repository.
  3. Push-time tree comparison Observes the current tree, resolves the whole push, and compares every selected source with the formatting receipt.
  4. Source and default-mode matrix Table rows cover explicit and configured refspecs plus every requested push.default mode.
  5. Remote and force matrix Rows cover url and pushurl redirects, alternate remotes, URL remotes, bulk and tag forms, and force variants.
  6. Bound behavioral contract Agent-coding item 13 states the complete tree, ref, and repository invariant and binds it to the table proof.

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)
  • Resolve the whole push once. Separate endpoint and omitted-refspec checks repeatedly left explicit or configured forms uncovered. One resolution object now carries the effective URL and every source/destination into the same policy.
  • Fail closed on Git fan-out and rewrites. Reimplementing mirror, matching, tag fan-out, URL rewrites, and dynamic shell forms would create an incomplete Git parser. Unsupported forms are refused before execution.
  • Resolve HEAD destinations from the checkout. A tree receipt does not identify a branch. Reading the symbolic ref prevents a same-tree checkout switch from sending a bare current-mode push to a different destination.
Validation (5 criteria)
Criterion Proof
Every requested push form resolves to the gated tree, own ref, and run repository or is refused src/core/harness/pi/toolRules.test.ts table “push invariant: *” passed, including explicit/configured refs, all push.default modes, URLs, remotes, bulk/tag, and force forms.
Coding PR post-step still follows the pushed destination after checkout moves src/core/dispatcher.test.ts cases “HEAD moved to another branch after the push…” and “the pushed branch is gone…” passed.
Touched behavior remains green npx vitest run on 7 touched test files — 7 files and 1,039 tests passed.
Changed TypeScript and lint rules pass NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json and npm run lint -- --no-cache — passed.
Rebased changed set meets repository gates Canonical changed-set Prettier, npm run hygiene:check, npm run specs:check, and check:pr-title all passed after rebasing onto origin/main at 1.260.3.
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

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2152-the-codin-af9451/u1 branch from 87878d3 to f79a553 Compare September 21, 2026 20:38
@coreplane-switchboard coreplane-switchboard Bot changed the title fix(harness): require formatting after tree changes fix(harness): require complete changed-set formatting receipts Sep 21, 2026

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2152-the-codin-af9451/u1 branch 2 times, most recently from 4dffa9b to ce8a962 Compare September 21, 2026 21:10

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2152-the-codin-af9451/u1 branch from 57812d3 to 785cccc Compare September 21, 2026 22:27
@coreplane-switchboard coreplane-switchboard Bot changed the title fix(harness): require complete changed-set formatting receipts fix(harness): guard pushes with formatting receipts Sep 21, 2026

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2152-the-codin-af9451/u1 branch from 785cccc to 7d9719f Compare September 21, 2026 22:40
@coreplane-switchboard coreplane-switchboard Bot changed the title fix(harness): guard pushes with formatting receipts fix(harness): bind push receipts to source trees Sep 21, 2026

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2152-the-codin-af9451/u1 branch from cd30118 to 12328f9 Compare September 21, 2026 23:27
@coreplane-switchboard coreplane-switchboard Bot changed the title fix(harness): bind push receipts to source trees fix(harness): authorize only gated push refspecs Sep 21, 2026

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2152-the-codin-af9451/u1 branch 2 times, most recently from ee75d43 to faa1f00 Compare September 21, 2026 23:43

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2152-the-codin-af9451/u1 branch from faa1f00 to 7b8734a Compare September 21, 2026 23:52
@coreplane-switchboard coreplane-switchboard Bot changed the title fix(harness): authorize only gated push refspecs fix(harness): bind pushes to gated repository trees Sep 21, 2026

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2152-the-codin-af9451/u1 branch from 7b8734a to 863394c Compare September 22, 2026 06:07

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant