From 63cfbf09c368b40c4291a3155f6514dd38ef03e3 Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Wed, 30 Sep 2026 22:07:46 -0700 Subject: [PATCH 1/3] fix(gh-aw): harden hosted canary contracts Validate bootstrap output in isolation before trusted writes, authorize mutating command dispatches, and bind review relays to the latest exact base-controlled run attempt. Closes #2103 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .changeset/secure-gh-aw-canary.md | 6 + .../skills/gh-aw-enlistment/SKILL.md | 7 +- .../skills/gh-aw-enlistment/SKILL.md | 7 +- .../skills/gh-aw-enlistment/SKILL.md | 7 +- scripts/gh-aw-hosted-e2e.mjs | 2 +- templates/skills/gh-aw-enlistment/SKILL.md | 7 +- test/gh-aw-bootstrap-workflow.test.ts | 26 ++++ test/gh-aw-command-parse.test.ts | 34 ++++- test/gh-aw-hosted-e2e.test.ts | 12 +- test/gh-aw-quality.test.ts | 6 +- test/gh-aw-research-online.test.ts | 4 +- test/gh-aw-review-guard.test.ts | 64 +++++++-- test/gh-aw-review-workflow.test.ts | 15 +++ workflows/package/squad-bootstrap.md | 71 ++++++++-- workflows/package/squad-command-router.md | 22 +++- workflows/package/squad-implement-worker.md | 2 + workflows/package/squad.md | 42 +++--- workflows/shared/squad-command-contract.mjs | 50 +++++-- workflows/shared/squad-planning-ontology.md | 42 +++--- workflows/shared/squad-review-guard.mjs | 123 ++++++++++++------ workflows/skills/gh-aw-enlistment/SKILL.md | 7 +- workflows/squad-bootstrap.md | 71 ++++++++-- workflows/squad-command-router.md | 22 +++- workflows/squad-implement-worker.md | 2 + workflows/squad-workflows.manifest.json | 32 ++--- 25 files changed, 518 insertions(+), 165 deletions(-) create mode 100644 .changeset/secure-gh-aw-canary.md diff --git a/.changeset/secure-gh-aw-canary.md b/.changeset/secure-gh-aw-canary.md new file mode 100644 index 000000000..04bec1f39 --- /dev/null +++ b/.changeset/secure-gh-aw-canary.md @@ -0,0 +1,6 @@ +--- +"@bradygaster/squad-cli": patch +"@bradygaster/squad-sdk": patch +--- + +Harden Squad workflow bootstrap, command routing, review relays, and installed workflow guidance for deterministic hosted canaries. diff --git a/.squad-templates/skills/gh-aw-enlistment/SKILL.md b/.squad-templates/skills/gh-aw-enlistment/SKILL.md index 15a62147e..7ac923375 100644 --- a/.squad-templates/skills/gh-aw-enlistment/SKILL.md +++ b/.squad-templates/skills/gh-aw-enlistment/SKILL.md @@ -389,7 +389,7 @@ gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}" # Safe-update report shows ONLY the two documented secrets + squad-init → approve once gh aw compile --strict --approve node .github/workflows/shared/squad-install-verifier.mjs --materialize-runtime -gh aw compile --strict # final, no --approve; only the bot-trigger warning remains +gh aw compile --strict # final, no --approve; exactly the two documented warnings remain node .github/workflows/shared/squad-install-verifier.mjs \ --verify-install --source-revision "${SQUAD_SHA}" --strict-compile @@ -446,8 +446,9 @@ gh pr merge --squash # auto-merge before human review. NEVER. else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). -- ❌ **Tolerating extra warnings.** Only the `squad.md` bot-trigger warning is - allowed; every other warning or error halts the run. +- ❌ **Tolerating extra warnings.** Only the documented `squad-review.md` + `pull_request_target` advisory and `squad.md` bot-trigger warning are allowed; + every other warning or error halts the run. - ❌ **Committing diagnostics.** Never commit `.github/aw/logs/` output; add the log `.gitignore` if missing. - ❌ **Clobbering existing workflows.** The install is additive; preserve unrelated diff --git a/packages/squad-cli/templates/skills/gh-aw-enlistment/SKILL.md b/packages/squad-cli/templates/skills/gh-aw-enlistment/SKILL.md index 15a62147e..7ac923375 100644 --- a/packages/squad-cli/templates/skills/gh-aw-enlistment/SKILL.md +++ b/packages/squad-cli/templates/skills/gh-aw-enlistment/SKILL.md @@ -389,7 +389,7 @@ gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}" # Safe-update report shows ONLY the two documented secrets + squad-init → approve once gh aw compile --strict --approve node .github/workflows/shared/squad-install-verifier.mjs --materialize-runtime -gh aw compile --strict # final, no --approve; only the bot-trigger warning remains +gh aw compile --strict # final, no --approve; exactly the two documented warnings remain node .github/workflows/shared/squad-install-verifier.mjs \ --verify-install --source-revision "${SQUAD_SHA}" --strict-compile @@ -446,8 +446,9 @@ gh pr merge --squash # auto-merge before human review. NEVER. else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). -- ❌ **Tolerating extra warnings.** Only the `squad.md` bot-trigger warning is - allowed; every other warning or error halts the run. +- ❌ **Tolerating extra warnings.** Only the documented `squad-review.md` + `pull_request_target` advisory and `squad.md` bot-trigger warning are allowed; + every other warning or error halts the run. - ❌ **Committing diagnostics.** Never commit `.github/aw/logs/` output; add the log `.gitignore` if missing. - ❌ **Clobbering existing workflows.** The install is additive; preserve unrelated diff --git a/packages/squad-sdk/templates/skills/gh-aw-enlistment/SKILL.md b/packages/squad-sdk/templates/skills/gh-aw-enlistment/SKILL.md index 15a62147e..7ac923375 100644 --- a/packages/squad-sdk/templates/skills/gh-aw-enlistment/SKILL.md +++ b/packages/squad-sdk/templates/skills/gh-aw-enlistment/SKILL.md @@ -389,7 +389,7 @@ gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}" # Safe-update report shows ONLY the two documented secrets + squad-init → approve once gh aw compile --strict --approve node .github/workflows/shared/squad-install-verifier.mjs --materialize-runtime -gh aw compile --strict # final, no --approve; only the bot-trigger warning remains +gh aw compile --strict # final, no --approve; exactly the two documented warnings remain node .github/workflows/shared/squad-install-verifier.mjs \ --verify-install --source-revision "${SQUAD_SHA}" --strict-compile @@ -446,8 +446,9 @@ gh pr merge --squash # auto-merge before human review. NEVER. else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). -- ❌ **Tolerating extra warnings.** Only the `squad.md` bot-trigger warning is - allowed; every other warning or error halts the run. +- ❌ **Tolerating extra warnings.** Only the documented `squad-review.md` + `pull_request_target` advisory and `squad.md` bot-trigger warning are allowed; + every other warning or error halts the run. - ❌ **Committing diagnostics.** Never commit `.github/aw/logs/` output; add the log `.gitignore` if missing. - ❌ **Clobbering existing workflows.** The install is additive; preserve unrelated diff --git a/scripts/gh-aw-hosted-e2e.mjs b/scripts/gh-aw-hosted-e2e.mjs index 7d418532b..bd0140ac4 100644 --- a/scripts/gh-aw-hosted-e2e.mjs +++ b/scripts/gh-aw-hosted-e2e.mjs @@ -611,7 +611,7 @@ export function waitForBaseControlledReviewCanary( ]); if (run.event !== 'pull_request_target' || run.path !== '.github/workflows/squad-review.lock.yml' - || run.head_sha !== castPr.headSha + || run.head_sha !== castPr.baseSha || run.repository?.full_name !== target || run.display_title !== `Squad review — PR #${castPr.number}` || run.run_attempt < verdict.run_attempt) { diff --git a/templates/skills/gh-aw-enlistment/SKILL.md b/templates/skills/gh-aw-enlistment/SKILL.md index 15a62147e..7ac923375 100644 --- a/templates/skills/gh-aw-enlistment/SKILL.md +++ b/templates/skills/gh-aw-enlistment/SKILL.md @@ -389,7 +389,7 @@ gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}" # Safe-update report shows ONLY the two documented secrets + squad-init → approve once gh aw compile --strict --approve node .github/workflows/shared/squad-install-verifier.mjs --materialize-runtime -gh aw compile --strict # final, no --approve; only the bot-trigger warning remains +gh aw compile --strict # final, no --approve; exactly the two documented warnings remain node .github/workflows/shared/squad-install-verifier.mjs \ --verify-install --source-revision "${SQUAD_SHA}" --strict-compile @@ -446,8 +446,9 @@ gh pr merge --squash # auto-merge before human review. NEVER. else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). -- ❌ **Tolerating extra warnings.** Only the `squad.md` bot-trigger warning is - allowed; every other warning or error halts the run. +- ❌ **Tolerating extra warnings.** Only the documented `squad-review.md` + `pull_request_target` advisory and `squad.md` bot-trigger warning are allowed; + every other warning or error halts the run. - ❌ **Committing diagnostics.** Never commit `.github/aw/logs/` output; add the log `.gitignore` if missing. - ❌ **Clobbering existing workflows.** The install is additive; preserve unrelated diff --git a/test/gh-aw-bootstrap-workflow.test.ts b/test/gh-aw-bootstrap-workflow.test.ts index c1fa5b053..b88472327 100644 --- a/test/gh-aw-bootstrap-workflow.test.ts +++ b/test/gh-aw-bootstrap-workflow.test.ts @@ -848,6 +848,10 @@ describe('automatic Squad bootstrap workflow', () => { expect(lock).toContain('"payload_sha256"'); expect(lock).not.toMatch(/"materialize-bootstrap":\{"inputs":\{"payload":/); expect(lock).toContain('reconstructBootstrapPayload(items[0])'); + expect(lock).toContain("mkdtempSync(join(tmpdir(), 'squad-bootstrap-candidate-'))"); + expect(lock).toContain('Bootstrap payload path crosses a symbolic link'); + expect(lock.indexOf("validate(payload, 'placeholder')")) + .toBeLessThan(lock.indexOf('safeTarget(checkout, file.path)')); expect(lock).toContain('mcp-cli'); expect(lock).toContain('--submit-envelope'); expect(parse(lock).jobs.agent.env.DEFAULT_BRANCH).toBe('${{ github.event.repository.default_branch }}'); @@ -865,6 +869,28 @@ describe('automatic Squad bootstrap workflow', () => { expect(lock).not.toMatch(/\$\{\{[^}]*\\u00(?:26|3[cCeE])/); }, 180000); + it('rejects a realistic source mutation that writes to the checkout before validation', () => { + const assertValidationBeforeCheckoutWrite = (source: string) => { + const validation = source.indexOf("validate(payload, 'placeholder')"); + const candidateWrite = source.indexOf( + "const target = safeTarget(candidate, file.path)", + ); + const checkoutWrite = source.indexOf( + "const target = safeTarget(checkout, file.path)", + ); + expect(candidateWrite).toBeGreaterThan(-1); + expect(candidateWrite).toBeLessThan(validation); + expect(validation).toBeGreaterThan(-1); + expect(checkoutWrite).toBeGreaterThan(validation); + }; + assertValidationBeforeCheckoutWrite(WORKFLOW); + const mutated = WORKFLOW.replace( + "const target = safeTarget(candidate, file.path);", + "const target = safeTarget(checkout, file.path);", + ); + expect(() => assertValidationBeforeCheckoutWrite(mutated)).toThrow(); + }); + it('detects a realistic compiled-lock mutation that removes the default-branch gate', () => { const mutated = WORKFLOW.replace( "if: github.ref_name == github.event.repository.default_branch\n", diff --git a/test/gh-aw-command-parse.test.ts b/test/gh-aw-command-parse.test.ts index 062cbb06b..5086b6546 100644 --- a/test/gh-aw-command-parse.test.ts +++ b/test/gh-aw-command-parse.test.ts @@ -4,7 +4,9 @@ import { pathToFileURL } from 'node:url'; import { join } from 'node:path'; import { classifySquadCommand, + commandRequiresAuthorization, enforceSquadCommandContract, + isAuthorizedPermission, rejectionComment, } from '../workflows/shared/squad-command-contract.mjs'; @@ -38,7 +40,6 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { ['/squad cast', 'cast', null], [' /squad cast ', 'cast', null], ['Context first.\n\n/squad status ', 'status', null], - ['Please run /squad plan implementation', 'plan implementation', null], ['/squad research Focus only on proposal P1', 'research', null], ['/squad activate phase 2', 'activate', 2], ['/squad plan accept implementation phase 12', 'plan accept implementation', 12], @@ -82,6 +83,18 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { }); }, ); + + it.each([ + ['Please run /squad plan implementation', 'embedded prose'], + ['Use `/squad cast` after review.', 'inline code'], + ['```text\n/squad cast\n```', 'fenced code'], + ['~~~text\n/squad cast\n~~~', 'tilde-fenced code'], + ])('ignores %s instead of activating control-plane work', (body) => { + expect(classifySquadCommand(payload(body), 'issues')).toEqual({ + status: 'none', + source: _name === 'issue body' ? 'issue' : 'comment', + }); + }); }); describe('workflow_dispatch call site', () => { @@ -186,6 +199,10 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { expect(DISCOVERY_WORKFLOW).toContain("startsWith(github.event.comment.body, '/squad ')"); expect(DISCOVERY_WORKFLOW).toContain('squad-command-contract.mjs'); expect(DISCOVERY_WORKFLOW).toContain('classifySquadCommand'); + expect(DISCOVERY_WORKFLOW).toContain('commandRequiresAuthorization'); + expect(DISCOVERY_WORKFLOW).toContain('getCollaboratorPermissionLevel'); + expect(DISCOVERY_WORKFLOW).toContain('isAuthorizedPermission'); + expect(DISCOVERY_WORKFLOW).toContain('No standalone Squad command was found'); expect(DISCOVERY_WORKFLOW).toContain('rejectionComment'); expect(DISCOVERY_WORKFLOW).toContain('github.rest.issues.createComment'); expect(DISCOVERY_WORKFLOW).toContain('core.setFailed(`Squad rejected command:'); @@ -194,6 +211,21 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { expect(DISCOVERY_WORKFLOW).toContain("command: result.argumentText || 'cast'"); }); + it('keeps open modes public and fails mutating modes closed on collaborator permission', () => { + for (const mode of ['status', 'review', 'research', 'plan', 'revoke-improvement']) { + expect(commandRequiresAuthorization({ status: 'accepted', mode })).toBe(false); + } + for (const mode of ['cast', 'triage', 'plan implementation', 'implement']) { + expect(commandRequiresAuthorization({ status: 'accepted', mode })).toBe(true); + } + expect(isAuthorizedPermission('admin')).toBe(true); + expect(isAuthorizedPermission('maintain')).toBe(true); + expect(isAuthorizedPermission('write')).toBe(true); + expect(isAuthorizedPermission('triage')).toBe(false); + expect(isAuthorizedPermission('read')).toBe(false); + expect(isAuthorizedPermission('unresolved')).toBe(false); + }); + it('realistic source mutation is caught by the accepted-command fixtures', async () => { mkdirSync(TEST_ROOT, { recursive: true }); const root = mkdtempSync(join(TEST_ROOT, 'command-contract-mutant-')); diff --git a/test/gh-aw-hosted-e2e.test.ts b/test/gh-aw-hosted-e2e.test.ts index 49c3df3af..2e6df056f 100644 --- a/test/gh-aw-hosted-e2e.test.ts +++ b/test/gh-aw-hosted-e2e.test.ts @@ -245,7 +245,7 @@ describe('Squad gh-aw hosted E2E controller', () => { it.each([ 'native-job-name', 'ui-context-name', 'wrong-check-id', 'wrong-head', - 'failed-check', 'incomplete-check', 'foreign-app', 'foreign-app-id', + 'wrong-run-head', 'failed-check', 'incomplete-check', 'foreign-app', 'foreign-app-id', ])('exercises the hosted canary API contract: %s', mutation => { const target = 'owner/consumer'; const verdict = { @@ -257,7 +257,8 @@ describe('Squad gh-aw hosted E2E controller', () => { workflow_path: '.github/workflows/squad-review.lock.yml', run_id: 31, run_attempt: 1, }; const run = { - id: 31, event: verdict.event, path: verdict.workflow_path, head_sha: CAST_SHA, + id: 31, event: verdict.event, path: verdict.workflow_path, + head_sha: currentCastPr.baseSha, repository: { full_name: target }, run_attempt: 1, display_title: `Squad review — PR #${currentCastPr.number}`, }; @@ -273,6 +274,7 @@ describe('Squad gh-aw hosted E2E controller', () => { case 'ui-context-name': check.name = 'Squad Review / review'; break; case 'wrong-check-id': check.id = 124; break; case 'wrong-head': check.head_sha = TARGET_SHA; break; + case 'wrong-run-head': run.head_sha = CAST_SHA; break; case 'failed-check': check.conclusion = 'failure'; break; case 'incomplete-check': check.status = 'in_progress'; break; case 'foreign-app': check.app.slug = 'other'; break; @@ -304,10 +306,14 @@ describe('Squad gh-aw hosted E2E controller', () => { expect(JSON.parse(readFileSync( resolve(evidence, 'base-controlled-review-canary.json'), 'utf8', )).check.name).toBe('review'); + } else if (mutation === 'wrong-run-head') { + expect(execute).toThrow('not bound to the base-controlled workflow run'); } else { expect(execute).toThrow('not bound to the trusted review job'); } - expect(routes.at(-1)).toBe(`repos/${target}/check-runs/123`); + expect(routes.at(-1)).toBe(mutation === 'wrong-run-head' + ? `repos/${target}/actions/runs/31` + : `repos/${target}/check-runs/123`); } finally { rmSync(evidence, { recursive: true, force: true }); } diff --git a/test/gh-aw-quality.test.ts b/test/gh-aw-quality.test.ts index 28c4be0e6..3743496a4 100644 --- a/test/gh-aw-quality.test.ts +++ b/test/gh-aw-quality.test.ts @@ -3961,8 +3961,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' - ? '1aa070ca330d8f8f9f874855c75db410851b30a38efc7543090b697352eda1d2' - : 'bf54175720e6e9ab3fe5d5e3054c28f3a69595c95cdf50e56f9b30988158270f'); + ? '80744f1a2c73bcffce57d53df39da71486556c1da0516574b0e974fc3a2090dc' + : '65e307c7f9a796c9a9f26556773d23f2daa90e93294a47f319821fd99b1ebfdd'); expect(() => validateCompilerActionPins(mutable)).toThrow(/invalid immutable action pin/); writeFileSync(lockPath, mutable); expect(verifyInstall(root).failures.join('\n')) @@ -4016,7 +4016,7 @@ describe('gh-aw: canonical package integrity contract', () => { compile(); const unpinned = readText(lockPath); expect(createHash('sha256').update(normalizeCompiledLock(unpinned, revisionA)).digest('hex')) - .toBe('bf54175720e6e9ab3fe5d5e3054c28f3a69595c95cdf50e56f9b30988158270f'); + .toBe('65e307c7f9a796c9a9f26556773d23f2daa90e93294a47f319821fd99b1ebfdd'); 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-research-online.test.ts b/test/gh-aw-research-online.test.ts index 21d4c4253..0fad9d402 100644 --- a/test/gh-aw-research-online.test.ts +++ b/test/gh-aw-research-online.test.ts @@ -197,8 +197,8 @@ describe('gh-aw: /squad research online-documentation capability', () => { it('mirrors the Online sources disclosure into the §3.2 research template of the planning ontology', () => { const ontology = readText(PLANNING_ONTOLOGY); - const start = ontology.indexOf('### 3.2 Research Findings'); - expect(start, '§3.2 Research Findings template must exist in the planning ontology').toBeGreaterThan(-1); + const start = ontology.indexOf('### 3.2 Squad Research'); + expect(start, '§3.2 Squad Research template must exist in the planning ontology').toBeGreaterThan(-1); // Scope to the §3.2 template block (up to the next ### heading) so a stray // match elsewhere in the file can't satisfy this. const rest = ontology.slice(start); diff --git a/test/gh-aw-review-guard.test.ts b/test/gh-aw-review-guard.test.ts index 4535d13c2..9ad6ceddf 100644 --- a/test/gh-aw-review-guard.test.ts +++ b/test/gh-aw-review-guard.test.ts @@ -27,7 +27,8 @@ afterEach(() => { function fixture(relay = false) { vi.spyOn(Date, 'now').mockReturnValue(NOW); const env = { - GITHUB_EVENT_NAME: 'pull_request_target', GITHUB_REPOSITORY: REPOSITORY, + GITHUB_EVENT_NAME: relay ? 'pull_request' : 'pull_request_target', + GITHUB_REPOSITORY: REPOSITORY, GITHUB_SERVER_URL: 'https://github.com', GITHUB_RUN_ID: '17', GITHUB_RUN_ATTEMPT: '1', SQUAD_REVIEW_PR: '42', SQUAD_REVIEW_HEAD: HEAD, @@ -65,9 +66,10 @@ function fixture(relay = false) { body: '', }; const run = { + id: 17, event: 'pull_request_target', path: '.github/workflows/squad-review.lock.yml', display_title: 'Squad review \u2014 PR #42', - repository: { full_name: REPOSITORY }, head_sha: HEAD, run_attempt: 1, + repository: { full_name: REPOSITORY }, head_sha: BASE, run_attempt: 1, pull_requests: [{ number: 42, head: { sha: HEAD }, base: { repo: { name: 'example' } } }], run_started_at: START, updated_at: FINISHED, status: 'completed', conclusion: 'success', }; @@ -94,6 +96,9 @@ function fixture(relay = false) { const state = { reviews: [review], comments: [] as typeof comment[], permission: 'admin', missingManifest: false, calls: [] as string[], prReads: 0, changeAfterFirstRead: false, + workflowRuns: [run], + attempts: new Map([[1, run]]), + attemptJobs: new Map([[1, jobs]]), }; const encode = (value: unknown) => { const content = Buffer.from(JSON.stringify(value)); @@ -124,10 +129,15 @@ function fixture(relay = false) { return encode(registry); } if (route.endsWith('/reviews')) return state.reviews; + if (route.endsWith('/actions/workflows/squad-review.lock.yml/runs')) { + return { workflow_runs: state.workflowRuns }; + } if (route.endsWith('/actions/runs/17')) return run; if (route.endsWith('/actions/runs/29')) return bootstrapRun; - if (route.endsWith('/attempts/1')) return { ...run, run_attempt: 1, run_started_at: START }; - if (/\/attempts\/[12]\/jobs$/.test(route)) return { jobs }; + const attempt = route.match(/\/actions\/runs\/\d+\/attempts\/(\d+)$/); + if (attempt) return state.attempts.get(Number(attempt[1])); + const attemptJobs = route.match(/\/actions\/runs\/\d+\/attempts\/(\d+)\/jobs$/); + if (attemptJobs) return { jobs: state.attemptJobs.get(Number(attemptJobs[1])) ?? [] }; if (route.endsWith('/comments')) return state.comments; if (route.endsWith('/permission')) return { permission: state.permission }; throw new Error(`Unexpected API route: ${route}`); @@ -168,6 +178,8 @@ function makeBootstrap(f: ReturnType) { } function retainFirstAttempt(f: ReturnType) { + const firstAttempt = structuredClone(f.run); + const firstJobs = structuredClone(f.jobs); const first = structuredClone(f.review); first.body = `${VERDICT_PREFIX}${JSON.stringify({ ...f.verdict, result: 'REQUEST_CHANGES' })}`; f.review.id = 124; @@ -179,6 +191,10 @@ function retainFirstAttempt(f: ReturnType) { f.run.updated_at = '2026-09-28T12:02:30Z'; f.jobs[0].started_at = FINISHED; f.jobs[0].completed_at = f.run.updated_at; + f.state.attempts.set(1, firstAttempt); + f.state.attempts.set(2, f.run); + f.state.attemptJobs.set(1, firstJobs); + f.state.attemptJobs.set(2, f.jobs); f.state.reviews = [first, f.review]; f.sync(); return first; @@ -274,6 +290,17 @@ describe('independent Squad review guard', () => { } }); + it('requires the correct event and trusted base workflow SHA on merged relays', async () => { + const f = fixture(true); + f.env.GITHUB_EVENT_NAME = 'pull_request_target'; + await expect(assertClearingReview(f.env, f.get, { relay: true })) + .rejects.toThrow('only merged pull request events'); + f.env.GITHUB_EVENT_NAME = 'pull_request'; + f.env.SQUAD_REVIEW_WORKFLOW_SHA = ''; + await expect(assertClearingReview(f.env, f.get, { relay: true })) + .rejects.toThrow('workflow source is not the exact PR base commit'); + }); + it.each([false, true])('selects a successful rerun with retained attempt-1 rejection (reverse=%s)', async reverse => { const f = fixture(true); retainFirstAttempt(f); @@ -315,10 +342,22 @@ describe('independent Squad review guard', () => { retainFirstAttempt(f); f.verdict.run_id = 18; f.verdict.run_attempt = f.run.run_attempt = 1; + f.run.id = 18; + f.state.attempts = new Map([[1, f.run]]); + f.state.attemptJobs = new Map([[1, f.jobs]]); f.sync(); - const get = (route: string, fields?: Record) => - f.get(route.replace('/actions/runs/18', '/actions/runs/17'), fields); - await expect(assertClearingReview(f.env, get, { relay: true })).resolves.toEqual(f.verdict); + await expect(assertClearingReview(f.env, f.get, { relay: true })).resolves.toEqual(f.verdict); + }); + + it('does not fall back when the latest trusted failed rerun emitted no review', async () => { + const f = fixture(true); + retainFirstAttempt(f); + f.state.reviews = [f.state.reviews[0]]; + f.state.attempts.get(2)!.conclusion = 'failure'; + f.state.attemptJobs.get(2)![0].conclusion = 'failure'; + await expect(assertClearingReview(f.env, f.get, { relay: true })) + .rejects.toThrow('missing or duplicate verdict evidence'); + expect(f.state.calls).not.toContain(`repos/${REPOSITORY}/actions/runs/17/attempts/1/jobs`); }); it('rejects duplicate verdicts for the selected attempt even with distinct review IDs', async () => { @@ -344,7 +383,7 @@ describe('independent Squad review guard', () => { case 'duplicate-job': f.jobs.push(f.jobs[0]); break; case 'wrong-workflow': f.run.path = '.github/workflows/untrusted.yml'; break; case 'wrong-event': f.run.event = 'workflow_dispatch'; break; - case 'wrong-head': f.run.head_sha = BASE; break; + case 'wrong-head': f.run.head_sha = HEAD; break; case 'replay': f.run.run_started_at = f.run.updated_at; break; case 'after-merge': f.jobs[0].completed_at = '2026-09-28T12:04:00Z'; break; case 'malformed': f.review.body = `${VERDICT_PREFIX}{broken`; break; @@ -360,7 +399,7 @@ describe('independent Squad review guard', () => { f.run.head_sha = HEAD; f.jobs[0].name = 'review'; await expect(assertClearingReview(f.env, f.get, { relay: true })) - .rejects.toThrow('not bound to this PR workflow run'); + .rejects.toThrow('missing trusted workflow run attempt'); }); it.each([ @@ -455,7 +494,7 @@ describe('independent Squad review guard', () => { case 'manual-run': f.run.event = 'workflow_dispatch'; break; case 'wrong-workflow': f.run.path = '.github/workflows/other.yml'; break; case 'wrong-run-repo': f.run.repository.full_name = 'other/repo'; break; - case 'wrong-run-sha': f.run.head_sha = BASE; break; + case 'wrong-run-sha': f.run.head_sha = HEAD; break; case 'wrong-run-pr': f.run.pull_requests[0].number = 43; break; case 'wrong-run-attempt': f.run.run_attempt = 0; break; case 'wrong-run-title': f.run.display_title = 'Squad review \u2014 PR #43'; break; @@ -469,7 +508,7 @@ describe('independent Squad review guard', () => { it('does not accept a manual gate or the merge commit as the reviewed head', async () => { const f = fixture(true); await expect(assertClearingReview({ ...f.env, GITHUB_EVENT_NAME: 'workflow_dispatch' }, f.get, { relay: true })) - .rejects.toThrow('only base-controlled PR target runs'); + .rejects.toThrow('only merged pull request events'); await expect(reviewTarget({ ...f.env, SQUAD_REVIEW_HEAD: BASE }, f.get, { relay: true })) .rejects.toThrow('head changed'); }); @@ -479,7 +518,8 @@ describe('independent Squad review guard', () => { f.run.pull_requests = []; await expect(assertClearingReview(f.env, f.get, { relay: true })).resolves.toEqual(f.verdict); f.run.display_title = ''; - await expect(assertClearingReview(f.env, f.get, { relay: true })).rejects.toThrow('bound'); + await expect(assertClearingReview(f.env, f.get, { relay: true })) + .rejects.toThrow('missing trusted workflow run attempt'); }); it('clears a rejection only through a human-admin SHA-scoped override bound to its run attempt', async () => { diff --git a/test/gh-aw-review-workflow.test.ts b/test/gh-aw-review-workflow.test.ts index c0d52f181..d75c2cd4f 100644 --- a/test/gh-aw-review-workflow.test.ts +++ b/test/gh-aw-review-workflow.test.ts @@ -82,6 +82,19 @@ function assertRelayHeadBinding(workflow: string): void { expect(relay).toContain("the run's `head_sha` is not the workflow source SHA"); } +function assertRelayRuntimeBinding(workflow: string): void { + const frontmatter = workflow.split('---')[1] ?? ''; + const relayBindings = frontmatter.match( + /SQUAD_REVIEW_PR:[\s\S]*?SQUAD_REVIEW_DEFAULT_BRANCH:/g, + ) ?? []; + expect(relayBindings).toHaveLength(2); + for (const binding of relayBindings) { + expect(binding).toContain( + 'SQUAD_REVIEW_WORKFLOW_SHA: ${{ github.event.pull_request.base.sha }}', + ); + } +} + function assertReviewerContract(workflow: string): void { const yaml = frontmatter(workflow); const tools = yamlBlock(yaml, 'tools'); @@ -319,6 +332,8 @@ describe('gh-aw enforcing Squad reviewer', () => { expect(relay).toMatch(/no base-controlled automatic review run exists for\s+the exact head/); assertRelayHeadBinding(ROUTER); assertRelayHeadBinding(read('workflows/package/squad.md')); + assertRelayRuntimeBinding(read('workflows/squad-implement-worker.md')); + assertRelayRuntimeBinding(read('workflows/package/squad-implement-worker.md')); }); it('rejects dispatcher source mutations conflating API run head and workflow source', () => { diff --git a/workflows/package/squad-bootstrap.md b/workflows/package/squad-bootstrap.md index be842e20b..e112632c3 100644 --- a/workflows/package/squad-bootstrap.md +++ b/workflows/package/squad-bootstrap.md @@ -250,8 +250,12 @@ safe-outputs: SQUAD_BOOTSTRAP_RUN_ID: ${{ github.run_id }} with: script: | - const { mkdirSync, readFileSync, writeFileSync } = await import('node:fs'); - const { dirname, join } = await import('node:path'); + const { + cpSync, existsSync, lstatSync, mkdirSync, mkdtempSync, + readFileSync, rmSync, writeFileSync, + } = await import('node:fs'); + const { tmpdir } = await import('node:os'); + const { dirname, isAbsolute, join, relative, resolve } = await import('node:path'); const { pathToFileURL } = await import('node:url'); const checkout = join(process.env.GITHUB_WORKSPACE, 'bootstrap-repo'); @@ -280,19 +284,39 @@ safe-outputs: core.setFailed(`Bootstrap payload transport is invalid: ${error.message}`); return; } - const payloadPath = join(checkout, '.github/workflows/squad-bootstrap-payload.json'); - for (const file of payload.files || []) { - const target = join(checkout, ...String(file.path || '').split('/')); - mkdirSync(dirname(target), { recursive: true }); - writeFileSync(target, String(file.content || '')); - } - writeFileSync(payloadPath, payloadText); + const safeTarget = (root, path) => { + if (typeof path !== 'string' || path.length === 0 || path.includes('\\') || + isAbsolute(path) || path.split('/').some((segment) => !segment || segment === '.' || segment === '..')) { + throw new Error(`Bootstrap payload contains an unsafe path: ${JSON.stringify(path)}`); + } + const target = resolve(root, path); + const within = relative(root, target); + if (!within || within.startsWith('..') || isAbsolute(within)) { + throw new Error(`Bootstrap payload path escapes the candidate tree: ${path}`); + } + let current = root; + for (const segment of path.split('/').slice(0, -1)) { + current = join(current, segment); + if (existsSync(current) && lstatSync(current).isSymbolicLink()) { + throw new Error(`Bootstrap payload path crosses a symbolic link: ${path}`); + } + } + if (existsSync(target) && lstatSync(target).isSymbolicLink()) { + throw new Error(`Bootstrap payload target is a symbolic link: ${path}`); + } + return target; + }; + for (const file of payload.files || []) safeTarget(checkout, file?.path); - const validate = (candidate, linkMode) => { - writeFileSync(payloadPath, `${JSON.stringify(candidate)}\n`); + const candidate = mkdtempSync(join(tmpdir(), 'squad-bootstrap-candidate-')); + const payloadPath = join(checkout, '.github/workflows/squad-bootstrap-payload.json'); + const validate = (candidatePayload, linkMode) => { + const candidatePayloadPath = + join(candidate, '.github/workflows/squad-bootstrap-payload.json'); + writeFileSync(candidatePayloadPath, `${JSON.stringify(candidatePayload)}\n`); const errors = validatorModule.validateBootstrapPayload({ - root: checkout, - payloadPath, + root: candidate, + payloadPath: candidatePayloadPath, repository: context.repo.owner + '/' + context.repo.repo, defaultBranch: process.env.SQUAD_BOOTSTRAP_DEFAULT_BRANCH, linkMode, @@ -301,7 +325,26 @@ safe-outputs: throw new Error(`Squad bootstrap validation failed:\n${errors.map((error) => `- ${error}`).join('\n')}`); } }; - validate(payload, 'placeholder'); + try { + cpSync(checkout, candidate, { + recursive: true, + filter: (source) => relative(checkout, source).split('/')[0] !== '.git', + }); + for (const file of payload.files || []) { + const target = safeTarget(candidate, file.path); + mkdirSync(dirname(target), { recursive: true }); + writeFileSync(target, file.content); + } + validate(payload, 'placeholder'); + for (const file of payload.files) { + const target = safeTarget(checkout, file.path); + mkdirSync(dirname(target), { recursive: true }); + writeFileSync(target, file.content); + } + writeFileSync(payloadPath, payloadText); + } finally { + rmSync(candidate, { recursive: true, force: true }); + } const listState = async () => { const pullRequests = await github.paginate(github.rest.pulls.list, { diff --git a/workflows/package/squad-command-router.md b/workflows/package/squad-command-router.md index 5b430f4fd..f09445927 100644 --- a/workflows/package/squad-command-router.md +++ b/workflows/package/squad-command-router.md @@ -73,9 +73,29 @@ safe-outputs: return; } if (result.status !== 'accepted') { - core.setFailed('Squad command discovery was activated without an invocation.'); + core.info('No standalone Squad command was found after excluding code contexts.'); return; } + if (contract.commandRequiresAuthorization(result)) { + let permission = 'unresolved'; + try { + permission = (await github.rest.repos.getCollaboratorPermissionLevel({ + ...context.repo, + username: context.actor, + })).data.permission || 'unresolved'; + } catch (error) { + core.warning(`Unable to resolve repository permission for ${context.actor}: ${error.message}`); + } + if (!contract.isAuthorizedPermission(permission)) { + await github.rest.issues.createComment({ + ...context.repo, + issue_number: issueNumber, + body: `⛔ /squad ${result.argumentText || 'cast'} was refused for @${context.actor} (repository permission: ${permission}). Mutating /squad modes require write, maintain, or admin repository permission. Ask a repository maintainer to run this command or grant the required access.`, + }); + core.setFailed(`Squad refused mutating mode ${result.mode} for ${context.actor}.`); + return; + } + } await github.rest.actions.createWorkflowDispatch({ ...context.repo, workflow_id: 'squad.lock.yml', diff --git a/workflows/package/squad-implement-worker.md b/workflows/package/squad-implement-worker.md index bd48b0ccc..844391954 100644 --- a/workflows/package/squad-implement-worker.md +++ b/workflows/package/squad-implement-worker.md @@ -331,6 +331,7 @@ safe-outputs: env: SQUAD_REVIEW_PR: ${{ github.event.pull_request.number }} SQUAD_REVIEW_HEAD: ${{ github.event.pull_request.head.sha }} + SQUAD_REVIEW_WORKFLOW_SHA: ${{ github.event.pull_request.base.sha }} SQUAD_REVIEW_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} with: script: | @@ -891,6 +892,7 @@ pre-agent-steps: env: SQUAD_REVIEW_PR: ${{ github.event.pull_request.number }} SQUAD_REVIEW_HEAD: ${{ github.event.pull_request.head.sha }} + SQUAD_REVIEW_WORKFLOW_SHA: ${{ github.event.pull_request.base.sha }} SQUAD_REVIEW_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} with: script: | diff --git a/workflows/package/squad.md b/workflows/package/squad.md index 696099b65..5a489a534 100644 --- a/workflows/package/squad.md +++ b/workflows/package/squad.md @@ -1050,36 +1050,44 @@ The issue body IS the intent. No special format required, but structured intents ``` -### 3.2 Research Findings +### 3.2 Squad Research ```markdown -## Research Findings +## 🔬 Squad Research — ### Summary <1-3 sentence overview of what was discovered> -### Sources -| # | Source | Type | Key Insight | -|---|--------|------|-------------| -| 1 | <link/file/doc> | <codebase/docs/external> | <insight> | +### Goals +- <What this research must establish> + +### Non-goals +- <What is explicitly outside this research> + +### Evidence table +| ID | Finding | Risk | Complexity | Citation | +|----|---------|------|------------|----------| +| R1 | <checkable finding> | <🟢/🟡/🔴> | <S/M/L/XL> | <one path, path:line, URL, or issue/PR reference> | + +### Load-bearing assumptions +- <Assumption referencing the Rn evidence it depends on> + +### Open decisions +- <Decision requiring human judgment> + +### Acceptance framing +- <Measurable evidence that would make the proposed next step acceptable> ### Online sources <`consulted` — list the URLs fetched this run (each also cited above); or `unavailable — <reason>` when no external documentation was fetched. Makes degradation observable: never claim `consulted` for a page not actually fetched.> -### Findings -#### Finding 1: <title> -<Evidence and analysis> - -#### Finding 2: <title> -<Evidence and analysis> - -### Open Questions -- <Unresolved question needing human input> - ### Recommendations -- <Actionable recommendation derived from evidence> +- <Actionable recommendation referencing its Rn evidence> + +### Next step +<`/squad triage` or `/squad plan`> ``` ### 3.3 Triage Disposition diff --git a/workflows/shared/squad-command-contract.mjs b/workflows/shared/squad-command-contract.mjs index a851ecb2c..8faa4a7e9 100644 --- a/workflows/shared/squad-command-contract.mjs +++ b/workflows/shared/squad-command-contract.mjs @@ -43,6 +43,14 @@ const PHASE_COMMANDS = new Map([ ['plan activate', 'plan activate'], ]); +const OPEN_MODES = new Set([ + 'status', + 'review', + 'research', + 'plan', + 'revoke-improvement', +]); + export const VALID_COMMANDS = Object.freeze([ '/squad', '/squad cast', @@ -108,16 +116,38 @@ function extractInvocation(source, text) { }; } - const lower = text.toLowerCase(); - const at = lower.indexOf('/squad'); - if (at === -1) return null; - const line = text.slice(at).split('\n', 1)[0].trimEnd(); - const slash = line.match(/^\/squad(?:\s|$)/); - return { - rejectedCommand: line, - argumentText: slash ? line.slice(slash[0].length).trim() : '', - malformedPrefix: !slash, - }; + let fence = ''; + for (const originalLine of normalizeNewlines(text).split('\n')) { + const fenceMatch = originalLine.match(/^\s*(`{3,}|~{3,})/); + if (fenceMatch) { + const marker = fenceMatch[1][0]; + if (!fence) fence = marker; + else if (fence === marker) fence = ''; + continue; + } + if (fence) continue; + const line = originalLine.replace(/`+[^`]*`+/g, '').trim(); + if (!/^\/squad/i.test(line)) continue; + const slash = line.match(/^\/squad(?:\s|$)/); + const invocation = line; + return { + rejectedCommand: invocation, + argumentText: slash ? invocation.slice(slash[0].length).trim() : '', + malformedPrefix: !slash, + }; + } + return null; +} + +export function commandRequiresAuthorization(result) { + if (result?.status !== 'accepted') { + throw new Error('An accepted command result is required.'); + } + return !OPEN_MODES.has(result.mode); +} + +export function isAuthorizedPermission(permission) { + return ['admin', 'maintain', 'write'].includes(permission); } function accepted(mode, argumentText, phase = null) { diff --git a/workflows/shared/squad-planning-ontology.md b/workflows/shared/squad-planning-ontology.md index c522d3e9d..5b2b738b5 100644 --- a/workflows/shared/squad-planning-ontology.md +++ b/workflows/shared/squad-planning-ontology.md @@ -128,36 +128,44 @@ The issue body IS the intent. No special format required, but structured intents <Links, prior art, relevant decisions> ``` -### 3.2 Research Findings +### 3.2 Squad Research ```markdown -## Research Findings +## 🔬 Squad Research — <Title> ### Summary <1-3 sentence overview of what was discovered> -### Sources -| # | Source | Type | Key Insight | -|---|--------|------|-------------| -| 1 | <link/file/doc> | <codebase/docs/external> | <insight> | +### Goals +- <What this research must establish> + +### Non-goals +- <What is explicitly outside this research> + +### Evidence table +| ID | Finding | Risk | Complexity | Citation | +|----|---------|------|------------|----------| +| R1 | <checkable finding> | <🟢/🟡/🔴> | <S/M/L/XL> | <one path, path:line, URL, or issue/PR reference> | + +### Load-bearing assumptions +- <Assumption referencing the Rn evidence it depends on> + +### Open decisions +- <Decision requiring human judgment> + +### Acceptance framing +- <Measurable evidence that would make the proposed next step acceptable> ### Online sources <`consulted` — list the URLs fetched this run (each also cited above); or `unavailable — <reason>` when no external documentation was fetched. Makes degradation observable: never claim `consulted` for a page not actually fetched.> -### Findings -#### Finding 1: <title> -<Evidence and analysis> - -#### Finding 2: <title> -<Evidence and analysis> - -### Open Questions -- <Unresolved question needing human input> - ### Recommendations -- <Actionable recommendation derived from evidence> +- <Actionable recommendation referencing its Rn evidence> + +### Next step +<`/squad triage` or `/squad plan`> ``` ### 3.3 Triage Disposition diff --git a/workflows/shared/squad-review-guard.mjs b/workflows/shared/squad-review-guard.mjs index 4761dda04..fffaf8ea0 100644 --- a/workflows/shared/squad-review-guard.mjs +++ b/workflows/shared/squad-review-guard.mjs @@ -4,6 +4,7 @@ import { evaluateImplementDispatchInputs } from './squad-retro-provenance.mjs'; export const VERDICT_PREFIX = 'Squad-Review-Verdict: '; export const OVERRIDE_PREFIX = 'Squad-Review-Override: '; const WORKFLOW = '.github/workflows/squad-review.lock.yml'; +const WORKFLOW_RUNS = 'actions/workflows/squad-review.lock.yml/runs'; const AUTHORITY_JOB = 'review'; const BOOTSTRAP_WORKFLOW = '.github/workflows/squad-bootstrap.lock.yml'; const SHA = /^[0-9a-f]{40}$/; @@ -226,6 +227,10 @@ export async function reviewTarget( requireThat(SHA.test(env.SQUAD_REVIEW_WORKFLOW_SHA ?? '') && env.SQUAD_REVIEW_WORKFLOW_SHA === pr.base.sha, 'workflow source is not the exact PR base commit'); + requireThat(env.GITHUB_EVENT_NAME === (relay ? 'pull_request' : 'pull_request_target'), + relay + ? 'only merged pull request events can relay review evidence' + : 'only base-controlled PR target runs can emit review output'); requireThat(relay ? pr.merged === true && pr.state === 'closed' && pr.base.ref === env.SQUAD_REVIEW_DEFAULT_BRANCH @@ -328,64 +333,106 @@ export async function enforceReviewOutputs(env, get, options = {}) { writeFileSync(env.GH_AW_AGENT_OUTPUT, JSON.stringify(output)); } +function runMatchesTarget(run, target, runId) { + return run?.id === runId && + run.event === 'pull_request_target' && run.path === WORKFLOW && + run.repository?.full_name === target.repository && run.head_sha === target.base_sha && + run.display_title === `Squad review \u2014 PR #${target.pull_request}` && + Array.isArray(run.pull_requests) && + (run.pull_requests.length === 0 || run.pull_requests.some(pr => + pr.number === target.pull_request && pr.head?.sha === target.head_sha && + pr.base?.repo?.name === target.pr.base.repo.name)); +} + +async function latestRelayAttempt(target, get, cutoff) { + const runs = await list( + get, + `repos/${target.repository}/${WORKFLOW_RUNS}`, + 'workflow_runs', + ); + const attempts = []; + for (const run of runs) { + if (!Number.isSafeInteger(run?.id) || run.id < 1 || + !runMatchesTarget(run, target, run.id)) continue; + requireThat(Number.isSafeInteger(run.run_attempt) && run.run_attempt > 0, + 'trusted workflow run has an invalid attempt count'); + for (let runAttempt = 1; runAttempt <= run.run_attempt; runAttempt++) { + const attempt = await get( + `repos/${target.repository}/actions/runs/${run.id}/attempts/${runAttempt}`, + ); + requireThat(runMatchesTarget(attempt, target, run.id) && + attempt.run_attempt === runAttempt, + 'invalid trusted workflow run attempt'); + const startedAt = timestamp(attempt.run_started_at); + if (startedAt <= cutoff) attempts.push({ run, attempt, startedAt }); + } + } + requireThat(attempts.length > 0, 'missing trusted workflow run attempt before merge'); + attempts.sort((left, right) => + left.startedAt - right.startedAt || + left.run.id - right.run.id || + left.attempt.run_attempt - right.attempt.run_attempt); + return attempts.at(-1); +} + +async function assertAuthorityJob(target, get, selected, cutoff) { + const jobs = await list(get, + `repos/${target.repository}/actions/runs/${selected.run.id}/attempts/${selected.attempt.run_attempt}/jobs`, + 'jobs'); + const authorityJobs = jobs.filter(job => job.name === AUTHORITY_JOB); + requireThat(authorityJobs.length === 1 && + authorityJobs[0].status === 'completed' && + authorityJobs[0].conclusion === 'success' && + timestamp(authorityJobs[0].started_at) >= timestamp(selected.attempt.run_started_at) && + timestamp(authorityJobs[0].completed_at) <= timestamp(selected.attempt.updated_at) && + timestamp(authorityJobs[0].completed_at) <= cutoff, + 'base-controlled review authority job did not pass before merge'); +} + export async function assertClearingReview(env, get, options = {}) { const { relay = false } = options; - requireThat(env.GITHUB_EVENT_NAME === 'pull_request_target', - 'only base-controlled PR target runs can clear review'); const target = await reviewTarget(env, get, { ...options, requireBootstrapRunSuccess: true, }); + const cutoff = relay ? timestamp(target.pr.merged_at) : Date.now(); const candidates = (await currentEvidence(target, get)).map(review => ({ review, verdict: validateVerdict( record(review.body, VERDICT_PREFIX), target, review, - relay ? timestamp(target.pr.merged_at) : Date.now(), + cutoff, ), })); - // GitHub retains reviews across reruns. Order by API metadata, never verdict prose. - if (relay) { - candidates.sort((a, b) => - timestamp(b.review.submitted_at) - timestamp(a.review.submitted_at) || - b.review.id - a.review.id); - } - const runId = relay ? String(candidates[0]?.verdict.run_id) : env.GITHUB_RUN_ID; - const runAttempt = relay ? String(candidates[0]?.verdict.run_attempt) : env.GITHUB_RUN_ATTEMPT; + const selected = relay ? await latestRelayAttempt(target, get, cutoff) : undefined; + const runId = relay ? String(selected.run.id) : env.GITHUB_RUN_ID; + const runAttempt = relay ? String(selected.attempt.run_attempt) : env.GITHUB_RUN_ATTEMPT; const bound = candidates.filter(({ verdict }) => String(verdict.run_id) === runId && String(verdict.run_attempt) === runAttempt); requireThat(bound.length === 1, 'missing or duplicate verdict evidence for this run'); const { review, verdict } = bound[0]; - const cutoff = relay ? timestamp(target.pr.merged_at) : Date.now(); - const run = await get(`repos/${target.repository}/actions/runs/${verdict.run_id}`); - requireThat(run.event === 'pull_request_target' && run.path === WORKFLOW && - run.repository?.full_name === target.repository && run.head_sha === target.head_sha && - run.run_attempt >= verdict.run_attempt && - run.display_title === `Squad review \u2014 PR #${target.pull_request}` && - Array.isArray(run.pull_requests) && - (run.pull_requests.length === 0 || run.pull_requests.some(pr => pr.number === target.pull_request && - pr.head?.sha === target.head_sha && pr.base?.repo?.name === target.pr.base.repo.name)), - 'verdict is not bound to this PR workflow run'); - const attempt = run.run_attempt === verdict.run_attempt ? run : - await get(`repos/${target.repository}/actions/runs/${verdict.run_id}/attempts/${verdict.run_attempt}`); - requireThat(attempt.event === 'pull_request_target' && attempt.path === WORKFLOW && - attempt.repository?.full_name === target.repository && - attempt.head_sha === target.head_sha && - attempt.run_attempt === verdict.run_attempt && - timestamp(attempt.run_started_at) <= timestamp(verdict.timestamp) && - (attempt.status !== 'completed' || - timestamp(review.submitted_at) <= timestamp(attempt.updated_at)), 'verdict outside workflow run'); if (relay) { - requireThat(attempt.status === 'completed' && attempt.conclusion === 'success', + requireThat(selected.attempt.status === 'completed' && + timestamp(selected.attempt.updated_at) <= cutoff && + selected.attempt.conclusion === 'success', 'review run attempt did not succeed'); - const jobs = await list(get, - `repos/${target.repository}/actions/runs/${verdict.run_id}/attempts/${verdict.run_attempt}/jobs`, - 'jobs'); - const authorityJobs = jobs.filter(job => job.name === AUTHORITY_JOB); - requireThat(authorityJobs.length === 1 && authorityJobs[0].conclusion === 'success' && - timestamp(authorityJobs[0].completed_at) <= cutoff, - 'base-controlled review authority job did not pass before merge'); + requireThat(timestamp(selected.attempt.run_started_at) <= timestamp(verdict.timestamp) && + timestamp(review.submitted_at) <= timestamp(selected.attempt.updated_at), + 'verdict outside workflow run'); + await assertAuthorityJob(target, get, selected, cutoff); + } else { + const run = await get(`repos/${target.repository}/actions/runs/${verdict.run_id}`); + requireThat(runMatchesTarget(run, target, verdict.run_id) && + run.run_attempt >= verdict.run_attempt, + 'verdict is not bound to this PR workflow run'); + const attempt = run.run_attempt === verdict.run_attempt ? run : + await get(`repos/${target.repository}/actions/runs/${verdict.run_id}/attempts/${verdict.run_attempt}`); + requireThat(runMatchesTarget(attempt, target, verdict.run_id) && + attempt.run_attempt === verdict.run_attempt && + timestamp(attempt.run_started_at) <= timestamp(verdict.timestamp) && + (attempt.status !== 'completed' || + timestamp(review.submitted_at) <= timestamp(attempt.updated_at)), 'verdict outside workflow run'); } if (verdict.result === 'REQUEST_CHANGES') { const comments = await list(get, `repos/${target.repository}/issues/${target.pull_request}/comments`); diff --git a/workflows/skills/gh-aw-enlistment/SKILL.md b/workflows/skills/gh-aw-enlistment/SKILL.md index 15a62147e..7ac923375 100644 --- a/workflows/skills/gh-aw-enlistment/SKILL.md +++ b/workflows/skills/gh-aw-enlistment/SKILL.md @@ -389,7 +389,7 @@ gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}" # Safe-update report shows ONLY the two documented secrets + squad-init → approve once gh aw compile --strict --approve node .github/workflows/shared/squad-install-verifier.mjs --materialize-runtime -gh aw compile --strict # final, no --approve; only the bot-trigger warning remains +gh aw compile --strict # final, no --approve; exactly the two documented warnings remain node .github/workflows/shared/squad-install-verifier.mjs \ --verify-install --source-revision "${SQUAD_SHA}" --strict-compile @@ -446,8 +446,9 @@ gh pr merge --squash # auto-merge before human review. NEVER. else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). -- ❌ **Tolerating extra warnings.** Only the `squad.md` bot-trigger warning is - allowed; every other warning or error halts the run. +- ❌ **Tolerating extra warnings.** Only the documented `squad-review.md` + `pull_request_target` advisory and `squad.md` bot-trigger warning are allowed; + every other warning or error halts the run. - ❌ **Committing diagnostics.** Never commit `.github/aw/logs/` output; add the log `.gitignore` if missing. - ❌ **Clobbering existing workflows.** The install is additive; preserve unrelated diff --git a/workflows/squad-bootstrap.md b/workflows/squad-bootstrap.md index e954c84f2..3bc3e9f54 100644 --- a/workflows/squad-bootstrap.md +++ b/workflows/squad-bootstrap.md @@ -241,8 +241,12 @@ safe-outputs: SQUAD_BOOTSTRAP_RUN_ID: ${{ github.run_id }} with: script: | - const { mkdirSync, readFileSync, writeFileSync } = await import('node:fs'); - const { dirname, join } = await import('node:path'); + const { + cpSync, existsSync, lstatSync, mkdirSync, mkdtempSync, + readFileSync, rmSync, writeFileSync, + } = await import('node:fs'); + const { tmpdir } = await import('node:os'); + const { dirname, isAbsolute, join, relative, resolve } = await import('node:path'); const { pathToFileURL } = await import('node:url'); const checkout = join(process.env.GITHUB_WORKSPACE, 'bootstrap-repo'); @@ -271,19 +275,39 @@ safe-outputs: core.setFailed(`Bootstrap payload transport is invalid: ${error.message}`); return; } - const payloadPath = join(checkout, '.github/workflows/squad-bootstrap-payload.json'); - for (const file of payload.files || []) { - const target = join(checkout, ...String(file.path || '').split('/')); - mkdirSync(dirname(target), { recursive: true }); - writeFileSync(target, String(file.content || '')); - } - writeFileSync(payloadPath, payloadText); + const safeTarget = (root, path) => { + if (typeof path !== 'string' || path.length === 0 || path.includes('\\') || + isAbsolute(path) || path.split('/').some((segment) => !segment || segment === '.' || segment === '..')) { + throw new Error(`Bootstrap payload contains an unsafe path: ${JSON.stringify(path)}`); + } + const target = resolve(root, path); + const within = relative(root, target); + if (!within || within.startsWith('..') || isAbsolute(within)) { + throw new Error(`Bootstrap payload path escapes the candidate tree: ${path}`); + } + let current = root; + for (const segment of path.split('/').slice(0, -1)) { + current = join(current, segment); + if (existsSync(current) && lstatSync(current).isSymbolicLink()) { + throw new Error(`Bootstrap payload path crosses a symbolic link: ${path}`); + } + } + if (existsSync(target) && lstatSync(target).isSymbolicLink()) { + throw new Error(`Bootstrap payload target is a symbolic link: ${path}`); + } + return target; + }; + for (const file of payload.files || []) safeTarget(checkout, file?.path); - const validate = (candidate, linkMode) => { - writeFileSync(payloadPath, `${JSON.stringify(candidate)}\n`); + const candidate = mkdtempSync(join(tmpdir(), 'squad-bootstrap-candidate-')); + const payloadPath = join(checkout, '.github/workflows/squad-bootstrap-payload.json'); + const validate = (candidatePayload, linkMode) => { + const candidatePayloadPath = + join(candidate, '.github/workflows/squad-bootstrap-payload.json'); + writeFileSync(candidatePayloadPath, `${JSON.stringify(candidatePayload)}\n`); const errors = validatorModule.validateBootstrapPayload({ - root: checkout, - payloadPath, + root: candidate, + payloadPath: candidatePayloadPath, repository: context.repo.owner + '/' + context.repo.repo, defaultBranch: process.env.SQUAD_BOOTSTRAP_DEFAULT_BRANCH, linkMode, @@ -292,7 +316,26 @@ safe-outputs: throw new Error(`Squad bootstrap validation failed:\n${errors.map((error) => `- ${error}`).join('\n')}`); } }; - validate(payload, 'placeholder'); + try { + cpSync(checkout, candidate, { + recursive: true, + filter: (source) => relative(checkout, source).split('/')[0] !== '.git', + }); + for (const file of payload.files || []) { + const target = safeTarget(candidate, file.path); + mkdirSync(dirname(target), { recursive: true }); + writeFileSync(target, file.content); + } + validate(payload, 'placeholder'); + for (const file of payload.files) { + const target = safeTarget(checkout, file.path); + mkdirSync(dirname(target), { recursive: true }); + writeFileSync(target, file.content); + } + writeFileSync(payloadPath, payloadText); + } finally { + rmSync(candidate, { recursive: true, force: true }); + } const listState = async () => { const pullRequests = await github.paginate(github.rest.pulls.list, { diff --git a/workflows/squad-command-router.md b/workflows/squad-command-router.md index fc1dab81f..0d652f069 100644 --- a/workflows/squad-command-router.md +++ b/workflows/squad-command-router.md @@ -70,9 +70,29 @@ safe-outputs: return; } if (result.status !== 'accepted') { - core.setFailed('Squad command discovery was activated without an invocation.'); + core.info('No standalone Squad command was found after excluding code contexts.'); return; } + if (contract.commandRequiresAuthorization(result)) { + let permission = 'unresolved'; + try { + permission = (await github.rest.repos.getCollaboratorPermissionLevel({ + ...context.repo, + username: context.actor, + })).data.permission || 'unresolved'; + } catch (error) { + core.warning(`Unable to resolve repository permission for ${context.actor}: ${error.message}`); + } + if (!contract.isAuthorizedPermission(permission)) { + await github.rest.issues.createComment({ + ...context.repo, + issue_number: issueNumber, + body: `⛔ /squad ${result.argumentText || 'cast'} was refused for @${context.actor} (repository permission: ${permission}). Mutating /squad modes require write, maintain, or admin repository permission. Ask a repository maintainer to run this command or grant the required access.`, + }); + core.setFailed(`Squad refused mutating mode ${result.mode} for ${context.actor}.`); + return; + } + } await github.rest.actions.createWorkflowDispatch({ ...context.repo, workflow_id: 'squad.lock.yml', diff --git a/workflows/squad-implement-worker.md b/workflows/squad-implement-worker.md index 4cc6461f9..52601eafd 100644 --- a/workflows/squad-implement-worker.md +++ b/workflows/squad-implement-worker.md @@ -137,6 +137,7 @@ pre-agent-steps: env: SQUAD_REVIEW_PR: ${{ github.event.pull_request.number }} SQUAD_REVIEW_HEAD: ${{ github.event.pull_request.head.sha }} + SQUAD_REVIEW_WORKFLOW_SHA: ${{ github.event.pull_request.base.sha }} SQUAD_REVIEW_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} with: script: | @@ -256,6 +257,7 @@ safe-outputs: env: SQUAD_REVIEW_PR: ${{ github.event.pull_request.number }} SQUAD_REVIEW_HEAD: ${{ github.event.pull_request.head.sha }} + SQUAD_REVIEW_WORKFLOW_SHA: ${{ github.event.pull_request.base.sha }} SQUAD_REVIEW_DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} with: script: | diff --git a/workflows/squad-workflows.manifest.json b/workflows/squad-workflows.manifest.json index d8af562d4..20a576ba0 100644 --- a/workflows/squad-workflows.manifest.json +++ b/workflows/squad-workflows.manifest.json @@ -14,18 +14,18 @@ "source": "workflows/package/squad.md", "destination": ".github/workflows/squad.md", "lock": ".github/workflows/squad.lock.yml", - "source_sha256": "e3cc1b34c38d345eb5c843d4c4cf4966d7bbdc8c78c567cd3fe44aa84abd0fe7", - "lock_sha256": "85aa7c5cbda5453d71af43897088fc06bdb0771c4ae825d88721c4e12e4feaf1", - "package_lock_sha256": "99d5bc838fd05c4d4bdf67c7796c0b0888250154ac26fdeb99b101f8f46cb7b3" + "source_sha256": "c448595b6c565b16e6945974e752773a666f3956c6a13a25ea6c1bd70e15b02f", + "lock_sha256": "5667e8e99ef82df5ead035c60ca2f8d7443d54c63b159eb724d97422b25bade4", + "package_lock_sha256": "6cad4d0709c07759c1e86bf310a7f423fbb1be6aa8c5795ff46cc29757d8f534" }, { "name": "squad-implement-worker", "source": "workflows/package/squad-implement-worker.md", "destination": ".github/workflows/squad-implement-worker.md", "lock": ".github/workflows/squad-implement-worker.lock.yml", - "source_sha256": "ae710dd5c89360af540576dac378a0d0df129cce5a9bbf16f0615dfd86d0b21c", - "lock_sha256": "d6d2fa7f2bac5845c414cf42a9ea565d3663dba23d9208942fe142fca6b23bc3", - "package_lock_sha256": "3339eb39837e326d32bdf5c0b4bb147987da26f28c06e3e943d2da27f43fe0e6" + "source_sha256": "049eabecb2bab774e389fc62c6bde9308f3fb1a923325401cbecc95d63fc2757", + "lock_sha256": "d52ab5f6064c62c2951bebe18446b977678ed4d927a8acb64b02303607210fc3", + "package_lock_sha256": "6e7883e0e5b5be35a5a3d463dd1a8a3a2c6da6a010e00ea1e16d719718ce4123" }, { "name": "squad-review", @@ -68,18 +68,18 @@ "source": "workflows/package/squad-bootstrap.md", "destination": ".github/workflows/squad-bootstrap.md", "lock": ".github/workflows/squad-bootstrap.lock.yml", - "source_sha256": "56cfa51bfe122ec17b2af0ad3361b58b71d29010db55caf5ac15330df080c705", - "lock_sha256": "7df45f7f65d5716e4f2412a48a9a57c65a2fc27470fddc0b18439c480c102de0", - "package_lock_sha256": "455c079fcd1ee6314907614687ee73860daffcd66e95f04f198cbe571d377547" + "source_sha256": "ca2981acbdee8ce1d4ce705b9132e3777ceb63574dd884d6c75af8a8dfe16240", + "lock_sha256": "fd1562548a1aace9c57bdf315266da413575ea07da1d4b64c4f6446afc6aefc5", + "package_lock_sha256": "8aa7f287d51079746d997e5c5f17e5807efd1dd53de1c403982fcd10de23dfc0" }, { "name": "squad-command-router", "source": "workflows/package/squad-command-router.md", "destination": ".github/workflows/squad-command-router.md", "lock": ".github/workflows/squad-command-router.lock.yml", - "source_sha256": "cbce466a6cfe64c194b17672ea3363fe7c22cb6b4258c195cc93a2d294d0bf88", - "lock_sha256": "9c5bf2b28dc82a575c528ce73a75c38b54129d512341e10cdce2a6e12ceecb4b", - "package_lock_sha256": "a0f657d9098e4caabf9bd398a0b4564c3017c252f75ac16947a28ea96347bf18" + "source_sha256": "c7e281a1d110f77f54a6f0a5257ecf8edd6f336bc894278d82efff604b03db79", + "lock_sha256": "cf1a52369304bb360e210e27a18253f769bd0fcd586220974316f704c69cd130", + "package_lock_sha256": "9074ef0d7ebf05ccc84a94fc5460fe2c7a8d1b11f4c28882ced3fd4bad23dd8d" } ], "shared_runtime": [ @@ -89,7 +89,7 @@ "package_destination": ".github/workflows/shared/squad-review-guard.mjs", "destination": ".github/workflows/shared/squad-review-guard.mjs", "ownership": "manifest", - "sha256": "95472a06daabc03428955e659a4f3c548afc316427ea48196760cc992891cad0" + "sha256": "744a7e53e24434552655ccb49f233c84e6dc5f3ec5d6d53b292bc8e13962096b" }, { "path": "shared/squad-install-verifier.mjs", @@ -105,7 +105,7 @@ "package_destination": ".github/workflows/shared/squad-command-contract.mjs", "destination": ".github/workflows/shared/squad-command-contract.mjs", "ownership": "manifest", - "sha256": "56c489a98a50024ac89a00e0d9a5e394d3ecfe5de5855a1baba5877ff29351c3" + "sha256": "5872f9098ec20e407c5115ff6f955b81be4021340d9e8f6ed744f4fcfdc23b0a" }, { "path": "shared/squad-cast-validator.mjs", @@ -169,7 +169,7 @@ "package_destination": ".github/aw/squad/runtime/shared/squad-planning-ontology.md", "destination": ".github/workflows/shared/squad-planning-ontology.md", "ownership": "manifest", - "sha256": "d629852abfb0447ad77955846373923b01a06848b251c7fed1ff1dbae4e852fe" + "sha256": "25faff76305e26f6b630afe6ae9fe9b93cd41544966bdc9a644a93c08dd5989f" }, { "path": "shared/squad-planning-policy.md", @@ -225,7 +225,7 @@ "name": "gh-aw-enlistment", "source": "workflows/skills/gh-aw-enlistment/SKILL.md", "destination": ".github/skills/gh-aw-enlistment/SKILL.md", - "sha256": "bf3fc780e906b6f0e3c1fc157aac7eaf38f9c0b637f5d62f31c8d3e6d6f9c994" + "sha256": "ea71807972e64343a5d10f1353125b0918461c75a319ca759cd80c81cf339ae6" } ], "bootstrap": { From c49a5908abdbd17df47db4e5bb4dcaf5052dc8de Mon Sep 17 00:00:00 2001 From: Brady Gaster <brady.gaster@gmail.com> Date: Wed, 30 Sep 2026 22:36:45 -0700 Subject: [PATCH 2/3] fix(gh-aw): close review authorization gaps Closes #2103 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/gh-aw-command-parse.test.ts | 35 +++++++++++++++++++++ test/gh-aw-quality.test.ts | 6 ++-- test/gh-aw-review-workflow.test.ts | 22 ++++++++++--- workflows/package/squad.md | 9 +++--- workflows/shared/squad-command-contract.mjs | 24 +++++++++----- workflows/squad-workflows.manifest.json | 8 ++--- workflows/squad.md | 9 +++--- 7 files changed, 86 insertions(+), 27 deletions(-) diff --git a/test/gh-aw-command-parse.test.ts b/test/gh-aw-command-parse.test.ts index 5086b6546..73e7ea4e2 100644 --- a/test/gh-aw-command-parse.test.ts +++ b/test/gh-aw-command-parse.test.ts @@ -89,6 +89,12 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { ['Use `/squad cast` after review.', 'inline code'], ['```text\n/squad cast\n```', 'fenced code'], ['~~~text\n/squad cast\n~~~', 'tilde-fenced code'], + ['````text\n```\n/squad cast\n````', 'code after a shorter backtick fence'], + ['~~~~text\n~~~\n/squad cast\n~~~~', 'code after a shorter tilde fence'], + ['````text\n~~~~\n/squad cast\n````', 'code after a different fence delimiter'], + ['````text\n```` trailing-info\n/squad cast\n````', 'code after a fence with trailing info'], + [' /squad cast', 'four-space-indented code'], + ['\t/squad cast', 'tab-indented code'], ])('ignores %s instead of activating control-plane work', (body) => { expect(classifySquadCommand(payload(body), 'issues')).toEqual({ status: 'none', @@ -250,4 +256,33 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { 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-')); + try { + const source = join(process.cwd(), 'workflows', 'shared', 'squad-command-contract.mjs'); + const mutant = join(root, 'squad-command-contract-mutant.mjs'); + cpSync(source, mutant); + const original = readFileSync(mutant, 'utf8'); + const changed = original.replace( + ' && closingFence[1].length >= fence.length) {', + ' ) {', + ); + expect(changed).not.toBe(original); + writeFileSync(mutant, changed); + const module = await import(`${pathToFileURL(mutant).href}?mutation=${Date.now()}`); + const hidden = comment('````text\n```\n/squad cast\n````'); + expect(module.classifySquadCommand(hidden, 'issue_comment')).toMatchObject({ + status: 'accepted', + mode: 'cast', + }); + expect(classifySquadCommand(hidden, 'issue_comment')).toEqual({ + status: 'none', + source: 'comment', + }); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); }); diff --git a/test/gh-aw-quality.test.ts b/test/gh-aw-quality.test.ts index 3743496a4..daa4dff6d 100644 --- a/test/gh-aw-quality.test.ts +++ b/test/gh-aw-quality.test.ts @@ -3961,8 +3961,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' - ? '80744f1a2c73bcffce57d53df39da71486556c1da0516574b0e974fc3a2090dc' - : '65e307c7f9a796c9a9f26556773d23f2daa90e93294a47f319821fd99b1ebfdd'); + ? '8456d8129241269c107cdf4b16ae079e89a98ef944152df36f53ec0decf3f30d' + : 'bac4d8b90269d827fc9e626dd02c1b536bccde7105e9037971740054cda4c19b'); expect(() => validateCompilerActionPins(mutable)).toThrow(/invalid immutable action pin/); writeFileSync(lockPath, mutable); expect(verifyInstall(root).failures.join('\n')) @@ -4016,7 +4016,7 @@ describe('gh-aw: canonical package integrity contract', () => { compile(); const unpinned = readText(lockPath); expect(createHash('sha256').update(normalizeCompiledLock(unpinned, revisionA)).digest('hex')) - .toBe('65e307c7f9a796c9a9f26556773d23f2daa90e93294a47f319821fd99b1ebfdd'); + .toBe('bac4d8b90269d827fc9e626dd02c1b536bccde7105e9037971740054cda4c19b'); 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-review-workflow.test.ts b/test/gh-aw-review-workflow.test.ts index d75c2cd4f..496654fe0 100644 --- a/test/gh-aw-review-workflow.test.ts +++ b/test/gh-aw-review-workflow.test.ts @@ -77,9 +77,17 @@ function provenanceRows(workflow: string): string[] { function assertRelayHeadBinding(workflow: string): void { const relay = workflow.match(/## skill: `squad-review-relay`([\s\S]*?)(?=\n## skill:)/)?.[1] ?? ''; - expect(relay).toContain("`head_sha` equals the pull request's exact current head SHA"); - expect(relay).toMatch(/review guard binds `workflow_sha` to the pull request's exact base\s+SHA/); - expect(relay).toContain("the run's `head_sha` is not the workflow source SHA"); + const normalized = relay.replace(/\s+/g, ' '); + expect(normalized).toContain("`head_sha` equals the pull request's exact base/workflow SHA"); + expect(normalized).toContain( + "`pull_requests[].head.sha` equals the pull request's exact current head SHA", + ); + expect(normalized).toContain( + "review guard binds `workflow_sha` to the pull request's exact base SHA", + ); + expect(normalized).toContain( + '`pull_requests[].head.sha` binds the reviewed revision to the exact pull request head', + ); } function assertRelayRuntimeBinding(workflow: string): void { @@ -338,8 +346,12 @@ describe('gh-aw enforcing Squad reviewer', () => { it('rejects dispatcher source mutations conflating API run head and workflow source', () => { for (const mutation of [ - ROUTER.replace("`head_sha` equals the pull request's exact current head SHA", - "`head_sha` equals the pull request's exact base SHA"), + ROUTER.replace("`head_sha` equals the pull request's exact base/workflow SHA", + "`head_sha` equals the pull request's exact current head SHA"), + ROUTER.replace( + "`pull_requests[].head.sha` equals the pull request's exact current head SHA", + "`pull_requests[].head.sha` equals the pull request's exact base SHA", + ), ROUTER.replace("review guard binds `workflow_sha` to the pull request's exact base", "review guard binds `workflow_sha` to the pull request's exact head"), ]) { diff --git a/workflows/package/squad.md b/workflows/package/squad.md index 5a489a534..f38dbe3d7 100644 --- a/workflows/package/squad.md +++ b/workflows/package/squad.md @@ -2247,10 +2247,11 @@ request, then stop. List workflow runs for `.github/workflows/squad-review.lock.yml`. Keep only `pull_request_target` runs associated with this pull request whose API -`head_sha` equals the pull request's exact current head SHA and whose recorded -pull request head equals the exact current head. Keep immutable workflow source -separate: the review guard binds `workflow_sha` to the pull request's exact base -SHA; the run's `head_sha` is not the workflow source SHA. +`head_sha` equals the pull request's exact base/workflow SHA and whose +`pull_requests[].head.sha` equals the pull request's exact current head SHA. +Keep immutable workflow source separate: the review guard binds `workflow_sha` to the pull request's exact base +SHA, while `pull_requests[].head.sha` binds the reviewed revision to the exact +pull request head. Select the newest run by creation time, breaking ties by numeric run ID, then fetch that run attempt's jobs and require exactly one job named `review`. A same-named job from any other run is diff --git a/workflows/shared/squad-command-contract.mjs b/workflows/shared/squad-command-contract.mjs index 8faa4a7e9..548e082b5 100644 --- a/workflows/shared/squad-command-contract.mjs +++ b/workflows/shared/squad-command-contract.mjs @@ -116,16 +116,26 @@ function extractInvocation(source, text) { }; } - let fence = ''; + let fence = null; for (const originalLine of normalizeNewlines(text).split('\n')) { - const fenceMatch = originalLine.match(/^\s*(`{3,}|~{3,})/); - if (fenceMatch) { - const marker = fenceMatch[1][0]; - if (!fence) fence = marker; - else if (fence === marker) fence = ''; + if (fence) { + const closingFence = originalLine.match(/^ {0,3}(`{3,}|~{3,})[ \t]*$/); + if (closingFence + && closingFence[1][0] === fence.delimiter + && closingFence[1].length >= fence.length) { + fence = null; + } continue; } - if (fence) continue; + const openingFence = originalLine.match(/^ {0,3}(`{3,}|~{3,})(.*)$/); + if (openingFence) { + fence = { + delimiter: openingFence[1][0], + length: openingFence[1].length, + }; + continue; + } + if (/^(?: {4}|\t)/.test(originalLine)) continue; const line = originalLine.replace(/`+[^`]*`+/g, '').trim(); if (!/^\/squad/i.test(line)) continue; const slash = line.match(/^\/squad(?:\s|$)/); diff --git a/workflows/squad-workflows.manifest.json b/workflows/squad-workflows.manifest.json index 20a576ba0..3aedea015 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": "c448595b6c565b16e6945974e752773a666f3956c6a13a25ea6c1bd70e15b02f", - "lock_sha256": "5667e8e99ef82df5ead035c60ca2f8d7443d54c63b159eb724d97422b25bade4", - "package_lock_sha256": "6cad4d0709c07759c1e86bf310a7f423fbb1be6aa8c5795ff46cc29757d8f534" + "source_sha256": "0f292c576983ac99dace0661e34f900ddd91fcd50f28ff6726eff78704e283f2", + "lock_sha256": "32df53558611b35f809ea40a0de0b1b33e4f4dda955400b5387e77f96cb25277", + "package_lock_sha256": "a3336602334921327ca585f2321f84547ac2fde94f22178e25dcb029dd1370e6" }, { "name": "squad-implement-worker", @@ -105,7 +105,7 @@ "package_destination": ".github/workflows/shared/squad-command-contract.mjs", "destination": ".github/workflows/shared/squad-command-contract.mjs", "ownership": "manifest", - "sha256": "5872f9098ec20e407c5115ff6f955b81be4021340d9e8f6ed744f4fcfdc23b0a" + "sha256": "137e30129602128cbea33fb4f113e66761fe14c9b1db906dff3b6f681f19de58" }, { "path": "shared/squad-cast-validator.mjs", diff --git a/workflows/squad.md b/workflows/squad.md index b48981e15..d7ab39723 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1136,10 +1136,11 @@ request, then stop. List workflow runs for `.github/workflows/squad-review.lock.yml`. Keep only `pull_request_target` runs associated with this pull request whose API -`head_sha` equals the pull request's exact current head SHA and whose recorded -pull request head equals the exact current head. Keep immutable workflow source -separate: the review guard binds `workflow_sha` to the pull request's exact base -SHA; the run's `head_sha` is not the workflow source SHA. +`head_sha` equals the pull request's exact base/workflow SHA and whose +`pull_requests[].head.sha` equals the pull request's exact current head SHA. +Keep immutable workflow source separate: the review guard binds `workflow_sha` to the pull request's exact base +SHA, while `pull_requests[].head.sha` binds the reviewed revision to the exact +pull request head. Select the newest run by creation time, breaking ties by numeric run ID, then fetch that run attempt's jobs and require exactly one job named `review`. A same-named job from any other run is From d765962e054a8899d9c394c8867c31ac730097d1 Mon Sep 17 00:00:00 2001 From: Brady Gaster <brady.gaster@gmail.com> Date: Wed, 30 Sep 2026 22:51:49 -0700 Subject: [PATCH 3/3] fix: ignore CommonMark tab-indented commands Closes #2103 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/gh-aw-command-parse.test.ts | 35 +++++++++++++++++++++ workflows/shared/squad-command-contract.mjs | 2 +- workflows/squad-workflows.manifest.json | 2 +- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/test/gh-aw-command-parse.test.ts b/test/gh-aw-command-parse.test.ts index 73e7ea4e2..2b5161652 100644 --- a/test/gh-aw-command-parse.test.ts +++ b/test/gh-aw-command-parse.test.ts @@ -38,7 +38,9 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { ])('%s call site', (_name, payload) => { it.each([ ['/squad cast', 'cast', null], + [' /squad cast ', 'cast', null], [' /squad cast ', 'cast', null], + [' /squad cast ', 'cast', null], ['Context first.\n\n/squad status ', 'status', null], ['/squad research Focus only on proposal P1', 'research', null], ['/squad activate phase 2', 'activate', 2], @@ -95,6 +97,9 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { ['````text\n```` trailing-info\n/squad cast\n````', 'code after a fence with trailing info'], [' /squad cast', 'four-space-indented code'], ['\t/squad cast', 'tab-indented code'], + [' \t/squad cast', 'one-space-plus-tab-indented code'], + [' \t/squad cast', 'two-spaces-plus-tab-indented code'], + [' \t/squad cast', 'three-spaces-plus-tab-indented code'], ])('ignores %s instead of activating control-plane work', (body) => { expect(classifySquadCommand(payload(body), 'issues')).toEqual({ status: 'none', @@ -285,4 +290,34 @@ describe('gh-aw: shared /squad command contract (#1824)', () => { rmSync(root, { recursive: true, force: true }); } }); + + it('realistic CommonMark tab-indentation mutation exposes hidden commands and is caught', async () => { + mkdirSync(TEST_ROOT, { recursive: true }); + const root = mkdtempSync(join(TEST_ROOT, 'command-indent-mutant-')); + try { + const source = join(process.cwd(), 'workflows', 'shared', 'squad-command-contract.mjs'); + const mutant = join(root, 'squad-command-contract-mutant.mjs'); + cpSync(source, mutant); + const original = readFileSync(mutant, 'utf8'); + const changed = original.replace( + '/^(?: {4}| {0,3}\\t)/', + '/^(?: {4}|\\t)/', + ); + expect(changed).not.toBe(original); + writeFileSync(mutant, changed); + const module = await import(`${pathToFileURL(mutant).href}?mutation=${Date.now()}`); + for (const body of [' \t/squad cast', ' \t/squad cast', ' \t/squad cast']) { + expect(module.classifySquadCommand(comment(body), 'issue_comment')).toMatchObject({ + status: 'accepted', + mode: 'cast', + }); + expect(classifySquadCommand(comment(body), 'issue_comment')).toEqual({ + status: 'none', + source: 'comment', + }); + } + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); }); diff --git a/workflows/shared/squad-command-contract.mjs b/workflows/shared/squad-command-contract.mjs index 548e082b5..be39ed342 100644 --- a/workflows/shared/squad-command-contract.mjs +++ b/workflows/shared/squad-command-contract.mjs @@ -135,7 +135,7 @@ function extractInvocation(source, text) { }; continue; } - if (/^(?: {4}|\t)/.test(originalLine)) continue; + if (/^(?: {4}| {0,3}\t)/.test(originalLine)) continue; const line = originalLine.replace(/`+[^`]*`+/g, '').trim(); if (!/^\/squad/i.test(line)) continue; const slash = line.match(/^\/squad(?:\s|$)/); diff --git a/workflows/squad-workflows.manifest.json b/workflows/squad-workflows.manifest.json index 3aedea015..f347970b8 100644 --- a/workflows/squad-workflows.manifest.json +++ b/workflows/squad-workflows.manifest.json @@ -105,7 +105,7 @@ "package_destination": ".github/workflows/shared/squad-command-contract.mjs", "destination": ".github/workflows/shared/squad-command-contract.mjs", "ownership": "manifest", - "sha256": "137e30129602128cbea33fb4f113e66761fe14c9b1db906dff3b6f681f19de58" + "sha256": "9c6d70b7c590b36488de48e5e8b2fcaa59793115060bb85bac9dbfd6fe934b7d" }, { "path": "shared/squad-cast-validator.mjs",