diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 1b2d253ba..cdc5a66a5 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -256,6 +256,7 @@ jobs: tests/dev-lead/unit/test_budget_poller.bats \ tests/test_qa_lead_test_surface.bats \ tests/test_qa_lead_advisory_gate.bats \ + tests/test_persona_pr_pregate.bats \ tests/test_qa_lead_holdout_calibration.bats \ tests/test_advisory_offline_mode.bats \ tests/agent_ingress_reference.bats \ diff --git a/.github/workflows/persona-runner-reusable.yml b/.github/workflows/persona-runner-reusable.yml index 27b596752..a0b35a647 100644 --- a/.github/workflows/persona-runner-reusable.yml +++ b/.github/workflows/persona-runner-reusable.yml @@ -73,6 +73,25 @@ on: required: false type: string default: 'advisory' + surface: + # Which surface the dispatch came from. Defaults to 'mention' so existing + # mention dispatches are unchanged. When 'pull_request', the persona's + # event pre-gate runs before the engine (#1905). The local + # qa-lead-pr-advisory.yml advise job now sets 'pull_request' too, so both + # its run and the router-served run share this workflow's per-(persona, + # repo,PR) concurrency group and the same already-advised marker check — + # the serialization that prevents the two paths double-posting. + required: false + type: string + default: 'mention' + event_action: + # The underlying event action the router observed (e.g. opened, + # ready_for_review). Forwarded for parity with the router payload and + # future per-persona gates; the surface's declared events are bound by + # the router (ADR-0009), so the qa-lead decision does not consider it. + required: false + type: string + default: '' secrets: CLAUDE_CODE_OAUTH_TOKEN: required: true @@ -107,9 +126,18 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 permissions: - contents: read # checkout THIS repo's tooling + prompts. The agent reads - # the public source repo with github.token; the comment is - # posted by the workflow with the persona's manifest PAT. + # checkout THIS repo's tooling + prompts. The agent reads the public source + # repo with github.token; the comment is posted with the persona's manifest + # PAT. + contents: read + # the event pre-gate (#1905) reads the PR's changed files, labels, and + # reviews via github.token — with only contents:read those reads 403 on a + # private source repo and every routed pull_request advisory fails closed + # (skips). + pull-requests: read + # …and the shared issue-comment stream the gate scans for the + # already-advised marker / automation budget. + issues: read steps: - name: Checkout persona tooling + prompts uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -151,6 +179,51 @@ jobs: ;; esac + - name: Event pre-gate — suppress the pull_request surface before the engine + # #1905. The router serves qa-lead's pull_request advisory here with + # surface=pull_request, but binds only the GENERIC brakes — it does NOT + # carry the persona-specific suppressors (no-test-surface, budget- + # exhausted, already-advised) the local qa-lead gate enforces. Run the + # persona's event pre-gate BEFORE the engine so a router-served PR + # dispatch is suppressed with the SAME fidelity as the local surface; + # otherwise the engine runs and posts on every trusted PR. For a mention + # dispatch (surface defaults to 'mention') the gate is a no-op ('run'), + # so existing mention behaviour is unchanged. Fails CLOSED: an unreadable + # signal yields a skip (no engine, no post) with a ::error naming it. + id: pregate + if: steps.resolve.outputs.skip == 'false' + env: + # github.token reads the (public) source repo — the same read scope the + # agent step uses. The pre-gate only READS PR signals; it never posts. + GH_TOKEN: ${{ github.token }} + PERSONA: ${{ inputs.persona }} + SURFACE: ${{ inputs.surface }} + SOURCE_REPO: ${{ inputs.source_repo }} + ITEM_NUMBER: ${{ inputs.item_number }} + EVENT_ACTION: ${{ inputs.event_action }} + run: | + set -euo pipefail + # shellcheck source=scripts/persona-pr-pregate.sh + source scripts/persona-pr-pregate.sh + + decision="$(persona_event_pregate \ + "$PERSONA" "$SURFACE" "$SOURCE_REPO" "$ITEM_NUMBER" "$EVENT_ACTION" || true)" + echo "persona pre-gate for '${PERSONA}' (surface=${SURFACE}) on ${SOURCE_REPO}#${ITEM_NUMBER}: ${decision}" + if [ "$decision" = "run" ]; then + echo "run=true" >> "$GITHUB_OUTPUT" + else + echo "run=false" >> "$GITHUB_OUTPUT" + # Policy skips (no-test-surface, budget-exhausted, already-advised, ...) + # stay green. An unreadable signal, malformed dispatch, or empty + # decision must be a retryable failure, not a silently lost advisory. + case "$decision" in + skip:signal-unavailable | skip:unknown-surface | "") + echo "::error::persona pre-gate could not decide (${decision:-empty}) — failing so the dispatch is retryable" + exit 1 + ;; + esac + fi + - name: Resolve the persona's posting identity from its manifest # Source of truth for who a persona posts as: personas//persona.yml # runtime.identity (the #1316/#1317 pattern). The runner honors the @@ -159,7 +232,7 @@ jobs: # PATs only for the sanctioned posting identities; any other declared # credential fails loudly rather than silently borrowing a default. id: identity - if: steps.resolve.outputs.skip == 'false' + if: steps.resolve.outputs.skip == 'false' && steps.pregate.outputs.run == 'true' env: PERSONA: ${{ inputs.persona }} run: | @@ -199,7 +272,7 @@ jobs: # derived per-persona from the manifest (via the identity step), so every # persona is covered without an enumerated list (AC #6). id: authz_preflight - if: steps.resolve.outputs.skip == 'false' + if: steps.resolve.outputs.skip == 'false' && steps.pregate.outputs.run == 'true' env: # Same manifest-driven PAT selection as the post step — the token that # WILL post is the token we probe. The agent step never sees these. @@ -235,7 +308,7 @@ jobs: bash scripts/persona-authz-preflight.sh - name: Install Claude Code - if: steps.resolve.outputs.skip == 'false' + if: steps.resolve.outputs.skip == 'false' && steps.pregate.outputs.run == 'true' env: CLAUDE_CODE_VERSION: ${{ vars.CLAUDE_CODE_VERSION || 'latest' }} run: | @@ -246,7 +319,7 @@ jobs: npm install -g "@anthropic-ai/claude-code@${CLAUDE_CODE_VERSION}" - name: Produce the advisory (agent — read only, no post) - if: steps.resolve.outputs.skip == 'false' + if: steps.resolve.outputs.skip == 'false' && steps.pregate.outputs.run == 'true' id: advise env: CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} @@ -323,7 +396,7 @@ jobs: - name: Enforce marker and post the comment (workflow — the only writer) id: post - if: steps.resolve.outputs.skip == 'false' && steps.advise.outputs.skip_post == 'false' + if: steps.resolve.outputs.skip == 'false' && steps.pregate.outputs.run == 'true' && steps.advise.outputs.skip_post == 'false' env: # The PAT lives ONLY here. The agent step above never had it. WHICH # PAT is used is driven by the persona manifest diff --git a/.github/workflows/persona-runner.yml b/.github/workflows/persona-runner.yml index 3b943c5ae..9d8025f46 100644 --- a/.github/workflows/persona-runner.yml +++ b/.github/workflows/persona-runner.yml @@ -4,8 +4,15 @@ # # The mention router (in petry-projects/.github) fires `persona-mention` at this # repo with client_payload {persona, mode, source_repo, item_number, comment_url, -# requested_by}. This stub hands that payload to the reusable, which runs the -# addressed persona's advisory prompt. +# requested_by, surface, event_action}. This stub hands that payload to the +# reusable, which runs the addressed persona's advisory prompt. +# +# `surface` selects which suppressors the reusable applies before the engine: +# it defaults to `mention` (existing mention dispatches unchanged), and a +# router-served `pull_request` dispatch (surface=pull_request) triggers the +# persona's event pre-gate — test-surface / budget / already-advised for qa-lead +# (#1905). This reusable is referenced by a LOCAL `./` path (same commit), so the +# new inputs are declared in the same change — no channel-skew concern. # # Thin caller: no logic here beyond mapping the payload to inputs. All behaviour # lives in the reusable. This repo is the `next` ring — the first, smallest @@ -21,7 +28,14 @@ permissions: {} jobs: run: permissions: + # A reusable is granted no more than its calling job, so the read scopes the + # reusable's event pre-gate needs (#1905) must be granted HERE too. Without + # pull-requests/issues:read the shared qa-lead gate 403s on a private source + # repo's PR files/labels/reviews/comments and every routed pull_request + # advisory fails closed (skips). contents: read + pull-requests: read + issues: read uses: ./.github/workflows/persona-runner-reusable.yml with: persona: ${{ github.event.client_payload.persona }} @@ -30,4 +44,7 @@ jobs: comment_url: ${{ github.event.client_payload.comment_url }} requested_by: ${{ github.event.client_payload.requested_by }} mode: ${{ github.event.client_payload.mode }} + # Absent surface -> 'mention' so existing dispatches are unchanged (#1905). + surface: ${{ github.event.client_payload.surface || 'mention' }} + event_action: ${{ github.event.client_payload.event_action || '' }} secrets: inherit diff --git a/.github/workflows/qa-lead-pr-advisory.yml b/.github/workflows/qa-lead-pr-advisory.yml index 1df1035fd..efb9312ad 100644 --- a/.github/workflows/qa-lead-pr-advisory.yml +++ b/.github/workflows/qa-lead-pr-advisory.yml @@ -21,7 +21,11 @@ # # Idempotency (AC #5): the gate scans the PR for an existing marker AND the caller # concurrency group serializes opened + ready_for_review for one PR, so the two -# events cannot both clear the gate and double-post. +# events cannot both clear the gate and double-post. The advise job also forwards +# surface=pull_request, so the reusable re-runs the same already-advised check and +# both the local and router-served qa-lead advisories share ONE reusable +# concurrency group (per persona+repo+PR) — serializing the two paths so they +# cannot both observe "no marker" and post duplicate advisories. # # Soak (AC #9): watch this workflow's runs for the first two weeks. Noise metrics # to report — advisories posted vs. PRs seen, skip-reason histogram (the gate @@ -54,7 +58,8 @@ jobs: timeout-minutes: 5 permissions: contents: read # checkout the gate script + libs - pull-requests: read # read changed files, labels, comments + pull-requests: read # read changed files, labels, reviews + issues: read # read issue comments (the shared PR comment stream) outputs: decision: ${{ steps.decide.outputs.decision }} steps: @@ -72,89 +77,16 @@ jobs: PR: ${{ github.event.pull_request.number }} run: | set -euo pipefail - # shellcheck source=scripts/qa-lead-advisory-gate.sh - source scripts/qa-lead-advisory-gate.sh + # shellcheck source=scripts/qa-lead-pr-gate.sh + source scripts/qa-lead-pr-gate.sh - # Fail closed: qa-lead advises only when EVERY signal is available. - # Silence is the safe default, so any signal-gathering API failure must - # SKIP — never degrade to an empty value ([] labels, [] events, "no - # marker") that would bypass a suppressor or duplicate an advisory. - skip_closed() { - echo "gate: $1 — failing closed (skip) for ${REPO}#${PR}" - echo "decision=skip" >> "$GITHUB_OUTPUT" - exit 0 - } - - # AC #3 — changed files. Fetch the raw JSON array of filenames so the - # list can be validated BEFORE it is flattened to the newline-delimited - # text qa_lead_test_surface consumes. An empty list on success is fine - # (an empty PR does not fire); only an API failure skips. - if ! files_json="$(gh api --paginate --slurp \ - "repos/${REPO}/pulls/${PR}/files" --jq '[.[][].filename]')"; then - skip_closed "changed-file list unavailable" - fi - - # Completeness: the /pulls/{n}/files endpoint caps at 3000 files, so a - # larger PR yields a truncated list that could omit a test/source path - # and mislabel the PR. Compare received vs. the declared changed_files - # count and fail closed (skip) on an incomplete list — never advise off - # partial data. - if ! declared_files="$(gh api "repos/${REPO}/pulls/${PR}" \ - --jq '.changed_files')"; then - skip_closed "changed-file count unavailable" - fi - received_files="$(jq 'length' <<< "$files_json")" - if ! qa_lead_file_list_complete "$received_files" "$declared_files"; then - skip_closed "changed-file list incomplete (${received_files}/${declared_files})" - fi - - # Control characters: a filename containing an LF would split into - # phantom records once flattened (a docs-only PR could forge a SOURCE - # path and trigger the advisory). Reject before flattening. - if qa_lead_paths_have_control_char "$files_json"; then - skip_closed "changed-file list contains control characters" - fi - - changed_paths="$(jq -r '.[]' <<< "$files_json")" - - # AC #4/#6 — label names as a JSON array. A labels API failure must NOT - # degrade to [] — that would bypass the opt-out and human-gating - # suppressors and advise a PR marked hands-off / needs-human-review. - if ! labels_json="$(gh api "repos/${REPO}/pulls/${PR}" \ - --jq '[.labels[]?.name]')"; then - skip_closed "labels unavailable" - fi - - # AC #5 — has a qa-lead advisory already been posted? Scan issue comments - # (the PR shares the issues comment stream) for the recursion marker. A - # comment API failure is indistinguishable from "no marker", so failing - # open here would risk a duplicate advisory — fail closed instead. - marker="" - if ! comment_bodies="$(gh api --paginate \ - "repos/${REPO}/issues/${PR}/comments" --jq '.[].body')"; then - skip_closed "existing-advisory scan unavailable" - fi - if grep -qF "$marker" <<< "$comment_bodies"; then - existing_advisory=1 - else - existing_advisory=0 - fi - - # AC #6 — is the per-PR automation budget exhausted? (#860/#926 breaker) - # gather_pr_automation_events returns non-zero when any API call fails; - # a failure must NOT degrade to an empty event list (an exhausted PR - # would then pass the budget gate during a transient outage), so skip. - budget_exhausted=0 - if events_json="$(gather_pr_automation_events "$PR" "$REPO")"; then - if pr_budget_exhausted "$events_json"; then - budget_exhausted=1 - fi - else - skip_closed "automation-budget events unavailable" - fi - - decision="$(qa_lead_gate_decision \ - "$changed_paths" "$labels_json" "$existing_advisory" "$budget_exhausted" || true)" + # The signal gathering + fire/skip decision lives in ONE shared script + # (#1905 AC #2/#4): the router-served pull_request surface calls the + # SAME qa_lead_pr_gather_and_decide via scripts/persona-pr-pregate.sh, + # so the suppressors are identical, not a copy. It fails closed — any + # unreadable signal yields a skip with a ::error naming it — so this + # workflow keeps advising only when EVERY signal is available. + decision="$(qa_lead_pr_gather_and_decide "$REPO" "$PR" || true)" echo "gate decision for ${REPO}#${PR}: ${decision}" # Normalize to run|skip for the downstream `if`; keep the reason in logs. if [ "$decision" = "run" ]; then @@ -168,6 +100,8 @@ jobs: if: needs.gate.outputs.decision == 'run' permissions: contents: read + pull-requests: read # the reusable re-runs the shared pre-gate below with + issues: read # surface=pull_request, so grant it the same reads. uses: ./.github/workflows/persona-runner-reusable.yml with: persona: qa-lead @@ -176,4 +110,13 @@ jobs: comment_url: '' requested_by: '' mode: advisory + # Drive the reusable's pull_request pre-gate (#1905) so BOTH the local and + # the router-served qa-lead advisory run the SAME already-advised marker + # check AND share the reusable's per-(persona,repo,PR) concurrency group + # (comment_url is '' on both paths). That serialization is what closes the + # duplicate-advisory race: the local advise formerly left surface unset + # (-> 'mention'), so its reusable run skipped the marker recheck and could + # post a second advisory after the router already posted one. + surface: pull_request + event_action: ${{ github.event.action }} secrets: inherit diff --git a/personas/qa-lead/README.md b/personas/qa-lead/README.md index 4e72a7d49..84934c1be 100644 --- a/personas/qa-lead/README.md +++ b/personas/qa-lead/README.md @@ -123,9 +123,13 @@ set lives in [`persona.yml`](./persona.yml). This README does not restate either The whole framework is safe because **the agent cannot post**: -- The **agent** runs with `github.token`. That token reads the (public) source - repo but is scoped to *this* repo, so it **cannot comment on the target**. The - agent only *prints* its advisory between sentinels. +- The **agent** runs with `github.token`. On the router-served path that token + reads **public** source repos and this repo only (it is scoped to *this* repo). + For other private source repos it has no read access, so the pull_request + pre-gate fails closed (`skip:signal-unavailable`, which the runner reports as a + retryable workflow failure). **Private source repositories other than + `.github-private`** are out of scope for the router-served pull_request surface + today. The agent only *prints* its advisory between sentinels; it never posts. - The **workflow** is the only writer. It extracts the advisory, mechanically guarantees the `` recursion marker, and posts with the persona's PAT (declared as `runtime.identity` in the manifest). diff --git a/personas/qa-lead/interaction.yml b/personas/qa-lead/interaction.yml index 38df7ea18..e2f567fb9 100644 --- a/personas/qa-lead/interaction.yml +++ b/personas/qa-lead/interaction.yml @@ -17,6 +17,10 @@ # router so it reaches beyond this repo (#1869). The local workflow is # retired only after that router change lands on persona-mention/v1-next and # the #1745 stop markers are verified on a pull_request-sourced dispatch. +# NOTE: Private source repositories other than .github-private are out of +# scope for the router-served pull_request path: its pre-gate runs with +# github.token scoped to .github-private and fails closed when it cannot +# read them (the runner exits non-zero, a retryable workflow failure). # Both paths post a single advisory comment as the persona's runtime.identity and # write nothing else. The workflow's read/write split mechanically stamps the # recursion marker; the prompt is not trusted to. diff --git a/personas/qa-lead/persona.yml b/personas/qa-lead/persona.yml index 7039806dd..9ca02de94 100644 --- a/personas/qa-lead/persona.yml +++ b/personas/qa-lead/persona.yml @@ -112,7 +112,7 @@ triggers: events: [opened, ready_for_review] enabled: true mode: advisory - notes: "Risk assessment + test-coverage review comments on PRs with test surface. Served LOCALLY in .github-private today by qa-lead-pr-advisory.yml (#1646) on [opened, ready_for_review]; this surface is NOT fleet-deployable via a caller stub because that workflow is self-contained, not a thin stub. ADR-0009 routes it through the shared persona-mention router so it reaches beyond .github-private (#1869); the local workflow is retired only after that router change lands on persona-mention/v1-next and the #1745 stop markers are verified on a pull_request-sourced dispatch. `synchronize` is deliberately EXCLUDED (one advisory as a PR enters review, not on every push) — so the declared events match what is deployed." + notes: "Risk assessment + test-coverage review comments on PRs with test surface. Served LOCALLY in .github-private today by qa-lead-pr-advisory.yml (#1646) on [opened, ready_for_review]; this surface is NOT fleet-deployable via a caller stub because that workflow is self-contained, not a thin stub. ADR-0009 routes it through the shared persona-mention router so it reaches beyond .github-private (#1869); the local workflow is retired only after that router change lands on persona-mention/v1-next and the #1745 stop markers are verified on a pull_request-sourced dispatch. `synchronize` is deliberately EXCLUDED (one advisory as a PR enters review, not on every push) — so the declared events match what is deployed. NOTE: The router-served pull_request pre-gate runs with github.token scoped to .github-private; it cannot read private source repositories other than .github-private, so those dispatches fail with a retryable workflow error before advisory generation — such private source repositories are out of scope." - surface: pull_request_review events: [submitted, created] diff --git a/scripts/lib/pr-automation-budget.sh b/scripts/lib/pr-automation-budget.sh index f5c42c224..e7b658e8a 100644 --- a/scripts/lib/pr-automation-budget.sh +++ b/scripts/lib/pr-automation-budget.sh @@ -95,9 +95,12 @@ compute_pr_automation_cycles() { # Malformed/empty/missing input degrades to "not escalated" (exit 1). pr_has_escalation_label() { local labels_json="${1:-[]}" - jq -e --arg l "${NEEDS_HUMAN_REVIEW_LABEL}" \ - 'if type == "array" then any(.[]; . == $l) else false end' \ - <<<"$labels_json" >/dev/null 2>&1 + if jq -e --arg l "${NEEDS_HUMAN_REVIEW_LABEL}" \ + 'if type == "array" then any(.[]; . == $l) else false end' \ + <<<"$labels_json" >/dev/null 2>&1; then + return 0 + fi + return 1 } # The fleet's human-hold label set (needs-human-review, dev-lead:needs-human, @@ -168,9 +171,12 @@ pr_hold_kind() { _pr_items_have_marker() { local items_json="${1:-[]}" needle="${2:-}" [ -n "$needle" ] || return 1 - jq -e --arg needle "$needle" ' + if jq -e --arg needle "$needle" ' if type == "array" then any(.[]; (.body? // "" | tostring) | contains($needle)) else false end - ' <<<"$items_json" >/dev/null 2>&1 + ' <<<"$items_json" >/dev/null 2>&1; then + return 0 + fi + return 1 } # pr_resume_suppressed [labels_json] [events_json] @@ -239,7 +245,10 @@ pr_budget_exhausted() { local events_json="${1:-[]}" local n n=$(compute_pr_automation_cycles "$events_json") - [ "${n:-0}" -ge "${MAX_PR_AUTOMATION_CYCLES:-10}" ] + if [ "${n:-0}" -ge "${MAX_PR_AUTOMATION_CYCLES:-10}" ]; then + return 0 + fi + return 1 } # gather_pr_automation_events @@ -282,9 +291,12 @@ gather_pr_automation_events() { # Exit 0 if the exhaustion escalation comment is already present (dedupe). pr_automation_already_escalated() { local pr="$1" repo="$2" - gh api --paginate "repos/${repo}/issues/${pr}/comments?per_page=100" 2>/dev/null \ - | jq -r '.[].body // ""' 2>/dev/null \ - | grep -qF "$PR_AUTOMATION_EXHAUSTION_MARKER" + if gh api --paginate "repos/${repo}/issues/${pr}/comments?per_page=100" 2>/dev/null \ + | jq -r '.[].body // ""' 2>/dev/null \ + | grep -qF "$PR_AUTOMATION_EXHAUSTION_MARKER"; then + return 0 + fi + return 1 } # pr_automation_escalate diff --git a/scripts/lib/qa-lead-test-surface.sh b/scripts/lib/qa-lead-test-surface.sh index 04181fa8e..5c8b6ef86 100755 --- a/scripts/lib/qa-lead-test-surface.sh +++ b/scripts/lib/qa-lead-test-surface.sh @@ -100,8 +100,11 @@ qa_lead_paths_have_control_char() { local json="${1:-[]}" # explode -> codepoints; any < 32 is a C0 control character. (jq's regex # engine does not honour \uXXXX escapes, so match on codepoints, not a class.) - jq -e 'any(.[]?; explode | any(.[]; . < 32))' \ - <<< "$json" >/dev/null 2>&1 + if jq -e 'any(.[]?; explode | any(.[]; . < 32))' \ + <<< "$json" >/dev/null 2>&1; then + return 0 + fi + return 1 } # qa_lead_file_list_complete @@ -113,5 +116,8 @@ qa_lead_paths_have_control_char() { # partial data (#1697 review). Boundary: 3000/3000 is complete, 3000/3001 is not. qa_lead_file_list_complete() { local received="${1:-0}" declared="${2:-0}" - [ "$received" -ge "$declared" ] + if [ "$received" -ge "$declared" ]; then + return 0 + fi + return 1 } diff --git a/scripts/persona-pr-pregate.sh b/scripts/persona-pr-pregate.sh new file mode 100644 index 000000000..ad9ce8a46 --- /dev/null +++ b/scripts/persona-pr-pregate.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +set -euo pipefail +# persona-pr-pregate.sh — the persona event pre-gate the shared runtime consults +# before running the engine (issue #1905). +# +# The mention router serves qa-lead's pull_request advisory by dispatching +# repository_dispatch:persona-mention with client_payload.surface=pull_request +# into .github/workflows/persona-runner-reusable.yml. The router binds the +# GENERIC brakes (stop markers, opt-out label, write-gate, trust floor, recursion +# axes, declared events); it does NOT carry the PERSONA-SPECIFIC suppressors the +# local qa-lead-pr-advisory.yml gate enforces (no-test-surface, budget-exhausted, +# already-advised). Without this pre-gate the runner posts on EVERY trusted PR. +# +# persona_event_pregate is what the runner calls after resolving the persona and +# BEFORE the engine: +# * surface == mention (or absent -> mention): "run", no gate. Existing mention +# dispatches are unchanged (AC #1). +# * an unrecognized surface: fail closed (skip) — malformed dispatch data must +# not be treated as a mention and bypass the pull_request suppressors. +# * surface == pull_request + a persona with a registered pre-gate (qa-lead): +# run that gate — the ONE shared gather+decide (AC #2). +# * surface == pull_request + a persona with NO registered pre-gate: skip +# (skip:no-registered-pregate) with a ::notice. Such personas (dev-lead, +# pr-review, ...) are served by their own dedicated workflows; running their +# engine here would post a duplicate advisory (AC #3). +# * any unreadable signal: fail closed (skip) with a ::error naming it (AC #3). +# +# Prints exactly one decision line on stdout: "run" or "skip:". Returns 0 +# for run, 1 for skip. Diagnostics (::error/::notice) go to stderr. + +_PERSONA_PREGATE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=scripts/qa-lead-pr-gate.sh +source "${_PERSONA_PREGATE_DIR}/qa-lead-pr-gate.sh" +# shellcheck source=scripts/lib/persona-runner.sh +source "${_PERSONA_PREGATE_DIR}/lib/persona-runner.sh" + +# persona_event_pregate [event_action] +# The single entry point the runner calls. See the header for the dispatch. +persona_event_pregate() { + local persona="$1" surface="${2:-mention}" repo="$3" item="$4" + # event_action ($5) is forwarded for parity with the router payload and future + # per-persona gates; the qa-lead decision does not consider it (the surface's + # declared events are bound by the router, per ADR-0009). + + # Recognized surfaces ONLY. A mention (or absent -> mention, via the default + # above) is unchanged: the pre-gate is a pull_request-only concern (AC #1). A + # pull_request surface falls through to the persona gate below. Any OTHER value + # is malformed dispatch data — fail CLOSED (skip) rather than treat an + # unrecognized surface as a mention, which would bypass every pull_request + # suppressor. + case "$surface" in + mention) + printf 'run\n' + return 0 + ;; + pull_request) ;; + *) + echo "::error::persona event pre-gate: unrecognized surface '${surface}' for ${repo}#${item} — failing closed (skip)" >&2 + printf 'skip:unknown-surface\n' + return 1 + ;; + esac + + case "$persona" in + qa-lead) + qa_lead_pr_gather_and_decide "$repo" "$item" + ;; + *) + echo "::notice::no persona-specific pull_request pre-gate registered for '${persona}' — skipping (served by its own workflow)" >&2 + printf 'skip:no-registered-pregate\n' + return 1 + ;; + esac +} + +# Allow direct CLI use for debugging: persona-pr-pregate.sh … +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + persona_event_pregate "$@" +fi diff --git a/scripts/qa-lead-advisory-gate.sh b/scripts/qa-lead-advisory-gate.sh index 28907ead4..9aad84df0 100755 --- a/scripts/qa-lead-advisory-gate.sh +++ b/scripts/qa-lead-advisory-gate.sh @@ -38,9 +38,12 @@ source "${_QA_LEAD_GATE_DIR}/lib/pr-automation-budget.sh" # dev-lead:needs-human). Malformed/empty input degrades to "not present". qa_lead_labels_contain() { local labels_json="${1:-[]}" needle="$2" - jq -e --arg l "$needle" \ - 'if type == "array" then any(.[]; . == $l) else false end' \ - <<< "$labels_json" >/dev/null 2>&1 + if jq -e --arg l "$needle" \ + 'if type == "array" then any(.[]; . == $l) else false end' \ + <<< "$labels_json" >/dev/null 2>&1; then + return 0 + fi + return 1 } # qa_lead_gate_decision diff --git a/scripts/qa-lead-pr-gate.sh b/scripts/qa-lead-pr-gate.sh new file mode 100644 index 000000000..3792ee3fe --- /dev/null +++ b/scripts/qa-lead-pr-gate.sh @@ -0,0 +1,143 @@ +#!/usr/bin/env bash +set -euo pipefail +# qa-lead-pr-gate.sh — signal gathering + fire/skip decision for the qa-lead +# pull_request advisory (issue #1905). +# +# This is the ONE implementation of the gathering the qa-lead PR advisory needs, +# shared by BOTH callers so the suppressors are identical, not copied: +# * .github/workflows/qa-lead-pr-advisory.yml — the local, event-driven surface +# (its gate job sources this and calls qa_lead_pr_gather_and_decide), and +# * .github/workflows/persona-runner-reusable.yml — the router-served surface +# (its event pre-gate calls it via scripts/persona-pr-pregate.sh). +# Before #1905 the gathering lived inline in the local workflow's YAML, so the +# router-served pull_request path had NONE of it and posted on every trusted PR. +# +# It gathers the PR's signals via `gh` and hands them to the pure, unit-tested +# decision qa_lead_gate_decision (scripts/qa-lead-advisory-gate.sh). Sourcing this +# file pulls in that decision + the heuristic/budget libs WITHOUT running main +# (guarded by BASH_SOURCE == $0 there). + +_QA_LEAD_PR_GATE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=scripts/qa-lead-advisory-gate.sh +source "${_QA_LEAD_PR_GATE_DIR}/qa-lead-advisory-gate.sh" + +# The recursion marker every qa-lead advisory begins with — the idempotency key +# for the already-advised suppressor (kept identical to the local workflow). +: "${QA_LEAD_ADVISORY_MARKER:=}" + +# _qa_lead_pr_gate_fail_closed +# Fail CLOSED: name the derivation that could not be resolved on a ::error +# (AGENTS.md: "when an input can't be resolved, fail loudly and name the +# derivation that failed") and print the skip decision. Silence is the safe +# default — a signal we cannot read must never degrade to an empty value that +# would bypass a suppressor or duplicate an advisory. +_qa_lead_pr_gate_fail_closed() { + local repo="$1" pr="$2" what="$3" + echo "::error::qa-lead pull_request pre-gate: ${what} for ${repo}#${pr} — failing closed (skip)" >&2 + printf 'skip:signal-unavailable\n' +} + +# qa_lead_pr_gather_and_decide +# Gather the PR's signals and return the fire/skip decision. Prints exactly one +# decision line on stdout: "run" or "skip:". Returns 0 for run, 1 for +# skip. Any unreadable signal fails closed (skip) via the helper above. +# +# Signals gathered (identical to the local workflow's former gate job): +# * changed files — with the completeness (3000-file cap) and control-character +# guards, so a truncated or LF-forged list never mislabels the PR; +# * label names — never degraded to [] on an API error (that would bypass the +# opt-out / human gates); +# * existing qa-lead advisory marker (idempotency); +# * per-PR automation budget events (#860/#926 breaker). +qa_lead_pr_gather_and_decide() { + local repo="$1" pr="$2" + local files_json declared_files received_files changed_paths labels_json + local comment_bodies existing_advisory budget_exhausted events_json decision + + # Changed files as a JSON array of filenames — validated BEFORE it is flattened + # to the newline text qa_lead_test_surface consumes. gh rejects --slurp together + # with --jq, so the filter runs in external jq; `pipefail` (set above) makes a + # failure of EITHER side of the pipe reach the fail-closed branch. + if ! files_json="$(gh api --paginate --slurp \ + "repos/${repo}/pulls/${pr}/files" | jq -c '[.[][].filename]')"; then + _qa_lead_pr_gate_fail_closed "$repo" "$pr" "changed-file list unavailable" + return 1 + fi + + # Completeness: the /pulls/{n}/files endpoint caps at 3000 files, so a larger + # PR yields a truncated list that could omit a test/source path. Compare the + # received count against the declared changed_files and skip on an incomplete + # list rather than advise off partial data. + if ! declared_files="$(gh api "repos/${repo}/pulls/${pr}" \ + --jq '.changed_files')"; then + _qa_lead_pr_gate_fail_closed "$repo" "$pr" "changed-file count unavailable" + return 1 + fi + received_files="$(jq 'length' <<< "$files_json")" + if ! qa_lead_file_list_complete "$received_files" "$declared_files"; then + _qa_lead_pr_gate_fail_closed "$repo" "$pr" \ + "changed-file list incomplete (${received_files}/${declared_files})" + return 1 + fi + + # Control characters: a filename containing an LF would split into phantom + # records once flattened (a docs-only PR could forge a SOURCE path). Reject + # before flattening. + if qa_lead_paths_have_control_char "$files_json"; then + _qa_lead_pr_gate_fail_closed "$repo" "$pr" \ + "changed-file list contains control characters" + return 1 + fi + + changed_paths="$(jq -r '.[]' <<< "$files_json")" + + # Label names — an API failure must NOT degrade to [] (that would bypass the + # opt-out and human-gating suppressors). + if ! labels_json="$(gh api "repos/${repo}/pulls/${pr}" \ + --jq '[.labels[]?.name]')"; then + _qa_lead_pr_gate_fail_closed "$repo" "$pr" "labels unavailable" + return 1 + fi + + # Existing qa-lead advisory? Scan the (shared) issue comment stream for the + # recursion marker. A comment API failure is indistinguishable from "no marker", + # so failing open here would risk a duplicate advisory — fail closed instead. + if ! comment_bodies="$(gh api --paginate \ + "repos/${repo}/issues/${pr}/comments" --jq 'if type == "array" then .[].body else error("expected comments array") end')"; then + _qa_lead_pr_gate_fail_closed "$repo" "$pr" "existing-advisory scan unavailable" + return 1 + fi + if grep -qF "$QA_LEAD_ADVISORY_MARKER" <<< "$comment_bodies"; then + existing_advisory=1 + else + existing_advisory=0 + fi + + # Per-PR automation budget. gather_pr_automation_events returns non-zero when + # any API call fails; a failure must NOT degrade to an empty event list (an + # exhausted PR would then pass the budget gate during a transient outage). + budget_exhausted=0 + if events_json="$(gather_pr_automation_events "$pr" "$repo")"; then + if pr_budget_exhausted "$events_json"; then + budget_exhausted=1 + fi + else + _qa_lead_pr_gate_fail_closed "$repo" "$pr" "automation-budget events unavailable" + return 1 + fi + + decision="$(qa_lead_gate_decision \ + "$changed_paths" "$labels_json" "$existing_advisory" "$budget_exhausted" || true)" + if [ -z "$decision" ]; then + _qa_lead_pr_gate_fail_closed "$repo" "$pr" "gate decision empty" + return 1 + fi + printf '%s\n' "$decision" + # Return the status explicitly rather than leaving a bare `[ ... ]` as the + # function's last line: under `set -e`, a standalone conditional that evaluates + # false propagates as a failure that can terminate a sourcing caller. + if [ "$decision" = "run" ]; then + return 0 + fi + return 1 +} diff --git a/tests/test_persona_pr_pregate.bats b/tests/test_persona_pr_pregate.bats new file mode 100644 index 000000000..9d4c27720 --- /dev/null +++ b/tests/test_persona_pr_pregate.bats @@ -0,0 +1,236 @@ +#!/usr/bin/env bats +# Unit tests for the persona pull_request event pre-gate +# (scripts/persona-pr-pregate.sh + scripts/qa-lead-pr-gate.sh, issue #1905). +# +# The router-served pull_request surface reaches persona-runner-reusable.yml with +# client_payload.surface=pull_request. Before #1905 the runner ignored `surface` +# and ran the engine + posted on EVERY trusted PR — losing the persona-specific +# suppressors the local qa-lead-pr-advisory.yml gate enforces (no-test-surface, +# budget-exhausted, already-advised) on top of the router's generic brakes. +# +# `persona_event_pregate` is what the runner now calls before the engine: +# * surface != pull_request (mention/absent) -> "run", no gate (mention path +# unchanged, AC #1). +# * surface == pull_request, persona qa-lead -> qa_lead_pr_gather_and_decide, +# the ONE gathering+decision shared with the local workflow (AC #2). +# * surface == pull_request, no registered gate -> skip + a logged notice (AC #3). +# * any unreadable signal -> fail closed (skip) with a ::error naming it (AC #3). +# +# gh is stubbed on PATH, keyed on the `--jq` filter each call uses, so the +# gathering can be exercised offline. jq is real. +# +# Run with: bats tests/test_persona_pr_pregate.bats + +setup() { + SCRIPT_DIR="$(cd "$(dirname "$BATS_TEST_FILENAME")/.." && pwd)" + # shellcheck source=/dev/null + source "$SCRIPT_DIR/scripts/persona-pr-pregate.sh" + + # A gh stub that returns canned output keyed on the --jq filter (each distinct + # call the gathering makes uses a distinct filter, or none for the budget + # gather). Per-test behaviour is driven by STUB_* environment variables. + STUB_BIN="$(mktemp -d "$BATS_TEST_TMPDIR/bin.XXXXXX")" + cat > "$STUB_BIN/gh" <<'STUB' +#!/usr/bin/env bash +# Find the --jq filter (if any) and the repos/... url. +jqf=""; url=""; prev=""; slurp=0; tmpl=0 +for a in "$@"; do + [ "$prev" = "--jq" ] && jqf="$a" + [ "$a" = "--slurp" ] && slurp=1 + case "$a" in --jq|--template) tmpl=1 ;; esac + case "$a" in repos/*) [ -z "$url" ] && url="$a" ;; esac + prev="$a" +done +# Behave like real gh: --slurp is rejected together with --jq/--template. +if [ "$slurp" = 1 ] && [ "$tmpl" = 1 ]; then + echo "the \`--slurp\` option is not supported with \`--jq\` or \`--template\`" >&2 + exit 1 +fi +case "$jqf" in + '.changed_files') printf '%s\n' "${STUB_CHANGED:-0}"; exit "${STUB_CHANGED_RC:-0}" ;; + '[.labels[]?.name]') printf '%s\n' "${STUB_LABELS:-[]}"; exit "${STUB_LABELS_RC:-0}" ;; + 'if type == "array" then .[].body else error("expected comments array") end') printf '%s' "${STUB_COMMENT_BODIES:-}"; exit "${STUB_COMMENTS_RC:-0}" ;; + '') + # gather_pr_automation_events raw calls (no --jq, piped to jq -s downstream) + case "$url" in + *"/pulls/"*"/files") # slurped: one array per page, wrapped in an outer array + printf '%s\n' "${STUB_FILES:-[]}" | jq -c '[map({filename: .})]' + exit "${STUB_FILES_RC:-0}" ;; + *"/issues/"*"/comments"*) printf '%s\n' "${STUB_RAW_COMMENTS:-[]}" ;; + *"/commits"*) printf '%s\n' "${STUB_RAW_COMMITS:-[]}" ;; + *"/reviews"*) printf '%s\n' "${STUB_RAW_REVIEWS:-[]}" ;; + *) echo "::error::gh stub: unhandled url '$url'" >&2; exit 42 ;; + esac + exit "${STUB_GATHER_RC:-0}" + ;; +esac +echo "::error::gh stub: unhandled --jq filter '$jqf'" >&2; exit 42 +STUB + chmod +x "$STUB_BIN/gh" + PATH="$STUB_BIN:$PATH" + + # Defaults that pass every gate — individual tests override one signal. + export STUB_FILES='["scripts/foo.sh"]' + export STUB_CHANGED='1' + export STUB_LABELS='[]' + export STUB_COMMENT_BODIES='' + export STUB_RAW_COMMENTS='[]' + export STUB_RAW_COMMITS='[]' + export STUB_RAW_REVIEWS='[]' +} + +# --------------------------------------------------------------------------- +# surface absent / mention -> the mention path is unchanged (no pre-gate) +# --------------------------------------------------------------------------- + +@test "AC#1: surface=mention -> run without gating (mention path unchanged)" { + run persona_event_pregate qa-lead mention petry-projects/.github-private 5 "" + [ "$status" -eq 0 ] + [ "$output" = "run" ] +} + +@test "AC#1: surface absent defaults to mention -> run without gating" { + run persona_event_pregate qa-lead "" petry-projects/.github-private 5 "" + [ "$status" -eq 0 ] + [ "$output" = "run" ] +} + +@test "AC#1: a mention dispatch never reads PR signals (no test surface needed)" { + # Even with a docs-only PR and an unreadable file list, the mention path runs: + # the pre-gate is a pull_request-only concern. + STUB_FILES='["README.md"]' STUB_FILES_RC=1 \ + run persona_event_pregate qa-lead mention petry-projects/.github-private 5 "" + [ "$status" -eq 0 ] + [ "$output" = "run" ] +} + +# --------------------------------------------------------------------------- +# An unrecognized surface is malformed dispatch data -> fail closed (skip), +# NOT treated as a mention (which would bypass every pull_request suppressor). +# --------------------------------------------------------------------------- + +@test "fail-closed: an unrecognized surface skips with a ::error (not run as mention)" { + run persona_event_pregate qa-lead bogus-surface petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == *"::error::"* ]] + [[ "$output" == *skip:unknown-surface* ]] +} + +# --------------------------------------------------------------------------- +# surface=pull_request, qa-lead -> the shared gather+decide +# --------------------------------------------------------------------------- + +@test "run: real test surface and no suppressor" { + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 0 ] + [ "$output" = "run" ] +} + +@test "skip: no-test-surface (docs-only PR)" { + STUB_FILES='["README.md"]' \ + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == skip:no-test-surface* ]] +} + +@test "skip: opt-out (qa-lead:hands-off label)" { + STUB_LABELS='["qa-lead:hands-off"]' \ + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == skip:opt-out* ]] +} + +@test "skip: human-gated (needs-human-review label)" { + STUB_LABELS='["needs-human-review"]' \ + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == skip:human-gated* ]] +} + +@test "skip: budget-exhausted (>= MAX automated actions since last human)" { + # 10 bot comments, no human -> compute_pr_automation_cycles >= MAX (10). + bots="$(jq -n '[range(10) | {created_at: "2026-09-20T00:00:0\(.)Z", user: {login: "github-actions[bot]"}}]')" + STUB_RAW_COMMENTS="$bots" \ + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == skip:budget-exhausted* ]] +} + +@test "skip: already-advised (a qa-lead marker is already present)" { + STUB_COMMENT_BODIES=' +prior advisory' \ + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == skip:already-advised* ]] +} + +# --------------------------------------------------------------------------- +# Fail closed on an unreadable signal, naming which derivation failed (AC #3) +# --------------------------------------------------------------------------- + +@test "fail-closed: unreadable changed-file list skips with a ::error" { + STUB_FILES_RC=1 \ + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == *skip:* ]] + [[ "$output" == *"::error::"* ]] + [[ "$output" == *"changed-file list unavailable"* ]] +} + +@test "fail-closed: unreadable labels skips with a ::error" { + STUB_LABELS_RC=1 \ + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == *"::error::"* ]] + [[ "$output" == *"labels unavailable"* ]] +} + +@test "fail-closed: unreadable budget events skips with a ::error" { + STUB_GATHER_RC=1 \ + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == *"::error::"* ]] + [[ "$output" == *"automation-budget events unavailable"* ]] +} + +@test "fail-closed: an incomplete (truncated) changed-file list skips" { + # Received 1 file, PR declares 5000 -> truncated -> never advise off partial data. + STUB_CHANGED='5000' \ + run persona_event_pregate qa-lead pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == *"::error::"* ]] + [[ "$output" == *"incomplete"* ]] +} + +# --------------------------------------------------------------------------- +# A persona with NO registered pre-gate skips on the pull_request surface (its +# own dedicated workflow serves it), makes no gh call, and logs a notice (AC #3). +# --------------------------------------------------------------------------- + +@test "no-registered-gate: skips, makes no gh call, logs a notice" { + printf '#!/usr/bin/env bash\necho called >> "%s/gh-called"\nexit 42\n' "$BATS_TEST_TMPDIR" > "$STUB_BIN/gh" + run persona_event_pregate scrum-master pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == *"skip:no-registered-pregate"* ]] + [[ "$output" == *"::notice::"* ]] + [[ "$output" == *"scrum-master"* ]] + [ ! -e "$BATS_TEST_TMPDIR/gh-called" ] +} + +@test "no-registered-gate: dev-lead and pr-review also skip" { + for p in dev-lead pr-review; do + run persona_event_pregate "$p" pull_request petry-projects/.github-private 5 opened + [ "$status" -eq 1 ] + [[ "$output" == *"skip:no-registered-pregate"* ]] + done +} + +# --------------------------------------------------------------------------- +# qa_lead_pr_gather_and_decide is the ONE implementation both callers use. +# --------------------------------------------------------------------------- + +@test "qa_lead_pr_gather_and_decide is defined and usable directly" { + run qa_lead_pr_gather_and_decide petry-projects/.github-private 5 + [ "$status" -eq 0 ] + [ "$output" = "run" ] +}