Uh oh!
There was an error while loading. Please reload this page.
fix(pii-gate): the override could not rescue an unset secret, and forks got impossible advice (backend#1409) - #154
Merged
Conversation
…ks got impossible advice (backend#1409)
Two defects, plus the first test this gate has ever had.
1 · THE DOCUMENTED ESCAPE HATCH DID NOT WORK
The `pii-gate-override` label was checked AFTER the unset-secret refusal, so on
a PR where PII_DENYLIST is missing the refusal exited first and the label could
not rescue anything. Both this file's own header ("Override a false positive:
add the 'pii-gate-override' label") and backend#1409's remediation note ("or
apply the pii-gate-override label to an individual PR") advertise it as the
per-PR way out of exactly that state.
That state is not hypothetical: the secret still does not exist, and today's
`.github` promotion (8aabe41, 13:53) armed the fail-closed path, so all seven
public repos now carry a red check that the documented workaround cannot clear.
An override is a statement that this PR should not be evaluated, so it now
precedes every reason the evaluation might refuse.
2 · A FORK PR WAS TOLD TO GO SET A SECRET THAT CANNOT HELP IT
GitHub does not pass secrets to `pull_request` runs from a forked repository, so
DENYLIST is empty on any fork PR regardless of what the org secret contains. The
gate then took the unset-secret path, whose message asks an org admin to run
`gh secret set` — advice that cannot work, because this is a property of the
event and not of the configuration.
Forks now get their own branch with a true reason and a real remedy: a
maintainer reads the PR text and applies the override label to record it. Still
fails closed. Compared against exactly "true", so an absent head.repo (deleted
fork) falls through to the paths below, which refuse on an empty denylist and
refuse again on a Compare read they cannot complete — an unknown fork status
cannot buy a pass.
This also unblocks #1409 defect 2. Making `pii-gate / pii-check` required would
have made every fork PR unmergeable; with the fork path explicit, the check can
be required for same-repo PRs and deliberately not for forks. Measured today:
0 fork PRs across all 7 public repos, so nothing is affected retroactively.
3 · A SELFTEST, IN THE SHAPE caller-drift ALREADY USES
scripts/tests/pii-gate-selftest.sh extracts the gate's `run:` block and executes
it against a stubbed `gh`. 15 cases: both new behaviours, the unset-secret and
fork refusals, title/body/commit matching, and every fail-closed path (comma-only
denylist, truncated commit list, unreadable Compare, missing base SHA, glob-shaped
term), plus a regression case for defect 3 — an early match in a ~150KB haystack,
which is the shape that used to be discarded via SIGPIPE.
Every case asserts the exit status AND a distinguishing phrase, because a fork PR
and an unconfigured org both exit 1 and the whole point of this change is that
they must not say the same thing. A status-only test would pass while the gate
gave impossible advice.
Proof the suite bites rather than merely being green: run against the currently
live main copy it fails exactly the 4 new cases and passes the other 11, so
nothing pre-existing regressed.
The test depends on the `run:` block staying free of `${{ }}` interpolation. That
is asserted, not assumed — extraction fails loudly if a future edit inlines an
expression, rather than silently covering less.
Not fixed here, and still open on backend#1409: the secret itself does not exist
(a content decision), and the check is required on 0 of 7 public repos. Scanning
the diff rather than only PR metadata is backend#1559.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>…env var The defect-3 case exported a ~150KB PR_BODY. Linux caps a single exec argument or environment string at 128KB (MAX_ARG_STRLEN), so every exec after that export died with E2BIG -- grep, head, tr and even the trap rm. macOS has a larger limit, so it passed locally and failed on the runner: "/usr/bin/grep: Argument list too long". Also unrealistic. GitHub caps a PR body at 65,536 characters, so no real run could produce that env var. Padding now comes from 250 commit messages generated straight into the fixture file, which never travel through argv or the environment -- and that is the faithful shape anyway: backend#1409 describes the haystack as title + body + up to 250 commit messages, title first. The case now asserts the haystack size (80,390 bytes) exceeds the 64KB pipe buffer. Without that, the fixture could shrink and the case would stop reaching the defect while staying green -- a regression test that no longer reproduces its bug is the same class of false comfort #1409 is about. Proof it reaches the defect: reverting the matcher to the pre-#130 pipe form makes this case, and only this case, fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saadqbal
self-requested a review
August 6, 2026 05:08
saadqbal
approved these changes
Aug 6, 2026
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of backend#1409, which is now unparked. Two defects I found while implementing its remaining steps — neither is in the ticket — plus the first test this gate has ever had.
1 · The documented escape hatch did not work
The
pii-gate-overridelabel was checked after the unset-secret refusal, so on a PR wherePII_DENYLISTis missing the refusal exited first and the label could not rescue anything.Both the gate's own header and backend#1409's remediation note advertise it as the per-PR way out of exactly that state:
And that state is live right now: the secret still doesn't exist, and today's
.githubpromotion (8aabe412, 13:53) armed the fail-closed path — so all 7 public repos currently carry a red check that the documented workaround cannot clear.An override says "don't evaluate this PR", so it now precedes every reason the evaluation might refuse.
2 · A fork PR was told to set a secret that cannot help it
GitHub does not pass secrets to
pull_requestruns from a forked repo, soDENYLISTis empty on any fork PR no matter what the org secret contains. The gate then took the unset-secret path, which asks an org admin to rungh secret set— advice that cannot work, because it's a property of the event, not the configuration.Forks now get a branch with a true reason and a real remedy: a maintainer reads the PR text and applies the override label to record that review. Still fails closed, and compared against exactly
"true"— an absenthead.repo(deleted fork) falls through to the paths below, which refuse on an empty denylist and refuse again on a Compare read they can't complete. An unknown fork status cannot buy a pass.This unblocks #1409 defect 2. Making
pii-gate / pii-checkrequired would have made every fork PR permanently unmergeable. With the fork path explicit, the check can be required for same-repo PRs and deliberately not for forks. Measured today: 0 fork PRs across all 7 public repos, so nothing is affected retroactively.3 · A selftest, in the shape
caller-driftalready usesscripts/tests/pii-gate-selftest.shextracts the gate'srun:block and executes it against a stubbedgh— 15 cases covering both new behaviours, title/body/commit matching, and every fail-closed path (comma-only denylist, truncated commit list, unreadable Compare, missing base SHA, glob-shaped term), plus a regression case for defect 3: an early match in a ~150KB haystack, the shape that used to be silently discarded via SIGPIPE.Every case asserts the exit status and a distinguishing phrase. That's deliberate: a fork PR and an unconfigured org both exit
1, and the entire point of change 2 is that they must not say the same thing. A status-only test would have passed while the gate gave impossible advice.Proof the suite bites rather than merely being green — run against the currently live
maincopy:Exactly the 4 new cases fail; the other 11 pass, so nothing pre-existing regressed. On this branch: 15 passed, 0 failed.
The suite depends on the
run:block staying free of${{ }}interpolation, which is what makes it executable standalone. That's asserted, not assumed — extraction fails loudly if a future edit inlines an expression, rather than quietly covering less.shellcheckandactionlintboth clean locally.Still open on backend#1409 after this
Test plan
bash scripts/tests/pii-gate-selftest.sh→ 15 passed, 0 failedPII gate selftestcheck runs on this PR (it touches all three trigger paths)pii-gate / pii-checkon this PR still fails on the missing secret — expected, and now clearable with thepii-gate-overridelabel, which is change 1's whole point. Worth using this PR to confirm that end to end.Epic: tracebloc/backend#1409 · Parent audit: tracebloc/backend#1408
🤖 Generated with Claude Code
Note
Medium Risk
Touches the org-wide public PII gate’s control flow and messaging; behavior stays fail-closed but prior bugs could block merges or mislead maintainers on fork vs missing-secret failures.
Overview
Fixes two decision-order bugs in the reusable public PII gate and adds the first automated regression suite for its shell logic.
The
pii-gate-overridelabel is now evaluated before any refusal paths (including missingPII_DENYLIST), so the documented per-PR bypass works when the org secret is unset. Fork PRs get a dedicated fail-closed branch via newIS_FORKenv: they no longer receive “set the org secret” guidance that cannot apply, and instead explain manual review plus override labeling.Adds
scripts/tests/pii-gate-selftest.sh, which extracts the gate’s env-onlyrun:block, stubsgh, and runs 15 cases asserting both exit code and distinguishing log text (override, fork vs unset secret, matches, fail-closed paths, defect-3 large-haystack regression).PII gate selftestworkflow runs that script on changes to the gate, selftest workflow, or test script.Reviewed by Cursor Bugbot for commit 05bb79f. Bugbot is set up for automated code reviews on this repo. Configure here.