Repository navigation
fix(policy): scope fixture digests to consumers and gate self-registration on review - #2973
joeharris76 wants to merge 6 commits into
Conversation
![benchbox-oracle[bot]](/s/com/githubusercontent/avatars/G.https/u/319264189?s=60&v=4){kind=small}
There was a problem hiding this comment.
oracle-verdict: failure for cabbae2e45f9ee6e346fcb2097e346f950e51935
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (645 code lines exceed 150; touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
The oracle-gated self-registration lets a PR authorize its own comment-policy exceptions and scope-notice exclusions on any successful check run, and the narrowed fixture digest can keep matching after semantic edits. Fix the check-run authentication, bind review to specific entries, validate candidate notices against base evidence, and tighten the scoped digest before merge.
Reviewers not available
- sol: absent (error)
- luna: absent (error)
|
/oracle-review |
There was a problem hiding this comment.
oracle-verdict: success for 6c38acfc576985d06c293d51e855f7d5a27291a8
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (762 code lines exceed 150; touches 3 areas).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
All five earlier defects are fixed by the new oracle binding, candidate-notice validation with fresh-path restriction, and scoped digest with order/shadow handling. No new merge-blocking defects found in the changed files.
Verified fixed: D1, D2, D3, D4, D5. Resolve their review threads.
Scope: files changed since head cabbae2e45f9ee6e346fcb2097e346f950e51935 was reviewed.
6c38acf to
a92d019
Compare
|
/oracle-review |
There was a problem hiding this comment.
oracle-verdict: success for a92d019277cc9623871479e0c6e4bb3be48f7d36
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (762 code lines exceed 150; touches 3 areas).
Decision: SHIP, carried from head 6c38acfc576985d06c293d51e855f7d5a27291a8.
The patch is unchanged since head 6c38acfc576985d06c293d51e855f7d5a27291a8 was reviewed, so its decision is carried to this head without running a reviewer. Its open review threads still apply.
|
/oracle-review |
There was a problem hiding this comment.
oracle-verdict: failure for 7a4d195348fbc5a2f2a2b0355490a8f0aeb2568e
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (749 code lines exceed 150; touches 3 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Same-PR comment-policy exceptions and scope-notice exclusions are gated on oracle-review check success, which is vacuously true for non-soundness-path pull requests, enabling self-authorization; candidate notices can additionally escalate a partial pin to a whole-file scan exclusion.
Scope: files changed since head a92d019277cc9623871479e0c6e4bb3be48f7d36 was reviewed.
There was a problem hiding this comment.
oracle-verdict: failure for 85d5a131f3c7e2b361ad52360685d08f01d551f5
This result supersedes those for earlier heads; their open review threads still apply.
Tier: very-high (large multi-area diff: 851 code lines across 3 areas).
Decision: SHIP WITH FIXES.
Reviewer: opus (claude claude-opus-5-5, effort medium).
The PR does four things. It lets a pull request register its own comment-policy exceptions and scope notices once an oracle success verdict covers the head. It adds a "scoped" fixture digest that covers only the reading symbol and the module bindings it resolves through, keeping the old whole-file digest valid for existing entries. It adds candidate-notice range pinning. It removes 27 fixture exceptions. Spot checks show those exceptions were stale: the code changed, or a reviewed argv already covers it.
The verdict gate matches the existing oracle_review_check rules and fails closed: bot identity, exact head commit, latest verdict, and a latest successful oracle-review workflow run on the head. Range pinning correctly blocks moving a notice to a path not pinned at the base, widening a range, and turning a partial pin into a whole-file pin.
Two problems must be fixed:
- The scoped digest misses several ways the executed input can change. These include module-level statements that mutate a binding without assigning it, state reached through self or class attributes, and pytest fixtures passed in as parameters. In those cases the executed input can change without invalidating the exception. That weakens the binding that the whole-file digest gave and that the docs promise.
- The authorization path never turns on in CI, because the comment-policy job provides neither a token nor permission to read checks. The documented workflow ("obtain the verdict, rerun, merge") therefore always fails.
Check the oracle-review check run itself (name, completed status, success conclusion, github-actions app) plus its workflow run path, head SHA, and conclusion before honoring candidate exceptions. Validate candidate notices against head bytes and digests, and only for paths already pinned at base so reviews cannot widen exclusions. Order scoped digest parts by line number; prefer the local module binding over builtin skips when a name is locally bound. Refresh registry digests for the tighter scheme and add auth, notice, and digest regression tests. Commit bypass: pre-commit comment-policy hook runs base transition mode, which cannot honor the refreshed new-scheme digests until an oracle review covers this head (review-gated self-registration). Hooks verified manually: ruff check/format clean, 959 focused tests pass, covered gate reports 0 violations.
…ng transition CI runs the base checker against the head registry, so registry entries must keep matching the base whole-tree digest until the scoped scheme is the base. Accept a fixture entry when its payload matches either the scoped digest or the legacy whole-tree digest, and keep the registry on whole-tree payloads. Proven green three ways: head base-mode 0 violations, head covered-mode 0 violations, and base-code simulation (base scripts against this tree) 0 violations.
…te ranges The oracle-review check also succeeds for changes that need no review, so check success alone lets a pull request authorize its own exceptions. Require an actual successful oracle verdict review on the head commit, by the oracle bot account, in addition to the bound check run. Pin candidate scope notices inside the byte range already pinned at the base and forbid partial-to-whole escalation, so a review cannot widen an exclusion through a candidate notice.
…ire CI token Fall back to the whole-file syntax-tree digest when a consumer reader touches parameters, attributes, or module bindings mutated inside compound statements or via non-name targets. Grant read permissions and wire GITHUB_TOKEN to the comment-policy CI workflow so check and pull request queries can evaluate review status.
85d5a13 to
6e4b03f
Compare
|
/oracle-review |
Pull request was converted to draft
There was a problem hiding this comment.
oracle-verdict: failure for 6e4b03fc5bd8dd194dd0414079b18df0acb9cf85
This result supersedes those for earlier heads; their open review threads still apply.
Tier: very-high (large multi-area diff: 1560 code lines across 4 areas).
Decision: SHIP WITH FIXES.
Reviewer: opus (claude claude-opus-5-5, effort medium).
D2 is fixed. The comment-policy job now passes GITHUB_TOKEN and grants checks:read and actions:read. run_comment_policy.py hands the environment through to the base checker, so oracle_review_covers() can actually reach the GitHub API in CI.
D1 is only partly fixed. The scoped digest now falls back to the whole file in these cases:
- the reader touches self/attribute state;
- a parameter shares a name with a module-level binding;
- module-level expressions mutate a binding through an attribute or subscript.
These channels are still not covered:
- Enclosing-function closures: _locally_bound treats any binding in an enclosing scope as local.
- Class-level pytest parametrize decorators and class-scoped fixtures.
- Fixtures registered under a different name, such as pytest.fixture(name=...).
- Module-level calls that mutate a binding passed to them.
There is also a new bug in _tree_mutates_scoped_bindings. It skips the targets of every module-level Assign, AnnAssign and AugAssign statement. As a result, CMD[0] = ... and CFG.attr = ... at module level are never flagged as mutations. They are also not in the binding index, so they never enter the digest. The four tests meant to prove this fallback all put the consumer at module level, where the code takes the whole-file path anyway, so they pass without exercising the check.
The rest of the change looks sound:
- Candidate notices are range-checked against the base pins.
- The legacy whole-file digest fallback can only match something stricter.
- Removed fixture entries are rescanned as revoked paths in transition mode, so CI would catch any finding that is still live.
Verified fixed: D2. Resolve their review threads.
Still open from earlier reviews
- Medium
scripts/comment_syntax.py:440-501: D1: Scoped fixture digest misses mutations, self/class state and pytest fixtures that feed the executed input
Scope: files changed since head 85d5a131f3c7e2b361ad52360685d08f01d551f5 was reviewed.
| if node is not tree and parent is tree and isinstance(node, (ast.Assign, ast.AnnAssign, ast.AugAssign)): | ||
| for child in ast.iter_child_nodes(node): | ||
| if child not in node.targets if isinstance(node, ast.Assign) else child is not node.target: | ||
| stack.append((child, node, in_compound)) | ||
| continue |
There was a problem hiding this comment.
Medium: Module-level subscript/attribute assignments escape both the mutation check and the scoped digest
_tree_mutates_scoped_bindings skips the targets of every module-level Assign, AnnAssign and AugAssign, so it never visits the Name inside CMD[0] = "b" or CFG.cmd = "x" (data name), or inside Runner.cmd = "x" (code name). The data_names Subscript/Attribute rule at line 530 and the code_names Store rule at lines 532-535 therefore never fire for these statements. The statements are not in the index either, because _module_binding_names only collects Store Names and the base Name of a subscript or attribute target is a Load. So in CMD = ["a"]; CMD[0] = "b"; def run(): exec(CMD[0]), editing the second line changes what run() executes but leaves the fixture digest unchanged, and the registered exemption keeps matching. The tests at tests/unit/scripts/test_comment_policy.py:1860-1863 put the exec at module level (symbol ""), so they take the whole-file path and never exercise this code. Fix: inspect non-Name targets of module-level assignments (or skip only bare Name targets), and add tests whose consumer is a function.
|
Superseded by #3019, which carries only the review-gated registration of same-PR exceptions and scope-notice pins (the verdict lookup, candidate-notice validation, the CI token and permissions, and the matching procedure text). Scoped fixture digests are not carried forward; the unresolved review thread on module-level subscript and attribute targets in the scoped digest applied only to that part. The branch fix/comment-policy-catch22-fossil is left in place if that work is picked up later. |
No description provided.