Skip to content

security-evidence guard decides from the lane's job log instead of its declared verdict #2541

Description

@kyle-sexton

Problem

scripts/verify-security-review-evidence.sh decides whether a security review actually happened by downloading the lane's job log (gh run view --log) and grepping it, and by re-deriving the PR's security scope in its own Python fnmatch implementation. Both channels are wrong in the same way: they infer a verdict the lane already computed.

The log grep is what #2517 was. The lane's Report review outcome step is an inline github-script whose SOURCE is echoed into the same log and contains the skip phrases as string literals, so the unanchored grep matched every successful in-scope pull request — the guard reddened exactly the ones it exists to approve. Anchoring to ##[warning]/##[error] lines removed the false positive but left the guard coupled to log text that nothing upstream pins, so the next wording change re-breaks it silently.

The local scope matcher is the same shape of problem. The lane matches with git check-ignore (gitignore semantics); this script matches with fnmatch plus hand-rolled ** handling, so the two disagree on exactly the patterns that distinguish them. It is also where the set -e scope-verdict defect lived: a helper that signalled out-of-scope by returning non-zero, called bare under errexit, killed the script and turned every out-of-scope PR red.

The self-tests could not catch either, because they re-implement the guard's regexes inside the harness and assert on the copies rather than running the guard.

Proposal

Consume the lane's declared outputs, which melodic-software/ci-workflows#461 adds to the reusable: relevant, review-ran, review-failed, failure-class, plus needs.security-review.result.

  • Delete the log read and the local Python matcher outright.
  • Fail CLOSED when the lane declares no verdict at a head that has not moved — the one legitimate absent-verdict cause is a run retired as superseded, and that is positively identified by comparing the live head against the event head rather than assumed.
  • Keep deferring on an external failure: the lane rules that GREEN on purpose so a provider outage does not lock every merge, and overturning it is a separate policy decision.
  • Rewrite the self-tests to EXECUTE the guard against environment fixtures.

Requires re-pinning the caller to the ci-workflows release carrying those outputs; the guard fails closed if pinned backwards past it.

Acceptance

  • scripts/verify-security-review-evidence.sh contains no gh run view --log and no local paths matcher, enforced by static checks in its own test file.
  • The self-tests run the guard end to end and cover: review ran, validation skip, external failure, out of scope, skipped job, skip-listed actor, absent verdict at a moved head, and absent verdict at an unmoved head.
  • The tests run in CI (they currently run nowhere).

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions