Skip to content

no-unreviewed-pr.py demands a reviewer request on redaction PRs, where granting it is the exposure the redaction gate exists to prevent #1392

Description

@d-morrison

hooks/no-unreviewed-pr.py fires on any PR opened or readied with no successful reviewer request after it, and offers exactly one legitimate deferral: the PR being a draft.

That misses a case where complying is actively harmful.

The conflict

A redaction PR carries the thing being redacted on the removed side of every hunk --- deleting a literal is the whole change --- so the reviewer's input is the secret. ucdavis/bcs#610 documents this concretely: the @claude reviewer quoted a network user id back into a PR comment while reviewing the PR that redacted it. ucdavis/bcs#614 merged a redaction-gate job that skips automatic AI review when a diff removes secret- or PHI-shaped lines.

The gate only covers the automatic path. An explicit requested_reviewers POST routes straight around it --- which is exactly what this hook instructs.

Live instance: ucdavis/bcs#615 removes 47 real participant identifiers. redaction-gate passed and ai-review reported skipping, as designed. The hook then fired demanding a Copilot request, whose only correct response is to refuse.

Why the existing deferral does not cover it

The draft carve-out is about timing --- a draft has nothing worth reviewing yet. This is about content: the PR is complete, wants human review, and must not reach an automated one. A session that drafts the PR to silence the hook makes things worse, since pr-on-claim notes a draft stalls its own ARDI loop.

Proposed fix

Add a second deferral condition, and prefer detecting it over trusting a session's say-so:

  1. Detect it. ucdavis/bcs already ships the detector --- .github/scripts/detect-redaction-diff.py reads a unified diff and reports whether any removed line matches a secret or PHI shape, reporting how many lines it examined rather than only how many matched. The hook could shell out to an equivalent, or accept a repo-declared marker.
  2. Failing that, an explicit escape, in the shape no-handrolled-verdict-parse.py already uses: an ALLOW_UNREVIEWED_REDACTION_PR=1 prefix, or a no-ai-review label on the PR (which ucdavis/bcs already honours in ai-code-review.yml).
  3. Say so in the message. The current text enumerates one deferral as though it were exhaustive, so a session that correctly refuses has no sanctioned way to record why, and the next one may comply.

Note the direction of the risk, per fail-fast: an over-warn here is safe and a wrong discharge is not, so the escape should be narrow and explicit rather than inferred loosely --- but no escape is worse than a narrow one, because the only currently-available responses are to leak or to lie about draft status.

Found 2026-08-10 driving ucdavis/bcs#615, the first real exercise of ucdavis/bcs#614's gate.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions