Stop resuming implementers after PR completion#328
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesImplementer exit PR-gated completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the orchestrator factory to complete an issue immediately upon an implementer agent's exit if a completion PR already exists, rather than resuming the agent. It adds the #issueHasCompletionPr helper method and corresponding unit tests. The reviewer pointed out a critical issue where draft PRs could trigger premature completion and block the exit handler for up to 120 seconds, and suggested ignoring draft PRs in the check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!issue) { | ||
| return false | ||
| } | ||
| return Boolean(await this.#completionPrForIssue(issue)) |
There was a problem hiding this comment.
Issue: Draft PRs trigger premature completion and block the exit handler
In sweepPrStateCompletions, draft PRs are explicitly ignored because they are not yet ready for review or merging. However, in #issueHasCompletionPr, any resolved PR (including draft PRs) is treated as a valid completion signal.
This leads to two major issues:
- Premature Completion: The issue will be marked as
donein Linear and Slack even though the PR is still a draft. - Blocking the Exit Handler: Since a draft PR is not merge-ready,
this.#completeIssuewill triggerthis.#runCompletionMergeGate, which callsthis.#waitForMergeReady. This will block/sleep for up to 120 seconds (12 attempts × 10 seconds) inside the agent exit handler, severely impacting the orchestrator's responsiveness.
Recommendation
Update #issueHasCompletionPr to ensure that draft PRs are ignored, matching the logic in sweepPrStateCompletions.
const pr = await this.#completionPrForIssue(issue)
return Boolean(pr && !pr.draft)
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c466f2a4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!issue) { | ||
| return false | ||
| } | ||
| return Boolean(await this.#completionPrForIssue(issue)) |
There was a problem hiding this comment.
When an implementer exits and the matching PR is still a draft, this new check treats any resolved PR as completion and calls #completeIssue, which marks the Linear issue done and releases both agents. The existing PR sweep explicitly keeps draft PRs in flight (pr.draft increments completionSweepDraftPr and returns), so this exit path bypasses that guard for the same resolver output. Please inspect the resolved PR and return false for drafts before completing.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!issue) { | ||
| return false | ||
| } | ||
| return Boolean(await this.#completionPrForIssue(issue)) |
There was a problem hiding this comment.
🔴 Draft PRs incorrectly trigger issue completion on implementer exit
The new #issueHasCompletionPr method at line 1636 uses Boolean(await this.#completionPrForIssue(issue)) to determine if the issue should be completed. However, #completionPrForIssue can return a PR object with draft: true — both resolveIssuePrFromMount (factory.ts:2525) and resolveIssuePrFromGh (factory.ts:2576) include draft PRs in their results. The existing completion sweep at lines 696-699 explicitly checks if (pr.draft) and skips such PRs, but the new exit handler path does not. This means when an implementer exits (e.g., with worker_exited) after opening a draft PR, the issue will be incorrectly completed instead of allowing the agent to resume and finish work.
| return Boolean(await this.#completionPrForIssue(issue)) | |
| const pr = await this.#completionPrForIssue(issue) | |
| return Boolean(pr && !pr.draft) |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
|
✅ APPROVE (reviewer: ar-254-review) — AR-254, PR #328 @ 6c466f2 Verified independently (checked out the branch, ran it myself):
All 3 acceptance criteria met:
Design is right:
Non-blocking note (fast-follow, do NOT hold merge):
Out of scope (correctly): the relay#1116 broker name-release bug itself. Cleared to merge at the maintainer's discretion (per repo policy I'm not merging). Nice work @ar-254-impl-main @ar-254-impl-linear. |
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
3 similar comments
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
#issueHasCompletionPr treated any resolved PR — including drafts — as a completion signal, so an implementer exiting while its PR was still a draft marked the issue done and released both agents prematurely, bypassing the #sweepPrStateCompletions draft guard. Flagged by gemini/codex/devin at factory.ts:1636. Gate on !pr.draft so only a ready PR counts as completion; a draft-PR exit falls through to the normal resume path. Adds a test for the draft case (no done, no release, resumes). Full factory-sdk suite green; typecheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
1 similar comment
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
|
ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed. pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
|
pr-reviewer could not complete review for #328 in AgentWorkforce/pear. |
|
Findings No blocking findings against the current checkout. I made no file edits. Addressed comments
Validation
|
1 participant
Summary
Tests