Skip to content

Fix the non-blocking review findings on the UK replay results PR builder - #194

Merged
MaxGhenis merged 6 commits into
mainfrom
w6/results-pr-delta2-fixes
Oct 19, 2026
Merged

MaxGhenis merged 6 commits into
mainfrom
w6/results-pr-delta2-fixes

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

This PR fixes the six non-blocking findings from the approving hard-tier review of #193, which reviewed head 78e080c6 and merged as 2c89c529. The review is at w6-lanes/reviews/review_193_delta2.md. The job split, the token permissions and the CI matrix mechanism are unchanged.

Changes

  1. The tip check runs on the guarded request (.github/scripts/uk_replay_results_pr.sh, rebuild()).
    • Before, rebuild re-checked the request against main's tip from its own git archive of the request. It did that before assemble had run the output guard, so it read unguarded JSON, through any symlink. A missing request also failed there with a raw git error.
    • The tip check now runs after assemble and the tree comparison, on $STAGE/$REQUEST, the copy the guard has already checked. The separate extraction is gone.
    • The order pin names it, and it requires that rebuild contains no git archive or tar and exactly one builder check, on the staged copy.
    • Two behavioural tests move main so the tip check would run. A state commit without the request now ends with "has no request". A state commit whose STATUS.json is a symlink is refused by the guard before anything reads it.
  2. A held variant is compared by identity, not field by field (pipeline/uk_replay_results_pr.py, check()).
    • A request whose variant id main already holds is skipped only when the variant_id and identity_sha256 agree and main's record is certified. The rest of the record is not compared. That covers the note, and anything a later change to the index (a schema migration) adds or reshapes.
    • Because the skip rests on the digest, VARIANT.json's identity_sha256 must now be the digest of its own identity.
    • Tests run in both directions, in strict and stdlib modes, with Hypothesis properties. The end-to-end skip test also migrates main's record after the merge.
  3. The matrix tests use the bytes Certify the policyengine.py 6.4.1 replay results from qualification attempt 3 #188 and Certify the policyengine.py 6.5.0 replay results, the first with the processed-value check #190 actually edited.
  4. The control-environment pin follows imports transitively.
    • ImportScan (in the test file) follows what runs from the builder's main and from the verify-sealed branch of uk_replay_qualify's main. That is each reached module's top level, the functions and classes reached code names (directly or through module aliases), and import statements only where reached code holds them.
    • The reached third-party set must be exactly CONTROL's three packages, and no release package may be reached.
    • A synthetic package tests the scanner itself.
    • The test uv stub now emulates uv run --with …: it refuses every installed package except the specs and what they require. So the end-to-end build jobs run the builder and verify-sealed with CONTROL's packages alone. Before, they ran with numpy and everything else the suite installs.
  5. GITHUB_SHA is set as a real dispatch sets it.
    • In the end-to-end harness, every run is a dispatch: GITHUB_SHA is main as dispatched, and both jobs check that commit out.
    • New tests:
      • A run dispatched before main moved proposes on the newer main.
      • A consistent older-base build, whose tree matches because job 1 really built on that base, passes with its own run's GITHUB_SHA. A later dispatch's GITHUB_SHA refuses it, and nothing else does.
  6. Nit: the orphan # scoped to comment line is removed and the comment is rewrapped.

docs/uk_replay/RELEASE_REPLAY.md ("The results pull request") and the workflow comment now describe the new order and the identity comparison.

Invariants

  • A held id: a request whose variant id main holds is skipped exactly when the id and the verified identity_sha256 agree and main's record is certified. No other change to main's record affects the decision. Property tests cover any non-identity change (skipped) and any identity leaf (refused).
  • The matrix edit: on any ci.yml, the stdlib edit succeeds only when the matrix lists exactly the certified variants before the new one (property test). Differential tests on the real inputs compare it with Certify the policyengine.py 6.5.0 replay results, the first with the processed-value check #190's committed bytes and Certify the policyengine.py 6.4.1 replay results from qualification attempt 3 #188's matrix hunk.
  • The guard comes first: job 2 reads request JSON only from the copy the output guard checked. This is pinned statically and behaviourally.
  • CONTROL's packages: what the strict paths can import equals CONTROL's packages. The static scan and the runtime emulation check this independently: the end-to-end build jobs, plus verify-sealed on every committed namespace.

Verification

  • Fleet ops' disk hold is on, so I ran no local pytest. bash -n, shellcheck and actionlint are clean, and ruff format --check passes.
  • I called the static and fixture tests directly with the shared venv's interpreter, and they pass. That covers the workflow, builder and order pins, both fixture matrix tests, the -I -S test, the wrong-matrix property, the control scan, the synthetic scan, the uv emulation probe, and verify-sealed under CONTROL on every committed namespace.
  • I checked each new pin by breaking the code it guards, and each one failed: the tip check moved back before the guard, the tip check reading its own extraction, a builder call on an ::error:: line, a builder call after a #, and the stdlib list check removed.
  • I also checked the scanner the same way. It catches an import added in validate_record, at the interface module's top level, behind a new verify_sealed → compute_uk_event call, and in a builder helper. It correctly ignores one in qualify's corpus branch.
  • CI runs the collision and end-to-end tests.

Review

  • Hard-tier review of c5c17db7: APPROVE (w6-lanes/reviews/review_194.md), with three P3 limitations in the new pins.
  • 3ddb3311 answers them:
    • the propose pin's message must be a whole quoted word;
    • ImportScan resolves names by Python scope and to every binding a name may have, treats a module used as a value as fully reachable, and flags aliased dynamic imports;
    • the uv emulation evaluates markers and purges modules loaded before it.
  • Delta review of 3ddb3311: APPROVE (w6-lanes/reviews/review_194b.md).
    • CI run 39252322886 on that head: 2,265 passed, 15 skipped.
    • Locally, the results-PR test file passes 84/84 (-n 4, after the disk hold was lifted).
  • Non-blocking notes the delta review left, for a follow-up after Read CI's replay-engine matrix from the variant index; drop the results bot's workflows permission #196 (which rewrites these tests):
    • The propose pin should track quote state across the whole body; its message regex can still be fooled by a closing quote.
    • ImportScan should bind a function's global imports and __main__-block imports module-wide, and add exec/eval to its attribute list.
    • The scan's branch selection, decorator registries and module __getattr__ are not modelled. None of these patterns occurs on the real entry points.

axiom: n/a: CI and test changes to the results-PR builder; no policy encoding.

🤖 Generated with Claude Code

Review of #193 at 78e080c, finding 1: rebuild's tip re-check extracted the
request from replay-state into its own directory and ran `builder check` on
it before `assemble` had run the output guard, so it read unguarded JSON
(through any symlink), and a missing request failed with a raw git archive
error. The tip check now runs after assemble and the tree comparison, on
$STAGE/$REQUEST, the copy the guard has already checked. The order pin
names it, and rebuild may hold no other extraction.

Also drops the orphan "# scoped to" comment line in uk-replay-results.yml
(finding 6) and reads JSON fields through one helper.

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

vercel Bot commented Oct 18, 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 19, 2026 12:29pm UTC

Request Review

Review of #193 at 78e080c, finding 2: the id-collision check compared the
committed record with VARIANT.json field by field (without the note), so a
later change to main's copy (a schema migration adding a field) would turn
a merged request's rerun into the misleading "qualify this pair again"
refusal. A request whose variant id main holds is now skipped exactly when
the id and identity_sha256 agree and main's record is certified; the rest
of the record is not compared. Because the skip rests on the digest,
VARIANT.json's identity_sha256 must be its identity's.

Tests in both directions, strict and stdlib: a migrated record (note,
added, dropped and reshaped fields) skips, and a Hypothesis property
extends that to any change outside the compared fields; another identity,
another digest, a non-certified record and a digest that is not the
identity's are refused, with a property over every identity leaf. The
end-to-end skip test also migrates main's record after the merge.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of #193 at 78e080c, finding 3. The strict == lean matrix test held
by construction: add_matrix_variant builds the edited text once and both
modes return it. Replaced with tests of what the propose job relies on:

- tests/fixtures/uk_replay/results_pr holds the ci.yml #188 and #190
  actually edited (e796ffb^ and f4164f2^, pinned by their git blob ids,
  since CI's shallow checkout has no history). With yaml unimportable, the
  stdlib edit of #190's input is #190's ci.yml blob (29f7a1b) exactly,
  and of #188's input is #188's matrix hunk alone (#188 also changed the
  determinism step). The strict mode raises ImportError there, so the
  block bites.
- The same edit under python3 -I -S, the propose job's interpreter.
- A Hypothesis property: on the real files, the stdlib list check alone
  refuses a matrix listing anything but the certified variants before the
  new one.

The propose pin no longer skips every line containing ::error::: only the
text of a quoted ::error::/::notice::/::warning:: string with no command
substitution is set aside, propose may hold no inline comment, and the pin
checks its own rule on a builder call after a message, inside one and
after a #.

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

Review of #193 at 78e080c, finding 4: the control-environment pin read the
imports of four named files and ignored every pipeline.* import, so a new
third-party import in another module those files reach would go unseen;
and the test uv stub ran the test interpreter, numpy and all, so nothing in
CI ran the build job's checks with CONTROL's packages.

- ImportScan follows what runs from the strict entry points (the builder's
  main, and uk_replay_qualify main's verify-sealed branch): a module's top
  level when imported, a function or class when reachable code names it,
  directly or through a module alias, and an import statement only where
  reachable code holds it. Pipeline modules are followed however they are
  named, including relative and bare sibling imports; dynamic imports in
  reached code fail the pin. The reached third-party set must be exactly
  CONTROL's (jsonschema, rfc8785, pyyaml), no release package may be
  reached, and the reached modules and key functions are pinned. A
  synthetic package checks the scan itself: calls across modules, a class
  method, a relative and a sibling import count; an unreachable function,
  another command's branch and a __main__ block do not.
- The test uv now runs `uv run [--with SPEC]... python -m ...` with every
  installed package refused except the specs and what they require
  (extras aside), so the end-to-end build jobs run the builder and
  verify-sealed with CONTROL's packages alone. Tests pin the emulation (on
  real modules: numpy, hypothesis and pytest refused, jsonschema's
  dependencies admitted) and run verify-sealed on every committed sealed
  namespace under it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of #193 at 78e080c, finding 5: the harness stripped every GITHUB_*
variable, so no end-to-end run set GITHUB_SHA to the dispatch-time main,
the production case, and the "consistent older base" test forged only the
base, so the tree comparison would also have refused it.

- Remote.run is a dispatch: GITHUB_SHA is main as dispatched (or a given
  commit), and both jobs check that commit out, as actions/checkout does.
  build_job and rebuild_job run one job each for the tests that drive them
  separately, so those carry GITHUB_SHA too (a rerun keeps its run's).
- A run dispatched before main moved proposes on the newer main, with the
  dispatch commit's code in both jobs.
- A consistent older-base build: job 1 really built on the older main, so
  its tree matches. Rebuilt with that run's GITHUB_SHA it passes; with a
  later dispatch's it is refused by the GITHUB_SHA check alone.
- Two behavioural pins for finding 1, with main moved so the tip check
  runs: a state commit without the request ends with "has no request",
  not a raw git error; and one whose STATUS.json is a link is refused by
  the output guard before any check reads it.

Docs and the workflow comment state the new order: the tree comparison,
then the decision on main's tip from the guarded request.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The hard review of c5c17db approved with three P3 limitations in the
pins this PR adds:

- The propose pin's message stripping ignored shell quoting:
  `echo ""::error::; builder check; : ""` hid a builder call. A message is
  now one whole double-quoted word (whitespace or line start before it,
  whitespace, `;` or line end after) holding no other quote, backslash,
  backtick or command substitution. That case and a single-quote variant
  are self-checks.
- ImportScan kept one binding per name per module, so an import in a
  function nothing reaches could overwrite a live binding, and it missed a
  module used as a value and aliased import_module. Names now resolve as
  Python scopes them (module-level imports everywhere, a function's own
  imports only in it) and to every binding they may have; a pipeline
  module used as a value (assigned, passed, given to getattr) makes all
  its functions reachable; and dynamic imports are caught by name,
  attribute or from-import, however aliased. A second synthetic package
  pins each case. On the real entry points the reached functions, modules
  and the three packages are unchanged.
- The uv emulation kept requirements behind false markers and could not
  refuse a module loaded at startup. It now evaluates markers with
  packaging (extras only where asked for), and drops every refused module
  already loaded (its own packaging, anything a .pth imported) so the next
  import is refused; the probe checks packaging is. Versions remain the
  suite's, as the comment says.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaxGhenis
MaxGhenis marked this pull request as ready for review October 19, 2026 12:46
@MaxGhenis
MaxGhenis merged commit 9321a7b into main Oct 19, 2026
11 checks passed
@MaxGhenis
MaxGhenis deleted the w6/results-pr-delta2-fixes branch October 19, 2026 12:47

This branch was successfully deployed

1 active deployment
Preview — 3ddb3311 Deployed Oct 19, 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