Skip to content

Close the review notes on the results PR builder's test pins - #202

Merged
MaxGhenis merged 2 commits into
mainfrom
w6/results-pr-test-pins
Oct 22, 2026
Merged

MaxGhenis merged 2 commits into
mainfrom
w6/results-pr-test-pins

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Closes the non-blocking notes that the second hard-tier review of #194 (review of 3ddb331, review_194b.md) left on the test pins in tests/test_uk_replay_results_pr.py. Test-only: no script, workflow or pipeline module changes. Stacked on #201 (and #196 under it), which rewrote the same file; it will be rebased onto main once they merge.

Draft until the hard-tier review agrees. An in-session adversarial pass (Opus) found no way past the lexer, ImportScan or the uv emulation, and one overclaim. The comment said the residue may not run text as code, but \. file, if . file, | "$BASH" and trap "$(cat file)" got through. The second commit widens the check to match, and those four cases are now self-checks.

1. Propose pin: a lexer, not a per-line regex

shell_residue reads the whole propose body left to right, as bash does:

  • Quote state carries across lines. "…", '…', $(…), (…) and ${…} nest as bash nests them, and escapes are honoured.
  • A comment line is dropped, and a message is set aside, only where it starts a line outside every quote, substitution and parenthesis. So a # line inside a multi-line string or substitution stays in the residue (review 3).
  • A message is echo "::error|notice|warning::…" alone on its line, optionally >&2. Its text holds no quote, backslash, backtick or newline, and its only $ expansions are plain names. That rules out $(, ${, $(( and bash 5.3's ${ cmd; } and ${|cmd;}. Bash reads such a line as an inert literal echoed to the log.
  • The lexer refuses what it does not model: a heredoc, a backtick, $'…', case inside parentheses, or a quote left open.
  • A residue line is flagged if it holds #, pipeline or the word builder, or if it names something that can run text as code: a shell by name or as $BASH, $SHELL or $0, eval, exec, source, trap, or the . command (escaped or not).

The self-checks cover each of the review's cases:

  • echo " "::error::; builder check; echo " "
  • echo x"::error::a "::error::; builder check; echo " z"
  • a quote that spans lines
  • #$(builder check) inside a multi-line string
  • ${ builder check; } and ${|builder check;}

They also include one exploit for each construct the lexer tracks or refuses, built on a "quote flip" template. Every case in the runs list runs builder check under /bin/bash 3.2 when builder is defined. The two ${ …; } forms need bash 5.3, which this host does not have. The refused list holds the rule-only cases: an inline comment, a case, an open quote, and a message that is piped or sits in parentheses.

2. ImportScan

  • An import in the __main__ block, or an import of a name the function declares global, binds that name module-wide.
  • The entry module's __main__ block is walked. An imported module's block is not.
  • exec and eval are added to DYNAMIC_ATTRIBUTES, so builtins.exec(…) is recorded.
  • entry() follows the elif and else of a skipped if args.command == "x". It walks any other test of args.command (!=, in, chained) in full, and that test does not count as the command's branch. A taken branch skips its own elif and else.
  • Each case is now in SCAN_FIXTURE or SCAN_CONSERVATIVE. The real scan is unchanged: the same four modules, third-party imports {jsonschema, rfc8785} (CONTROL as Read CI's replay-engine matrix from the variant index; drop the results bot's workflows permission #196 left it), and no dynamic imports. The real entry points' command branches have no else, and their __main__ blocks are raise SystemExit(main()).

3. The test uv

UV_RUN now allows only these:

  • the standard library
  • the top-level modules of the --with closure
  • modules that the finders after its own would load from the checkout, or that are built-in or frozen

It refuses everything else, so a module that no distribution owns is refused, whether or not it is already loaded. _virtualenv, which _virtualenv.pth loads at startup, is one example. The probe test adds a stray module outside the checkout that a sitecustomize loads at startup, and asserts it is refused. It also asserts _virtualenv is refused wherever that module exists, and that a checkout module is imported.

Mutation check

Each pin's checks were weakened one at a time, and the pin's test was run against each mutant. 36 of 37 mutants fail, as they should. The one that survives lets a ' appear inside a message. That mutant is equivalent: bash reads ' literally inside double quotes, and single quotes are tracked, so no input tells the two apart. The rule stays because the brief asks for it. The mutation script and the bash run of every self-check are in ~/PolicyEngine/_drafts/continuous-budget-ci/w6-lanes/pins/.

Not addressed (beyond the brief)

The review also lists these ImportScan gaps, and this PR does not touch them:

  • decorator registries
  • module __getattr__
  • globals(), sys.modules and __dict__ lookups
  • star imports
  • getattr with a string name
  • pkgutil.resolve_name and EntryPoint.load
  • package __init__.py files

A text pin also cannot see words split by quotes (bui""lder). End-to-end runs back that up: propose runs as its own process, where builder() is not defined, because it is defined only inside build and rebuild.

Checks

  • pytest tests/test_uk_replay_results_pr.py: 83 passed locally.
  • ruff format --check: clean.

axiom: n/a: test-only change

🤖 Generated with Claude Code

@vercel

vercel Bot commented Oct 19, 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 21, 2026 11:34pm UTC

Request Review

MaxGhenis and others added 2 commits October 21, 2026 19:33
Review of 3ddb331 (review_194b), the non-blocking notes on the test pins:

- propose pin: shell_residue lexes the whole propose body left to right,
  carrying quote state across lines and nesting quotes, $(...), (...) and
  ${...} as bash does. Only a comment line, or a message line (an echo of
  a literal ::error::/::notice::/::warning:: string, alone on its line),
  that starts outside all of them is set aside. Heredocs, backticks,
  $'...', case inside parentheses and open quotes are refused, and the
  residue may not run text as code (a shell, eval, exec, source or .).
  Each self-check that bash should run does run `builder check` in bash
  3.2 (bash 5.3 for the ${ cmd; } forms).
- ImportScan: imports in the __main__ block and imports of names a
  function declares global bind module-wide; the entry module's
  __main__ block runs; exec and eval count as dynamic attributes; a
  skipped command branch's elif and else are followed, and a test other
  than args.command == "x" runs both ways.
- UV_RUN: allows the standard library, the specs' closure and modules
  that load from the checkout, and refuses everything else, so a module
  no distribution owns (_virtualenv via its .pth) is refused, loaded or
  not.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The adversarial pass on 10fc1c0 found the residue check narrower than
its comment: `\. file`, `if . file`, `| "$BASH"` and a trap of a file's
contents ran echoed messages unflagged. SHELL now matches a shell by name
or as $BASH, $SHELL or $0, eval, exec, source, trap, and the . command
escaped or after any word, and each of those four cases is a self-check
that bash 3.2 runs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaxGhenis
MaxGhenis force-pushed the w6/results-pr-test-pins branch from 2443472 to f378a01 Compare October 21, 2026 23:33
@MaxGhenis
MaxGhenis changed the base branch from w6/results-pr-tests-live-newest to main October 21, 2026 23:33
@MaxGhenis
MaxGhenis marked this pull request as ready for review October 21, 2026 23:33
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

#201 merged as fdcdea29. I rebased this branch onto main (git rebase --onto origin/main 00afa363), force-pushed it with a lease on 24434729, retargeted it to main and marked it ready, as the stack review's process steps asked. The new head is f378a016. Its git patch-id --stable matches the reviewed change 5c10b3f8..24434729 (88c0888c).

Local checkouts of w6/results-pr-test-pins are still at the old head. Reset them to the remote before pushing.

🤖 Generated with Claude Code

@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Merged by squash at head f378a016.

The fix stack (#199, #196, #201, #202) is now on main. Next: close #206 and rebuild the 6.9.0 results PR from request 705e2b7a.

🤖 Generated with Claude Code

This branch was successfully deployed

1 active deployment
Preview — f378a016 Deployed Oct 21, 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