-
Notifications
You must be signed in to change notification settings - Fork 0
fix: stop finished PRs from blocking idle pickup #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,8 +182,11 @@ coordinator; only unresolved decisions reach the user in that conversation. Its | |
| precheck continues only when: | ||
|
|
||
| - fewer issue workers in the repository are active than the limit, starting at | ||
| one; include workers waiting for a reply, not just those generating output; | ||
| - fewer than the limit of the user's ready PRs await review; | ||
| one; include workers waiting for a reply, not just those generating output. | ||
| A worker is active only until its Dispatch settles. A settled worker's open | ||
| PR does not hold a slot while it waits for review or merge: the merge gate | ||
| and [repair](#repair-conflicting-prs) own it from there, and its linked | ||
| worktree already keeps its issue from being picked up again; | ||
| - an open `agent-ready` issue exists without `agent-working`, `needs-spec`, | ||
| `human-only`, an assignee, a linked Orca worktree, or an open blocker | ||
| (`issue_dependencies_summary.blocked_by` above 0 in | ||
|
|
@@ -199,9 +202,11 @@ start the computation, then list again after a short wait. | |
|
|
||
| Recheck those conditions, then pick one issue: first an issue that open issues | ||
| are blocked by, then milestone order, then the oldest. Skip issues blocked by an | ||
| open issue. Do not run two issues at once that allocate in the same generated | ||
| file, such as a protocol registry: the second waits for the first to merge, | ||
| because a sibling merging first leaves the other PR conflicting. Before claiming, read the issue body and comments. When the text | ||
| open issue. Before claiming, check whether the issue would edit the same files | ||
| as one of the user's open, unmerged PRs, such as a shared generated file or | ||
| protocol registry; a sibling merging first would leave the second PR | ||
| conflicting. Stack it instead of waiting, per [Stack overlapping work](#stack-overlapping-work). | ||
| Before claiming, read the issue body and comments. When the text | ||
| names an open issue as a prerequisite and no blocked-by link records it, do not | ||
| claim the issue; skip it and list it in the report so the link gets added. | ||
| Idle pickup requires explicit authority for workers to commit, push, | ||
|
|
@@ -229,6 +234,10 @@ merge, publish releases, flash firmware, or operate equipment. | |
| A successful send proves enqueue only; a worker reply establishes receipt. | ||
| Process the full delivery before acknowledgment. The worker follows its live | ||
| preamble for mailbox checks, blocking `ask`, and exactly one `worker_done`. | ||
| It runs the review phase of the | ||
| [review follow-up](../../origin89-commits/references/pr-review-follow-up.md) | ||
| with a bounded cap, then settles; it does not stay live to wait for merge, | ||
| so a finished PR frees its slot. | ||
| 4. Answer questions from available evidence within the grant. Missing product | ||
| information must reach the user; never invent an answer. A worker that cannot | ||
| finish records the blocker on the issue, which is the durable record once the | ||
|
|
@@ -264,6 +273,25 @@ back to a standalone agent. Do not redispatch an already running standalone | |
| worker merely to attach messaging; preserve its work and use Orca's documented | ||
| recovery or terminal-reuse route once its state permits that transition. | ||
|
|
||
| ### Stack overlapping work | ||
|
|
||
| When the next issue overlaps an open, unmerged PR's files, or builds on that | ||
| PR's change, launch its worker on a new layer above that PR's branch with | ||
| [gh-stack](../../origin89-gh-stack/SKILL.md) rather than on the default branch. | ||
| The new layer contains the lower change, so allocations in a shared file follow | ||
| it and cannot collide, and the PR shows only its own diff. Stack only on a PR | ||
| whose worker has settled and that lacks `needs-human-review` and `human-only`; | ||
| 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 | ||
| the lower layer. A finding that belongs in the lower layer goes back to the | ||
| coordinator. | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Once this path creates a stack, its bottom PR is still a stack member, but the merge gate continues to invoke Useful? React with 👍 / 👎. |
||
|
|
||
| ### Repair conflicting PRs | ||
|
|
||
| A picked-up PR can start conflicting after its worker settles, typically when a | ||
|
|
@@ -297,6 +325,13 @@ unneeded update restarts reviews. A resolution that would choose between | |
| behaviours is a blocker: the worker comments it on the PR and the coordinator | ||
| hands the PR over. | ||
|
|
||
| A stacked PR also needs repair when its lower layer has merged, so the gate can | ||
| reach it, or when `gh stack view --json` reports `needsRebase`. Its repair | ||
| 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 | ||
|
Comment on lines
+330
to
+331
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 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:
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
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 |
||
| the only repair that force-pushes, and only the stack's own layers. On a | ||
| conflict, it follows gh-stack's exit 3 recovery under the same intent rules. | ||
|
|
||
| A repair does not use the issue limit and does not claim or relabel the PR's | ||
| issue. Run at most two repairs at once across the job's repositories, and start | ||
| them before any new issue. Verify the new head is `MERGEABLE` with checks green | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 saysaddmust run from the top of an existing stack and otherwise exits, whilegh stack initis 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 👍 / 👎.