fix(githubbot): tell a PR's sessions when it closes - #1548
Open
0xAlcibiades wants to merge 2 commits into
Open
Conversation
Closing a pull request returned early with "nothing to drive once closed/merged", so a turn already working the PR never learned. It kept running, kept holding a sandbox slot, and kept pushing commits to a branch whose PR no longer existed -- in one case for a further 1.6 hours on a fleet that was refusing admissions the same night. Worse than the wasted slot: a bare close reads to the agent as "start over" rather than "this was superseded". One turn responded by opening a fresh pull request re-implementing work that had already been collapsed elsewhere, which then had to be found, understood and closed by hand. The close is now delivered to the PR's sessions as an appended message. It is context for whatever is already running, not new work, so no execution is started and no sandbox slot is consumed. The turn finishes its current step knowing the outcome rather than discovering nothing. The turn is deliberately not cancelled. Cancelling would discard unpushed work, which is the loss this fleet already suffers often enough. Both PR-scoped threads are told, because a close matters to the management turn driving CI and merges and to a review-response turn addressing feedback on the same PR; they are separate sessions and either may be mid-flight. Merged and closed-without-merging say different things, since that is the distinction the duplicate PR turned on.
The close notice is appended through the session API, which mints the session on the first append. Guarding on nothing, every closed PR in the workspace -- including ones nobody ever handed to the bot -- created a management and a review session that held only the notice. Fetch the PR and skip when it is not owned, matching the ownership check every other lifecycle action already does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1547
Change
handlePullRequestEventno longer returns early onclosed. The close isappended to the PR's sessions as durable context.
forwardToSessionApiwith noexecuteMessageappends and returns withoutstarting an execution, so a close consumes no sandbox slot — which matters,
because the whole problem is a turn holding one it should not.
Three decisions
The turn is not cancelled. Cancelling matches the intent of a close but
discards unpushed work, which is the loss a close is often trying to avoid. On a
fleet where turns run for hours that trades one silent loss for another. Telling
the turn is the cheaper half and stops the duplicate-work case on its own; a
cancel or a bound is a reasonable follow-up once the notification exists.
Merged and closed-without-merging say different things. A bare close reads
to an agent as "start over", which is what produced a fresh PR re-implementing
work that had already been collapsed elsewhere. The closed message explicitly
says not to re-open or re-implement; the merged one says the work is complete.
Both PR-scoped threads are notified. The management turn driving CI and
merges and a review-response turn addressing feedback are separate sessions for
the same PR, and either may be mid-flight. Notification is per-thread
best-effort, so one failing does not stop the other being told, and a missed
notice does not fail the webhook.
Testing
Three new tests: both thread keys are notified; no
/executecall is made (theno-slot property); and the merged and closed wordings differ, with the closed
one carrying the "do not re-implement" instruction.
bun testinservices/githubbot: 104 pass.bun run check:typesclean.