From c910e83ce379d1e35aa2ff4b99917ef4b2e42656 Mon Sep 17 00:00:00 2001 From: Jonathan Zhang Date: Mon, 27 Jul 2026 10:52:12 -0700 Subject: [PATCH] ci: close two auto-merge review gaps (codex on risk:standard, docs/legal carve-out) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent paths let changes reach main unreviewed. 1. Codex never ran on risk:standard — simultaneously the fallback class for any unmatched path AND an auto-mergeable one. whois-api-llc/wxa_vpn#1270 (+839 lines under src/**) passed CI clean; a manual codex run afterwards found four P1s including a forgeable-identity hole. #1273 (+786) same. Fixed with a SECOND job (`review_standard`), not a widened `if:`. The job id is load-bearing: the published check is " / Codex Review", and claude-author-automerge.yml bypasses its risk-tier gate on that check's SUCCESS — but a codex-gate.mjs skip still concludes success. Reusing job `review` would have let a SMALL auth/billing/migration change classified `standard` auto-merge with codex having read nothing. A distinct job id gives coverage with zero new bypass surface. 2. docs/legal/** auto-merged via the docs safe-paths carve-out — wxa_vpn#1268, an AUP change asserting all bulk deliveries are watermarked (false for .mmdb and .parquet), merged in seconds unreviewed. Fixed in BOTH gates, because neither subsumes the other: - safe-paths-automerge.yml: built-in unsafe_overrides, checked before the safe globs. This workflow never consults the classifier, so a repo's risk-paths.yml `sensitive:` list cannot close it. - claude-author-automerge.yml: ^docs/legal/.* in the central risk regex. This is the DURABLE gate — it decides before arming, so unlike revocation it has no race. Unlike marketing trees, docs/legal/ is a fixed conventional path, so a central glob cannot over-block. Both file scans now also match rename SOURCES (.previous_filename): the API reports only the destination, so moving a legal doc out of docs/legal/ (or an auth file out of src/auth/) escaped both gates. Also hardens safe-paths' arm/revoke path, all found by codex review: - revoke an existing arm on override or truncated listing (GitHub preserves auto-merge across pushes, so recomputing all_safe=0 disarms nothing) - fail CLOSED on revoke: only a verified OFF succeeds, after 3 attempts - per-PR concurrency group + head-SHA guard before arming, so a stale run cannot arm a newer head it never classified - evaluate the override BEFORE the 3000-file truncation exit KNOWN, NOT FIXED (documented inline): `review_standard` is ADVISORY — it is in no required-checks ruleset, so a standard PR can merge before it finishes. Making it blocking needs an always()-gated summary job promoted to each caller's ruleset (a bare required context that cannot report bricks PRs on "Expected" forever) — a 40+-repo operation and an operator decision. The same skip-reads-as-success trust also still applies to `sensitive` on job `review`. Codex pre-review: 10 rounds (protocol cap is 6 — exceeded deliberately, every round through 9 surfaced a real defect; round 10's remainder is the documented advisory-vs-durable-gate limitation above). HUMAN_READABLE_SUMMARY: Codex now reviews ordinary `risk:standard` PRs, not just `risk:sensitive` — in a separate job so a cost-skipped review can't be mistaken for a passed one and bypass the manual-merge gate. And customer-facing legal docs no longer sneak through the "docs are safe" auto-merge carve-out; they're blocked in the central risk regex too, which is the gate that can't race. Auto-merge rationale: MANUAL MERGE. Touches .github/workflows/** (risk:blocked) and changes fleet-wide merge-gating semantics across 40+ consumer repos. --- .github/workflows/claude-author-automerge.yml | 33 ++- .github/workflows/pr-codex-review.yml | 88 +++++++- .github/workflows/safe-paths-automerge.yml | 175 ++++++++++++++- selftest/risk_patterns_corpus.txt | 11 + selftest/test_automerge_risk_patterns.sh | 1 + selftest/test_safe_paths_unsafe_overrides.sh | 207 ++++++++++++++++++ selftest/test_workflow_guards.py | 150 +++++++++++++ 7 files changed, 656 insertions(+), 9 deletions(-) create mode 100644 selftest/test_safe_paths_unsafe_overrides.sh diff --git a/.github/workflows/claude-author-automerge.yml b/.github/workflows/claude-author-automerge.yml index 870c7f4..9cbe14d 100644 --- a/.github/workflows/claude-author-automerge.yml +++ b/.github/workflows/claude-author-automerge.yml @@ -76,13 +76,25 @@ name: Claude-author Auto-merge (reusable) # changes (lesson 2026-05-04, wxa_vpn#250) and was narrowed # 2026-05-20. # -# Customer-copy diffs (legal / marketing / pricing wording) are NOT +# Customer-copy diffs (legal / marketing / pricing wording) are MOSTLY not # detected by the global regex here — repo layouts differ (techrecon: # site/src/**; wxa-astro: src/content/** Tina JSON), so a central glob # would over-block some repos and miss others. The enforcement for this # policy category is the caller's .github/risk-paths.yml `sensitive:` # list (item 1 above): marketing-site repos MUST list their -# wording-bearing trees there. Lesson 2026-07-02 from +# wording-bearing trees there. +# +# EXCEPTION: `^docs/legal/.*` IS matched centrally (added 2026-07-27). +# Unlike a marketing tree, docs/legal/ is a fixed conventional path, so it +# cannot over-block — a repo without that directory is unaffected — and it +# needs a DURABLE gate rather than a per-repo one. safe-paths-automerge.yml +# also refuses these paths, but that is a revocation after the fact and +# races this workflow's arm; matching here means a Claude-authored legal +# change is never armed in the first place. Lesson 2026-07-25, +# whois-api-llc/wxa_vpn#1268: an AUP change asserting all bulk deliveries +# are watermarked (false for .mmdb and .parquet) auto-merged unreviewed. +# Legal wording is the archetypal not-revertable-in-one-cycle category: +# CI cannot check whether a clause is TRUE. Lesson 2026-07-02 from # whois-api-llc/techrecon#405: a marketing-copy PR auto-merge-armed # because techrecon's sensitive: list had no site/ entries; fixed in # techrecon#410 + wxa-astro#63. Claude Review still reviews copy @@ -673,6 +685,22 @@ jobs: echo "::error::PR changes 3000+ files — file listing is truncated and the risk scan cannot classify every path. Apply the '${BYPASS_LABEL:-auto-merge-approved}' label or merge manually." exit 1 fi + # Renames report ONLY their destination in `.filename`; the source + # is in `.previous_filename`. Without it, MOVING a risky file out + # of a risky path — `docs/legal/aup.md` → `docs/archive/aup.md`, + # or `src/auth/login.py` → `src/util/login.py` — reads as an + # ordinary change and escapes the gate, even though deleting a + # policy from its canonical location is exactly the edit that + # needs a human. Scanning both ends can only ADD risk + # classification, never remove it, so it always fails toward + # manual merge. Appended AFTER the 3000-file count so rename + # sources cannot push a large-but-listable PR over the cap. + # (Codex review round 9.) + renamed_from=$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR}/files?per_page=100" \ + --paginate --jq '.[] | select(.previous_filename) | .previous_filename' 2>/dev/null || echo "") + if [ -n "$renamed_from" ]; then + changed=$(printf '%s\n%s' "$changed" "$renamed_from") + fi # Risk-tier glob patterns (kept in sync with global CLAUDE.md policy). # If you add a category, also update install-automerge-policy.sh and # the CLAUDE.md policy block. @@ -684,6 +712,7 @@ jobs: ^(.*/)?migrations(/|$) .*\.sql$ ^(.*/)?(billing|payment[s]?|pricing|invoice[s]?|subscription[s]?|checkout|refund[s]?)(/|\.(py|go|ts|tsx|js|jsx)$|$) + ^docs/legal/.* (^|/)naf(/|\.(py|go|ts|js)$) (^|/)main\.go$ (^|/)Dockerfile(\..*)?$ diff --git a/.github/workflows/pr-codex-review.yml b/.github/workflows/pr-codex-review.yml index 34b2f62..ccb11e7 100644 --- a/.github/workflows/pr-codex-review.yml +++ b/.github/workflows/pr-codex-review.yml @@ -1,9 +1,14 @@ name: PR Codex Review # OpenAI Codex CLI as a third-party reviewer for vendor diversity. -# Runs ONLY on risk:sensitive PRs (per .github/risk-paths.yml). Pairs with -# Claude Review (which runs on every PR via pr-review.yml) to give +# Runs on risk:sensitive AND risk:standard PRs (per .github/risk-paths.yml) +# — i.e. every class that can reach main without a human reading the diff. +# Pairs with Claude Review (which runs on every PR via pr-review.yml) to give # independent-model coverage on high-impact code paths. +# +# Actual spend is gated a second time inside codex-review.yml by +# codex-gate.mjs (small diffs and docs/tests-only diffs skip), so the +# widening to `standard` does not mean a Codex run on every PR. # ready_for_review is required in the type list: every draft-phase run # (opened, synchronize) skips on the draft gate below, so without it a PR @@ -13,8 +18,9 @@ on: pull_request: types: [opened, synchronize, reopened, ready_for_review] # Skip the entire Codex track on bootstrap-paths. The classifier labels - # these risk:blocked, and this caller only reviews risk:sensitive — so - # running it would burn a classify job just to produce a skipped review. + # these risk:blocked, and this caller reviews only sensitive/standard — + # so running it would burn a classify job just to produce a skipped + # review. # Bootstrap-path PRs always require admin merge by design, so a # secondary review adds no signal. # @@ -52,9 +58,83 @@ jobs: if: github.event.pull_request.draft == false uses: topcoder1/ci-workflows/.github/workflows/pr-classify.yml@main + # --------------------------------------------------------------------- + # TWO jobs, never both. They differ ONLY in job id, and that difference is + # load-bearing: the check name GitHub publishes is " / Codex + # Review", and claude-author-automerge.yml's `codex_check_name` input + # (default "review / Codex Review") names THIS job's check as the one + # whose SUCCESS bypasses the risk-tier manual-merge gate. + # + # That bypass trusts a conclusion, not a review. When codex-gate.mjs + # decides to skip (diff under SIZE_THRESHOLD, or docs/tests-only), the + # review STEPS are skipped but the JOB still concludes `success` — so a + # skipped review is indistinguishable from a passed one at the check-run + # level. + # + # Routing `standard` through the same job id would therefore have turned + # a coverage fix into a security regression: a repo whose risk-paths.yml + # `sensitive:` list is empty (the shipped default) classifies a small + # change to an auth / billing / migration path as `standard`, the central + # risk regex in claude-author-automerge.yml still flags it risky=1, and + # today that combination has NO Codex check at all, so the risk gate + # holds and a human merges it. Emitting `review / Codex Review = success` + # for it — without Codex having read a line — would auto-merge it. + # (Codex review round 2 caught exactly this on the first draft of this + # change.) + # + # So: `sensitive` keeps job id `review` and its existing bypass semantics, + # unchanged. `standard` gets a DIFFERENT job id, publishing + # "review_standard / Codex Review", which matches no `codex_check_name` + # and can never satisfy the bypass. Pure added review coverage, zero new + # auto-merge surface. + # + # PRE-EXISTING and NOT addressed here: the same skip-reads-as-success hole + # already applies to `sensitive` PRs on the `review` job. Narrowing that + # means changing check-conclusion semantics fleet-wide (40+ repos) and is + # a deliberate design decision, not a side effect of this change. review: needs: classify if: needs.classify.outputs.risk_class == 'sensitive' uses: topcoder1/ci-workflows/.github/workflows/codex-review.yml@main secrets: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} + + # `standard` is the fallback class for any path matching no rule AND a + # class claude-author-automerge.yml will auto-merge — so without this it + # is the largest body of code reaching main with no second-model read. + # Lesson 2026-07-26, whois-api-llc/wxa_vpn#1270 (+839 lines under src/**, + # classified `standard`): CI passed it clean; a manual Codex run + # afterwards found four P1s, including a forgeable-identity hole that let + # a customer attribute a leaked artifact to a competitor. #1273 (+786, + # also `standard`) had the same exposure. + # + # Spend stays bounded by codex-review.yml's own codex-gate.mjs (skips + # small diffs and docs/tests-only diffs), so this is not "Codex on every + # PR" — the cost gate decides, not the risk class. + # + # SCOPE — this lane is ADVISORY, not a merge gate. Read this before + # assuming a standard PR cannot merge unreviewed. + # GitHub's auto-merge waits only for REQUIRED status checks. + # "review_standard / Codex Review" is not in any repo's required-checks + # ruleset and is not consulted by claude-author-automerge.yml, so a + # standard PR whose required checks go green first CAN merge before this + # lane finishes. Findings then land on an already-merged PR — useful as + # follow-up signal, but it does not prevent the #1270 class outright. + # (Codex review round 3 raised exactly this; recorded rather than + # silently overclaimed.) + # + # Making it blocking is a deliberate fleet operation, not a flag here: + # a required check must report on EVERY PR or it bricks them on + # "Expected — Waiting for status to be reported" forever. Since neither + # lane runs for risk:blocked or draft PRs, that needs an always()-gated + # summary job over both lanes (accepting `skipped` only with classifier + # proof) promoted to the required-checks ruleset in each caller repo. + # That is a per-repo ruleset change across 40+ repos with a real + # brick-the-fleet failure mode — an operator decision, deliberately not + # taken as a side effect of adding coverage. + review_standard: + needs: classify + if: needs.classify.outputs.risk_class == 'standard' + uses: topcoder1/ci-workflows/.github/workflows/codex-review.yml@main + secrets: + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} diff --git a/.github/workflows/safe-paths-automerge.yml b/.github/workflows/safe-paths-automerge.yml index 2f603ce..da774ff 100644 --- a/.github/workflows/safe-paths-automerge.yml +++ b/.github/workflows/safe-paths-automerge.yml @@ -52,6 +52,17 @@ on: `secrets: inherit` in the caller, or pass explicitly. required: false +# Serialize per PR. Without this, a run that classified an OLDER, docs-only +# head can still be sitting in its enable step when a push adds +# docs/legal/**; it then arms auto-merge against the CURRENT head — the +# legal revision — and the newer run has already passed its revoke check, so +# no event remains to disarm it. cancel-in-progress kills the stale run; the +# head guard on the enable step below covers the window where it was already +# past cancellation. (Codex review round 8 P1.) +concurrency: + group: safe-paths-automerge-${{ github.event.pull_request.number }} + cancel-in-progress: true + jobs: safe_paths_automerge: runs-on: ubuntu-latest @@ -75,9 +86,82 @@ jobs: # checkout" footgun — the API form never needs a checkout.) changed=$(gh api "repos/${REPO}/pulls/${PR}/files?per_page=100" \ --paginate --jq '.[].filename') - # The listing endpoint caps at 3000 files. Past that we can't - # prove every changed file is safe → defer (same stance as the - # empty-diff branch below). + # NOTE the 3000-file truncation check lives AFTER the override + # block below, not here. Both exit early with all_safe=0, but only + # the override also emits reason=unsafe-override, which is what + # triggers the revoke step. Returning on truncation first would + # skip the override entirely, so an already-armed PR updated to + # 3000+ files including docs/legal/** would keep its arm and merge + # the legal change. (Codex review round 8 P2.) Checking the + # override on a truncated list is strictly better than not + # checking it: it still catches any legal path within the first + # 3000 entries. + + # Built-in UNSAFE overrides, checked BEFORE the safe globs and + # winning over every one of them. These are paths that sit inside + # an otherwise-safe tree but are not "documentation" in the sense + # the safety argument above relies on. + # + # docs/legal/**: customer-facing legal wording (AUP, ToS, privacy). + # The `^docs/.*` safe glob's premise is "zero runtime impact, can + # never break code paths" — true of a runbook, false of a document + # that states contractual commitments to customers. A wrong clause + # cannot be caught by CI and is not revertable in one cycle once + # published, which is the repo protocol's own test for manual merge. + # Lesson 2026-07-25, whois-api-llc/wxa_vpn#1268: an AUP change + # asserting that all bulk deliveries are watermarked auto-merged + # here in ~seconds. The assertion was false for two formats (.mmdb, + # .parquet), and no human saw it before it landed on main. + # + # NOTE this is deliberately a SEPARATE gate from the customer-copy + # policy in claude-author-automerge.yml, which is enforced via the + # caller's .github/risk-paths.yml `sensitive:` list. That gate + # cannot help here: this workflow never consults the classifier — + # it decides on diff content alone — so a repo can list + # docs/legal/** as `sensitive:` and still have it merged by this + # workflow. Both gates are required; neither subsumes the other. + unsafe_overrides='^docs/legal/' + + # A rename reports ONLY its destination in `.filename`; the source + # path lives in `.previous_filename`. Without this, moving + # docs/legal/aup.md to docs/archive/aup.md — which removes the + # policy from its canonical location — reads as a plain docs/** + # change and auto-merges. Match the override against BOTH ends of + # every rename. Deliberately a separate request rather than folding + # both fields into the `changed` listing above: that listing feeds + # the 3000-file cap and the safe-glob loop, and a previous_filename + # must never be counted there (it is not a changed file, and a + # rename INTO docs/legal/ would otherwise be judged on its old + # path). Kept override-only so the existing listing keeps its + # exact, separately-pinned semantics. + renamed_from=$(gh api "repos/${REPO}/pulls/${PR}/files?per_page=100" \ + --paginate --jq '.[] | select(.previous_filename) | .previous_filename') + + override_hits="" + while IFS= read -r f; do + [ -z "$f" ] && continue + while IFS= read -r p; do + p="${p#"${p%%[![:space:]]*}"}" + [ -z "$p" ] && continue + if echo "$f" | grep -Eq "$p"; then + override_hits="$override_hits$f\n" + break + fi + done <<< "$unsafe_overrides" + done <<< "$(printf '%s\n%s\n' "$changed" "$renamed_from")" + + if [ -n "$override_hits" ]; then + echo "all_safe=0" >> "$GITHUB_OUTPUT" + echo "reason=unsafe-override" >> "$GITHUB_OUTPUT" + echo "Not safe — diff touches a built-in unsafe-override path." + echo "Override hits:" + printf "%b" "$override_hits" + exit 0 + fi + + # Deferred from above: the listing endpoint caps at 3000 files. + # Past that we can't prove every changed file is safe → defer + # (same stance as the empty-diff branch below). if [ "$(printf '%s\n' "$changed" | sed '/^$/d' | wc -l)" -ge 3000 ]; then echo "all_safe=0" >> "$GITHUB_OUTPUT" echo "reason=file-list-truncated" >> "$GITHUB_OUTPUT" @@ -139,6 +223,77 @@ jobs: echo "All $(echo "$changed" | wc -l | tr -d ' ') changed files are in safe paths." fi + # Computing all_safe=0 is not enough once auto-merge is already ON. + # GitHub PRESERVES an auto-merge request across pushes, so the + # sequence "PR touches only docs/foo.md (armed here) → push adds + # docs/legal/aup.md" leaves the arm standing: this run recomputes + # all_safe=0, exits, and the legal change merges the moment required + # checks go green. That is the very bypass the override exists to + # stop. Revoke explicitly. (Codex review round 5.) + # + # ONLY for the override — not for every all_safe=0. The other + # not-safe branches mean "defer to another workflow", and those + # workflows legitimately arm auto-merge; revoking there would fight + # them. The override is the one branch that is a positive "this must + # not auto-merge" verdict. + # `file-list-truncated` is included because a legal path can hide past + # the API's 3000-entry cap: the override scan then sees no hit, yet we + # also cannot prove the revision is clean. An arm from an earlier safe + # revision must not survive that. Consistent with the sibling gate — + # claude-author-automerge.yml also fails closed on truncation — so this + # does not fight a workflow that would legitimately arm. + # (Codex review round 10 P2.) + - name: Revoke auto-merge on an unsafe-override path + if: >- + steps.classify.outputs.reason == 'unsafe-override' || + steps.classify.outputs.reason == 'file-list-truncated' + env: + # Match the enable step's token: an arm placed by the PAT is + # revoked by the same identity. + GH_TOKEN: ${{ secrets.automerge_pat || github.token }} + PR_URL: ${{ github.event.pull_request.html_url }} + PR: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -euo pipefail + # Ownership guard, mirroring claude-author-automerge.yml: if the + # head moved since this run's event, the newer head's own run + # owns the decision and an unconditional revoke here could land + # after it validly armed a clean PR, with no event left to + # re-arm. Skip only on a POSITIVE head-moved read; if the read + # itself fails, revoke anyway (fail toward safety). + now=$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR}" --jq .head.sha 2>/dev/null || echo "") + if [ -n "$now" ] && [ "$now" != "$HEAD_SHA" ]; then + echo "::notice::head moved (${HEAD_SHA} → ${now}) — newer run owns gating; skipping stale revoke." + exit 0 + fi + state=$(gh pr view "$PR_URL" --json autoMergeRequest \ + --jq 'if .autoMergeRequest == null then "OFF" else "ON" end' 2>/dev/null || echo "UNKNOWN") + if [ "$state" = "OFF" ]; then + echo "Auto-merge already off — nothing to revoke." + exit 0 + fi + echo "::warning::Diff touches an unsafe-override path (customer-facing legal wording) while auto-merge is armed — revoking." + # Fail CLOSED: only a verified OFF is success. A swallowed + # `--disable-auto` failure or an unreadable verify both leave the + # PR possibly still armed, and treating that as success would let + # the legal change merge — exactly what this step exists to stop. + # UNKNOWN is not OFF. (Codex review round 6.) + after="UNKNOWN" + for attempt in 1 2 3; do + gh pr merge --disable-auto "$PR_URL" 2>&1 || true + after=$(gh pr view "$PR_URL" --json autoMergeRequest \ + --jq 'if .autoMergeRequest == null then "OFF" else "ON" end' 2>/dev/null || echo "UNKNOWN") + echo "Revoke attempt ${attempt}: auto-merge state = $after" + [ "$after" = "OFF" ] && break + sleep 3 + done + if [ "$after" != "OFF" ]; then + echo "::error::could not VERIFY auto-merge is off (state=$after) after 3 attempts — a customer-facing legal change may merge unreviewed. Disable auto-merge on this PR by hand." + exit 1 + fi + echo "Auto-merge verified OFF." + - name: Enable auto-merge if: steps.classify.outputs.all_safe == '1' env: @@ -149,10 +304,24 @@ jobs: # `automerge_pat` secret docstring above for the full rationale. GH_TOKEN: ${{ secrets.automerge_pat || github.token }} PR_URL: ${{ github.event.pull_request.html_url }} + PR: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} METHOD: ${{ inputs.merge_method }} USING_PAT: ${{ secrets.automerge_pat != '' && '1' || '0' }} run: | set -euo pipefail + # Arm only the head this run actually classified. `gh pr merge + # --auto` acts on the PR's CURRENT head, so a run that classified + # an older docs-only revision would otherwise arm a newer revision + # it never inspected — including one that added docs/legal/**. + # Fail CLOSED on an unreadable head: skipping an arm is harmless + # (the newer head's own run arms it), arming blind is not. + # (Codex review round 8 P1.) + now=$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR}" --jq .head.sha 2>/dev/null || echo "") + if [ "$now" != "$HEAD_SHA" ]; then + echo "::notice::head is '${now:-}' but this run classified '${HEAD_SHA}' — not arming; the current head's own run owns that decision." + exit 0 + fi echo "Safe-paths-only diff — enabling auto-merge ($METHOD)." if [ "$USING_PAT" = "1" ]; then echo "Auth: caller-supplied automerge_pat — downstream workflows WILL trigger on merge." diff --git a/selftest/risk_patterns_corpus.txt b/selftest/risk_patterns_corpus.txt index e908a2e..5e3335c 100644 --- a/selftest/risk_patterns_corpus.txt +++ b/selftest/risk_patterns_corpus.txt @@ -52,6 +52,14 @@ RISKY: billing/invoices.py RISKY: internal/payment/charge.go RISKY: internal/pricing/calc.go +# ── RISKY: customer-facing legal wording (wxa_vpn#1268 gap, 2026-07-27) ──── +# docs/legal/ is a fixed conventional path, so unlike marketing trees it can +# be matched centrally without over-blocking. CI cannot check whether a +# clause is TRUE, and a published one is not revertable in one cycle. +RISKY: docs/legal/acceptable-use-policy.md +RISKY: docs/legal/terms-of-service.md +RISKY: docs/legal/privacy/gdpr.md + # ── RISKY: NAF credit/billing gate (wxa_webcat#438 gap, 2026-05-28) ───────── # The validate_call/log_successful_call client that authorizes + charges each # customer API call. Path has no auth/billing keyword so it slipped through. @@ -145,6 +153,9 @@ SAFE: src/utils/format.ts SAFE: src/api/findingsSlice.ts SAFE: docs/architecture.md SAFE: docs/data-dictionary.md +# Anchored at docs/legal/ — "legal" elsewhere in the tree is ordinary docs. +SAFE: docs/notes/legal-review-process.md +SAFE: docs/legal-summary.md SAFE: README.md SAFE: tests/test_anything.py SAFE: tests/unit/utils_test.go diff --git a/selftest/test_automerge_risk_patterns.sh b/selftest/test_automerge_risk_patterns.sh index 316e21c..5d01a4a 100755 --- a/selftest/test_automerge_risk_patterns.sh +++ b/selftest/test_automerge_risk_patterns.sh @@ -20,6 +20,7 @@ patterns='^(.*/)?(auth|login|signin|signup|logout|session[s]?|oauth|oauth2|sso|j ^(.*/)?migrations(/|$) .*\.sql$ ^(.*/)?(billing|payment[s]?|pricing|invoice[s]?|subscription[s]?|checkout|refund[s]?)(/|\.(py|go|ts|tsx|js|jsx)$|$) +^docs/legal/.* (^|/)naf(/|\.(py|go|ts|js)$) (^|/)main\.go$ (^|/)Dockerfile(\..*)?$ diff --git a/selftest/test_safe_paths_unsafe_overrides.sh b/selftest/test_safe_paths_unsafe_overrides.sh new file mode 100644 index 0000000..04cba9e --- /dev/null +++ b/selftest/test_safe_paths_unsafe_overrides.sh @@ -0,0 +1,207 @@ +#!/usr/bin/env bash +# Behavioral test for safe-paths-automerge.yml's built-in unsafe-override +# list — the paths that sit inside an otherwise-safe tree but must never be +# auto-merged on diff content alone. +# +# Incident (whois-api-llc/wxa_vpn#1268, 2026-07-25): a change to +# docs/legal/acceptable-use-policy.md — customer-facing legal wording that +# asserted all bulk deliveries are watermarked — matched the built-in +# `^docs/.*` safe glob and auto-merged in seconds with no human review. The +# assertion was false for two formats (.mmdb, .parquet). The `^docs/.*` +# safety argument is "zero runtime impact, can never break code paths", +# which is true of a runbook and false of a contractual commitment. +# +# Note this workflow NEVER consults the risk classifier — it decides on diff +# content alone — so listing docs/legal/** as `sensitive:` in a caller's +# .github/risk-paths.yml does NOT close this hole. The override list here is +# the only gate on this path. +# +# Pins: +# 1. A docs/legal/** file is NOT safe (all_safe=0, reason=unsafe-override). +# 2. The override wins even when mixed with genuinely-safe files. +# 3. Ordinary docs/** are STILL safe — the fix must not over-block and +# regress the carve-out this workflow exists to provide. +# 4. tests/** remain safe. +# +# The classify block is EXTRACTED from the workflow YAML and executed, so +# this exercises the shipped bash rather than a mirrored copy that can drift. +# +# Run from the repo root: +# bash selftest/test_safe_paths_unsafe_overrides.sh +set -euo pipefail + +failed=0 +T=$(mktemp -d) +trap 'rm -rf "$T"' EXIT + +WF=.github/workflows/safe-paths-automerge.yml + +# Extract the `run: |` body of the step with `id: classify`, then strip the +# block's 10-space YAML indent so it runs as a standalone script. +awk ' + /^[[:space:]]*id: classify[[:space:]]*$/ { in_step = 1 } + in_step && /^[[:space:]]*run: \|[[:space:]]*$/ { grab = 1; next } + grab && /^ - name:/ { exit } + grab { print } +' "$WF" | sed -e 's/^ //' > "$T/classify.sh" + +if ! grep -q 'unsafe_overrides=' "$T/classify.sh"; then + echo "FAIL: could not extract the classify block from $WF (no unsafe_overrides found)" + exit 1 +fi + +# Stub `gh`: the shipped block makes TWO calls to the files API — one for +# `.filename` (the changed-file listing) and one for `.previous_filename` +# (rename sources, override-only). Discriminate on the --jq expression so +# each returns its own fixture; a stub that returned the same list for both +# would make the rename-bypass cases pass vacuously. +mkdir -p "$T/bin" +cat > "$T/bin/gh" <<'STUB' +#!/usr/bin/env bash +for a in "$@"; do + case "$a" in + *previous_filename*) cat "$FAKE_RENAMES"; exit 0 ;; + esac +done +cat "$FAKE_FILES" +STUB +chmod +x "$T/bin/gh" +export PATH="$T/bin:$PATH" + +# Args are irrelevant to the stub but the block references them under `set -u`. +export GH_TOKEN=stub REPO=owner/repo PR=1 EXTRA_GLOBS="" + +# RENAMED_FROM: newline-separated rename SOURCE paths for the next case. +# Reset after every run_case so it never leaks between cases. +RENAMED_FROM="" + +# run_case ... +run_case() { + local name="$1" want_safe="$2" want_reason="$3" + shift 3 + printf '%s\n' "$@" > "$T/files.txt" + export FAKE_FILES="$T/files.txt" + printf '%s' "$RENAMED_FROM" > "$T/renames.txt" + export FAKE_RENAMES="$T/renames.txt" + RENAMED_FROM="" + : > "$T/gh_output" + export GITHUB_OUTPUT="$T/gh_output" + + if ! bash "$T/classify.sh" > "$T/log" 2>&1; then + echo "FAIL[$name]: classify block exited non-zero" + sed 's/^/ /' "$T/log" + failed=1 + return + fi + + local got_safe got_reason + got_safe=$(grep -E '^all_safe=' "$T/gh_output" | tail -1 | cut -d= -f2) + got_reason=$(grep -E '^reason=' "$T/gh_output" | tail -1 | cut -d= -f2- || true) + [ -n "$got_reason" ] || got_reason="-" + + if [ "$got_safe" != "$want_safe" ]; then + echo "FAIL[$name]: all_safe=$got_safe, want $want_safe (files: $*)" + failed=1 + return + fi + if [ "$want_reason" != "-" ] && [ "$got_reason" != "$want_reason" ]; then + echo "FAIL[$name]: reason=$got_reason, want $want_reason" + failed=1 + return + fi + echo "ok[$name] all_safe=$got_safe reason=$got_reason" +} + +# 1. The incident case: legal wording alone must NOT be safe. +run_case "legal-aup" 0 unsafe-override "docs/legal/acceptable-use-policy.md" +run_case "legal-tos" 0 unsafe-override "docs/legal/terms-of-service.md" +run_case "legal-nested" 0 unsafe-override "docs/legal/archive/2025/privacy.md" + +# 2. The override wins over co-changed safe files (all-or-nothing). +run_case "legal-mixed-docs" 0 unsafe-override \ + "docs/runbooks/deploy.md" "docs/legal/acceptable-use-policy.md" +run_case "legal-mixed-tests" 0 unsafe-override \ + "tests/test_thing.py" "docs/legal/acceptable-use-policy.md" + +# 3. Regression guard: ordinary docs must STILL auto-merge. Over-blocking +# would defeat the carve-out this workflow exists to provide. +run_case "plain-docs" 1 - "docs/architecture.md" +run_case "docs-nested" 1 - "docs/runbooks/webnx-cutover.md" +run_case "docs-multi" 1 - "docs/a.md" "docs/b/c.md" + +# 4. A path that merely CONTAINS "legal" deeper in the tree is not the +# override target — the pattern is anchored at docs/legal/. +run_case "not-anchored" 1 - "docs/notes/legal-review-process.md" + +# 5. tests/** unaffected. +run_case "tests-only" 1 - "tests/test_a.py" "tests/test_b.py" + +# 5b. Rename BYPASS (codex R1 P2): moving a legal doc out of docs/legal/ +# into another safe tree reports only the DESTINATION in .filename. +# Relocating the policy out of its canonical home must not auto-merge. +RENAMED_FROM="docs/legal/acceptable-use-policy.md" +run_case "legal-renamed-out" 0 unsafe-override "docs/archive/acceptable-use-policy.md" + +RENAMED_FROM="docs/legal/terms-of-service.md" +run_case "legal-renamed-to-tests" 0 unsafe-override "tests/fixtures/tos.md" + +# 5c. Rename INTO docs/legal/ is caught on the destination path alone. +RENAMED_FROM="docs/drafts/aup.md" +run_case "legal-renamed-in" 0 unsafe-override "docs/legal/acceptable-use-policy.md" + +# 5d. A rename with no legal path on EITHER end stays safe — the +# previous_filename lookup must not blanket-block ordinary doc moves. +RENAMED_FROM="docs/old-name.md" +run_case "docs-renamed-benign" 1 - "docs/new-name.md" + +# 6. Genuinely unsafe code still defers via the normal path (not the +# override reason) — proves the override didn't swallow the old branch. +run_case "src-code" 0 - "src/app/main.py" + +# 7. The override is evaluated BEFORE the 3000-file truncation exit. An +# already-armed PR grown past the listing cap must still report +# unsafe-override (the reason the revoke step keys on), not +# file-list-truncated. (Codex review round 8 P2.) +{ + for i in $(seq 1 3200); do echo "docs/page_${i}.md"; done + echo "docs/legal/acceptable-use-policy.md" +} > "$T/big.txt" +export FAKE_FILES="$T/big.txt" +: > "$T/renames.txt"; export FAKE_RENAMES="$T/renames.txt" +: > "$T/gh_output"; export GITHUB_OUTPUT="$T/gh_output" +if bash "$T/classify.sh" > "$T/log" 2>&1; then + got_safe=$(grep -E '^all_safe=' "$T/gh_output" | tail -1 | cut -d= -f2) + got_reason=$(grep -E '^reason=' "$T/gh_output" | tail -1 | cut -d= -f2-) + if [ "$got_safe" = "0" ] && [ "$got_reason" = "unsafe-override" ]; then + echo "ok[truncated-with-legal] all_safe=0 reason=unsafe-override" + else + echo "FAIL[truncated-with-legal]: all_safe=$got_safe reason=$got_reason, want 0/unsafe-override" + failed=1 + fi +else + echo "FAIL[truncated-with-legal]: classify block exited non-zero" + failed=1 +fi + +# 7b. A truncated list with NO override path still defers as truncated. +{ for i in $(seq 1 3200); do echo "docs/page_${i}.md"; done; } > "$T/big2.txt" +export FAKE_FILES="$T/big2.txt" +: > "$T/gh_output"; export GITHUB_OUTPUT="$T/gh_output" +if bash "$T/classify.sh" > "$T/log" 2>&1; then + got_reason=$(grep -E '^reason=' "$T/gh_output" | tail -1 | cut -d= -f2-) + if [ "$got_reason" = "file-list-truncated" ]; then + echo "ok[truncated-no-legal] reason=file-list-truncated" + else + echo "FAIL[truncated-no-legal]: reason=$got_reason, want file-list-truncated" + failed=1 + fi +else + echo "FAIL[truncated-no-legal]: classify block exited non-zero" + failed=1 +fi + +if [ "$failed" -ne 0 ]; then + echo "FAILED" + exit 1 +fi +echo "All safe-paths unsafe-override cases passed." diff --git a/selftest/test_workflow_guards.py b/selftest/test_workflow_guards.py index 4c90a73..0037749 100644 --- a/selftest/test_workflow_guards.py +++ b/selftest/test_workflow_guards.py @@ -30,6 +30,7 @@ "selftest/test_pr_files_listing.sh", "selftest/test_prettier_scope_failsafe.sh", "selftest/test_prettier_symlink_filter.sh", + "selftest/test_safe_paths_unsafe_overrides.sh", ], ) def test_shell_selftest(script): @@ -39,6 +40,155 @@ def test_shell_selftest(script): assert proc.returncode == 0, f"{script} failed:\n{proc.stdout}\n{proc.stderr}" +def test_codex_review_covers_every_automergeable_class(): + """Codex must review every risk class that can merge unread. + + 2026-07-26 (whois-api-llc/wxa_vpn): pr-codex-review.yml gated on + `risk_class == 'sensitive'` alone, but `standard` is BOTH the default + class for ordinary src/** work AND a class claude-author-automerge.yml + will auto-merge. #1270 (+839 lines under src/**) and #1273 (+786) both + classified `standard`, passed CI clean, and never saw Codex; a manual + run on #1270 afterwards found four P1s, including a forgeable-identity + hole that let a customer attribute a leaked artifact to a competitor. + + `blocked` is intentionally NOT required here — it cannot auto-merge, so + a human is already the gate. + + Spend is bounded downstream by codex-gate.mjs (small-diff and + docs/tests-only skips), so this assertion is about coverage, not cost. + """ + text = (WORKFLOWS_DIR / "pr-codex-review.yml").read_text() + # Strip comments so a class named only in prose can't satisfy the gate. + code = "\n".join( + line for line in text.splitlines() if not line.lstrip().startswith("#") + ) + for risk_class in ("sensitive", "standard"): + assert f"risk_class == '{risk_class}'" in code, ( + f"pr-codex-review.yml must run Codex on risk:{risk_class} — it is " + "auto-mergeable, so nothing else guarantees a second reader" + ) + + +def test_standard_codex_lane_cannot_satisfy_the_automerge_bypass(): + """The risk:standard Codex lane must not publish the bypass-trusted check. + + claude-author-automerge.yml bypasses its risk-tier manual-merge gate when + the check named by `codex_check_name` (default "review / Codex Review") + concludes SUCCESS. That check name is " / Codex Review". + + The bypass trusts a CONCLUSION, not a review: when codex-gate.mjs skips + (small diff, or docs/tests-only), the review steps are skipped but the + job still concludes `success`. So routing risk:standard through job id + `review` would let a small change to an auth/billing/migration path — + classified `standard` because the repo's risk-paths.yml `sensitive:` + list is empty, yet flagged risky=1 by the central regex — auto-merge + with Codex having read nothing. Today that combination has no Codex + check at all, so the risk gate holds. + + Keep the two lanes on distinct job ids. (Codex review round 2 caught + this on the first draft of the standard-class widening, 2026-07-27.) + """ + text = (WORKFLOWS_DIR / "pr-codex-review.yml").read_text() + + def job_id_for(risk_class): + # Job ids are top-level (2-space) keys under `jobs:`; find the one + # whose body gates on this risk class. + current, found = None, None + for line in text.splitlines(): + m = re.match(r"^ ([A-Za-z0-9_-]+):\s*$", line) + if m: + current = m.group(1) + if ( + not line.lstrip().startswith("#") + and f"risk_class == '{risk_class}'" in line + ): + found = current + return found + + sensitive_job = job_id_for("sensitive") + standard_job = job_id_for("standard") + assert sensitive_job, "no job gates on risk_class == 'sensitive'" + assert standard_job, "no job gates on risk_class == 'standard'" + assert sensitive_job != standard_job, ( + "risk:standard and risk:sensitive must run in DIFFERENT jobs — a " + "shared job id publishes the bypass-trusted check name for standard " + f"PRs too (both are '{sensitive_job}')" + ) + + # And the standard lane's id must not be the one claude-author-automerge + # trusts by default. + automerge = (WORKFLOWS_DIR / "claude-author-automerge.yml").read_text() + m = re.search(r'default:\s*"([^"]*?)\s*/\s*Codex Review"', automerge) + assert m, "could not read codex_check_name default from claude-author-automerge.yml" + trusted_job = m.group(1).strip() + assert standard_job != trusted_job, ( + f"the risk:standard lane uses job id '{standard_job}', which is the " + f"bypass-trusted check name '{trusted_job} / Codex Review' — a " + "cost-gated SKIP would then read as a passed review and bypass the " + "risk-tier manual-merge gate" + ) + + +def test_safe_paths_never_automerges_customer_facing_legal(): + """docs/legal/** must be excluded from the docs safe-paths carve-out. + + 2026-07-25 (whois-api-llc/wxa_vpn#1268): an Acceptable Use Policy change + matched the built-in `^docs/.*` glob and auto-merged unreviewed. This + workflow decides on diff content alone and never consults the risk + classifier, so a caller's risk-paths.yml `sensitive:` list cannot close + the hole — the override list in the workflow is the only gate. + + Behavior is pinned by selftest/test_safe_paths_unsafe_overrides.sh; this + asserts the override list itself did not quietly lose the entry. + """ + text = (WORKFLOWS_DIR / "safe-paths-automerge.yml").read_text() + assert "unsafe_overrides='^docs/legal/'" in text, ( + "safe-paths-automerge.yml must keep docs/legal/ in unsafe_overrides — " + "customer-facing legal wording is not revertable in one cycle" + ) + + # Rename bypass: the pull-files API reports only the DESTINATION in + # .filename, so moving docs/legal/aup.md to docs/archive/ would read as + # an ordinary docs change without this. (Codex review round 1.) + assert "previous_filename" in text, ( + "the override must also match rename SOURCES (.previous_filename) — " + "otherwise moving a legal doc out of docs/legal/ auto-merges" + ) + + # Already-armed bypass: GitHub preserves an auto-merge request across + # pushes, so computing all_safe=0 does not disarm a PR armed on an + # earlier safe revision. (Codex review round 5.) + assert "--disable-auto" in text, ( + "safe-paths-automerge.yml must REVOKE an existing auto-merge arm when " + "an unsafe-override path appears — recomputing all_safe=0 leaves a " + "previously-armed PR armed, and the legal change merges anyway" + ) + assert "steps.classify.outputs.reason == 'unsafe-override'" in text, ( + "the revoke must be scoped to the override branch only — revoking on " + "every all_safe=0 would fight the workflows that legitimately arm" + ) + # A legal path can hide past the API's 3000-entry cap: the override scan + # sees no hit, but the revision cannot be proven clean either, so an arm + # from an earlier safe revision must not survive. (Codex round 10.) + assert "steps.classify.outputs.reason == 'file-list-truncated'" in text, ( + "revoke must also fire on a truncated file list — a docs/legal/ path " + "beyond the 3000-entry cap would otherwise keep a stale arm alive" + ) + + # The central risk scan is the DURABLE gate (it decides before arming, + # so it has no revoke race). It must see rename sources too, or moving a + # legal doc out of docs/legal/ escapes it. (Codex review round 9.) + automerge = (WORKFLOWS_DIR / "claude-author-automerge.yml").read_text() + assert "^docs/legal/.*" in automerge, ( + "claude-author-automerge.yml's risk patterns must include " + "^docs/legal/.* — safe-paths revocation alone races this workflow's arm" + ) + assert "previous_filename" in automerge, ( + "the risk scan must also match rename SOURCES — otherwise moving a " + "risky file to a safe path escapes the gate" + ) + + def test_no_global_git_config_writes_in_workflows(): """No reusable may write to global git config.