diff --git a/test/gh-aw-command-parse.test.ts b/test/gh-aw-command-parse.test.ts index e7349d8dd..01196057c 100644 --- a/test/gh-aw-command-parse.test.ts +++ b/test/gh-aw-command-parse.test.ts @@ -48,20 +48,45 @@ function bashBlockAfter(heading: string): string { const PC0_HEADING = '### Step PC-0: Normalize a dispatched command'; const PC1_HEADING = '### Step PC-1: Extract the command argument'; const PC3_HEADING = '### Step PC-3: No recognized command'; +const AG1_HEADING = '### Step AG-1: Classify the parsed mode'; +const AG2_HEADING = '### Step AG-2: Resolve actor permission'; +const AG3_HEADING = '### Step AG-3: Decide authorization'; +const AG4_HEADING = '### Step AG-4: Refuse unauthorized mutation loudly'; -/** Run a command extracted from the workflow with a value in the environment. */ -function runDeclared(command: string, value: string, varName = 'SQUAD_TRIGGER_BODY'): string { +/** Run a command extracted from the workflow with real environment values. */ +function runDeclaredEnv(command: string, env: Record): string { if (!POSIX_SHELL) throw new Error('no POSIX shell resolved'); return execFileSync(POSIX_SHELL, ['-c', command], { encoding: 'utf8', - env: { ...process.env, [varName]: value }, + env: { ...process.env, ...env }, }).replace(/\r/g, '').replace(/\n+$/, ''); } +/** Run a command extracted from the workflow with one value in the environment. */ +function runDeclared(command: string, value: string, varName = 'SQUAD_TRIGGER_BODY'): string { + return runDeclaredEnv(command, { [varName]: value }); +} + const parse = (body: string) => runDeclared(bashBlockAfter(PC1_HEADING), body); const diagnose = (body: string) => runDeclared(bashBlockAfter(PC3_HEADING), body); const normalize = (command: string) => runDeclared(bashBlockAfter(PC0_HEADING), command, 'SQUAD_DISPATCH_COMMAND'); +const classifyMode = (mode: string) => runDeclared(bashBlockAfter(AG1_HEADING), mode, 'SQUAD_PARSED_MODE'); +const decideAuthorization = (modeClass: string, permission: string) => + runDeclaredEnv(bashBlockAfter(AG3_HEADING), { + SQUAD_MODE_AUTH_CLASS: modeClass, + SQUAD_ACTOR_PERMISSION: permission, + }); +const resolveActorPermission = ( + eventName: string, + actor = '', + repository = '' +) => + runDeclaredEnv(bashBlockAfter(AG2_HEADING), { + SQUAD_EVENT_NAME: eventName, + SQUAD_TRIGGER_ACTOR: actor, + SQUAD_REPOSITORY: repository, + }); /** The dispatch path exactly as the workflow runs it: PC-0, then PC-1. */ const parseDispatch = (command: string) => parse(normalize(command)); @@ -307,4 +332,106 @@ describe('gh-aw: /squad command parsing (#1824)', () => { ); }); }); + + describe('Actor Authorization Guard (#1730)', () => { + const ag1 = workflow.slice(workflow.indexOf(AG1_HEADING), workflow.indexOf(AG2_HEADING)); + const ag2 = workflow.slice(workflow.indexOf(AG2_HEADING), workflow.indexOf(AG3_HEADING)); + const ag4 = workflow.slice(workflow.indexOf(AG4_HEADING), workflow.indexOf('## Execute Mode')); + + it.each([ + 'implement', + 'cast', + 'connect', + 'adopt', + 'cast-member', + 'retire', + 'plan revise', + 'triage', + 'triage revise', + 'plan program', + 'plan program revise', + 'plan implementation', + 'plan validate', + 'plan accept', + 'plan accept scope', + 'plan accept implementation', + 'plan activate', + ])( + 'classifies mutating mode %j as requiring authorization', + mode => { + expect( + classifyMode(mode), + `${JSON.stringify(mode)} must require authorization before the router mutates state or dispatches work.` + ).toBe('AUTH_REQUIRED'); + } + ); + + it.each(['research', 'status', 'plan'])( + 'classifies open mode %j as bypassing authorization', + mode => { + expect( + classifyMode(mode), + `${JSON.stringify(mode)} must stay on the explicit read-only allow-list so its UX stays open.` + ).toBe('READ_ONLY'); + } + ); + + it.each(['NO_COMMAND', 'unknown', 'plan accepted?', ''])( + 'does not silently classify malformed mode %j as read-only', + mode => { + expect( + classifyMode(mode), + `${JSON.stringify(mode)} must not bypass the guard by landing on the read-only branch.` + ).toBe('AUTH_REQUIRED'); + } + ); + + it.each(['DISPATCH_AUTHORIZED', 'admin', 'maintain', 'write'])( + 'permits mutating modes for repository permission %j', + permission => { + expect( + decideAuthorization('AUTH_REQUIRED', permission), + `${JSON.stringify(permission)} must authorize a mutating /squad mode.` + ).toBe('AUTHORIZED'); + } + ); + + it.each(['read', 'triage', 'none', '', 'PERMISSION_UNRESOLVED'])( + 'refuses mutating modes for repository permission %j', + permission => { + expect( + decideAuthorization('AUTH_REQUIRED', permission), + `${JSON.stringify(permission)} must fail closed for a mutating /squad mode.` + ).toBe('REFUSE'); + } + ); + + it('skips the permission branch entirely for read-only modes', () => { + expect(decideAuthorization('READ_ONLY', 'none')).toBe('AUTH_SKIPPED'); + expect(ag1).toContain('skip the permission lookup entirely'); + expect(ag2).toContain('When **Step AG-1** returned `AUTH_REQUIRED`'); + }); + + it('uses GitHub workflow_dispatch authorization for controlled relays', () => { + expect(resolveActorPermission('workflow_dispatch')).toBe('DISPATCH_AUTHORIZED'); + expect(ag2).toContain('GitHub requires write access to trigger `workflow_dispatch`'); + expect(ag2).toContain('Only the exact `workflow_dispatch` event'); + }); + + it('fails closed when non-dispatch actor identity cannot be resolved', () => { + expect(resolveActorPermission('issue_comment')).toBe('PERMISSION_UNRESOLVED'); + }); + + it('uses the collaborator-permission API and routes refusals to an actionable comment', () => { + expect(ag2).toContain('collaborators/$actor/permission'); + expect(ag2).toContain("jq -r '.permission // empty'"); + expect(ag2).toContain('PERMISSION_UNRESOLVED'); + expect(ag4).toContain('add-comment'); + expect(ag4).toContain('::error::'); + expect(ag4).toContain('Ask a repository maintainer'); + expect(ag4).toContain('admin'); + expect(ag4).toContain('maintain'); + expect(ag4).toContain('write'); + }); + }); }); diff --git a/test/gh-aw-quality.test.ts b/test/gh-aw-quality.test.ts index 8a0337d0b..aa13157e4 100644 --- a/test/gh-aw-quality.test.ts +++ b/test/gh-aw-quality.test.ts @@ -199,6 +199,24 @@ function extractWorkflowDispatchInputs(frontmatter: string): Record | undefined { + const lines = frontmatter.split('\n'); + const concurrencyLine = lines.findIndex(line => /^concurrency:\s*$/.test(line)); + if (concurrencyLine === -1) return undefined; + + const block: Record = {}; + for (let i = concurrencyLine + 1; i < lines.length; i++) { + const line = lines[i]; + if (/^\S/.test(line)) break; + + const propertyMatch = line.match(/^ ([A-Za-z0-9_-]+):\s*(.+)$/); + if (propertyMatch) { + block[propertyMatch[1]] = propertyMatch[2]; + } + } + + return Object.keys(block).length > 0 ? block : undefined; +} /** Extract mode table rows from the "## Modes" section of the workflow body. */ function extractModeTable(content: string): Array<{ command: string; mode: string; description: string }> { @@ -347,6 +365,33 @@ describe('gh-aw: safe-output configuration', () => { }); }); +describe('gh-aw: router concurrency guard (#1730)', () => { + const frontmatter = extractFrontmatter(SQUAD_WORKFLOW); + const concurrency = extractConcurrency(frontmatter); + + it('lets the router post explicit mode-specific authorization refusals', () => { + expect(frontmatter).toMatch(/^ roles: all$/m); + }); + + it('declares issue-scoped concurrency without cancel-in-progress', () => { + expect(concurrency, 'squad.md should declare a concurrency block').toBeDefined(); + expect(concurrency?.group).toBe( + '"squad-${{ github.event.inputs.issue_number || github.event.issue.number || github.event.pull_request.number || github.run_id }}"' + ); + expect(concurrency?.['cancel-in-progress']).toBe('false'); + expect(concurrency?.group, 'group must resolve the manual-dispatch issue number').toContain( + 'github.event.inputs.issue_number' + ); + expect(concurrency?.group, 'group must resolve issue and issue_comment events').toContain( + 'github.event.issue.number' + ); + expect(concurrency?.group, 'group must resolve pull_request_review_comment events').toContain( + 'github.event.pull_request.number' + ); + expect(concurrency?.group, 'group must not collapse to a static/global lock').toContain('github.run_id'); + }); +}); + // --------------------------------------------------------------------------- // Test: Mode Dispatch Completeness // --------------------------------------------------------------------------- diff --git a/workflows/squad.md b/workflows/squad.md index 4c9606e2b..77b18c292 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -6,6 +6,7 @@ emoji: "🤖" private: false on: bots: ["github-actions[bot]"] + roles: all slash_command: name: squad events: @@ -30,6 +31,9 @@ permissions: copilot-requests: write issues: read pull-requests: read +concurrency: + group: "squad-${{ github.event.inputs.issue_number || github.event.issue.number || github.event.pull_request.number || github.run_id }}" + cancel-in-progress: false network: allowed: - defaults @@ -412,9 +416,94 @@ Steps 1–3 are load-bearing precisely because their output is observable: an agent chooses. Do not drop them in favor of step 4, and do not call step 4 a guarantee. +## Actor Authorization Guard + +Run this guard after **Step PC-2** resolves the parsed mode and before **Execute Mode** loads any skill. + +### Step AG-1: Classify the parsed mode [MANDATORY] + +Authorization is opt-out only for the explicit open-mode allow-list below. Never infer "read-only" from a prefix, from the absence of a mutating keyword, or from prose. Anything outside the allow-list — including empty, malformed, or future mode strings — requires authorization or should already have been stopped by **Step PC-3**. Unknown text must never bypass this guard by being treated as read-only. + +Assign the parsed mode string from **Step PC-2** to `SQUAD_PARSED_MODE` and run exactly this: + +```bash +mode="${SQUAD_PARSED_MODE-}" +case "$mode" in + status|research|plan) + echo READ_ONLY + ;; + *) + echo AUTH_REQUIRED + ;; +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`, `research`, and `plan` (plan preview). These commands remain available to any actor. Every other recognized mode either changes repository state, revises or advances a durable planning artifact, or dispatches implementation work, so it requires authorization. + +### Step AG-2: Resolve actor permission [MANDATORY for `AUTH_REQUIRED`] + +When **Step AG-1** returned `AUTH_REQUIRED`, resolve the event, actor, and repository only through named YAML `env:` bindings; never embed Actions expressions inside a shell block. Use `github.event_name` for `SQUAD_EVENT_NAME`, `github.actor` for `SQUAD_TRIGGER_ACTOR`, and `github.repository` for `SQUAD_REPOSITORY`. + +GitHub requires write access to trigger `workflow_dispatch`. That platform authorization also covers the controlled `dispatch-workflow` relay from `squad-implement-worker`; do not look up the relay bot as though it were a human collaborator. For all issue, issue-comment, and pull-request-review-comment paths, call the collaborator-permission API for the triggering actor. + +```bash +event="${SQUAD_EVENT_NAME-}" +actor="${SQUAD_TRIGGER_ACTOR-}" +repository="${SQUAD_REPOSITORY-}" +if [ "$event" = "workflow_dispatch" ]; then + echo DISPATCH_AUTHORIZED +elif [ -z "$actor" ] || [ -z "$repository" ]; then + echo PERMISSION_UNRESOLVED +else + perm="$(gh api "repos/$repository/collaborators/$actor/permission" 2>/dev/null | jq -r '.permission // empty' 2>/dev/null || true)" + if [ -n "$perm" ]; then + printf '%s\n' "$perm" + else + echo PERMISSION_UNRESOLVED + fi +fi +``` + +Only the exact `workflow_dispatch` event receives `DISPATCH_AUTHORIZED`; never use a generic event fallback. On every other event, API errors, empty output, and missing actor/repository identity are all `PERMISSION_UNRESOLVED`. Fail closed — never continue a mutating mode on an unresolved permission signal. + +### Step AG-3: Decide authorization [MANDATORY] + +Assign **Step AG-1**'s output to `SQUAD_MODE_AUTH_CLASS` and **Step AG-2**'s output to `SQUAD_ACTOR_PERMISSION`, then run exactly this: + +```bash +mode_class="${SQUAD_MODE_AUTH_CLASS-}" +perm="${SQUAD_ACTOR_PERMISSION-}" +if [ "$mode_class" = "READ_ONLY" ]; then + echo AUTH_SKIPPED +else + case "$perm" in + DISPATCH_AUTHORIZED|admin|maintain|write) echo AUTHORIZED ;; + *) echo REFUSE ;; + esac +fi +``` + +- `AUTHORIZED` → continue to **Execute Mode**. +- `AUTH_SKIPPED` → continue to **Execute Mode**. +- `REFUSE` → go to **Step AG-4**. This includes `read`, `triage`, `none`, `PERMISSION_UNRESOLVED`, empty output, and every other value not explicitly authorized above. + +### Step AG-4: Refuse unauthorized mutation loudly [MANDATORY] + +When **Step AG-3** returned `REFUSE`: + +1. Emit `echo "::error::Squad refused mutating mode '' for actor '' — repository permission admin, maintain, or write is required"` so the run log is visibly red. If the permission lookup failed, say that it was unresolved instead of inventing a tier. +2. Use the existing `add-comment` safe-output to post exactly one refusal comment on the triggering issue or pull request: + `⛔ /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`, `plan revise`, `triage`, `triage revise`, `plan program`, `plan program revise`, `plan implementation`, `plan validate`, `plan accept`, `plan accept scope`, `plan accept implementation`, `plan activate`, and `implement`. Phase variants inherit their base parsed mode: `plan accept phase {N}` → `plan accept`, `plan accept implementation phase {N}` → `plan accept implementation`, `plan activate phase {N}` → `plan activate`. + ## Execute Mode -Each mode's playbook ships as a **skill**. Load only the one skill for the parsed mode, then follow it verbatim. +Each mode's playbook ships as a **skill**. Enter this section only after **Actor Authorization Guard** returned `AUTHORIZED` or `AUTH_SKIPPED`. Load only the one skill for the parsed mode, then follow it verbatim. **MODE ISOLATION:** Execute ONLY the active mode's skill. Other modes' instructions do not apply — do not load more than one mode skill.