diff --git a/Makefile b/Makefile index 076fcacb..494f3c1b 100644 --- a/Makefile +++ b/Makefile @@ -50,6 +50,7 @@ script-test: $(call run-timed,bash scripts/post-code-test.sh) $(call run-timed,bash scripts/post-review-test.sh) $(call run-timed,bash scripts/post-fix-test.sh) + $(call run-timed,bash skills/merge-queue/scripts/write-approval-check-test.sh) $(call run-timed,bash scripts/post-retro-test.sh) $(call run-timed,bash scripts/post-scribe-test.sh) $(call run-timed,bash scripts/validate-output-schema-test.sh) diff --git a/docs/code.md b/docs/code.md index 25b88c73..674811cb 100644 --- a/docs/code.md +++ b/docs/code.md @@ -31,6 +31,7 @@ on issues (not PRs). |-------|---------| | `ready-to-code` | Triggers the code agent. Applied by the [triage](triage.md) agent for low-risk categories (bug, documentation, performance), or manually by a human for feature work after prioritization. Not applied when the triage result sets `requires_workflow_changes`, since the code agent cannot modify workflow files. | | `ready-for-review` | Applied by the code agent after pushing a PR. In per-repo installs, triggers the [review agent](review.md) when applied to a PR. Also marks workflow state for humans and the [retro agent](retro.md). | +| `needs-write-approval` | Applied when `TRIGGER_ROLE` is `triage` — the dispatching user held only the GitHub `triage` role, not write+. `skills/merge-queue/scripts/enqueue-pr.sh` and `await-and-enqueue.sh` refuse to enqueue such a PR without an APPROVE review, on its current head commit, from a currently admin/maintain/write human collaborator (checked live — this label having ever been applied is derived from the immutable issue-events timeline, not the label's current presence, since GitHub's `triage` role can remove it). This does not prevent a write+ collaborator from merging directly via GitHub's native UI or `gh pr merge`, which is unaware of this label. | ## Configuration @@ -42,6 +43,7 @@ See [Customizing with AGENTS.md](https://fullsend.sh/docs/guides/user/customizin | Variable | Description | Default | Valid values | |----------|-------------|---------|--------------| | `CODE_ALLOWED_TARGET_BRANCHES` | Restricts which branches the code agent can target when pushing. The post-code script validates the agent's chosen target branch against this variable before pushing. Set via `runner_env` in `harness/code.yaml` (never injected into the sandbox). | Repo default branch (auto-detected via GitHub API; falls back to `main`) | Comma-separated branch names (e.g. `main,develop`) or `*` for any branch | +| `TRIGGER_ROLE` | Permission tier that authorized this dispatch, set by dispatch routing. When `triage`, the post-code script applies the `needs-write-approval` label. | Unset (treated as `write` — no gate) | `triage`, `write` | ## How the agent works diff --git a/docs/fix.md b/docs/fix.md index ca3bca4d..0f2ea605 100644 --- a/docs/fix.md +++ b/docs/fix.md @@ -105,6 +105,7 @@ The fix agent enforces iteration caps to prevent infinite review-fix loops: |-------|---------| | `fullsend-no-fix` | Prevents automatic fix runs on this PR. Applied by `/fs-fix-stop`. Manual `/fs-fix` commands are unaffected. | | `needs-human` | The fix agent is approaching its iteration cap and needs human direction. Applied automatically when an automatic fix iteration reaches the warning threshold. | +| `needs-write-approval` | Applied when `TRIGGER_ROLE` is `triage` — the dispatching user held only the GitHub `triage` role, not write+. `skills/merge-queue/scripts/enqueue-pr.sh` and `await-and-enqueue.sh` refuse to enqueue such a PR without an APPROVE review, on its current head commit, from a currently admin/maintain/write human collaborator (checked live — this label having ever been applied is derived from the immutable issue-events timeline, not the label's current presence, since GitHub's `triage` role can remove it). This does not prevent a write+ collaborator from merging directly via GitHub's native UI or `gh pr merge`, which is unaware of this label. | ## Configuration @@ -113,7 +114,9 @@ See [Customizing with AGENTS.md](https://fullsend.sh/docs/guides/user/customizin ### Variables -None. +| Variable | Description | Default | Valid values | +|----------|-------------|---------|--------------| +| `TRIGGER_ROLE` | Permission tier that authorized this dispatch, set by dispatch routing. When `triage`, the post-fix script applies the `needs-write-approval` label. | Unset (treated as `write` — no gate) | `triage`, `write` | ## Custom sandbox image diff --git a/scripts/lib/write-approval-gate.lib.sh b/scripts/lib/write-approval-gate.lib.sh new file mode 100644 index 00000000..2e507e2e --- /dev/null +++ b/scripts/lib/write-approval-gate.lib.sh @@ -0,0 +1,68 @@ +#!/usr/bin/env bash +# write-approval-gate.lib.sh — Merge gate for triage-role-triggered code/fix runs. +# +# Source from post-code.src.sh / post-fix.src.sh (after post-failure-report.lib.sh +# for gha_echo): +# source "${SCRIPT_DIR}/lib/write-approval-gate.lib.sh" +# +# fullsend-ai/fullsend#5687: dispatch now accepts the GitHub `triage` role for +# /fs-code and /fs-fix. Triage-role users still get a bot-authored PR (the +# agent always held write-level credentials), but that PR must carry an +# explicit visible marker so reviewers and merge tooling know it needs a +# write+ collaborator's approval — the actual enforcement (requiring an +# approval from a currently write+ user, not just any reviewDecision) lives +# in skills/merge-queue/scripts/await-and-enqueue.sh, which checks this label. + +# shellcheck shell=bash + +[[ -n "${WRITE_APPROVAL_GATE_SH_LOADED:-}" ]] && return 0 +WRITE_APPROVAL_GATE_SH_LOADED=1 + +# Emit a runner warning through gha_echo when available. +_write_approval_gate_warn() { + if declare -F gha_echo >/dev/null 2>&1; then + gha_echo warning "$*" + else + echo "warning: $*" >&2 + fi +} + +# Normalize a TRIGGER_ROLE value: lowercase and trim surrounding whitespace. +_normalize_trigger_role() { + printf '%s' "${1:-}" | tr '[:upper:]' '[:lower:]' | xargs +} + +# Apply the needs-write-approval label to a PR when TRIGGER_ROLE is "triage" +# (case-insensitive, surrounding whitespace ignored). No-op (and no gh calls) +# when TRIGGER_ROLE is unset or "write" — unset means the trigger was resolved +# at write+ (or a path that predates TRIGGER_ROLE, e.g. bot-triggered +# review->fix), so no gate is needed. Any other non-empty value is treated +# the same as "write" (no gate) but logged, since it likely indicates a bug +# in the caller rather than a legitimate write+ trigger. +# Requires REPO_FULL_NAME. Best-effort: never fails the calling script. +# Note: parameter is target_pr (not pr_number) to avoid SC2153 against +# PR_NUMBER from post-failure-report.lib.sh once both libs are bundled into +# post-code.sh / post-fix.sh — same fix as maybe_assign_pr in pr-assignee.lib.sh. +apply_write_approval_gate_if_needed() { + local target_pr="$1" + local role + role="$(_normalize_trigger_role "${TRIGGER_ROLE:-}")" + + if [[ -z "${role}" ]]; then + return 0 + fi + if [[ "${role}" != "triage" ]]; then + if [[ "${role}" != "write" ]]; then + _write_approval_gate_warn "Unrecognized TRIGGER_ROLE '${TRIGGER_ROLE}' — treating as write (no gate applied). Expected 'triage' or 'write'." + fi + return 0 + fi + + echo "Trigger role is 'triage' — applying needs-write-approval gate to PR #${target_pr}" + gh label create "needs-write-approval" --repo "${REPO_FULL_NAME}" \ + --description "Triggered by a triage-role user; needs write+ approval before merge" \ + --color "B60205" 2>/dev/null || true + gh pr edit "${target_pr}" --repo "${REPO_FULL_NAME}" \ + --add-label "needs-write-approval" 2>/dev/null || \ + _write_approval_gate_warn "Failed to apply needs-write-approval label to PR #${target_pr}" +} diff --git a/scripts/post-code-test.sh b/scripts/post-code-test.sh index e7cf7e4d..fb99f79d 100755 --- a/scripts/post-code-test.sh +++ b/scripts/post-code-test.sh @@ -1170,6 +1170,53 @@ else echo "PASS: script-has-noop-comment" fi +# Verify apply_write_approval_gate_if_needed is present in the post-code script +if ! grep -q 'apply_write_approval_gate_if_needed' "${POST_SCRIPT}"; then + echo "FAIL: bundled-script-has-write-approval-gate" + echo " ${POST_SCRIPT} missing apply_write_approval_gate_if_needed" + FAILURES=$((FAILURES + 1)) +else + echo "PASS: bundled-script-has-write-approval-gate" +fi + +# --------------------------------------------------------------------------- +# Test helper — reimplements the gating predicate from +# lib/write-approval-gate.lib.sh's apply_write_approval_gate_if_needed: +# gate applies when TRIGGER_ROLE normalizes (lowercased, trimmed) to "triage". +# --------------------------------------------------------------------------- +gate_applies_for_role() { + local trigger_role="${1:-}" + local role + role="$(printf '%s' "${trigger_role}" | tr '[:upper:]' '[:lower:]' | xargs)" + [[ "${role}" == "triage" ]] +} + +run_gate_test() { + local test_name="$1" + local trigger_role="$2" + local expected="$3" # "yes" or "no" + + local actual="no" + gate_applies_for_role "${trigger_role}" && actual="yes" + + if [ "${actual}" != "${expected}" ]; then + echo "FAIL: ${test_name}" + echo " TRIGGER_ROLE='${trigger_role}' expected gate=${expected}, got gate=${actual}" + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS: ${test_name}" +} + +run_gate_test "gate-applies-for-triage" "triage" "yes" +run_gate_test "gate-applies-for-mixed-case" "Triage" "yes" +run_gate_test "gate-applies-for-upper-case" "TRIAGE" "yes" +run_gate_test "gate-applies-with-surrounding-whitespace" " triage " "yes" +run_gate_test "gate-skipped-for-write" "write" "no" +run_gate_test "gate-skipped-for-unset" "" "no" +run_gate_test "gate-skipped-for-garbage-value" "admin" "no" + # --- Summary --- echo "" diff --git a/scripts/post-code.sh b/scripts/post-code.sh index da5ae40c..566bad40 100755 --- a/scripts/post-code.sh +++ b/scripts/post-code.sh @@ -34,6 +34,10 @@ # branch is allowed. (default: auto-detected) # POST_FAILURE_DETAIL_MAX_LINES # — max lines of failure detail in issue/PR comments (default: 30) +# TRIGGER_ROLE — permission tier that authorized this dispatch: "triage" +# or "write" (default: unset, treated as write — no gate). +# When "triage", the created PR is labeled +# needs-write-approval (fullsend-ai/fullsend#5687). # # Exit codes: # 0 — branch pushed and PR created, OR agent determined nothing to do @@ -676,6 +680,76 @@ maybe_assign_pr() { } } # END bundled: lib/pr-assignee.lib.sh +# shellcheck source=lib/write-approval-gate.lib.sh +# BEGIN bundled: lib/write-approval-gate.lib.sh +# write-approval-gate.lib.sh — Merge gate for triage-role-triggered code/fix runs. +# +# Source from post-code.src.sh / post-fix.src.sh (after post-failure-report.lib.sh +# for gha_echo): +# source "${SCRIPT_DIR}/lib/write-approval-gate.lib.sh" +# +# fullsend-ai/fullsend#5687: dispatch now accepts the GitHub `triage` role for +# /fs-code and /fs-fix. Triage-role users still get a bot-authored PR (the +# agent always held write-level credentials), but that PR must carry an +# explicit visible marker so reviewers and merge tooling know it needs a +# write+ collaborator's approval — the actual enforcement (requiring an +# approval from a currently write+ user, not just any reviewDecision) lives +# in skills/merge-queue/scripts/await-and-enqueue.sh, which checks this label. + +# shellcheck shell=bash + +[[ -n "${WRITE_APPROVAL_GATE_SH_LOADED:-}" ]] && return 0 +WRITE_APPROVAL_GATE_SH_LOADED=1 + +# Emit a runner warning through gha_echo when available. +_write_approval_gate_warn() { + if declare -F gha_echo >/dev/null 2>&1; then + gha_echo warning "$*" + else + echo "warning: $*" >&2 + fi +} + +# Normalize a TRIGGER_ROLE value: lowercase and trim surrounding whitespace. +_normalize_trigger_role() { + printf '%s' "${1:-}" | tr '[:upper:]' '[:lower:]' | xargs +} + +# Apply the needs-write-approval label to a PR when TRIGGER_ROLE is "triage" +# (case-insensitive, surrounding whitespace ignored). No-op (and no gh calls) +# when TRIGGER_ROLE is unset or "write" — unset means the trigger was resolved +# at write+ (or a path that predates TRIGGER_ROLE, e.g. bot-triggered +# review->fix), so no gate is needed. Any other non-empty value is treated +# the same as "write" (no gate) but logged, since it likely indicates a bug +# in the caller rather than a legitimate write+ trigger. +# Requires REPO_FULL_NAME. Best-effort: never fails the calling script. +# Note: parameter is target_pr (not pr_number) to avoid SC2153 against +# PR_NUMBER from post-failure-report.lib.sh once both libs are bundled into +# post-code.sh / post-fix.sh — same fix as maybe_assign_pr in pr-assignee.lib.sh. +apply_write_approval_gate_if_needed() { + local target_pr="$1" + local role + role="$(_normalize_trigger_role "${TRIGGER_ROLE:-}")" + + if [[ -z "${role}" ]]; then + return 0 + fi + if [[ "${role}" != "triage" ]]; then + if [[ "${role}" != "write" ]]; then + _write_approval_gate_warn "Unrecognized TRIGGER_ROLE '${TRIGGER_ROLE}' — treating as write (no gate applied). Expected 'triage' or 'write'." + fi + return 0 + fi + + echo "Trigger role is 'triage' — applying needs-write-approval gate to PR #${target_pr}" + gh label create "needs-write-approval" --repo "${REPO_FULL_NAME}" \ + --description "Triggered by a triage-role user; needs write+ approval before merge" \ + --color "B60205" 2>/dev/null || true + gh pr edit "${target_pr}" --repo "${REPO_FULL_NAME}" \ + --add-label "needs-write-approval" 2>/dev/null || \ + _write_approval_gate_warn "Failed to apply needs-write-approval label to PR #${target_pr}" +} +# END bundled: lib/write-approval-gate.lib.sh # --------------------------------------------------------------------------- # Setup @@ -1152,6 +1226,7 @@ if [ -n "${EXISTING_PR_NUM}" ]; then echo "PR: ${EXISTING_PR_URL}" echo "pr_url=${EXISTING_PR_URL}" >> "${GITHUB_OUTPUT:-/dev/null}" maybe_assign_pr "${EXISTING_PR_NUM}" + apply_write_approval_gate_if_needed "${EXISTING_PR_NUM}" exit 0 fi @@ -1303,12 +1378,18 @@ rm -f "${PR_CREATE_STDERR}" echo "PR created: ${PR_URL}" echo "pr_url=${PR_URL}" >> "${GITHUB_OUTPUT:-/dev/null}" +PR_NUMBER_FROM_URL="${PR_URL##*/}" + +# Apply the write-approval gate BEFORE ready-for-review: ready-for-review is +# what dispatches the review agent, so the gate label must already be in +# place before any downstream automation can act on this PR. +apply_write_approval_gate_if_needed "${PR_NUMBER_FROM_URL}" + # Apply ready-for-review label so the review agent is dispatched via the # issues.labeled path. pull_request_target.opened requires the PR author to # pass authorization checks that often exclude bot accounts; the label path # is used instead (label application requires repo write access). See # .github/scripts/check-e2e-authorization-test.sh for trusted-actor rules. -PR_NUMBER_FROM_URL="${PR_URL##*/}" gh issue edit "${PR_NUMBER_FROM_URL}" \ --repo "${REPO_FULL_NAME}" \ --add-label "ready-for-review" 2>/dev/null || \ diff --git a/scripts/post-code.src.sh b/scripts/post-code.src.sh index 7a62821f..fa9ef93f 100755 --- a/scripts/post-code.src.sh +++ b/scripts/post-code.src.sh @@ -33,6 +33,10 @@ # branch is allowed. (default: auto-detected) # POST_FAILURE_DETAIL_MAX_LINES # — max lines of failure detail in issue/PR comments (default: 30) +# TRIGGER_ROLE — permission tier that authorized this dispatch: "triage" +# or "write" (default: unset, treated as write — no gate). +# When "triage", the created PR is labeled +# needs-write-approval (fullsend-ai/fullsend#5687). # # Exit codes: # 0 — branch pushed and PR created, OR agent determined nothing to do @@ -46,6 +50,8 @@ source "${SCRIPT_DIR_POST}/lib/post-failure-report.lib.sh" source "${SCRIPT_DIR_POST}/lib/gitleaks-install.lib.sh" # shellcheck source=lib/pr-assignee.lib.sh source "${SCRIPT_DIR_POST}/lib/pr-assignee.lib.sh" +# shellcheck source=lib/write-approval-gate.lib.sh +source "${SCRIPT_DIR_POST}/lib/write-approval-gate.lib.sh" # --------------------------------------------------------------------------- # Setup @@ -522,6 +528,7 @@ if [ -n "${EXISTING_PR_NUM}" ]; then echo "PR: ${EXISTING_PR_URL}" echo "pr_url=${EXISTING_PR_URL}" >> "${GITHUB_OUTPUT:-/dev/null}" maybe_assign_pr "${EXISTING_PR_NUM}" + apply_write_approval_gate_if_needed "${EXISTING_PR_NUM}" exit 0 fi @@ -673,12 +680,18 @@ rm -f "${PR_CREATE_STDERR}" echo "PR created: ${PR_URL}" echo "pr_url=${PR_URL}" >> "${GITHUB_OUTPUT:-/dev/null}" +PR_NUMBER_FROM_URL="${PR_URL##*/}" + +# Apply the write-approval gate BEFORE ready-for-review: ready-for-review is +# what dispatches the review agent, so the gate label must already be in +# place before any downstream automation can act on this PR. +apply_write_approval_gate_if_needed "${PR_NUMBER_FROM_URL}" + # Apply ready-for-review label so the review agent is dispatched via the # issues.labeled path. pull_request_target.opened requires the PR author to # pass authorization checks that often exclude bot accounts; the label path # is used instead (label application requires repo write access). See # .github/scripts/check-e2e-authorization-test.sh for trusted-actor rules. -PR_NUMBER_FROM_URL="${PR_URL##*/}" gh issue edit "${PR_NUMBER_FROM_URL}" \ --repo "${REPO_FULL_NAME}" \ --add-label "ready-for-review" 2>/dev/null || \ diff --git a/scripts/post-fix-test.sh b/scripts/post-fix-test.sh index 9c652031..1e372272 100755 --- a/scripts/post-fix-test.sh +++ b/scripts/post-fix-test.sh @@ -386,6 +386,53 @@ run_postfix_integration_test "integration-neither-filename-fails-closed" "true" rm -rf "${INTEGRATION_TMPDIR}" +# Verify apply_write_approval_gate_if_needed is present in the post-fix script +if ! grep -q 'apply_write_approval_gate_if_needed' "${POST_SCRIPT}"; then + echo "FAIL: bundled-script-has-write-approval-gate" + echo " ${POST_SCRIPT} missing apply_write_approval_gate_if_needed" + FAILURES=$((FAILURES + 1)) +else + echo "PASS: bundled-script-has-write-approval-gate" +fi + +# --------------------------------------------------------------------------- +# Test helper — reimplements the gating predicate from +# lib/write-approval-gate.lib.sh's apply_write_approval_gate_if_needed: +# gate applies when TRIGGER_ROLE normalizes (lowercased, trimmed) to "triage". +# --------------------------------------------------------------------------- +gate_applies_for_role() { + local trigger_role="${1:-}" + local role + role="$(printf '%s' "${trigger_role}" | tr '[:upper:]' '[:lower:]' | xargs)" + [[ "${role}" == "triage" ]] +} + +run_gate_test() { + local test_name="$1" + local trigger_role="$2" + local expected="$3" # "yes" or "no" + + local actual="no" + gate_applies_for_role "${trigger_role}" && actual="yes" + + if [ "${actual}" != "${expected}" ]; then + echo "FAIL: ${test_name}" + echo " TRIGGER_ROLE='${trigger_role}' expected gate=${expected}, got gate=${actual}" + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS: ${test_name}" +} + +run_gate_test "gate-applies-for-triage" "triage" "yes" +run_gate_test "gate-applies-for-mixed-case" "Triage" "yes" +run_gate_test "gate-applies-for-upper-case" "TRIAGE" "yes" +run_gate_test "gate-applies-with-surrounding-whitespace" " triage " "yes" +run_gate_test "gate-skipped-for-write" "write" "no" +run_gate_test "gate-skipped-for-unset" "" "no" +run_gate_test "gate-skipped-for-garbage-value" "admin" "no" + # --- Summary --- echo "" diff --git a/scripts/post-fix.sh b/scripts/post-fix.sh index 5ead9ec1..8d3f516c 100755 --- a/scripts/post-fix.sh +++ b/scripts/post-fix.sh @@ -45,6 +45,10 @@ # PUSH_TOKEN_SOURCE — "github-app" (for logging) # POST_FAILURE_DETAIL_MAX_LINES # — max lines of failure detail in issue/PR comments (default: 30) +# TRIGGER_ROLE — permission tier that authorized this dispatch: "triage" +# or "write" (default: unset, treated as write — no gate). +# When "triage", the PR is labeled needs-write-approval +# (fullsend-ai/fullsend#5687). # # Exit codes: # 0 — branch pushed, PR updated @@ -502,6 +506,76 @@ install_gitleaks() { export PATH="${HOME}/.local/bin:${PATH}" } # END bundled: lib/gitleaks-install.lib.sh +# shellcheck source=lib/write-approval-gate.lib.sh +# BEGIN bundled: lib/write-approval-gate.lib.sh +# write-approval-gate.lib.sh — Merge gate for triage-role-triggered code/fix runs. +# +# Source from post-code.src.sh / post-fix.src.sh (after post-failure-report.lib.sh +# for gha_echo): +# source "${SCRIPT_DIR}/lib/write-approval-gate.lib.sh" +# +# fullsend-ai/fullsend#5687: dispatch now accepts the GitHub `triage` role for +# /fs-code and /fs-fix. Triage-role users still get a bot-authored PR (the +# agent always held write-level credentials), but that PR must carry an +# explicit visible marker so reviewers and merge tooling know it needs a +# write+ collaborator's approval — the actual enforcement (requiring an +# approval from a currently write+ user, not just any reviewDecision) lives +# in skills/merge-queue/scripts/await-and-enqueue.sh, which checks this label. + +# shellcheck shell=bash + +[[ -n "${WRITE_APPROVAL_GATE_SH_LOADED:-}" ]] && return 0 +WRITE_APPROVAL_GATE_SH_LOADED=1 + +# Emit a runner warning through gha_echo when available. +_write_approval_gate_warn() { + if declare -F gha_echo >/dev/null 2>&1; then + gha_echo warning "$*" + else + echo "warning: $*" >&2 + fi +} + +# Normalize a TRIGGER_ROLE value: lowercase and trim surrounding whitespace. +_normalize_trigger_role() { + printf '%s' "${1:-}" | tr '[:upper:]' '[:lower:]' | xargs +} + +# Apply the needs-write-approval label to a PR when TRIGGER_ROLE is "triage" +# (case-insensitive, surrounding whitespace ignored). No-op (and no gh calls) +# when TRIGGER_ROLE is unset or "write" — unset means the trigger was resolved +# at write+ (or a path that predates TRIGGER_ROLE, e.g. bot-triggered +# review->fix), so no gate is needed. Any other non-empty value is treated +# the same as "write" (no gate) but logged, since it likely indicates a bug +# in the caller rather than a legitimate write+ trigger. +# Requires REPO_FULL_NAME. Best-effort: never fails the calling script. +# Note: parameter is target_pr (not pr_number) to avoid SC2153 against +# PR_NUMBER from post-failure-report.lib.sh once both libs are bundled into +# post-code.sh / post-fix.sh — same fix as maybe_assign_pr in pr-assignee.lib.sh. +apply_write_approval_gate_if_needed() { + local target_pr="$1" + local role + role="$(_normalize_trigger_role "${TRIGGER_ROLE:-}")" + + if [[ -z "${role}" ]]; then + return 0 + fi + if [[ "${role}" != "triage" ]]; then + if [[ "${role}" != "write" ]]; then + _write_approval_gate_warn "Unrecognized TRIGGER_ROLE '${TRIGGER_ROLE}' — treating as write (no gate applied). Expected 'triage' or 'write'." + fi + return 0 + fi + + echo "Trigger role is 'triage' — applying needs-write-approval gate to PR #${target_pr}" + gh label create "needs-write-approval" --repo "${REPO_FULL_NAME}" \ + --description "Triggered by a triage-role user; needs write+ approval before merge" \ + --color "B60205" 2>/dev/null || true + gh pr edit "${target_pr}" --repo "${REPO_FULL_NAME}" \ + --add-label "needs-write-approval" 2>/dev/null || \ + _write_approval_gate_warn "Failed to apply needs-write-approval label to PR #${target_pr}" +} +# END bundled: lib/write-approval-gate.lib.sh # --------------------------------------------------------------------------- # Helper: Bot user detection @@ -893,6 +967,8 @@ if [ "${ITERATION}" -ge "${WARN_THRESHOLD}" ] && is_bot_user "${TRIGGER_SOURCE}" --add-label "needs-human" 2>/dev/null || true fi +apply_write_approval_gate_if_needed "${PR_NUMBER}" + # --------------------------------------------------------------------------- # 7. Summary # --------------------------------------------------------------------------- diff --git a/scripts/post-fix.src.sh b/scripts/post-fix.src.sh index 6a6ddf82..34794336 100755 --- a/scripts/post-fix.src.sh +++ b/scripts/post-fix.src.sh @@ -44,6 +44,10 @@ # PUSH_TOKEN_SOURCE — "github-app" (for logging) # POST_FAILURE_DETAIL_MAX_LINES # — max lines of failure detail in issue/PR comments (default: 30) +# TRIGGER_ROLE — permission tier that authorized this dispatch: "triage" +# or "write" (default: unset, treated as write — no gate). +# When "triage", the PR is labeled needs-write-approval +# (fullsend-ai/fullsend#5687). # # Exit codes: # 0 — branch pushed, PR updated @@ -55,6 +59,8 @@ SCRIPT_DIR_POST="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" source "${SCRIPT_DIR_POST}/lib/post-failure-report.lib.sh" # shellcheck source=lib/gitleaks-install.lib.sh source "${SCRIPT_DIR_POST}/lib/gitleaks-install.lib.sh" +# shellcheck source=lib/write-approval-gate.lib.sh +source "${SCRIPT_DIR_POST}/lib/write-approval-gate.lib.sh" # --------------------------------------------------------------------------- # Helper: Bot user detection @@ -446,6 +452,8 @@ if [ "${ITERATION}" -ge "${WARN_THRESHOLD}" ] && is_bot_user "${TRIGGER_SOURCE}" --add-label "needs-human" 2>/dev/null || true fi +apply_write_approval_gate_if_needed "${PR_NUMBER}" + # --------------------------------------------------------------------------- # 7. Summary # --------------------------------------------------------------------------- diff --git a/skills/merge-queue/SKILL.md b/skills/merge-queue/SKILL.md index 29a97d39..7bab9b19 100644 --- a/skills/merge-queue/SKILL.md +++ b/skills/merge-queue/SKILL.md @@ -18,6 +18,12 @@ Omit the argument to enqueue the current branch's PR. If the PR is not yet eligible (checks pending, missing approvals), use `await-and-enqueue.sh` instead — see below. +A PR that ever carried the `needs-write-approval` label (fullsend-ai/fullsend#5687 — +its dispatching user held only GitHub's `triage` role, not write+) is refused +until an APPROVE review exists, on the PR's current head commit, from a +collaborator whose current permission is admin/maintain/write. Both +`enqueue-pr.sh` and `await-and-enqueue.sh` enforce this independently. + ### Accepted input formats - **PR number:** `652` (uses the current repo context from `gh`) @@ -72,3 +78,4 @@ Set `POLL_INTERVAL` (default: 30 seconds) to control how often it checks. - **"Resource not accessible by integration"** — the `gh` token lacks sufficient permissions. - **"status checks are expected"** — required checks haven't finished yet. Use `await-and-enqueue.sh` to poll and enqueue once they pass. - **`gh pr merge --auto` fails with merge queues** — GitHub's auto-merge API does not support merge queues. Use `await-and-enqueue.sh` instead. +- **"Refusing to enqueue ... required needs-write-approval"** — the PR was triggered by a triage-role user and has no qualifying write+ approval yet. A collaborator with admin/maintain/write permission needs to review and approve the PR's current head commit. diff --git a/skills/merge-queue/scripts/await-and-enqueue.sh b/skills/merge-queue/scripts/await-and-enqueue.sh index c25a40ea..0fac13a9 100755 --- a/skills/merge-queue/scripts/await-and-enqueue.sh +++ b/skills/merge-queue/scripts/await-and-enqueue.sh @@ -9,20 +9,31 @@ set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/write-approval-check.lib.sh +source "${SCRIPT_DIR}/lib/write-approval-check.lib.sh" + POLL_INTERVAL="${POLL_INTERVAL:-30}" pr="${1:-}" -# Resolve PR URL, repo, and base branch +# Resolve PR URL, number, repo, and base branch if [[ -z "$pr" ]]; then - pr_json_init="$(gh pr view --json url,baseRefName,headRepository -q '{url,baseRefName,nwo:.headRepository.owner.login+"/"+.headRepository.name}')" + pr_json_init="$(gh pr view --json url,number,baseRefName,headRepository -q '{url,number,baseRefName,nwo:.headRepository.owner.login+"/"+.headRepository.name}')" else - pr_json_init="$(gh pr view "$pr" --json url,baseRefName,headRepository -q '{url,baseRefName,nwo:.headRepository.owner.login+"/"+.headRepository.name}')" + pr_json_init="$(gh pr view "$pr" --json url,number,baseRefName,headRepository -q '{url,number,baseRefName,nwo:.headRepository.owner.login+"/"+.headRepository.name}')" fi pr_url="$(echo "$pr_json_init" | jq -r .url)" +pr_number="$(echo "$pr_json_init" | jq -r .number)" base_branch="$(echo "$pr_json_init" | jq -r .baseRefName)" repo_nwo="$(echo "$pr_json_init" | jq -r .nwo)" +# fullsend-ai/fullsend#5687: whether this PR ever required write-approval +# (per the immutable issue-events timeline, not current label state — see +# lib/write-approval-check.lib.sh) does not change over the life of the PR, +# so resolve it once rather than on every poll. +ever_required_write_approval="$(write_approval_ever_required "$repo_nwo" "$pr_number")" + # Fetch required status checks from branch rulesets (fail-closed on error). # Note: only the rulesets API is queried. Repositories using classic branch # protection rules (without rulesets) will not have their required checks @@ -41,10 +52,11 @@ fi echo "Waiting for checks and approvals on: $pr_url" while true; do - # Get check rollup and review decision in one call - pr_json="$(gh pr view "$pr_url" --json statusCheckRollup,reviewDecision)" + # Get check rollup, review decision, and current head commit in one call + pr_json="$(gh pr view "$pr_url" --json statusCheckRollup,reviewDecision,headRefOid)" review_decision="$(echo "$pr_json" | jq -r '.reviewDecision // "NONE"')" + head_sha="$(echo "$pr_json" | jq -r '.headRefOid')" # Use jq to analyze all check statuses and required check coverage in one pass result="$(echo "$pr_json" | jq -r --argjson required "$required_json" ' @@ -93,10 +105,16 @@ while true; do continue fi + if [[ "$ever_required_write_approval" == "true" ]] && ! has_write_plus_approval "$repo_nwo" "$pr_number" "$head_sha"; then + echo "PR required needs-write-approval at some point but has no APPROVE review, on the current head commit, from a currently admin/maintain/write collaborator — waiting ${POLL_INTERVAL}s" + sleep "$POLL_INTERVAL" + continue + fi + echo "All checks passed and PR is approved. Enqueuing..." break done -# Delegate to the enqueue script -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# Delegate to the enqueue script, which independently re-verifies the +# write-approval gate at the actual enqueue call site. exec bash "$SCRIPT_DIR/enqueue-pr.sh" "$pr_url" diff --git a/skills/merge-queue/scripts/enqueue-pr.sh b/skills/merge-queue/scripts/enqueue-pr.sh index 3e6e2649..250cb56d 100755 --- a/skills/merge-queue/scripts/enqueue-pr.sh +++ b/skills/merge-queue/scripts/enqueue-pr.sh @@ -7,19 +7,29 @@ set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=lib/write-approval-check.lib.sh +source "${SCRIPT_DIR}/lib/write-approval-check.lib.sh" + pr="${1:-}" -# Resolve PR to its URL and node ID in a single API call +# Resolve PR to its URL, number, node ID, head commit, and repo in one call if [[ -z "$pr" ]]; then - pr_json="$(gh pr view --json url,id)" -elif [[ "$pr" =~ ^[0-9]+$ ]]; then - pr_json="$(gh pr view "$pr" --json url,id)" + pr_json="$(gh pr view --json url,number,id,headRefOid,headRepository -q '{url,number,id,headRefOid,nwo:.headRepository.owner.login+"/"+.headRepository.name}')" else - pr_json="$(gh pr view "$pr" --json url,id)" + pr_json="$(gh pr view "$pr" --json url,number,id,headRefOid,headRepository -q '{url,number,id,headRefOid,nwo:.headRepository.owner.login+"/"+.headRepository.name}')" fi pr_url="$(echo "$pr_json" | jq -r .url)" +pr_number="$(echo "$pr_json" | jq -r .number)" pr_node_id="$(echo "$pr_json" | jq -r .id)" +pr_head_sha="$(echo "$pr_json" | jq -r .headRefOid)" +repo_nwo="$(echo "$pr_json" | jq -r .nwo)" + +if ! enforce_write_approval_gate "$repo_nwo" "$pr_number" "$pr_head_sha"; then + echo "Refusing to enqueue $pr_url — see message above." >&2 + exit 1 +fi echo "Enqueuing: $pr_url" diff --git a/skills/merge-queue/scripts/lib/write-approval-check.lib.sh b/skills/merge-queue/scripts/lib/write-approval-check.lib.sh new file mode 100644 index 00000000..a9ec5967 --- /dev/null +++ b/skills/merge-queue/scripts/lib/write-approval-check.lib.sh @@ -0,0 +1,88 @@ +#!/usr/bin/env bash +# write-approval-check.lib.sh — Shared enforcement for fullsend-ai/fullsend#5687. +# +# A PR whose issue/PR timeline ever recorded a needs-write-approval label +# (applied by post-code.sh/post-fix.sh when the dispatching user held only +# GitHub's triage role, not write+) must not be enqueued/merged without a +# live APPROVE review, on the PR's current head commit, from a human +# collaborator whose CURRENT permission is admin/maintain/write. +# +# Source from enqueue-pr.sh / await-and-enqueue.sh: +# source "${SCRIPT_DIR}/lib/write-approval-check.lib.sh" +# +# Design notes: +# - "Ever recorded", not "currently labeled": GitHub's triage role includes +# repo-wide label management, so a triage-role user could otherwise strip +# needs-write-approval from their own bot-authored PR and skip this check +# entirely. The issue events timeline records the labeled event +# permanently; removing the label later does not erase it. +# - Pinned to the current head commit: an APPROVE review does not +# automatically become stale when new commits land unless a repo's +# branch protection specifically enables that — which this check cannot +# assume is configured. Re-deriving "is the latest approval for the +# current code" here closes that gap independent of branch protection. +# - Bot/App reviewers never count: even though nothing in this pipeline +# currently submits an automated APPROVE review, this stays correct if +# that ever changes. +# - This must be enforced at the actual enqueue call site (enqueue-pr.sh), +# not only in a polling wrapper — await-and-enqueue.sh is one of two +# documented entry points and simply execs enqueue-pr.sh at the end, so +# putting the check there alone would not cover a direct enqueue-pr.sh +# invocation. + +[[ -n "${WRITE_APPROVAL_CHECK_SH_LOADED:-}" ]] && return 0 +WRITE_APPROVAL_CHECK_SH_LOADED=1 + +# Whether needs-write-approval was ever applied to this PR, per the issue +# events timeline (immutable to a later label removal). Prints "true"/"false". +write_approval_ever_required() { + local repo="$1" number="$2" + gh api "repos/${repo}/issues/${number}/events" --paginate 2>/dev/null \ + | jq -s -r ' + [add // [] | .[] | select(.event == "labeled" and .label.name == "needs-write-approval")] + | length > 0 + ' 2>/dev/null || echo "true" +} + +# At least one APPROVE review, on the PR's current head commit, from a human +# (non-bot) collaborator whose current permission is admin/maintain/write. +has_write_plus_approval() { + local repo="$1" number="$2" head_sha="$3" + local reviews approved_users user role + reviews="$(gh api "repos/${repo}/pulls/${number}/reviews" --paginate 2>/dev/null)" || return 1 + approved_users="$(echo "${reviews}" | jq -s -r --arg head "${head_sha}" ' + add // [] + | group_by(.user.login) + | map(max_by(.submitted_at)) + | map(select(.state == "APPROVED" and .commit_id == $head)) + | map(select((.user.login // "") | test("\\[bot\\]$") | not)) + | map(select(.user.login != "dependabot")) + | .[].user.login + ')" + [[ -z "${approved_users}" ]] && return 1 + while IFS= read -r user; do + [[ -z "${user}" ]] && continue + role="$(gh api "repos/${repo}/collaborators/${user}/permission" --jq '.role_name' 2>/dev/null || echo "")" + case "${role}" in + admin|maintain|write) return 0 ;; + esac + done <<< "${approved_users}" + return 1 +} + +# Fails closed: on any error resolving the requirement, treat the gate as +# required and unsatisfied rather than silently permitting enqueue. +# Args: repo, pr_number, head_sha. Prints a message and returns 1 on failure. +enforce_write_approval_gate() { + local repo="$1" number="$2" head_sha="$3" + local ever_required + ever_required="$(write_approval_ever_required "${repo}" "${number}")" + if [[ "${ever_required}" != "true" ]]; then + return 0 + fi + if has_write_plus_approval "${repo}" "${number}" "${head_sha}"; then + return 0 + fi + echo "PR #${number} required needs-write-approval at some point (GitHub triage-role trigger, fullsend-ai/fullsend#5687) but has no APPROVE review, on its current head commit, from a currently admin/maintain/write human collaborator." >&2 + return 1 +} diff --git a/skills/merge-queue/scripts/write-approval-check-test.sh b/skills/merge-queue/scripts/write-approval-check-test.sh new file mode 100644 index 00000000..a0a20cf0 --- /dev/null +++ b/skills/merge-queue/scripts/write-approval-check-test.sh @@ -0,0 +1,153 @@ +#!/usr/bin/env bash +# write-approval-check-test.sh — Tests the jq filters in +# lib/write-approval-check.lib.sh against fixture JSON, without live gh calls. +# +# Run from the repo root: +# bash skills/merge-queue/scripts/write-approval-check-test.sh + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +FAILURES=0 + +if ! grep -q 'enforce_write_approval_gate' "${SCRIPT_DIR}/enqueue-pr.sh"; then + echo "FAIL: enqueue-pr-has-write-approval-gate" + echo " enqueue-pr.sh does not enforce the write-approval gate (this is the primary documented enqueue entry point)" + FAILURES=$((FAILURES + 1)) +else + echo "PASS: enqueue-pr-has-write-approval-gate" +fi + +if ! grep -q 'write_approval_ever_required' "${SCRIPT_DIR}/await-and-enqueue.sh"; then + echo "FAIL: await-and-enqueue-has-write-approval-gate" + FAILURES=$((FAILURES + 1)) +else + echo "PASS: await-and-enqueue-has-write-approval-gate" +fi + +# --------------------------------------------------------------------------- +# write_approval_ever_required — same jq filter as lib/write-approval-check.lib.sh +# --------------------------------------------------------------------------- +EVER_REQUIRED_FILTER=' + [add // [] | .[] | select(.event == "labeled" and .label.name == "needs-write-approval")] + | length > 0 +' + +run_ever_required_test() { + local test_name="$1" + local events_json="$2" + local expected="$3" # "true" or "false" + + local actual + actual="$(echo "${events_json}" | jq -s -r "${EVER_REQUIRED_FILTER}")" + + if [ "${actual}" != "${expected}" ]; then + echo "FAIL: ${test_name}" + echo " expected: ${expected}" + echo " actual: ${actual}" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name}" +} + +run_ever_required_test "never-labeled" \ + '[{"event": "commented"}, {"event": "assigned"}]' \ + "false" + +run_ever_required_test "currently-labeled" \ + '[{"event": "labeled", "label": {"name": "needs-write-approval"}}]' \ + "true" + +run_ever_required_test "labeled-then-unlabeled-still-counts" \ + '[{"event": "labeled", "label": {"name": "needs-write-approval"}}, {"event": "unlabeled", "label": {"name": "needs-write-approval"}}]' \ + "true" + +run_ever_required_test "different-label-does-not-count" \ + '[{"event": "labeled", "label": {"name": "ready-for-review"}}]' \ + "false" + +run_ever_required_test "empty-events" \ + '[]' \ + "false" + +# --------------------------------------------------------------------------- +# has_write_plus_approval's approved_users filter — same jq filter as +# lib/write-approval-check.lib.sh, run against fixture PR reviews. +# --------------------------------------------------------------------------- +APPROVED_USERS_FILTER=' + add // [] + | group_by(.user.login) + | map(max_by(.submitted_at)) + | map(select(.state == "APPROVED" and .commit_id == $head)) + | map(select((.user.login // "") | test("\\[bot\\]$") | not)) + | map(select(.user.login != "dependabot")) + | .[].user.login +' + +run_approved_users_test() { + local test_name="$1" + local reviews_json="$2" + local head_sha="$3" + local expected="$4" # newline-separated expected logins, or "" for none + + local actual + actual="$(echo "${reviews_json}" | jq -s -r --arg head "${head_sha}" "${APPROVED_USERS_FILTER}")" + + if [ "${actual}" != "${expected}" ]; then + echo "FAIL: ${test_name}" + echo " expected: '${expected}'" + echo " actual: '${actual}'" + FAILURES=$((FAILURES + 1)) + return + fi + echo "PASS: ${test_name}" +} + +run_approved_users_test "single-human-approval-on-head" \ + '[{"user": {"login": "alice"}, "state": "APPROVED", "commit_id": "abc123", "submitted_at": "2026-01-01T00:00:00Z"}]' \ + "abc123" \ + "alice" + +run_approved_users_test "bot-approval-excluded" \ + '[{"user": {"login": "some-bot[bot]"}, "state": "APPROVED", "commit_id": "abc123", "submitted_at": "2026-01-01T00:00:00Z"}]' \ + "abc123" \ + "" + +run_approved_users_test "dependabot-excluded" \ + '[{"user": {"login": "dependabot"}, "state": "APPROVED", "commit_id": "abc123", "submitted_at": "2026-01-01T00:00:00Z"}]' \ + "abc123" \ + "" + +run_approved_users_test "stale-approval-on-old-commit-excluded" \ + '[{"user": {"login": "alice"}, "state": "APPROVED", "commit_id": "old-sha", "submitted_at": "2026-01-01T00:00:00Z"}]' \ + "new-sha" \ + "" + +run_approved_users_test "latest-review-wins-approve-then-request-changes" \ + '[{"user": {"login": "alice"}, "state": "APPROVED", "commit_id": "abc123", "submitted_at": "2026-01-01T00:00:00Z"}, {"user": {"login": "alice"}, "state": "CHANGES_REQUESTED", "commit_id": "abc123", "submitted_at": "2026-01-02T00:00:00Z"}]' \ + "abc123" \ + "" + +run_approved_users_test "latest-review-wins-request-changes-then-approve" \ + '[{"user": {"login": "alice"}, "state": "CHANGES_REQUESTED", "commit_id": "abc123", "submitted_at": "2026-01-01T00:00:00Z"}, {"user": {"login": "alice"}, "state": "APPROVED", "commit_id": "abc123", "submitted_at": "2026-01-02T00:00:00Z"}]' \ + "abc123" \ + "alice" + +run_approved_users_test "multiple-approvers-mixed" \ + '[{"user": {"login": "alice"}, "state": "APPROVED", "commit_id": "abc123", "submitted_at": "2026-01-01T00:00:00Z"}, {"user": {"login": "bot-account[bot]"}, "state": "APPROVED", "commit_id": "abc123", "submitted_at": "2026-01-01T00:00:00Z"}]' \ + "abc123" \ + "alice" + +run_approved_users_test "no-reviews" \ + '[]' \ + "abc123" \ + "" + +echo "" +if [ ${FAILURES} -gt 0 ]; then + echo "${FAILURES} test(s) failed" + exit 1 +fi +echo "All tests passed"