Repository navigation
Scope strict comment-policy check to owned paths and refresh pinned wheel - #2915
joeharris76 wants to merge 2 commits into
Conversation
![benchbox-oracle[bot]](https://avatars.githubusercontent.com/u/319264189?s=60&v=4){kind=small}
There was a problem hiding this comment.
oracle-verdict: failure for 26cdc9088a76cf4b7b9775e5d2057dd53daf4885
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
The wheel bump itself matches pyproject/uv.lock 0.9.1, but the PR weakens the gates around it: wheels become unscanned, strict trusts candidate HEAD, staged validation reads the wrong revision, and the unblockable mode-flip PR loses its prescribed manual check. Fix those before merge.
Reviewers not available
- sol: absent (error)
- luna: absent (error)
| decoded = {} | ||
| errors = [] | ||
| for path, raw in sources.items(): | ||
| if path.endswith(".whl"): |
There was a problem hiding this comment.
High: Blanket .whl skip bypasses ownership and coverage enforcement
Any path ending in '.whl' is silently skipped before external/ownership and coverage handling. Under owned roots such as '_project/scripts/', language() returns 'unsupported' for '.whl' so source_paths() inventories wheels; previously an undecodable or unexpected wheel produced a coverage-error (which introduced() always enforces) or was gated by external/external_members logic. Now a tampered, unpinned, or newly added wheel is invisible in strict, report, and transition comparison modes. Gate the skip on verified ownership exclusion instead of blanket-skipping.
| if mode == "strict" and not requested: | ||
| return git(root, "rev-parse", "HEAD").decode().strip() |
There was a problem hiding this comment.
High: Strict ownership base defaults to mutable candidate HEAD
When strict mode has no explicit --ownership-base, ownership_base() shells out to 'git rev-parse HEAD' and ownership_exclusions() then loads trusted_external from HEAD:quality/comment-policy.json and verifies blobs at HEAD. The candidate therefore attests its own exclusions, whereas strict previously used no ownership exclusions (None). The Makefile wires this in explicitly. A candidate can self-exempt paths in the diagnostic reviewers are told to use before merge. Default strict to no ownership exclusions or to an immutable base, not HEAD.
| .PHONY: comment-policy-strict | ||
| comment-policy-strict: | ||
| uv run -- python scripts/check_comment_policy.py --mode strict | ||
| uv run -- python scripts/check_comment_policy.py --mode strict --ownership-base "$(shell git rev-parse HEAD)" |
There was a problem hiding this comment.
High: Strict Makefile target trusts candidate HEAD for ownership
The strict target passes --ownership-base "$(shell git rev-parse HEAD)", pinning the ownership manifest to the mutable candidate commit. Combined with the checker default, strict honors candidate-controlled external/ownership entries rather than reporting whole-inventory debt. This makes the pre-merge diagnostic under-report the same violations CI comparison would otherwise surface via the base intersection. Remove the HEAD wiring so strict does not trust the candidate.
| if entry["path"].endswith(".whl") and not (root / entry["path"]).is_file(): | ||
| raise ValueError(f"external wheel missing: {entry['path']}") |
There was a problem hiding this comment.
Medium: Pinned-wheel existence check ignores staged revision
The new wheel check uses working-tree '(root / path).is_file()' while main() loads the policy from the index ('git show :quality/comment-policy.json') under --staged and reads staged sources. A staged policy bump with a differing worktree (or vice versa) therefore yields a false pass or false failure, making the validation verdict unverifiable. It also checks only existence, not the pyproject/uv.lock binding and content hash that immutable_dependency_artifacts() requires, while scan_sources() now skips wheels entirely. Check the same revision being evaluated and verify the pinned binding.
| Moving from `advisory` to `blocking` is a one-line change to the policy. It is | ||
| checked against the base policy, so that change is not blocked by itself, and | ||
| the next pull request is. The pull request that flips the mode is therefore | ||
| exempt by construction: its findings are reported but cannot fail it, so check | ||
| it with `make comment-policy-strict` before merging. A policy cannot be moved back from `blocking` to | ||
| the next pull request is. A policy cannot be moved back from `blocking` to | ||
| `advisory`. Flip it after the open pull requests have merged or been cleaned, | ||
| so that no one meets the new rule on a branch that was started before it | ||
| existed. |
There was a problem hiding this comment.
Medium: Mode-flip pre-merge check instruction removed
The deleted paragraph documented that the advisory-to-blocking flip PR is exempt by construction and must be checked with 'make comment-policy-strict' before merging. With CI unable to fail that PR and strict simultaneously weakened (HEAD ownership manifest plus .whl skip), removing the instruction leaves the one unblockable mode flip with no prescribed pre-merge validation. Restore the flip-PR check instruction or an equivalent enforced gate.
There was a problem hiding this comment.
oracle-verdict: failure for 85f924e3c78472c85cfee5ef7c54de65449bbebd
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Strict/ownership bypasses remain and two new staged/ownership gaps in the pinned-wheel fix block merge; wheel-bump content can evade ownership while staged provenance checks read the wrong revision.
Verified fixed: D4, D5. Resolve their review threads.
Still open from earlier reviews
- High
scripts/check_comment_policy.py:233: D1: Blanket .whl skip bypasses ownership and coverage enforcement - High
scripts/check_comment_policy.py:200-201: D2: Strict ownership base defaults to mutable candidate HEAD - High
Makefile:458: D3: Strict Makefile target trusts candidate HEAD for ownership
Scope: files changed since head 26cdc9088a76cf4b7b9775e5d2057dd53daf4885 was reviewed.
| if path.endswith(".whl") and any(path == e["path"] for e in policy["external"]): | ||
| continue |
There was a problem hiding this comment.
High: Pinned-wheel scan skip bypasses base-trusted ownership manifest
When ownership_exclusions is active, scan_sources only honors ownership_excluded_members, then unconditionally skips any .whl exactly matching policy external. A pinned wheel absent from the base-trusted ownership set therefore escapes both ownership enforcement and scanning. Combined with the wheel-bump ratchet allowance for the same distribution under a new filename, a swapped wheel version is verified only for existence, never for ownership or content hash. Require ownership-set membership before skipping.
|
|
||
|
|
||
| def validate_consumers(root: Path, policy: dict) -> None: | ||
| def validate_consumers(root: Path, policy: dict, staged: bool = False) -> None: |
There was a problem hiding this comment.
Medium: Staged validation checks worktree instead of index for provenance
In staged mode sources come from the index via git show :path and wheels are checked via git cat-file -e :path, but exception-consumer and external-provenance existence still use worktree (root / path).is_file(). A staged deletion of provenance/consumer is masked by the worktree copy, and a staged-only addition is rejected, so the staged gate verifies the wrong revision. Check the index when staged is true.
There was a problem hiding this comment.
oracle-verdict: failure for 7d47cdcc3000312a3c66364c1150e8133444de74
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
New pinned-wheel logic does not repair the prior gate-weakening issues: strict ownership still defaults to mutable candidate HEAD and the .whl scan skip is still self-attested from candidate-controlled pyproject/uv.lock/wheel bytes rather than a base-trusted pin, so a forged wheel plus matching candidate lock entries skips coverage. Staged consumer validation now reads the index and the blanket .whl skip is now conditional. No distinct new must-fix defect found in the two changed-since files beyond the carried-over priors; the remaining priors must be fixed before merge.
Verified fixed: D1, D7. Resolve their review threads.
Still open from earlier reviews
- High
scripts/check_comment_policy.py:200-201: D2: Strict ownership base defaults to mutable candidate HEAD - High
Makefile:458: D3: Strict Makefile target trusts candidate HEAD for ownership - High
scripts/check_comment_policy.py:241-242: D6: Pinned-wheel scan skip bypasses base-trusted ownership manifest
Scope: files changed since head 85f924e3c78472c85cfee5ef7c54de65449bbebd was reviewed.
7d47cdc to
6e1e434
Compare
There was a problem hiding this comment.
oracle-verdict: failure for 6e1e4342e29ef51e50fca8452a932c49e3be9d92
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
The strict-mode base derivation is fixed, but D6 remains: the wheel skip still bypasses the ownership manifest with weaker checks. Four new must-fix defects block merge: an unverified wheel-bump path swap, silent strict fallback to self-attested externals, an unanchored --ownership-base override, and the residual wheel-skip bypass.
Verified fixed: D2, D3. Resolve their review threads.
Still open from earlier reviews
- High
scripts/check_comment_policy.py:241-242: D6: Pinned-wheel scan skip bypasses base-trusted ownership manifest
Defects without a review thread
- Critical
scripts/check_comment_policy.py:97-99: D8: Wheel-bump ratchet exemption permits unverified replacement wheel bytes
check_ratchet exempts a wheel-path swap on directory, dist prefix, owner and provenance only (97-99), with no hash, lock-binding, dependency-membership or dist-info METADATA verification for the NEW path. wheel_lock_verified cannot verify new paths because the base revision lacks the file (base_raw is None at 363-365 returns False), the ownership manifest only covers base entries, and validate_consumers checks only presence (322-323). This PR's own 0.8.1->0.9.1 swap (quality/comment-policy.json:76) therefore introduces executable wheel bytes the gate never integrity-checks. Verify the new wheel's sha256 against the candidate uv.lock wheels entry, the tool.uv.sources path binding, project dependencies membership, and dist-info Name/Version before exempting.
Defects outside the diff count like the others: fix them before merge.
| if mode == "strict": | ||
| try: | ||
| return git(root, "merge-base", "HEAD", "origin/develop").decode().strip() | ||
| except subprocess.CalledProcessError: | ||
| return None |
There was a problem hiding this comment.
High: Strict ownership silently degrades to self-attested externals when origin/develop is unavailable
When 'git merge-base HEAD origin/develop' fails (no origin/develop, fetch failure, no common ancestor), ownership_base returns None (204-206), ownership_exclusions(None) returns None (211-212), and scan_sources with ownership_members None falls back to honoring the candidate's own external list (243-245). Strict mode then reports a self-attested result while docs/internal/comment-policy.md:59-60 claims it applies the base-trusted ownership manifest. A review run without the remote gets a false-clean diagnostic. Fail closed (raise) instead of returning None.
| if requested: | ||
| return requested |
There was a problem hiding this comment.
High: Explicit --ownership-base lets strict mode load an attacker-chosen manifest
Any --ownership-base value is returned unchecked for strict/report (200-201); the only later validation requires it be an ancestor of HEAD (216), so any PR-branch-only ancestor qualifies, and trusted externals are loaded from that commit (217). Strict mode then applies a candidate-influenced manifest while documented as base-trusted. Anchor the base to origin/develop history (e.g. ancestor of the merge-base) or reject the flag in strict mode.
| if path.endswith(".whl") and wheel_lock_verified(root, path, raw, policy, revision): | ||
| continue |
There was a problem hiding this comment.
High: Pinned-wheel scan skip bypasses the base-trusted ownership manifest
The skip at 246-247 applies whenever wheel_lock_verified passes, without requiring the path be in ownership_excluded_members. wheel_lock_verified (348-380) omits checks that immutable_external_ownership/immutable_dependency_artifacts enforces: project dependencies membership, dist-info METADATA Name/Version match, and candidate-side binding equality (cf. scripts/check_comment_cleanup_scope.py:1073-1096,1098-1113). A wheel whose base-revision pin metadata matches but whose candidate binding drifted or whose metadata mismatches can be hidden from the scan even when the ownership manifest withholds exclusion. Gate the skip on ownership exclusion or replicate the full checks including candidate binding.
…hip manifest and track the current todo-db wheel make comment-policy-strict failed on a clean develop checkout for two reasons. It ignored the ownership manifest, so vendored sources and tracked binaries were reported as first-party findings, and the policy still named the superseded todo-db 0.8.1 wheel while the repository vendors 0.9.1. Strict mode now derives its ownership base from the merge-base with origin/develop, which is HEAD on a clean develop checkout, and fails when that merge-base cannot be derived instead of silently scanning with the candidate's own exclusions. An explicit --ownership-base must be an ancestor of the merge-base. Exception consumers, external provenance and pinned vendored wheels are checked against the index when validating staged changes, and a pinned wheel whose file is missing is a configuration error. The policy entry now names the 0.9.1 wheel. The documentation describes strict as a diagnostic that applies the manifest, and replaces the instruction to run strict before the mode flip with reproducing the CI comparison against the base commit.
6e1e434 to
8803540
Compare
There was a problem hiding this comment.
oracle-verdict: failure for 8803540a0a4ae7c856d60faddd7dd0d82c364ca7
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (touches 2 areas).
Decision: SHIP WITH FIXES.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Incremental change fixes strict fail-closed default and base-trusted scan gating, but D8 (unverified replacement wheel bytes via ratchet exemption; presence check is not content verification) and D10 (attacker-chosen --ownership-base accepted whenever origin/develop is unavailable) remain open and block merge. No additional new must-fix defects in the changed files.
Verified fixed: D6, D9, D11. Resolve their review threads.
Still open from earlier reviews
- Critical
scripts/check_comment_policy.py:97-99: D8: Wheel-bump ratchet exemption permits unverified replacement wheel bytes - High
scripts/check_comment_policy.py:200-201: D10: Explicit --ownership-base lets strict mode load an attacker-chosen manifest
…hash and metadata, and anchor explicit ownership bases The ratchet lets a policy entry for a vendored wheel move to a new filename of the same distribution. It checked only the directory, distribution prefix, owner and provenance, so any bytes under the new name were accepted. A bumped wheel must now be bound by the owning pyproject.toml through tool.uv.sources, be a declared dependency, appear in uv.lock exactly once with its filename, hash and version, and carry dist-info METADATA whose name and version match. The files are read from the index when validating staged changes. This does not make the bump base-trusted, since a bump legitimately changes the wheel, the pyproject and the lock together, but it makes the three agree and leaves the bump to the required review of the policy file. An explicit --ownership-base is now rejected when origin/develop is not available to anchor it, so strict cannot load a manifest from a commit that cannot be tied to the base history.
There was a problem hiding this comment.
oracle-verdict: pending for f5be7494b0c6f6e55e79a802968eb16836953bfc
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (touches 2 areas).
Scope: files changed since head 8803540a0a4ae7c856d60faddd7dd0d82c364ca7 was reviewed.
Reviewers not available
- muse: absent (invalid)
- sol: absent (error)
- luna: absent (error)
- sonnet: skipped: same family as the author (claude)
- agy: disabled (CI authentication and quota output are not yet calibrated, and the workflow has no agy job)
There was a problem hiding this comment.
oracle-verdict: pending for f5be7494b0c6f6e55e79a802968eb16836953bfc
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (touches 2 areas).
Scope: files changed since head 8803540a0a4ae7c856d60faddd7dd0d82c364ca7 was reviewed.
Reviewers not available
- muse: absent (invalid)
- sol: absent (error)
- luna: absent (error)
- sonnet: skipped: same family as the author (claude)
- agy: disabled (CI authentication and quota output are not yet calibrated, and the workflow has no agy job)
There was a problem hiding this comment.
oracle-verdict: success for f5be7494b0c6f6e55e79a802968eb16836953bfc
This result supersedes those for earlier heads; their open review threads still apply.
Tier: medium-high (touches 2 areas).
Decision: SHIP.
Reviewer: muse (muse muse-spark-1.3-contributor, effort medium).
Wheel-bump verification and strict-ownership anchoring close the two prior holes without introducing a new merge-blocking defect; the 0.8.1 to 0.9.1 bump is consistent with the new binding, lock-hash and METADATA checks.
Verified fixed: D8, D10. Resolve their review threads.
Scope: files changed since head 8803540a0a4ae7c856d60faddd7dd0d82c364ca7 was reviewed.
This branch was successfully deployed
1 participant
Summary
make comment-policy-strictfailed on a clean develop checkout for two reasons: it ignored the ownership manifest, so vendored sources and tracked binaries were reported as first-party findings, andquality/comment-policy.jsonstill named the superseded todo-db 0.8.1 wheel while the repository vendors 0.9.1.origin/develop, which isHEADon a clean develop checkout. It fails when that merge-base cannot be derived, instead of scanning with the candidate's own exclusions. An explicit--ownership-basemust be an ancestor of the merge-base.docs/internal/comment-policy.mddescribes strict as a diagnostic that applies the base-trusted manifest, and replaces the instruction to run strict before the mode flip with reproducing the CI comparison against the base commit.This replaces an earlier version of the branch that added a wheel-skip path in the scanner. That path needed its own lock and hash verification and kept opening bypasses, so it is gone: with the policy entry corrected and the manifest applied, no wheel needs skipping on develop.
Verification
make pr-preflightpasses (823 tests for changed files);tests/unit/scripts/test_comment_policy.py, the newtest_comment_policy_strict_ownership.pyandtests/unit/test_ledger_coverage.pypass together (833 tests).origin/developat this head):strictreports 0 violations over 7040 source files.test_comment_policy.pynow publishrefs/remotes/origin/develop, which strict requires.