Skip to content

hand a reviewed public PR back to its opener, or land it ourselves when the branch is writable - #6067

Merged
atomantic merged 4 commits into
mainfrom
cos/task-mtljhoa7/agent-5a745ea2
Sep 3, 2026
Merged

hand a reviewed public PR back to its opener, or land it ourselves when the branch is writable#6067
atomantic merged 4 commits into
mainfrom
cos/task-mtljhoa7/agent-5a745ea2

Conversation

@atomantic

Copy link
Copy Markdown
Owner

Summary

pr-reviewer stage 3 posts its review and, when everything lines up, merges. A PR that came back with blockers — or that was approved but not merge-ready (red CI, a conflict, a rebase the forge refused) — was left with nobody holding it: the contributor got a review notification, PortOS kept re-polling, and the PR appeared in no one's assigned queue.

Stage 3 now resolves an owner for every PR it does not merge:

  • PortOS can push to the head branch (same-repo, or a fork with maintainerCanModify) and the review is a concrete work order → queue an agent to implement the review, get the PR green, and merge it.
  • Otherwise → assign the opener; the next move is theirs.

A deferred verdict, or findings the coordinator could not anchor to a diff line, never becomes an agent work order — there is nothing concrete to implement, so those go to the opener.

Applied at the four points where a PR stops being watched: a blocking review posted, an approved PR with red CI, the poller's CI failure, and the poller's tick exhaustion.

Design notes

Fails closed on both axes. An unknown head-repository relationship counts as no write access (an older gh, a partial read, a non-GitHub shape must not be optimistically treated as writable). A per-PR ledger caps remediation at 3 attempts and stops a scheduled sweep re-dispatching for a revision it already handed back.

No contributor prose reaches the remediation prompt. It points the agent at the PR to read the review PortOS already posted, so screened titles, bodies, and diffs stay behind the Stage 1 model-abuse boundary. The only contributor-derived values in the prompt are the PR number, URL, head branch name, and author login.

buildCiMergeGateSteps gains a deleteBranch option, and the remediation prompt asks for false: push rights on a fork are not permission to delete a contributor's branch. Existing callers keep the default and still emit --delete-branch.

The remediation task's worktree is the agent's own. createWorktree's existing-branch path resolves origin/<branch> only, so PortOS cannot attach one to a fork head — the prompt has the agent create a throwaway worktree via gh pr checkout. #6064 fixes that gap at the worktree layer for every caller (it also breaks /resolve and spawnReviewLoopFollowUp on fork PRs today); once it lands, this task becomes useWorktree: true and that step goes away.

Review findings addressed

  • The prompt claimed "@<login> left the head branch writable by maintainers" unconditionally — false for a same-repo head, where no contributor did that. Both halves are now conditional on the resolved access, each branch covered by a test.
  • The ledger was written wholesale from a pre-queue snapshot, but two passes own entries in it and can be in flight together (cos.js fires the perpetual refill before the completing output hook settles) — one pass erased the other's entry, dropping the dedup and re-spawning an agent for a PR one was already working. persistState now accepts a patch function evaluated against fresh state inside its serialized queue; a pass applies only its own entries, merged by PR number.
  • spawnPrRemediationFollowUp returned null both for a duplicate (an agent already owns the PR) and for a failed write, so the caller assigned the opener on top of a running agent. It now returns a discriminated status; only a real failure falls back, and an already-queued observation no longer burns an attempt.

Test plan

  • npm test -w server1904 files, 38,445 tests, 0 failures.
  • New: server/lib/prHandbackPolicy.test.js (the pure disposition matrix, incl. fail-closed write access), server/services/prRemediationFollowUp.test.js (task shape, both prompt branches, untrusted-content containment, no --delete-branch, queued vs already-queued vs failed), and a handback block in server/services/issueWatcher.test.js (remediate-vs-assign per write access, unanchorable and deferred reviews, red CI, per-revision dedup, attempt budget, poller tick exhaustion, ledger merge, in-flight-agent double-ownership).
  • The two regression tests for the review findings were bypass-probed: both fail against the pre-fix code and pass against the fix.

…en the branch is writable

pr-reviewer stage 3 posted its review and, when everything lined up, merged.
A PR that came back with blockers — or that was approved but not merge-ready
(red CI, a conflict, a rebase the forge refused) — was left with nobody holding
it: the contributor got a review notification, PortOS kept re-polling, and the
PR appeared in no one's assigned queue.

Stage 3 now resolves an owner for every PR it does not merge. When PortOS can
push to the head branch (same-repo, or a fork with maintainerCanModify) and the
review is a concrete work order, it queues an agent to implement the review,
get the PR green, and merge it. Otherwise the opener is assigned and the next
move is theirs. A deferred verdict or findings that could not be anchored to a
diff line never become an agent work order — there is nothing to implement.

Fails closed on both axes: an unknown head-repository relationship counts as
no write access, and a per-PR ledger caps remediation at 3 attempts and stops
a scheduled sweep re-dispatching for a revision it already handed back.

The remediation prompt carries no contributor prose — it points the agent at
the PR to read the review PortOS already posted, so screened titles, bodies,
and diffs stay on the far side of the Stage 1 model-abuse boundary. Its
CI-gate/merge tail reuses buildCiMergeGateSteps, which gains a deleteBranch
option: push rights on a fork are not permission to delete someone else's
branch.
…fork claim

The remediation prompt asserted "@<login> left the head branch writable by
maintainers" unconditionally. For a same-repo head there is no contributor who
did that, so the instruction handed the agent a false claim about a real
person. Both halves of the sentence are now conditional on the resolved write
access, and each branch's wording is covered by a test.

Also drops three dead fallbacks the review surfaced: a headRefOid alternative
the sole caller never passes, a taskId seed both branches always overwrite, and
a comment naming a prompt-template placeholder that does not exist in this file.
…double-owning a PR

Two reviewer findings, both reachable in one sequence.

The ledger was written wholesale from a snapshot taken before the write queue
was entered. processTaskOutput and processPendingApprovals both own entries in
it and can be in flight together for one app -- cos.js fires the perpetual
refill on agent:completed before the completing task's own output hook has
settled -- so one pass silently erased the other's entry. That is not a benign
re-observation: losing an entry drops the same-revision dedup and the attempt
budget, so the next sweep spawns a second remediation agent for a PR one is
already working. persistState now accepts a patch function evaluated against
the freshly-read state inside its serialized queue, and a pass applies only the
entries it produced, merged by PR number.

(The wholesale-replace shape is pre-existing on approvedPullRequests, which is
left alone: losing a poll entry only costs a re-review, while losing a ledger
entry spends an agent.)

spawnPrRemediationFollowUp returned null both for a duplicate -- meaning a task
is already queued and an agent OWNS the PR -- and for a genuinely failed write.
The caller could not tell them apart and assigned the opener on top of a
running agent, putting one PR in two queues. It now returns a discriminated
status; only a real failure falls back to the opener, and an already-queued
observation no longer burns an attempt no new agent consumed.

Both regressions are covered by tests verified to fail against the prior code.
The lib README merges with git's union driver, so #6066's edit to that row and
this branch's insertion beside it both survived the rebase. Keeps #6066's
current text and restores alphabetical order.
@atomantic
atomantic force-pushed the cos/task-mtljhoa7/agent-5a745ea2 branch from 82c9449 to a9045f7 Compare September 3, 2026 14:11
@atomantic
atomantic merged commit f5caf02 into main Sep 3, 2026
7 checks passed
@atomantic
atomantic deleted the cos/task-mtljhoa7/agent-5a745ea2 branch September 3, 2026 14:16
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