From 04f4140445096f6111288c2b8d9c34a72e3c432c Mon Sep 17 00:00:00 2001 From: Copilot Date: Wed, 14 Oct 2026 17:26:45 +0300 Subject: [PATCH] fix(gh-aw): bind implementation acceptance to selected plan revision (#2206) Require granular activation (impl-accepted and impl-phases-accepted) to uniquely reference the selected implementation plan comment revision and match any recorded Implementation SHA-256 before reservation or issue creation. Closes #2206 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cf0811c-a02a-41a7-ab00-904b6900db7b --- test/gh-aw-activation-finalization.test.ts | 24 ++++- ...h-aw-activation-label-provisioning.test.ts | 6 +- .../gh-aw-activation-summary-outcomes.test.ts | 6 +- test/gh-aw-plan-lifecycle.test.ts | 87 ++++++++++++++++++- workflows/shared/squad-activation.mjs | 46 +++++++++- workflows/squad-workflows.manifest.json | 2 +- 6 files changed, 163 insertions(+), 8 deletions(-) diff --git a/test/gh-aw-activation-finalization.test.ts b/test/gh-aw-activation-finalization.test.ts index 1b3af1ae5..c65451ef9 100644 --- a/test/gh-aw-activation-finalization.test.ts +++ b/test/gh-aw-activation-finalization.test.ts @@ -118,7 +118,11 @@ function fixture({ count = 2, epic = false } = {}) { const comments = new Map>([[10, comment(10, planBody)]]); if (epic) { comments.set(7, comment(7, block('Structured data', envelope('scope-accepted')))); - comments.set(8, comment(8, block('Structured data', envelope('impl-accepted')))); + comments.set(8, comment(8, [ + '## Implementation Accepted', + `- **Implementation plan version:** ${comments.get(10)!.html_url}`, + block('Structured data', envelope('impl-accepted')), + ].join('\n'))); } function issue(number: number, values: Record = {}) { return { @@ -1603,6 +1607,24 @@ describe('combined implementation-phase acceptance and activation', () => { await expect(f.create(capture)).rejects.toThrow(); expect(f.writes).toEqual([]); }); + + it('rejects activation when implementation acceptance references an older plan revision (#2206)', async () => { + const f = fixture({ epic: true }); + f.comments.set(20, f.comment(20, f.comments.get(10)!.body)); + writeFileSync(f.outputPath, JSON.stringify({ items: [{ type: 'activate_plan', plan_comment_id: '20' }] })); + await expect(f.prepare()).rejects.toThrow(/implementation acceptance must uniquely reference the selected implementation plan revision/); + expect(f.writes).toEqual([]); + expect(f.createIssue).not.toHaveBeenCalled(); + + f.comments.set(21, f.comment(21, [ + '## Implementation Accepted', + `- **Implementation plan version:** ${f.comments.get(20)!.html_url}`, + block('Structured data', envelope('impl-accepted')), + ].join('\n'))); + const capture = await f.prepare(); + expect(capture.plan_comment.id).toBe(20); + expect(capture.approvals.implementation.id).toBe(21); + }); }); describe('explicit pending-source current-state reconciliation', () => { diff --git a/test/gh-aw-activation-label-provisioning.test.ts b/test/gh-aw-activation-label-provisioning.test.ts index cb8885d57..646016e96 100644 --- a/test/gh-aw-activation-label-provisioning.test.ts +++ b/test/gh-aw-activation-label-provisioning.test.ts @@ -84,7 +84,11 @@ function fixture({ owners = ['Builder', 'Reviewer', '@copilot'], implementation const comments = new Map([[10, comment(10, planBody)]]); if (implementation) { comments.set(7, comment(7, block('scope-accepted'))); - comments.set(8, comment(8, block('impl-accepted'))); + comments.set(8, comment(8, [ + '## Implementation Accepted', + `- **Implementation plan version:** ${comments.get(10)!.html_url}`, + block('impl-accepted'), + ].join('\n'))); } const issue = (number: number, params: any = {}) => ({ number, id: number + 1000, node_id: `I_labels_${number}`, diff --git a/test/gh-aw-activation-summary-outcomes.test.ts b/test/gh-aw-activation-summary-outcomes.test.ts index d9d504fa1..a479fb875 100644 --- a/test/gh-aw-activation-summary-outcomes.test.ts +++ b/test/gh-aw-activation-summary-outcomes.test.ts @@ -83,7 +83,11 @@ function fixture(command: string = 'activate', secondOwner = 'Kint') { const comments = new Map([[10, comment(10, planBody)]]); if (granular) { comments.set(7, comment(7, block('Structured data', envelope('scope-accepted')))); - comments.set(8, comment(8, block('Structured data', envelope('impl-accepted')))); + comments.set(8, comment(8, [ + '## Implementation Accepted', + `- **Implementation plan version:** ${comments.get(10)!.html_url}`, + block('Structured data', envelope('impl-accepted')), + ].join('\n'))); } const issue = (number: number, values: Record = {}) => ({ number, id: number + 1000, node_id: `I_${number}`, body: '', labels: [], diff --git a/test/gh-aw-plan-lifecycle.test.ts b/test/gh-aw-plan-lifecycle.test.ts index 32fcf55f2..ec2d75220 100644 --- a/test/gh-aw-plan-lifecycle.test.ts +++ b/test/gh-aw-plan-lifecycle.test.ts @@ -232,7 +232,7 @@ function activationFixture(body = taskTable(), command = 'activate', kind = 'pla }); const artifacts = (kind: string) => [...comments.values()].filter(item => parseStructuredData(item.body)?.squad_artifact === kind); - return { prepare, create, finalize, artifacts, comments, comment, issues, createIssue, github, context }; + return { prepare, create, finalize, artifacts, comments, comment, issues, createIssue, github, context, root, outputPath }; } // --------------------------------------------------------------------------- @@ -579,7 +579,11 @@ describe('#1758.1: squad-plan-accept routes granular plans before failing', () = f.comments.set(7, f.comment(7, structured('scope-accepted'))); await expect(f.prepare()).rejects.toThrow(/granular activation requires existing impl-accepted/); expect(f.createIssue).not.toHaveBeenCalled(); - f.comments.set(8, f.comment(8, structured('impl-accepted'))); + f.comments.set(8, f.comment(8, [ + '## Implementation Accepted', + `- **Implementation plan version:** ${f.comments.get(10)!.html_url}`, + structured('impl-accepted'), + ].join('\n'))); const capture = await f.prepare(); expect(capture.plan.kind).toBe('implementation'); expect(capture.operations.map((operation: any) => operation.id)).toEqual(['epic:epic1', 'task:1']); @@ -588,6 +592,85 @@ describe('#1758.1: squad-plan-accept routes granular plans before failing', () = expect(f.artifacts('activated')).toHaveLength(1); }); + it('binds implementation acceptance to the selected plan revision and rejects stale, missing, or ambiguous links (#2206)', async () => { + // 1. Reproduce exact #2206 scenario: plan 10 accepted by comment 13, then newer unaccepted plan 20 is added + const f = activationFixture(implementation, 'plan activate', 'implementation'); + f.comments.set(11, f.comment(11, structured('scope-accepted'))); + f.comments.set(13, f.comment(13, [ + '## Implementation Accepted', + `- **Implementation plan version:** ${f.comments.get(10)!.html_url}`, + structured('impl-accepted'), + ].join('\n'))); + f.comments.set(20, f.comment(20, `${implementation}\n${structured('implementation')}`)); + writeFileSync(f.outputPath, JSON.stringify({ items: [{ type: 'activate_plan', plan_comment_id: '20' }] })); + await expect(f.prepare()).rejects.toThrow(/implementation acceptance must uniquely reference the selected implementation plan revision/); + expect(f.createIssue).not.toHaveBeenCalled(); + expect(f.github.rest.issues.createComment).not.toHaveBeenCalled(); + + // 2. Missing link in impl-accepted rejects + f.comments.set(13, f.comment(13, structured('impl-accepted'))); + await expect(f.prepare()).rejects.toThrow(/implementation acceptance must uniquely reference the selected implementation plan revision/); + + // 3. Ambiguous links (both 10 and 20 referenced) rejects + f.comments.set(13, f.comment(13, [ + '## Implementation Accepted', + `- **Implementation plan version:** ${f.comments.get(10)!.html_url} and ${f.comments.get(20)!.html_url}`, + structured('impl-accepted'), + ].join('\n'))); + await expect(f.prepare()).rejects.toThrow(/implementation acceptance must uniquely reference the selected implementation plan revision/); + + // 4. Matching acceptance for plan 20 (via standard comment-id or URL format) succeeds + f.comments.set(21, f.comment(21, [ + '## Implementation Accepted', + '- **Implementation plan version:** #42 (comment-id: 20)', + structured('impl-accepted'), + ].join('\n'))); + const capture = await f.prepare(); + expect(capture.plan_comment.id).toBe(20); + expect(capture.approvals.implementation.id).toBe(21); + }); + + it('binds phased implementation acceptance to the selected plan revision and rejects stale phase acceptance (#2206)', async () => { + const phasedImpl = [ + '### Phase 1', + taskTable(['| 1 | Phase 1 task | Builder | builder | None | epic1 |'], true), + ].join('\n'); + const f = activationFixture(phasedImpl, 'plan activate phase 1', 'implementation'); + f.comments.set(11, f.comment(11, structured('scope-accepted'))); + f.comments.set(13, f.comment(13, [ + '## Implementation Phase 1 Accepted', + `**Implementation plan:** ${f.comments.get(10)!.html_url}`, + `**Implementation SHA-256:** ${digest(f.comments.get(10)!.body)}`, + structured('impl-phases-accepted', [1]), + ].join('\n'))); + + // Add newer implementation revision 20 without accepting phase 1 for revision 20 + f.comments.set(20, f.comment(20, `${phasedImpl}\n${structured('implementation')}`)); + writeFileSync(f.outputPath, JSON.stringify({ items: [{ type: 'activate_plan', plan_comment_id: '20' }] })); + await expect(f.prepare()).rejects.toThrow(/implementation acceptance must uniquely reference the selected implementation plan revision/); + expect(f.createIssue).not.toHaveBeenCalled(); + + // Even if URL points to 20, a stale SHA-256 on impl-phases-accepted rejects + f.comments.set(14, f.comment(14, [ + '## Implementation Phase 1 Accepted', + `**Implementation plan:** ${f.comments.get(20)!.html_url}`, + `**Implementation SHA-256:** ${'0'.repeat(64)}`, + structured('impl-phases-accepted', [1]), + ].join('\n'))); + await expect(f.prepare()).rejects.toThrow(/implementation acceptance SHA-256 does not match/); + + // Matching URL and SHA-256 for revision 20 succeeds + f.comments.set(15, f.comment(15, [ + '## Implementation Phase 1 Accepted', + `**Implementation plan:** ${f.comments.get(20)!.html_url}`, + `**Implementation SHA-256:** ${digest(f.comments.get(20)!.body)}`, + structured('impl-phases-accepted', [1]), + ].join('\n'))); + const capture = await f.prepare(); + expect(capture.plan_comment.id).toBe(20); + expect(capture.approvals.implementation.id).toBe(15); + }); + it('fails closed without a supported task plan and never turns a program into invented tasks', async () => { for (const kind of ['program', 'research']) { const f = activationFixture('No implementation task table exists.', 'activate', kind); diff --git a/workflows/shared/squad-activation.mjs b/workflows/shared/squad-activation.mjs index 2a9fada31..2a271f3f2 100644 --- a/workflows/shared/squad-activation.mjs +++ b/workflows/shared/squad-activation.mjs @@ -553,12 +553,13 @@ export async function prepareActivation({ github, context, env, root, outputPath validateImplementationVerdict(validation.body, planComment, fullPlan); approvals = { scope, validation }; } else { - const implementation = acceptedComments.find(comment => { + const implementation = acceptedComments.filter(comment => { const value = envelope(comment.body); return value.squad_artifact === 'impl-accepted' || (command.phase !== null && value.squad_artifact === 'impl-phases-accepted' && value.phases?.includes(command.phase)); - }); + }).at(-1); requireThat(implementation, 'granular activation requires existing impl-accepted or selected implementation-phase acceptance'); + validateImplementationAcceptance(implementation, planComment, `${context.repo.owner}/${context.repo.repo}`, origin); approvals = { scope, implementation }; } } @@ -683,6 +684,47 @@ function validationTable(body, columns) { return rows; } +function extractImplementationPlanCommentIds(body, repository, origin) { + const text = String(body ?? '') + .replace(//g, '') + .replace(/```[\s\S]*?```/g, ''); + const escapedRepo = repository.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const ids = []; + const collect = source => { + for (const match of source.matchAll( + new RegExp(`https://github\\.com/${escapedRepo}/issues/${origin}#issuecomment-([1-9]\\d*)\\b`, 'g'), + )) { + ids.push(Number(match[1])); + } + for (const match of source.matchAll(/(?:^|[^A-Za-z0-9_/])#issuecomment-([1-9]\d*)\b/g)) { + ids.push(Number(match[1])); + } + for (const match of source.matchAll(/\bcomment(?:[-_\s]+id)?\s*[:=#]?\s*([1-9]\d*)\b/gi)) { + ids.push(Number(match[1])); + } + }; + const labeledLines = text.split(/\r?\n/).filter(line => + /^(?:[-*]\s+)?(?:\*\*)?(?:implementation(?:\s+plan)?(?:\s+version)?|impl\s+plan(?:\s+version)?|plan\s+version)(?:\*\*)?\s*:/i + .test(line.trim()), + ); + if (labeledLines.length) { + for (const line of labeledLines) collect(line); + } else { + collect(text); + } + return [...new Set(ids.filter(positive))]; +} + +function validateImplementationAcceptance(comment, planComment, repository, origin) { + const body = String(comment.body ?? ''); + const ids = extractImplementationPlanCommentIds(body, repository, origin); + requireThat(ids.length === 1 && ids[0] === planComment.id, + 'implementation acceptance must uniquely reference the selected implementation plan revision'); + const shas = [...body.matchAll(/^\*\*Implementation SHA-256:\*\*\s*(\S+)\s*$/gm)].map(match => match[1]); + requireThat(shas.length <= 1 && (shas.length === 0 || shas[0] === digest(planComment.body)), + 'implementation acceptance SHA-256 does not match the selected implementation plan revision'); +} + function validateImplementationVerdict(body, planComment, plan) { const verdicts = [...body.matchAll(/^RESULT:\s*(PASS|FAIL)\s*$/gm)]; requireThat(verdicts.length === 1 && verdicts[0][1] === 'PASS' && diff --git a/workflows/squad-workflows.manifest.json b/workflows/squad-workflows.manifest.json index ffe25f1a5..aaa97f82b 100644 --- a/workflows/squad-workflows.manifest.json +++ b/workflows/squad-workflows.manifest.json @@ -144,7 +144,7 @@ "package_destination": ".github/workflows/shared/squad-activation.mjs", "destination": ".github/workflows/shared/squad-activation.mjs", "ownership": "manifest", - "sha256": "16c579cc85ff191614a9635757ee5281f4f9f3f1f0f8ead44fe33c44b90a0980" + "sha256": "1d6cc4f5a924d471e83b4bd0cfb754fe8e5376efea339d2a303138350c3bc4f8" }, { "path": "shared/check-agent-binding.mjs",