fix: stop finished PRs from blocking idle pickup - #37
Conversation
A worker's slot now frees when its Dispatch settles. Open PRs waiting for review or merge no longer count against pickup; the merge gate and repair own them. Issues that overlap an open PR are stacked on it with gh stack instead of waiting for it to merge, and repair restacks with gh stack sync. Claude-Session: https://claude.ai/code/session_01E9psw1USAWcT2Vtmcfbgsa
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughIdle pickup now counts active workers through Dispatch settlement, while settled workers with open PRs no longer occupy slots. Before claiming an issue, the coordinator checks for overlap with open, unmerged PRs. The update defines rules for creating and restacking layers, and adds repair handling for stacked PRs whose lower layer merged or needs rebase. Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to Stack repair can release while a stack remains unreconciled or force-push a layer beyond the task’s authorization. Require reconciliation checks and explicit scoped authorization before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Automated repair can now rewrite branches in a PR stack. The workflow limits who may do that, but authorization for every affected branch and recovery from an unsuccessful sync are not fully established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![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: e63561e27b
ℹ️ 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".
| otherwise pick another issue. The spec names the lower branch and head SHA and | ||
| has the worker create the layer with `gh stack add <branch>` from a checkout of | ||
| that branch, open the PR with `gh stack submit --auto --open`, and never edit |
There was a problem hiding this comment.
When the lower PR was created by the existing unstacked pickup flow, merely checking out its branch does not make it a stack layer, so the prescribed gh stack add <branch> cannot proceed. The referenced command documentation says add must run from the top of an existing stack and otherwise exits, while gh stack init is what adopts an existing branch (skills/origin89-gh-stack/references/commands.md:21-38). Initialize the stack with the lower branch, or use the documented worktree-compatible linking flow, before adding the new layer.
Useful? React with 👍 / 👎.
| Stacks keep one writer per layer. Only the coordinator, through a repair | ||
| worker, restacks, and only when no live worker holds any layer of the stack. | ||
| Keep at most three open layers in one stack; beyond that, pick another issue. | ||
| The merge gate merges the bottom layer only, as its rule 1 requires. |
There was a problem hiding this comment.
Once this path creates a stack, its bottom PR is still a stack member, but the merge gate continues to invoke gh pr merge at lines 473–475. The referenced gh-stack instructions explicitly state that gh pr merge cannot merge a stack and require a scoped gh stack merge <target> --yes instead (skills/origin89-gh-stack/SKILL.md:70-75). Consequently the gate cannot land the bottom layer, so repair never gets an opportunity to retarget the upper layers; make the gate detect stacked PRs and use the stack-aware command while preserving its head-change guard.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
skills/origin89-orca/references/unattended-run.md-330-331 (1)
330-331: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winTreat
Sync abortedas a failed stack repair.The repair worker can receive exit status 0 while
gh stack syncmakes no changes. Add explicit handling for this output before releasing the worker.Suggested fix
On a conflict, it follows gh-stack's exit 3 recovery under the same intent rules. +If `gh stack sync` prints `Sync aborted`, treat the sync as unsuccessful even +when it exits 0. Check the stack state and each PR base before release. If the +stack is not reconciled, hand it over instead of releasing the worker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: a6588c98-d47d-4b17-aa99-cc813e868a86
📒 Files selected for processing (1)
skills/origin89-orca/references/unattended-run.md
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| worker restacks with `gh stack sync` instead of merging the base: `sync` | ||
| handles squash-merged layers and retargets each PR to its new base. This is |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,45p' skills/origin89-gh-stack/SKILL.md
sed -n '310,345p' skills/origin89-orca/references/unattended-run.md
rg -n 'task authorization|authorized|authorization|repair worker|gh stack sync' skills/origin89-orca skills/origin89-gh-stackRepository: origin89hq/engineering
Length of output: 7202
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- changed files ---'
git diff --stat 007d10f8f686d110d9a0c62b586398716c5ee90c e63561e27bfeef460b9fad5f7cbf69ad4d9f870d
printf '%s\n' '--- relevant diff ---'
git diff --unified=25 007d10f8f686d110d9a0c62b586398716c5ee90c e63561e27bfeef460b9fad5f7cbf69ad4d9f870d -- skills/origin89-orca/references/unattended-run.md skills/origin89-orca/SKILL.md skills/origin89-commits/SKILL.md skills/origin89-gh-stack/SKILL.md
printf '%s\n' '--- authorization and repair references ---'
rg -n -C 5 'authorization|authorized|worker-start|repair|stack sync|force-push|remote effects|inherits' skills/origin89-commits skills/origin89-orca skills/origin89-gh-stackRepository: origin89hq/engineering
Length of output: 42278
🤖 get_repo_knowledge executed:
get_repo_knowledge origin89hq/engineering /tmp/coderabbit-repo-knowledge/origin89hq-engineering-afe2c562/architecture /tmp/coderabbit-repo-knowledge/origin89hq-engineering-afe2c562/conventions
Length of output: 24114
🏁 Script executed:
#!/bin/bash
set -o pipefail
printf '%s\n' '--- Orca authorization contract ---'
sed -n '1,125p' skills/origin89-orca/SKILL.md
printf '%s\n' '--- commits authorization contract ---'
sed -n '1,180p' skills/origin89-commits/SKILL.md
printf '%s\n' '--- review follow-up authorization and wait-for-merge ---'
rg -n -C 8 'Wait for merge|authorization|authorized|force-push|rebase|push|scope|worker' skills/origin89-commits/references/pr-review-follow-up.md
printf '%s\n' '--- gh-stack scope and authorization ---'
rg -n -C 8 'authorization|authorized|scope|force-push|sync|own layers|branch|PR' skills/origin89-gh-stack/SKILL.md skills/origin89-gh-stack/references
printf '%s\n' '--- Orca worker inheritance references ---'
rg -n -C 8 'inherit|authority|authorization|scope|worker-start|repair worker|repair' skills/origin89-orca skills/origin89-commitsRepository: origin89hq/engineering
Length of output: 42144
Require a scoped sync grant before stacked repair.
gh stack sync rebases and pushes remote branches. A generic push grant does not authorize those additional effects. This reachable repair path starts a worker for a stacked PR when its lower layer merged or needsRebase is set, but it does not require the worker spec to include authorization for sync across every affected stack layer. The worker can therefore force-push an unauthorized layer.
Suggested fix
Start one supervised repair worker in the PR's existing worktree with
-`worker-start --worktree path:<worktree>`. Its spec applies the Wait for merge
+`worker-start --worktree path:<worktree>`. Before starting a stacked repair,
+verify that the coordinator's task authorization explicitly covers `gh stack
+sync`, including its rebase and force-push effects, for every layer that the
+command may update. Put that scoped grant and the inspected stack layer set in
+the worker spec; otherwise hand the PR over. Its spec applies the Wait for merge
section of the review follow-up: merge the base into the branch, never rebase
Change
Idle pickup stopped taking new issues once the user had a few open PRs, even when every worker had finished and the PRs were only waiting for review or merge. The precheck counted "ready PRs awaiting review" against capacity, and nothing said the worker should settle before merge.
Now a worker holds a slot only until its Dispatch settles. It runs the bounded review phase of the review follow-up, then settles; the merge gate and conflict repair own the PR after that. The PR's linked worktree still keeps its issue from being picked up twice.
The old rule made a second issue touching the same generated file (such as the km43 protocol registry) wait for the first to merge. The new "Stack overlapping work" section stacks it on the open PR with gh stack instead, so its allocation follows the lower change and cannot collide. It keeps one writer per layer, lets only a coordinator repair worker restack, and caps a stack at three open layers. Stacked PRs carry a
Stacked on #Nbody line. Repair now also covers a stacked PR whose lower PR merged without its merge commit in the head, or that reportsneedsRebase, usinggh stack sync, which handles squash merges and retargets bases. With squash merges and branch deletion, GitHub retargets such a layer tomainwithout it necessarily showing as conflicting, so the body line is how a later pass finds it.The local Origin89 issue coordinator job has already been updated to match: its precheck no longer applies the open-PR limit,
pr-conflict-precheck.shflags stacked PRs that need a restack, and its prompt settles workers after the review phase and stacks overlapping issues.Validation
just check: 32 tests passed; link and JSON checks passed.Stacked on #Nparsing on sample PR bodies and the compare API'sahead/behindstatus on real commits.https://claude.ai/code/session_01E9psw1USAWcT2Vtmcfbgsa