From e922351b78fd08397a611955f10dbd837f489c5f Mon Sep 17 00:00:00 2001 From: core-devops Date: Mon, 11 May 2026 11:30:34 -0700 Subject: [PATCH 1/2] feat(ci): add qa-review + security-review checks (RFC#324 Step 1 of 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the two job-conclusion-as-status review-gate workflows that will replace sop-tier-check (Step 3 of RFC#324). Both: - Trigger on pull_request_target (opened/synchronize/reopened) for the initial status, plus issue_comment for /qa-recheck and /security-recheck slash-command refire (Gitea 1.22.6 doesn't refire on pull_request_review per go-gitea/gitea#33700). - Use job name 'approved' so the published context is 'qa-review / approved' and 'security-review / approved' — NO POST /statuses, NO write:repository scope (RFC#324 v1.1 addendum A1-α). - Privilege-check slash-command commenters via /repos/.../collaborators/{u} (NOT github.event.comment.author_association — that field doesn't exist on Gitea 1.22.6, defect #1 from sop-tier-refire). - Run under pull_request_target's BASE-branch trust boundary; checkout pins to default_branch (never head.sha) and the workflows only HTTP-call the Gitea API; no PR-head code is executed (RFC#324 A4 + internal#116). Shared evaluator lives at .gitea/scripts/review-check.sh, parameterized by TEAM + TEAM_ID. Pass condition: at least one APPROVED, non-dismissed, non-author review whose user is a member of the named team. Branch-protection flip (Step 2) is intentionally NOT included in this PR. That is Owners-tier and blocked on (a) the first run of these workflows capturing the EXACT status-context names, and (b) RFC_324_TEAM_READ_TOKEN provisioning (filed as internal#325). Refs: internal#324, internal#325 (token follow-up). Closes: nothing yet — Steps 2 and 3 must land before #292/#319/#321 close. Co-Authored-By: Claude Opus 4.7 (1M context) --- .gitea/scripts/review-check.sh | 184 +++++++++++++++++++++++++++ .gitea/workflows/qa-review.yml | 146 +++++++++++++++++++++ .gitea/workflows/security-review.yml | 71 +++++++++++ 3 files changed, 401 insertions(+) create mode 100755 .gitea/scripts/review-check.sh create mode 100644 .gitea/workflows/qa-review.yml create mode 100644 .gitea/workflows/security-review.yml diff --git a/.gitea/scripts/review-check.sh b/.gitea/scripts/review-check.sh new file mode 100755 index 00000000..dcb92ac4 --- /dev/null +++ b/.gitea/scripts/review-check.sh @@ -0,0 +1,184 @@ +#!/usr/bin/env bash +# review-check — evaluate whether a PR satisfies a single team-review gate. +# +# RFC#324 Step 1 of 5 — qa-review + security-review check workflows. +# +# This is the shared evaluator invoked by: +# .gitea/workflows/qa-review.yml (TEAM=qa, TEAM_ID=20) +# .gitea/workflows/security-review.yml (TEAM=security, TEAM_ID=21) +# +# Pass condition (per RFC#324 v1.1 addendum): +# ≥ 1 review on the PR where: +# • state == APPROVED +# • review.dismissed == false +# • review.user.login != PR.user.login (non-author) +# • review.user.login ∈ team-members +# +# Strict mode (default OFF for v1; see RFC trade-off note): +# If REVIEW_CHECK_STRICT=1, additionally require review.commit_id == PR.head.sha. +# With dismiss_stale_reviews: true at the protection layer, stale reviews +# are already dismissed, so the additional commit_id check is belt-and- +# suspenders. Keeping it off in v1 simplifies semantics; flip in a follow-up +# PR if reviewer telemetry shows residual stale-APPROVE merges. +# +# Privilege gate (RFC A1.1 — CRITICAL): +# The /qa-recheck and /security-recheck slash-command refire must NOT +# re-fire for non-collaborators. The workflow-yaml privilege-check step +# handles this BEFORE this script runs. This script does not re-check +# privilege; the workflow guards the entry. +# +# Trust boundary (RFC A4): +# This script is loaded from the BASE branch (sourced via .gitea/scripts/ +# on the workflow's checkout-of-base). It does NOT execute any PR-HEAD +# code. It only reads PR review state via the Gitea API. +# +# Token scope (RFC A1-α): +# The job's own conclusion (exit 0 / exit 1) is what publishes the +# `qa-review / approved` / `security-review / approved` status context. +# NO `POST /statuses` call here → NO `write:repository` scope on the +# token. `read:organization` (for team-membership probe) and +# `read:repository` (for PR + reviews) are enough. +# +# Required env: +# GITEA_TOKEN — least-priv read:repository + read:organization. See note +# below about the team-membership API requiring the token +# owner to be in the queried team (Gitea 1.22.6 quirk). +# GITEA_HOST — e.g. git.moleculesai.app +# REPO — owner/name (from github.repository) +# PR_NUMBER — int (from github.event.pull_request.number or +# github.event.issue.number for issue_comment events) +# TEAM — short team name (qa | security) for log lines +# TEAM_ID — Gitea team id (20=qa, 21=security at time of writing) +# +# Optional: +# REVIEW_CHECK_DEBUG=1 — per-API-call diagnostic lines +# REVIEW_CHECK_STRICT=1 — also require review.commit_id == pr.head.sha + +set -euo pipefail + +# jq is required for JSON parsing — same install dance sop-tier-check uses. +if ! command -v jq >/dev/null 2>&1; then + echo "::notice::jq not on PATH — installing..." + if apt-get update -qq && apt-get install -y -qq jq 2>/dev/null; then + echo "::notice::jq installed via apt-get: $(jq --version)" + elif timeout 60 curl -sSL \ + "https://github.com/jqlang/jq/releases/download/jq-1.7.1/jq-linux-amd64" \ + -o /usr/local/bin/jq && chmod +x /usr/local/bin/jq; then + echo "::notice::jq downloaded as binary: $(/usr/local/bin/jq --version)" + else + echo "::error::jq install failed; cannot parse Gitea API JSON" + exit 1 + fi +fi + +: "${GITEA_TOKEN:?GITEA_TOKEN required}" +: "${GITEA_HOST:?GITEA_HOST required}" +: "${REPO:?REPO required (owner/name)}" +: "${PR_NUMBER:?PR_NUMBER required}" +: "${TEAM:?TEAM required (qa|security)}" +: "${TEAM_ID:?TEAM_ID required (integer)}" + +OWNER="${REPO%%/*}" +NAME="${REPO##*/}" +API="https://${GITEA_HOST}/api/v1" +AUTH="Authorization: token ${GITEA_TOKEN}" + +debug() { + if [ "${REVIEW_CHECK_DEBUG:-}" = "1" ]; then + echo " [debug] $*" >&2 + fi +} + +echo "::notice::${TEAM}-review evaluating repo=${OWNER}/${NAME} pr=${PR_NUMBER} team_id=${TEAM_ID}" + +# --- Fetch the PR (for author + head.sha) --- +PR_JSON=$(mktemp) +trap 'rm -f "$PR_JSON" "$REVIEWS_JSON" "$TEAM_PROBE_TMP" 2>/dev/null || true' EXIT +HTTP_CODE=$(curl -sS -o "$PR_JSON" -w '%{http_code}' \ + -H "$AUTH" "${API}/repos/${OWNER}/${NAME}/pulls/${PR_NUMBER}") +if [ "$HTTP_CODE" != "200" ]; then + echo "::error::GET /pulls/${PR_NUMBER} returned HTTP ${HTTP_CODE} (token scope?)" + cat "$PR_JSON" >&2 + exit 1 +fi +PR_AUTHOR=$(jq -r '.user.login // ""' "$PR_JSON") +PR_HEAD_SHA=$(jq -r '.head.sha // ""' "$PR_JSON") +PR_STATE=$(jq -r '.state // ""' "$PR_JSON") +debug "pr_author=${PR_AUTHOR} pr_head=${PR_HEAD_SHA:0:7} pr_state=${PR_STATE}" + +if [ "$PR_STATE" != "open" ]; then + echo "::notice::PR ${PR_NUMBER} is ${PR_STATE} — exiting 0 (closed PRs do not gate)" + exit 0 +fi +if [ -z "$PR_AUTHOR" ] || [ -z "$PR_HEAD_SHA" ]; then + echo "::error::PR ${PR_NUMBER} missing user.login or head.sha — webhook payload malformed" + exit 1 +fi + +# --- Fetch all reviews on the PR --- +REVIEWS_JSON=$(mktemp) +HTTP_CODE=$(curl -sS -o "$REVIEWS_JSON" -w '%{http_code}' \ + -H "$AUTH" "${API}/repos/${OWNER}/${NAME}/pulls/${PR_NUMBER}/reviews") +if [ "$HTTP_CODE" != "200" ]; then + echo "::error::GET /pulls/${PR_NUMBER}/reviews returned HTTP ${HTTP_CODE}" + cat "$REVIEWS_JSON" >&2 + exit 1 +fi + +# Filter: state=APPROVED, not-dismissed, non-author. Optionally strict-mode +# adds commit_id==head.sha (off by default; see header). +JQ_FILTER='.[] + | select(.state == "APPROVED") + | select(.dismissed != true) + | select(.user.login != $author)' +if [ "${REVIEW_CHECK_STRICT:-}" = "1" ]; then + JQ_FILTER="${JQ_FILTER} + | select(.commit_id == \$head)" +fi +JQ_FILTER="${JQ_FILTER} + | .user.login" + +CANDIDATES=$(jq -r --arg author "$PR_AUTHOR" --arg head "$PR_HEAD_SHA" "$JQ_FILTER" "$REVIEWS_JSON" | sort -u) +debug "candidate non-author approvers: $(echo "$CANDIDATES" | tr '\n' ' ')" + +if [ -z "$CANDIDATES" ]; then + echo "::error::${TEAM}-review awaiting non-author APPROVE from ${TEAM} team (no candidates yet)" + exit 1 +fi + +# --- Probe team membership per candidate --- +# Endpoint: GET /api/v1/teams/{id}/members/{username} +# 200/204 → is member +# 403 → token owner is not in this team (Gitea 1.22.6 'Must be a team +# member' constraint — see follow-up issue for token-provisioning) +# 404 → not a member +TEAM_PROBE_TMP=$(mktemp) +for U in $CANDIDATES; do + CODE=$(curl -sS -o "$TEAM_PROBE_TMP" -w '%{http_code}' \ + -H "$AUTH" "${API}/teams/${TEAM_ID}/members/${U}") + debug "probe ${U} in team ${TEAM} (id=${TEAM_ID}) → HTTP ${CODE}" + case "$CODE" in + 200|204) + echo "::notice::${TEAM}-review APPROVED by ${U} (team=${TEAM})" + exit 0 + ;; + 403) + # Token owner is not in the team being probed; the API refuses to + # confirm membership. This is the RFC#324 follow-up token-scope gap. + # Fail closed — never grant approval on a 403; surface clearly. + echo "::error::team-probe for ${U} in ${TEAM} returned 403 (token owner not in ${TEAM} team — RFC#324 token-scope follow-up). Cannot confirm membership; failing closed." + cat "$TEAM_PROBE_TMP" >&2 + exit 1 + ;; + 404) + debug "${U} not a member of ${TEAM}" + ;; + *) + echo "::warning::team-probe for ${U} in ${TEAM} returned unexpected HTTP ${CODE}" + cat "$TEAM_PROBE_TMP" >&2 + ;; + esac +done + +echo "::error::${TEAM}-review awaiting non-author APPROVE from ${TEAM} team (candidates: $(echo "$CANDIDATES" | tr '\n' ',' | sed 's/,$//') — none are in team)" +exit 1 diff --git a/.gitea/workflows/qa-review.yml b/.gitea/workflows/qa-review.yml new file mode 100644 index 00000000..18919a2d --- /dev/null +++ b/.gitea/workflows/qa-review.yml @@ -0,0 +1,146 @@ +# qa-review — non-author APPROVE from the `qa` Gitea team required to merge. +# +# RFC#324 Step 1 of 5 (workflow-add). Pairs with `security-review.yml` and the +# branch-protection flip in Step 2. +# +# === DESIGN (RFC#324 v1.1 addendum) === +# +# A1-α (refire mechanism): +# Triggers on: +# - `pull_request_target`: opened, synchronize, reopened +# → initial status posts when PR opens / re-pushes +# - `issue_comment`: /qa-recheck slash-command on the PR +# → manual re-fire after a QA reviewer clicks APPROVE +# (Gitea 1.22.6 doesn't re-fire on pull_request_review, per +# go-gitea/gitea#33700 + feedback_pull_request_review_no_refire) +# Workflow name = `qa-review` ; job name = `approved`. +# The job's own pass/fail conclusion publishes the status context +# `qa-review / approved ()` — NO `POST /statuses` call → NO +# write:repository token scope needed. Sidesteps internal#321 defect #2. +# +# A1.1 (privilege check on slash-comment): +# The `issue_comment` event fires for ANY commenter, including +# non-collaborators. We MUST NOT use `github.event.comment.author_association` +# (the field doesn't exist on Gitea 1.22.6 webhook payload — this was +# sop-tier-refire's defect #1). Instead, the Privilege check step probes +# GET /repos/.../collaborators/{login}: +# - 204 → is-collaborator → proceed +# - 404 → not-collaborator → exit 0 (ignored; status unchanged) +# +# A4 (no PR-head checkout under pull_request_target): +# We check out the BASE ref explicitly so the review-check.sh script is +# loaded from trusted source. We NEVER use `ref: ${{ github.event.pull_request.head.sha }}`. +# No PR-head code is executed in the runner. Trust boundary preserved. +# +# A5 (real Gitea team): +# `qa` team (id=20) verified by orchestrator preflight 2026-05-11; queried +# at run time via /api/v1/teams/20/members/{login}. +# +# === TOKEN === +# +# The workflow reads PR state, PR reviews, and team membership. +# Gitea 1.22.6's /api/v1/teams/{id}/members/{u} returns 403 ('Must be a +# team member') for tokens whose owner is not in that team. The default +# `secrets.GITHUB_TOKEN` is owned by a workflow-scoped identity that is +# also not in qa/security teams → also 403. +# +# Resolution: a dedicated `RFC_324_TEAM_READ_TOKEN` secret, owned by an +# identity that IS in both `qa` and `security` teams (Owners-tier +# claude-ceo-assistant, or a new service-bot added to both teams). +# Provisioning of this secret is tracked as a follow-up issue (filed by +# core-devops at PR open). +# +# Until that secret is provisioned, the job will exit 1 with a clear +# 403-on-team-probe error and the `qa-review / approved` status will +# stay `failure`. This is the correct fail-closed behavior — the gate +# blocks merge until both (a) a QA team member APPROVEs and (b) the +# workflow has a token that can confirm their team membership. +# +# === SLASH-COMMAND CONTRACT === +# +# /qa-recheck — re-evaluate the gate (e.g. after an APPROVE lands) +# +# Restricted to repo collaborators only (privilege-check step A1.1). + +name: qa-review + +on: + pull_request_target: + types: [opened, synchronize, reopened] + issue_comment: + types: [created] + +permissions: + contents: read + pull-requests: read + +jobs: + approved: + # Gate the job: + # - On pull_request_target events: always run. + # - On issue_comment events: only when it's a PR comment and the body + # contains the slash-command. The privilege check is enforced as + # a job step (A1.1), not here — `if:` can't call the collaborator + # API. The privilege step exits the job early for non-collaborators + # while still publishing the job's previous status (i.e. it does + # NOT flip qa-review from failure to success without a real + # team-member APPROVE). + if: | + github.event_name == 'pull_request_target' || + (github.event_name == 'issue_comment' && + github.event.issue.pull_request != null && + startsWith(github.event.comment.body, '/qa-recheck')) + runs-on: ubuntu-latest + steps: + - name: Privilege check (A1.1 — collaborator API, NOT author_association) + # Only runs on issue_comment events. For pull_request_target the + # commenter doesn't exist, so the step is a no-op skip. + id: priv + if: github.event_name == 'issue_comment' + env: + GITEA_TOKEN: ${{ secrets.RFC_324_TEAM_READ_TOKEN || secrets.GITHUB_TOKEN }} + run: | + set -euo pipefail + login="${{ github.event.comment.user.login }}" + code=$(curl -sS -o /dev/null -w '%{http_code}' \ + -H "Authorization: token ${GITEA_TOKEN}" \ + "${{ github.server_url }}/api/v1/repos/${{ github.repository }}/collaborators/${login}") + if [ "$code" = "204" ]; then + echo "::notice::commenter ${login} is a collaborator — proceeding" + echo "proceed=true" >> "$GITHUB_OUTPUT" + else + echo "::notice::comment from non-collaborator ${login} (HTTP ${code}) ignored" + echo "proceed=false" >> "$GITHUB_OUTPUT" + fi + + - name: Check out BASE ref (A4 — never PR-head) + # Loads the review-check.sh script from a trusted ref. For + # pull_request_target the default checkout is BASE already; we + # set ref explicitly for the issue_comment event too so the + # script source is always the default-branch version. + # NEVER use ref: ${{ github.event.pull_request.head.sha }} — + # that would execute PR-head code with secrets-context. + if: | + github.event_name == 'pull_request_target' || + (github.event_name == 'issue_comment' && steps.priv.outputs.proceed == 'true') + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.event.repository.default_branch }} + + - name: Evaluate qa-review + if: | + github.event_name == 'pull_request_target' || + (github.event_name == 'issue_comment' && steps.priv.outputs.proceed == 'true') + env: + GITEA_TOKEN: ${{ secrets.RFC_324_TEAM_READ_TOKEN || secrets.GITHUB_TOKEN }} + GITEA_HOST: git.moleculesai.app + REPO: ${{ github.repository }} + # PR number lives in different places per event: + # pull_request_target → github.event.pull_request.number + # issue_comment → github.event.issue.number + PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }} + TEAM: qa + TEAM_ID: '20' + REVIEW_CHECK_DEBUG: '0' + REVIEW_CHECK_STRICT: '0' + run: bash .gitea/scripts/review-check.sh diff --git a/.gitea/workflows/security-review.yml b/.gitea/workflows/security-review.yml new file mode 100644 index 00000000..c7b4d3fb --- /dev/null +++ b/.gitea/workflows/security-review.yml @@ -0,0 +1,71 @@ +# security-review — non-author APPROVE from the `security` Gitea team +# required to merge. +# +# RFC#324 Step 1 of 5 (workflow-add). Mirror of `qa-review.yml`; differs +# only in TEAM=security, TEAM_ID=21, and the slash-command name. +# +# See `qa-review.yml` header for the full A1-α / A1.1 / A4 / A5 design +# rationale; everything below is identical in shape. + +name: security-review + +on: + pull_request_target: + types: [opened, synchronize, reopened] + issue_comment: + types: [created] + +permissions: + contents: read + pull-requests: read + +jobs: + approved: + if: | + github.event_name == 'pull_request_target' || + (github.event_name == 'issue_comment' && + github.event.issue.pull_request != null && + startsWith(github.event.comment.body, '/security-recheck')) + runs-on: ubuntu-latest + steps: + - name: Privilege check (A1.1 — collaborator API, NOT author_association) + id: priv + if: github.event_name == 'issue_comment' + env: + GITEA_TOKEN: ${{ secrets.RFC_324_TEAM_READ_TOKEN || secrets.GITHUB_TOKEN }} + run: | + set -euo pipefail + login="${{ github.event.comment.user.login }}" + code=$(curl -sS -o /dev/null -w '%{http_code}' \ + -H "Authorization: token ${GITEA_TOKEN}" \ + "${{ github.server_url }}/api/v1/repos/${{ github.repository }}/collaborators/${login}") + if [ "$code" = "204" ]; then + echo "::notice::commenter ${login} is a collaborator — proceeding" + echo "proceed=true" >> "$GITHUB_OUTPUT" + else + echo "::notice::comment from non-collaborator ${login} (HTTP ${code}) ignored" + echo "proceed=false" >> "$GITHUB_OUTPUT" + fi + + - name: Check out BASE ref (A4 — never PR-head) + if: | + github.event_name == 'pull_request_target' || + (github.event_name == 'issue_comment' && steps.priv.outputs.proceed == 'true') + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.event.repository.default_branch }} + + - name: Evaluate security-review + if: | + github.event_name == 'pull_request_target' || + (github.event_name == 'issue_comment' && steps.priv.outputs.proceed == 'true') + env: + GITEA_TOKEN: ${{ secrets.RFC_324_TEAM_READ_TOKEN || secrets.GITHUB_TOKEN }} + GITEA_HOST: git.moleculesai.app + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }} + TEAM: security + TEAM_ID: '21' + REVIEW_CHECK_DEBUG: '0' + REVIEW_CHECK_STRICT: '0' + run: bash .gitea/scripts/review-check.sh From ecbfa60f044f20c9d9eca21f4d5722cc4eb2b870 Mon Sep 17 00:00:00 2001 From: core-devops Date: Mon, 11 May 2026 11:45:59 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(ci):=20close=20fail-open=20in=20qa/secu?= =?UTF-8?q?rity=20review=20checks=20(RFC#324=20v1.3=20=C2=A7A1.1)=20+=20dr?= =?UTF-8?q?op=20dead=20jq=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses hongming-pc review #1421 on PR #535. Blocker 1 (fail-open privilege gate): Original v1.2 design `if:`-gated the "Check out BASE" and "Evaluate" steps on the privilege-check step's `proceed` output. A non-collaborator commenting `/qa-recheck` produced proceed=false → both steps skipped → job conclusion = success → `qa-review / approved` context published as success with ZERO real APPROVE. Any visitor could green the gate. Fix per RFC#324 v1.3 §A1.1 option (b): drop privilege-gating of the eval entirely. The eval is read-only and idempotent (reads pulls/{N}/reviews + teams/{id}/members/{u}, both server-side state uninfluenced by who commented). Re-running on a non-collaborator's comment is harmless: if a real team-member APPROVE exists, the eval flips green; if not, it stays red. The privilege step is retained as a `::notice::` log line only (griefer-spotting), not a gate. Non-blocking nit 5 (dead jq fallback): `apt-get install jq` (no root) and `curl -o /usr/local/bin/jq` (no write perm on uid-1001 rootless runner) both can't succeed. Per feedback_ci_runner_install_needs_writable_path + #391/#402, jq is already baked into runner-base. Replace the install dance with a clear `exit 1` + diagnostic so a missing-jq runner fails loud rather than confusingly. Smoke-test (mocked Gitea API): no-approve → exit 1 (gate red) self-approve → exit 1 (gate red) dismissed-approve → exit 1 (gate red) non-team-approve → exit 1 (gate red) team-approve → exit 0 (gate green) Blocker 2 (A1-α event-suffix context-name verification) is the smoke-PR's job and is flagged in a follow-up comment on this PR — does not require workflow changes here. Co-Authored-By: Claude Opus 4.7 (1M context) --- .gitea/scripts/review-check.sh | 38 ++++++++------- .gitea/workflows/qa-review.yml | 69 +++++++++++++++++----------- .gitea/workflows/security-review.yml | 20 ++++---- 3 files changed, 70 insertions(+), 57 deletions(-) diff --git a/.gitea/scripts/review-check.sh b/.gitea/scripts/review-check.sh index dcb92ac4..858e6c41 100755 --- a/.gitea/scripts/review-check.sh +++ b/.gitea/scripts/review-check.sh @@ -21,11 +21,18 @@ # suspenders. Keeping it off in v1 simplifies semantics; flip in a follow-up # PR if reviewer telemetry shows residual stale-APPROVE merges. # -# Privilege gate (RFC A1.1 — CRITICAL): -# The /qa-recheck and /security-recheck slash-command refire must NOT -# re-fire for non-collaborators. The workflow-yaml privilege-check step -# handles this BEFORE this script runs. This script does not re-check -# privilege; the workflow guards the entry. +# Privilege gate (RFC#324 v1.3 §A1.1 — INFORMATIONAL ONLY): +# The /qa-recheck and /security-recheck slash-commands can be triggered +# by anyone who can comment on the PR. The workflow's privilege step +# logs collaborator-status but does NOT gate execution of this script. +# Why this is safe: this evaluator is read-only and idempotent — +# reading `pulls/{N}/reviews` and `teams/{id}/members/{u}` can't be +# influenced by who triggered the run. If a real team-member APPROVE +# exists the gate flips green; otherwise it stays red. A +# non-collaborator commenting /qa-recheck cannot manufacture a green +# gate. Original (v1.2) design with `if:`-gating of this step was +# fail-open (skipped-via-`if:` job still publishes the status as +# `success`) — corrected in v1.3 per hongming-pc review 1421. # # Trust boundary (RFC A4): # This script is loaded from the BASE branch (sourced via .gitea/scripts/ @@ -56,19 +63,16 @@ set -euo pipefail -# jq is required for JSON parsing — same install dance sop-tier-check uses. +# jq is required for JSON parsing. It is pre-baked into the runner-base +# image (per RFC#268 workflow-smoke), so the only reason we'd not find it +# is a broken runner. The previous fallback dance (apt-get + curl to +# /usr/local/bin/jq) cannot succeed on a uid-1001 rootless runner +# (#391/#402 + feedback_ci_runner_install_needs_writable_path), so it's +# dropped. Fail loud with a clear diagnostic rather than attempt an +# install that physically cannot work. if ! command -v jq >/dev/null 2>&1; then - echo "::notice::jq not on PATH — installing..." - if apt-get update -qq && apt-get install -y -qq jq 2>/dev/null; then - echo "::notice::jq installed via apt-get: $(jq --version)" - elif timeout 60 curl -sSL \ - "https://github.com/jqlang/jq/releases/download/jq-1.7.1/jq-linux-amd64" \ - -o /usr/local/bin/jq && chmod +x /usr/local/bin/jq; then - echo "::notice::jq downloaded as binary: $(/usr/local/bin/jq --version)" - else - echo "::error::jq install failed; cannot parse Gitea API JSON" - exit 1 - fi + echo "::error::jq missing from runner-base image — bake it into the runner image (see RFC#268 workflow-smoke / feedback_ci_runner_install_needs_writable_path). This evaluator cannot run without jq." + exit 1 fi : "${GITEA_TOKEN:?GITEA_TOKEN required}" diff --git a/.gitea/workflows/qa-review.yml b/.gitea/workflows/qa-review.yml index 18919a2d..3bc343b9 100644 --- a/.gitea/workflows/qa-review.yml +++ b/.gitea/workflows/qa-review.yml @@ -18,14 +18,30 @@ # `qa-review / approved ()` — NO `POST /statuses` call → NO # write:repository token scope needed. Sidesteps internal#321 defect #2. # -# A1.1 (privilege check on slash-comment): +# A1.1 (privilege check on slash-comment — INFORMATIONAL ONLY, NOT a gate): # The `issue_comment` event fires for ANY commenter, including -# non-collaborators. We MUST NOT use `github.event.comment.author_association` -# (the field doesn't exist on Gitea 1.22.6 webhook payload — this was -# sop-tier-refire's defect #1). Instead, the Privilege check step probes -# GET /repos/.../collaborators/{login}: -# - 204 → is-collaborator → proceed -# - 404 → not-collaborator → exit 0 (ignored; status unchanged) +# non-collaborators. The original (v1.2) design gated the eval step +# behind a collaborator probe → if a non-collaborator commented +# /qa-recheck, the eval was `if:`-skipped → the job exited 0 anyway → +# the status context published `success` with ZERO real APPROVE. +# That was a fail-open: any visitor could green the gate. +# +# RFC#324 v1.3 §A1.1 correction (option b per hongming-pc 1421): +# drop privilege-gating of the evaluation entirely. The eval is +# read-only and idempotent — it reads `pulls/{N}/reviews` and +# `teams/{id}/members/{u}` (both API-side state that a commenter can't +# change). Re-running it on a non-collaborator's comment is harmless +# AND correct: if a real team-member APPROVE exists, the eval flips +# green; if not, it stays red. +# +# We KEEP the privilege step as a `::notice::` log line only — useful +# for griefer-spotting (one operator spamming /recheck) without +# touching the gate. If rate-limiting is needed later, add it as a +# separate concern (time-window throttle, not a privilege gate). +# +# We MUST NOT use `github.event.comment.author_association` (the +# field doesn't exist on Gitea 1.22.6 webhook payload — this was +# sop-tier-refire's defect #1). # # A4 (no PR-head checkout under pull_request_target): # We check out the BASE ref explicitly so the review-check.sh script is @@ -60,7 +76,9 @@ # # /qa-recheck — re-evaluate the gate (e.g. after an APPROVE lands) # -# Restricted to repo collaborators only (privilege-check step A1.1). +# Open to any PR commenter. The eval is read-only and idempotent, so +# unprivileged refires are harmless (RFC#324 v1.3 §A1.1). Collaborator +# status is logged for griefer-spotting but does NOT gate execution. name: qa-review @@ -79,12 +97,10 @@ jobs: # Gate the job: # - On pull_request_target events: always run. # - On issue_comment events: only when it's a PR comment and the body - # contains the slash-command. The privilege check is enforced as - # a job step (A1.1), not here — `if:` can't call the collaborator - # API. The privilege step exits the job early for non-collaborators - # while still publishing the job's previous status (i.e. it does - # NOT flip qa-review from failure to success without a real - # team-member APPROVE). + # contains the slash-command. NO privilege gate at the step level + # (RFC#324 v1.3 §A1.1): a non-collaborator's /qa-recheck is fine + # because the eval is read-only and idempotent — re-running it + # just re-confirms whether a real team-member APPROVE exists. if: | github.event_name == 'pull_request_target' || (github.event_name == 'issue_comment' && @@ -92,10 +108,15 @@ jobs: startsWith(github.event.comment.body, '/qa-recheck')) runs-on: ubuntu-latest steps: - - name: Privilege check (A1.1 — collaborator API, NOT author_association) - # Only runs on issue_comment events. For pull_request_target the - # commenter doesn't exist, so the step is a no-op skip. - id: priv + - name: Privilege check (A1.1 — INFORMATIONAL log only, NOT a gate) + # RFC#324 v1.3 §A1.1: this step does NOT gate subsequent steps. + # It exists solely as a log line for griefer-spotting (one + # operator spamming /qa-recheck without merit). Re-running the + # read-only eval on a non-collaborator comment is harmless; + # gating it would be fail-open (skipped steps still publish + # `success` for the job's status context). + # Only runs on issue_comment events; pull_request_target has + # no comment.user.login so the step is a no-op skip there. if: github.event_name == 'issue_comment' env: GITEA_TOKEN: ${{ secrets.RFC_324_TEAM_READ_TOKEN || secrets.GITHUB_TOKEN }} @@ -106,11 +127,9 @@ jobs: -H "Authorization: token ${GITEA_TOKEN}" \ "${{ github.server_url }}/api/v1/repos/${{ github.repository }}/collaborators/${login}") if [ "$code" = "204" ]; then - echo "::notice::commenter ${login} is a collaborator — proceeding" - echo "proceed=true" >> "$GITHUB_OUTPUT" + echo "::notice::Recheck from ${login} (collaborator=true)" else - echo "::notice::comment from non-collaborator ${login} (HTTP ${code}) ignored" - echo "proceed=false" >> "$GITHUB_OUTPUT" + echo "::notice::Recheck from ${login} (collaborator=false, HTTP ${code}) — proceeding with read-only eval anyway" fi - name: Check out BASE ref (A4 — never PR-head) @@ -120,17 +139,11 @@ jobs: # script source is always the default-branch version. # NEVER use ref: ${{ github.event.pull_request.head.sha }} — # that would execute PR-head code with secrets-context. - if: | - github.event_name == 'pull_request_target' || - (github.event_name == 'issue_comment' && steps.priv.outputs.proceed == 'true') uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: ref: ${{ github.event.repository.default_branch }} - name: Evaluate qa-review - if: | - github.event_name == 'pull_request_target' || - (github.event_name == 'issue_comment' && steps.priv.outputs.proceed == 'true') env: GITEA_TOKEN: ${{ secrets.RFC_324_TEAM_READ_TOKEN || secrets.GITHUB_TOKEN }} GITEA_HOST: git.moleculesai.app diff --git a/.gitea/workflows/security-review.yml b/.gitea/workflows/security-review.yml index c7b4d3fb..2243d9ca 100644 --- a/.gitea/workflows/security-review.yml +++ b/.gitea/workflows/security-review.yml @@ -21,6 +21,8 @@ permissions: jobs: approved: + # See qa-review.yml header for full A1-α / A1.1 (v1.3 — informational + # log only, NOT a gate) / A4 / A5 design rationale. if: | github.event_name == 'pull_request_target' || (github.event_name == 'issue_comment' && @@ -28,8 +30,10 @@ jobs: startsWith(github.event.comment.body, '/security-recheck')) runs-on: ubuntu-latest steps: - - name: Privilege check (A1.1 — collaborator API, NOT author_association) - id: priv + - name: Privilege check (A1.1 — INFORMATIONAL log only, NOT a gate) + # RFC#324 v1.3 §A1.1: does NOT gate subsequent steps. See + # qa-review.yml for full rationale. Eval is read-only/idempotent + # so re-running on a non-collaborator comment is harmless. if: github.event_name == 'issue_comment' env: GITEA_TOKEN: ${{ secrets.RFC_324_TEAM_READ_TOKEN || secrets.GITHUB_TOKEN }} @@ -40,25 +44,17 @@ jobs: -H "Authorization: token ${GITEA_TOKEN}" \ "${{ github.server_url }}/api/v1/repos/${{ github.repository }}/collaborators/${login}") if [ "$code" = "204" ]; then - echo "::notice::commenter ${login} is a collaborator — proceeding" - echo "proceed=true" >> "$GITHUB_OUTPUT" + echo "::notice::Recheck from ${login} (collaborator=true)" else - echo "::notice::comment from non-collaborator ${login} (HTTP ${code}) ignored" - echo "proceed=false" >> "$GITHUB_OUTPUT" + echo "::notice::Recheck from ${login} (collaborator=false, HTTP ${code}) — proceeding with read-only eval anyway" fi - name: Check out BASE ref (A4 — never PR-head) - if: | - github.event_name == 'pull_request_target' || - (github.event_name == 'issue_comment' && steps.priv.outputs.proceed == 'true') uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: ref: ${{ github.event.repository.default_branch }} - name: Evaluate security-review - if: | - github.event_name == 'pull_request_target' || - (github.event_name == 'issue_comment' && steps.priv.outputs.proceed == 'true') env: GITEA_TOKEN: ${{ secrets.RFC_324_TEAM_READ_TOKEN || secrets.GITHUB_TOKEN }} GITEA_HOST: git.moleculesai.app