From 5dce0aa7603a1efba8d303e59d66fb7044e6bc48 Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Tue, 29 Sep 2026 12:19:37 -0700 Subject: [PATCH 1/5] fix(gh-aw): use native review authority Closes #2103 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .changeset/dedicated-review-app.md | 4 +- .github/agents.md | 10 +- .../skills/gh-aw-enlistment/SKILL.md | 67 +++--- docs/src/content/docs/guide/gh-aw.md | 124 +++++------ .../skills/gh-aw-enlistment/SKILL.md | 67 +++--- .../skills/gh-aw-enlistment/SKILL.md | 67 +++--- scripts/gh-aw-hosted-e2e.mjs | 209 +++++------------- templates/skills/gh-aw-enlistment/SKILL.md | 67 +++--- test/gh-aw-enlistment-skill.test.ts | 21 +- test/gh-aw-hosted-e2e.test.ts | 98 +------- test/gh-aw-review-guard.test.ts | 136 +++--------- test/gh-aw-review-workflow.test.ts | 204 +++-------------- test/standalone-release-workflow.test.ts | 2 - workflows/package/squad-bootstrap.md | 2 +- workflows/package/squad-review.md | 193 ++-------------- workflows/package/squad.md | 17 +- workflows/shared/squad-install-verifier.mjs | 28 ++- workflows/shared/squad-review-guard.mjs | 59 +---- workflows/skills/gh-aw-enlistment/SKILL.md | 67 +++--- workflows/squad-bootstrap.md | 2 +- workflows/squad-review.md | 190 ++-------------- workflows/squad-workflows.manifest.json | 18 +- workflows/squad.md | 17 +- 23 files changed, 459 insertions(+), 1210 deletions(-) diff --git a/.changeset/dedicated-review-app.md b/.changeset/dedicated-review-app.md index 30e809777..81f7afbb0 100644 --- a/.changeset/dedicated-review-app.md +++ b/.changeset/dedicated-review-app.md @@ -3,4 +3,6 @@ "@bradygaster/squad-sdk": patch --- -Require a dedicated, branch-restricted GitHub App identity for authoritative Squad review checks. +Use the base-controlled native `Squad Review / review` job as the authoritative +review result without requiring a separately provisioned reviewer App, token, +private key, secret, environment, or custom Checks API publisher. diff --git a/.github/agents.md b/.github/agents.md index 44593c3ab..ba6d52a2e 100644 --- a/.github/agents.md +++ b/.github/agents.md @@ -35,12 +35,10 @@ GitHub Agentic Workflows (`gh-aw`) are composable AI workflows triggered by slas > the repository's default branch. > The installation PR is a manual trust boundary: its PR-controlled workflow > cannot mint a trusted Squad verdict. After merge, require the automatically -> opened Cast PR to pass the exact-head `Squad Review / review` attestation from -> the base-controlled `pull_request_target` authority before treating bootstrap -> or lifecycle automation as trusted. That check must be published by a -> dedicated GitHub App token minted only inside the exact-default-branch -> `squad-review-authority` environment. Missing or misconfigured reviewer -> credentials fail closed; the workflow never falls back to `github.token`. +> opened Cast PR to pass the native `Squad Review / review` job from the +> base-controlled `pull_request_target` workflow before treating bootstrap or +> lifecycle automation as trusted. Reviewer authority requires no separate PAT, +> GitHub App, private key, secret, environment, or external service. The quick start installs this workflow set: diff --git a/.squad-templates/skills/gh-aw-enlistment/SKILL.md b/.squad-templates/skills/gh-aw-enlistment/SKILL.md index 9005f105e..a8fd5fc76 100644 --- a/.squad-templates/skills/gh-aw-enlistment/SKILL.md +++ b/.squad-templates/skills/gh-aw-enlistment/SKILL.md @@ -182,17 +182,17 @@ uses a moving branch reference. On a clean repo, `gh aw add` reports these expected safe updates and **nothing else**: -- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`**, - **`SQUAD_GITHUB_TOKEN`**, and **`SQUAD_REVIEW_APP_PRIVATE_KEY`** +- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`** and **`SQUAD_GITHUB_TOKEN`** - Action: **`bradygaster/squad/.github/actions/squad-init`** -> **Compilation approval is not credential provisioning.** `gh aw add` lists -> referenced names so you can approve the surface. `SQUAD_GITHUB_APP_PRIVATE_KEY` -> and `SQUAD_GITHUB_TOKEN` remain optional activation credentials. -> `SQUAD_REVIEW_APP_PRIVATE_KEY` is different: trusted check publication requires -> it in the exact-default-branch `squad-review-authority` environment. Never put -> reviewer credentials at repository scope and never fall back to `github.token`. +> **These are referenced names, not prerequisites.** `gh aw add` lists the secrets +> the workflows *reference* so you can approve that surface — it is not asking you +> to supply them. Both secrets are optional, they need not exist, and neither is +> required to enlist a repository. Single-repo activation runs on the built-in +> `github.token`. Configure them only for cross-repo access or elevated +> permissions. Auth precedence: GitHub App token, then the PAT, then +> `github.token`. Never block an enlistment waiting for a credential. If — and only if — the report contains exactly those documented entries, complete the one-time approval: @@ -284,25 +284,30 @@ gh pr checks --watch `main`. - Request Copilot review, address feedback, and wait for required checks. -### 9. Verify the external review-authority prerequisite when API access permits - -Before claiming trusted review is ready, query the repository environment. It -must be named `squad-review-authority`, use custom deployment branch policies, -allow exactly the captured `${default_branch}`, contain the environment secret -`SQUAD_REVIEW_APP_PRIVATE_KEY`, and define environment variables -`SQUAD_REVIEW_APP_ID`, `SQUAD_REVIEW_APP_SLUG`, and -`SQUAD_REVIEW_APP_OWNER`. The App ID/slug must differ from -`15368`/`github-actions`. All four reviewer-specific names must also be absent -from repository-level Actions secrets and variables; repository fallback does -not satisfy this gate. - -If the authenticated account cannot read environment configuration, or any -requirement is absent, report the exact external prerequisite and do not claim -the required check is trusted. The installation PR remains a human boundary; -after it merges, the Cast canary intentionally fails closed until an -administrator provisions the environment and reruns the base-controlled -review. Configure a ruleset only from the canary check's observed App -integration ID, never from user input or a guessed value. +### 9. Verify the native review contract after merge + +The installation PR remains an explicit human trust boundary. After a human +merges it, inspect the automatically opened Cast PR and require the native +`Squad Review / review` job from the base-controlled `pull_request_target` +workflow to succeed. Verify the exact workflow path, immutable base/workflow +SHA, PR base/head, run ID and attempt, successful `review` job, and exact-head +GitHub Actions check-run binding. + +This review path uses only the native GitHub Actions/gh-aw runtime identity. +Never request or provision a reviewer PAT, GitHub App, private key, secret, +environment, hosted attestor, callback, or external service. The optional +`SQUAD_GITHUB_APP_*` and `SQUAD_GITHUB_TOKEN` activation credentials remain +unrelated to reviewer authority. + +For authoritative merge enforcement, use a source-bound required-workflow or +equivalent ruleset when available. A context-only requirement for +`Squad Review / review` is advisory because a PR-controlled workflow may be +able to emit the same workflow/job name through the shared GitHub Actions +identity. If source-bound enforcement is unavailable, retain independent human +review rather than relying on that context alone or adding a credential-based +publisher. Require branches to be up to date before merge (or use an equivalent +merge-queue freshness guarantee), and enable the required context only after +the workflow installation is merged and the post-merge Cast canary succeeds. ### 10. Never auto-merge — and explain what comes next @@ -366,9 +371,8 @@ gh pr edit --add-reviewer @copilot # then wait for review + checks; DO NOT mer ### ✓ Correct: STOP on an undocumented safe-update entry ```text -gh aw add reports an additional secret: `ACME_DEPLOY_KEY`. -→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN, - SQUAD_REVIEW_APP_PRIVATE_KEY) +gh aw add reports a third secret: `ACME_DEPLOY_KEY`. +→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN) and is NOT the squad-init action. Do NOT run `--approve`. Halt, report "unexpected safe-update entry: ACME_DEPLOY_KEY", and wait. ``` @@ -402,8 +406,7 @@ gh pr merge --squash # auto-merge before human review. NEVER. - ❌ **Blanket staging** (`git add .` / `-A` / `git commit -a`). Stage only `.gitattributes`, `.github/aw/`, `.github/workflows/`, `.github/skills/`, by path. - ❌ **Approving unknown safe updates.** Approve ONLY `SQUAD_GITHUB_APP_PRIVATE_KEY`, - `SQUAD_GITHUB_TOKEN`, `SQUAD_REVIEW_APP_PRIVATE_KEY`, and - `bradygaster/squad/.github/actions/squad-init`. Anything + `SQUAD_GITHUB_TOKEN`, and `bradygaster/squad/.github/actions/squad-init`. Anything else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). diff --git a/docs/src/content/docs/guide/gh-aw.md b/docs/src/content/docs/guide/gh-aw.md index 0c9cfabe7..41839c379 100644 --- a/docs/src/content/docs/guide/gh-aw.md +++ b/docs/src/content/docs/guide/gh-aw.md @@ -119,6 +119,9 @@ GitHub Actions bot comment, so editable PR prose cannot rebind an old bootstrap run to a changed branch head. Only after that base-controlled canary succeeds should the Cast/bootstrap lifecycle be treated as trusted. Do not enable the required check in a ruleset before this post-install canary has been observed. +The review uses only the native GitHub Actions/gh-aw runtime identity; it does +not require a reviewer PAT, GitHub App, private key, secret, environment, or +external service. Step 2 deliberately checks Issues before creating the bootstrap branch or installing any workflow. Squad commands are issue comments, and the merged @@ -298,21 +301,17 @@ source/lock pairs, and one coherent 40-character revision. On a clean repository, `gh aw add` reports these expected safe-update changes: -- Restricted secrets: `SQUAD_GITHUB_APP_PRIVATE_KEY`, `SQUAD_GITHUB_TOKEN`, and - `SQUAD_REVIEW_APP_PRIVATE_KEY` +- Restricted secrets: `SQUAD_GITHUB_APP_PRIVATE_KEY` and `SQUAD_GITHUB_TOKEN` - Action: `bradygaster/squad/.github/actions/squad-init` > **These are referenced names, not prerequisites.** `gh aw add` lists the secrets > the workflows *reference* so you can approve that surface — it is not asking you -> to supply them during compilation. `SQUAD_GITHUB_APP_PRIVATE_KEY` and -> `SQUAD_GITHUB_TOKEN` remain optional activation credentials; single-repo -> activation can use the built-in `github.token`. Configure those only for cross-repo access or +> to supply them. Both secrets are optional, they need not exist, and you do not +> need to create either one to enlist a repository. Single-repo activation runs on +> the built-in `github.token`. Configure these only for cross-repo access or > elevated permissions — see [enhanced permissions with a GitHub > App](#optional-enhanced-permissions-with-a-github-app) and [PAT -> fallback](#optional-pat-fallback). `SQUAD_REVIEW_APP_PRIVATE_KEY` is different: -> it belongs only in the branch-restricted `squad-review-authority` environment -> described below. Trusted required-check publication intentionally fails closed -> until that external prerequisite is provisioned. +> fallback](#optional-pat-fallback). Review the report before approving it. If it contains only those documented entries, complete the first-install approval with: @@ -436,47 +435,6 @@ a Personal Access Token: **Auth precedence:** GitHub App token → `SQUAD_GITHUB_TOKEN` → `github.token`. -### Required: dedicated review authority App - -The required `Squad Review / review` check must not use `github.token`. -`github.token` checks are owned by the shared GitHub Actions App -(`id: 15368`, slug: `github-actions`), which is also available to ordinary -same-repository PR workflows. The reviewer instead mints a short-lived token -for a dedicated GitHub App inside a protected environment and has no fallback. - -Before expecting the post-install Cast canary to pass: - -1. Create a dedicated GitHub App with repository **Checks: read and write** and - read access to Actions, contents, metadata, and pull requests. Install it - only on the consumer repository. -2. Create the Actions environment **`squad-review-authority`**. -3. Configure a custom deployment branch policy containing exactly the - repository default branch. Do not allow wildcard, feature, release, tag, or - additional branch policies; a PR workflow must not be able to request this - environment from its own ref. -4. Store these reviewer-specific values in that environment, not as - repository-level Actions secrets or variables: - - | Setting | Type | Purpose | - |---------|------|---------| - | `SQUAD_REVIEW_APP_ID` | Variable | Dedicated App numeric ID; must not be `15368` | - | `SQUAD_REVIEW_APP_SLUG` | Variable | Dedicated App slug; must not be `github-actions` | - | `SQUAD_REVIEW_APP_OWNER` | Variable | Repository owner where the App is installed | - | `SQUAD_REVIEW_APP_PRIVATE_KEY` | Secret | Dedicated App private key | - -The publisher validates the configured ID and slug, mints an installation token -inside the environment, publishes with that token, and validates the App -identity returned by the Checks API. Missing environment protection, missing -credentials, token-mint failure, Actions App identity, or any ID/slug mismatch -fails closed. An ordinary `github.token` may create a same-named check owned by -the Actions App, but it cannot update the dedicated App's check and cannot -satisfy an integration-bound ruleset. - -Before rollout, verify the four reviewer-specific names are absent from -repository-level Actions secrets and variables. GitHub expression contexts can -otherwise fall back to a repository-scoped value with the same name. The hosted -controller treats that overlap as a hard configuration error. - --- ## Supported-path validation checklist @@ -1556,13 +1514,22 @@ exactly one pull request review. It returns: The trusted writer publishes these logical verdicts in a native `COMMENT` review: GitHub does not permit an account to approve or request changes on its -own PR. The native `Squad Review Authority / attest` job proves that the base-controlled -workflow executed. Its publisher creates `Squad Review / review` on the exact -PR head with a `squad-review-check/v1` attestation linked to that run. A -PR-controlled workflow may create advisory output with the same display name, -but it cannot satisfy the relay without the successful native authority job and -exact event/base/head/run/App binding. The status check enforces -`REQUEST_CHANGES`; no additional bot accounts or Apps are needed. +own PR. The final native job is named `review`, so GitHub exposes the stable +required status context `Squad Review / review`. It uses `if: always()` and +fails unless the agent, safe-output processing, and deterministic current-head +verdict gate all succeed. The relay independently binds the verdict to the exact +automatic `pull_request_target` run, workflow path, immutable base/workflow SHA, +PR base/head, run ID and attempt, and that run attempt's successful `review` +job. No custom Checks API publisher, reviewer credential, environment, hosted +attestor, or external service is involved. + +Each successful run attempt starts with no verdict evidence for its exact run ID +and attempt, then publishes exactly one fresh safe-output review. Pre-existing +exact-attempt evidence is rejected rather than accepted through a `noop` or +deduplication path, because another PR-controlled workflow shares the +`github-actions[bot]` identity and could otherwise pre-seed a valid-looking +record. A rerun receives an incremented attempt and must publish a new review; +evidence from prior attempts is neither replayed nor reused. The reviewer has no file-editing, issue-creation, pull-request-creation, remediation, merge, or `APPROVE` authority. Its verdict @@ -1576,9 +1543,9 @@ The lifecycle is: ``` The epic relay repeats the guard before running the agent and before processing -any outputs. It requires a clearing verdict, the successful native authority -job from the base-controlled run, and the exact-head published check completed -**before merge** for the merged PR's **head SHA**, not the merge commit. +any outputs. It requires a clearing verdict and the successful native `review` +job from the exact base-controlled run attempt completed **before merge** for +the merged PR's **head SHA**, not the merge commit. Bypassing a missing Squad review at merge cannot silently advance the epic. Missing evidence stops the run with an explicit error; it never dispatches the next wave. @@ -1588,29 +1555,38 @@ error; it never dispatches the next wave. The stable literal check name is **`Squad Review / review`**. There is no manual reviewer run. `/squad review` only explains how to rerun the existing automatic `pull_request_target` run, and only that exact base-controlled -run ID, attempt, native authority job, and exact-head attestation can satisfy -its emitted verdict and check. +run ID, attempt, native authority job, and exact-head verdict can satisfy the +relay. Brady (or the consumer repository administrator) must install/merge the complete generated bundle through the explicit human installation boundary, then observe a successful base-controlled `pull_request_target` check on the post-install draft Cast PR before changing rulesets. The installation PR itself is not evidence: its workflow and -check name are controlled by that PR. The Cast canary remains intentionally red -until the [dedicated review authority App](#required-dedicated-review-authority-app) -and exact-default-branch environment policy are provisioned. Record the -activation canary's actual -check name, external ID, details URL, attestation summary, and GitHub App ID -and slug from the check-runs API. The attestation's `publisher_app_id` and -`publisher_app_slug` must equal the check's returned App identity and must not -be `15368`/`github-actions`. Enable the required context with that observed App ID -on `dev` **only after advisory soak** has met the +check name are controlled by that PR. Record the activation canary's exact +automatic run ID/attempt, workflow path and base SHA, native `review` job +conclusion, exact PR head SHA, and the job's native `Squad Review / review` +check-run binding. Enable the required context on `dev` **only after advisory soak** has met the evidence gate in #1734; promote to `main` after the agreed soak. This change does not modify repository rulesets and does not claim live soak evidence. Keep native human approving-review requirements enabled independently, with stale -approvals dismissed. The bot's `COMMENT` is not a human approval. Do not use an -unobserved guessed App ID, a user-supplied integration ID, an Actions-owned -same-name check, or the manual job. +approvals dismissed. The bot's `COMMENT` is not a human approval. +Require branches to be up to date before merge (or an equivalent merge-queue +freshness guarantee) so the required review run is always based on the current +default-branch workflow and guard. Merge the workflow installation first, +observe the post-merge Cast canary, and only then enable the required context. + +GitHub's context-only required-status-check configuration identifies the +context by name and does not, by itself, demonstrate that another workflow +cannot emit the same name. The relay is not vulnerable to that ambiguity +because it reads the exact trusted run attempt's jobs. For authoritative merge enforcement, use a source-bound required-workflow or +equivalent ruleset when the repository's GitHub plan and policy expose one. A +context-only `Squad Review / review` requirement is advisory because a +PR-controlled workflow may be able to emit the same workflow/job name through +the shared GitHub Actions identity. If source-bound enforcement is unavailable, +keep the same-name source-identity question explicit and retain independent +human review rather than relying on that context alone; do not invent a +credential or external attestor as a workaround. Because rulesets apply to all PRs, existing human/Copilot PRs must add committed attribution before enabling the requirement. Fork review remains unsupported; the workflow rejects foreign head repositories even though its base-controlled 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 9005f105e..a8fd5fc76 100644 --- a/packages/squad-cli/templates/skills/gh-aw-enlistment/SKILL.md +++ b/packages/squad-cli/templates/skills/gh-aw-enlistment/SKILL.md @@ -182,17 +182,17 @@ uses a moving branch reference. On a clean repo, `gh aw add` reports these expected safe updates and **nothing else**: -- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`**, - **`SQUAD_GITHUB_TOKEN`**, and **`SQUAD_REVIEW_APP_PRIVATE_KEY`** +- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`** and **`SQUAD_GITHUB_TOKEN`** - Action: **`bradygaster/squad/.github/actions/squad-init`** -> **Compilation approval is not credential provisioning.** `gh aw add` lists -> referenced names so you can approve the surface. `SQUAD_GITHUB_APP_PRIVATE_KEY` -> and `SQUAD_GITHUB_TOKEN` remain optional activation credentials. -> `SQUAD_REVIEW_APP_PRIVATE_KEY` is different: trusted check publication requires -> it in the exact-default-branch `squad-review-authority` environment. Never put -> reviewer credentials at repository scope and never fall back to `github.token`. +> **These are referenced names, not prerequisites.** `gh aw add` lists the secrets +> the workflows *reference* so you can approve that surface — it is not asking you +> to supply them. Both secrets are optional, they need not exist, and neither is +> required to enlist a repository. Single-repo activation runs on the built-in +> `github.token`. Configure them only for cross-repo access or elevated +> permissions. Auth precedence: GitHub App token, then the PAT, then +> `github.token`. Never block an enlistment waiting for a credential. If — and only if — the report contains exactly those documented entries, complete the one-time approval: @@ -284,25 +284,30 @@ gh pr checks --watch `main`. - Request Copilot review, address feedback, and wait for required checks. -### 9. Verify the external review-authority prerequisite when API access permits - -Before claiming trusted review is ready, query the repository environment. It -must be named `squad-review-authority`, use custom deployment branch policies, -allow exactly the captured `${default_branch}`, contain the environment secret -`SQUAD_REVIEW_APP_PRIVATE_KEY`, and define environment variables -`SQUAD_REVIEW_APP_ID`, `SQUAD_REVIEW_APP_SLUG`, and -`SQUAD_REVIEW_APP_OWNER`. The App ID/slug must differ from -`15368`/`github-actions`. All four reviewer-specific names must also be absent -from repository-level Actions secrets and variables; repository fallback does -not satisfy this gate. - -If the authenticated account cannot read environment configuration, or any -requirement is absent, report the exact external prerequisite and do not claim -the required check is trusted. The installation PR remains a human boundary; -after it merges, the Cast canary intentionally fails closed until an -administrator provisions the environment and reruns the base-controlled -review. Configure a ruleset only from the canary check's observed App -integration ID, never from user input or a guessed value. +### 9. Verify the native review contract after merge + +The installation PR remains an explicit human trust boundary. After a human +merges it, inspect the automatically opened Cast PR and require the native +`Squad Review / review` job from the base-controlled `pull_request_target` +workflow to succeed. Verify the exact workflow path, immutable base/workflow +SHA, PR base/head, run ID and attempt, successful `review` job, and exact-head +GitHub Actions check-run binding. + +This review path uses only the native GitHub Actions/gh-aw runtime identity. +Never request or provision a reviewer PAT, GitHub App, private key, secret, +environment, hosted attestor, callback, or external service. The optional +`SQUAD_GITHUB_APP_*` and `SQUAD_GITHUB_TOKEN` activation credentials remain +unrelated to reviewer authority. + +For authoritative merge enforcement, use a source-bound required-workflow or +equivalent ruleset when available. A context-only requirement for +`Squad Review / review` is advisory because a PR-controlled workflow may be +able to emit the same workflow/job name through the shared GitHub Actions +identity. If source-bound enforcement is unavailable, retain independent human +review rather than relying on that context alone or adding a credential-based +publisher. Require branches to be up to date before merge (or use an equivalent +merge-queue freshness guarantee), and enable the required context only after +the workflow installation is merged and the post-merge Cast canary succeeds. ### 10. Never auto-merge — and explain what comes next @@ -366,9 +371,8 @@ gh pr edit --add-reviewer @copilot # then wait for review + checks; DO NOT mer ### ✓ Correct: STOP on an undocumented safe-update entry ```text -gh aw add reports an additional secret: `ACME_DEPLOY_KEY`. -→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN, - SQUAD_REVIEW_APP_PRIVATE_KEY) +gh aw add reports a third secret: `ACME_DEPLOY_KEY`. +→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN) and is NOT the squad-init action. Do NOT run `--approve`. Halt, report "unexpected safe-update entry: ACME_DEPLOY_KEY", and wait. ``` @@ -402,8 +406,7 @@ gh pr merge --squash # auto-merge before human review. NEVER. - ❌ **Blanket staging** (`git add .` / `-A` / `git commit -a`). Stage only `.gitattributes`, `.github/aw/`, `.github/workflows/`, `.github/skills/`, by path. - ❌ **Approving unknown safe updates.** Approve ONLY `SQUAD_GITHUB_APP_PRIVATE_KEY`, - `SQUAD_GITHUB_TOKEN`, `SQUAD_REVIEW_APP_PRIVATE_KEY`, and - `bradygaster/squad/.github/actions/squad-init`. Anything + `SQUAD_GITHUB_TOKEN`, and `bradygaster/squad/.github/actions/squad-init`. Anything else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). 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 9005f105e..a8fd5fc76 100644 --- a/packages/squad-sdk/templates/skills/gh-aw-enlistment/SKILL.md +++ b/packages/squad-sdk/templates/skills/gh-aw-enlistment/SKILL.md @@ -182,17 +182,17 @@ uses a moving branch reference. On a clean repo, `gh aw add` reports these expected safe updates and **nothing else**: -- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`**, - **`SQUAD_GITHUB_TOKEN`**, and **`SQUAD_REVIEW_APP_PRIVATE_KEY`** +- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`** and **`SQUAD_GITHUB_TOKEN`** - Action: **`bradygaster/squad/.github/actions/squad-init`** -> **Compilation approval is not credential provisioning.** `gh aw add` lists -> referenced names so you can approve the surface. `SQUAD_GITHUB_APP_PRIVATE_KEY` -> and `SQUAD_GITHUB_TOKEN` remain optional activation credentials. -> `SQUAD_REVIEW_APP_PRIVATE_KEY` is different: trusted check publication requires -> it in the exact-default-branch `squad-review-authority` environment. Never put -> reviewer credentials at repository scope and never fall back to `github.token`. +> **These are referenced names, not prerequisites.** `gh aw add` lists the secrets +> the workflows *reference* so you can approve that surface — it is not asking you +> to supply them. Both secrets are optional, they need not exist, and neither is +> required to enlist a repository. Single-repo activation runs on the built-in +> `github.token`. Configure them only for cross-repo access or elevated +> permissions. Auth precedence: GitHub App token, then the PAT, then +> `github.token`. Never block an enlistment waiting for a credential. If — and only if — the report contains exactly those documented entries, complete the one-time approval: @@ -284,25 +284,30 @@ gh pr checks --watch `main`. - Request Copilot review, address feedback, and wait for required checks. -### 9. Verify the external review-authority prerequisite when API access permits - -Before claiming trusted review is ready, query the repository environment. It -must be named `squad-review-authority`, use custom deployment branch policies, -allow exactly the captured `${default_branch}`, contain the environment secret -`SQUAD_REVIEW_APP_PRIVATE_KEY`, and define environment variables -`SQUAD_REVIEW_APP_ID`, `SQUAD_REVIEW_APP_SLUG`, and -`SQUAD_REVIEW_APP_OWNER`. The App ID/slug must differ from -`15368`/`github-actions`. All four reviewer-specific names must also be absent -from repository-level Actions secrets and variables; repository fallback does -not satisfy this gate. - -If the authenticated account cannot read environment configuration, or any -requirement is absent, report the exact external prerequisite and do not claim -the required check is trusted. The installation PR remains a human boundary; -after it merges, the Cast canary intentionally fails closed until an -administrator provisions the environment and reruns the base-controlled -review. Configure a ruleset only from the canary check's observed App -integration ID, never from user input or a guessed value. +### 9. Verify the native review contract after merge + +The installation PR remains an explicit human trust boundary. After a human +merges it, inspect the automatically opened Cast PR and require the native +`Squad Review / review` job from the base-controlled `pull_request_target` +workflow to succeed. Verify the exact workflow path, immutable base/workflow +SHA, PR base/head, run ID and attempt, successful `review` job, and exact-head +GitHub Actions check-run binding. + +This review path uses only the native GitHub Actions/gh-aw runtime identity. +Never request or provision a reviewer PAT, GitHub App, private key, secret, +environment, hosted attestor, callback, or external service. The optional +`SQUAD_GITHUB_APP_*` and `SQUAD_GITHUB_TOKEN` activation credentials remain +unrelated to reviewer authority. + +For authoritative merge enforcement, use a source-bound required-workflow or +equivalent ruleset when available. A context-only requirement for +`Squad Review / review` is advisory because a PR-controlled workflow may be +able to emit the same workflow/job name through the shared GitHub Actions +identity. If source-bound enforcement is unavailable, retain independent human +review rather than relying on that context alone or adding a credential-based +publisher. Require branches to be up to date before merge (or use an equivalent +merge-queue freshness guarantee), and enable the required context only after +the workflow installation is merged and the post-merge Cast canary succeeds. ### 10. Never auto-merge — and explain what comes next @@ -366,9 +371,8 @@ gh pr edit --add-reviewer @copilot # then wait for review + checks; DO NOT mer ### ✓ Correct: STOP on an undocumented safe-update entry ```text -gh aw add reports an additional secret: `ACME_DEPLOY_KEY`. -→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN, - SQUAD_REVIEW_APP_PRIVATE_KEY) +gh aw add reports a third secret: `ACME_DEPLOY_KEY`. +→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN) and is NOT the squad-init action. Do NOT run `--approve`. Halt, report "unexpected safe-update entry: ACME_DEPLOY_KEY", and wait. ``` @@ -402,8 +406,7 @@ gh pr merge --squash # auto-merge before human review. NEVER. - ❌ **Blanket staging** (`git add .` / `-A` / `git commit -a`). Stage only `.gitattributes`, `.github/aw/`, `.github/workflows/`, `.github/skills/`, by path. - ❌ **Approving unknown safe updates.** Approve ONLY `SQUAD_GITHUB_APP_PRIVATE_KEY`, - `SQUAD_GITHUB_TOKEN`, `SQUAD_REVIEW_APP_PRIVATE_KEY`, and - `bradygaster/squad/.github/actions/squad-init`. Anything + `SQUAD_GITHUB_TOKEN`, and `bradygaster/squad/.github/actions/squad-init`. Anything else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). diff --git a/scripts/gh-aw-hosted-e2e.mjs b/scripts/gh-aw-hosted-e2e.mjs index e58960d0f..56b6015f6 100644 --- a/scripts/gh-aw-hosted-e2e.mjs +++ b/scripts/gh-aw-hosted-e2e.mjs @@ -39,9 +39,6 @@ const RESEARCH_MARKER = ''; const PROVENANCE_MARKER_PATTERN = /^$/gm; const ACTIONS_BOT_LOGIN = 'github-actions[bot]'; const ACTIONS_BOT_ID = 41898282; -const ACTIONS_APP_ID = 15368; -const ACTIONS_APP_SLUG = 'github-actions'; -const REVIEW_AUTHORITY_ENVIRONMENT = 'squad-review-authority'; const SAFE_CHILD_ENV = Object.freeze([ 'CI', 'GH_CONFIG_DIR', @@ -370,113 +367,6 @@ export function guardedTargetMutation({ return mutate(); } -export function validateReviewAuthorityEnvironment( - environment, - branchPolicies, - secretNames, - variables, - repositorySecretNames, - repositoryVariableNames, - defaultBranch, - targetOwner, -) { - if (environment?.name !== REVIEW_AUTHORITY_ENVIRONMENT - || environment.deployment_branch_policy?.custom_branch_policies !== true - || environment.deployment_branch_policy?.protected_branches !== false) { - throw new Error( - 'Dedicated review authority environment must use an exact custom default-branch policy.', - ); - } - if (!Array.isArray(branchPolicies) - || branchPolicies.length !== 1 - || branchPolicies[0]?.type !== 'branch' - || branchPolicies[0]?.name !== defaultBranch) { - throw new Error( - 'Dedicated review authority environment must allow only the repository default branch.', - ); - } - if (!secretNames.includes('SQUAD_REVIEW_APP_PRIVATE_KEY')) { - throw new Error('Dedicated review authority environment is missing its reviewer App private key.'); - } - const reviewerNames = new Set([ - 'SQUAD_REVIEW_APP_ID', - 'SQUAD_REVIEW_APP_SLUG', - 'SQUAD_REVIEW_APP_OWNER', - 'SQUAD_REVIEW_APP_PRIVATE_KEY', - ]); - if (repositorySecretNames.some((name) => reviewerNames.has(name)) - || repositoryVariableNames.some((name) => reviewerNames.has(name))) { - throw new Error('Reviewer App credentials must not exist at repository scope.'); - } - const values = Object.fromEntries(variables.map(({ name, value }) => [name, value])); - const appId = Number(values.SQUAD_REVIEW_APP_ID); - const appSlug = values.SQUAD_REVIEW_APP_SLUG; - const appOwner = values.SQUAD_REVIEW_APP_OWNER; - if (!Number.isSafeInteger(appId) || appId < 1 || appId === ACTIONS_APP_ID - || typeof appSlug !== 'string' || !/^[a-z0-9][a-z0-9-]*$/.test(appSlug) - || appSlug === ACTIONS_APP_SLUG || appOwner !== targetOwner) { - throw new Error('Dedicated review authority App variables are missing or misconfigured.'); - } - return { id: appId, slug: appSlug, owner: appOwner }; -} - -function reviewAuthorityEnvironment(target, defaultBranch) { - const [targetOwner] = target.split('/'); - const environment = ghJson([ - 'api', `repos/${target}/environments/${REVIEW_AUTHORITY_ENVIRONMENT}`, - ]); - const branchPolicies = ghJson([ - 'api', '--paginate', '--slurp', - `repos/${target}/environments/${REVIEW_AUTHORITY_ENVIRONMENT}/deployment-branch-policies?per_page=100`, - ]).flat().flatMap((page) => page.branch_policies ?? []); - const secretNames = ghJson([ - 'api', - `repos/${target}/environments/${REVIEW_AUTHORITY_ENVIRONMENT}/secrets?per_page=100`, - '--jq', '[.secrets[].name]', - ]); - const variables = ghJson([ - 'api', - `repos/${target}/environments/${REVIEW_AUTHORITY_ENVIRONMENT}/variables?per_page=100`, - '--jq', '[.variables[] | {name,value}]', - ]); - const repositorySecretNames = ghJson([ - 'api', `repos/${target}/actions/secrets?per_page=100`, - '--jq', '[.secrets[].name]', - ]); - const repositoryVariableNames = ghJson([ - 'api', `repos/${target}/actions/variables?per_page=100`, - '--jq', '[.variables[].name]', - ]); - return validateReviewAuthorityEnvironment( - environment, - branchPolicies, - secretNames, - variables, - repositorySecretNames, - repositoryVariableNames, - defaultBranch, - targetOwner, - ); -} - -export function observedRequiredCheck(check, attestation) { - if (check?.name !== 'Squad Review / review' - || !Number.isSafeInteger(check.app?.id) - || check.app.id < 1 - || check.app.id === ACTIONS_APP_ID - || typeof check.app?.slug !== 'string' - || check.app.slug === ACTIONS_APP_SLUG - || attestation?.publisher_app_id !== check.app.id - || attestation?.publisher_app_slug !== check.app.slug) { - throw new Error('Required-check rollout must use the observed dedicated App identity.'); - } - return { - context: check.name, - integration_id: check.app.id, - integration_slug: check.app.slug, - }; -} - export function authorizeTarget(args, sourceInfo, contract, github = githubAdapter) { const target = requireArg(args, 'target'); const expectedRepositoryId = Number(requireArg(args, 'target_repository_id')); @@ -514,9 +404,8 @@ export function authorizeTarget(args, sourceInfo, contract, github = githubAdapt || workflowPermissions.can_approve_pull_request_reviews !== true) { throw new Error('Target Actions permissions do not match the supported secure configuration.'); } - const reviewApp = reviewAuthorityEnvironment(target, info.default_branch); const baseline = assertPristineTarget(target, info.default_branch, contract, github); - return { target, info, workflowPermissions, actor, reviewApp, baseline }; + return { target, info, workflowPermissions, actor, baseline }; } function waitForBootstrapRun(target, headSha, startedAt, evidence) { @@ -676,26 +565,9 @@ export function waitForBootstrapOutputs( throw new Error('Timed out waiting for the draft Cast PR and bootstrap research issue.'); } -function waitForBaseControlledReviewCanary(target, castPr, expectedApp, evidence) { +function waitForBaseControlledReviewCanary(target, castPr, evidence) { const deadline = Date.now() + 20 * 60 * 1000; while (Date.now() < deadline) { - const checks = ghJson([ - 'api', - '-H', 'Accept: application/vnd.github+json', - `repos/${target}/commits/${castPr.headSha}/check-runs`, - '--jq', '{check_runs:[.check_runs[] | {id,name,status,conclusion,head_sha,external_id,details_url,completed_at,output:{summary:.output.summary},app:{id:.app.id,slug:.app.slug}}]}', - ]).check_runs; - const externalId = - `squad-review-authority/v1:${target}:${castPr.number}:${castPr.baseSha}:${castPr.headSha}`; - const trustedChecks = checks.filter((check) => - check.name === 'Squad Review / review' && - check.external_id === externalId && - check.head_sha === castPr.headSha && - check.app?.id === expectedApp.id && - check.app?.slug === expectedApp.slug); - if (trustedChecks.length > 1) { - throw new Error(`Expected one base-controlled Squad Review canary check; found ${trustedChecks.length}.`); - } const reviews = ghJson([ 'api', `repos/${target}/pulls/${castPr.number}/reviews?per_page=100`, @@ -709,14 +581,7 @@ function waitForBaseControlledReviewCanary(target, castPr, expectedApp, evidence if (verdicts.length > 1) { throw new Error(`Expected one base-controlled Squad Review canary verdict; found ${verdicts.length}.`); } - if (trustedChecks.length === 1 && trustedChecks[0].status === 'completed') { - const check = trustedChecks[0]; - if (check.conclusion !== 'success') { - throw new Error(`Base-controlled Squad Review canary failed for ${castPr.url}.`); - } - if (verdicts.length !== 1) { - throw new Error('Successful Squad Review canary check is missing its exact-head verdict.'); - } + if (verdicts.length === 1) { const marker = String(verdicts[0].body).match(/^Squad-Review-Verdict: (\{[^\r\n]+\})$/m); if (!marker) throw new Error('Squad Review canary verdict marker is malformed.'); const verdict = JSON.parse(marker[1]); @@ -733,33 +598,60 @@ function waitForBaseControlledReviewCanary(target, castPr, expectedApp, evidence || verdict.workflow_path !== '.github/workflows/squad-review.lock.yml') { throw new Error('Squad Review canary verdict is not bound to the base-controlled bootstrap activation.'); } - const attestation = JSON.parse(check.output?.summary ?? ''); - if (attestation.schema !== 'squad-review-check/v1' - || attestation.repository !== target - || attestation.pull_request !== castPr.number - || attestation.base_sha !== castPr.baseSha - || attestation.head_sha !== castPr.headSha - || attestation.workflow_sha !== castPr.baseSha - || attestation.run_id !== verdict.run_id - || attestation.event !== 'pull_request_target' - || attestation.workflow_path !== '.github/workflows/squad-review.lock.yml' - || attestation.authority_job !== 'Squad Review Authority / attest' - || attestation.publisher_app_id !== expectedApp.id - || attestation.publisher_app_slug !== expectedApp.slug - || check.app?.id === ACTIONS_APP_ID - || check.app?.slug === ACTIONS_APP_SLUG - || check.details_url !== `https://github.com/${target}/actions/runs/${verdict.run_id}`) { - throw new Error('Squad Review canary check is not bound to the base-controlled authority run.'); + const run = ghJson([ + 'api', `repos/${target}/actions/runs/${verdict.run_id}`, + '--jq', + '{id,event,path,head_sha,run_attempt,status,conclusion,display_title,repository:{full_name:.repository.full_name}}', + ]); + if (run.event !== 'pull_request_target' + || run.path !== '.github/workflows/squad-review.lock.yml' + || run.head_sha !== castPr.headSha + || run.repository?.full_name !== target + || run.display_title !== `Squad review — PR #${castPr.number}` + || run.run_attempt < verdict.run_attempt) { + throw new Error('Squad Review canary verdict is not bound to the base-controlled workflow run.'); + } + const jobs = ghJson([ + 'api', '--paginate', '--slurp', + `repos/${target}/actions/runs/${verdict.run_id}/attempts/${verdict.run_attempt}/jobs?per_page=100`, + ]).flat().flatMap((page) => page.jobs ?? []); + const authorityJobs = jobs.filter((job) => job.name === 'review'); + if (authorityJobs.length !== 1) { + throw new Error(`Expected one native Squad Review authority job; found ${authorityJobs.length}.`); + } + const job = authorityJobs[0]; + if (job.status !== 'completed' || job.conclusion !== 'success' + || typeof job.check_run_url !== 'string') { + throw new Error(`Base-controlled Squad Review canary failed for ${castPr.url}.`); + } + const checkId = Number(job.check_run_url.match(/\/check-runs\/([1-9][0-9]*)$/)?.[1]); + if (!Number.isSafeInteger(checkId)) { + throw new Error('Native Squad Review job did not expose a valid check-run binding.'); + } + const check = ghJson([ + 'api', + '-H', 'Accept: application/vnd.github+json', + `repos/${target}/check-runs/${checkId}`, + '--jq', '{id,name,status,conclusion,head_sha,details_url,app:{id:.app.id,slug:.app.slug}}', + ]); + if (check.id !== checkId + || check.name !== 'Squad Review / review' + || check.head_sha !== castPr.headSha + || check.status !== 'completed' + || check.conclusion !== 'success' + || check.app?.id !== 15368 + || check.app?.slug !== 'github-actions') { + throw new Error('Native Squad Review required check is not bound to the trusted review job.'); } writeJson(resolve(evidence, 'base-controlled-review-canary.json'), { pull_request: castPr, - observed_required_check: observedRequiredCheck(check, attestation), + run, + job, check, review: verdicts[0], verdict, - attestation, }); - return { check, verdict }; + return { run, job, check, verdict }; } sleep(10_000); } @@ -937,7 +829,6 @@ function hosted(args, repositoryRoot) { const reviewCanary = waitForBaseControlledReviewCanary( targetState.target, outputs.castPr, - targetState.reviewApp, evidence, ); diff --git a/templates/skills/gh-aw-enlistment/SKILL.md b/templates/skills/gh-aw-enlistment/SKILL.md index 9005f105e..a8fd5fc76 100644 --- a/templates/skills/gh-aw-enlistment/SKILL.md +++ b/templates/skills/gh-aw-enlistment/SKILL.md @@ -182,17 +182,17 @@ uses a moving branch reference. On a clean repo, `gh aw add` reports these expected safe updates and **nothing else**: -- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`**, - **`SQUAD_GITHUB_TOKEN`**, and **`SQUAD_REVIEW_APP_PRIVATE_KEY`** +- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`** and **`SQUAD_GITHUB_TOKEN`** - Action: **`bradygaster/squad/.github/actions/squad-init`** -> **Compilation approval is not credential provisioning.** `gh aw add` lists -> referenced names so you can approve the surface. `SQUAD_GITHUB_APP_PRIVATE_KEY` -> and `SQUAD_GITHUB_TOKEN` remain optional activation credentials. -> `SQUAD_REVIEW_APP_PRIVATE_KEY` is different: trusted check publication requires -> it in the exact-default-branch `squad-review-authority` environment. Never put -> reviewer credentials at repository scope and never fall back to `github.token`. +> **These are referenced names, not prerequisites.** `gh aw add` lists the secrets +> the workflows *reference* so you can approve that surface — it is not asking you +> to supply them. Both secrets are optional, they need not exist, and neither is +> required to enlist a repository. Single-repo activation runs on the built-in +> `github.token`. Configure them only for cross-repo access or elevated +> permissions. Auth precedence: GitHub App token, then the PAT, then +> `github.token`. Never block an enlistment waiting for a credential. If — and only if — the report contains exactly those documented entries, complete the one-time approval: @@ -284,25 +284,30 @@ gh pr checks --watch `main`. - Request Copilot review, address feedback, and wait for required checks. -### 9. Verify the external review-authority prerequisite when API access permits - -Before claiming trusted review is ready, query the repository environment. It -must be named `squad-review-authority`, use custom deployment branch policies, -allow exactly the captured `${default_branch}`, contain the environment secret -`SQUAD_REVIEW_APP_PRIVATE_KEY`, and define environment variables -`SQUAD_REVIEW_APP_ID`, `SQUAD_REVIEW_APP_SLUG`, and -`SQUAD_REVIEW_APP_OWNER`. The App ID/slug must differ from -`15368`/`github-actions`. All four reviewer-specific names must also be absent -from repository-level Actions secrets and variables; repository fallback does -not satisfy this gate. - -If the authenticated account cannot read environment configuration, or any -requirement is absent, report the exact external prerequisite and do not claim -the required check is trusted. The installation PR remains a human boundary; -after it merges, the Cast canary intentionally fails closed until an -administrator provisions the environment and reruns the base-controlled -review. Configure a ruleset only from the canary check's observed App -integration ID, never from user input or a guessed value. +### 9. Verify the native review contract after merge + +The installation PR remains an explicit human trust boundary. After a human +merges it, inspect the automatically opened Cast PR and require the native +`Squad Review / review` job from the base-controlled `pull_request_target` +workflow to succeed. Verify the exact workflow path, immutable base/workflow +SHA, PR base/head, run ID and attempt, successful `review` job, and exact-head +GitHub Actions check-run binding. + +This review path uses only the native GitHub Actions/gh-aw runtime identity. +Never request or provision a reviewer PAT, GitHub App, private key, secret, +environment, hosted attestor, callback, or external service. The optional +`SQUAD_GITHUB_APP_*` and `SQUAD_GITHUB_TOKEN` activation credentials remain +unrelated to reviewer authority. + +For authoritative merge enforcement, use a source-bound required-workflow or +equivalent ruleset when available. A context-only requirement for +`Squad Review / review` is advisory because a PR-controlled workflow may be +able to emit the same workflow/job name through the shared GitHub Actions +identity. If source-bound enforcement is unavailable, retain independent human +review rather than relying on that context alone or adding a credential-based +publisher. Require branches to be up to date before merge (or use an equivalent +merge-queue freshness guarantee), and enable the required context only after +the workflow installation is merged and the post-merge Cast canary succeeds. ### 10. Never auto-merge — and explain what comes next @@ -366,9 +371,8 @@ gh pr edit --add-reviewer @copilot # then wait for review + checks; DO NOT mer ### ✓ Correct: STOP on an undocumented safe-update entry ```text -gh aw add reports an additional secret: `ACME_DEPLOY_KEY`. -→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN, - SQUAD_REVIEW_APP_PRIVATE_KEY) +gh aw add reports a third secret: `ACME_DEPLOY_KEY`. +→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN) and is NOT the squad-init action. Do NOT run `--approve`. Halt, report "unexpected safe-update entry: ACME_DEPLOY_KEY", and wait. ``` @@ -402,8 +406,7 @@ gh pr merge --squash # auto-merge before human review. NEVER. - ❌ **Blanket staging** (`git add .` / `-A` / `git commit -a`). Stage only `.gitattributes`, `.github/aw/`, `.github/workflows/`, `.github/skills/`, by path. - ❌ **Approving unknown safe updates.** Approve ONLY `SQUAD_GITHUB_APP_PRIVATE_KEY`, - `SQUAD_GITHUB_TOKEN`, `SQUAD_REVIEW_APP_PRIVATE_KEY`, and - `bradygaster/squad/.github/actions/squad-init`. Anything + `SQUAD_GITHUB_TOKEN`, and `bradygaster/squad/.github/actions/squad-init`. Anything else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). diff --git a/test/gh-aw-enlistment-skill.test.ts b/test/gh-aw-enlistment-skill.test.ts index 5d400576e..bf895d7b4 100644 --- a/test/gh-aw-enlistment-skill.test.ts +++ b/test/gh-aw-enlistment-skill.test.ts @@ -115,11 +115,10 @@ describe('gh-aw-enlistment skill', () => { // Every backtick-delimited token in the region — order-independent exact set. const tokens = [...region.matchAll(/`([^`]+)`/g)].map(m => m[1]); - expect(tokens.slice().sort(), 'allowlist tokens must be exactly the four documented entries').toEqual( + expect(tokens.slice().sort(), 'allowlist tokens must be exactly the three documented entries').toEqual( [ 'SQUAD_GITHUB_APP_PRIVATE_KEY', 'SQUAD_GITHUB_TOKEN', - 'SQUAD_REVIEW_APP_PRIVATE_KEY', 'bradygaster/squad/.github/actions/squad-init', ].sort() ); @@ -187,6 +186,13 @@ describe('gh-aw-enlistment skill', () => { expect(content).toContain('default_workflow_permissions=read'); }); + it('uses native review authority without reviewer credentials or external services', () => { + expect(content).toContain('native GitHub Actions/gh-aw runtime identity'); + expect(content).toContain('successful `review` job'); + expect(content).toContain('context-only requirement'); + expect(content).not.toMatch(/SQUAD_REVIEW_APP_|squad-review-authority/); + }); + it('requires GitHub Issues before installation and stops when they cannot be enabled', () => { expect(content).toContain('gh api "repos/${owner_repo}" --jq \'.has_issues\''); expect(content).toContain( @@ -303,6 +309,17 @@ describe('gh-aw-enlistment skill', () => { ); }); + it('documents the native required review job without a credential prerequisite', () => { + const flatGuide = guide.replace(/\s+/g, ' '); + expect(flatGuide).toContain('stable required status context `Squad Review / review`'); + expect(guide).toContain('No custom Checks API publisher'); + expect(flatGuide).toContain('same-name source-identity question'); + expect(flatGuide).toContain('context-only `Squad Review / review` requirement is advisory'); + expect(agentGuide).toContain('requires no separate PAT'); + expect(guide).not.toMatch(/SQUAD_REVIEW_APP_|squad-review-authority/); + expect(agentGuide).not.toMatch(/SQUAD_REVIEW_APP_|squad-review-authority/); + }); + it('makes public-guide package verification fail fast and coherent', () => { expect(guide).toContain('gh aw add "bradygaster/squad/workflows@${SQUAD_SHA}"'); expect(guide).toContain('squad-install-verifier.mjs --materialize-runtime'); diff --git a/test/gh-aw-hosted-e2e.test.ts b/test/gh-aw-hosted-e2e.test.ts index 9728a0bf7..2872cac3b 100644 --- a/test/gh-aw-hosted-e2e.test.ts +++ b/test/gh-aw-hosted-e2e.test.ts @@ -6,10 +6,8 @@ import { parse } from 'yaml'; import { assertPristineTarget, guardedTargetMutation, - observedRequiredCheck, sanitizedEnvironment, selectBootstrapOutputs, - validateReviewAuthorityEnvironment, waitForBootstrapOutputs, } from '../scripts/gh-aw-hosted-e2e.mjs'; import { @@ -219,103 +217,17 @@ describe('Squad gh-aw hosted E2E controller', () => { it('requires a base-controlled review canary after the manual installation boundary', () => { expect(SCRIPT).toContain('waitForBaseControlledReviewCanary'); - expect(SCRIPT).toContain("check.name === 'Squad Review / review'"); - expect(SCRIPT).toContain('check.app?.id === expectedApp.id'); - expect(SCRIPT).toContain('check.app?.slug === expectedApp.slug'); - expect(SCRIPT).toContain('observed_required_check'); - expect(SCRIPT).toContain('integration_id: check.app.id'); + expect(SCRIPT).toContain("job.name === 'review'"); + expect(SCRIPT).toContain('job.check_run_url'); + expect(SCRIPT).toContain("check.name !== 'Squad Review / review'"); + expect(SCRIPT).toContain("check.app?.slug !== 'github-actions'"); expect(SCRIPT).toContain("'@squad/base-controlled-bootstrap'"); expect(SCRIPT).toContain("'@squad/base-controlled-review'"); expect(SCRIPT).toContain("verdict.result !== 'COMMENT'"); expect(SCRIPT.indexOf('waitForBaseControlledReviewCanary')).toBeLessThan( SCRIPT.indexOf("kind: 'squad-bootstrap-trigger-probe'"), ); - }); - - it('requires a branch-restricted environment with reviewer-only App credentials', () => { - const environment = { - name: 'squad-review-authority', - deployment_branch_policy: { - custom_branch_policies: true, - protected_branches: false, - }, - }; - const policies = [{ type: 'branch', name: 'main' }]; - const secrets = ['SQUAD_REVIEW_APP_PRIVATE_KEY']; - const variables = [ - { name: 'SQUAD_REVIEW_APP_ID', value: '424242' }, - { name: 'SQUAD_REVIEW_APP_SLUG', value: 'squad-review-authority' }, - { name: 'SQUAD_REVIEW_APP_OWNER', value: 'owner' }, - ]; - expect(validateReviewAuthorityEnvironment( - environment, - policies, - secrets, - variables, - [], - [], - 'main', - 'owner', - )).toEqual({ id: 424242, slug: 'squad-review-authority', owner: 'owner' }); - - const invalid = [ - [undefined, policies, secrets, variables, [], []], - [{ ...environment, deployment_branch_policy: null }, policies, secrets, variables, [], []], - [environment, [], secrets, variables, [], []], - [environment, [{ type: 'branch', name: '*' }], secrets, variables, [], []], - [environment, [...policies, { type: 'branch', name: 'release' }], secrets, variables, [], []], - [environment, policies, [], variables, [], []], - [environment, policies, secrets, - variables.filter(({ name }) => name !== 'SQUAD_REVIEW_APP_ID'), [], []], - [environment, policies, secrets, variables.map(variable => - variable.name === 'SQUAD_REVIEW_APP_ID' ? { ...variable, value: '15368' } : variable), [], []], - [environment, policies, secrets, variables.map(variable => - variable.name === 'SQUAD_REVIEW_APP_SLUG' - ? { ...variable, value: 'github-actions' } - : variable), [], []], - [environment, policies, secrets, variables.map(variable => - variable.name === 'SQUAD_REVIEW_APP_OWNER' - ? { ...variable, value: 'attacker' } - : variable), [], []], - [environment, policies, secrets, variables, ['SQUAD_REVIEW_APP_PRIVATE_KEY'], []], - [environment, policies, secrets, variables, [], ['SQUAD_REVIEW_APP_ID']], - ] as const; - for (const args of invalid) { - expect(() => validateReviewAuthorityEnvironment( - args[0], - args[1], - args[2], - args[3], - args[4], - args[5], - 'main', - 'owner', - )).toThrow(); - } - }); - - it('derives ruleset integration only from the observed dedicated canary check', () => { - const check = { - name: 'Squad Review / review', - app: { id: 424242, slug: 'squad-review-authority' }, - }; - const attestation = { - publisher_app_id: 424242, - publisher_app_slug: 'squad-review-authority', - }; - expect(observedRequiredCheck(check, attestation)).toEqual({ - context: 'Squad Review / review', - integration_id: 424242, - integration_slug: 'squad-review-authority', - }); - expect(() => observedRequiredCheck( - { ...check, app: { id: 15368, slug: 'github-actions' } }, - { publisher_app_id: 15368, publisher_app_slug: 'github-actions' }, - )).toThrow(); - expect(() => observedRequiredCheck(check, { - ...attestation, - publisher_app_id: 1, - })).toThrow(); + expect(SCRIPT).not.toMatch(/SQUAD_REVIEW_APP_|squad-review-authority|publisher_app_/); }); it('never executes candidate package or installed verifier with inherited credentials', () => { diff --git a/test/gh-aw-review-guard.test.ts b/test/gh-aw-review-guard.test.ts index 42cddf84a..4951ce6cb 100644 --- a/test/gh-aw-review-guard.test.ts +++ b/test/gh-aw-review-guard.test.ts @@ -3,7 +3,7 @@ import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { - assertClearingReview, CHECK_NAME, enforceReviewOutputs, OVERRIDE_PREFIX, + assertClearingReview, enforceReviewOutputs, OVERRIDE_PREFIX, reviewTarget, validateAttribution, validateVerdict, VERDICT_PREFIX, } from '../workflows/shared/squad-review-guard.mjs'; @@ -16,8 +16,6 @@ const SUBMITTED = '2026-09-28T12:01:01Z'; const FINISHED = '2026-09-28T12:02:00Z'; const MERGED = '2026-09-28T12:03:00Z'; const NOW = Date.parse('2026-09-28T12:04:00Z'); -const REVIEW_APP_ID = 424242; -const REVIEW_APP_SLUG = 'squad-review-authority'; const workspaces: string[] = []; afterEach(() => { @@ -69,42 +67,16 @@ function fixture(relay = false) { const run = { event: 'pull_request_target', path: '.github/workflows/squad-review.lock.yml', display_title: 'Squad review \u2014 PR #42', - repository: { full_name: REPOSITORY }, head_sha: BASE, run_attempt: 1, + repository: { full_name: REPOSITORY }, head_sha: HEAD, 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', }; const jobs = [{ - name: 'Squad Review Authority / attest', - conclusion: 'success', - completed_at: FINISHED, - }]; - const checkRuns = [{ - id: 88, - name: CHECK_NAME, - external_id: `squad-review-authority/v1:${REPOSITORY}:42:${BASE}:${HEAD}`, - head_sha: HEAD, + name: 'review', status: 'completed', conclusion: 'success', + started_at: START, completed_at: FINISHED, - details_url: `https://github.com/${REPOSITORY}/actions/runs/17`, - app: { id: REVIEW_APP_ID, slug: REVIEW_APP_SLUG }, - output: { - summary: JSON.stringify({ - schema: 'squad-review-check/v1', - repository: REPOSITORY, - pull_request: 42, - base_sha: BASE, - head_sha: HEAD, - workflow_sha: BASE, - run_id: 17, - run_attempt: 1, - event: 'pull_request_target', - workflow_path: '.github/workflows/squad-review.lock.yml', - authority_job: 'Squad Review Authority / attest', - publisher_app_id: REVIEW_APP_ID, - publisher_app_slug: REVIEW_APP_SLUG, - }), - }, }]; const bootstrapRun = { event: 'push', path: '.github/workflows/squad-bootstrap.lock.yml', @@ -156,7 +128,6 @@ function fixture(relay = false) { 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 }; - if (route.endsWith(`/commits/${HEAD}/check-runs`)) return { check_runs: checkRuns }; if (route.endsWith('/comments')) return state.comments; if (route.endsWith('/permission')) return { permission: state.permission }; throw new Error(`Unexpected API route: ${route}`); @@ -167,7 +138,7 @@ function fixture(relay = false) { }; sync(); return { - env, attribution, registry, pr, verdict, review, run, bootstrapRun, jobs, checkRuns, + env, attribution, registry, pr, verdict, review, run, bootstrapRun, jobs, override, comment, state, encode, get, sync, }; @@ -286,39 +257,11 @@ describe('independent Squad review guard', () => { } }); - it('ignores an ordinary-token forged check and accepts only the dedicated App check', async () => { - const f = fixture(true); - f.checkRuns.unshift({ - ...structuredClone(f.checkRuns[0]), - id: 87, - app: { id: 15368, slug: 'github-actions' }, - output: { - summary: JSON.stringify({ - ...JSON.parse(f.checkRuns[0].output.summary), - publisher_app_id: 15368, - publisher_app_slug: 'github-actions', - }), - }, - }); - await expect(assertClearingReview(f.env, f.get, { relay: true })) - .resolves.toEqual(f.verdict); - f.checkRuns.splice(1); - await expect(assertClearingReview(f.env, f.get, { relay: true })) - .rejects.toThrow('missing or duplicate exact-head authority check'); - }); - - it('refuses PR-controlled shaped review, job, check, and artifact evidence', async () => { + it('refuses PR-controlled shaped review and job evidence from another run', async () => { const f = fixture(true); f.run.event = 'pull_request'; f.run.head_sha = HEAD; - f.jobs[0].name = CHECK_NAME; - f.checkRuns[0].external_id = - `squad-review-authority/v1:${REPOSITORY}:42:${BASE}:${HEAD}`; - f.checkRuns[0].output.summary = JSON.stringify({ - ...JSON.parse(f.checkRuns[0].output.summary), - event: 'pull_request', - workflow_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'); }); @@ -378,10 +321,8 @@ describe('independent Squad review guard', () => { 'missing', 'duplicate', 'malformed', 'native-rejected', 'dismissed', 'foreign-bot', 'wrong-native-sha', 'future-submission', 'missing-manifest', 'bad-marker', 'fenced-marker', 'wrong-branch', 'duplicate-marker', 'stale-head', 'foreign-head', 'foreign-base', 'head-race', - 'not-merged', 'wrong-base', 'merge-before-review', 'merge-before-check', - 'failed-run', 'failed-check', 'missing-check', 'duplicate-check', 'wrong-check', - 'forged-check', 'actions-app-check', 'wrong-check-app', 'wrong-check-app-slug', - 'wrong-check-attestation', 'wrong-attested-app-id', 'wrong-attested-app-slug', + 'not-merged', 'wrong-base', 'merge-before-review', 'merge-before-job', + 'failed-run', 'failed-job', 'missing-job', 'duplicate-job', 'wrong-job', 'manual-run', 'wrong-workflow', 'wrong-run-repo', 'wrong-run-sha', 'wrong-run-pr', 'wrong-run-attempt', 'wrong-run-title', 'verdict-before-run', 'wrong-bot-id', 'wrong-bot-type', @@ -408,45 +349,22 @@ describe('independent Squad review guard', () => { case 'not-merged': f.pr.merged = false; break; case 'wrong-base': f.pr.base.ref = 'other'; break; case 'merge-before-review': f.pr.merged_at = START; break; - case 'merge-before-check': f.pr.merged_at = SUBMITTED; break; + case 'merge-before-job': f.pr.merged_at = SUBMITTED; break; case 'failed-run': f.run.conclusion = 'failure'; break; - case 'failed-check': f.jobs[0].conclusion = 'failure'; break; - case 'missing-check': f.jobs.splice(0); break; - case 'duplicate-check': f.jobs.push(f.jobs[0]); break; - case 'wrong-check': f.jobs[0].name = 'Squad Review / manual'; break; + case 'failed-job': f.jobs[0].conclusion = 'failure'; break; + case 'missing-job': f.jobs.splice(0); break; + case 'duplicate-job': f.jobs.push(f.jobs[0]); break; + case 'wrong-job': f.jobs[0].name = 'manual'; break; 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 = HEAD; break; + case 'wrong-run-sha': f.run.head_sha = BASE; 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; case 'verdict-before-run': f.run.run_started_at = FINISHED; break; case 'wrong-bot-id': f.review.user.id = 1; break; case 'wrong-bot-type': f.review.user.type = 'User'; break; - case 'forged-check': f.checkRuns[0].external_id = 'forged'; break; - case 'actions-app-check': - f.checkRuns[0].app = { id: 15368, slug: 'github-actions' }; - f.checkRuns[0].output.summary = JSON.stringify({ - ...JSON.parse(f.checkRuns[0].output.summary), - publisher_app_id: 15368, - publisher_app_slug: 'github-actions', - }); - break; - case 'wrong-check-app': f.checkRuns[0].app.id = 1; break; - case 'wrong-check-app-slug': f.checkRuns[0].app.slug = 'other-review-app'; break; - case 'wrong-attested-app-id': f.checkRuns[0].output.summary = JSON.stringify({ - ...JSON.parse(f.checkRuns[0].output.summary), - publisher_app_id: 1, - }); break; - case 'wrong-attested-app-slug': f.checkRuns[0].output.summary = JSON.stringify({ - ...JSON.parse(f.checkRuns[0].output.summary), - publisher_app_slug: 'other-review-app', - }); break; - case 'wrong-check-attestation': f.checkRuns[0].output.summary = JSON.stringify({ - ...JSON.parse(f.checkRuns[0].output.summary), - event: 'pull_request', - }); break; } await expect(assertClearingReview(f.env, f.get, { relay: true })).rejects.toThrow(); }); @@ -467,20 +385,15 @@ describe('independent Squad review guard', () => { await expect(assertClearingReview(f.env, f.get, { relay: true })).rejects.toThrow('bound'); }); - it('clears a rejection only through a human-admin SHA-scoped override, including reruns', async () => { + it('clears a rejection only through a human-admin SHA-scoped override bound to its run attempt', async () => { const f = fixture(true); f.verdict.result = 'REQUEST_CHANGES'; f.sync(); await expect(assertClearingReview(f.env, f.get, { relay: true })).rejects.toThrow('override'); f.state.comments = [f.comment]; f.state.comments.push({ ...f.comment, body: `${OVERRIDE_PREFIX}${JSON.stringify({ ...f.override, head_sha: BASE })}` }); - f.run.run_attempt = 2; - f.checkRuns[0].output.summary = JSON.stringify({ - ...JSON.parse(f.checkRuns[0].output.summary), - run_attempt: 2, - }); await expect(assertClearingReview(f.env, f.get, { relay: true })).resolves.toEqual(f.verdict); - expect(f.state.calls.some(route => route.endsWith('/attempts/2/jobs'))).toBe(true); + expect(f.state.calls.some(route => route.endsWith('/attempts/1/jobs'))).toBe(true); }); it.each([ @@ -573,25 +486,26 @@ describe('independent Squad review guard', () => { expect(JSON.parse(readFileSync(path, 'utf8')).items[0].body).toBe('Diagnostic.'); }); - it('deduplicates only evidence from the exact run attempt', async () => { + it('rejects pre-seeded exact-attempt evidence and requires fresh safe output', async () => { const f = fixture(); const workspace = mkdtempSync(join(tmpdir(), 'squad-review-dedup-')); workspaces.push(workspace); const path = join(workspace, 'output.json'); const env = { ...f.env, GH_AW_AGENT_OUTPUT: path }; writeFileSync(path, JSON.stringify({ items: [{ type: 'noop' }] })); - await expect(enforceReviewOutputs(env, f.get)).resolves.toBeUndefined(); + await expect(enforceReviewOutputs(env, f.get)) + .rejects.toThrow('pre-existing verdict evidence'); writeFileSync(path, JSON.stringify({ items: [{ - type: 'submit_pull_request_review', event: 'COMMENT', body: 'Duplicate.', + type: 'submit_pull_request_review', event: 'COMMENT', body: 'Forged duplicate.', }] })); - await expect(enforceReviewOutputs(env, f.get)).rejects.toThrow('already has a verdict'); - f.verdict.run_attempt = 2; - f.sync(); + await expect(enforceReviewOutputs(env, f.get)) + .rejects.toThrow('pre-existing verdict evidence'); + env.GITHUB_RUN_ATTEMPT = '2'; writeFileSync(path, JSON.stringify({ items: [{ type: 'submit_pull_request_review', event: 'COMMENT', body: 'New attempt.', }] })); await expect(enforceReviewOutputs(env, f.get)).resolves.toBeUndefined(); - expect(readFileSync(path, 'utf8')).toContain('\\"run_attempt\\":1'); + expect(readFileSync(path, 'utf8')).toContain('\\"run_attempt\\":2'); f.review.body = `${VERDICT_PREFIX}{broken`; writeFileSync(path, JSON.stringify({ items: [{ type: 'noop' }] })); await expect(enforceReviewOutputs(env, f.get)).rejects.toThrow(); diff --git a/test/gh-aw-review-workflow.test.ts b/test/gh-aw-review-workflow.test.ts index ee6bc7700..5ef4178a2 100644 --- a/test/gh-aw-review-workflow.test.ts +++ b/test/gh-aw-review-workflow.test.ts @@ -113,9 +113,9 @@ function assertReviewerContract(workflow: string): void { expect(workflow).toContain('inlined-imports: true'); expect(workflow).toContain('await guard.enforceReviewOutputs'); expect(workflow).toContain('await guard.assertClearingReview'); - expect(workflow).toContain('Squad Review Authority / attest'); - expect(workflow).toContain('Publish exact-head required check'); - expect(workflow).toContain("run.event !== 'pull_request_target'"); + expect(workflow).toContain('name: review'); + expect(workflow).not.toContain('Publish exact-head required check'); + expect(workflow).not.toMatch(/SQUAD_REVIEW_APP_|squad-review-authority|checks:\s*write/); expect(workflow).toContain('The workflow-installation PR is an explicit human trust boundary.'); expect(workflow).toContain('Trusted review starts on the automatic post-merge Cast PR.'); expect(workflow).toContain('Human approval remains mandatory.'); @@ -126,84 +126,6 @@ interface CompiledContract { safeOutputs: Record>; } -async function executePublisher( - script: string, - { - publishedApp, - existing = [], - updateError, - }: { - publishedApp: { id: number; slug: string }; - existing?: Array>; - updateError?: Error; - }, -): Promise<{ failures: string[]; created: number; updated: number; deleted: number }> { - const failures: string[] = []; - let created = 0; - let updated = 0; - let deleted = 0; - const listJobs = async () => undefined; - const listChecks = async () => undefined; - const github = { - paginate: async (method: unknown) => method === listJobs - ? [{ name: 'Squad Review Authority / attest', conclusion: 'success' }] - : existing, - rest: { - actions: { - getWorkflowRun: async () => ({ data: { - event: 'pull_request_target', - path: '.github/workflows/squad-review.lock.yml', - head_sha: 'b'.repeat(40), - repository: { full_name: 'squad/example' }, - } }), - listJobsForWorkflowRunAttempt: listJobs, - }, - checks: { - listForRef: listChecks, - create: async () => { - created++; - return { data: { id: 88, app: publishedApp } }; - }, - update: async () => { - updated++; - if (updateError) throw updateError; - return { data: { id: 88, app: publishedApp } }; - }, - delete: async () => { - deleted++; - return { data: {} }; - }, - }, - }, - }; - const core = { setFailed: (message: string) => failures.push(message) }; - const previous = { ...process.env }; - Object.assign(process.env, { - AUTHORITY_RESULT: 'success', - GITHUB_REPOSITORY: 'squad/example', - GITHUB_RUN_ID: '17', - GITHUB_RUN_ATTEMPT: '1', - GITHUB_SERVER_URL: 'https://github.com', - SQUAD_REVIEW_APP_ID: '424242', - SQUAD_REVIEW_APP_SLUG: 'squad-review-authority', - SQUAD_REVIEW_PR: '42', - SQUAD_REVIEW_BASE: 'b'.repeat(40), - SQUAD_REVIEW_HEAD: 'a'.repeat(40), - SQUAD_REVIEW_WORKFLOW_SHA: 'b'.repeat(40), - }); - try { - const run = new Function( - 'github', - 'core', - `return (async () => {\n${script}\n})();`, - ) as (github: unknown, core: unknown) => Promise; - await run(github, core); - } finally { - process.env = previous; - } - return { failures, created, updated, deleted }; -} - function compileReviewer(workflow = REVIEWER): CompiledContract { const workspace = mkdtempSync(resolve(ROOT, '.squad-review-contract-')); compileWorkspaces.push(workspace); @@ -241,8 +163,11 @@ function assertCompiledGate(lock: string): void { expect(workflow.on).not.toHaveProperty('pull_request'); expect(workflow['run-name']).toBe('Squad review — PR #${{ github.event.pull_request.number }}'); const review = jobs.review; - expect(review.name).toBe('Squad Review Authority / attest'); + expect(review.name).toBe('review'); expect(review.if).toBe('always()'); + expect(JSON.stringify(review)).not.toContain( + 'github.event.pull_request.head.repo.full_name == github.repository', + ); expect(review.needs).toEqual(expect.arrayContaining(['agent', 'safe_outputs'])); const execution = review.steps.find((step: { name: string }) => step.name === 'Require successful PR review execution'); expect(execution.run).toContain('test "$AGENT_RESULT" = success'); @@ -279,44 +204,11 @@ function assertCompiledGate(lock: string): void { expect(finalGate.with.script).not.toContain('Squad-Review-Verdict:'); for (const step of review.steps) expect(step['continue-on-error']).toBeUndefined(); expect(review['continue-on-error']).toBeUndefined(); - const publish = jobs.publish; - expect(publish.name).toBe('Publish Squad Review required check'); - expect(publish.if).toBe('always()'); - expect(publish.needs).toEqual(expect.arrayContaining(['agent', 'safe_outputs', 'review'])); - expect(publish.environment).toBe('squad-review-authority'); - expect(publish.permissions).toMatchObject({ - actions: 'read', - contents: 'read', - 'pull-requests': 'read', - }); - expect(publish.permissions.checks).toBeUndefined(); - const validateStep = publish.steps.find((step: { name: string }) => - step.name === 'Validate dedicated reviewer App configuration'); - expect(validateStep.env.SQUAD_REVIEW_APP_ID).toBe('${{ vars.SQUAD_REVIEW_APP_ID }}'); - expect(validateStep.env.SQUAD_REVIEW_APP_SLUG).toBe('${{ vars.SQUAD_REVIEW_APP_SLUG }}'); - expect(validateStep.env.SQUAD_REVIEW_APP_OWNER).toBe('${{ vars.SQUAD_REVIEW_APP_OWNER }}'); - expect(validateStep.env.SQUAD_REVIEW_APP_PRIVATE_KEY) - .toBe('${{ secrets.SQUAD_REVIEW_APP_PRIVATE_KEY }}'); - expect(validateStep.run).toContain('test "${SQUAD_REVIEW_APP_ID}" != "15368"'); - expect(validateStep.run).toContain('test "${SQUAD_REVIEW_APP_SLUG}" != "github-actions"'); - const mintStep = publish.steps.find((step: { name: string }) => - step.name === 'Mint dedicated reviewer App token'); - expect(mintStep.uses).toMatch(/^actions\/create-github-app-token@[0-9a-f]{40}$/); - expect(mintStep['continue-on-error']).toBeUndefined(); - const publishStep = publish.steps.find((step: { name: string }) => - step.name === 'Publish exact-head required check'); - expect(publishStep.with['github-token']) - .toBe('${{ steps.squad-review-app-token.outputs.token }}'); - expect(publishStep.with.script).toContain("const checkName = 'Squad Review / review'"); - expect(publishStep.with.script).toContain("run.event !== 'pull_request_target'"); - expect(publishStep.with.script).toContain('run.head_sha !== workflowSha'); - expect(publishStep.with.script).toContain('authorityJobs.length !== 1'); - expect(publishStep.with.script).toContain("external_id: externalId"); - expect(publishStep.with.script).toContain("head_sha: headSha"); - expect(publishStep.with.script).toContain('published.data.app?.id !== expectedAppId'); - expect(publishStep.with.script).toContain('publisher_app_id: expectedAppId'); - expect(publishStep.with.script).toContain('publisher_app_slug: expectedAppSlug'); - expect(publishStep.with.script).not.toContain('github.event.pull_request.head.ref'); + expect(jobs.publish).toBeUndefined(); + expect(lock).not.toMatch(/SQUAD_REVIEW_APP_|squad-review-authority/); + expect(lock).not.toMatch(/checks:\s*write/); + expect(lock).not.toContain('actions/create-github-app-token'); + expect(lock).not.toContain('github.rest.checks.'); expect(lock).not.toContain('Checkout PR branch'); expect(lock).not.toMatch(/uses:\s+\.\/\.github\/actions\//); } @@ -400,42 +292,6 @@ async function executeCompiledGuardLoader( } describe('gh-aw enforcing Squad reviewer', () => { - it('rejects ordinary-token publication and accepts the dedicated App identity', async () => { - const { lock } = compileReviewer(); - const workflow = parse(lock); - const publishStep = workflow.jobs.publish.steps.find((step: { name: string }) => - step.name === 'Publish exact-head required check'); - const ordinary = await executePublisher(publishStep.with.script, { - publishedApp: { id: 15368, slug: 'github-actions' }, - }); - expect(ordinary).toEqual({ - failures: ['Squad review check was not published by the configured dedicated GitHub App.'], - created: 1, - updated: 0, - deleted: 1, - }); - const dedicated = await executePublisher(publishStep.with.script, { - publishedApp: { id: 424242, slug: 'squad-review-authority' }, - }); - expect(dedicated).toEqual({ failures: [], created: 1, updated: 0, deleted: 0 }); - }); - - it('cannot update the dedicated App check with an ordinary workflow token', async () => { - const { lock } = compileReviewer(); - const workflow = parse(lock); - const publishStep = workflow.jobs.publish.steps.find((step: { name: string }) => - step.name === 'Publish exact-head required check'); - await expect(executePublisher(publishStep.with.script, { - publishedApp: { id: 15368, slug: 'github-actions' }, - existing: [{ - id: 88, - external_id: `squad-review-authority/v1:squad/example:42:${'b'.repeat(40)}:${'a'.repeat(40)}`, - app: { id: 424242, slug: 'squad-review-authority' }, - }], - updateError: Object.assign(new Error('Resource not accessible by integration'), { status: 403 }), - })).rejects.toMatchObject({ status: 403 }); - }); - it('keeps review automatic and makes /squad review a non-dispatching rerun guide', () => { const routerDispatch = yamlBlock(ROUTER_FRONTMATTER, 'dispatch-workflow'); const reviewTrigger = yamlBlock(REVIEWER_FRONTMATTER, 'on'); @@ -448,11 +304,12 @@ describe('gh-aw enforcing Squad reviewer', () => { expect(reviewTrigger).toMatch(/pull_request_target:\n\s+types: \[opened, reopened, ready_for_review, synchronize\]/); expect(reviewTrigger).not.toContain('workflow_dispatch:'); expect(relay).toContain('.github/workflows/squad-review.lock.yml'); - expect(relay).toContain('exact current PR head'); - expect(relay).toMatch(/require\s+exactly one job named `Squad Review Authority \/ attest`/); + expect(relay).toContain('exact current head'); + expect(relay).toMatch(/require exactly one job named `review`/); + expect(relay).not.toContain('check runs'); + expect(relay).not.toContain('dispatch-workflow'); expect(relay).toMatch(/command did not rerun it\./); expect(relay).toMatch(/no base-controlled automatic review run exists for\s+the exact head/); - expect(relay).not.toContain('dispatch-workflow'); }); it('limits slash commands to issue and pull request conversation surfaces', () => { @@ -560,7 +417,7 @@ describe('gh-aw enforcing Squad reviewer', () => { assertReviewerContract(REVIEWER); }); - it('deduplicates by head SHA, cancels stale runs, and retains fork protection', () => { + it('requires fresh attempt-bound evidence, cancels stale runs, and retains fork protection', () => { const concurrency = yamlBlock(REVIEWER_FRONTMATTER, 'concurrency'); expect(concurrency).toContain( @@ -568,9 +425,11 @@ describe('gh-aw enforcing Squad reviewer', () => { ); expect(concurrency).toContain('cancel-in-progress: true'); expect(REVIEWER_FRONTMATTER).not.toMatch(/^\s+forks:/m); - expect(REVIEWER).toContain('If an existing bot review contains a `Squad-Review-Verdict:` record'); - expect(REVIEWER).toContain('Evidence from a different run or attempt cannot deduplicate'); - expect(REVIEWER).toMatch(/Re-fetch the pull request\s+immediately before emitting/); + expect(REVIEWER).toContain('Every workflow run attempt must submit exactly one fresh review.'); + expect(REVIEWER).toMatch(/Never use\s+`noop` to reuse or deduplicate/); + expect(REVIEWER).toContain('rejects any pre-existing verdict evidence'); + expect(REVIEWER).toMatch(/a GitHub\s+Actions rerun receives an incremented attempt/); + expect(REVIEWER).toMatch(/Re-fetch the pull request\s+immediately\s+before emitting/); }); it('covers acceptance, routing, protected files, tests, and changesets', () => { @@ -592,12 +451,25 @@ describe('gh-aw enforcing Squad reviewer', () => { assertCompiledGate(lock); const agentJob = lock.match(/^ agent:\n([\s\S]*?)(?=^ [\w-]+:\n)/m)?.[1] ?? ''; const permissionBlock = yamlBlock(agentJob, 'permissions'); + const workflow = parse(lock); + const safeOutputPermissions = workflow.jobs.safe_outputs.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(safeOutputPermissions).toMatchObject({ + contents: 'read', + issues: 'write', + 'pull-requests': 'write', + }); + expect(workflow.jobs.review.permissions).toMatchObject({ + actions: 'read', + contents: 'read', + issues: 'read', + 'pull-requests': 'read', + }); expect(safeOutputs.add_comment).toMatchObject({ max: 1 }); expect(safeOutputs.create_pull_request_review_comment).toMatchObject({ max: 10 }); expect(safeOutputs.submit_pull_request_review).toMatchObject({ @@ -737,15 +609,11 @@ describe('gh-aw enforcing Squad reviewer', () => { const mutations = [ REVIEWER.replace('await guard.enforceReviewOutputs', 'void guard.enforceReviewOutputs'), REVIEWER.replace('await guard.assertClearingReview', 'void guard.assertClearingReview'), - REVIEWER.replace(' name: Squad Review Authority / attest', ' name: Other check'), + REVIEWER.replace(' name: review', ' name: Other check'), REVIEWER.replace('pathToFileURL(baseGuard)', "pathToFileURL('.squad-review-workflow/guard.mjs')"), REVIEWER.replace('No trusted Squad verdict was emitted.', 'Squad-Review-Verdict: trusted'), REVIEWER.replace(' if: always()\n', ' if: success()\n'), REVIEWER.replace(' needs: [agent, safe_outputs]', ' needs: [agent]'), - REVIEWER.replace( - "run.event !== 'pull_request_target'", - "run.event !== 'pull_request'", - ), REVIEWER.replace( "uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0", "uses: ./.github/actions/pr-controlled", diff --git a/test/standalone-release-workflow.test.ts b/test/standalone-release-workflow.test.ts index 68f7eb2cf..cd0f44125 100644 --- a/test/standalone-release-workflow.test.ts +++ b/test/standalone-release-workflow.test.ts @@ -707,8 +707,6 @@ describe('automated package publication', () => { expect(ghAwGuide).toContain('`.vscode/settings.json`'); expect(ghAwGuide).toContain('SQUAD_GITHUB_APP_PRIVATE_KEY'); expect(ghAwGuide).toContain('SQUAD_GITHUB_TOKEN'); - expect(ghAwGuide).toContain('SQUAD_REVIEW_APP_PRIVATE_KEY'); - expect(ghAwGuide).toContain('squad-review-authority'); expect(ghAwGuide).toContain('both slash-command and `github-actions[bot]`'); expect(ghAwGuide).toContain('The completion comment links the created Cast PR'); expect(ghAwGuide).toContain('application CI is `action_required`'); diff --git a/workflows/package/squad-bootstrap.md b/workflows/package/squad-bootstrap.md index 216ffc9a8..058520aba 100644 --- a/workflows/package/squad-bootstrap.md +++ b/workflows/package/squad-bootstrap.md @@ -151,7 +151,7 @@ pre-agent-steps: node --check "$path" >/dev/null } # BEGIN GENERATED RESOURCE DIGESTS - check_hash "$install_verifier" "4f385df488cc65165e398e499663c015ce63851cb9c938f4c3dd0af38d25f991" + check_hash "$install_verifier" "71418638e37a53f570b13919bf5b232f5999be217dac4efc6b76e91918f85f16" check_hash "$cast_validator" "0988e04aeef316f4d7a0107c902bbbcf6538b8899b9fffba5f62150717967685" check_hash "$bootstrap_validator" "d449b9204f7fad133ff7133c1a30c9381c87e3c0c9d481352819ca93ea1a1dad" # END GENERATED RESOURCE DIGESTS diff --git a/workflows/package/squad-review.md b/workflows/package/squad-review.md index 7ad1673bf..c4ab6dfc3 100644 --- a/workflows/package/squad-review.md +++ b/workflows/package/squad-review.md @@ -80,7 +80,7 @@ safe-outputs: commit-id: ${{ github.event.pull_request.head.sha }} jobs: review: - name: Squad Review Authority / attest + name: review if: always() needs: - agent @@ -128,162 +128,6 @@ jobs: const guard = await import(pathToFileURL(baseGuard).href); await guard.assertClearingReview(process.env, async (route, fields) => (await github.request(`GET /${route}`, fields)).data); - publish: - name: Publish Squad Review required check - if: always() - needs: - - agent - - safe_outputs - - review - runs-on: ubuntu-latest - environment: squad-review-authority - permissions: - actions: read - contents: read - pull-requests: read - steps: - - name: Validate dedicated reviewer App configuration - env: - SQUAD_REVIEW_APP_ID: ${{ vars.SQUAD_REVIEW_APP_ID }} - SQUAD_REVIEW_APP_SLUG: ${{ vars.SQUAD_REVIEW_APP_SLUG }} - SQUAD_REVIEW_APP_OWNER: ${{ vars.SQUAD_REVIEW_APP_OWNER }} - SQUAD_REVIEW_APP_PRIVATE_KEY: ${{ secrets.SQUAD_REVIEW_APP_PRIVATE_KEY }} - run: | - set -euo pipefail - test "${SQUAD_REVIEW_APP_ID}" != "15368" - test "${SQUAD_REVIEW_APP_SLUG}" != "github-actions" - [[ "${SQUAD_REVIEW_APP_ID}" =~ ^[1-9][0-9]*$ ]] - [[ "${SQUAD_REVIEW_APP_SLUG}" =~ ^[a-z0-9][a-z0-9-]*$ ]] - [[ "${SQUAD_REVIEW_APP_OWNER}" =~ ^[A-Za-z0-9_.-]+$ ]] - test -n "${SQUAD_REVIEW_APP_PRIVATE_KEY}" - - name: Mint dedicated reviewer App token - id: squad-review-app-token - uses: actions/create-github-app-token@v3.2.0 - with: - app-id: ${{ vars.SQUAD_REVIEW_APP_ID }} - private-key: ${{ secrets.SQUAD_REVIEW_APP_PRIVATE_KEY }} - owner: ${{ vars.SQUAD_REVIEW_APP_OWNER }} - - name: Publish exact-head required check - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 - env: - AUTHORITY_RESULT: ${{ needs.review.result }} - SQUAD_REVIEW_APP_ID: ${{ vars.SQUAD_REVIEW_APP_ID }} - SQUAD_REVIEW_APP_SLUG: ${{ vars.SQUAD_REVIEW_APP_SLUG }} - SQUAD_REVIEW_PR: ${{ github.event.pull_request.number }} - SQUAD_REVIEW_BASE: ${{ github.event.pull_request.base.sha }} - SQUAD_REVIEW_HEAD: ${{ github.event.pull_request.head.sha }} - SQUAD_REVIEW_WORKFLOW_SHA: ${{ github.workflow_sha }} - with: - github-token: ${{ steps.squad-review-app-token.outputs.token }} - script: | - const checkName = 'Squad Review / review'; - const authorityJob = 'Squad Review Authority / attest'; - const repository = process.env.GITHUB_REPOSITORY; - const [owner, repo] = repository.split('/'); - const expectedAppId = Number(process.env.SQUAD_REVIEW_APP_ID); - const expectedAppSlug = process.env.SQUAD_REVIEW_APP_SLUG; - const pullRequest = Number(process.env.SQUAD_REVIEW_PR); - const baseSha = process.env.SQUAD_REVIEW_BASE; - const headSha = process.env.SQUAD_REVIEW_HEAD; - const workflowSha = process.env.SQUAD_REVIEW_WORKFLOW_SHA; - const runId = Number(process.env.GITHUB_RUN_ID); - const runAttempt = Number(process.env.GITHUB_RUN_ATTEMPT); - if (!Number.isSafeInteger(expectedAppId) || expectedAppId < 1 || - expectedAppId === 15368 || - !/^[a-z0-9][a-z0-9-]*$/.test(expectedAppSlug) || - expectedAppSlug === 'github-actions' || - !Number.isSafeInteger(pullRequest) || pullRequest < 1 || - !/^[0-9a-f]{40}$/.test(baseSha) || !/^[0-9a-f]{40}$/.test(headSha) || - workflowSha !== baseSha || !Number.isSafeInteger(runId) || runId < 1 || - !Number.isSafeInteger(runAttempt) || runAttempt < 1) { - core.setFailed('Invalid base-controlled Squad review check context.'); - return; - } - const run = (await github.rest.actions.getWorkflowRun({ - owner, repo, run_id: runId, - })).data; - if (run.event !== 'pull_request_target' || - run.path !== '.github/workflows/squad-review.lock.yml' || - run.head_sha !== workflowSha || - run.repository.full_name !== repository) { - core.setFailed('Squad review check is not bound to the base-controlled workflow run.'); - return; - } - const jobs = await github.paginate(github.rest.actions.listJobsForWorkflowRunAttempt, { - owner, repo, run_id: runId, attempt_number: runAttempt, per_page: 100, - }); - const authorityJobs = jobs.filter(job => job.name === authorityJob); - if (authorityJobs.length !== 1) { - core.setFailed('Missing or duplicate base-controlled Squad review authority job.'); - return; - } - const succeeded = process.env.AUTHORITY_RESULT === 'success' && - authorityJobs[0].conclusion === 'success'; - const externalId = - `squad-review-authority/v1:${repository}:${pullRequest}:${baseSha}:${headSha}`; - const existing = await github.paginate(github.rest.checks.listForRef, { - owner, repo, ref: headSha, check_name: checkName, per_page: 100, - }); - const owned = existing.filter(check => - check.external_id === externalId && - check.app?.id === expectedAppId && - check.app?.slug === expectedAppSlug); - if (owned.length > 1) { - core.setFailed('Duplicate exact-head Squad review authority checks.'); - return; - } - const detailsUrl = - `${process.env.GITHUB_SERVER_URL}/${repository}/actions/runs/${runId}`; - const output = { - title: succeeded - ? 'Base-controlled Squad review passed' - : 'Base-controlled Squad review did not pass', - summary: JSON.stringify({ - schema: 'squad-review-check/v1', - repository, - pull_request: pullRequest, - base_sha: baseSha, - head_sha: headSha, - workflow_sha: workflowSha, - run_id: runId, - run_attempt: runAttempt, - event: 'pull_request_target', - workflow_path: '.github/workflows/squad-review.lock.yml', - authority_job: authorityJob, - publisher_app_id: expectedAppId, - publisher_app_slug: expectedAppSlug, - }), - }; - const request = { - owner, repo, name: checkName, head_sha: headSha, - status: 'completed', - conclusion: succeeded ? 'success' : 'failure', - details_url: detailsUrl, - external_id: externalId, - output, - }; - const published = owned.length === 1 - ? await github.rest.checks.update({ - ...request, - check_run_id: owned[0].id, - }) - : await github.rest.checks.create(request); - if (published.data.app?.id !== expectedAppId || - published.data.app?.slug !== expectedAppSlug) { - try { - await github.rest.checks.delete({ - owner, repo, check_run_id: published.data.id, - }); - } finally { - core.setFailed( - 'Squad review check was not published by the configured dedicated GitHub App.' - ); - } - return; - } - if (!succeeded) { - core.setFailed('Base-controlled Squad review authority did not succeed.'); - } --- @@ -323,11 +167,11 @@ Read `.squad-review.json` at the exact PR head SHA and the committed `persistent_name` entries in that registry, and must differ. Act as that `reviewer_agent`, reading its base-commit charter. Missing or malformed attribution is a refusal, never a fallback to a display name or GitHub login. -The deterministic output guard and authority job repeat these checks. The -publisher creates `Squad Review / review` on the exact PR head only after the -native `Squad Review Authority / attest` job in the same base-controlled run -succeeds. A refusal, missing verdict, stale head, malformed provenance, or -same-named PR-controlled check cannot clear the authority contract. +The deterministic output guard and final job repeat these checks. GitHub +publishes the native job result as the stable `Squad Review / review` required +status context. The job uses `if: always()` and fails unless the agent, safe +outputs, and exact-head verdict gate all succeed. A refusal, missing verdict, +stale head, or malformed provenance therefore fails the required job. The workflow-installation PR is an explicit human trust boundary. Its base does not contain this workflow, guard, or manifest, so no trusted automatic Squad @@ -377,23 +221,26 @@ human and Copilot PRs) needs the committed stable-agent attribution above to clear the check. `Unattributed` is not permission to skip the gate or fabricate an identity. Malformed marker text remains a refusal on both trigger paths. -## SHA deduplication +## Run-attempt evidence 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 bot review contains a `Squad-Review-Verdict:` record for the -current head SHA and this exact workflow run ID and attempt, call `noop` and -stop. Evidence from a different run or attempt cannot deduplicate or clear this -run. The deterministic gate validates the `pull_request_target` event, immutable -workflow SHA, workflow path, PR, base, head, run, attempt, native authority job, -exact-head published check, and GitHub Actions App/bot identity; an arbitrary -comment, PR-controlled workflow, same-named check, or legacy head marker is not -clearing evidence. Re-fetch the pull request -immediately before emitting outputs; if its head SHA changed, call `noop` and -let the newer run review it. +Every workflow run attempt must submit exactly one fresh review. Never use +`noop` to reuse or deduplicate an existing `Squad-Review-Verdict:` record. +The deterministic safe-output guard rejects any pre-existing verdict evidence +for the current head SHA, workflow run ID, and attempt before it publishes the +new review. Evidence from an earlier run or attempt is not reused; a GitHub +Actions rerun receives an incremented attempt and must publish its own review. +The deterministic gate validates the `pull_request_target` event, immutable +workflow SHA, workflow path, PR, base, head, run, attempt, native `review` job, +and GitHub Actions bot identity; an arbitrary comment, PR-controlled workflow, +same-named job in a different run, attacker-claimed attempt number, or legacy +head marker is not clearing evidence. 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 diff --git a/workflows/package/squad.md b/workflows/package/squad.md index 411000183..2cdd93131 100644 --- a/workflows/package/squad.md +++ b/workflows/package/squad.md @@ -2239,14 +2239,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 workflow -head SHA equals the pull request's exact base SHA. Select the newest run by -creation time, breaking ties by numeric run ID, then fetch its jobs and require -exactly one job named `Squad Review Authority / attest`. Also fetch check runs -for the exact current PR head and require exactly one `Squad Review / review` -check whose external ID, details URL, GitHub Actions App identity, and -`squad-review-check/v1` summary bind that base-controlled run, PR, base SHA, -head SHA, run ID, and attempt. A same-named check without that binding is -advisory only. If no matching run or authority check exists, emit exactly one +head SHA equals the pull request's exact base SHA and whose recorded pull +request head equals the exact current 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 +advisory only. If no matching run or native authority job exists, emit exactly one `add-comment` stating that no base-controlled automatic review run exists for the exact head and that a new PR event (`synchronize`, reopen, or ready-for-review) is required. Stop without implying @@ -2259,8 +2256,8 @@ instruction: > Squad Review is automatic and cannot be dispatched from a branch. In the > selected base-controlled automatic run, choose **Re-run all jobs**. This > command did not rerun it. Only the new attempt of that exact -> `pull_request_target` run can publish verdict evidence and the exact-head -> `Squad Review / review` check for PR #{pull-request-number} +> `pull_request_target` run can publish verdict evidence and complete the native +> `Squad Review / review` required job for PR #{pull-request-number} > at head `{current-head-sha}`. Do not review the diff in this router, emit a verdict, edit files, create an diff --git a/workflows/shared/squad-install-verifier.mjs b/workflows/shared/squad-install-verifier.mjs index 6b7ad970e..34c92aacc 100644 --- a/workflows/shared/squad-install-verifier.mjs +++ b/workflows/shared/squad-install-verifier.mjs @@ -536,20 +536,28 @@ export function checkSource(root) { } } for (const required of [ - 'environment: squad-review-authority', - 'SQUAD_REVIEW_APP_PRIVATE_KEY: ${{ secrets.SQUAD_REVIEW_APP_PRIVATE_KEY }}', - 'github-token: ${{ steps.squad-review-app-token.outputs.token }}', - 'published.data.app?.id !== expectedAppId', - "expectedAppId === 15368", - "expectedAppSlug === 'github-actions'", + 'pull_request_target:', + 'checkout: false', + 'name: review', + 'if: always()', + 'needs: [agent, safe_outputs]', + 'await guard.assertClearingReview', ]) { if (!reviewer.includes(required)) { - failures.push(`Dedicated review authority contract is missing: ${required}`); + failures.push(`Native review authority contract is missing: ${required}`); } } - const publishJob = reviewer.match(/\n publish:\n([\s\S]*?)\n---\n/)?.[1] ?? ''; - if (publishJob.includes('checks: write')) { - failures.push('Dedicated review publisher must not grant checks:write to github.token.'); + for (const forbidden of [ + 'SQUAD_REVIEW_APP_', + 'squad-review-authority', + 'actions/create-github-app-token', + 'checks: write', + 'github.rest.checks.', + '\n publish:\n', + ]) { + if (reviewer.includes(forbidden)) { + failures.push(`Native review authority contains a forbidden publisher surface: ${forbidden}`); + } } } catch (error) { failures.push(error instanceof Error ? error.message : String(error)); diff --git a/workflows/shared/squad-review-guard.mjs b/workflows/shared/squad-review-guard.mjs index e7a11cbc1..13f9fff13 100644 --- a/workflows/shared/squad-review-guard.mjs +++ b/workflows/shared/squad-review-guard.mjs @@ -1,21 +1,15 @@ import { readFileSync, writeFileSync } from 'node:fs'; import { evaluateImplementDispatchInputs } from './squad-retro-provenance.mjs'; -export const CHECK_NAME = 'Squad Review / review'; export const VERDICT_PREFIX = 'Squad-Review-Verdict: '; export const OVERRIDE_PREFIX = 'Squad-Review-Override: '; const WORKFLOW = '.github/workflows/squad-review.lock.yml'; -const AUTHORITY_JOB = 'Squad Review Authority / attest'; -const CHECK_SCHEMA = 'squad-review-check/v1'; -const CHECK_EXTERNAL_ID = 'squad-review-authority/v1'; +const AUTHORITY_JOB = 'review'; const BOOTSTRAP_WORKFLOW = '.github/workflows/squad-bootstrap.lock.yml'; const SHA = /^[0-9a-f]{40}$/; const ID = /^[a-z][a-z0-9-]*$/; const BOT = 'github-actions[bot]'; const BOT_ID = 41898282; -const ACTIONS_APP_ID = 15368; -const ACTIONS_APP_SLUG = 'github-actions'; -const APP_SLUG = /^[a-z0-9][a-z0-9-]*$/; const BOOTSTRAP_BRANCH = 'squad/bootstrap-cast'; const BOOTSTRAP_TITLE = '[squad] Cast your Squad'; const BOOTSTRAP_AUTHOR = '@squad/base-controlled-bootstrap'; @@ -299,11 +293,8 @@ export async function enforceReviewOutputs(env, get, options = {}) { return String(verdict.run_id) === env.GITHUB_RUN_ID && String(verdict.run_attempt) === env.GITHUB_RUN_ATTEMPT; }); - requireThat(currentRun.length <= 1, 'duplicate verdict evidence for this run'); - if (currentRun.length === 1) { - requireThat(verdicts.length === 0, 'unchanged head already has a verdict'); - return; - } + requireThat(currentRun.length === 0, + 'pre-existing verdict evidence for this run attempt is forbidden'); requireThat(verdicts.length === 1, 'exactly one review is required; noop cannot clear review'); const item = verdicts[0]; requireThat(['COMMENT', 'REQUEST_CHANGES'].includes(item.event) && @@ -361,7 +352,7 @@ export async function assertClearingReview(env, get, options = {}) { 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.workflow_sha && + 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) && @@ -372,53 +363,21 @@ export async function assertClearingReview(env, get, options = {}) { 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.workflow_sha && + 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(run.status === 'completed' && run.conclusion === 'success', 'review run did not succeed'); + requireThat(attempt.status === 'completed' && attempt.conclusion === 'success', + 'review run attempt did not succeed'); const jobs = await list(get, - `repos/${target.repository}/actions/runs/${verdict.run_id}/attempts/${run.run_attempt}/jobs`, 'jobs'); + `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'); - const checkRuns = await list(get, - `repos/${target.repository}/commits/${target.head_sha}/check-runs`, 'check_runs'); - const externalId = - `${CHECK_EXTERNAL_ID}:${target.repository}:${target.pull_request}:${target.base_sha}:${target.head_sha}`; - const checks = checkRuns.filter(check => check.name === CHECK_NAME && - check.external_id === externalId && - Number.isSafeInteger(check.app?.id) && - check.app.id > 0 && - check.app.id !== ACTIONS_APP_ID && - typeof check.app?.slug === 'string' && - APP_SLUG.test(check.app.slug) && - check.app.slug !== ACTIONS_APP_SLUG); - requireThat(checks.length === 1, 'missing or duplicate exact-head authority check'); - const check = checks[0]; - requireThat(check.head_sha === target.head_sha && check.status === 'completed' && - check.conclusion === 'success' && - check.details_url === - `${env.GITHUB_SERVER_URL ?? 'https://github.com'}/${target.repository}/actions/runs/${verdict.run_id}`, - 'required review check is not attributable to the dedicated GitHub App'); - const summary = JSON.parse(check.output?.summary ?? ''); - requireThat(exactKeys(summary, [ - 'schema', 'repository', 'pull_request', 'base_sha', 'head_sha', 'workflow_sha', - 'run_id', 'run_attempt', 'event', 'workflow_path', 'authority_job', - 'publisher_app_id', 'publisher_app_slug', - ]) && summary.schema === CHECK_SCHEMA && summary.repository === target.repository && - summary.pull_request === target.pull_request && summary.base_sha === target.base_sha && - summary.head_sha === target.head_sha && summary.workflow_sha === target.workflow_sha && - summary.run_id === verdict.run_id && summary.run_attempt === run.run_attempt && - summary.event === 'pull_request_target' && summary.workflow_path === WORKFLOW && - summary.authority_job === AUTHORITY_JOB && - summary.publisher_app_id === check.app.id && - summary.publisher_app_slug === check.app.slug && - timestamp(check.completed_at) <= cutoff, - 'required review check attestation or dedicated App identity is invalid'); } 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 a2bf36c08..c704a9df6 100644 --- a/workflows/skills/gh-aw-enlistment/SKILL.md +++ b/workflows/skills/gh-aw-enlistment/SKILL.md @@ -182,17 +182,17 @@ uses a moving branch reference. On a clean repo, `gh aw add` reports these expected safe updates and **nothing else**: -- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`**, - **`SQUAD_GITHUB_TOKEN`**, and **`SQUAD_REVIEW_APP_PRIVATE_KEY`** +- Restricted secrets: **`SQUAD_GITHUB_APP_PRIVATE_KEY`** and **`SQUAD_GITHUB_TOKEN`** - Action: **`bradygaster/squad/.github/actions/squad-init`** -> **Compilation approval is not credential provisioning.** `gh aw add` lists -> referenced names so you can approve the surface. `SQUAD_GITHUB_APP_PRIVATE_KEY` -> and `SQUAD_GITHUB_TOKEN` remain optional activation credentials. -> `SQUAD_REVIEW_APP_PRIVATE_KEY` is different: trusted check publication requires -> it in the exact-default-branch `squad-review-authority` environment. Never put -> reviewer credentials at repository scope and never fall back to `github.token`. +> **These are referenced names, not prerequisites.** `gh aw add` lists the secrets +> the workflows *reference* so you can approve that surface — it is not asking you +> to supply them. Both secrets are optional, they need not exist, and neither is +> required to enlist a repository. Single-repo activation runs on the built-in +> `github.token`. Configure them only for cross-repo access or elevated +> permissions. Auth precedence: GitHub App token, then the PAT, then +> `github.token`. Never block an enlistment waiting for a credential. If — and only if — the report contains exactly those documented entries, complete the one-time approval: @@ -284,25 +284,30 @@ gh pr checks --watch `main`. - Request Copilot review, address feedback, and wait for required checks. -### 9. Verify the external review-authority prerequisite when API access permits - -Before claiming trusted review is ready, query the repository environment. It -must be named `squad-review-authority`, use custom deployment branch policies, -allow exactly the captured `${default_branch}`, contain the environment secret -`SQUAD_REVIEW_APP_PRIVATE_KEY`, and define environment variables -`SQUAD_REVIEW_APP_ID`, `SQUAD_REVIEW_APP_SLUG`, and -`SQUAD_REVIEW_APP_OWNER`. The App ID/slug must differ from -`15368`/`github-actions`. All four reviewer-specific names must also be absent -from repository-level Actions secrets and variables; repository fallback does -not satisfy this gate. - -If the authenticated account cannot read environment configuration, or any -requirement is absent, report the exact external prerequisite and do not claim -the required check is trusted. The installation PR remains a human boundary; -after it merges, the Cast canary intentionally fails closed until an -administrator provisions the environment and reruns the base-controlled -review. Configure a ruleset only from the canary check's observed App -integration ID, never from user input or a guessed value. +### 9. Verify the native review contract after merge + +The installation PR remains an explicit human trust boundary. After a human +merges it, inspect the automatically opened Cast PR and require the native +`Squad Review / review` job from the base-controlled `pull_request_target` +workflow to succeed. Verify the exact workflow path, immutable base/workflow +SHA, PR base/head, run ID and attempt, successful `review` job, and exact-head +GitHub Actions check-run binding. + +This review path uses only the native GitHub Actions/gh-aw runtime identity. +Never request or provision a reviewer PAT, GitHub App, private key, secret, +environment, hosted attestor, callback, or external service. The optional +`SQUAD_GITHUB_APP_*` and `SQUAD_GITHUB_TOKEN` activation credentials remain +unrelated to reviewer authority. + +For authoritative merge enforcement, use a source-bound required-workflow or +equivalent ruleset when available. A context-only requirement for +`Squad Review / review` is advisory because a PR-controlled workflow may be +able to emit the same workflow/job name through the shared GitHub Actions +identity. If source-bound enforcement is unavailable, retain independent human +review rather than relying on that context alone or adding a credential-based +publisher. Require branches to be up to date before merge (or use an equivalent +merge-queue freshness guarantee), and enable the required context only after +the workflow installation is merged and the post-merge Cast canary succeeds. ### 10. Never auto-merge — and explain what comes next @@ -366,9 +371,8 @@ gh pr edit --add-reviewer @copilot # then wait for review + checks; DO NOT mer ### ✓ Correct: STOP on an undocumented safe-update entry ```text -gh aw add reports an additional secret: `ACME_DEPLOY_KEY`. -→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN, - SQUAD_REVIEW_APP_PRIVATE_KEY) +gh aw add reports a third secret: `ACME_DEPLOY_KEY`. +→ This is NOT in the allowlist (SQUAD_GITHUB_APP_PRIVATE_KEY, SQUAD_GITHUB_TOKEN) and is NOT the squad-init action. Do NOT run `--approve`. Halt, report "unexpected safe-update entry: ACME_DEPLOY_KEY", and wait. ``` @@ -402,8 +406,7 @@ gh pr merge --squash # auto-merge before human review. NEVER. - ❌ **Blanket staging** (`git add .` / `-A` / `git commit -a`). Stage only `.gitattributes`, `.github/aw/`, `.github/workflows/`, `.github/skills/`, by path. - ❌ **Approving unknown safe updates.** Approve ONLY `SQUAD_GITHUB_APP_PRIVATE_KEY`, - `SQUAD_GITHUB_TOKEN`, `SQUAD_REVIEW_APP_PRIVATE_KEY`, and - `bradygaster/squad/.github/actions/squad-init`. Anything + `SQUAD_GITHUB_TOKEN`, and `bradygaster/squad/.github/actions/squad-init`. Anything else is a STOP. - ❌ **Treating `--approve` as the final compile.** Always finish with a plain `gh aw compile --strict` (no `--approve`). diff --git a/workflows/squad-bootstrap.md b/workflows/squad-bootstrap.md index ca9ee3058..b207e8e11 100644 --- a/workflows/squad-bootstrap.md +++ b/workflows/squad-bootstrap.md @@ -158,7 +158,7 @@ pre-agent-steps: node --check "$path" >/dev/null } # BEGIN GENERATED RESOURCE DIGESTS - check_hash "$install_verifier" "4f385df488cc65165e398e499663c015ce63851cb9c938f4c3dd0af38d25f991" + check_hash "$install_verifier" "71418638e37a53f570b13919bf5b232f5999be217dac4efc6b76e91918f85f16" check_hash "$cast_validator" "0988e04aeef316f4d7a0107c902bbbcf6538b8899b9fffba5f62150717967685" check_hash "$bootstrap_validator" "d449b9204f7fad133ff7133c1a30c9381c87e3c0c9d481352819ca93ea1a1dad" # END GENERATED RESOURCE DIGESTS diff --git a/workflows/squad-review.md b/workflows/squad-review.md index 84e34a0a3..63b0d00d0 100644 --- a/workflows/squad-review.md +++ b/workflows/squad-review.md @@ -73,7 +73,7 @@ safe-outputs: commit-id: "${{ github.event.pull_request.head.sha }}" jobs: review: - name: Squad Review Authority / attest + name: review if: always() needs: [agent, safe_outputs] runs-on: ubuntu-latest @@ -119,159 +119,6 @@ jobs: const guard = await import(pathToFileURL(baseGuard).href); await guard.assertClearingReview(process.env, async (route, fields) => (await github.request(`GET /${route}`, fields)).data); - publish: - name: Publish Squad Review required check - if: always() - needs: [agent, safe_outputs, review] - runs-on: ubuntu-latest - environment: squad-review-authority - permissions: - actions: read - contents: read - pull-requests: read - steps: - - name: Validate dedicated reviewer App configuration - env: - SQUAD_REVIEW_APP_ID: ${{ vars.SQUAD_REVIEW_APP_ID }} - SQUAD_REVIEW_APP_SLUG: ${{ vars.SQUAD_REVIEW_APP_SLUG }} - SQUAD_REVIEW_APP_OWNER: ${{ vars.SQUAD_REVIEW_APP_OWNER }} - SQUAD_REVIEW_APP_PRIVATE_KEY: ${{ secrets.SQUAD_REVIEW_APP_PRIVATE_KEY }} - run: | - set -euo pipefail - test "${SQUAD_REVIEW_APP_ID}" != "15368" - test "${SQUAD_REVIEW_APP_SLUG}" != "github-actions" - [[ "${SQUAD_REVIEW_APP_ID}" =~ ^[1-9][0-9]*$ ]] - [[ "${SQUAD_REVIEW_APP_SLUG}" =~ ^[a-z0-9][a-z0-9-]*$ ]] - [[ "${SQUAD_REVIEW_APP_OWNER}" =~ ^[A-Za-z0-9_.-]+$ ]] - test -n "${SQUAD_REVIEW_APP_PRIVATE_KEY}" - - name: Mint dedicated reviewer App token - id: squad-review-app-token - uses: actions/create-github-app-token@v3.2.0 - with: - app-id: ${{ vars.SQUAD_REVIEW_APP_ID }} - private-key: ${{ secrets.SQUAD_REVIEW_APP_PRIVATE_KEY }} - owner: ${{ vars.SQUAD_REVIEW_APP_OWNER }} - - name: Publish exact-head required check - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - AUTHORITY_RESULT: ${{ needs.review.result }} - SQUAD_REVIEW_APP_ID: ${{ vars.SQUAD_REVIEW_APP_ID }} - SQUAD_REVIEW_APP_SLUG: ${{ vars.SQUAD_REVIEW_APP_SLUG }} - SQUAD_REVIEW_PR: ${{ github.event.pull_request.number }} - SQUAD_REVIEW_BASE: ${{ github.event.pull_request.base.sha }} - SQUAD_REVIEW_HEAD: ${{ github.event.pull_request.head.sha }} - SQUAD_REVIEW_WORKFLOW_SHA: ${{ github.workflow_sha }} - with: - github-token: ${{ steps.squad-review-app-token.outputs.token }} - script: | - const checkName = 'Squad Review / review'; - const authorityJob = 'Squad Review Authority / attest'; - const repository = process.env.GITHUB_REPOSITORY; - const [owner, repo] = repository.split('/'); - const expectedAppId = Number(process.env.SQUAD_REVIEW_APP_ID); - const expectedAppSlug = process.env.SQUAD_REVIEW_APP_SLUG; - const pullRequest = Number(process.env.SQUAD_REVIEW_PR); - const baseSha = process.env.SQUAD_REVIEW_BASE; - const headSha = process.env.SQUAD_REVIEW_HEAD; - const workflowSha = process.env.SQUAD_REVIEW_WORKFLOW_SHA; - const runId = Number(process.env.GITHUB_RUN_ID); - const runAttempt = Number(process.env.GITHUB_RUN_ATTEMPT); - if (!Number.isSafeInteger(expectedAppId) || expectedAppId < 1 || - expectedAppId === 15368 || - !/^[a-z0-9][a-z0-9-]*$/.test(expectedAppSlug) || - expectedAppSlug === 'github-actions' || - !Number.isSafeInteger(pullRequest) || pullRequest < 1 || - !/^[0-9a-f]{40}$/.test(baseSha) || !/^[0-9a-f]{40}$/.test(headSha) || - workflowSha !== baseSha || !Number.isSafeInteger(runId) || runId < 1 || - !Number.isSafeInteger(runAttempt) || runAttempt < 1) { - core.setFailed('Invalid base-controlled Squad review check context.'); - return; - } - const run = (await github.rest.actions.getWorkflowRun({ - owner, repo, run_id: runId, - })).data; - if (run.event !== 'pull_request_target' || - run.path !== '.github/workflows/squad-review.lock.yml' || - run.head_sha !== workflowSha || - run.repository.full_name !== repository) { - core.setFailed('Squad review check is not bound to the base-controlled workflow run.'); - return; - } - const jobs = await github.paginate(github.rest.actions.listJobsForWorkflowRunAttempt, { - owner, repo, run_id: runId, attempt_number: runAttempt, per_page: 100, - }); - const authorityJobs = jobs.filter(job => job.name === authorityJob); - if (authorityJobs.length !== 1) { - core.setFailed('Missing or duplicate base-controlled Squad review authority job.'); - return; - } - const succeeded = process.env.AUTHORITY_RESULT === 'success' && - authorityJobs[0].conclusion === 'success'; - const externalId = - `squad-review-authority/v1:${repository}:${pullRequest}:${baseSha}:${headSha}`; - const existing = await github.paginate(github.rest.checks.listForRef, { - owner, repo, ref: headSha, check_name: checkName, per_page: 100, - }); - const owned = existing.filter(check => - check.external_id === externalId && - check.app?.id === expectedAppId && - check.app?.slug === expectedAppSlug); - if (owned.length > 1) { - core.setFailed('Duplicate exact-head Squad review authority checks.'); - return; - } - const detailsUrl = - `${process.env.GITHUB_SERVER_URL}/${repository}/actions/runs/${runId}`; - const output = { - title: succeeded - ? 'Base-controlled Squad review passed' - : 'Base-controlled Squad review did not pass', - summary: JSON.stringify({ - schema: 'squad-review-check/v1', - repository, - pull_request: pullRequest, - base_sha: baseSha, - head_sha: headSha, - workflow_sha: workflowSha, - run_id: runId, - run_attempt: runAttempt, - event: 'pull_request_target', - workflow_path: '.github/workflows/squad-review.lock.yml', - authority_job: authorityJob, - publisher_app_id: expectedAppId, - publisher_app_slug: expectedAppSlug, - }), - }; - const request = { - owner, repo, name: checkName, head_sha: headSha, - status: 'completed', - conclusion: succeeded ? 'success' : 'failure', - details_url: detailsUrl, - external_id: externalId, - output, - }; - const published = owned.length === 1 - ? await github.rest.checks.update({ - ...request, - check_run_id: owned[0].id, - }) - : await github.rest.checks.create(request); - if (published.data.app?.id !== expectedAppId || - published.data.app?.slug !== expectedAppSlug) { - try { - await github.rest.checks.delete({ - owner, repo, check_run_id: published.data.id, - }); - } finally { - core.setFailed( - 'Squad review check was not published by the configured dedicated GitHub App.' - ); - } - return; - } - if (!succeeded) { - core.setFailed('Base-controlled Squad review authority did not succeed.'); - } --- # Squad Review @@ -310,11 +157,11 @@ Read `.squad-review.json` at the exact PR head SHA and the committed `persistent_name` entries in that registry, and must differ. Act as that `reviewer_agent`, reading its base-commit charter. Missing or malformed attribution is a refusal, never a fallback to a display name or GitHub login. -The deterministic output guard and authority job repeat these checks. The -publisher creates `Squad Review / review` on the exact PR head only after the -native `Squad Review Authority / attest` job in the same base-controlled run -succeeds. A refusal, missing verdict, stale head, malformed provenance, or -same-named PR-controlled check cannot clear the authority contract. +The deterministic output guard and final job repeat these checks. GitHub +publishes the native job result as the stable `Squad Review / review` required +status context. The job uses `if: always()` and fails unless the agent, safe +outputs, and exact-head verdict gate all succeed. A refusal, missing verdict, +stale head, or malformed provenance therefore fails the required job. The workflow-installation PR is an explicit human trust boundary. Its base does not contain this workflow, guard, or manifest, so no trusted automatic Squad @@ -364,23 +211,26 @@ human and Copilot PRs) needs the committed stable-agent attribution above to clear the check. `Unattributed` is not permission to skip the gate or fabricate an identity. Malformed marker text remains a refusal on both trigger paths. -## SHA deduplication +## Run-attempt evidence 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 bot review contains a `Squad-Review-Verdict:` record for the -current head SHA and this exact workflow run ID and attempt, call `noop` and -stop. Evidence from a different run or attempt cannot deduplicate or clear this -run. The deterministic gate validates the `pull_request_target` event, immutable -workflow SHA, workflow path, PR, base, head, run, attempt, native authority job, -exact-head published check, and GitHub Actions App/bot identity; an arbitrary -comment, PR-controlled workflow, same-named check, or legacy head marker is not -clearing evidence. Re-fetch the pull request -immediately before emitting outputs; if its head SHA changed, call `noop` and -let the newer run review it. +Every workflow run attempt must submit exactly one fresh review. Never use +`noop` to reuse or deduplicate an existing `Squad-Review-Verdict:` record. +The deterministic safe-output guard rejects any pre-existing verdict evidence +for the current head SHA, workflow run ID, and attempt before it publishes the +new review. Evidence from an earlier run or attempt is not reused; a GitHub +Actions rerun receives an incremented attempt and must publish its own review. +The deterministic gate validates the `pull_request_target` event, immutable +workflow SHA, workflow path, PR, base, head, run, attempt, native `review` job, +and GitHub Actions bot identity; an arbitrary comment, PR-controlled workflow, +same-named job in a different run, attacker-claimed attempt number, or legacy +head marker is not clearing evidence. 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 diff --git a/workflows/squad-workflows.manifest.json b/workflows/squad-workflows.manifest.json index 7c6ab32ba..bda6d6fd9 100644 --- a/workflows/squad-workflows.manifest.json +++ b/workflows/squad-workflows.manifest.json @@ -14,8 +14,8 @@ "source": "workflows/package/squad.md", "destination": ".github/workflows/squad.md", "lock": ".github/workflows/squad.lock.yml", - "source_sha256": "bb7f231c852dfb73c5d1a6ba5e9f23f9f7c9fc5af9e1f1021c4c5213fd9389f9", - "lock_sha256": "f2e62f0ded2b937c99dcdab321f1ea2f886ff2f0537a324af4342908466a2bcf" + "source_sha256": "3dcf5517cac190c0cd98fc9e7f4cbb284ff11b7d76b356c508f1bb79b0c59690", + "lock_sha256": "8354d050354ea5f2ab3e63d3fe6b6648410f7e86691c2d8626c2318e4626a35f" }, { "name": "squad-implement-worker", @@ -30,8 +30,8 @@ "source": "workflows/package/squad-review.md", "destination": ".github/workflows/squad-review.md", "lock": ".github/workflows/squad-review.lock.yml", - "source_sha256": "27adf384cbe16c11ec89f4ecd76c9087a409c911ab9760575a3254b1d21dfb56", - "lock_sha256": "c3ed91988744077da65cbbc8a77a6d262bf4dca3bbfc593ab520283c3e9ced6b" + "source_sha256": "545ba959d0e4cfc0ba4cc507c2ed2c0d4b2044ca3b9e20a746c8b39cdc100bef", + "lock_sha256": "944dd43831110142a9bb9832ff38d5eb4c0e4df14347781c869fbbccfc0d901f" }, { "name": "squad-deps-worker", @@ -62,8 +62,8 @@ "source": "workflows/package/squad-bootstrap.md", "destination": ".github/workflows/squad-bootstrap.md", "lock": ".github/workflows/squad-bootstrap.lock.yml", - "source_sha256": "9df28d8e4094b66e7ddf4aed1ff3626efaa65c1dcd3815984d70573b40af640d", - "lock_sha256": "9a23ee0bd51cd7dd69b91538872a3fce0fb2376423ce8ac56c7c9f6f5b7f4b1b" + "source_sha256": "0336b294366e17a4294108e5b87ffbc7a46f3213e3683de0c18a02250c39f7bc", + "lock_sha256": "9c924d0a40f4313c3e0a1435413141e88aac8b32b7c2429e74275970787c205e" }, { "name": "squad-command-router", @@ -81,7 +81,7 @@ "package_destination": ".github/workflows/shared/squad-review-guard.mjs", "destination": ".github/workflows/shared/squad-review-guard.mjs", "ownership": "manifest", - "sha256": "6685f7e16b4ee13958bcdddf2a868a287f76b26913b0c17c4a943b398bbb4938" + "sha256": "0fea7b38811917b988f36876e8cca3ceddefcbe1f7c576c630d3ec274454fd50" }, { "path": "shared/squad-install-verifier.mjs", @@ -89,7 +89,7 @@ "package_destination": ".github/workflows/shared/squad-install-verifier.mjs", "destination": ".github/workflows/shared/squad-install-verifier.mjs", "ownership": "manifest", - "sha256": "4f385df488cc65165e398e499663c015ce63851cb9c938f4c3dd0af38d25f991" + "sha256": "71418638e37a53f570b13919bf5b232f5999be217dac4efc6b76e91918f85f16" }, { "path": "shared/squad-command-contract.mjs", @@ -217,7 +217,7 @@ "name": "gh-aw-enlistment", "source": "workflows/skills/gh-aw-enlistment/SKILL.md", "destination": ".github/skills/gh-aw-enlistment/SKILL.md", - "sha256": "2123f535c9fa5863d85fbed89ec5820579222eb39d279ffe1dea9a79a13fbadb" + "sha256": "0ac1979ab70bce2b5aa428ce9319a91a664ba8bca3a9b8ca1dbca277cec87d99" } ], "bootstrap": { diff --git a/workflows/squad.md b/workflows/squad.md index f5c43b20d..bf1057f0c 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1136,14 +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 workflow -head SHA equals the pull request's exact base SHA. Select the newest run by -creation time, breaking ties by numeric run ID, then fetch its jobs and require -exactly one job named `Squad Review Authority / attest`. Also fetch check runs -for the exact current PR head and require exactly one `Squad Review / review` -check whose external ID, details URL, GitHub Actions App identity, and -`squad-review-check/v1` summary bind that base-controlled run, PR, base SHA, -head SHA, run ID, and attempt. A same-named check without that binding is -advisory only. If no matching run or authority check exists, emit exactly one +head SHA equals the pull request's exact base SHA and whose recorded pull +request head equals the exact current 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 +advisory only. If no matching run or native authority job exists, emit exactly one `add-comment` stating that no base-controlled automatic review run exists for the exact head and that a new PR event (`synchronize`, reopen, or ready-for-review) is required. Stop without implying @@ -1156,8 +1153,8 @@ instruction: > Squad Review is automatic and cannot be dispatched from a branch. In the > selected base-controlled automatic run, choose **Re-run all jobs**. This > command did not rerun it. Only the new attempt of that exact -> `pull_request_target` run can publish verdict evidence and the exact-head -> `Squad Review / review` check for PR #{pull-request-number} +> `pull_request_target` run can publish verdict evidence and complete the native +> `Squad Review / review` required job for PR #{pull-request-number} > at head `{current-head-sha}`. Do not review the diff in this router, emit a verdict, edit files, create an From 6573b65a930a759cc5f4bb69a9e4b4abc91bd348 Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Tue, 29 Sep 2026 12:31:04 -0700 Subject: [PATCH 2/5] fix(gh-aw): address native review feedback Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- scripts/gh-aw-hosted-e2e.mjs | 2 +- test/gh-aw-hosted-e2e.test.ts | 2 +- test/gh-aw-review-guard.test.ts | 20 ++++++++++++++++++++ workflows/package/squad.md | 4 ++-- workflows/shared/squad-review-guard.mjs | 18 +++++++++++++++--- workflows/squad-workflows.manifest.json | 6 +++--- workflows/squad.md | 4 ++-- 7 files changed, 44 insertions(+), 12 deletions(-) diff --git a/scripts/gh-aw-hosted-e2e.mjs b/scripts/gh-aw-hosted-e2e.mjs index 56b6015f6..e4d777d2f 100644 --- a/scripts/gh-aw-hosted-e2e.mjs +++ b/scripts/gh-aw-hosted-e2e.mjs @@ -635,7 +635,7 @@ function waitForBaseControlledReviewCanary(target, castPr, evidence) { '--jq', '{id,name,status,conclusion,head_sha,details_url,app:{id:.app.id,slug:.app.slug}}', ]); if (check.id !== checkId - || check.name !== 'Squad Review / review' + || check.name !== 'review' || check.head_sha !== castPr.headSha || check.status !== 'completed' || check.conclusion !== 'success' diff --git a/test/gh-aw-hosted-e2e.test.ts b/test/gh-aw-hosted-e2e.test.ts index 2872cac3b..ba6f5a411 100644 --- a/test/gh-aw-hosted-e2e.test.ts +++ b/test/gh-aw-hosted-e2e.test.ts @@ -219,7 +219,7 @@ describe('Squad gh-aw hosted E2E controller', () => { expect(SCRIPT).toContain('waitForBaseControlledReviewCanary'); expect(SCRIPT).toContain("job.name === 'review'"); expect(SCRIPT).toContain('job.check_run_url'); - expect(SCRIPT).toContain("check.name !== 'Squad Review / review'"); + expect(SCRIPT).toContain("check.name !== 'review'"); expect(SCRIPT).toContain("check.app?.slug !== 'github-actions'"); expect(SCRIPT).toContain("'@squad/base-controlled-bootstrap'"); expect(SCRIPT).toContain("'@squad/base-controlled-review'"); diff --git a/test/gh-aw-review-guard.test.ts b/test/gh-aw-review-guard.test.ts index 4951ce6cb..2e960d468 100644 --- a/test/gh-aw-review-guard.test.ts +++ b/test/gh-aw-review-guard.test.ts @@ -257,6 +257,26 @@ describe('independent Squad review guard', () => { } }); + it('relays the latest valid rerun attempt instead of rejecting prior attempt evidence', async () => { + const f = fixture(true); + const rerunVerdict = { + ...f.verdict, + timestamp: '2026-09-28T12:01:30Z', + run_attempt: 2, + }; + const rerunReview = { + ...f.review, + id: 124, + submitted_at: '2026-09-28T12:01:31Z', + body: `${VERDICT_PREFIX}${JSON.stringify(rerunVerdict)}`, + }; + f.run.run_attempt = 2; + f.state.reviews.push(rerunReview); + await expect(assertClearingReview(f.env, f.get, { relay: true })) + .resolves.toEqual(rerunVerdict); + expect(f.state.calls.some(route => route.endsWith('/attempts/2/jobs'))).toBe(true); + }); + it('refuses PR-controlled shaped review and job evidence from another run', async () => { const f = fixture(true); f.run.event = 'pull_request'; diff --git a/workflows/package/squad.md b/workflows/package/squad.md index 2cdd93131..fdbaa9210 100644 --- a/workflows/package/squad.md +++ b/workflows/package/squad.md @@ -2239,8 +2239,8 @@ 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 workflow -head SHA equals the pull request's exact base SHA and whose recorded pull -request head equals the exact current head. Select the newest run by creation +head SHA equals the pull request's exact current head and whose recorded pull +request head equals that same exact current 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 advisory only. If no matching run or native authority job exists, emit exactly one diff --git a/workflows/shared/squad-review-guard.mjs b/workflows/shared/squad-review-guard.mjs index 13f9fff13..0a3708245 100644 --- a/workflows/shared/squad-review-guard.mjs +++ b/workflows/shared/squad-review-guard.mjs @@ -344,9 +344,21 @@ export async function assertClearingReview(env, get, options = {}) { relay ? timestamp(target.pr.merged_at) : Date.now(), ), })); - const bound = relay ? candidates : candidates.filter(({ verdict }) => - String(verdict.run_id) === env.GITHUB_RUN_ID && - String(verdict.run_attempt) === env.GITHUB_RUN_ATTEMPT); + let bound; + if (relay) { + const ordered = [...candidates].sort((left, right) => + timestamp(left.review.submitted_at) - timestamp(right.review.submitted_at) || + left.review.id - right.review.id); + const latest = ordered.at(-1); + requireThat(latest, 'missing verdict evidence for this run'); + bound = candidates.filter(({ verdict }) => + verdict.run_id === latest.verdict.run_id && + verdict.run_attempt === latest.verdict.run_attempt); + } else { + bound = candidates.filter(({ verdict }) => + String(verdict.run_id) === env.GITHUB_RUN_ID && + String(verdict.run_attempt) === env.GITHUB_RUN_ATTEMPT); + } 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(); diff --git a/workflows/squad-workflows.manifest.json b/workflows/squad-workflows.manifest.json index bda6d6fd9..2d01bf6da 100644 --- a/workflows/squad-workflows.manifest.json +++ b/workflows/squad-workflows.manifest.json @@ -14,8 +14,8 @@ "source": "workflows/package/squad.md", "destination": ".github/workflows/squad.md", "lock": ".github/workflows/squad.lock.yml", - "source_sha256": "3dcf5517cac190c0cd98fc9e7f4cbb284ff11b7d76b356c508f1bb79b0c59690", - "lock_sha256": "8354d050354ea5f2ab3e63d3fe6b6648410f7e86691c2d8626c2318e4626a35f" + "source_sha256": "49d65e2a022b084be3001e6bb804173b715e4baecbc5b3f70d458d2dbfb449d0", + "lock_sha256": "84e565a675ccddeb15eea375e46cbe6f9f1d8b3ef90fc50723f9166c4eafbff0" }, { "name": "squad-implement-worker", @@ -81,7 +81,7 @@ "package_destination": ".github/workflows/shared/squad-review-guard.mjs", "destination": ".github/workflows/shared/squad-review-guard.mjs", "ownership": "manifest", - "sha256": "0fea7b38811917b988f36876e8cca3ceddefcbe1f7c576c630d3ec274454fd50" + "sha256": "16024b2a265d61a74f37a09e396c7eeea2a409bf035db3f34fa70bbe3b247f3e" }, { "path": "shared/squad-install-verifier.mjs", diff --git a/workflows/squad.md b/workflows/squad.md index bf1057f0c..07af45672 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1136,8 +1136,8 @@ 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 workflow -head SHA equals the pull request's exact base SHA and whose recorded pull -request head equals the exact current head. Select the newest run by creation +head SHA equals the pull request's exact current head and whose recorded pull +request head equals that same exact current 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 advisory only. If no matching run or native authority job exists, emit exactly one From 066c17bc31a1a5ed56ac893fe3217689f61d76e2 Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Tue, 29 Sep 2026 12:37:56 -0700 Subject: [PATCH 3/5] docs(gh-aw): qualify native review authority Part of #2109 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .changeset/dedicated-review-app.md | 10 +++++++--- .github/agents.md | 12 ++++++++---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/.changeset/dedicated-review-app.md b/.changeset/dedicated-review-app.md index 81f7afbb0..7e4d48814 100644 --- a/.changeset/dedicated-review-app.md +++ b/.changeset/dedicated-review-app.md @@ -3,6 +3,10 @@ "@bradygaster/squad-sdk": patch --- -Use the base-controlled native `Squad Review / review` job as the authoritative -review result without requiring a separately provisioned reviewer App, token, -private key, secret, environment, or custom Checks API publisher. +Use the base-controlled deterministic relay's native `Squad Review / review` +job as the authoritative review result only when it validates the exact trusted +automatic workflow run. Authoritative merge enforcement requires a source-bound +required-workflow or equivalent ruleset; where that is unavailable, the context +is advisory and an independent human approving review remains required. This +path needs no separately provisioned reviewer App, token, private key, secret, +environment, or custom Checks API publisher. diff --git a/.github/agents.md b/.github/agents.md index ba6d52a2e..db7632007 100644 --- a/.github/agents.md +++ b/.github/agents.md @@ -35,10 +35,14 @@ GitHub Agentic Workflows (`gh-aw`) are composable AI workflows triggered by slas > the repository's default branch. > The installation PR is a manual trust boundary: its PR-controlled workflow > cannot mint a trusted Squad verdict. After merge, require the automatically -> opened Cast PR to pass the native `Squad Review / review` job from the -> base-controlled `pull_request_target` workflow before treating bootstrap or -> lifecycle automation as trusted. Reviewer authority requires no separate PAT, -> GitHub App, private key, secret, environment, or external service. +> opened Cast PR to pass the deterministic relay's native +> `Squad Review / review` job, which is authoritative only when it validates the +> exact trusted automatic run from the base-controlled `pull_request_target` +> workflow. Authoritative merge enforcement requires a source-bound +> required-workflow or equivalent ruleset. Where source binding is unavailable, +> treat the context as advisory and continue to require an independent human +> approving review. The relay requires no separate PAT, GitHub App, private key, +> secret, environment, or external service. The quick start installs this workflow set: From 158e88b4ee49f48bfe822bf28448ce7a6882a148 Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Tue, 29 Sep 2026 12:40:44 -0700 Subject: [PATCH 4/5] Revert "fix(gh-aw): address native review feedback" This reverts commit 6573b65a930a759cc5f4bb69a9e4b4abc91bd348. --- scripts/gh-aw-hosted-e2e.mjs | 2 +- test/gh-aw-hosted-e2e.test.ts | 2 +- test/gh-aw-review-guard.test.ts | 20 -------------------- workflows/package/squad.md | 4 ++-- workflows/shared/squad-review-guard.mjs | 18 +++--------------- workflows/squad-workflows.manifest.json | 6 +++--- workflows/squad.md | 4 ++-- 7 files changed, 12 insertions(+), 44 deletions(-) diff --git a/scripts/gh-aw-hosted-e2e.mjs b/scripts/gh-aw-hosted-e2e.mjs index e4d777d2f..56b6015f6 100644 --- a/scripts/gh-aw-hosted-e2e.mjs +++ b/scripts/gh-aw-hosted-e2e.mjs @@ -635,7 +635,7 @@ function waitForBaseControlledReviewCanary(target, castPr, evidence) { '--jq', '{id,name,status,conclusion,head_sha,details_url,app:{id:.app.id,slug:.app.slug}}', ]); if (check.id !== checkId - || check.name !== 'review' + || check.name !== 'Squad Review / review' || check.head_sha !== castPr.headSha || check.status !== 'completed' || check.conclusion !== 'success' diff --git a/test/gh-aw-hosted-e2e.test.ts b/test/gh-aw-hosted-e2e.test.ts index ba6f5a411..2872cac3b 100644 --- a/test/gh-aw-hosted-e2e.test.ts +++ b/test/gh-aw-hosted-e2e.test.ts @@ -219,7 +219,7 @@ describe('Squad gh-aw hosted E2E controller', () => { expect(SCRIPT).toContain('waitForBaseControlledReviewCanary'); expect(SCRIPT).toContain("job.name === 'review'"); expect(SCRIPT).toContain('job.check_run_url'); - expect(SCRIPT).toContain("check.name !== 'review'"); + expect(SCRIPT).toContain("check.name !== 'Squad Review / review'"); expect(SCRIPT).toContain("check.app?.slug !== 'github-actions'"); expect(SCRIPT).toContain("'@squad/base-controlled-bootstrap'"); expect(SCRIPT).toContain("'@squad/base-controlled-review'"); diff --git a/test/gh-aw-review-guard.test.ts b/test/gh-aw-review-guard.test.ts index 2e960d468..4951ce6cb 100644 --- a/test/gh-aw-review-guard.test.ts +++ b/test/gh-aw-review-guard.test.ts @@ -257,26 +257,6 @@ describe('independent Squad review guard', () => { } }); - it('relays the latest valid rerun attempt instead of rejecting prior attempt evidence', async () => { - const f = fixture(true); - const rerunVerdict = { - ...f.verdict, - timestamp: '2026-09-28T12:01:30Z', - run_attempt: 2, - }; - const rerunReview = { - ...f.review, - id: 124, - submitted_at: '2026-09-28T12:01:31Z', - body: `${VERDICT_PREFIX}${JSON.stringify(rerunVerdict)}`, - }; - f.run.run_attempt = 2; - f.state.reviews.push(rerunReview); - await expect(assertClearingReview(f.env, f.get, { relay: true })) - .resolves.toEqual(rerunVerdict); - expect(f.state.calls.some(route => route.endsWith('/attempts/2/jobs'))).toBe(true); - }); - it('refuses PR-controlled shaped review and job evidence from another run', async () => { const f = fixture(true); f.run.event = 'pull_request'; diff --git a/workflows/package/squad.md b/workflows/package/squad.md index fdbaa9210..2cdd93131 100644 --- a/workflows/package/squad.md +++ b/workflows/package/squad.md @@ -2239,8 +2239,8 @@ 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 workflow -head SHA equals the pull request's exact current head and whose recorded pull -request head equals that same exact current head. Select the newest run by creation +head SHA equals the pull request's exact base SHA and whose recorded pull +request head equals the exact current 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 advisory only. If no matching run or native authority job exists, emit exactly one diff --git a/workflows/shared/squad-review-guard.mjs b/workflows/shared/squad-review-guard.mjs index 0a3708245..13f9fff13 100644 --- a/workflows/shared/squad-review-guard.mjs +++ b/workflows/shared/squad-review-guard.mjs @@ -344,21 +344,9 @@ export async function assertClearingReview(env, get, options = {}) { relay ? timestamp(target.pr.merged_at) : Date.now(), ), })); - let bound; - if (relay) { - const ordered = [...candidates].sort((left, right) => - timestamp(left.review.submitted_at) - timestamp(right.review.submitted_at) || - left.review.id - right.review.id); - const latest = ordered.at(-1); - requireThat(latest, 'missing verdict evidence for this run'); - bound = candidates.filter(({ verdict }) => - verdict.run_id === latest.verdict.run_id && - verdict.run_attempt === latest.verdict.run_attempt); - } else { - bound = candidates.filter(({ verdict }) => - String(verdict.run_id) === env.GITHUB_RUN_ID && - String(verdict.run_attempt) === env.GITHUB_RUN_ATTEMPT); - } + const bound = relay ? candidates : candidates.filter(({ verdict }) => + String(verdict.run_id) === env.GITHUB_RUN_ID && + String(verdict.run_attempt) === env.GITHUB_RUN_ATTEMPT); 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(); diff --git a/workflows/squad-workflows.manifest.json b/workflows/squad-workflows.manifest.json index 2d01bf6da..bda6d6fd9 100644 --- a/workflows/squad-workflows.manifest.json +++ b/workflows/squad-workflows.manifest.json @@ -14,8 +14,8 @@ "source": "workflows/package/squad.md", "destination": ".github/workflows/squad.md", "lock": ".github/workflows/squad.lock.yml", - "source_sha256": "49d65e2a022b084be3001e6bb804173b715e4baecbc5b3f70d458d2dbfb449d0", - "lock_sha256": "84e565a675ccddeb15eea375e46cbe6f9f1d8b3ef90fc50723f9166c4eafbff0" + "source_sha256": "3dcf5517cac190c0cd98fc9e7f4cbb284ff11b7d76b356c508f1bb79b0c59690", + "lock_sha256": "8354d050354ea5f2ab3e63d3fe6b6648410f7e86691c2d8626c2318e4626a35f" }, { "name": "squad-implement-worker", @@ -81,7 +81,7 @@ "package_destination": ".github/workflows/shared/squad-review-guard.mjs", "destination": ".github/workflows/shared/squad-review-guard.mjs", "ownership": "manifest", - "sha256": "16024b2a265d61a74f37a09e396c7eeea2a409bf035db3f34fa70bbe3b247f3e" + "sha256": "0fea7b38811917b988f36876e8cca3ceddefcbe1f7c576c630d3ec274454fd50" }, { "path": "shared/squad-install-verifier.mjs", diff --git a/workflows/squad.md b/workflows/squad.md index 07af45672..bf1057f0c 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1136,8 +1136,8 @@ 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 workflow -head SHA equals the pull request's exact current head and whose recorded pull -request head equals that same exact current head. Select the newest run by creation +head SHA equals the pull request's exact base SHA and whose recorded pull +request head equals the exact current 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 advisory only. If no matching run or native authority job exists, emit exactly one From 2aada459d9603619e01465a3cd1f31e9c5408b5e Mon Sep 17 00:00:00 2001 From: Brady Gaster Date: Tue, 29 Sep 2026 12:47:16 -0700 Subject: [PATCH 5/5] fix(gh-aw): independently correct native review evidence Bind Checks API evidence to the native job name and distinguish run head SHA from immutable workflow source. Select the latest bot review using API submission time and ID, then require one verdict for its exact run attempt without falling back to earlier evidence. Independent revision by github-integration-dev after reverting the locked-out author's revision. Preserves the independent docs correction. Closes #2103 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- scripts/gh-aw-hosted-e2e.mjs | 24 +++--- test/gh-aw-hosted-e2e.test.ts | 77 +++++++++++++++++++- test/gh-aw-review-guard.test.ts | 97 +++++++++++++++++++++++++ test/gh-aw-review-workflow.test.ts | 20 +++++ workflows/package/squad.md | 9 ++- workflows/shared/squad-review-guard.mjs | 16 +++- workflows/squad-workflows.manifest.json | 6 +- workflows/squad.md | 9 ++- 8 files changed, 234 insertions(+), 24 deletions(-) diff --git a/scripts/gh-aw-hosted-e2e.mjs b/scripts/gh-aw-hosted-e2e.mjs index 56b6015f6..2a949f556 100644 --- a/scripts/gh-aw-hosted-e2e.mjs +++ b/scripts/gh-aw-hosted-e2e.mjs @@ -565,10 +565,15 @@ export function waitForBootstrapOutputs( throw new Error('Timed out waiting for the draft Cast PR and bootstrap research issue.'); } -function waitForBaseControlledReviewCanary(target, castPr, evidence) { - const deadline = Date.now() + 20 * 60 * 1000; - while (Date.now() < deadline) { - const reviews = ghJson([ +export function waitForBaseControlledReviewCanary( + target, + castPr, + evidence, + { request = ghJson, now = Date.now, pause = sleep } = {}, +) { + const deadline = now() + 20 * 60 * 1000; + while (now() < deadline) { + const reviews = request([ 'api', `repos/${target}/pulls/${castPr.number}/reviews?per_page=100`, ]); @@ -598,7 +603,7 @@ function waitForBaseControlledReviewCanary(target, castPr, evidence) { || verdict.workflow_path !== '.github/workflows/squad-review.lock.yml') { throw new Error('Squad Review canary verdict is not bound to the base-controlled bootstrap activation.'); } - const run = ghJson([ + const run = request([ 'api', `repos/${target}/actions/runs/${verdict.run_id}`, '--jq', '{id,event,path,head_sha,run_attempt,status,conclusion,display_title,repository:{full_name:.repository.full_name}}', @@ -611,7 +616,7 @@ function waitForBaseControlledReviewCanary(target, castPr, evidence) { || run.run_attempt < verdict.run_attempt) { throw new Error('Squad Review canary verdict is not bound to the base-controlled workflow run.'); } - const jobs = ghJson([ + const jobs = request([ 'api', '--paginate', '--slurp', `repos/${target}/actions/runs/${verdict.run_id}/attempts/${verdict.run_attempt}/jobs?per_page=100`, ]).flat().flatMap((page) => page.jobs ?? []); @@ -628,14 +633,15 @@ function waitForBaseControlledReviewCanary(target, castPr, evidence) { if (!Number.isSafeInteger(checkId)) { throw new Error('Native Squad Review job did not expose a valid check-run binding.'); } - const check = ghJson([ + const check = request([ 'api', '-H', 'Accept: application/vnd.github+json', `repos/${target}/check-runs/${checkId}`, '--jq', '{id,name,status,conclusion,head_sha,details_url,app:{id:.app.id,slug:.app.slug}}', ]); + // The Checks API exposes the job name, not the UI's workflow/job context. if (check.id !== checkId - || check.name !== 'Squad Review / review' + || check.name !== 'review' || check.head_sha !== castPr.headSha || check.status !== 'completed' || check.conclusion !== 'success' @@ -653,7 +659,7 @@ function waitForBaseControlledReviewCanary(target, castPr, evidence) { }); return { run, job, check, verdict }; } - sleep(10_000); + pause(10_000); } throw new Error('Timed out waiting for the base-controlled Squad Review activation canary.'); } diff --git a/test/gh-aw-hosted-e2e.test.ts b/test/gh-aw-hosted-e2e.test.ts index 2872cac3b..53ee052eb 100644 --- a/test/gh-aw-hosted-e2e.test.ts +++ b/test/gh-aw-hosted-e2e.test.ts @@ -1,4 +1,5 @@ -import { readFileSync } from 'node:fs'; +import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; @@ -8,6 +9,7 @@ import { guardedTargetMutation, sanitizedEnvironment, selectBootstrapOutputs, + waitForBaseControlledReviewCanary, waitForBootstrapOutputs, } from '../scripts/gh-aw-hosted-e2e.mjs'; import { @@ -219,7 +221,8 @@ describe('Squad gh-aw hosted E2E controller', () => { expect(SCRIPT).toContain('waitForBaseControlledReviewCanary'); expect(SCRIPT).toContain("job.name === 'review'"); expect(SCRIPT).toContain('job.check_run_url'); - expect(SCRIPT).toContain("check.name !== 'Squad Review / review'"); + expect(SCRIPT).toContain("check.name !== 'review'"); + expect(SCRIPT).not.toContain("check.name !== 'Squad Review / review'"); expect(SCRIPT).toContain("check.app?.slug !== 'github-actions'"); expect(SCRIPT).toContain("'@squad/base-controlled-bootstrap'"); expect(SCRIPT).toContain("'@squad/base-controlled-review'"); @@ -230,6 +233,76 @@ describe('Squad gh-aw hosted E2E controller', () => { expect(SCRIPT).not.toMatch(/SQUAD_REVIEW_APP_|squad-review-authority|publisher_app_/); }); + it.each([ + 'native-job-name', 'ui-context-name', 'wrong-check-id', 'wrong-head', + 'failed-check', 'incomplete-check', 'foreign-app', 'foreign-app-id', + ])('exercises the hosted canary API contract: %s', mutation => { + const target = 'owner/consumer'; + const verdict = { + schema: 'squad-review-verdict/v1', repository: target, + pull_request: currentCastPr.number, base_sha: currentCastPr.baseSha, + head_sha: CAST_SHA, author_agent: '@squad/base-controlled-bootstrap', + reviewer_agent: '@squad/base-controlled-review', result: 'COMMENT', + event: 'pull_request_target', workflow_sha: currentCastPr.baseSha, + 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, + repository: { full_name: target }, run_attempt: 1, + display_title: `Squad review — PR #${currentCastPr.number}`, + }; + const job = { + name: 'review', status: 'completed', conclusion: 'success', + check_run_url: `https://api.github.com/repos/${target}/check-runs/123`, + }; + const check = { + id: 123, name: 'review', head_sha: CAST_SHA, status: 'completed', conclusion: 'success', + app: { id: 15368, slug: 'github-actions' }, + }; + switch (mutation) { + 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 'failed-check': check.conclusion = 'failure'; break; + case 'incomplete-check': check.status = 'in_progress'; break; + case 'foreign-app': check.app.slug = 'other'; break; + case 'foreign-app-id': check.app.id = 1; break; + } + const routes: string[] = []; + const request = (args: string[]) => { + const route = args.find(arg => arg.startsWith(`repos/${target}/`)); + routes.push(route!); + switch (route) { + case `repos/${target}/pulls/${currentCastPr.number}/reviews?per_page=100`: + return [{ + user: ACTIONS_BOT, commit_id: CAST_SHA, + body: `Squad-Review-Verdict: ${JSON.stringify(verdict)}`, + }]; + case `repos/${target}/actions/runs/31`: return run; + case `repos/${target}/actions/runs/31/attempts/1/jobs?per_page=100`: return [{ jobs: [job] }]; + case `repos/${target}/check-runs/123`: return check; + default: throw new Error(`Unexpected API route: ${route}`); + } + }; + const evidence = mkdtempSync(resolve(tmpdir(), 'squad-review-canary-')); + try { + const execute = () => waitForBaseControlledReviewCanary(target, currentCastPr, evidence, { + request, now: () => 0, pause: () => { throw new Error('Unexpected polling'); }, + }); + if (mutation === 'native-job-name') { + expect(execute()).toEqual({ run, job, check, verdict }); + expect(JSON.parse(readFileSync( + resolve(evidence, 'base-controlled-review-canary.json'), 'utf8', + )).check.name).toBe('review'); + } else { + expect(execute).toThrow('not bound to the trusted review job'); + } + expect(routes.at(-1)).toBe(`repos/${target}/check-runs/123`); + } finally { + rmSync(evidence, { recursive: true, force: true }); + } + }); + it('never executes candidate package or installed verifier with inherited credentials', () => { expect(SCRIPT).toContain( 'env: sanitizedEnvironment(options.env, { allowGitHubToken: options.allowGitHubToken === true })', diff --git a/test/gh-aw-review-guard.test.ts b/test/gh-aw-review-guard.test.ts index 4951ce6cb..4535d13c2 100644 --- a/test/gh-aw-review-guard.test.ts +++ b/test/gh-aw-review-guard.test.ts @@ -167,6 +167,23 @@ function makeBootstrap(f: ReturnType) { f.sync(); } +function retainFirstAttempt(f: ReturnType) { + const first = structuredClone(f.review); + first.body = `${VERDICT_PREFIX}${JSON.stringify({ ...f.verdict, result: 'REQUEST_CHANGES' })}`; + f.review.id = 124; + f.review.submitted_at = '2026-09-28T12:02:11Z'; + f.verdict.timestamp = '2026-09-28T12:02:10Z'; + f.verdict.run_attempt = 2; + f.run.run_attempt = 2; + f.run.run_started_at = FINISHED; + 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.reviews = [first, f.review]; + f.sync(); + return first; +} + describe('independent Squad review guard', () => { it('uses reserved workflow roles only for the base-controlled post-install bootstrap PR', async () => { const f = fixture(); @@ -257,6 +274,86 @@ describe('independent Squad review guard', () => { } }); + it.each([false, true])('selects a successful rerun with retained attempt-1 rejection (reverse=%s)', async reverse => { + const f = fixture(true); + retainFirstAttempt(f); + if (reverse) f.state.reviews.reverse(); + await expect(assertClearingReview(f.env, f.get, { relay: true })).resolves.toEqual(f.verdict); + expect(f.state.calls).toContain(`repos/${REPOSITORY}/actions/runs/17/attempts/2/jobs`); + expect(f.state.calls).not.toContain(`repos/${REPOSITORY}/actions/runs/17/attempts/1/jobs`); + }); + + it('keeps the executing gate scoped to its exact attempt with retained older evidence', async () => { + const f = fixture(); + retainFirstAttempt(f); + f.env.GITHUB_RUN_ATTEMPT = '2'; + await expect(assertClearingReview(f.env, f.get)).resolves.toEqual(f.verdict); + f.state.reviews.push({ ...f.review, id: 125 }); + await expect(assertClearingReview(f.env, f.get)).rejects.toThrow('duplicate verdict evidence'); + }); + + it('orders by GitHub submission time and review ID, not verdict time or list order', async () => { + const f = fixture(true); + const first = retainFirstAttempt(f); + first.submitted_at = f.review.submitted_at; + first.body = `${VERDICT_PREFIX}${JSON.stringify({ + ...f.verdict, run_attempt: 1, result: 'REQUEST_CHANGES', timestamp: first.submitted_at, + })}`; + f.state.reviews.reverse(); + await expect(assertClearingReview(f.env, f.get, { relay: true })).resolves.toEqual(f.verdict); + // Submission time takes precedence even when the earlier review has a larger ID. + first.submitted_at = SUBMITTED; + first.id = 125; + first.body = `${VERDICT_PREFIX}${JSON.stringify({ + ...f.verdict, run_attempt: 1, result: 'REQUEST_CHANGES', timestamp: ISSUED, + })}`; + await expect(assertClearingReview(f.env, f.get, { relay: true })).resolves.toEqual(f.verdict); + }); + + it('selects a newer automatic run independently of the relay environment run ID', async () => { + const f = fixture(true); + retainFirstAttempt(f); + f.verdict.run_id = 18; + f.verdict.run_attempt = f.run.run_attempt = 1; + 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); + }); + + it('rejects duplicate verdicts for the selected attempt even with distinct review IDs', async () => { + const f = fixture(true); + retainFirstAttempt(f); + f.state.reviews.push({ ...f.review, id: 125 }); + await expect(assertClearingReview(f.env, f.get, { relay: true })) + .rejects.toThrow('duplicate verdict evidence'); + }); + + it.each([ + 'failed-run', 'failed-job', 'duplicate-job', 'wrong-workflow', 'wrong-event', + 'wrong-head', 'replay', 'after-merge', 'malformed', 'invalid-id', 'latest-rejection', + ])('never falls back to an earlier attempt when the latest has %s', async mutation => { + const f = fixture(true); + retainFirstAttempt(f); + f.state.reviews[0].body = `${VERDICT_PREFIX}${JSON.stringify({ + ...f.verdict, run_attempt: 1, timestamp: ISSUED, + })}`; + switch (mutation) { + case 'failed-run': f.run.conclusion = 'failure'; break; + case 'failed-job': f.jobs[0].conclusion = 'failure'; break; + 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 '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; + case 'invalid-id': f.review.id = 0; break; + case 'latest-rejection': f.verdict.result = 'REQUEST_CHANGES'; f.sync(); break; + } + await expect(assertClearingReview(f.env, f.get, { relay: true })).rejects.toThrow(); + }); + it('refuses PR-controlled shaped review and job evidence from another run', async () => { const f = fixture(true); f.run.event = 'pull_request'; diff --git a/test/gh-aw-review-workflow.test.ts b/test/gh-aw-review-workflow.test.ts index 5ef4178a2..c0d52f181 100644 --- a/test/gh-aw-review-workflow.test.ts +++ b/test/gh-aw-review-workflow.test.ts @@ -75,6 +75,13 @@ function provenanceRows(workflow: string): string[] { .map(match => `${match[1]}:${match[2].trim()}:${match[3].trim()}`); } +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"); +} + function assertReviewerContract(workflow: string): void { const yaml = frontmatter(workflow); const tools = yamlBlock(yaml, 'tools'); @@ -310,6 +317,19 @@ describe('gh-aw enforcing Squad reviewer', () => { expect(relay).not.toContain('dispatch-workflow'); expect(relay).toMatch(/command did not rerun it\./); expect(relay).toMatch(/no base-controlled automatic review run exists for\s+the exact head/); + assertRelayHeadBinding(ROUTER); + assertRelayHeadBinding(read('workflows/package/squad.md')); + }); + + 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("review guard binds `workflow_sha` to the pull request's exact base", + "review guard binds `workflow_sha` to the pull request's exact head"), + ]) { + expect(() => assertRelayHeadBinding(mutation)).toThrow(); + } }); it('limits slash commands to issue and pull request conversation surfaces', () => { diff --git a/workflows/package/squad.md b/workflows/package/squad.md index 2cdd93131..696099b65 100644 --- a/workflows/package/squad.md +++ b/workflows/package/squad.md @@ -2238,9 +2238,12 @@ post one `add-comment` explaining that `/squad review` must target a pull 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 workflow -head SHA equals the pull request's exact base SHA and whose recorded pull -request head equals the exact current head. Select the newest run by creation +`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. +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 advisory only. If no matching run or native authority job exists, emit exactly one diff --git a/workflows/shared/squad-review-guard.mjs b/workflows/shared/squad-review-guard.mjs index 13f9fff13..4761dda04 100644 --- a/workflows/shared/squad-review-guard.mjs +++ b/workflows/shared/squad-review-guard.mjs @@ -80,7 +80,8 @@ export function validateVerdict(value, expected, review, now = Date.now()) { Number.isSafeInteger(value.run_attempt) && value.run_attempt > 0 && value.workflow_path === WORKFLOW && value.workflow_sha === expected.workflow_sha, 'invalid verdict result or run'); - requireThat(review.user?.login === BOT && review.user?.id === BOT_ID && + requireThat(Number.isSafeInteger(review.id) && review.id > 0 && + review.user?.login === BOT && review.user?.id === BOT_ID && review.user?.type === 'Bot' && review.commit_id === value.head_sha && review.state === 'COMMENTED', 'review author, commit, or native result mismatch'); @@ -344,9 +345,16 @@ export async function assertClearingReview(env, get, options = {}) { relay ? timestamp(target.pr.merged_at) : Date.now(), ), })); - const bound = relay ? candidates : candidates.filter(({ verdict }) => - String(verdict.run_id) === env.GITHUB_RUN_ID && - String(verdict.run_attempt) === env.GITHUB_RUN_ATTEMPT); + // 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 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(); diff --git a/workflows/squad-workflows.manifest.json b/workflows/squad-workflows.manifest.json index bda6d6fd9..91b5edeb9 100644 --- a/workflows/squad-workflows.manifest.json +++ b/workflows/squad-workflows.manifest.json @@ -14,8 +14,8 @@ "source": "workflows/package/squad.md", "destination": ".github/workflows/squad.md", "lock": ".github/workflows/squad.lock.yml", - "source_sha256": "3dcf5517cac190c0cd98fc9e7f4cbb284ff11b7d76b356c508f1bb79b0c59690", - "lock_sha256": "8354d050354ea5f2ab3e63d3fe6b6648410f7e86691c2d8626c2318e4626a35f" + "source_sha256": "e3cc1b34c38d345eb5c843d4c4cf4966d7bbdc8c78c567cd3fe44aa84abd0fe7", + "lock_sha256": "85aa7c5cbda5453d71af43897088fc06bdb0771c4ae825d88721c4e12e4feaf1" }, { "name": "squad-implement-worker", @@ -81,7 +81,7 @@ "package_destination": ".github/workflows/shared/squad-review-guard.mjs", "destination": ".github/workflows/shared/squad-review-guard.mjs", "ownership": "manifest", - "sha256": "0fea7b38811917b988f36876e8cca3ceddefcbe1f7c576c630d3ec274454fd50" + "sha256": "95472a06daabc03428955e659a4f3c548afc316427ea48196760cc992891cad0" }, { "path": "shared/squad-install-verifier.mjs", diff --git a/workflows/squad.md b/workflows/squad.md index bf1057f0c..b48981e15 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1135,9 +1135,12 @@ post one `add-comment` explaining that `/squad review` must target a pull 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 workflow -head SHA equals the pull request's exact base SHA and whose recorded pull -request head equals the exact current head. Select the newest run by creation +`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. +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 advisory only. If no matching run or native authority job exists, emit exactly one