diff --git a/.changeset/dedicated-review-app.md b/.changeset/dedicated-review-app.md index 30e809777..7e4d48814 100644 --- a/.changeset/dedicated-review-app.md +++ b/.changeset/dedicated-review-app.md @@ -3,4 +3,10 @@ "@bradygaster/squad-sdk": patch --- -Require a dedicated, branch-restricted GitHub App identity for authoritative Squad review checks. +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 44593c3ab..db7632007 100644 --- a/.github/agents.md +++ b/.github/agents.md @@ -35,12 +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 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 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: 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..2a949f556 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,27 +565,15 @@ export function waitForBootstrapOutputs( throw new Error('Timed out waiting for the draft Cast PR and bootstrap research issue.'); } -function waitForBaseControlledReviewCanary(target, castPr, expectedApp, 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([ +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`, ]); @@ -709,14 +586,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,35 +603,63 @@ 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 = 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}}', + ]); + 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 = request([ + '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 = 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 !== '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); + pause(10_000); } throw new Error('Timed out waiting for the base-controlled Squad Review activation canary.'); } @@ -937,7 +835,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..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'; @@ -6,10 +7,9 @@ import { parse } from 'yaml'; import { assertPristineTarget, guardedTargetMutation, - observedRequiredCheck, sanitizedEnvironment, selectBootstrapOutputs, - validateReviewAuthorityEnvironment, + waitForBaseControlledReviewCanary, waitForBootstrapOutputs, } from '../scripts/gh-aw-hosted-e2e.mjs'; import { @@ -219,103 +219,88 @@ 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 !== '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'"); expect(SCRIPT).toContain("verdict.result !== 'COMMENT'"); expect(SCRIPT.indexOf('waitForBaseControlledReviewCanary')).toBeLessThan( SCRIPT.indexOf("kind: 'squad-bootstrap-trigger-probe'"), ); + expect(SCRIPT).not.toMatch(/SQUAD_REVIEW_APP_|squad-review-authority|publisher_app_/); }); - 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, - }, + 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 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' }, + id: 123, name: 'review', head_sha: CAST_SHA, status: 'completed', conclusion: 'success', + app: { id: 15368, slug: 'github-actions' }, }; - const attestation = { - publisher_app_id: 424242, - publisher_app_slug: 'squad-review-authority', + 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}`); + } }; - 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(); + 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', () => { diff --git a/test/gh-aw-review-guard.test.ts b/test/gh-aw-review-guard.test.ts index 42cddf84a..4535d13c2 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, }; @@ -196,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(); @@ -286,39 +274,91 @@ describe('independent Squad review guard', () => { } }); - it('ignores an ordinary-token forged check and accepts only the dedicated App check', async () => { + it.each([false, true])('selects a successful rerun with retained attempt-1 rejection (reverse=%s)', async reverse => { 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); + 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('missing or duplicate exact-head authority check'); + .rejects.toThrow('duplicate verdict evidence'); }); - it('refuses PR-controlled shaped review, job, check, and artifact evidence', async () => { + 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'; 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 +418,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 +446,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 +482,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 +583,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..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'); @@ -113,9 +120,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 +133,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 +170,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 +211,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 +299,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 +311,25 @@ 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'); + 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', () => { @@ -560,7 +437,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 +445,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 +471,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 +629,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..696099b65 100644 --- a/workflows/package/squad.md +++ b/workflows/package/squad.md @@ -2238,15 +2238,15 @@ 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. 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 +`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 `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 +2259,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..4761dda04 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'; @@ -86,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'); @@ -299,11 +294,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) && @@ -353,15 +345,22 @@ 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(); 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 +371,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..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": "bb7f231c852dfb73c5d1a6ba5e9f23f9f7c9fc5af9e1f1021c4c5213fd9389f9", - "lock_sha256": "f2e62f0ded2b937c99dcdab321f1ea2f886ff2f0537a324af4342908466a2bcf" + "source_sha256": "e3cc1b34c38d345eb5c843d4c4cf4966d7bbdc8c78c567cd3fe44aa84abd0fe7", + "lock_sha256": "85aa7c5cbda5453d71af43897088fc06bdb0771c4ae825d88721c4e12e4feaf1" }, { "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": "95472a06daabc03428955e659a4f3c548afc316427ea48196760cc992891cad0" }, { "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..b48981e15 100644 --- a/workflows/squad.md +++ b/workflows/squad.md @@ -1135,15 +1135,15 @@ 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. 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 +`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 `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 +1156,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