Skip to content

Run CI's checks before proposing results; allowlist propose's commands - #217

Draft
MaxGhenis wants to merge 6 commits into
mainfrom
results-pr-preflight
Draft

MaxGhenis wants to merge 6 commits into
mainfrom
results-pr-preflight

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

This closes findings 3 and 2 of the GPT-6.1 Sol review of the results-PR stack (#199, #196, #201, #202). Both were non-blocking gaps. The report, RESULTS_STACK_REVIEW.md, is copied to ~/reviews/scorecard-results-stack-review/. Finding 1, the HF-token fetch order in ci.yml, is a separate task.

1. The results bot runs CI's own checks before it proposes (finding 3)

The App opened results PRs #195, #204 and #206 as ready, and each failed CI's pytest · ruff format job. The builder verified seals, regenerated the replay namespaces and built the database twice. It never ran the suite or the format check.

Now, after every check that stops the run, the build job runs ci.yml's python-job lines on the assembled tree:

  • uvx ruff format --check be nz scorecard_db tests pipeline;
  • uv run --with pytest --with hypothesis --with pypdf --with pyyaml --with numpy --with psutil --with rfc8785==0.1.4 --with jsonschema pytest tests/ -q, with PYTHONPATH=.;
  • the no-drift check after the suite, git status --porcelain.

A test pins both lines, and their environments, to ci.yml word for word.

Permissions and token handling are unchanged. The checks run in build, which has contents: read, unpersisted credentials and no secret. Around them:

  • The script refuses to start the checks if GH_TOKEN or GITHUB_TOKEN is set.
  • The variables that name the step's command files are unset, so a test that writes one by name writes nowhere. That is hygiene, not a sandbox: a process that finds the files on disk can still write them. That is why propose takes every build output as build's word, checked for shape.
  • Workflow commands are paused around the checks' output (::stop-commands:: with a token only the script holds).
  • Everything build outputs about the commit is read before the checks run.

A failure doesn't stop the run. Instead:

  • The builder's preflight subcommand reports each check's exit status, how many things failed, and the first 50 listable names, as one line of JSON. The names are pytest's node ids from the last short test summary, ruff's files in either output format, and git's paths. Listable means printable ASCII, no backtick, no space at either end, at most 200 characters, so three full lists keep the body under GitHub's 65,536-character limit.
  • propose passes the report into the body, where read_preflight refuses any other shape.
  • The PR opens as a draft, and "Validation in the results run" lists the failures in a fenced block.
  • A rerun that should be a draft turns an open ready PR back into a draft (gh pr ready --undo) before the force-push. The builder never marks a PR ready.
  • The build job takes about 12 minutes longer, well within its 45-minute timeout.

2. The detector checks an allowlist, not shell names (finding 2)

runs_pipeline_code's shell-name regex matched text before bash's quote removal, so { …; } | b''ash and | b\ash got past it and ran bash.

The regex is gone. Commands reads the output of shell_residue (#202's lexer, unchanged) and lists every simple command bash would run, in all of these places:

  • the top level;
  • if, while and until bodies;
  • { } and ( );
  • $(…), <(…) and >(…);
  • bash 5.3's ${ …; } and ${|…;};
  • function bodies.

Anything it doesn't model raises Unlexed, so it is flagged: case, for, arithmetic, array indices, brace expansion, a parameter expansion beyond a name with a literal default, and a word after a compound command.

disallowed then requires every command word, as written, to be in PROPOSE_RUNS. That set is exactly what propose runs, and a test pins the equality: git, gh, grep, cut, cat, printf, echo, :, [, local, exit, plan, pushgit. It also flags:

  • Variables: assignments to non-local (uppercase) names, command-prefix environments, local with options or declaring a name twice, printf without a literal non-option format, and [ … -v … ].
  • Arithmetic: anywhere bash evaluates it past a digit or @/* subscript (${a[x]}, ${y:x}, ${!x}, $[x], a[x]=, ([x]=v)). Arithmetic expands a command substitution held in a value (x='a[$(cmd)]').
  • git and gh: subcommands outside the ones propose uses, and git -c other than the credential helper.
  • Options written as words: git options that run a program or write a file (--upload-pack, --receive-pack, --exec, --output), and gh's --editor/--web. This covers arguments, variable values and array elements, however quoted, escaped (backslash line breaks included), abbreviated (git takes any unambiguous prefix) or bundled (-dw).
  • Writes: anything other than >&2 and >> "$GITHUB_STEP_SUMMARY".
  • Functions: any function definition but pushgit. Its definition is pinned, and it must sit at the top level before its calls.

A function with an allowlisted name would shadow that program. So the test pins that no definition anywhere in the script has one, except plan and pushgit, and that the script contains no eval, source or .. It also pins plan and its field helper: the isolated runner's Python, running a fixed program that imports only json and sys.

Both bypasses from the stack review are must-flag cases, as are the 14 found by the review of this PR (below).

Invariants (each run as a test)

  • Allowlist (property): for any spelling of any command name, in 20 command positions, the command is flagged exactly when the word as written is not an allowed name.
  • Report writer vs reader (differential property): for any exit statuses and log contents, job 1's report is one line, under 100 KB, that job 2's read_preflight accepts unchanged.
  • Summary round trip (property): pytest node ids come back exactly. This holds whatever the messages contain (-, ], extra lines) and when a parametrised id holds - or an unmatched [.
  • Shape: job 2 refuses 25 malformed reports.
  • Draft rule (property): a PR is a draft exactly when integrity is incomplete or a check fails.
  • Listing (property): each listed name is a line of its own inside exactly one fence, and any count beyond the list is stated.
  • Size (end to end): with every check failing on 80 names of the longest listable length, the body stays under 65,536 characters.
  • End to end: a stubbed suite and ruff run against a local bare remote, with every command-file variable set as the runner sets them.
    • CI's checks run on the verified commit with no token or command-file variable in their environment, and between the pause and resume tokens.
    • Each failure mode opens a draft that names it: failed tests, a collection error, unformatted files in both ruff styles, a suite exiting with no summary, and drift.
    • A forged report is refused before the token is minted.
    • A failing rerun turns the PR back into a draft before the push.
    • A passing rerun never marks it ready.

Review

The GPT-6.1 Sol review (lane run 20261021-225349-pr217-review-3; report in ~/reviews/scorecard-results-stack-review/pr217-review-3/) returned CHANGES REQUESTED. It confirmed the production change, the credential boundary, the shape check, the draft rule, the parsers, the size limits and CI-line equality. Fifteen of its 16 mutations were caught by the tests.

It found 14 inputs the detector missed. Each was executed under bash 3.2, 5.2.37 (the CI runner's version) and 5.3. The current propose uses none of them.

  • [ -v 'a[$(cmd)]' ];
  • an array redeclared from a quoted string;
  • process substitution inside ${…};
  • printf {-v,PATH};
  • git and gh options passed through variables, arrays, braces and line continuations;
  • gh option bundles.

The commit after the review applies the reviewer's patch and fixes its other findings:

  • definitions found anywhere in a line, plus the eval/source pin;
  • node ids parsed correctly with unbalanced brackets, and the count bound raised to 10^9;
  • the duplicate-count mutation now caught;
  • every command-file variable set in the end-to-end tests;
  • outputs read before the suite runs;
  • re-drafting before the push;
  • workflow commands paused;
  • git's -u dropped (in git 2.55 it is --set-upstream, not a program);
  • the duplicated cases trimmed.

A re-review of the fixes follows.

Known limits

  • The detector reads propose's body and the helpers it calls. It doesn't read the script's top-level lines, which every job runs: variable settings, the request-id check and the dispatch case.
  • An option assembled at run time (u=--w; u+=eb) is beyond any rule on words.
  • Beyond the rules above, the detector doesn't model what an allowed program does with its arguments.
  • The report is job 1's word, checked only for shape, like the verify-sealed and content-hash lines. A package in job 1 could claim the checks passed; CI on the PR reruns them.
  • On CI, pytest prints whole messages over several lines. A message line that itself starts with FAILED or ERROR is read as another node id. This changes only what is listed, not the draft decision, which uses exit codes.
  • A rerun on an unchanged main pushes nothing, so it leaves the body as it was.

Checks run

  • CI: all green at cf8066bc, including the full suite and the 6.9.0 replay leg from main.
  • Locally, under a host reservation, so not the full suite:
    • tests/test_uk_replay_results_pr.py with -n 4: 128 passed after the review fixes.
    • The replay-focused files (results_pr, ci_gates, discover, variants, qualify, determinism and engine_matrix): 444 passed and 1 skipped at 56ebf542.
    • uvx ruff format --check be nz scorecard_db tests pipeline passes.

axiom: n/a: CI and test tooling

🤖 Generated with Claude Code

The results bot verified seals, regenerated the replay namespaces and
built the database twice, but never ran the suite or the format check, so
#195, #204 and #206 opened ready and failed CI. The build job now runs
ci.yml's own ruff and pytest lines (pinned word for word) and its no-drift
check on the assembled tree, with no token and no step command files in
reach. A failure no longer means a red ready PR: the PR opens as a draft
whose body names the failing tests, files or paths, and a failing rebuild
turns an open PR back into a draft. Nothing marks a PR ready.

runs_pipeline_code matched shell names before quote removal, so b''ash and
b\ash ran bash past it. It now lists every command bash would run from
propose (substitutions, groups, compound commands, function bodies) and
requires each command word, as written, to be one propose uses; what the
parser does not model is flagged. Both bypasses are test cases.

Review of 2443472, findings 2 and 3.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vercel

vercel Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
policyengine-scorecard Ready Ready Preview Oct 22, 2026 8:37am UTC

Request Review

MaxGhenis and others added 2 commits October 21, 2026 21:35
…ctor

Bash evaluates an array subscript, a substring offset, ${!name}, $[...]
and an array literal's [index] as arithmetic, and arithmetic runs a
command substitution held in a value: with x='a[$(touch m)]', each of
${a[x]}, ${y:x}, ${!x}, a[x]=1, $[x] and a+=([x]=v) ran touch under bash
3.2, past the allowlist. Parameter expansions are now a name with at most
a literal default, alternative or error word, subscripts are @, * or
digits, and indices and $[...] are unmodelled, so flagged.

printf's format must be a literal that is not an option (quoted -v or an
expansion is still -v). git's options that run a program or write a file
are matched after quote removal and as abbreviations, and --output joins
them; gh may not open an editor or browser. The script-level shadowing
check also finds `name () {` and `function name` definitions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Three checks listing 100 names of up to 300 characters could reach about
90,000 characters, past the 65,536 GitHub accepts for a pull request body,
and the branch would already be pushed when gh pr create failed. Each
check now lists at most 50 names of at most 200 characters (about 31,000
for all three), and an end-to-end test fails every check with 80 names of
the longest listable length and checks the body still fits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The suite runs with $GITHUB_OUTPUT and the other command-file variables
unset, which stops a test that writes one by name. A process that finds
the files on the runner's disk can still write them; job 2 already takes
every build output as build's word, checked for shape. The script comment,
the release doc and a test comment claimed more.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of cf8066b (GPT-6.1 Sol, CHANGES REQUESTED on the test-side
detector only) found 14 inputs bash runs that the detector passed, each
executed under bash 3.2, 5.2.37 and 5.3: [ -v 'a[$(cmd)]' ], an array
redeclared from a quoted value, process substitution inside ${...},
brace-expanded printf -v, and git or gh options passed through variables,
arrays, braces, backslash line breaks or short-option bundles. Its patch
is applied as written: options are checked in every word, after a
backslash line break is removed, and brace expansion, [ -v ] and a local
declared twice are flagged. The claim is narrowed to options written as
words; one assembled at run time is beyond any rule on words. git's -u is
dropped from the rule: in git 2.55 it is --set-upstream.

Function definitions now count wherever they start, and the script holds
no eval, source or . to build one. A node id's brackets end at the first
] its message follows, so an unbalanced [ no longer glues the message on;
the report's count bound is 10^9; a duplicate failure counts once.

The build job reads what it outputs about the commit before the suite
runs in its tree, and pauses workflow commands (::stop-commands:: with a
token only it holds) around the checks' output. A rebuild that should be
a draft turns its open pull request back into one before the push, not
after, and every gh pr ready carries --undo.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch was successfully deployed

1 active deployment
Preview — f936b49c Deployed Oct 22, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant