diff --git a/test/gh-aw-command-parse.test.ts b/test/gh-aw-command-parse.test.ts index 01196057c..e64631280 100644 --- a/test/gh-aw-command-parse.test.ts +++ b/test/gh-aw-command-parse.test.ts @@ -366,7 +366,7 @@ describe('gh-aw: /squad command parsing (#1824)', () => { } ); - it.each(['research', 'status', 'plan'])( + it.each(['research', 'status', 'review', 'plan'])( 'classifies open mode %j as bypassing authorization', mode => { expect( diff --git a/test/gh-aw-review-workflow.test.ts b/test/gh-aw-review-workflow.test.ts new file mode 100644 index 000000000..f2e565139 --- /dev/null +++ b/test/gh-aw-review-workflow.test.ts @@ -0,0 +1,269 @@ +import { afterAll, describe, expect, it } from 'vitest'; +import { cpSync, mkdirSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { execFileSync } from 'node:child_process'; +import { tmpdir } from 'node:os'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..'); +const REVIEWER = read('workflows/squad-review.md'); +const ROUTER = read('workflows/squad.md'); +const REVIEWER_FRONTMATTER = frontmatter(REVIEWER); +const ROUTER_FRONTMATTER = frontmatter(ROUTER); +const compileWorkspaces: string[] = []; + +function read(relativePath: string): string { + return readFileSync(resolve(ROOT, relativePath), 'utf8').replace(/\r\n/g, '\n'); +} + +function frontmatter(markdown: string): string { + return markdown.match(/^---\n([\s\S]*?)\n---/)?.[1] ?? ''; +} + +function yamlBlock(yaml: string, key: string): string { + const lines = yaml.split('\n'); + const escapedKey = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const keyPattern = new RegExp(`^(\\s*)${escapedKey}:\\s*(.*)$`); + const start = lines.findIndex(line => keyPattern.test(line)); + if (start === -1) return ''; + + const indent = lines[start].match(keyPattern)![1].length; + const block = [lines[start]]; + for (let index = start + 1; index < lines.length; index++) { + const line = lines[index]; + if (line.trim() !== '' && line.search(/\S/) <= indent) break; + block.push(line); + } + return block.join('\n'); +} + +function listInBlock(block: string, key: string): string[] { + const escapedKey = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const inline = block.match(new RegExp(`^\\s*${escapedKey}:\\s*\\[(.*)\\]\\s*$`, 'm')); + if (inline) { + return inline[1] + .split(',') + .map(item => item.trim().replace(/^['"]|['"]$/g, '')) + .filter(Boolean); + } + + const lines = block.split('\n'); + const keyPattern = new RegExp(`^(\\s*)${escapedKey}:\\s*$`); + const start = lines.findIndex(line => keyPattern.test(line)); + if (start === -1) return []; + + const indent = lines[start].match(keyPattern)![1].length; + const items: string[] = []; + for (let index = start + 1; index < lines.length; index++) { + const line = lines[index]; + if (line.trim() !== '' && line.search(/\S/) <= indent) break; + const item = line.match(/^\s+-\s+(.+)$/)?.[1]; + if (item) items.push(item.trim().replace(/^['"]|['"]$/g, '')); + } + return items; +} + +function provenanceRows(workflow: string): string[] { + const section = workflow.match(/## Provenance decision tree\n([\s\S]*?)(?=\n## )/)?.[1] ?? ''; + return [...section.matchAll(/^\|\s*([1-4])\s*\|\s*([^|]+)\|\s*([^|]+)\|$/gm)] + .map(match => `${match[1]}:${match[2].trim()}:${match[3].trim()}`); +} + +function assertReviewerContract(workflow: string): void { + const yaml = frontmatter(workflow); + const tools = yamlBlock(yaml, 'tools'); + const outputs = yamlBlock(yaml, 'safe-outputs'); + const submitReview = yamlBlock(outputs, 'submit-pull-request-review'); + const concurrency = yamlBlock(yaml, 'concurrency'); + const trigger = yamlBlock(yaml, 'on'); + const rows = provenanceRows(workflow); + + expect(tools).not.toMatch(/^\s+edit:/m); + expect(outputs).not.toMatch(/^\s+(dispatch-workflow|create-issue|create-pull-request|update-pull-request):/m); + expect(listInBlock(submitReview, 'allowed-events')).toEqual(['COMMENT', 'REQUEST_CHANGES']); + expect(submitReview).not.toContain('APPROVE'); + expect(concurrency).toContain('cancel-in-progress: true'); + expect(trigger).not.toMatch(/^\s+forks:/m); + expect(yaml).toContain('github.event.pull_request.head.repo.full_name == github.repository'); + expect(workflow).toContain('Squad-Review-Head: {40-character lowercase head SHA}'); + expect(rows).toEqual([ + '1:One validated durable worker marker:Squad-authored', + '2:`squad/implement-*` head branch with no marker-like text:Squad-authored fallback', + '3:Login `copilot-swe-agent[bot]` or `copilot/*` head branch with no marker-like text:Copilot-authored', + '4:None of the above:Unattributed', + ]); + expect(workflow).toMatch(/invalid provenance\. Fail closed with\s+`noop`; do not fall back/); + expect(workflow).toMatch(/`Unattributed` is an automatic-review\s+refusal/); + expect(workflow).toContain('Human approval remains mandatory.'); +} + +interface CompiledContract { + lock: string; + safeOutputs: Record>; +} + +function compileReviewer(): CompiledContract { + const workspace = mkdtempSync(resolve(tmpdir(), 'squad-review-contract-')); + compileWorkspaces.push(workspace); + const workflowDir = resolve(workspace, '.github', 'workflows'); + mkdirSync(workflowDir, { recursive: true }); + cpSync(resolve(ROOT, 'workflows'), workflowDir, { recursive: true }); + execFileSync('git', ['init', '--quiet'], { cwd: workspace }); + execFileSync( + 'gh', + ['aw', 'compile', 'squad-review', '--strict', '--no-check-update'], + { cwd: workspace, encoding: 'utf8', stdio: 'pipe' }, + ); + + const lock = readFileSync(resolve(workflowDir, 'squad-review.lock.yml'), 'utf8').replace(/\r\n/g, '\n'); + const lines = lock.split('\n'); + const configStart = lines.findIndex(line => line.includes('/safeoutputs/config.json') && line.includes('<<')); + const delimiter = lines[configStart]?.match(/<< '([^']+)'/)?.[1]; + const configEnd = delimiter + ? lines.findIndex((line, index) => index > configStart && line.trim() === delimiter) + : -1; + + expect(configStart, 'compiled reviewer must write safe-output config').toBeGreaterThanOrEqual(0); + expect(delimiter, 'safe-output config must use a parseable heredoc').toBeDefined(); + expect(configEnd, 'safe-output config heredoc must terminate').toBeGreaterThan(configStart); + + return { + lock, + safeOutputs: JSON.parse(lines.slice(configStart + 1, configEnd).join('\n')) as Record< + string, + Record + >, + }; +} + +afterAll(() => { + for (const workspace of compileWorkspaces) { + rmSync(workspace, { recursive: true, force: true }); + } +}); + +describe('gh-aw advisory Squad reviewer', () => { + it('routes /squad review to the isolated workflow and supports automatic PR events', () => { + const routerDispatch = yamlBlock(ROUTER_FRONTMATTER, 'dispatch-workflow'); + const reviewTrigger = yamlBlock(REVIEWER_FRONTMATTER, 'on'); + const relay = ROUTER.match(/## skill: `squad-review-relay`([\s\S]*?)(?=\n## skill:)/)?.[1] ?? ''; + const relayPayload = JSON.parse(relay.match(/```json\n([\s\S]*?)\n```/)?.[1] ?? '{}') as { + workflow_name?: string; + inputs?: Record; + issue_number?: string; + }; + + expect(listInBlock(routerDispatch, 'workflows')).toContain('squad-review'); + expect(ROUTER).toContain('| `/squad review` | Review Relay |'); + expect(ROUTER_FRONTMATTER).toContain('- pull_request_comment'); + expect(REVIEWER).not.toContain('slash_command:'); + expect(reviewTrigger).toMatch(/workflow_dispatch:\n\s+inputs:/); + expect(reviewTrigger).toMatch(/pull_request:\n\s+types: \[ready_for_review, synchronize\]/); + expect(relayPayload).toEqual({ + workflow_name: 'squad-review', + inputs: { + issue_number: '{pull-request-number}', + expected_head_sha: '{current-head-sha}', + request_origin: 'manual', + }, + }); + expect(relayPayload.issue_number).toBeUndefined(); + expect(relay).not.toContain('"pr_number"'); + expect(relay).toContain('Never call the generic'); + }); + + it('enforces attribution priority and refuses malformed or unattributed automatic provenance', () => { + expect(REVIEWER).toContain( + '^$', + ); + expect(REVIEWER).toContain('require exactly one marker-like occurrence'); + expect(REVIEWER).toContain('^squad/implement-{captured-issue}-'); + assertReviewerContract(REVIEWER); + }); + + it('keeps reviewer authority advisory-only with bounded verdicts', () => { + const safeOutputs = yamlBlock(REVIEWER_FRONTMATTER, 'safe-outputs'); + const outputNames = [...safeOutputs.matchAll(/^ ([\w-]+):\s*$/gm)].map(match => match[1]); + + expect(outputNames).toEqual([ + 'add-comment', + 'create-pull-request-review-comment', + 'submit-pull-request-review', + ]); + expect(REVIEWER).toContain('Never use `APPROVE`'); + assertReviewerContract(REVIEWER); + }); + + it('deduplicates by head SHA, cancels stale runs, and retains fork protection', () => { + const concurrency = yamlBlock(REVIEWER_FRONTMATTER, 'concurrency'); + + expect(concurrency).toContain( + 'group: "squad-review-${{ github.event.inputs.issue_number || github.event.pull_request.number || github.run_id }}"', + ); + expect(concurrency).toContain('cancel-in-progress: true'); + expect(REVIEWER_FRONTMATTER).not.toMatch(/^\s+forks:/m); + expect(REVIEWER).toContain('If an existing review body contains the exact marker'); + expect(REVIEWER).toContain('Never re-review an unchanged head'); + expect(REVIEWER).toContain('Re-fetch the pull request immediately before emitting'); + }); + + it('covers acceptance, routing, protected files, tests, and changesets', () => { + for (const requirement of [ + 'acceptance criteria', + '.squad/routing.md', + 'charter named by any `squad:{member}` issue label', + 'protected-file and implementation allowlist policy', + 'changed behavior has focused tests', + 'packages/*/src/', + '.changeset/*.md', + ]) { + expect(REVIEWER).toContain(requirement); + } + }); + + it('strict-compiles to read-only agent permissions and only advisory write handlers', () => { + const { lock, safeOutputs } = compileReviewer(); + const agentJob = lock.match(/^ agent:\n([\s\S]*?)(?=^ [\w-]+:\n)/m)?.[1] ?? ''; + const permissionBlock = yamlBlock(agentJob, 'permissions'); + + expect(permissionBlock).toContain('contents: read'); + expect(permissionBlock).toContain('issues: read'); + expect(permissionBlock).toContain('pull-requests: read'); + expect(permissionBlock).toContain('copilot-requests: write'); + expect(permissionBlock).not.toMatch(/^\s+(contents|issues|pull-requests): write$/m); + expect(safeOutputs.add_comment).toMatchObject({ max: 1 }); + expect(safeOutputs.create_pull_request_review_comment).toMatchObject({ max: 10 }); + expect(safeOutputs.submit_pull_request_review).toMatchObject({ + max: 1, + allowed_events: ['COMMENT', 'REQUEST_CHANGES'], + }); + expect(safeOutputs).not.toHaveProperty('dispatch_workflow'); + expect(safeOutputs).not.toHaveProperty('create_issue'); + expect(safeOutputs).not.toHaveProperty('create_pull_request'); + expect(lock).toContain('GH_AW_HEAD_SHA: ${{ github.event.pull_request.head.sha }}'); + }, 30000); + + it('kills mutations of every important authority and provenance gate', () => { + const mutations = [ + REVIEWER.replace('tools:\n bash:', 'tools:\n edit:\n bash:'), + REVIEWER.replace('safe-outputs:\n add-comment:', 'safe-outputs:\n dispatch-workflow:\n max: 1\n add-comment:'), + REVIEWER.replace('allowed-events: [COMMENT, REQUEST_CHANGES]', 'allowed-events: [COMMENT, APPROVE]'), + REVIEWER.replace('cancel-in-progress: true', 'cancel-in-progress: false'), + REVIEWER.replace( + 'github.event.pull_request.head.repo.full_name == github.repository', + 'github.event.pull_request.head.repo.full_name != github.repository', + ), + REVIEWER.replace( + '| 1 | One validated durable worker marker | Squad-authored |', + '| 1 | `squad/implement-*` head branch with no marker-like text | Squad-authored fallback |', + ), + REVIEWER.replace('invalid provenance. Fail closed with', 'invalid provenance. Continue with'), + REVIEWER.replace('`Unattributed` is an automatic-review', '`Unattributed` is an automatic'), + REVIEWER.replaceAll('Squad-Review-Head: {40-character lowercase head SHA}', 'Reviewed head SHA'), + ]; + + for (const mutation of mutations) { + expect(() => assertReviewerContract(mutation)).toThrow(); + } + }); +}); diff --git a/workflows/squad-review.md b/workflows/squad-review.md new file mode 100644 index 000000000..6b9b9b90e --- /dev/null +++ b/workflows/squad-review.md @@ -0,0 +1,167 @@ +--- +name: Squad Review +run-name: "Squad review — PR #${{ github.event.inputs.issue_number || github.event.pull_request.number }}" +description: Independently reviews agent-authored pull requests without editing or remediating +private: false +on: + workflow_dispatch: + inputs: + issue_number: + description: Pull request number to review + required: true + type: string + expected_head_sha: + description: Pull request head SHA observed by the /squad review relay + required: true + type: string + request_origin: + description: Review request origin + required: true + type: string + pull_request: + types: [ready_for_review, synchronize] +if: >- + github.event_name == 'workflow_dispatch' || + (github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository && + (contains(github.event.pull_request.body, '$`. +Normalize CRLF to LF before testing lines. Marker-like text means any occurrence +of `squad:implement` anywhere in the body. + +For priority 1, require exactly one marker-like occurrence, exactly one exact +marker line, and a head ref matching +`^squad/implement-{captured-issue}-`. Missing, duplicated, embedded, malformed, +or branch-mismatched marker evidence is invalid provenance. Fail closed with +`noop`; do not fall back to branch or Copilot attribution. + +For automatic `pull_request` runs, `Unattributed` is an automatic-review +refusal: call `noop` and stop without a comment or review. For the manual path, +continue with an `Unattributed (manual)` classification; manual review is +allowed on any non-fork pull request, but malformed marker text remains +untrusted and must not be presented as durable provenance. + +## SHA deduplication + +Fetch all existing reviews on the pull request before analyzing the diff. The +machine-readable review marker is a standalone final line: + +`Squad-Review-Head: {40-character lowercase head SHA}` + +If an existing review body contains the exact marker for the current head SHA, +call `noop` and stop. Never re-review an unchanged head, including duplicate +`synchronize` deliveries. Re-fetch the pull request immediately before emitting +outputs; if its head SHA changed, call `noop` and let the newer run review it. + +## Review procedure + +1. Fetch the complete pull request metadata, changed-file list, diff, existing + review comments, and checks. Review changed lines only and avoid duplicate + findings. +2. Resolve linked issues from the validated worker issue number first, then + GitHub closing references in the pull request body. Read each linked issue's + acceptance criteria. Missing or ambiguous linkage is a review finding, not + permission to invent acceptance criteria. +3. Read the repository's current instruction files, `.squad/routing.md`, + `.squad/team.md`, and the charter named by any `squad:{member}` issue label. + Compare changed paths and work type with the expected specialist domain. +4. Read applicable protected-file and implementation allowlist policy. Flag + edits to protected or excluded paths and changes outside the allowed scope. +5. Check that changed behavior has focused tests, including negative and edge + cases for authority or provenance gates. +6. If any file under `packages/*/src/` changed, require an appropriate + `.changeset/*.md`. Do not require a changeset otherwise. +7. Add at most ten high-signal inline review comments on changed lines. Do not + post style-only nits or comments that merely repeat automated checks. + +## Verdict + +Submit exactly one review: + +- `REQUEST_CHANGES` for an acceptance-criteria violation, unsafe authority + expansion, protected-file/allowlist violation, missing tests for changed + behavior, required-but-missing changeset, or another concrete merge blocker. +- `COMMENT` when findings are advisory or no merge blocker is established. +- Never use `APPROVE`. Human approval remains mandatory. + +Keep the body concise. State the provenance classification, linked issue scope, +and blocking themes. End with these two standalone lines, substituting the +reviewed SHA: + +`Human approval remains mandatory.` + +`Squad-Review-Head: {40-character lowercase head SHA}` diff --git a/workflows/squad.md b/workflows/squad.md index 77b18c292..ad70885af 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -12,6 +12,7 @@ on: events: - issues - issue_comment + - pull_request_comment - pull_request_review_comment workflow_dispatch: inputs: @@ -105,7 +106,7 @@ safe-outputs: max: 20 target: "*" dispatch-workflow: - workflows: [squad-implement-worker] + workflows: [squad-implement-worker, squad-review] max: 3 --- @@ -220,6 +221,7 @@ Repository owners must configure Copilot setup steps separately when needed. | `/squad cast-member ` | Cast Member | | `/squad retire ` | Retire | | `/squad status` | Status | +| `/squad review` | Review Relay | | `/squad research` | Research | | `/squad plan` | Plan | | `/squad plan revise ` | Plan Revise | @@ -375,7 +377,7 @@ requested. First-token-wins is the contract; keep extraction anchored to 3. Otherwise match **longest-prefix-first**: - `plan accept implementation` (3), `plan accept scope` (3), `plan program revise` (3) - `plan implementation` (2), `plan program` (2), `plan activate` (2), `plan validate` (2), `plan accept` (2), `plan revise` (2), `triage revise` (2) - - `cast-member` (1), `plan` (1), `cast`, `connect`, `adopt`, `retire`, `status`, `research`, `triage`, `implement` + - `cast-member` (1), `plan` (1), `cast`, `connect`, `adopt`, `retire`, `status`, `review`, `research`, `triage`, `implement` 4. No prefix matches → go to **Step PC-3**. 5. **Phase selector:** If remaining args contain `phase {N}`, extract N. @@ -429,7 +431,7 @@ Assign the parsed mode string from **Step PC-2** to `SQUAD_PARSED_MODE` and run ```bash mode="${SQUAD_PARSED_MODE-}" case "$mode" in - status|research|plan) + status|review|research|plan) echo READ_ONLY ;; *) @@ -441,7 +443,11 @@ 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. +**Open-mode allow-list:** `status`, `review` (advisory relay), `research`, and +`plan` (plan preview). 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. ### Step AG-2: Resolve actor permission [MANDATORY for `AUTH_REQUIRED`] @@ -517,6 +523,7 @@ Each mode's playbook ships as a **skill**. Enter this section only after **Actor | `cast-member` | `squad-cast-member` | | `retire` | `squad-retire` | | `status` | `squad-status` | +| `review` | `squad-review-relay` | | `research` | `squad-research` | | `plan` | `squad-plan` | | `plan revise` | `squad-plan-revise` | @@ -532,7 +539,7 @@ Each mode's playbook ships as a **skill**. Enter this section only after **Actor | `plan activate` | `squad-plan-activate` | | `implement` | `squad-implement` | -**Planning modes only** — before running the mode skill, also load `squad-planning-policy` (policy resolution) and `squad-planning-ontology` (artifact schemas and the lifecycle state machine). The non-planning modes (Cast, Connect, Adopt, Cast Member, Retire, Status, Implement) must not load them. +**Planning modes only** — before running the mode skill, also load `squad-planning-policy` (policy resolution) and `squad-planning-ontology` (artifact schemas and the lifecycle state machine). The non-planning modes (Cast, Connect, Adopt, Cast Member, Retire, Status, Review Relay, Implement) must not load them. If the parsed mode's skill cannot be loaded, report the failure in plain language and stop. Never improvise a mode playbook from memory. @@ -541,7 +548,7 @@ If the parsed mode's skill cannot be loaded, report the failure in plain languag ## Team Guard **Applies to:** Research, Triage, Plan, Plan Program, Plan Implementation, Plan Validate, Plan Revise, Triage Revise, Plan Accept, Plan Accept Scope, Plan Accept Implementation, Plan Activate. -**Exempt:** Cast, Connect, Adopt, Cast Member, Retire, Status, Implement (these run their own pre-checks). +**Exempt:** Cast, Connect, Adopt, Cast Member, Retire, Status, Review Relay, Implement (these run their own pre-checks). ### Step TG-1: Check Team Presence @@ -740,6 +747,35 @@ Create `meet-the-squad.md` at repo root with: title, universe name, team table ( `add-comment`: `🧑‍🤝‍🧑 Your Squad is ready for review.\n\n**PR:** #{pr_number}\n\nMerge the PR to activate your team. Run /squad status afterward to verify.` +## skill: `squad-review-relay` +--- +description: Relay `/squad review` on a pull request to the independent reviewer. +--- + +This mode is only valid from a pull request comment or pull request review +comment. Resolve the pull request number and current 40-character lowercase +head SHA from GitHub's API, not from user text. If either cannot be established, +post one `add-comment` explaining that `/squad review` must target a pull +request, then stop without dispatching. + +Use only the typed `dispatch-workflow` safe-output. Never call the generic +`dispatch_workflow` tool. Emit exactly one dispatch: + +```json +{ + "workflow_name": "squad-review", + "inputs": { + "issue_number": "{pull-request-number}", + "expected_head_sha": "{current-head-sha}", + "request_origin": "manual" + } +} +``` + +Do not review the diff in this router, emit a verdict, edit files, create an +issue, or dispatch any other workflow. The independent reviewer owns all +provenance, deduplication, and review decisions. + ## skill: `squad-connect` --- description: Connect an existing external team source into Squad.