From e906775c717d452df1fecaf090f6ce89cc577f23 Mon Sep 17 00:00:00 2001 From: brady gaster Date: Fri, 21 Aug 2026 01:09:20 -0700 Subject: [PATCH] fix(gh-aw): refill dispatch slots from the root, not the parent epic The implement-worker refilled a freed dispatch slot by dispatching `/squad implement` against the completing task's *immediate parent epic*. In a three-level tree (root -> epics -> leaf tasks) that scopes the refill to one epic: when Epic A drains, the worker finds nothing, exits green, and Epic B's unstarted leaf tasks are never reached. Slots sit idle with no error surfaced -- the same silent-no-op signature as #1772. Walk the parent chain to the root issue and dispatch that instead. squad.md's implement mode already descends the full sub-tree recursively (#1778), so root-scoped dispatch covers every sibling epic without any new traversal code in the worker. Budgets are untouched: dispatch-workflow max stays at 2 and the 3-slot concurrency cap in squad.md's Epic Dispatch still governs selection. The bug was traversal scope, not budget, and raising max would widen blast radius on the empty-probe path closed in #1778. Leaf-only rule hardened, not regressed. Root-scoped refill descends past drained-but-still-open epics, which have no *open* sub-issues and so passed squad.md's previous leaf test -- reviving the #1758 defect 2 shape at the end of an epic's life. Leafness is now "no sub-issues at all" plus not labeled epic/initiative. Testing: test/gh-aw-implement-workflow.test.ts gains a traversal simulator rather than a wording assertion (#1784 showed prompt-text presence cannot prove prompt obedience). It parses the shipped dispatch payload, binds its issue_number placeholder to the traversal it names, and runs that traversal over a two-sibling-epic fixture using squad.md's own leaf rule and slot budget. Verified red against the pre-fix workflows: 3 failures, including "refill must scan from the root, not the drained parent epic: expected 110 to be 100". Closes #1779 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 70370e36-33b0-4786-bd72-4cf15518daa6 --- test/gh-aw-implement-workflow.test.ts | 238 +++++++++++++++++++++++++- test/gh-aw-quality.test.ts | 2 +- workflows/squad-implement-worker.md | 32 +++- workflows/squad.md | 13 +- 4 files changed, 275 insertions(+), 10 deletions(-) diff --git a/test/gh-aw-implement-workflow.test.ts b/test/gh-aw-implement-workflow.test.ts index cb4949c95..c2a250f50 100644 --- a/test/gh-aw-implement-workflow.test.ts +++ b/test/gh-aw-implement-workflow.test.ts @@ -141,7 +141,7 @@ describe('gh-aw implement workflows', () => { workflow_name: 'squad', inputs: { command: 'implement', - issue_number: '{parent-epic-number}', + issue_number: '{root-issue-number}', }, }); expect(payload.command).toBeUndefined(); @@ -170,3 +170,239 @@ describe('gh-aw implement workflows', () => { expect(guide).toContain('The single command installs the dedicated worker first'); }); }); + +// --------------------------------------------------------------------------- +// Cross-sibling refill traversal (#1779) +// --------------------------------------------------------------------------- +// The worker refills a freed dispatch slot by asking `squad`'s implement mode to +// re-scan a sub-tree. WHICH sub-tree it names is the whole defect: naming the +// completing task's immediate parent epic scopes the refill to that epic, so +// once the epic drains the run exits green while sibling epics still hold +// unstarted leaf tasks (#1779). Naming the root makes the scan cover every +// sibling epic. +// +// These tests do NOT assert that the prompt contains particular wording -- a +// substring check cannot prove a prompt is obeyed, as #1784 demonstrated +// empirically. Instead they parse the machine-readable dispatch payload the +// worker ships, bind its `issue_number` placeholder to the traversal it names, +// and RUN that traversal over a fixture tree using `squad.md`'s own leaf-only +// descent and slot budget. The assertion is on where traversal lands. +// +// Against the pre-fix payload (`{parent-epic-number}`) the traversal returns an +// empty dispatch set for the drained-epic fixture and these tests go red. + +interface FixtureIssue { + number: number; + parent: number | null; + state: 'open' | 'closed'; + /** Leaf already has an open implementation PR -- occupies a concurrency slot. */ + active?: boolean; +} + +/** + * Root #100 + * ├─ Epic A #110 (open) + * │ ├─ #111 closed merged earlier + * │ └─ #112 closed <- the task whose PR just merged; Epic A is now drained + * └─ Epic B #120 (open) + * ├─ #121 open + * └─ #122 open + */ +const TWO_EPIC_TREE: FixtureIssue[] = [ + { number: 100, parent: null, state: 'open' }, + { number: 110, parent: 100, state: 'open' }, + { number: 111, parent: 110, state: 'closed' }, + { number: 112, parent: 110, state: 'closed' }, + { number: 120, parent: 100, state: 'open' }, + { number: 121, parent: 120, state: 'open' }, + { number: 122, parent: 120, state: 'open' }, +]; + +function byNumber(tree: FixtureIssue[], number: number): FixtureIssue { + const issue = tree.find(candidate => candidate.number === number); + if (!issue) throw new Error(`fixture has no issue #${number}`); + return issue; +} + +function parentOf(tree: FixtureIssue[], number: number): number | null { + return byNumber(tree, number).parent; +} + +/** Walk the parent chain to the topmost ancestor, guarding against cycles. */ +function rootOf(tree: FixtureIssue[], number: number): number { + const seen = new Set([number]); + let current = number; + for (;;) { + const parent = parentOf(tree, current); + if (parent === null || seen.has(parent)) return current; + seen.add(parent); + current = parent; + } +} + +function openChildren(tree: FixtureIssue[], number: number): FixtureIssue[] { + return tree.filter(issue => issue.parent === number && issue.state === 'open'); +} + +function anyChildren(tree: FixtureIssue[], number: number): FixtureIssue[] { + return tree.filter(issue => issue.parent === number); +} + +/** + * `squad.md` Step 1.3/1.4: descend recursively through every level and keep the + * open descendants that group nothing at all. Intermediate parents (epics, the + * root) are never returned -- that is the leaf-only rule from #1758 defect 2, + * modeled here so a regression in it fails these tests. + * + * Leafness is "has no sub-issues at all", not "has no OPEN sub-issues". A + * drained epic -- every child implemented and closed, the epic itself still + * open -- passes the open-children-only test and would be dispatched as if it + * were implementable. Root-scoped refill walks straight into exactly that + * state, so the distinction is load-bearing here rather than academic. + */ +function openLeafDescendants(tree: FixtureIssue[], target: number): number[] { + const leaves: number[] = []; + const walk = (number: number): void => { + for (const child of openChildren(tree, number)) { + if (anyChildren(tree, child.number).length === 0) leaves.push(child.number); + else walk(child.number); + } + }; + walk(target); + return leaves.sort((a, b) => a - b); +} + +/** + * Bind the shipped payload's `issue_number` placeholder to the traversal it + * names. An unrecognized placeholder is a hard failure rather than a silent + * pass -- the whole point is that this token determines runtime scope. + */ +function resolveRefillTarget(placeholder: string, tree: FixtureIssue[], mergedIssue: number): number { + switch (placeholder) { + case '{root-issue-number}': + return rootOf(tree, mergedIssue); + case '{parent-epic-number}': + return parentOf(tree, mergedIssue) ?? mergedIssue; + default: + throw new Error( + `Unrecognized continuation dispatch placeholder "${placeholder}". ` + + 'Update resolveRefillTarget() to model the traversal it names, ' + + 'and prove the model still reaches sibling-epic leaves.', + ); + } +} + +/** `squad.md` Epic Dispatch: exclude active leaves, then fill available slots in issue order. */ +function simulateRefill( + placeholder: string, + tree: FixtureIssue[], + mergedIssue: number, + slotCap: number, +): { target: number; dispatched: number[] } { + const target = resolveRefillTarget(placeholder, tree, mergedIssue); + const leaves = openLeafDescendants(tree, target); + const activeCount = leaves.filter(number => byNumber(tree, number).active).length; + const availableSlots = Math.max(0, slotCap - activeCount); + const ready = leaves.filter(number => !byNumber(tree, number).active); + return { target, dispatched: ready.slice(0, availableSlots) }; +} + +describe('gh-aw implement worker: cross-sibling refill traversal (#1779)', () => { + const dispatcher = read('workflows/squad.md'); + const worker = read('workflows/squad-implement-worker.md'); + + const continuation = continuationSection(worker); + const payloadBlock = continuation.match(/```json\r?\n([\s\S]*?)\r?\n```/)?.[1]; + const placeholder = (JSON.parse(payloadBlock ?? '{}') as { inputs?: { issue_number?: string } }).inputs + ?.issue_number; + const slotCap = Number( + dispatcher.match(/available-slots = max\(0, (\d+) - active-implementation-count\)/)?.[1], + ); + + it('exposes a parseable refill scope and slot budget', () => { + expect(payloadBlock, 'continuation must ship a JSON dispatch payload').toBeDefined(); + expect(placeholder, 'continuation payload must name the refill target').toBeTruthy(); + expect(Number.isFinite(slotCap) && slotCap > 0, 'squad.md must declare a numeric slot cap').toBe(true); + }); + + // SC-2: after Epic A drains, the refill must reach Epic B's leaves. + it('reaches a sibling epic once the completing task drains its own epic', () => { + const { target, dispatched } = simulateRefill(placeholder!, TWO_EPIC_TREE, 112, slotCap); + + expect( + openLeafDescendants(TWO_EPIC_TREE, 110), + 'fixture precondition: Epic A must be drained so only a wider scan can find work', + ).toEqual([]); + expect(target, 'refill must scan from the root, not the drained parent epic').toBe(100); + expect( + dispatched, + 'sibling Epic B still holds unstarted leaf tasks; the freed slot must not sit idle', + ).toContain(121); + expect(dispatched.length).toBeGreaterThan(0); + }); + + // SC-1 corollary: the traversal is full-subtree, not immediate-children. + it('never dispatches an epic or the root itself (leaf-only rule, #1758 defect 2)', () => { + const { dispatched } = simulateRefill(placeholder!, TWO_EPIC_TREE, 112, slotCap); + + for (const number of dispatched) { + expect( + openChildren(TWO_EPIC_TREE, number), + `#${number} has open sub-issues and must never be dispatched as implementable`, + ).toEqual([]); + } + expect(dispatched).not.toContain(100); + expect(dispatched).not.toContain(110); + expect(dispatched).not.toContain(120); + }); + + it('never dispatches a drained-but-open epic as if it were implementable', () => { + // Epic A #110 is open with every child closed. It has no *open* sub-issues, + // so an open-children-only leaf test reclassifies it as implementable. Root- + // scoped refill descends straight past it, which is why this case is now + // reachable and must stay excluded. + const { dispatched } = simulateRefill(placeholder!, TWO_EPIC_TREE, 112, slotCap); + + expect(openChildren(TWO_EPIC_TREE, 110), 'fixture precondition: #110 has no open children').toEqual([]); + expect(anyChildren(TWO_EPIC_TREE, 110).length, 'fixture precondition: #110 still groups issues').toBeGreaterThan(0); + expect(dispatched, 'a drained epic groups issues and is never a leaf task').not.toContain(110); + }); + + it('still refills within the completing epic before it drains', () => { + // #111 merges while #112 is still open: the near case must not regress. + const tree = TWO_EPIC_TREE.map(issue => + issue.number === 112 ? { ...issue, state: 'open' as const } : issue, + ); + const { dispatched } = simulateRefill(placeholder!, tree, 111, slotCap); + + expect(dispatched, 'same-epic refill must still happen').toContain(112); + }); + + it('respects the existing slot budget instead of widening it', () => { + // Epic B's #121 is already in flight, so it occupies one of the slots. + const tree = TWO_EPIC_TREE.map(issue => + issue.number === 121 ? { ...issue, active: true } : issue, + ); + const { dispatched } = simulateRefill(placeholder!, tree, 112, slotCap); + + expect(dispatched).not.toContain(121); + expect(dispatched.length).toBeLessThanOrEqual(slotCap - 1); + + const workerDispatch = yamlBlock(frontmatter(worker), 'dispatch-workflow'); + const dispatchMax = Number(scalarInBlock(workerDispatch, 'max')); + expect(dispatchMax, 'refill scope is the fix; do not paper over it by raising max').toBe(2); + expect(dispatchMax).toBeLessThanOrEqual(slotCap); + }); + + // Negative control: documents the pre-fix scope and proves the model above is + // discriminating rather than vacuously green. + it('control: scoping the refill to the immediate parent epic strands sibling work', () => { + const { target, dispatched } = simulateRefill('{parent-epic-number}', TWO_EPIC_TREE, 112, slotCap); + + expect(target).toBe(110); + expect(dispatched, 'this is the #1779 defect: drained epic yields no dispatch').toEqual([]); + expect(placeholder, 'the shipped payload must not use the stranding scope').not.toBe( + '{parent-epic-number}', + ); + }); +}); diff --git a/test/gh-aw-quality.test.ts b/test/gh-aw-quality.test.ts index ccc4781d0..ff8607317 100644 --- a/test/gh-aw-quality.test.ts +++ b/test/gh-aw-quality.test.ts @@ -1062,7 +1062,7 @@ describe('gh-aw: merge continuation dispatch contract', () => { expect(payload.issue_number, 'issue_number must not be a top-level dispatch_workflow argument').toBeUndefined(); expect(payload.inputs).toEqual({ command: 'implement', - issue_number: '{parent-epic-number}', + issue_number: '{root-issue-number}', }); for (const key of Object.keys(payload.inputs ?? {})) { diff --git a/workflows/squad-implement-worker.md b/workflows/squad-implement-worker.md index e939356ca..23628f5b0 100644 --- a/workflows/squad-implement-worker.md +++ b/workflows/squad-implement-worker.md @@ -211,7 +211,7 @@ This workflow has two modes: 1. A `workflow_dispatch` implements issue `${{ github.event.inputs.issue_number }}` and opens a focused pull request. -2. A merged `pull_request` continues the parent epic. +2. A merged `pull_request` continues the root issue's remaining sub-tree. ## Continue Parent Epic After Merge @@ -223,7 +223,26 @@ For a merged pull request: relationship, falling back to its `Parent: #N` body line. 3. If no parent epic exists, comment on the merged pull request saying its issue is standalone and that no further work was queued, then stop. -4. WRITE-ONCE: call the prompt-listed `dispatch_workflow` safe-output tool +4. RESOLVE THE ROOT, NOT THE PARENT. Keep walking the parent chain upward from + the parent epic — native parent relationship first, `Parent: #N` body line as + fallback — until you reach an issue with no parent. That topmost ancestor is + the **root issue**, and it is the dispatch target. Do not stop at the + immediate parent epic. A three-level tree (root → epics → leaf tasks) puts + sibling epics beside the completing task's epic; dispatching the immediate + parent scopes the refill to that one epic, so once it drains the run exits + green while sibling epics still hold unstarted leaf tasks and the + concurrency slots sit idle. Dispatching the root makes `squad`'s implement + mode descend the **entire** remaining sub-tree, which is what refills the + freed slot from wherever work actually remains. Guard the walk against + cycles: track visited issue numbers and treat a repeat as the root. If the + parent chain cannot be walked past the parent epic, use the parent epic as + the root rather than skipping the dispatch. +5. Selection and budget stay where they already are. `squad`'s implement mode + dispatches only **leaf tasks** — open descendants with no open sub-issues — + and never an epic or the root itself, and it caps concurrent work with its + own available-slots calculation. Do not pre-select tasks, widen any cap, or + dispatch a worker directly from here to compensate for a drained epic. +6. WRITE-ONCE: call the prompt-listed `dispatch_workflow` safe-output tool exactly once, and only when the complete payload is ready. NEVER call `dispatch_workflow` with empty, partial, or placeholder arguments to probe or discover its schema. The full schema is already given in this prompt; there @@ -238,7 +257,7 @@ For a merged pull request: "workflow_name": "squad", "inputs": { "command": "implement", - "issue_number": "{parent-epic-number}" + "issue_number": "{root-issue-number}" } } ``` @@ -252,8 +271,11 @@ workflow is dispatched and the visible continuation comment is queued. comment — never a silent exit. Cover both terminal cases: - Parent epic resolved → comment on the parent epic (`item_number` set to the - parent epic number), name the epic, and state that its next children were - queued. + parent epic number), name the epic, name the root issue the refill was + dispatched against, and state which next leaf tasks were queued. When the + parent epic itself has no open leaf tasks left, say so and state that the + refill was widened to the root's remaining sub-tree — never report the epic + as drained without naming that wider scan. - No parent epic → state that the pull request's issue is standalone and that nothing further was queued. diff --git a/workflows/squad.md b/workflows/squad.md index d281513c4..3d4ee8192 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -545,9 +545,16 @@ to this mode so it can automatically refill the parent's available slots. hierarchy (initiative → epic → task), not just immediate children. Also include open issues whose body contains a `Parent: #{ancestor-issue-number}` line for any ancestor, for compatibility with older plans. -4. Identify the **leaf tasks**: open descendants that themselves have no open - sub-issues. Intermediate parents (initiatives and epics that only group other - issues) are never dispatched to a worker — only leaf tasks are implemented. +4. Identify the **leaf tasks**: open descendants that have **no sub-issues at + all** — neither open nor closed — and are not labeled `epic` or `initiative`. + Intermediate parents (initiatives and epics that only group other issues) + are never dispatched to a worker — only leaf tasks are implemented. Use "no + sub-issues at all" rather than "no *open* sub-issues": an epic whose children + have all been implemented and closed stays open until someone closes it, and + an open-children-only test would reclassify that drained epic as a leaf and + dispatch a worker against a grouping issue. That is the #1758 defect 2 + failure shape reappearing at the end of an epic's life, and it is reachable + whenever a refill scan descends from the root across sibling epics. 5. If the target has one or more open leaf descendants, treat the target as a parent and follow the Epic Dispatch procedure below over the leaf-task set. Do not implement the parent body directly.