Skip to content

verify-security-review-evidence.sh shallows the base ref, so the guard fails with 'no merge base' on any branch behind main #2553

Description

@kyle-sexton

What's wrong

scripts/verify-security-review-evidence.sh:158 fetches the base ref shallow:

git fetch origin "$base_ref" --depth=1 >/dev/null 2>&1 || true

.github/workflows/claude-security-review.yml:93 already checks out with fetch-depth: 0, so the
full history is present before this line runs. The --depth=1 fetch then truncates that ref to a
single commit, and the scope check immediately asks for a three-dot diff against it:

["git", "diff", "--name-only", f"origin/{base_ref}...HEAD"]

A three-dot diff needs a merge base. Once origin/main is a lone commit with no ancestry, there is
none unless the PR branch happens to be based on main's current tip — so the guard dies with:

fatal: origin/main...HEAD: no merge base

Blast radius

Any PR whose branch is behind origin/main when the workflow runs — which, at this repo's merge
rate, is most PRs more than a few minutes old. Observed on #2539: reproduced across two independent
runs (31655807987 jobs 94310219602 and 94312908610), deterministic, not flaky. The branch was
7 commits behind main at the time.

The failure is fail-closed, which is the right default and is clearly deliberate in the script's own
comments — but the guard is erroring rather than evaluating, so it never reaches the evidence check
it exists to perform, and it is red on PRs it has nothing to say about. security-review-evidence is
not in the ruleset's required-checks list, so it does not block merges; it just goes red.

Fix

Drop --depth=1:

git fetch origin "$base_ref" >/dev/null 2>&1 || true

The workflow's fetch-depth: 0 checkout already supplies full history, so this is not a cost
regression — it is a normal fetch against an already-complete clone. Nothing about the guard's
fail-closed posture changes; it simply gets a usable merge base and evaluates scope as designed.

Worth deciding alongside the fix: whether the script should defend against being called from a
shallow clone at all (git rev-parse --is-shallow-repository--unshallow). Its only caller today
checks out full history, so that may be scaffolding for a case that does not exist — noting it as a
question rather than a requirement.

Verification

scripts/verify-security-review-evidence.sh.test.sh exists and should cover the behind-main case,
which currently has no test — a branch several commits behind its base should reach a scope verdict
rather than dying on the diff.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: securitySecurity-relevant: vulnerability, hardening, or disclosure follow-up.priority: needs-triageDefault until a priority tier is assigned.work-class: scopedA briefed fix or small feature; blast radius bounded by the brief, tests exist.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions