diff --git a/.changeset/prevent-command-replay.md b/.changeset/prevent-command-replay.md new file mode 100644 index 000000000..116df88b0 --- /dev/null +++ b/.changeset/prevent-command-replay.md @@ -0,0 +1,6 @@ +--- +"@bradygaster/squad-cli": patch +"@bradygaster/squad-sdk": patch +--- + +Prevent edited issue and comment events from replaying unchanged embedded `/squad` commands, require dual-principal repository authorization for `/squad revoke-improvement`, and ignore unauthorized revocation comments during live approval revalidation. diff --git a/docs/src/content/docs/guide/gh-aw.md b/docs/src/content/docs/guide/gh-aw.md index 71ca1f445..27868e618 100644 --- a/docs/src/content/docs/guide/gh-aw.md +++ b/docs/src/content/docs/guide/gh-aw.md @@ -657,7 +657,7 @@ wins: `/squad plan accept scope` is not treated as `/squad plan`. | Review | `/squad review` | Show how to rerun the current pull request's automatic independent review | Does not dispatch branch-selected code; human approval remains mandatory | | Retrospective | `/squad retro` | Run the shared retrospective immediately | Authorized manual run; weekly and evidence-driven wakeups use the same durable gate | | Governance | `/squad approve-improvement` | Request implementation of an exact retrospective proposal revision | Human write/maintain/admin permission, `Approved-Revision:` hash and exact `Approved-Path:` lines; dispatcher relays nested `issue_number` and `approval_comment_id`, never approval authority | -| Governance | `/squad revoke-improvement` | Withdraw a prior `/squad approve-improvement` | Reserved, read-only command available to any actor; emits no output of any kind — the comment itself is the record that later runs re-check | +| Governance | `/squad revoke-improvement` | Withdraw a prior `/squad approve-improvement` | Reserved state-changing command requiring write, maintain, or admin authorization; emits no output of any kind — the comment itself is the record that later runs re-check | ### Implementation provenance @@ -875,9 +875,11 @@ and validates its human author/provenance, permissions, issue state, revision an scope, both before work and before safe outputs; it never substitutes a newer approval or treats a relay actor as an approver. -`/squad revoke-improvement` is a reserved no-dispatch route: a later human comment -withdraws the referenced approval. Missing/edited/stale/revoked approvals require -a fresh comment for the current content. Manual retries select +`/squad revoke-improvement` is a reserved no-dispatch route: only a later +unedited, non-app human issue comment from an author with live write, maintain, +or admin permission withdraws the referenced approval. Unresolved permission, +malformed timestamps, incomplete history, or missing/edited/stale/revoked +approvals fail closed and require a fresh comment for the current content. Manual retries select **Actions → Squad Improvement Worker → Run workflow**, on the default branch, and supply the same `issue_number` and `approval_comment_id` (the numeric suffix of its `#issuecomment-N` URL). Retrying is not approval. Both workers remain @@ -889,7 +891,7 @@ Opt-in reconciliation adds at most 48: three action pages, five all-state PR pages and two comment pages for each of twenty candidates. Candidates rotate every six hours; an incomplete history requires human inspection. Live retro output checks add at most 14 reads; receiving ordinary-worker checks at most -nine. Improvement authorization is bounded to fourteen reads per check, twice +nine. Improvement authorization is bounded to 36 reads per check, twice per worker run, plus two dispatcher checks. GitHub supplies no transaction covering a comment and a PR: revocation is checked immediately before safe outputs, not after publication; human review remains the final authority. diff --git a/test/gh-aw-command-parse.test.ts b/test/gh-aw-command-parse.test.ts index 5c9b7653a..0ec8273df 100644 --- a/test/gh-aw-command-parse.test.ts +++ b/test/gh-aw-command-parse.test.ts @@ -5,6 +5,7 @@ import { join } from 'node:path'; import { classifySquadCommand, commandRequiresAuthorization, + editedCommandShouldRoute, enforceSquadCommandContract, isAuthorizedPermission, rejectionComment, @@ -115,6 +116,7 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { ['\n\n/squad status\n', 'status', null], ['activate phase 3', 'activate', 3], ['research Focus only on P1', 'research', null], + ['revoke-improvement', 'revoke-improvement', null], ])('accepts bare or slash-prefixed dispatch %j', (command, mode, phase) => { expect(classifySquadCommand(dispatch(command), 'workflow_dispatch')).toMatchObject({ status: 'accepted', @@ -124,6 +126,104 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { }); }); + describe.each([ + ['issues', issue], + ['issue_comment', comment], + ] as const)('edited-event replay gate for %s', (eventName, payload) => { + function edited(previousBody: unknown, currentBody: string) { + return { + ...payload(currentBody), + action: 'edited', + changes: { body: { from: previousBody } }, + }; + } + + it.each([ + [ + 'Context before.\n/squad plan implementation\nContext after.', + 'Changed context before.\n\n /squad plan implementation \nChanged context after.', + ], + [ + 'Context before.\n/squad status extra\nContext after.', + 'Changed context before.\n\n /squad status extra \nChanged context after.', + ], + ])('noops when only surrounding prose or command whitespace changes', (before, after) => { + const event = edited(before, after); + const current = classifySquadCommand(event, eventName); + expect(editedCommandShouldRoute(event, eventName, current)).toBe(false); + }); + + it.each([ + ['/squad DANCE', '/squad dance'], + ['/squad STATUS EXTRA', '/squad status extra'], + ])('noops a rejected invocation case-only edit while preserving its current text: %j -> %j', (before, after) => { + const event = edited(before, after); + const current = classifySquadCommand(event, eventName); + expect(current).toMatchObject({ status: 'rejected', rejectedCommand: after }); + expect(editedCommandShouldRoute(event, eventName, current)).toBe(false); + expect(rejectionComment(current as Extract)) + .toContain(`\`\`\`text\n${after}\n\`\`\``); + }); + + it.each([ + ['ordinary body', '/squad status'], + ['/squad status', '/squad plan'], + ['/squad status extra', '/squad dance'], + ['Use `/squad status` after review.', '/squad status'], + ['```text\n/squad status\n```', '/squad status'], + ])('routes a newly introduced or materially changed command: %j -> %j', (before, after) => { + const event = edited(before, after); + const current = classifySquadCommand(event, eventName); + expect(editedCommandShouldRoute(event, eventName, current)).toBe(true); + }); + + it('rejects issue-body and edited-comment revocations but accepts a new comment revocation', () => { + expect(classifySquadCommand(issue('/squad revoke-improvement'), 'issues')).toMatchObject({ + status: 'rejected', + source: 'issue', + reason: 'Issue-body revocations are not durable. Post /squad revoke-improvement as a new, unedited human issue comment.', + }); + expect(classifySquadCommand(comment('/squad revoke-improvement'), 'issue_comment')).toMatchObject({ + status: 'accepted', + source: 'comment', + mode: 'revoke-improvement', + }); + expect(classifySquadCommand({ + ...comment('/squad revoke-improvement'), + action: 'edited', + }, 'issue_comment')).toMatchObject({ + status: 'rejected', + source: 'comment', + reason: 'Edited comment revocations are not durable. Post /squad revoke-improvement as a new, unedited human issue comment.', + }); + }); + + if (eventName === 'issue_comment') { + it('routes an unchanged edited revocation from its accepted pre-edit state to rejection', () => { + const event = edited('/squad revoke-improvement', '/squad revoke-improvement'); + const current = classifySquadCommand(event, eventName); + expect(current).toMatchObject({ + status: 'rejected', + reason: 'Edited comment revocations are not durable. Post /squad revoke-improvement as a new, unedited human issue comment.', + }); + expect(editedCommandShouldRoute(event, eventName, current)).toBe(true); + }); + } + + it.each([ + undefined, + null, + 42, + { unexpected: 'shape' }, + ])('fails closed when previous-body evidence is unavailable or malformed: %j', previousBody => { + const event = previousBody === undefined + ? { ...payload('/squad status'), action: 'edited' } + : edited(previousBody, '/squad status'); + const current = classifySquadCommand(event, eventName); + expect(editedCommandShouldRoute(event, eventName, current)).toBe(false); + }); + }); + it.each(['', ' ', '\n\n'])('keeps empty activation probe non-eventful: %j', command => { expect(classifySquadCommand(dispatch(command), 'workflow_dispatch')).toEqual({ status: 'none', @@ -162,6 +262,23 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { expect(posted[0].body).toContain('The workflow stopped without running a Squad mode.'); }); + it('fails the shared main-workflow contract for an issue-body revocation', async () => { + const posted: Array<{ issueNumber: number; body: string }> = []; + await expect( + enforceSquadCommandContract({ + payload: issue('/squad revoke-improvement'), + eventName: 'issues', + createComment: async (issueNumber, body) => { + posted.push({ issueNumber, body }); + }, + }), + ).rejects.toThrow('Squad rejected command: /squad revoke-improvement'); + + expect(posted).toHaveLength(1); + expect(posted[0]).toMatchObject({ issueNumber: 1824 }); + expect(posted[0].body).toContain('Issue-body revocations are not durable.'); + }); + it('does not comment or throw for valid commands and no-invocation non-events', async () => { const posted: string[] = []; const createComment = async (_issueNumber: number, body: string) => { @@ -233,10 +350,10 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { }); it('keeps open modes public and fails mutating modes closed on collaborator permission', () => { - for (const mode of ['status', 'review', 'research', 'plan', 'revoke-improvement']) { + for (const mode of ['status', 'review', 'research', 'plan']) { expect(commandRequiresAuthorization({ status: 'accepted', mode })).toBe(false); } - for (const mode of ['cast', 'triage', 'plan implementation', 'implement']) { + for (const mode of ['cast', 'triage', 'plan implementation', 'implement', 'revoke-improvement']) { expect(commandRequiresAuthorization({ status: 'accepted', mode })).toBe(true); } expect(isAuthorizedPermission('admin')).toBe(true); @@ -272,6 +389,44 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { } }); + it('realistic mutations expose issue-body revocation dispatch and rejected case-only replay', async () => { + mkdirSync(TEST_ROOT, { recursive: true }); + const root = mkdtempSync(join(TEST_ROOT, 'command-revoke-replay-mutants-')); + try { + const source = join(process.cwd(), 'workflows', 'shared', 'squad-command-contract.mjs'); + const original = readFileSync(source, 'utf8'); + + const revokeMutant = join(root, 'squad-command-contract-revoke-mutant.mjs'); + const withoutIssueGuard = original.replace( + " if (sourceData.source === 'issue' && classification.mode === 'revoke-improvement') {", + " if (false && sourceData.source === 'issue' && classification.mode === 'revoke-improvement') {", + ); + expect(withoutIssueGuard).not.toBe(original); + writeFileSync(revokeMutant, withoutIssueGuard); + const revokeModule = await import(`${pathToFileURL(revokeMutant).href}?mutation=${Date.now()}`); + expect(revokeModule.classifySquadCommand(issue('/squad revoke-improvement'), 'issues')) + .toMatchObject({ status: 'accepted', mode: 'revoke-improvement' }); + + const replayMutant = join(root, 'squad-command-contract-replay-mutant.mjs'); + const withoutRejectedCaseNormalization = original.replace( + "result.rejectedCommand.replace(/\\s+/g, ' ').trim().toLowerCase(),", + "result.rejectedCommand.replace(/\\s+/g, ' ').trim(),", + ); + expect(withoutRejectedCaseNormalization).not.toBe(original); + writeFileSync(replayMutant, withoutRejectedCaseNormalization); + const replayModule = await import(`${pathToFileURL(replayMutant).href}?mutation=${Date.now() + 1}`); + const editedEvent = { + ...comment('/squad status extra'), + action: 'edited', + changes: { body: { from: '/squad STATUS EXTRA' } }, + }; + const current = replayModule.classifySquadCommand(editedEvent, 'issue_comment'); + expect(replayModule.editedCommandShouldRoute(editedEvent, 'issue_comment', current)).toBe(true); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + it('realistic fence-length mutation exposes a hidden command and is caught', async () => { mkdirSync(TEST_ROOT, { recursive: true }); const root = mkdtempSync(join(TEST_ROOT, 'command-fence-mutant-')); diff --git a/test/gh-aw-command-router-relay.test.ts b/test/gh-aw-command-router-relay.test.ts index ae8744037..103a6b11f 100644 --- a/test/gh-aw-command-router-relay.test.ts +++ b/test/gh-aw-command-router-relay.test.ts @@ -100,6 +100,7 @@ async function runRouter({ authorType, senderType, body, + previousBody, permissions, scriptMutation, }: { @@ -110,6 +111,7 @@ async function runRouter({ authorType?: string; senderType?: string; body: string; + previousBody?: string | null | Record; permissions: Record; scriptMutation?: (script: string) => string; }): Promise { @@ -132,6 +134,9 @@ async function runRouter({ action, issue: textSource, ...(comment ? { comment } : {}), + ...(action === 'edited' && previousBody !== undefined + ? { changes: { body: { from: previousBody } } } + : {}), repository: { default_branch: 'dev' }, sender: { login: actor, type: senderType ?? 'User' }, }, @@ -208,6 +213,7 @@ describe('gh-aw: router-to-repair workflow_dispatch relay (#2, #3 findings)', () expect(ROUTER).toContain('getCollaboratorPermissionLevel'); assertAuthorBinding(ROUTER); expect(commandRequiresAuthorization({ status: 'accepted', mode: 'cast' })).toBe(true); + expect(commandRequiresAuthorization({ status: 'accepted', mode: 'revoke-improvement' })).toBe(true); expect(commandRequiresAuthorization({ status: 'accepted', mode: 'status' })).toBe(false); expect(isAuthorizedPermission('read')).toBe(false); expect(isAuthorizedPermission('write')).toBe(true); @@ -391,6 +397,7 @@ describe('gh-aw: mutating router authorization is bound to text provenance', () actor: 'maintainer-editor', author: 'unprivileged-author', body: 'Please run this request.\n/squad cast', + ...(action === 'edited' ? { previousBody: 'Please run this request.' } : {}), permissions: { 'maintainer-editor': 'write', 'unprivileged-author': 'read', @@ -414,6 +421,7 @@ describe('gh-aw: mutating router authorization is bound to text provenance', () actor: 'maintainer-editor', author: 'maintainer-author', body: 'Please run this request.\n/squad cast', + previousBody: 'Please run this request.', permissions: { 'maintainer-editor': 'maintain', 'maintainer-author': 'write', @@ -432,6 +440,7 @@ describe('gh-aw: mutating router authorization is bound to text provenance', () actor: 'unprivileged-editor', author: 'maintainer-author', body: 'Please run this request.\n/squad cast', + previousBody: 'Please run this request.', permissions: { 'unprivileged-editor': 'read', 'maintainer-author': 'admin', @@ -469,6 +478,7 @@ describe('gh-aw: mutating router authorization is bound to text provenance', () action: 'edited', actor: 'reader', body: 'Please report current state.\n/squad status', + previousBody: 'Please report current state.', permissions: {}, }); @@ -495,6 +505,288 @@ describe('gh-aw: mutating router authorization is bound to text provenance', () expect(result.dispatchedInputs).toMatchObject({ command: 'cast', issue_number: '4242' }); }); + it.each([ + ['issues', 'Please update the title.\n/squad status', 'Updated title context.\n\n /squad status '], + ['issue_comment', 'Before.\n/squad status extra', 'After.\n\n /squad status extra '], + ] as const)('noops an unrelated %s edit when the accepted or rejected invocation is unchanged', async ( + eventName, + previousBody, + body, + ) => { + const result = await runRouter({ + eventName, + action: 'edited', + actor: 'editor', + author: 'author', + previousBody, + body, + permissions: { editor: 'write', author: 'write' }, + }); + + expect(result.dispatchedInputs).toBeNull(); + expect(result.failure).toBeNull(); + expect(result.postedComments).toEqual([]); + expect(result.permissionLookups).toEqual([]); + }); + + it.each(['issues', 'issue_comment'] as const)( + 'noops a %s.edited change that only recases parsed command keywords', + async eventName => { + const result = await runRouter({ + eventName, + action: 'edited', + actor: 'maintainer-editor', + author: 'maintainer-author', + previousBody: 'Before.\n/squad PLAN REVISE Keep APIName casing', + body: 'After.\n/squad plan revise Keep APIName casing', + permissions: { + 'maintainer-editor': 'maintain', + 'maintainer-author': 'write', + }, + }); + + expect(result.dispatchedInputs).toBeNull(); + expect(result.failure).toBeNull(); + expect(result.postedComments).toEqual([]); + expect(result.permissionLookups).toEqual([]); + }, + ); + + it.each(['issues', 'issue_comment'] as const)( + 'noops a rejected %s.edited invocation that only changes command casing', + async eventName => { + const result = await runRouter({ + eventName, + action: 'edited', + actor: 'maintainer-editor', + author: 'maintainer-author', + previousBody: 'Before.\n/squad STATUS EXTRA', + body: 'After.\n/squad status extra', + permissions: { + 'maintainer-editor': 'maintain', + 'maintainer-author': 'write', + }, + }); + + expect(result.dispatchedInputs).toBeNull(); + expect(result.failure).toBeNull(); + expect(result.postedComments).toEqual([]); + expect(result.permissionLookups).toEqual([]); + }, + ); + + it.each(['issues', 'issue_comment'] as const)( + 'routes a %s.edited change to meaningful free-form argument case', + async eventName => { + const result = await runRouter({ + eventName, + action: 'edited', + actor: 'maintainer-editor', + author: 'maintainer-author', + previousBody: 'Before.\n/squad PLAN REVISE Keep APIName casing', + body: 'After.\n/squad plan revise Keep ApiName casing', + permissions: { + 'maintainer-editor': 'maintain', + 'maintainer-author': 'write', + }, + }); + + expect(result.failure).toBeNull(); + expect(result.permissionLookups).toEqual(['maintainer-editor', 'maintainer-author']); + expect(result.dispatchedInputs).toMatchObject({ + command: 'plan revise Keep ApiName casing', + issue_number: '4242', + }); + }, + ); + + it.each([ + ['issue_comment', 'Use `/squad revoke-improvement` only after approval.'], + ['issue_comment', '```text\n/squad revoke-improvement\n```'], + ] as const)('rejects an edited revoke command from %s after code contexts are excluded', async ( + eventName, + previousBody, + ) => { + const result = await runRouter({ + eventName, + action: 'edited', + actor: 'maintainer-editor', + author: 'maintainer-author', + previousBody, + body: 'Approval changed.\n/squad revoke-improvement', + permissions: { + 'maintainer-editor': 'maintain', + 'maintainer-author': 'write', + }, + }); + + expect(result.dispatchedInputs).toBeNull(); + expect(result.permissionLookups).toEqual([]); + expect(result.failure).toBe('Squad rejected command: /squad revoke-improvement'); + expect(result.postedComments).toHaveLength(1); + expect(result.postedComments[0]).toContain('Edited comment revocations are not durable.'); + }); + + it('rejects an edited existing revocation even when its canonical command is unchanged', async () => { + const result = await runRouter({ + eventName: 'issue_comment', + action: 'edited', + actor: 'maintainer-editor', + author: 'maintainer-author', + previousBody: '/squad revoke-improvement', + body: 'Context changed.\n/squad revoke-improvement', + permissions: { + 'maintainer-editor': 'maintain', + 'maintainer-author': 'write', + }, + }); + + expect(result.dispatchedInputs).toBeNull(); + expect(result.permissionLookups).toEqual([]); + expect(result.failure).toBe('Squad rejected command: /squad revoke-improvement'); + expect(result.postedComments[0]).toContain('Edited comment revocations are not durable.'); + }); + + it('rejects an issue-body revoke-improvement without permission lookup or dispatch', async () => { + const result = await runRouter({ + eventName: 'issues', + action: 'edited', + actor: 'maintainer-editor', + author: 'maintainer-author', + previousBody: 'Approval state changed.', + body: 'Approval state changed.\n/squad revoke-improvement', + permissions: { + 'maintainer-editor': 'maintain', + 'maintainer-author': 'admin', + }, + }); + + expect(result.dispatchedInputs).toBeNull(); + expect(result.permissionLookups).toEqual([]); + expect(result.failure).toBe('Squad rejected command: /squad revoke-improvement'); + expect(result.postedComments).toHaveLength(1); + expect(result.postedComments[0]).toContain('Issue-body revocations are not durable.'); + expect(result.postedComments[0]).toContain('```text\n/squad revoke-improvement\n```'); + }); + + it.each(['issues', 'issue_comment'] as const)( + 'fails closed for %s.edited when changes.body.from is absent or malformed', + async eventName => { + for (const previousBody of [undefined, null, { invalid: true }]) { + const result = await runRouter({ + eventName, + action: 'edited', + actor: 'maintainer', + author: 'maintainer', + ...(previousBody === undefined ? {} : { previousBody }), + body: '/squad status', + permissions: { maintainer: 'write' }, + }); + expect(result.dispatchedInputs).toBeNull(); + expect(result.failure).toBeNull(); + expect(result.postedComments).toEqual([]); + expect(result.permissionLookups).toEqual([]); + } + }, + ); + + it.each([ + ['status', 'read', 'read'], + ['review', 'read', 'read'], + ['research focused scope', 'none', 'read'], + ['plan', 'read', 'none'], + ])('keeps public read-only mode %j open without permission lookups', async ( + command, + actorPermission, + authorPermission, + ) => { + const result = await runRouter({ + eventName: 'issue_comment', + action: 'created', + actor: 'reader', + author: 'external-author', + body: `/squad ${command}`, + permissions: { + reader: actorPermission, + 'external-author': authorPermission, + }, + }); + + expect(result.failure).toBeNull(); + expect(result.permissionLookups).toEqual([]); + expect(result.dispatchedInputs).not.toBeNull(); + }); + + it.each([ + ['read', 'write', 'event actor reader=read'], + ['write', 'read', 'command author external-author=read'], + ['unresolved', 'admin', 'event actor reader=unresolved'], + ['maintain', 'unresolved', 'command author external-author=unresolved'], + ])('requires both principals for revoke-improvement: actor=%s author=%s', async ( + actorPermission, + authorPermission, + failure, + ) => { + const result = await runRouter({ + eventName: 'issue_comment', + action: 'created', + actor: 'reader', + author: 'external-author', + body: '/squad revoke-improvement', + permissions: { + reader: actorPermission, + 'external-author': authorPermission, + }, + }); + + expect(result.dispatchedInputs).toBeNull(); + expect(result.failure).toContain(failure); + expect(result.permissionLookups).toEqual(['reader', 'external-author']); + }); + + it('allows comment revoke-improvement only when both the event actor and original author are authorized', async () => { + const result = await runRouter({ + eventName: 'issue_comment', + action: 'created', + actor: 'maintainer', + author: 'maintainer', + body: '/squad revoke-improvement', + permissions: { + maintainer: 'maintain', + }, + }); + + expect(result.failure).toBeNull(); + expect(result.permissionLookups).toEqual(['maintainer']); + expect(result.dispatchedInputs).toMatchObject({ command: 'revoke-improvement' }); + }); + + it('kills the replay-gate mutation that redispatches an unchanged edited command', async () => { + const gate = [ + 'if (!contract.editedCommandShouldRoute(', + ' context.payload,', + ' process.env.SQUAD_EVENT_NAME,', + ' result,', + ')) {', + " core.info('Ignoring an edited body whose canonical /squad invocation did not change or whose previous body is unavailable.');", + ' return;', + '}', + ].join('\n'); + expect(extractScript(ROUTER, 'Route or reject discovered command')).toContain(gate); + + const vulnerable = await runRouter({ + eventName: 'issue_comment', + action: 'edited', + actor: 'maintainer', + author: 'maintainer', + previousBody: 'Before.\n/squad status', + body: 'After.\n/squad status', + permissions: { maintainer: 'write' }, + scriptMutation: script => script.replace(gate, ''), + }); + expect(vulnerable.dispatchedInputs).toMatchObject({ command: 'status' }); + }); + it('kills the author-binding mutation that substitutes the editor for the command author', async () => { expect(() => assertAuthorBinding( ROUTER.replace( @@ -513,6 +805,7 @@ describe('gh-aw: mutating router authorization is bound to text provenance', () actor: 'maintainer-editor', author: 'unprivileged-author', body: 'Please run this request.\n/squad cast', + previousBody: 'Please run this request.', permissions: { 'maintainer-editor': 'write', 'unprivileged-author': 'read', @@ -531,6 +824,7 @@ describe('gh-aw: mutating router authorization is bound to text provenance', () it('preserves author binding in the generated package source and strict-compiled artifact', () => { assertAuthorBinding(PACKAGE_ROUTER); + expect(PACKAGE_ROUTER).toContain('contract.editedCommandShouldRoute('); const workspace = mkdtempSync(join(tmpdir(), 'squad-command-router-compiled-')); try { const workflowDir = join(workspace, '.github', 'workflows'); @@ -548,6 +842,7 @@ describe('gh-aw: mutating router authorization is bound to text provenance', () 'utf8', ); assertAuthorBinding(compiled); + expect(compiled).toContain('contract.editedCommandShouldRoute('); expect(compiled).toContain('context.payload.comment?.user?.login'); expect(compiled).toContain('context.payload.issue?.user?.login'); } finally { @@ -576,6 +871,7 @@ describe('gh-aw: bot-authored /squad command text is never replayed as a trusted authorType: 'Bot', senderType: 'Bot', body: 'Please run this request.\n/squad cast', + ...(action === 'edited' ? { previousBody: 'Please run this request.' } : {}), permissions: {}, }); @@ -597,6 +893,7 @@ describe('gh-aw: bot-authored /squad command text is never replayed as a trusted authorType: 'User', senderType: 'Bot', body: 'Please run this request.\n/squad cast', + previousBody: 'Please run this request.', permissions: { 'human-author': 'write' }, }); @@ -637,6 +934,7 @@ describe('gh-aw: bot-authored /squad command text is never replayed as a trusted authorType: 'Bot', senderType: 'Bot', body: 'Please run this request.\n/squad cast', + previousBody: 'Please run this request.', permissions: { 'relay-bot': 'write' }, scriptMutation: script => { expect(script).toContain(dedentedMarker); @@ -658,6 +956,7 @@ describe('gh-aw: bot-authored /squad command text is never replayed as a trusted authorType: 'User', senderType: 'Bot', body: 'Please run this request.\n/squad cast', + previousBody: 'Please run this request.', permissions: { 'human-author': 'write', 'editing-bot': 'write' }, scriptMutation: script => { expect(script).toContain(marker); diff --git a/test/gh-aw-improvement-worker.test.ts b/test/gh-aw-improvement-worker.test.ts index 00d19c5e4..a94c8f220 100644 --- a/test/gh-aw-improvement-worker.test.ts +++ b/test/gh-aw-improvement-worker.test.ts @@ -50,6 +50,7 @@ const env = (root = ROOT) => ({ }); function api(options: { proposal?: any; approved?: any; comments?: any[]; permission?: any; revision?: any; pulls?: any[]; + permissions?: Record; commentFailure?: boolean; pullFailure?: boolean; native?: any; nativeFailure?: boolean; } = {}) { const proposal = options.proposal || issue(); @@ -62,7 +63,10 @@ function api(options: { if (route === `repos/${REPO}/issues/77`) return proposal; if (route === `repos/${REPO}/issues/comments/901`) return approved; if (route.endsWith('/comments')) return options.commentFailure ? { __status: 503 } : fields.page === 1 ? options.comments || [approved] : []; - if (route.endsWith('/permission')) return options.permission || { permission: 'write' }; + if (route.endsWith('/permission')) { + const login = decodeURIComponent(route.split('/').at(-2) || ''); + return options.permissions?.[login] || options.permission || { permission: 'write' }; + } if (route.endsWith('/pulls')) return options.pullFailure ? { __status: 503 } : fields.page === 1 ? options.pulls || [] : []; throw new Error(`Unexpected route: ${route}`); }, @@ -216,10 +220,97 @@ describe('improvement: live revalidation and permanent deduplication', () => { ])('fails closed on %s', async (_name, options) => { expect((await gate.collectImprovementContext(env(), api(options))).authorized).toBe(false); }); - it('honors later revocation even with a newer unrelated approval, but not bot-generated revocation', async () => { + it('honors only an authorized, unedited human revocation after the approval', async () => { const revoke = comment({ id: 902, body: '/squad revoke-improvement', created_at: '2026-09-03T00:00:00Z', updated_at: '2026-09-03T00:00:00Z' }); - expect((await gate.collectImprovementContext(env(), api({ comments: [comment(), revoke, comment({ id: 903 })] }))).reason).toBe('approval-revoked'); + const authorized = api({ comments: [comment(), revoke, comment({ id: 903 })] }); + expect((await gate.collectImprovementContext(env(), authorized)).reason).toBe('approval-revoked'); + expect((await gate.collectImprovementContext(env(), api({ + comments: [comment(), { ...revoke, body: 'Context.\n/squad REVOKE-IMPROVEMENT ' }], + }))).reason).toBe('approval-revoked'); + expect((await gate.collectImprovementContext(env(), api({ + comments: [comment(), { ...revoke, body: '```text\n/squad revoke-improvement\n```' }], + }))).reason).toBe('approved'); + expect(authorized.calls).toContain(`repos/${REPO}/collaborators/maintainer/permission`); expect((await gate.collectImprovementContext(env(), api({ comments: [{ ...revoke, user: { login: 'bot', type: 'Bot' } }] }))).authorized).toBe(true); + expect((await gate.collectImprovementContext(env(), api({ + comments: [{ ...revoke, updated_at: '2026-09-04T00:00:00Z' }], + }))).authorized).toBe(true); + expect((await gate.collectImprovementContext(env(), api({ + comments: [{ ...revoke, performed_via_github_app: { id: 1 } }], + }))).authorized).toBe(true); + expect((await gate.collectImprovementContext(env(), api({ + comments: [{ ...revoke, created_at: '2026-09-01T00:00:00Z', updated_at: '2026-09-01T00:00:00Z' }], + }))).authorized).toBe(true); + expect(await gate.collectImprovementContext(env(), api({ + comments: [{ ...revoke, created_at: 'not-a-timestamp', updated_at: 'not-a-timestamp' }], + }))).toMatchObject({ authorized: false, reason: 'revocation-history-incomplete' }); + }); + it('uses complete comment order to resolve equal-timestamp revocation boundaries', async () => { + const revoke = comment({ + id: 902, + body: '/squad revoke-improvement', + created_at: AT, + updated_at: AT, + }); + expect(await gate.collectImprovementContext(env(), api({ + comments: [revoke, comment()], + }))).toMatchObject({ authorized: true, reason: 'approved' }); + expect(await gate.collectImprovementContext(env(), api({ + comments: [comment(), revoke], + }))).toMatchObject({ authorized: false, reason: 'approval-revoked' }); + }); + it('uses numeric comment IDs for equal timestamps when the approval is absent from the complete list', async () => { + const before = comment({ + id: 900, + body: '/squad revoke-improvement', + created_at: AT, + updated_at: AT, + }); + const after = comment({ + id: 902, + body: '/squad revoke-improvement', + created_at: AT, + updated_at: AT, + }); + expect(await gate.collectImprovementContext(env(), api({ + comments: [before], + }))).toMatchObject({ authorized: true, reason: 'approved' }); + expect(await gate.collectImprovementContext(env(), api({ + comments: [after], + }))).toMatchObject({ authorized: false, reason: 'approval-revoked' }); + }); + it('ignores an unprivileged revocation and fails closed when revocation permission is unresolved', async () => { + const revoke = comment({ + id: 902, + user: { login: 'reader', type: 'User' }, + body: '/squad revoke-improvement', + created_at: '2026-09-03T00:00:00Z', + updated_at: '2026-09-03T00:00:00Z', + }); + expect(await gate.collectImprovementContext(env(), api({ + comments: [comment(), revoke], + permissions: { reader: { permission: 'read' }, maintainer: { permission: 'write' } }, + }))).toMatchObject({ authorized: true, reason: 'approved' }); + expect(await gate.collectImprovementContext(env(), api({ + comments: [comment(), revoke], + permissions: { reader: { __status: 403 }, maintainer: { permission: 'write' } }, + }))).toMatchObject({ authorized: false, reason: 'revocation-permission-unresolved' }); + expect(await gate.collectImprovementContext(env(), api({ + comments: [comment(), revoke], + permissions: { reader: {}, maintainer: { permission: 'write' } }, + }))).toMatchObject({ authorized: false, reason: 'revocation-permission-unresolved' }); + expect(await gate.collectImprovementContext(env(), api({ + comments: [comment(), revoke], + permissions: { reader: { permission: 'unexpected' }, maintainer: { permission: 'write' } }, + }))).toMatchObject({ authorized: false, reason: 'revocation-permission-unresolved' }); + const failed = api({ comments: [comment(), revoke] }); + const fetchJson = failed.fetchJson; + failed.fetchJson = async (route, fields) => { + if (route === `repos/${REPO}/collaborators/reader/permission`) throw new Error('network failure'); + return fetchJson(route, fields); + }; + expect(await gate.collectImprovementContext(env(), failed)) + .toMatchObject({ authorized: false, reason: 'revocation-permission-unresolved' }); }); it.each([ ['open', null], ['closed', '2026-09-03T00:00:00Z'], ['closed', null], @@ -468,8 +559,8 @@ describe('improvement: final safe-output enforcement', () => { { body: prItem().body.replace('Scope-Digest:', 'Wrong-Digest:') }, ])('rejects a mutated output: %o', async mutation => { expect((await enforce([prItem(mutation)])).ok).toBe(false); }); it('checks live revocation again after implementation', async () => { - const revoked = comment({ body: '/squad revoke-improvement', created_at: AT, updated_at: AT }); - expect((await enforce([prItem()], patchFor(), { comments: [revoked] })).ok).toBe(false); + const revoked = comment({ id: 902, body: '/squad revoke-improvement', created_at: AT, updated_at: AT }); + expect((await enforce([prItem()], patchFor(), { comments: [comment(), revoked] })).ok).toBe(false); }); it('refuses unsupported bundle transport rather than inspecting a decoy am patch', async () => { const root = scratch(); @@ -588,6 +679,13 @@ describe('improvement: one authorized dispatcher route and installed contract', expect(job).toContain('ref: refs/heads/${{ github.event.repository.default_branch }}'); expect(job).toContain('SQUAD_IMPROVE_APPROVAL_COMMENT_ID'); }, 90000); + + it('documents the same live revocation trust boundary enforced by the gate', () => { + expect(WORKER).toContain('Only a later unedited, non-app'); + expect(WORKER).toContain('live write, maintain, or admin repository permission'); + expect(WORKER).toContain('Revocation permission or history that cannot be resolved'); + expect(WORKER).toContain('fails closed'); + }); }); // Finding #3 (octodemo/zava-social-backend-20261001184422#1): the router @@ -747,4 +845,3 @@ describe('improvement: second-hop relay context survives gh-aw engine aw_context expect(lock).toContain('"squad_approval_relay"'); }, 120000); }); - diff --git a/test/gh-aw-quality.test.ts b/test/gh-aw-quality.test.ts index f47a7cdc7..23da2836a 100644 --- a/test/gh-aw-quality.test.ts +++ b/test/gh-aw-quality.test.ts @@ -3981,8 +3981,8 @@ describe('gh-aw: canonical package integrity contract', () => { expect(mutable).not.toContain(actionReference); expect(createHash('sha256').update(normalizeCompiledLock(mutable, revisionA)).digest('hex')) .toBe(sourceBinding === 'workflow' - ? '9bec39a94b57b8d6c2baf92b25bf2817ff7bf389533d6462d71a1937b2154cbe' - : '00bb10abb90600fd03411e6036ab67741b208da1c9698f8eb1f5faea142583a9'); + ? 'c86cd65d09ee74d8da4dfec27f64ccba818d22f282c5a4acf33f04e04f5d6bd6' + : '206fcc936e5c03e81928565104bc7b2fef9c492f9e8152b17df563bd07e5065a'); expect(() => validateCompilerActionPins(mutable)).toThrow(/invalid immutable action pin/); writeFileSync(lockPath, mutable); expect(verifyInstall(root).failures.join('\n')) @@ -4036,7 +4036,7 @@ describe('gh-aw: canonical package integrity contract', () => { compile(); const unpinned = readText(lockPath); expect(createHash('sha256').update(normalizeCompiledLock(unpinned, revisionA)).digest('hex')) - .toBe('00bb10abb90600fd03411e6036ab67741b208da1c9698f8eb1f5faea142583a9'); + .toBe('206fcc936e5c03e81928565104bc7b2fef9c492f9e8152b17df563bd07e5065a'); expect(verifyInstall(root).failures.join('\n')).toContain('Installed digest mismatch'); const seedPins = () => spawnSync(process.execPath, ['--input-type=module', '-e', seed!], { diff --git a/test/gh-aw-retro-workflow.test.ts b/test/gh-aw-retro-workflow.test.ts index c3ab8608c..ba00df2c5 100644 --- a/test/gh-aw-retro-workflow.test.ts +++ b/test/gh-aw-retro-workflow.test.ts @@ -1642,6 +1642,10 @@ describe('Squad retrospective workflow integration', () => { expect(RETRO).toContain('/squad approve-improvement'); expect(RETRO).toContain('Approved-Path: {first proposed path}'); expect(RETRO).toContain('/squad revoke-improvement'); + expect(RETRO).toContain('Only a later unedited, non-app human issue comment'); + expect(RETRO).toContain('live repository permission is'); + expect(RETRO).toContain('Unresolved revocation permission or'); + expect(RETRO).toContain('comment-history evidence fails closed'); expect(RETRO).toContain('Never post that command yourself in a comment'); }); diff --git a/workflows/package/squad-command-router.md b/workflows/package/squad-command-router.md index 384f050cb..abab7e901 100644 --- a/workflows/package/squad-command-router.md +++ b/workflows/package/squad-command-router.md @@ -59,10 +59,18 @@ safe-outputs: context.payload, process.env.SQUAD_EVENT_NAME, ); + if (!contract.editedCommandShouldRoute( + context.payload, + process.env.SQUAD_EVENT_NAME, + result, + )) { + core.info('Ignoring an edited body whose canonical /squad invocation did not change or whose previous body is unavailable.'); + return; + } if (result.status === 'accepted' || result.status === 'rejected') { // A bot-authored issue or comment body is never a trusted Squad // command, open mode or not: open modes (`status`, `review`, - // `research`, `plan`, `revoke-improvement`) intentionally skip + // `research`, `plan`) intentionally skip // the permission check below, so without this guard any bot that // reposts or quotes `/squad` text (for example a relay, mirror, // or notification bot) could replay it into a real dispatch with diff --git a/workflows/package/squad-improvement-worker.md b/workflows/package/squad-improvement-worker.md index 138e1599c..a813cd0f8 100644 --- a/workflows/package/squad-improvement-worker.md +++ b/workflows/package/squad-improvement-worker.md @@ -140,10 +140,13 @@ Approved-Path: {exact proposed file} Compute the digest using the installed gate's `--revision` command as documented in the gh-aw guide. The issue's exact `Proposed-Path:` set must equal the approved -set. An edited approval comment is invalid. Any later human -`/squad revoke-improvement` withdraws it until a new approval is posted. -Body edits, scope changes, missing revision/permission data and incomplete scans -fail closed. Never post, quote, echo, or reconstruct an approval yourself. +set. An edited approval comment is invalid. Only a later unedited, non-app +human issue comment containing `/squad revoke-improvement`, whose author has +live write, maintain, or admin repository permission, withdraws it until a new +approval is posted. Revocation permission or history that cannot be resolved +fails closed. Body edits, scope changes, missing revision/permission data and +incomplete scans fail closed. Never post, quote, echo, or reconstruct an +approval yourself. If `context.authorized` is false, make no edits. Post one refusal on `context.issue_number` when it resolves, naming `context.reason` and the manual diff --git a/workflows/package/squad-retro.md b/workflows/package/squad-retro.md index e21aa8930..117481230 100644 --- a/workflows/package/squad-retro.md +++ b/workflows/package/squad-retro.md @@ -1105,8 +1105,11 @@ Approved-Path: {second proposed path} Compute the revision digest with the installed improvement gate's `--revision` command (see the gh-aw guide), then post this as a new human comment. -`/squad revoke-improvement` withdraws it. The improvement worker is in the -standard install; every other proposal stays a manual pull request. +Only a later unedited, non-app human issue comment containing +`/squad revoke-improvement`, from an author whose live repository permission is +write, maintain, or admin, withdraws it. Unresolved revocation permission or +comment-history evidence fails closed. The improvement worker is in the standard +install; every other proposal stays a manual pull request. ```` Never post that command yourself in a comment, and never claim a proposal is diff --git a/workflows/package/squad.md b/workflows/package/squad.md index c61d87a78..7aea66ac1 100644 --- a/workflows/package/squad.md +++ b/workflows/package/squad.md @@ -1755,7 +1755,7 @@ Assign the accepted `mode` from `squad-command-context.json` to ```bash mode="${SQUAD_PARSED_MODE-}" case "$mode" in - status|review|research|plan|revoke-improvement) + status|review|research|plan) echo READ_ONLY ;; *) @@ -1767,17 +1767,12 @@ esac - `READ_ONLY` → skip the permission lookup entirely and continue to **Execute Mode** unchanged. - `AUTH_REQUIRED` → continue to **Step AG-2**. -**Open-mode allow-list:** `status`, `review` (advisory relay), `research`, -`plan` (plan preview), and `revoke-improvement`. These commands remain -available to any actor. Every other recognized mode changes repository state, -revises or advances a durable planning artifact, or dispatches implementation -work, so it requires authorization. - -`revoke-improvement` qualifies because it emits nothing and only ever REMOVES -authority: `squad-improvement-worker`'s gate honors a revocation from any -author, so a red refusal here would contradict a withdrawal that is honored -anyway. `approve-improvement` grants authority and dispatches a worker, so it -stays authorization-required. +**Open-mode allow-list:** `status`, `review` (advisory relay), `research`, and +`plan` (plan preview). These read-only commands remain available to any actor. +Every other recognized mode changes repository state, revises or advances a +durable planning artifact, or dispatches implementation work, so it requires +authorization. This includes `revoke-improvement`, which mutates durable +approval state even though it emits no output. ### Step AG-2: Resolve actor permission [MANDATORY for `AUTH_REQUIRED`] @@ -1835,7 +1830,7 @@ When **Step AG-3** returned `REFUSE`: `⛔ /squad was refused for @ (repository permission: ). Mutating /squad modes require write, maintain, or admin repository permission. Ask a repository maintainer to run this command or grant the required access.` 3. Stop immediately. Do not load **Execute Mode**, do not post success breadcrumbs for the requested mutating mode, and do not emit `dispatch-workflow`, `create-issue`, or `create-pull-request`. -**Authorization-required modes guarded by this section:** `cast`, `connect`, `adopt`, `cast-member`, `retire`, `retro`, `approve-improvement`, `plan revise`, `triage`, `triage revise`, `plan program`, `plan program revise`, `plan implementation`, `plan validate`, `activate`, `plan accept`, `plan accept scope`, `plan accept implementation`, `plan activate`, and `implement`. Phase variants inherit their base parsed mode: `activate phase {N}` → `activate`, `plan accept phase {N}` → `plan accept`, `plan accept implementation phase {N}` → `plan accept implementation`, `plan activate phase {N}` → `plan activate`. +**Authorization-required modes guarded by this section:** `cast`, `connect`, `adopt`, `cast-member`, `retire`, `retro`, `approve-improvement`, `revoke-improvement`, `plan revise`, `triage`, `triage revise`, `plan program`, `plan program revise`, `plan implementation`, `plan validate`, `activate`, `plan accept`, `plan accept scope`, `plan accept implementation`, `plan activate`, and `implement`. Phase variants inherit their base parsed mode: `activate phase {N}` → `activate`, `plan accept phase {N}` → `plan accept`, `plan accept implementation phase {N}` → `plan accept implementation`, `plan activate phase {N}` → `plan activate`. ## Execute Mode diff --git a/workflows/shared/squad-command-contract.mjs b/workflows/shared/squad-command-contract.mjs index be39ed342..b153619af 100644 --- a/workflows/shared/squad-command-contract.mjs +++ b/workflows/shared/squad-command-contract.mjs @@ -48,7 +48,6 @@ const OPEN_MODES = new Set([ 'review', 'research', 'plan', - 'revoke-improvement', ]); export const VALID_COMMANDS = Object.freeze([ @@ -221,9 +220,63 @@ export function classifySquadCommand(payload, eventName = '') { reason: 'Unknown command or malformed arguments.', }; } + if (sourceData.source === 'issue' && classification.mode === 'revoke-improvement') { + return { + status: 'rejected', + source: sourceData.source, + rejectedCommand: invocation.rejectedCommand, + reason: 'Issue-body revocations are not durable. Post /squad revoke-improvement as a new, unedited human issue comment.', + }; + } + if (sourceData.source === 'comment' && payload?.action === 'edited' && + classification.mode === 'revoke-improvement') { + return { + status: 'rejected', + source: sourceData.source, + rejectedCommand: invocation.rejectedCommand, + reason: 'Edited comment revocations are not durable. Post /squad revoke-improvement as a new, unedited human issue comment.', + }; + } return { ...classification, source: sourceData.source }; } +function canonicalInvocation(result) { + if (result.status === 'none') return null; + if (result.status === 'accepted') { + const normalizedArgument = result.argumentText.replace(/\s+/g, ' ').trim(); + const canonicalArgument = result.phase !== null + ? `${result.mode} phase ${result.phase}` + : !normalizedArgument || normalizedArgument.toLowerCase() === result.mode + ? result.mode + : `${result.mode} ${normalizedArgument.slice(result.mode.length).trim()}`; + return JSON.stringify([ + 'accepted', + result.mode, + canonicalArgument, + result.phase, + ]); + } + return JSON.stringify([ + 'rejected', + result.reason, + result.rejectedCommand.replace(/\s+/g, ' ').trim().toLowerCase(), + ]); +} + +export function editedCommandShouldRoute(payload, eventName, currentResult) { + const action = payload?.action; + if (action !== 'edited' || !['issues', 'issue_comment'].includes(eventName)) { + return true; + } + const previousBody = payload?.changes?.body?.from; + if (typeof previousBody !== 'string') return false; + const previousPayload = eventName === 'issue_comment' + ? { ...payload, action: 'created', comment: { ...payload.comment, body: previousBody } } + : { ...payload, action: 'opened', issue: { ...payload.issue, body: previousBody } }; + const previousResult = classifySquadCommand(previousPayload, eventName); + return canonicalInvocation(previousResult) !== canonicalInvocation(currentResult); +} + export function rejectionComment(result) { if (result?.status !== 'rejected') throw new Error('A rejected command result is required.'); return [ diff --git a/workflows/shared/squad-improvement-gate.mjs b/workflows/shared/squad-improvement-gate.mjs index e456ea72e..8e0a04704 100644 --- a/workflows/shared/squad-improvement-gate.mjs +++ b/workflows/shared/squad-improvement-gate.mjs @@ -11,6 +11,7 @@ import { readAgentOutputItems, evaluateRetroPullRequestItems, pullLinksIssue, findNativeLinkedPulls, NATIVE_LINK_MAX_PAGES, } from './squad-retro-provenance.mjs'; +import { classifySquadCommand } from './squad-command-contract.mjs'; export { isNumericId }; export { ACTION_LABEL, PROPOSAL_LABEL, extractActionKey, readAgentOutputItems }; @@ -20,9 +21,13 @@ export const TRUSTED_PERMISSIONS = Object.freeze(['admin', 'maintain', 'write']) export const ALLOWED_PATH_PREFIXES = Object.freeze(['.squad/skills/', '.squad/decisions/inbox/']); export const ALLOWED_FILE_MODE = '100644'; const MAX_PAGES = 5; +const MAX_REVOCATION_PRINCIPALS = 20; +const KNOWN_REVOCATION_PERMISSIONS = new Set([...TRUSTED_PERMISSIONS, 'triage', 'read', 'none']); // issue + comment + comments(<=MAX_PAGES) + permission + pulls(<=MAX_PAGES) + -// revision (1 graphql) + native-link scan (<=NATIVE_LINK_MAX_PAGES graphql). -export const IMPROVEMENT_API_REQUEST_CEILING = 3 + 2 * MAX_PAGES + 1 + NATIVE_LINK_MAX_PAGES; +// revocation permissions + revision (1 graphql) + +// native-link scan (<=NATIVE_LINK_MAX_PAGES graphql). +export const IMPROVEMENT_API_REQUEST_CEILING = + 3 + 2 * MAX_PAGES + MAX_REVOCATION_PRINCIPALS + 1 + NATIVE_LINK_MAX_PAGES; const digest = value => createHash('sha256').update(value).digest('hex'); export const extractProposedPaths = body => [...new Set(standaloneValues(body, 'Proposed-Path'))]; export const scopeDigest = (actionKey, paths) => digest(`${actionKey}\n${[...paths].sort().join('\n')}`).slice(0, 16); @@ -43,8 +48,11 @@ export function isSafePath(path) { } export function parseCommandComment(body) { + const routed = classifySquadCommand({ comment: { body } }, 'issue_comment'); + if (routed.status === 'accepted' && routed.mode === 'revoke-improvement') { + return { command: 'revoke', paths: [] }; + } const lines = normalizeText(body).split('\n').filter(line => line.trim() !== ''); - if (lines[0] === REVOKE_COMMAND) return { command: 'revoke', paths: [] }; if (lines[0] !== APPROVE_COMMAND) return { command: null, paths: [] }; let revision = null; const paths = []; @@ -336,9 +344,55 @@ export async function collectImprovementContext(env = process.env, { if (parsed.command !== 'approve') return refuse('no-approval-comment'); const comments = await collectPages(fetchJson, `repos/${repository}/issues/${number}/comments`, {}, MAX_PAGES); if (comments.truncated) return refuse('comment-history-incomplete'); - const revoked = comments.values.some(entry => entry.user?.type === 'User' && - parseCommandComment(entry.body).command === 'revoke' && - Date.parse(entry.updated_at || entry.created_at) >= Date.parse(comment.created_at)); + const revocationPermissions = new Map(); + const approvedAt = Date.parse(comment.created_at); + const approvalListIndex = comments.values.findIndex(entry => sameNumericId(entry.id, comment.id)); + let revoked = false; + for (const [entryIndex, entry] of comments.values.entries()) { + const parsedEntry = parseCommandComment(entry.body); + if (entry.user?.type === 'User' && !entry.performed_via_github_app && + entry.created_at === entry.updated_at && parsedEntry.command === 'revoke' && + (!Number.isFinite(approvedAt) || !Number.isFinite(Date.parse(entry.created_at)))) { + return refuse('revocation-history-incomplete'); + } + const revokedAt = Date.parse(entry.created_at); + const followsApproval = revokedAt > approvedAt || + (revokedAt === approvedAt && (approvalListIndex >= 0 + ? entryIndex > approvalListIndex + : isNumericId(entry.id) && Number(entry.id) > Number(comment.id))); + if (entry.user?.type !== 'User' || entry.performed_via_github_app || + entry.created_at !== entry.updated_at || + parsedEntry.command !== 'revoke' || + !followsApproval) continue; + const login = typeof entry.user?.login === 'string' && entry.user.login.trim() + ? entry.user.login.trim() + : null; + if (!login) return refuse('revocation-permission-unresolved'); + if (!revocationPermissions.has(login)) { + if (revocationPermissions.size >= MAX_REVOCATION_PRINCIPALS) { + return refuse('revocation-history-incomplete'); + } + let revocationPermission; + try { + revocationPermission = await fetchJson( + `repos/${repository}/collaborators/${encodeURIComponent(login)}/permission`, + {}, + ); + } catch { + return refuse('revocation-permission-unresolved'); + } + if (!revocationPermission || revocationPermission.__status || + typeof revocationPermission.permission !== 'string' || + !KNOWN_REVOCATION_PERMISSIONS.has(revocationPermission.permission)) { + return refuse('revocation-permission-unresolved'); + } + revocationPermissions.set(login, revocationPermission.permission); + } + if (isTrustedApprover({ permission: revocationPermissions.get(login) }).trusted) { + revoked = true; + break; + } + } const permission = await fetchJson(`repos/${repository}/collaborators/${encodeURIComponent(comment.user?.login || '')}/permission`, {}); const revision = await fetchGraphql( 'query($owner:String!,$name:String!,$number:Int!){repository(owner:$owner,name:$name){issue(number:$number){number title body lastEditedAt}}}', diff --git a/workflows/squad-command-router.md b/workflows/squad-command-router.md index 38552ce11..c16d57e11 100644 --- a/workflows/squad-command-router.md +++ b/workflows/squad-command-router.md @@ -56,10 +56,18 @@ safe-outputs: context.payload, process.env.SQUAD_EVENT_NAME, ); + if (!contract.editedCommandShouldRoute( + context.payload, + process.env.SQUAD_EVENT_NAME, + result, + )) { + core.info('Ignoring an edited body whose canonical /squad invocation did not change or whose previous body is unavailable.'); + return; + } if (result.status === 'accepted' || result.status === 'rejected') { // A bot-authored issue or comment body is never a trusted Squad // command, open mode or not: open modes (`status`, `review`, - // `research`, `plan`, `revoke-improvement`) intentionally skip + // `research`, `plan`) intentionally skip // the permission check below, so without this guard any bot that // reposts or quotes `/squad` text (for example a relay, mirror, // or notification bot) could replay it into a real dispatch with diff --git a/workflows/squad-improvement-worker.md b/workflows/squad-improvement-worker.md index 54d02f6c2..27c5b4001 100644 --- a/workflows/squad-improvement-worker.md +++ b/workflows/squad-improvement-worker.md @@ -143,10 +143,13 @@ Approved-Path: {exact proposed file} Compute the digest using the installed gate's `--revision` command as documented in the gh-aw guide. The issue's exact `Proposed-Path:` set must equal the approved -set. An edited approval comment is invalid. Any later human -`/squad revoke-improvement` withdraws it until a new approval is posted. -Body edits, scope changes, missing revision/permission data and incomplete scans -fail closed. Never post, quote, echo, or reconstruct an approval yourself. +set. An edited approval comment is invalid. Only a later unedited, non-app +human issue comment containing `/squad revoke-improvement`, whose author has +live write, maintain, or admin repository permission, withdraws it until a new +approval is posted. Revocation permission or history that cannot be resolved +fails closed. Body edits, scope changes, missing revision/permission data and +incomplete scans fail closed. Never post, quote, echo, or reconstruct an +approval yourself. If `context.authorized` is false, make no edits. Post one refusal on `context.issue_number` when it resolves, naming `context.reason` and the manual diff --git a/workflows/squad-retro.md b/workflows/squad-retro.md index 807ec2aae..dd0b5c5b7 100644 --- a/workflows/squad-retro.md +++ b/workflows/squad-retro.md @@ -582,8 +582,11 @@ Approved-Path: {second proposed path} Compute the revision digest with the installed improvement gate's `--revision` command (see the gh-aw guide), then post this as a new human comment. -`/squad revoke-improvement` withdraws it. The improvement worker is in the -standard install; every other proposal stays a manual pull request. +Only a later unedited, non-app human issue comment containing +`/squad revoke-improvement`, from an author whose live repository permission is +write, maintain, or admin, withdraws it. Unresolved revocation permission or +comment-history evidence fails closed. The improvement worker is in the standard +install; every other proposal stays a manual pull request. ```` Never post that command yourself in a comment, and never claim a proposal is diff --git a/workflows/squad-workflows.manifest.json b/workflows/squad-workflows.manifest.json index 876421f9b..7a1718ec6 100644 --- a/workflows/squad-workflows.manifest.json +++ b/workflows/squad-workflows.manifest.json @@ -14,9 +14,9 @@ "source": "workflows/package/squad.md", "destination": ".github/workflows/squad.md", "lock": ".github/workflows/squad.lock.yml", - "source_sha256": "c1e015f48e2a86b0c5cb00bb2e622109ed5e741e4326263e88999699d182bb0c", - "lock_sha256": "fc69c0ba0c9fb562d49c3eb92f3546ebe086f2a43e155ed67dbf87bb83ec4a2e", - "package_lock_sha256": "2d2bc47293b3e2b3be774667eebff94b95bc0637a333af0f6736fd50e7658a18" + "source_sha256": "450e9d3d7ef1b173cedd054d89e72700d8545406bd1fe0acd813d5ed76e04e91", + "lock_sha256": "c5d6e1b7a42e7f4b7dd748ae59c762d08acba9f20e0d6328290eec72e5358717", + "package_lock_sha256": "d0688be3e9492288091c103bd55d6e2845f4541f9e6915db2d4a84e488d4b763" }, { "name": "squad-implement-worker", @@ -50,18 +50,18 @@ "source": "workflows/package/squad-retro.md", "destination": ".github/workflows/squad-retro.md", "lock": ".github/workflows/squad-retro.lock.yml", - "source_sha256": "3ee1bdd2d032f70d83f5593ff2bfc71fef6782d1cef0daf17069dfcb95adafcd", - "lock_sha256": "7850fb52a8ac0eb4e7f97a9846c54986607c77d3fa1fb4dce934bdc44b632994", - "package_lock_sha256": "cb11982ae6720f511ab73a8932773a521ec32e5afe453ac6ca6d015dd2bb5c2c" + "source_sha256": "8d7e1fdd5fa337da8208832b1cc7c6dbfd253dde2c1b45a7f68a805eb9025620", + "lock_sha256": "41fe367f5391eea781a0045adff8cb5039a689123b5b1798367a0078e19073e9", + "package_lock_sha256": "19933818ea4a2a89040e9e12706f80b56e6b6d6e53925f92c66f88d99676184f" }, { "name": "squad-improvement-worker", "source": "workflows/package/squad-improvement-worker.md", "destination": ".github/workflows/squad-improvement-worker.md", "lock": ".github/workflows/squad-improvement-worker.lock.yml", - "source_sha256": "89896bc4321f76e791d40ee8239ba7ed6a2b8e090113a1cdb3017cb2a68e5c90", - "lock_sha256": "65d905f43b88b2dffaa8bf90c8fd9f4d10ac2a3db3789a439ec998a2e5ad84d1", - "package_lock_sha256": "caf333235528b716c1177684be94752e56a288a5dc87723b6abcf99074d388d1" + "source_sha256": "5bcc0f095b98a351c8f5fd8b31d22fee97f2cf00eeaa02519c394f5bbb5a6bad", + "lock_sha256": "9eb03c71348274e867be04af31526b5f303f26fe6a232869f36185b990e2b129", + "package_lock_sha256": "8d65224df7922b63bf85908d10912014f865a1d62c7cbfaf26a0631206405ac3" }, { "name": "squad-bootstrap", @@ -77,9 +77,9 @@ "source": "workflows/package/squad-command-router.md", "destination": ".github/workflows/squad-command-router.md", "lock": ".github/workflows/squad-command-router.lock.yml", - "source_sha256": "0af5bebf4c347f1ae5dcfaa3e9d4a45bc1960e343d8f25e7ccd727ebb0822c99", - "lock_sha256": "a8c21ef7884f608098cc0124d860bf4967c11776c189e36fb3822ba138602516", - "package_lock_sha256": "cba7225bba79187068ccf7d827f2ab7c6d73b94c6506abdfc50af86568f18ac1" + "source_sha256": "451d5b1b11d5a5778595f9bed8d532c09b353156ef763efe1170e0b95f967011", + "lock_sha256": "39aa2b8705b817a47b9e7d4f4406bf2e69243c0b4b21184b65d577b23b4382d9", + "package_lock_sha256": "1a11377af6525bdfd0c2ec20d8aadf2636a3c6989f83569068410b6e0afadde4" } ], "shared_runtime": [ @@ -105,7 +105,7 @@ "package_destination": ".github/workflows/shared/squad-command-contract.mjs", "destination": ".github/workflows/shared/squad-command-contract.mjs", "ownership": "manifest", - "sha256": "9c6d70b7c590b36488de48e5e8b2fcaa59793115060bb85bac9dbfd6fe934b7d" + "sha256": "cbe2506688c4137f9074452ba0d55eed073fb0f33aec39bae1be82b71806c2ec" }, { "path": "shared/squad-cast-validator.mjs", @@ -129,7 +129,7 @@ "package_destination": ".github/workflows/shared/squad-improvement-gate.mjs", "destination": ".github/workflows/shared/squad-improvement-gate.mjs", "ownership": "manifest", - "sha256": "dd63e47f818035f7757b148942bd9779134a29b7e6795eadb5ea85406621e0f7" + "sha256": "99b9f8e3a93d6f0bff57df927ace89f7be0ea6ba86bb99472103ff53b6f641e1" }, { "path": "shared/squad-retro-evidence.mjs", diff --git a/workflows/squad.md b/workflows/squad.md index e9328615e..1a93f9385 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -610,7 +610,7 @@ Assign the accepted `mode` from `squad-command-context.json` to ```bash mode="${SQUAD_PARSED_MODE-}" case "$mode" in - status|review|research|plan|revoke-improvement) + status|review|research|plan) echo READ_ONLY ;; *) @@ -622,17 +622,12 @@ esac - `READ_ONLY` → skip the permission lookup entirely and continue to **Execute Mode** unchanged. - `AUTH_REQUIRED` → continue to **Step AG-2**. -**Open-mode allow-list:** `status`, `review` (advisory relay), `research`, -`plan` (plan preview), and `revoke-improvement`. These commands remain -available to any actor. Every other recognized mode changes repository state, -revises or advances a durable planning artifact, or dispatches implementation -work, so it requires authorization. - -`revoke-improvement` qualifies because it emits nothing and only ever REMOVES -authority: `squad-improvement-worker`'s gate honors a revocation from any -author, so a red refusal here would contradict a withdrawal that is honored -anyway. `approve-improvement` grants authority and dispatches a worker, so it -stays authorization-required. +**Open-mode allow-list:** `status`, `review` (advisory relay), `research`, and +`plan` (plan preview). These read-only commands remain available to any actor. +Every other recognized mode changes repository state, revises or advances a +durable planning artifact, or dispatches implementation work, so it requires +authorization. This includes `revoke-improvement`, which mutates durable +approval state even though it emits no output. ### Step AG-2: Resolve actor permission [MANDATORY for `AUTH_REQUIRED`] @@ -690,7 +685,7 @@ When **Step AG-3** returned `REFUSE`: `⛔ /squad was refused for @ (repository permission: ). Mutating /squad modes require write, maintain, or admin repository permission. Ask a repository maintainer to run this command or grant the required access.` 3. Stop immediately. Do not load **Execute Mode**, do not post success breadcrumbs for the requested mutating mode, and do not emit `dispatch-workflow`, `create-issue`, or `create-pull-request`. -**Authorization-required modes guarded by this section:** `cast`, `connect`, `adopt`, `cast-member`, `retire`, `retro`, `approve-improvement`, `plan revise`, `triage`, `triage revise`, `plan program`, `plan program revise`, `plan implementation`, `plan validate`, `activate`, `plan accept`, `plan accept scope`, `plan accept implementation`, `plan activate`, and `implement`. Phase variants inherit their base parsed mode: `activate phase {N}` → `activate`, `plan accept phase {N}` → `plan accept`, `plan accept implementation phase {N}` → `plan accept implementation`, `plan activate phase {N}` → `plan activate`. +**Authorization-required modes guarded by this section:** `cast`, `connect`, `adopt`, `cast-member`, `retire`, `retro`, `approve-improvement`, `revoke-improvement`, `plan revise`, `triage`, `triage revise`, `plan program`, `plan program revise`, `plan implementation`, `plan validate`, `activate`, `plan accept`, `plan accept scope`, `plan accept implementation`, `plan activate`, and `implement`. Phase variants inherit their base parsed mode: `activate phase {N}` → `activate`, `plan accept phase {N}` → `plan accept`, `plan accept implementation phase {N}` → `plan accept implementation`, `plan activate phase {N}` → `plan activate`. ## Execute Mode