From 7e41a5f58dc6b1db8f2d8bce9c01cff9d90797b1 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Sat, 31 Oct 2026 13:06:10 +0100 Subject: [PATCH] fix(override): make approved overrides unblock app repos and split FAILED from HIGH approve-override.yml re-ran the blocked run with the platform repo's github.token, which cannot touch app repos, so an approved override never restarted an app's apply. The repo and run now come from the plan key (validated before signing); platform runs are still re-run automatically and app runs get the exact "Re-run failed jobs" step in the job summary and Slack. invoke-plan-review.sh retries reviews the service could not complete, and the logs, PR comment, Slack alert and blocked Terraform Apply say whether the review service failed or the review found HIGH risk, with how to request an override. docs/apply-gate.md documents the request path. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/approve-override.yml | 35 ++-- CLAUDE.md | 6 +- docs/apply-gate.md | 65 ++++++- docs/ci-workflow.md | 2 +- docs/lambda-functions.md | 2 +- docs/reusable-workflows.md | 6 +- scripts/invoke-apply-gate.sh | 1 + scripts/invoke-plan-review.sh | 72 ++++++-- scripts/override-help.sh | 51 ++++++ scripts/override-target.sh | 70 ++++++++ scripts/tests/test_override_flow.py | 168 ++++++++++++++++++ terraform/lambda-src/plan_review/handler.py | 28 ++- .../lambda-src/tests/test_plan_review.py | 29 ++- 13 files changed, 486 insertions(+), 49 deletions(-) create mode 100644 scripts/override-help.sh create mode 100644 scripts/override-target.sh create mode 100644 scripts/tests/test_override_flow.py diff --git a/.github/workflows/approve-override.yml b/.github/workflows/approve-override.yml index bca1177..2d7c4a1 100644 --- a/.github/workflows/approve-override.yml +++ b/.github/workflows/approve-override.yml @@ -2,21 +2,26 @@ name: Approve Override # Workflow dispatch — only runs after approval from the 'override-approval' # GitHub environment (requires board member review). +# +# Only plan_key and reason are needed: the repo and run come from the plan +# key. For a platform run the failed jobs are re-run here; github.token cannot +# touch app repos, so for an app run the job prints the run URL and the +# developer clicks "Re-run failed jobs" (docs/apply-gate.md#requesting-an-override). on: workflow_dispatch: inputs: plan_key: - description: "S3 plan key (from the Slack alert)" + description: "S3 plan key, plans/javaBin///tfplan (from the blocked run's log or the Slack alert)" required: true type: string repo: - description: "Repository (e.g. javaBin/moresleep)" - required: true + description: "Optional check: repository (e.g. javaBin/moresleep); must match the plan key" + required: false type: string run_id: - description: "Failed apply run ID to retrigger" - required: true + description: "Optional check: blocked run ID; must match the plan key" + required: false type: string reason: description: "Reason for override" @@ -42,6 +47,15 @@ jobs: with: sparse-checkout: scripts + # Before signing: a plan key that names no run is refused here. + - name: Resolve blocked run + id: target + env: + PLAN_KEY: ${{ inputs.plan_key }} + REPO: ${{ inputs.repo }} + RUN_ID: ${{ inputs.run_id }} + run: sh scripts/override-target.sh resolve + - name: Configure AWS credentials via OIDC uses: aws-actions/configure-aws-credentials@v6 with: @@ -60,14 +74,15 @@ jobs: - name: Notify Slack env: SSM_WEBHOOK_PARAM: /javabin/slack/platform-override-alerts-webhook - REPO: ${{ inputs.repo }} + REPO: ${{ steps.target.outputs.repo }} + RUN_URL: ${{ steps.target.outputs.run_url }} PLAN_KEY: ${{ inputs.plan_key }} REASON: ${{ inputs.reason }} - run: sh scripts/notify-slack.sh "Risk Override Approved" "*Repo:* ${REPO}\n*Plan:* \`${PLAN_KEY}\`\n*By:* ${GITHUB_ACTOR}\n*Reason:* ${REASON}" + run: sh scripts/notify-slack.sh "Risk Override Approved" "*Repo:* ${REPO}\n*Plan:* \`${PLAN_KEY}\`\n*By:* ${GITHUB_ACTOR}\n*Reason:* ${REASON}\n*Next:* Re-run failed jobs on the blocked run (automatic for javaBin/platform)" "$RUN_URL" "Blocked run" - name: Retrigger failed apply env: GH_TOKEN: ${{ github.token }} - RUN_ID: ${{ inputs.run_id }} - REPO: ${{ inputs.repo }} - run: gh run rerun "$RUN_ID" --repo "$REPO" --failed + RUN_ID: ${{ steps.target.outputs.run_id }} + REPO: ${{ steps.target.outputs.repo }} + run: sh scripts/override-target.sh retrigger diff --git a/CLAUDE.md b/CLAUDE.md index 6e8b747..11d8ec6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -152,7 +152,7 @@ terraform/state/ tf-apply.yml SHA verify + apply via project role eb-deploy.yml Elastic Beanstalk deploy (transition) ecs-deploy.yml ECS task definition update - approve-override.yml Risk gate override (board members) + approve-override.yml Risk gate override (platform team; docs/apply-gate.md#requesting-an-override) expand-terraform.yml Validate app.yaml, generate terraform/, commit it ``` @@ -193,7 +193,9 @@ terraform/state/ | `scripts/provision-atlassian.py` | CI: resolve atlassian.yaml to java.no emails, invoke team-provisioner `sync_atlassian` (report-only unless `atlassian_sync_apply`) | | `scripts/sync-members.py` | CI: sync approved heroes from Google Sheets into members.yaml | | `scripts/review-plan.py` | CI: LLM plan review via Bedrock (platform CI; also bundled into the plan-review Lambda) | -| `scripts/invoke-plan-review.sh` | CI: app tf-plan → plan-review Lambda | +| `scripts/invoke-plan-review.sh` | CI: app tf-plan → plan-review Lambda; retries FAILED (service) reviews, says FAILED vs HIGH (tests: `scripts/tests/`) | +| `scripts/override-help.sh` | CI: what a blocked apply tells the developer (HIGH: request override; FAILED: re-run all jobs) | +| `scripts/override-target.sh` | CI (approve-override): repo + run from the plan key; re-runs platform runs, prints the re-run step for app repos | | `scripts/notify-slack.sh` | CI: generic Slack webhook notification (via `slack_post.py`) | | `scripts/notify-high-risk.sh` | CI: blocked-apply alert (risk HIGH or review FAILED) with plan key + override link (via `slack_post.py`) | | `scripts/slack_post.py` | CI: builds escaped, size-limited Block Kit, checks Slack's reply, falls back to plain text (tests: `scripts/tests/`) | diff --git a/docs/apply-gate.md b/docs/apply-gate.md index e282d71..fd820b2 100644 --- a/docs/apply-gate.md +++ b/docs/apply-gate.md @@ -86,7 +86,7 @@ other registered team's `apps/{team}/` prefix. The list is built from | LOW | Auto-approve, credentials issued | | MEDIUM | Auto-approve, credentials issued | | HIGH | Blocked until override token exists and HMAC is valid | -| FAILED | Blocked (review script error) | +| FAILED | Blocked: the review service gave no verdict (Bedrock or Lambda failure). Not a risk finding; re-run all jobs or override | ## Override Tokens @@ -109,13 +109,66 @@ An override token is an HMAC-signed JSON file stored in S3 alongside the plan: ## Override Flow -1. Apply blocks on HIGH risk → Slack alert with plan key and override link -2. Admin triggers `approve-override.yml` in the platform repo - - Protected by GitHub environment `override-approval` (requires board member review) +1. Apply blocks on HIGH or FAILED → Terraform Apply fails with what to do + (see below), and a Slack alert with the plan key, run link and override link + goes to the override channel +2. A platform team member dispatches `approve-override.yml` in the platform repo + with the plan key and a reason + - Protected by GitHub environment `override-approval` (requires review by the + `javaBin/platform` team) + - The repo and run are read from the plan key + (`plans/javaBin///tfplan`); the optional `repo` and `run_id` + inputs only have to match it. A malformed key fails before anything is signed 3. Override workflow assumes `javabin-ci-override-approver` OIDC role 4. Invokes gate Lambda `sign` action → Lambda computes HMAC, writes `override.json` to S3 -5. Workflow retriggers the failed apply run via `gh run rerun --failed` -6. Apply re-runs → gate Lambda finds `override.json` → verifies HMAC → issues credentials +5. The blocked run is re-run: + - javaBin/platform runs: the workflow runs `gh run rerun --failed` itself + - app repos: the workflow's `github.token` only covers javaBin/platform (no + platform GitHub App has `actions: write`), so the job summary and Slack + message give the run URL and the developer clicks **Re-run failed jobs** +6. Apply re-runs → gate Lambda finds `override.json` → verifies HMAC → issues + credentials. The gate checks for an override on every run, so no other state + has to change. Docker build and ECS deploy, which wait for a successful + apply, run after it. + +## Requesting an override + +A HIGH or FAILED review blocks Terraform Apply, and with it the image build +and ECS deploy of the same run. The Terraform Apply job prints which case it +is, and the plan key. + +**HIGH — the review found HIGH risk.** Read the review in the Terraform Plan +job log or the PR comment. If the change is intended: + +1. Ask the platform team (GitHub team `javaBin/platform`, or the override Slack + channel where the alert was posted) for an override. Send the plan key + (`plans/javaBin///tfplan`), the run URL, and why the change is + safe. +2. They run [approve-override.yml](https://github.com/javaBin/platform/actions/workflows/approve-override.yml) + with that plan key and a reason, or: + + ```bash + gh workflow run approve-override.yml -R javaBin/platform \ + -f plan_key=plans/javaBin///tfplan -f reason="..." + ``` + + A `javaBin/platform` team member must then approve the `override-approval` + environment. +3. When it has finished, open your blocked run and click **Re-run failed jobs**. + +**FAILED — the review service failed.** This is not a risk finding: Bedrock +or the plan-review Lambda did not return a verdict. `invoke-plan-review.sh` +already retries (3 invokes by default, each with 3 Bedrock attempts) before +giving up. Click **Re-run all jobs** on the run later; that re-plans and +re-reviews. **Re-run failed jobs** does not help, since it re-reads the same +FAILED verdict. If the service stays down, request an override as for HIGH, +then click **Re-run failed jobs**. + +The override is for that plan key (HMAC over the key) and expires with the +plan bucket's lifecycle. A push of a new commit makes a new run and plan key, +which is reviewed afresh. Known gap: **Re-run all jobs** re-plans the same +commit under the same key (the key holds the run ID, not the attempt), and an +`override.json` signed for an earlier attempt is still accepted. ## S3 Plan Artifact Layout diff --git a/docs/ci-workflow.md b/docs/ci-workflow.md index 9440894..1dc3ef7 100644 --- a/docs/ci-workflow.md +++ b/docs/ci-workflow.md @@ -44,7 +44,7 @@ Monday 08:00 ──► securityhub-summary Lambda ──► #javabin-infra-ale - **LOW/MEDIUM** → auto-apply - **HIGH** → blocked, Slack notification sent, board member must run approve-override workflow -- **FAILED** → blocked (review script error) +- **FAILED** → blocked (the review could not run; a service failure, not a risk finding). See [Requesting an override](apply-gate.md#requesting-an-override) ## Plan Artifact Security diff --git a/docs/lambda-functions.md b/docs/lambda-functions.md index 8f490d9..d4d0004 100644 --- a/docs/lambda-functions.md +++ b/docs/lambda-functions.md @@ -226,7 +226,7 @@ Idempotent: skips resources that already have a `created-by` tag (preserves orig **Trigger:** Invoked by app `tf-plan` (via the `javabin-ci-app-broker` OIDC role) after the plan is uploaded **Purpose:** LLM risk review of app Terraform plans with platform credentials. Team CI roles have no Bedrock access (developer boundary), and any code in a plan job runs with them. -**Flow:** Verify the job's GitHub OIDC token (aud `javabin-plan-review`) → plan must be under `plans/javaBin//` → read `plan-output.txt` → review with Bedrock using `scripts/review-plan.py` (bundled as `review_plan.py`), up to 3 attempts with backoff → write `risk.json` (read by apply-gate) and `review.json` → on HIGH/FAILED for pushes to main, post to the override Slack channel → return risk + markdown for the PR comment. +**Flow:** Verify the job's GitHub OIDC token (aud `javabin-plan-review`) → plan must be under `plans/javaBin//` → read `plan-output.txt` → review with Bedrock using `scripts/review-plan.py` (bundled as `review_plan.py`), up to 3 attempts with backoff → write `risk.json` (read by apply-gate) and `review.json` → on HIGH/FAILED for pushes to main, post to the override Slack channel (HIGH as a risk finding, FAILED as a service failure with no verdict; FAILED is not posted while the caller passes `retrying: true`) → return risk + markdown for the PR comment. Team roles are denied writing `risk.json`, `review.json` and `override.json` in the plan bucket. `risk.json` is written as FAILED before the review starts, so a crash or timeout still leaves a verdict the gate (and a board override) can act on. The plan text is uploaded by the plan job itself, so the review catches mistakes; it is not a control against code in the job forging its own plan text. diff --git a/docs/reusable-workflows.md b/docs/reusable-workflows.md index 76c7ad0..ef72d07 100644 --- a/docs/reusable-workflows.md +++ b/docs/reusable-workflows.md @@ -133,14 +133,14 @@ Docker BuildKit build + push to ECR repo `{team}-{app_name}`. Uses GitHub Action ### tf-plan.yml -Plans the committed `terraform/` at the branch head (after expand), uploads the plan to S3 with its SHA256, has the `javabin-plan-review` Lambda review it (Bedrock Claude Haiku 5.5, platform credentials; it writes `risk.json`) and posts plan + review to the PR. A review that cannot run after retries is `FAILED`: re-run the workflow, or request an override if it persists. +Plans the committed `terraform/` at the branch head (after expand), uploads the plan to S3 with its SHA256, has the `javabin-plan-review` Lambda review it (Bedrock Claude Haiku 5.5, platform credentials; it writes `risk.json`) and posts plan + review to the PR. A review the service could not run is `FAILED`, which is a service failure, not a risk finding: `invoke-plan-review.sh` retries it (`REVIEW_INVOKE_ATTEMPTS`, default 3) and the log says which case it is (`Plan review service failed` or `Plan review found HIGH risk`). Both block the apply; see [Requesting an override](apply-gate.md#requesting-an-override). **AWS role:** `javabin-ci-app-broker` (OIDC) → broker Lambda → `javabin-ci-plan-{team}` (read-only: refresh, state read, state lock, plan upload; tf-plan runs on any branch, so it cannot change resources); the review step uses the broker OIDC role to invoke `javabin-plan-review` **Outputs:** `has_changes`, `plan_key`, `plan_sha256`, `risk_level`, `planned_sha` ### tf-apply.yml -Checks out the commit tf-plan planned, verifies the plan SHA256 and asks the apply-gate Lambda for credentials (HIGH/FAILED risk needs a board-member override), then applies. +Checks out the commit tf-plan planned, verifies the plan SHA256 and asks the apply-gate Lambda for credentials (HIGH/FAILED risk needs an override approved by the platform team), then applies. A blocked apply prints the plan key and what to do; docker-build and ecs-deploy wait for it. See [Requesting an override](apply-gate.md#requesting-an-override). **AWS role:** `javabin-ci-apply-gate` (OIDC) → apply-gate Lambda → `javabin-ci-team-{team}` @@ -156,7 +156,7 @@ Transitional Elastic Beanstalk deploy via EB CLI. Will be removed per-repo when ### approve-override.yml -`workflow_dispatch` — only board members can trigger. Writes a signed override for a blocked plan. Posts confirmation to Slack. +`workflow_dispatch` behind the `override-approval` environment (reviewed by the `javaBin/platform` team). Inputs: `plan_key` and `reason`; the repo and run come from the plan key. Writes a signed override for the blocked plan and posts to Slack. Platform runs are re-run automatically; for an app repo the job prints the run URL and the developer clicks **Re-run failed jobs** (`github.token` cannot re-run another repo's workflows). **AWS role:** `javabin-ci-override-approver` diff --git a/scripts/invoke-apply-gate.sh b/scripts/invoke-apply-gate.sh index 6cc9473..01d7e61 100644 --- a/scripts/invoke-apply-gate.sh +++ b/scripts/invoke-apply-gate.sh @@ -56,6 +56,7 @@ echo "Risk level: $RISK" if [ "$APPROVED" != "true" ]; then echo "Apply blocked: ${REASON:-risk gate denied}" + sh "$(dirname "$0")/override-help.sh" "$RISK" "$PLAN_KEY" # Notify Slack about the block if [ -n "$SSM_WEBHOOK_PARAM" ]; then diff --git a/scripts/invoke-plan-review.sh b/scripts/invoke-plan-review.sh index 36efdc2..65baaf1 100644 --- a/scripts/invoke-plan-review.sh +++ b/scripts/invoke-plan-review.sh @@ -7,36 +7,74 @@ # access). The Lambda verifies this job's GitHub OIDC token, reviews the # plan text next to , and writes risk.json itself. # -# Env: NOTIFY=true to post HIGH/FAILED to the override Slack channel. +# A FAILED review means the review service did not produce a verdict (invoke +# error, Lambda timeout, Bedrock outage); it is not a risk finding. Those are +# retried up to REVIEW_INVOKE_ATTEMPTS times (default 3), REVIEW_RETRY_DELAY +# seconds apart (doubling). Refusals (bad OIDC token, someone else's plan) +# are not retried. The gate still blocks a plan whose last verdict is FAILED. +# +# Env: NOTIFY=true to post HIGH/FAILED to the override Slack channel (FAILED +# only once retries are used up). # Output: review-output.txt (markdown for the PR comment), # risk_level= in GITHUB_OUTPUT. set -e PLAN_KEY="${1:?Usage: invoke-plan-review.sh }" +ATTEMPTS="${REVIEW_INVOKE_ATTEMPTS:-3}" +DELAY="${REVIEW_RETRY_DELAY:-20}" ID_TOKEN=$(sh "$(dirname "$0")/github-oidc-token.sh" "${PROJECT:-javabin}-plan-review") -# A failed invoke is a FAILED review, not a failed job: the PR still gets a -# comment and the gate still blocks. -echo '{}' > /tmp/plan-review.json -aws lambda invoke \ - --function-name "${PROJECT:-javabin}-plan-review" \ - --cli-read-timeout 310 \ - --payload "$(jq -nc --arg plan_key "$PLAN_KEY" --arg id_token "$ID_TOKEN" --argjson notify "${NOTIFY:-false}" \ - '{plan_key: $plan_key, id_token: $id_token, notify: $notify}')" \ - --cli-binary-format raw-in-base64-out \ - /tmp/plan-review.json >/dev/null || echo '{"error": "could not invoke the plan-review Lambda"}' > /tmp/plan-review.json - -ERROR=$(jq -r '.error // .errorMessage // (if .risk then empty else "empty response" end)' /tmp/plan-review.json) +attempt=1 +while :; do + # The caller retries a FAILED result itself, so the Lambda alerts on it only + # on the last attempt (HIGH is final and always alerts). + RETRYING=false + [ "$attempt" -lt "$ATTEMPTS" ] && RETRYING=true + + # A failed invoke is a FAILED review, not a failed job: the PR still gets a + # comment and the gate still blocks. + echo '{}' > /tmp/plan-review.json + aws lambda invoke \ + --function-name "${PROJECT:-javabin}-plan-review" \ + --cli-read-timeout 310 \ + --payload "$(jq -nc --arg plan_key "$PLAN_KEY" --arg id_token "$ID_TOKEN" \ + --argjson notify "${NOTIFY:-false}" --argjson retrying "$RETRYING" \ + '{plan_key: $plan_key, id_token: $id_token, notify: $notify, retrying: $retrying}')" \ + --cli-binary-format raw-in-base64-out \ + /tmp/plan-review.json >/dev/null || echo '{"error": "could not invoke the plan-review Lambda"}' > /tmp/plan-review.json + + ERROR=$(jq -r '.error // .errorMessage // (if .risk then empty else "empty response" end)' /tmp/plan-review.json) + RISK=$(jq -r '.risk // "FAILED"' /tmp/plan-review.json) + [ -n "$ERROR" ] && RISK=FAILED + + case "$ERROR" in + "Invalid GitHub OIDC token"*|*"does not belong to"*) break ;; # refused, retrying will not help + esac + [ "$RISK" != "FAILED" ] && break + [ "$attempt" -ge "$ATTEMPTS" ] && break + + echo "::warning::Plan review service failed (attempt ${attempt}/${ATTEMPTS}): ${ERROR:-no verdict}. Retrying in ${DELAY}s." + sleep "$DELAY" + DELAY=$((DELAY * 2)) + attempt=$((attempt + 1)) +done + if [ -n "$ERROR" ]; then - echo "::error::Plan review failed: $ERROR" - RISK=FAILED - printf '**Risk: ⚪ FAILED**\n\nPlan review could not run: %s\n' "$ERROR" > review-output.txt + printf '**Risk: ⚪ FAILED**\n\nThe plan-review service could not review this plan: %s\n' "$ERROR" > review-output.txt else - RISK=$(jq -r '.risk' /tmp/plan-review.json) jq -r '.markdown' /tmp/plan-review.json > review-output.txt fi +case "$RISK" in + FAILED) + echo "::error title=Plan review service failed::No risk verdict after ${attempt} attempt(s): ${ERROR:-the review did not complete}. This is a service failure, not a risk finding. Terraform Apply will block: re-run all jobs later, or request an override (https://github.com/javaBin/platform/blob/main/docs/apply-gate.md#requesting-an-override)." + ;; + HIGH) + echo "::error title=Plan review found HIGH risk::The reviewer rated this plan HIGH risk. Terraform Apply will block until the platform team approves an override (https://github.com/javaBin/platform/blob/main/docs/apply-gate.md#requesting-an-override)." + ;; +esac + cat review-output.txt echo "risk_level=${RISK}" >> "${GITHUB_OUTPUT:-/dev/null}" echo "Plan review risk: ${RISK}" diff --git a/scripts/override-help.sh b/scripts/override-help.sh new file mode 100644 index 0000000..c1ff822 --- /dev/null +++ b/scripts/override-help.sh @@ -0,0 +1,51 @@ +#!/bin/sh +# Tell the developer how to get a blocked apply through (invoke-apply-gate.sh). +# +# Usage: override-help.sh +# +# HIGH: the review found HIGH risk; a platform team member must approve an +# override, then the developer re-runs the failed jobs. +# FAILED: the review service gave no verdict (not a risk finding); re-running +# all jobs re-plans and re-reviews. "Re-run failed jobs" would only +# re-read the FAILED verdict. An override also works. +# Other risk levels (or none): nothing to add. +# +# Writes an ::error annotation and the same text to the job summary. +# Env: GITHUB_SERVER_URL, GITHUB_REPOSITORY, GITHUB_RUN_ID, GITHUB_STEP_SUMMARY + +RISK="$1" +PLAN_KEY="$2" +SERVER="${GITHUB_SERVER_URL:-https://github.com}" +RUN_URL="${SERVER}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}" +DOC="${SERVER}/javaBin/platform/blob/main/docs/apply-gate.md#requesting-an-override" +APPROVE="${SERVER}/javaBin/platform/actions/workflows/approve-override.yml" + +case "$RISK" in + HIGH) + TITLE="Apply blocked — the plan review found HIGH risk" + WHY="Read the review in the Terraform Plan job (or the PR comment). If the change is intended, ask the platform team (GitHub team javaBin/platform) to approve an override." + AFTER="When the override is approved, open ${RUN_URL} and click \"Re-run failed jobs\"." + ;; + FAILED) + TITLE="Apply blocked — the plan-review service failed (no risk verdict)" + WHY="This is a service failure, not a risk finding. Open ${RUN_URL} and click \"Re-run all jobs\" to re-plan and re-review (\"Re-run failed jobs\" reuses the FAILED verdict). If the review keeps failing, ask the platform team (GitHub team javaBin/platform) for an override." + AFTER="After an override, click \"Re-run failed jobs\" instead." + ;; + *) + exit 0 + ;; +esac + +ASK="Send them the plan key \`${PLAN_KEY}\` and this run (${RUN_URL}). They run ${APPROVE} with plan_key=${PLAN_KEY} and a reason, or: gh workflow run approve-override.yml -R javaBin/platform -f plan_key=${PLAN_KEY} -f reason=\"...\"" + +echo "::error title=${TITLE}::${WHY} ${AFTER} Details: ${DOC}" +echo "$ASK" +{ + echo "### ${TITLE}" + echo + echo "$WHY" + echo + echo "$ASK" + echo + echo "$AFTER See ${DOC}" +} >> "${GITHUB_STEP_SUMMARY:-/dev/null}" diff --git a/scripts/override-target.sh b/scripts/override-target.sh new file mode 100644 index 0000000..bdecab8 --- /dev/null +++ b/scripts/override-target.sh @@ -0,0 +1,70 @@ +#!/bin/sh +# Resolve and retrigger the blocked run an override is for (approve-override.yml). +# +# Usage: override-target.sh resolve +# override-target.sh retrigger +# +# resolve: reads the repo and run ID from PLAN_KEY +# (plans////tfplan, as written by upload-plan.sh). +# REPO and RUN_ID are optional; if given they must match the plan. +# Writes repo, run_id and run_url to GITHUB_OUTPUT. Run it before +# signing, so a typo never signs an override for the wrong plan. +# retrigger: re-runs the failed jobs of the blocked run when it is in this +# repo (GH_TOKEN is this workflow's github.token, which cannot +# touch other repos). For an app repo it prints, and writes to the +# job summary, the step the developer takes instead: open the run +# and click "Re-run failed jobs". The apply gate checks for the +# override every time it runs, so a plain re-run is enough. +# +# Env: PLAN_KEY, REPO, RUN_ID, GITHUB_REPOSITORY, GITHUB_SERVER_URL, +# GITHUB_OUTPUT, GITHUB_STEP_SUMMARY, GH_TOKEN (retrigger) + +set -e + +SERVER="${GITHUB_SERVER_URL:-https://github.com}" + +case "$1" in + resolve) + # plans////tfplan; names are GitHub-safe characters only + PARSED=$(printf '%s\n' "$PLAN_KEY" | + sed -n 's#^plans/\([A-Za-z0-9_.-]*\)/\([A-Za-z0-9_.-]*\)/\([0-9][0-9]*\)/tfplan$#\1/\2 \3#p') + if [ -z "$PARSED" ]; then + echo "::error::plan_key '${PLAN_KEY}' is not plans////tfplan. Copy it from the blocked run's Terraform Apply log or the Slack alert." + exit 1 + fi + PLAN_REPO="${PARSED% *}" + PLAN_RUN="${PARSED#* }" + if [ -n "$REPO" ] && [ "$REPO" != "$PLAN_REPO" ]; then + echo "::error::repo '${REPO}' does not match the plan's repo '${PLAN_REPO}'." + exit 1 + fi + if [ -n "$RUN_ID" ] && [ "$RUN_ID" != "$PLAN_RUN" ]; then + echo "::error::run_id '${RUN_ID}' does not match the plan's run '${PLAN_RUN}'." + exit 1 + fi + { + echo "repo=${PLAN_REPO}" + echo "run_id=${PLAN_RUN}" + echo "run_url=${SERVER}/${PLAN_REPO}/actions/runs/${PLAN_RUN}" + } >> "${GITHUB_OUTPUT:-/dev/null}" + echo "Override target: ${PLAN_REPO} run ${PLAN_RUN}" + ;; + + retrigger) + RUN_URL="${SERVER}/${REPO}/actions/runs/${RUN_ID}" + if [ "$REPO" = "$GITHUB_REPOSITORY" ]; then + gh run rerun "$RUN_ID" --repo "$REPO" --failed + MSG="Override signed. Re-running the failed jobs of ${RUN_URL}" + else + MSG="Override signed. Next step for the developer: open ${RUN_URL} and click \"Re-run failed jobs\". Terraform Apply will find the override and apply; the image build and ECS deploy follow." + echo "::notice title=Re-run the blocked apply::${MSG}" + fi + echo "$MSG" + echo "$MSG" >> "${GITHUB_STEP_SUMMARY:-/dev/null}" + ;; + + *) + echo "Usage: override-target.sh resolve|retrigger" >&2 + exit 2 + ;; +esac diff --git a/scripts/tests/test_override_flow.py b/scripts/tests/test_override_flow.py new file mode 100644 index 0000000..9e85c50 --- /dev/null +++ b/scripts/tests/test_override_flow.py @@ -0,0 +1,168 @@ +"""Tests for the override flow scripts — run: python3 -m unittest discover scripts/tests + +invoke-plan-review.sh (retries, FAILED vs HIGH), override-help.sh (what a +blocked apply tells the developer) and override-target.sh (approve-override.yml). +""" + +import json +import os +import subprocess +import tempfile +import unittest + +HERE = os.path.dirname(os.path.abspath(__file__)) +SCRIPTS = os.path.join(HERE, "..") +PLAN = "plans/javaBin/frosting/123/tfplan" + + +def fake_bin(tmp, name, body): + path = os.path.join(tmp, "bin", name) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as f: + f.write("#!/bin/sh\n" + body) + os.chmod(path, 0o755) + + +def read(path): + with open(path) as f: + return f.read() + + +def run(script, args, tmp, env=None, check=False): + files = {k: os.path.join(tmp, k) for k in ("output", "summary")} + for p in files.values(): + open(p, "w").close() + full_env = dict(os.environ, PATH=f"{tmp}/bin:{os.environ['PATH']}", GITHUB_OUTPUT=files["output"], + GITHUB_STEP_SUMMARY=files["summary"], GITHUB_REPOSITORY="javaBin/frosting", + GITHUB_RUN_ID="123", GITHUB_SERVER_URL="https://github.com") + full_env.update(env or {}) + r = subprocess.run(["sh", os.path.join(SCRIPTS, script), *args], cwd=tmp, env=full_env, + capture_output=True, text=True, check=check) + out = {k: read(p) for k, p in files.items()} + return r, out + + +class InvokePlanReviewTest(unittest.TestCase): + """aws answers with responses[0], responses[1], ...; payloads are recorded.""" + + def review(self, responses, attempts=3): + with tempfile.TemporaryDirectory() as tmp: + for i, resp in enumerate(responses): + with open(os.path.join(tmp, f"resp{i}"), "w") as f: + f.write(resp if isinstance(resp, str) else json.dumps(resp)) + fake_bin(tmp, "curl", 'echo \'{"value": "tok"}\'\n') + # Last arg is the outfile; the payload follows --payload. + fake_bin(tmp, "aws", f''' +n=$(cat "{tmp}/count" 2>/dev/null || echo 0); echo $((n + 1)) > "{tmp}/count" +while [ "$#" -gt 1 ]; do [ "$1" = "--payload" ] && echo "$2" >> "{tmp}/payloads"; shift; done +[ -f "{tmp}/resp$n" ] || exit 255 +[ "$(cat "{tmp}/resp$n")" = "INVOKE_ERROR" ] && exit 255 +cp "{tmp}/resp$n" "$1" +''') + r, out = run("invoke-plan-review.sh", [PLAN], tmp, env={ + "ACTIONS_ID_TOKEN_REQUEST_URL": "http://x/?a=b", "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "t", + "REVIEW_INVOKE_ATTEMPTS": str(attempts), "REVIEW_RETRY_DELAY": "0", "NOTIFY": "true"}) + self.assertEqual(r.returncode, 0, r.stderr) + payloads = [json.loads(line) for line in read(os.path.join(tmp, "payloads")).splitlines()] + return r.stdout, out["output"], payloads, read(os.path.join(tmp, "review-output.txt")) + + def test_transient_failure_is_retried_until_a_verdict(self): + stdout, output, payloads, _ = self.review( + ["INVOKE_ERROR", {"errorMessage": "Task timed out"}, {"risk": "LOW", "markdown": "ok"}]) + self.assertIn("risk_level=LOW", output) + self.assertEqual(len(payloads), 3) + self.assertIn("Retrying", stdout) + self.assertNotIn("::error", stdout) + + def test_only_the_last_attempt_lets_the_lambda_alert_on_failed(self): + _, _, payloads, _ = self.review([{"risk": "FAILED", "markdown": "x"}] * 3) + self.assertEqual([p["retrying"] for p in payloads], [True, True, False]) + self.assertTrue(all(p["notify"] for p in payloads)) + + def test_persistent_failure_says_service_failed_not_risk(self): + stdout, output, payloads, md = self.review(["INVOKE_ERROR"] * 2, attempts=2) + self.assertIn("risk_level=FAILED", output) + self.assertEqual(len(payloads), 2) + self.assertIn("title=Plan review service failed", stdout) + self.assertIn("not a risk finding", stdout) + self.assertNotIn("found HIGH risk", stdout) + self.assertIn("could not invoke", md) + + def test_high_is_final_and_says_high_risk(self): + stdout, output, payloads, md = self.review([{"risk": "HIGH", "markdown": "Deletes the database"}]) + self.assertIn("risk_level=HIGH", output) + self.assertEqual(len(payloads), 1) + self.assertIn("title=Plan review found HIGH risk", stdout) + self.assertNotIn("service failed", stdout) + self.assertIn("Deletes the database", md) + + def test_refusal_is_not_retried(self): + _, output, payloads, _ = self.review([{"error": "Plan 'x' does not belong to cupcake"}] * 3) + self.assertIn("risk_level=FAILED", output) + self.assertEqual(len(payloads), 1) + + +class OverrideHelpTest(unittest.TestCase): + def help(self, risk): + with tempfile.TemporaryDirectory() as tmp: + return run("override-help.sh", [risk, PLAN], tmp, check=True) + + def test_high_asks_for_override_then_rerun_failed_jobs(self): + r, out = self.help("HIGH") + self.assertIn("found HIGH risk", r.stdout) + self.assertIn("gh workflow run approve-override.yml -R javaBin/platform -f plan_key=" + PLAN, r.stdout) + self.assertIn('click "Re-run failed jobs"', out["summary"]) + self.assertIn("https://github.com/javaBin/frosting/actions/runs/123", out["summary"]) + + def test_failed_says_service_failed_and_rerun_all_jobs(self): + r, out = self.help("FAILED") + self.assertIn("service failed", r.stdout) + self.assertIn("not a risk finding", r.stdout) + self.assertIn('"Re-run all jobs"', out["summary"]) + + def test_other_risks_print_nothing(self): + r, out = self.help("UNKNOWN") + self.assertEqual(r.stdout, "") + self.assertEqual(out["summary"], "") + + +class OverrideTargetTest(unittest.TestCase): + def target(self, cmd, env): + with tempfile.TemporaryDirectory() as tmp: + fake_bin(tmp, "gh", f'echo "$@" > "{tmp}/gh-args"\n') + r, out = run("override-target.sh", [cmd], tmp, env=dict({"GITHUB_REPOSITORY": "javaBin/platform"}, **env)) + gh = os.path.join(tmp, "gh-args") + out["gh"] = read(gh) if os.path.exists(gh) else None + return r, out + + def test_resolve_reads_repo_and_run_from_plan_key(self): + r, out = self.target("resolve", {"PLAN_KEY": PLAN, "REPO": "", "RUN_ID": ""}) + self.assertEqual(r.returncode, 0, r.stdout) + self.assertIn("repo=javaBin/frosting\n", out["output"]) + self.assertIn("run_id=123\n", out["output"]) + self.assertIn("run_url=https://github.com/javaBin/frosting/actions/runs/123\n", out["output"]) + + def test_resolve_refuses_mismatched_inputs_and_bad_keys(self): + for env in ({"PLAN_KEY": PLAN, "REPO": "javaBin/cupcake"}, + {"PLAN_KEY": PLAN, "RUN_ID": "999"}, + {"PLAN_KEY": "plans/javaBin/frosting/../x/tfplan"}, + {"PLAN_KEY": "javaBin/frosting/123/tfplan"}): + r, out = self.target("resolve", env) + self.assertEqual(r.returncode, 1, env) + self.assertEqual(out["output"], "") + + def test_retrigger_reruns_platform_runs(self): + r, out = self.target("retrigger", {"REPO": "javaBin/platform", "RUN_ID": "77"}) + self.assertEqual(r.returncode, 0) + self.assertEqual(out["gh"], "run rerun 77 --repo javaBin/platform --failed\n") + + def test_retrigger_tells_app_developer_to_rerun(self): + r, out = self.target("retrigger", {"REPO": "javaBin/frosting", "RUN_ID": "123"}) + self.assertEqual(r.returncode, 0) + self.assertIsNone(out["gh"]) # github.token cannot touch app repos + self.assertIn('open https://github.com/javaBin/frosting/actions/runs/123 and click "Re-run failed jobs"', + out["summary"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/terraform/lambda-src/plan_review/handler.py b/terraform/lambda-src/plan_review/handler.py index d7ffd5b..a8c947e 100644 --- a/terraform/lambda-src/plan_review/handler.py +++ b/terraform/lambda-src/plan_review/handler.py @@ -13,9 +13,11 @@ cannot write these objects. (The plan text itself is uploaded by the plan job, so the review guards against mistakes, not against code in the job forging its own plan text.) - 5. For HIGH or FAILED on main, post the override request to Slack. + 5. For HIGH or FAILED on main, post the override request to Slack. A + FAILED verdict (Bedrock or service failure, not a risk finding) is not + posted while the caller says it will retry (retrying=true). -Input: {plan_key, id_token, notify?} +Input: {plan_key, id_token, notify?, retrying?} Output: {risk, markdown} or {error} """ @@ -40,6 +42,7 @@ OIDC_AUDIENCE = os.environ.get("OIDC_AUDIENCE", f"{PROJECT}-plan-review") SLACK_PARAM = os.environ.get("SLACK_WEBHOOK_PARAM", f"/{PROJECT}/slack/platform-override-alerts-webhook") OVERRIDE_URL = f"https://github.com/{GITHUB_ORG}/platform/actions/workflows/approve-override.yml" +OVERRIDE_DOC = f"https://github.com/{GITHUB_ORG}/platform/blob/main/docs/apply-gate.md#requesting-an-override" ATTEMPTS = int(os.environ.get("REVIEW_ATTEMPTS", "3")) s3 = boto3.client("s3") @@ -77,9 +80,16 @@ def put_json(key, body): def notify(repo, plan_key, risk, review): - summary = review.get("summary", "") if review else "The LLM review could not run; re-run the workflow or request an override." - text = (f"*Apply blocked — {risk} risk*\n*Repo:* {GITHUB_ORG}/{repo}\n*Plan:* `{plan_key}`\n{summary}\n" - f"<{OVERRIDE_URL}|Approve override>") + if review: + title = f"*Apply blocked — the review found {risk} risk*" + summary = review.get("summary", "") + else: + title = "*Apply blocked — the plan-review service failed (no risk verdict)*" + summary = "This is a service failure, not a risk finding. Re-run all jobs later, or approve an override if the plan is safe." + run_id = plan_key.split("/")[3] if plan_key.count("/") >= 4 else "" + run = f" | " if run_id else "" + text = (f"{title}\n*Repo:* {GITHUB_ORG}/{repo}\n*Plan:* `{plan_key}`\n{summary}\n" + f"{run}<{OVERRIDE_URL}|Approve override>") try: post_to_slack(get_webhook_url(ssm, SLACK_PARAM), {"text": text}) except Exception as e: @@ -115,10 +125,12 @@ def handler(event, context): put_json(f"{plan_dir}/review.json", review or {"risk": "FAILED"}) logger.info("Reviewed %s: %s", plan_key, risk) - if risk in ("HIGH", "FAILED") and event.get("notify"): + final = risk == "HIGH" or (risk == "FAILED" and not event.get("retrying")) + if final and event.get("notify"): notify(repo, plan_key, risk, review) markdown = review_plan.format_review(review) if review else ( - "**Risk: ⚪ FAILED**\n\nThe LLM review could not run after " - f"{ATTEMPTS} attempts. Re-run the workflow; if it keeps failing, request an override.") + "**Risk: ⚪ FAILED**\n\nThe plan-review service could not review this plan after " + f"{ATTEMPTS} attempts. This is a service failure, not a risk finding. Re-run all jobs " + f"later; if it keeps failing, [request an override]({OVERRIDE_DOC}).") return {"risk": risk, "markdown": markdown} diff --git a/terraform/lambda-src/tests/test_plan_review.py b/terraform/lambda-src/tests/test_plan_review.py index 0cc1fff..f14baf2 100644 --- a/terraform/lambda-src/tests/test_plan_review.py +++ b/terraform/lambda-src/tests/test_plan_review.py @@ -115,7 +115,7 @@ def test_persistent_failure_is_failed_not_high(self): h = load(self.s3, FakeBedrock([RuntimeError("boom")] * 3)) result = h.handler({"plan_key": PLAN, "id_token": "good"}, None) self.assertEqual(result["risk"], "FAILED") - self.assertIn("could not run", result["markdown"]) + self.assertIn("could not review this plan", result["markdown"]) self.assertEqual(json.loads(self.s3.objects["plans/javaBin/frosting/123/risk.json"])["level"], "FAILED") def test_failed_is_recorded_before_review_so_a_crash_leaves_a_verdict(self): @@ -132,6 +132,33 @@ def test_stops_retrying_when_time_runs_out(self): self.assertEqual(h.handler({"plan_key": PLAN, "id_token": "good"}, ctx)["risk"], "FAILED") self.assertEqual(bedrock.calls, 0) + def test_failed_is_not_alerted_while_the_caller_retries(self): + h = load(self.s3, FakeBedrock([RuntimeError("boom")] * 3)) + sent = [] + h.notify = lambda repo, plan_key, risk, review: sent.append(risk) + h.handler({"plan_key": PLAN, "id_token": "good", "notify": True, "retrying": True}, None) + self.assertEqual(sent, []) + + def test_failed_is_alerted_as_service_failure_on_the_last_attempt(self): + h = load(self.s3, FakeBedrock([RuntimeError("boom")] * 3)) + posted = [] + h.get_webhook_url = lambda ssm, param: "https://hooks" + h.post_to_slack = lambda url, body: posted.append(body["text"]) + result = h.handler({"plan_key": PLAN, "id_token": "good", "notify": True, "retrying": False}, None) + self.assertIn("not a risk finding", result["markdown"]) + self.assertEqual(len(posted), 1) + self.assertIn("plan-review service failed", posted[0]) + self.assertIn("https://github.com/javaBin/frosting/actions/runs/123|Blocked run", posted[0]) + + def test_high_is_alerted_even_when_retrying(self): + h = load(self.s3, FakeBedrock([tool_response("HIGH")])) + posted = [] + h.get_webhook_url = lambda ssm, param: "https://hooks" + h.post_to_slack = lambda url, body: posted.append(body["text"]) + h.handler({"plan_key": PLAN, "id_token": "good", "notify": True, "retrying": True}, None) + self.assertEqual(len(posted), 1) + self.assertIn("the review found HIGH risk", posted[0]) + def test_another_repos_plan_is_refused(self): h = load(self.s3, FakeBedrock([]), repo="cupcake") self.assertIn("does not belong", h.handler({"plan_key": PLAN, "id_token": "good"}, None)["error"])