Skip to content

Commit 15dd8f0

Browse files
Merge pull request #694 from navapbc/fix/c9e9-empty-net-diff-exemption
fix(review-gate): in-gate empty-net-diff exemption for clean merge commits (c9e9)
2 parents 5957d3b + 174a612 commit 15dd8f0

8 files changed

Lines changed: 341 additions & 51 deletions

plugins/dso/scripts/ci/fp-recovery-audit-sweep.sh

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -43,30 +43,31 @@ fi
4343
# shellcheck source=../lib/review-coverage-lib.sh
4444
source "$_FPA_LIB"
4545

46-
# ── 0cd7 DD3: exempt-merge scaffolding (the audit had none before this story) ──
46+
# ── 0cd7 DD3 + c9e9: exempt-merge scaffolding (the audit had none before this) ──
4747
# A merged main PR whose content carries NO reviewable application code is NOT a
48-
# bypass — flagging it would be audit noise. Two diff-scoped exemptions, IDENTICAL
49-
# to the other two consumers (DD6 equivalence test):
50-
# (a) a CLEAN merge commit (>=2 parents, empty combined diff) — its parents are
51-
# reviewed independently;
48+
# bypass — flagging it would be audit noise. Two diff-scoped exemptions, computed by
49+
# the SAME shared predicates as the other two consumers (DD6 equivalence test) — NO
50+
# parallel implementation:
51+
# (a) a genuinely-empty-net MERGE commit (shared rc_diff_is_empty_net: >=2 parents,
52+
# diff-tree rc==0, empty combined diff) — its parents are reviewed independently;
5253
# (b) a TICKETS_ONLY commit (shared rc_diff_is_tickets_only) — diff entirely in
5354
# the ticket store.
55+
# Replaces the former INLINE clean-merge check, which duplicated the empty-net logic
56+
# (and masked diff-tree's rc inside the command substitution); the shared predicate
57+
# captures rc separately and is DD6-equivalent across consumers.
5458
# AUDIT ERROR POSTURE (distinct from coverage's fail-closed-BLOCK): this is a
5559
# non-blocking REPORTING tool, so it must POSITIVELY CONFIRM the exemption to skip a
56-
# PR. Any doubt — local commit not checked out, git error, rc_diff returns 2 — yields
57-
# NOT-exempt, so the PR is RECORDED for manual follow-up rather than silently dropped.
58-
# Requires the merge SHA to be present locally (CI: actions/checkout fetch-depth 0);
59-
# when it is not, the PR is (correctly) recorded.
60+
# PR. Any doubt — local commit not checked out, git error, predicate returns 2 —
61+
# yields NOT-exempt (the predicate returns non-zero), so the PR is RECORDED for manual
62+
# follow-up rather than silently dropped. Requires the merge SHA present locally
63+
# (CI: actions/checkout fetch-depth 0); when it is not, the PR is (correctly) recorded.
6064
_fpa_is_exempt_merge() {
61-
local _s="${1:-}" _parents _cc
65+
local _s="${1:-}"
6266
[[ -z "$_s" ]] && return 1
6367
git rev-parse --verify --quiet "${_s}^{commit}" >/dev/null 2>&1 || return 1 # not local -> record
64-
# (a) clean merge: >=2 parents AND empty combined diff (positive-confirm only).
65-
_parents="$(git rev-list --parents -n1 "$_s" 2>/dev/null)" || return 1
66-
if [[ "$(printf '%s' "$_parents" | wc -w)" -ge 3 ]]; then
67-
_cc="$(git diff-tree --cc --no-commit-id "$_s" 2>/dev/null)" || return 1
68-
[[ -z "$_cc" ]] && return 0 # clean merge -> exempt
69-
return 1 # evil merge -> record
68+
# (a) genuinely-empty-net merge (shared predicate; rc 0 = exempt, 1/2 = record).
69+
if declare -F rc_diff_is_empty_net >/dev/null 2>&1 && rc_diff_is_empty_net "$_s"; then
70+
return 0
7071
fi
7172
# (b) tickets-only (shared predicate; rc 0 = exempt, 1/2 = record).
7273
declare -F rc_diff_is_tickets_only >/dev/null 2>&1 || return 1

plugins/dso/scripts/ci/review-coverage-invariant.sh

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ if [[ -z "$_SHAS" ]]; then
115115
exit 0
116116
fi
117117

118-
_total=0; _ledger_hits=0; _verified=0; _unreviewed=0; _errors=0; _exempt_tickets=0
118+
_total=0; _ledger_hits=0; _verified=0; _unreviewed=0; _errors=0; _exempt_tickets=0; _exempt_empty_net=0
119119
_violations=""
120120

121121
# cca8 (linear-history cutover, DD3): the clean-merge exemption (_is_clean_merge)
@@ -150,6 +150,18 @@ while IFS= read -r _sha; do
150150
_exempt_tickets=$(( _exempt_tickets + 1 ))
151151
continue
152152
fi
153+
# Genuinely-empty-net merge exemption (c9e9): a >=2-parent merge whose COMBINED
154+
# net diff is genuinely empty (e.g. a clean staged->main 2-parent merge / clean
155+
# octopus) carries no reviewable content, yet no MERGED PR covers it (A3b excludes
156+
# the sub-PR whose merge_commit_sha IS this SHA) — a false UNREVIEWED wedge.
157+
# Computed by the shared rc_diff_is_empty_net (review-coverage-lib.sh) so all three
158+
# consumers agree (DD6). Only rc 0 (proven empty-net) exempts; rc 1 (non-empty /
159+
# non-merge) OR rc 2 (uncomputable) falls through to the normal coverage path,
160+
# which itself fails closed — so an error here can never launder an unreviewed SHA.
161+
if rc_diff_is_empty_net "$_sha"; then
162+
_exempt_empty_net=$(( _exempt_empty_net + 1 ))
163+
continue
164+
fi
153165
# Perf prefilter: a ledger HIT (proven reviewed in a prior run) skips
154166
# re-verification. This is a ledger hit, NOT reachability-to-main.
155167
if _ledger_has "$_sha"; then
@@ -177,7 +189,7 @@ while IFS= read -r _sha; do
177189
esac
178190
done <<< "$_SHAS"
179191

180-
echo "review-coverage-invariant: ${_total} SHA(s) in ${_BASE_REF}..${_HEAD} — verified=${_verified} exempt_tickets=${_exempt_tickets} ledger_hits=${_ledger_hits} unreviewed=${_unreviewed} errors=${_errors}"
192+
echo "review-coverage-invariant: ${_total} SHA(s) in ${_BASE_REF}..${_HEAD} — verified=${_verified} exempt_tickets=${_exempt_tickets} exempt_empty_net=${_exempt_empty_net} ledger_hits=${_ledger_hits} unreviewed=${_unreviewed} errors=${_errors}"
181193

182194
# 3ebb DD1: resolve the per-SHA tallies through the tristate lattice. A genuine
183195
# unreviewed SHA is FAIL (1, the safe bottom — never retried/downgraded); a

plugins/dso/scripts/lib/review-coverage-lib.sh

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,71 @@ rc_diff_is_tickets_only() {
143143
return 0
144144
}
145145

146+
# ── c9e9: shared COMBINED-DIFF-EMPTY MERGE exemption predicate ───────────────
147+
# rc_diff_is_empty_net <sha>
148+
#
149+
# A commit is EXEMPT from per-SHA review coverage IFF it is a MERGE commit that
150+
# adds NO content of its OWN beyond its parents — i.e. its combined diff
151+
# (`git diff-tree --cc`) is empty (a clean 2-parent staged->main merge, a clean
152+
# octopus). NOTE: "combined-diff-empty" is NOT the same as "no net change over
153+
# main": a clean --no-ff feature merge (feature adds X, main adds Y, no overlap)
154+
# has an empty --cc yet introduces X relative to its first parent. The exemption
155+
# is sound anyway because it covers ONLY the merge commit's own (zero) content —
156+
# the merge's content-bearing PARENTS are independently enumerated by the
157+
# base..head coverage/provenance walk and must EACH be proven reviewed (guarded
158+
# by T13 / t15b). So this predicate must NEVER be read as "the merge carries no
159+
# reachable new code"; it asserts only "the merge commit itself resolves no
160+
# conflicts and introduces no own content". Such a merge carries no reviewable
161+
# content of its own, yet it fail-closes the coverage invariant and the
162+
# provenance walk because no MERGED PR covers it (A3b excludes the sub-PR whose
163+
# merge_commit_sha IS that SHA) — a false wedge (bug c9e9). This is the ONE shared
164+
# helper sourced by the coverage consumers (review-coverage-invariant.sh,
165+
# verify-session-provenance.sh, fp-recovery-audit-sweep.sh) so they compute the
166+
# IDENTICAL exemption (DD6 equivalence test guards divergence).
167+
#
168+
# SAFETY CRUX — rc-vs-emptiness disambiguation (preserve EXACTLY): emptiness ALONE
169+
# never proves "genuinely empty". `git diff-tree --cc --no-commit-id --name-only`
170+
# ALSO emits empty output when the SHA is uncomputable (bad sha, rc=128). So the
171+
# exit code MUST be captured SEPARATELY from emptiness and checked === 0 before
172+
# trusting the empty list. NEVER place diff-tree in a pipe that masks its rc.
173+
# - merge commit, diff-tree rc==0, EMPTY file list -> 0 EXEMPT (genuinely empty)
174+
# - merge commit, diff-tree rc==0, NON-empty list -> 1 NOT exempt (content/
175+
# conflict-resolution/evil merge — has net change)
176+
# - non-merge / single-parent SHA -> 1 NOT exempt (a single-
177+
# parent content commit must be proven-reviewed like any SHA)
178+
# - diff-tree rc!=0 (uncomputable / bad sha) -> 2 ERROR — caller MUST
179+
# fail closed (emptiness + rc!=0 = uncomputable, NEVER "genuinely empty")
180+
#
181+
# FAIL-OPEN VECTORS this closes by construction: (1) inferring "empty" from a bad
182+
# SHA's empty output (rc check blocks it); (2) exempting a content/evil merge that
183+
# resolves conflicts with its own changes (non-empty list blocks it); (3) exempting
184+
# a single-parent commit whose first-parent diff happens to be reported empty
185+
# (the >=2-parent gate blocks it). Mirrors rc_diff_is_tickets_only's contract and
186+
# the I-1 guard's discipline (emptiness is never silently read as exempt).
187+
#
188+
# Returns: 0 EXEMPT (>=2-parent merge, diff-tree rc==0, empty combined diff)
189+
# 1 NOT exempt (non-merge, OR rc==0 with a non-empty combined diff)
190+
# 2 ERROR (diff-tree rc!=0 / bad sha) — caller MUST fail closed
191+
rc_diff_is_empty_net() {
192+
local _sha="${1:-}" _files _rc _nparents
193+
[[ -z "$_sha" ]] && return 2
194+
# (a) Must be a merge commit: >=2 parents. `rev-list --parents -n1` prints the
195+
# SHA followed by each parent SHA, so >=3 whitespace-separated words == >=2
196+
# parents. A bad SHA makes rev-list fail -> ERROR (fail closed).
197+
_nparents="$(git rev-list --parents -n1 "$_sha" 2>/dev/null)" || return 2
198+
[[ -z "$_nparents" ]] && return 2
199+
[[ "$(printf '%s' "$_nparents" | wc -w)" -ge 3 ]] || return 1 # not a merge -> NOT exempt
200+
# (b)+(c) Combined diff over a merge. core.quotepath=false for literal paths,
201+
# consistent with rc_diff_is_tickets_only. CRITICAL: capture rc SEPARATELY (own
202+
# statement, NOT through a pipe) so an uncomputable diff is distinguishable from
203+
# a genuinely-empty one.
204+
_files="$(git -c core.quotepath=false diff-tree --cc --no-commit-id --name-only "$_sha" 2>/dev/null)"
205+
_rc=$?
206+
[[ $_rc -ne 0 ]] && return 2 # uncomputable / bad sha -> ERROR (fail closed)
207+
[[ -z "$_files" ]] && return 0 # genuinely-empty net merge -> EXEMPT
208+
return 1 # non-empty combined diff -> NOT exempt
209+
}
210+
146211
# rc_review_check_verdict (check-runs JSON on STDIN)
147212
# SINGLE SOURCE OF TRUTH for the poison-on-failure "did the review pass" verdict
148213
# over a SHA's check-runs. Any failure-class conclusion (failure/cancelled/

plugins/dso/scripts/verify-session-provenance.sh

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,26 @@ while IFS=' ' read -r sha subject; do
423423
continue
424424
fi
425425

426+
# Genuinely-empty-net merge exemption (c9e9). A >=2-parent merge whose COMBINED
427+
# net diff is genuinely empty (e.g. a clean staged->main 2-parent merge / clean
428+
# octopus) carries no reviewable application code, so provenance must NOT flag it
429+
# unprovenanced — no MERGED PR covers it (A3b excludes the sub-PR whose
430+
# merge_commit_sha IS this SHA). This transitively fixes the dispatcher's
431+
# empty-net-diff guard, which consumes unprovenanced-shas.txt. Computed by the
432+
# shared rc_diff_is_empty_net (review-coverage-lib.sh) so this gate agrees with the
433+
# coverage invariant and the fp-recovery sweep (DD6). The helper uses bare `git`
434+
# (cwd), so invoke it cd'd into GIT_REPO_PATH. Only rc 0 (proven empty-net)
435+
# exempts; rc 1 (non-empty/non-merge) OR rc 2 (uncomputable) falls through to the
436+
# normal covering-PR provenance path (which itself flags unprovenanced on doubt) —
437+
# so an error here can never launder a SHA.
438+
if declare -F rc_diff_is_empty_net >/dev/null 2>&1 \
439+
&& ( cd "$GIT_REPO_PATH" 2>/dev/null && rc_diff_is_empty_net "$sha" ); then
440+
echo "commit $sha status=EMPTY_NET_MERGE; exempt (genuinely-empty net merge diff)"
441+
_covered_shas+=("$sha")
442+
_cache_set "$sha" "provenanced" || true
443+
continue
444+
fi
445+
426446
# Identity-based admin exemption (ADR-0022): handled in the G3 covering-PR loop
427447
# below — a covering PR merged by a designated bypass-actor (server-set
428448
# merged_by ∈ set) counts as reviewed-equivalent there, so an admin web-UI
Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
#!/usr/bin/env bash
2+
# tests/scripts/test-rc-diff-is-empty-net.sh — c9e9 unit test
3+
#
4+
# Drives the PRODUCTION rc_diff_is_empty_net (review-coverage-lib.sh) over REAL git
5+
# fixtures. This is the SHARED "genuinely-empty net diff" exemption helper, a sibling
6+
# of rc_diff_is_tickets_only, recognizing a review-exempt class distinct from the
7+
# I-1 empty-file-list fail-closed case: a 2+-parent MERGE whose combined (--cc) diff
8+
# is EMPTY (rc==0, zero file entries) carries no net change to review.
9+
#
10+
# SAFETY CRUX (preserve exactly): rc==0 is MANDATORY. Emptiness alone NEVER implies
11+
# "genuinely empty" — an uncomputable diff-tree (bad sha, rc!=0) also produces empty
12+
# output and MUST be ERROR (2), so every caller fails closed on doubt.
13+
#
14+
# E1 clean 2-parent empty-net merge -> EXEMPT (0)
15+
# E2 clean octopus (3-parent) empty-net merge -> EXEMPT (0)
16+
# E3 evil 2-parent merge (own content in merge) -> NOT exempt (1)
17+
# NOTE: a CLEAN auto-merge of disjoint, non-conflicting changes has an EMPTY
18+
# --cc combined diff by definition (--cc only shows paths differing from ALL
19+
# parents), so it is correctly empty-net/EXEMPT — the not-exempt cases are the
20+
# merges that introduce OWN content: conflict-resolution (E4) and evil (E3/E6).
21+
# E4 conflict-resolution merge (own content) -> NOT exempt (1)
22+
# E5 single-parent content commit -> NOT exempt (1) (not a merge)
23+
# E6 evil octopus (own content in merge) -> NOT exempt (1)
24+
# E7 bogus / unknown SHA -> ERROR (2) (fail closed)
25+
26+
set -uo pipefail
27+
export GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null GIT_TERMINAL_PROMPT=0
28+
REPO_ROOT="$(git rev-parse --show-toplevel)"
29+
LIB="$REPO_ROOT/plugins/dso/scripts/lib/review-coverage-lib.sh" # shim-exempt: test sources the lib under test
30+
31+
PASS=0; FAIL=0
32+
_pass() { echo "PASS: $1"; PASS=$((PASS+1)); }
33+
_fail() { echo "FAIL: $1 ($2)"; FAIL=$((FAIL+1)); }
34+
35+
_W="$(mktemp -d "${TMPDIR:-/tmp}/dso-emptynet.XXXXXX")"; trap 'rm -rf "$_W"' EXIT
36+
R="$_W/repo"; mkdir -p "$R"
37+
(
38+
cd "$R" || exit 1
39+
git init -q -b main
40+
git config user.email t@e.st; git config user.name t; git config commit.gpgsign false
41+
echo base > README.md; git add README.md; git commit -q -m base
42+
base_sha="$(git rev-parse HEAD)"
43+
44+
# E1 clean 2-parent empty-net merge: a side branch whose change is identical to
45+
# what main already has (so the merge introduces NO net change in the combined diff).
46+
git checkout -q -b e1side "$base_sha"
47+
echo shared > shared.txt; git add shared.txt; git commit -q -m e1-side
48+
git checkout -q main
49+
echo shared > shared.txt; git add shared.txt; git commit -q -m e1-main
50+
git merge -q --no-ff -m "e1 empty-net merge" e1side || true
51+
git rev-parse HEAD > "$_W/E1"
52+
e1_after="$(git rev-parse HEAD)"
53+
54+
# E2 clean octopus (3-parent) empty-net merge. Two side branches that each add a
55+
# file ALSO added identically on main before the merge -> combined --cc diff empty.
56+
git checkout -q -b e2a "$e1_after"
57+
echo o1 > oct1.txt; git add oct1.txt; git commit -q -m e2a
58+
git checkout -q -b e2b "$e1_after"
59+
echo o2 > oct2.txt; git add oct2.txt; git commit -q -m e2b
60+
git checkout -q main
61+
echo o1 > oct1.txt; echo o2 > oct2.txt; git add oct1.txt oct2.txt; git commit -q -m e2-main
62+
git merge -q --no-ff -m "e2 octopus empty-net" e2a e2b || true
63+
git rev-parse HEAD > "$_W/E2"
64+
e2_after="$(git rev-parse HEAD)"
65+
66+
# E3 evil 2-parent merge: a 2-parent merge whose merge commit carries OWN content
67+
# (a file no parent has) -> --cc combined diff is NON-empty (differs from both
68+
# parents). A clean auto-merge of disjoint changes would be empty-net by --cc
69+
# semantics, so the not-exempt case must introduce content IN the merge itself.
70+
git checkout -q -b e3side "$e2_after"
71+
echo c3 > content3.txt; git add content3.txt; git commit -q -m e3-side
72+
git checkout -q main
73+
echo unrel > unrel3.txt; git add unrel3.txt; git commit -q -m e3-main
74+
git merge -q --no-ff --no-commit e3side >/dev/null 2>&1 || true
75+
echo evil3 > evil3.txt; git add -A
76+
git commit -q -m "e3 evil 2-parent merge"
77+
git rev-parse HEAD > "$_W/E3"
78+
e3_after="$(git rev-parse HEAD)"
79+
80+
# E4 conflict-resolution merge with OWN content: both sides edit the same line
81+
# differently; the merge commit carries a resolution -> non-empty combined diff.
82+
echo line > conflict.txt; git add conflict.txt; git commit -q -m e4-seed
83+
e4_seed="$(git rev-parse HEAD)"
84+
git checkout -q -b e4side "$e4_seed"
85+
echo sideval > conflict.txt; git add conflict.txt; git commit -q -m e4-side
86+
git checkout -q main
87+
echo mainval > conflict.txt; git add conflict.txt; git commit -q -m e4-main
88+
git merge -q --no-ff -m "e4 conflict merge" e4side >/dev/null 2>&1 || {
89+
echo resolved > conflict.txt; git add conflict.txt
90+
git commit -q --no-edit
91+
}
92+
git rev-parse HEAD > "$_W/E4"
93+
e4_after="$(git rev-parse HEAD)"
94+
95+
# E5 single-parent content commit (not a merge).
96+
echo e5 > five.txt; git add five.txt; git commit -q -m e5-single
97+
git rev-parse HEAD > "$_W/E5"
98+
e5_after="$(git rev-parse HEAD)"
99+
100+
# E6 evil octopus: octopus merge that carries OWN content (a file no parent has)
101+
# -> combined diff non-empty.
102+
git checkout -q -b e6a "$e5_after"
103+
echo a6 > e6a.txt; git add e6a.txt; git commit -q -m e6a
104+
git checkout -q -b e6b "$e5_after"
105+
echo b6 > e6b.txt; git add e6b.txt; git commit -q -m e6b
106+
git checkout -q main
107+
git merge -q --no-ff --no-commit e6a e6b >/dev/null 2>&1 || true
108+
echo evil > evil6.txt; git add -A
109+
git commit -q -m "e6 evil octopus"
110+
git rev-parse HEAD > "$_W/E6"
111+
) || { echo "FIXTURE SETUP FAILED"; exit 1; }
112+
113+
# shellcheck source=/dev/null
114+
source "$LIB"
115+
116+
_verdict() { # _verdict <sha> ; echoes rc
117+
( cd "$R" && rc_diff_is_empty_net "$1" >/dev/null 2>&1; echo $? )
118+
}
119+
120+
_assert() { # _assert <name> <got> <want>
121+
if [[ "$2" == "$3" ]]; then _pass "$1"; else _fail "$1" "rc=$2 want $3"; fi
122+
}
123+
_assert "E1 clean 2-parent empty-net merge -> exempt" "$(_verdict "$(cat "$_W/E1")")" 0
124+
_assert "E2 clean octopus empty-net merge -> exempt" "$(_verdict "$(cat "$_W/E2")")" 0
125+
_assert "E3 content merge -> not exempt" "$(_verdict "$(cat "$_W/E3")")" 1
126+
_assert "E4 conflict-resolution merge -> not exempt" "$(_verdict "$(cat "$_W/E4")")" 1
127+
_assert "E5 single-parent content commit -> not exempt" "$(_verdict "$(cat "$_W/E5")")" 1
128+
_assert "E6 evil octopus -> not exempt" "$(_verdict "$(cat "$_W/E6")")" 1
129+
_assert "E7 bogus SHA -> error (fail closed)" "$(_verdict "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")" 2
130+
131+
echo ""
132+
echo "=== test-rc-diff-is-empty-net.sh: PASS=$PASS FAIL=$FAIL ==="
133+
[[ $FAIL -eq 0 ]]

0 commit comments

Comments
 (0)