fix(review-gate): in-gate empty-net-diff exemption for clean merge commits (c9e9)#694
Conversation
…mmits (c9e9) In the two-tier ci-pr flow, a 2-parent PR1 merge commit (source->staged) with a GENUINELY-EMPTY net diff fail-closed the PR2 llm-review — at review-coverage- invariant.sh (no covering merged PR survives the A1/A3b self-merge guards -> UNREVIEWED) and transitively at the dispatcher's empty-net-diff guard (the SHA is written to unprovenanced-shas.txt). The real code commits were already identity- exempt (ADR-0022); only the content-free merge commit tripped the wedge, forcing a redundant web-UI bypass on #634/#671/#678. This false wedge gates the 3ee4 enforce-flip. Fix (588e panel's revised direction — in-gate exemption on the shared predicate, NOT the rejected HMAC-ledger / content-hash approaches): - New shared predicate rc_diff_is_empty_net in review-coverage-lib.sh. EXEMPT iff the SHA is a merge (>=2 parents) AND `git diff-tree --cc` exits 0 AND its file list is empty. Returns NOT-exempt for non-merge / non-empty (-cc) and ERROR (rc 2) when diff-tree exits non-zero — the exit code, captured separately from emptiness, is the genuinely-empty-net vs uncomputable disambiguator (empirically pinned: clean merge/octopus rc0+empty; content/conflict/evil/single-parent rc0+nonempty; bad sha rc128). Every uncertain case fails closed. - Wired additively + fail-closed (only rc==0 exempts) into all three DD6 shared- predicate consumers: review-coverage-invariant.sh, verify-session-provenance.sh (marks the SHA provenanced -> transitively fixes the dispatcher, which is NOT edited), and fp-recovery-audit-sweep.sh (its inline clean-merge check replaced by the shared predicate — removes a parallel implementation, restores DD6 one-predicate/N-consumers). - New unit test (7 fixtures incl. evil-octopus + bad-sha) + DD6 equivalence extended (empty-net exempt + evil-merge negative control). Anti-laundering controls preserved: an evil merge stays UNREVIEWED and an unreviewed content parent still blocks (the exemption covers only the content-free merge commit, never launders content). Three stale cca8 "clean merge never exempt" assertions retargeted to the c9e9 behavior (not weakened). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 51 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR introduces ChangesEmpty-net merge exemption predicate and consumers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| case "$kind" in | ||
| emptynet|evilmerge) _hfn=rc_diff_is_empty_net ;; | ||
| *) _hfn=rc_diff_is_tickets_only ;; | ||
| esac | ||
| # (1) Independently confirm the helper's verdict matches the fixture content. | ||
| hlp="$(_helper_verdict "$r" "$f")" | ||
| hlp="$(_helper_verdict "$r" "$f" "$_hfn")" | ||
| if [[ "$hlp" != "$exp" ]]; then | ||
| _fail "$name" "helper verdict=$hlp != expected=$exp — rc_diff_is_tickets_only wrong on fixture '$kind'" | ||
| return |
There was a problem hiding this comment.
💡 Quality: Hardcoded helper name in failure message misleads debugging
In the modified _run helper, the diff now selects _hfn between rc_diff_is_empty_net and rc_diff_is_tickets_only (lines 168-171) and calls it via _helper_verdict ... "$_hfn". However, the failure message at line 175 still hardcodes rc_diff_is_tickets_only wrong on fixture '$kind'. For the new emptynet/evilmerge fixtures the actual predicate under test is rc_diff_is_empty_net, so a failure on those fixtures would print a misleading helper name and could send a debugger to the wrong function. Use $_hfn in the message instead of the literal name.
Reference the actually-selected helper function ($_hfn) in the failure message.:
if [[ "$hlp" != "$exp" ]]; then
_fail "$name" "helper verdict=$hlp != expected=$exp — $_hfn wrong on fixture '$kind'"
return
fi
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/scripts/test-rc-diff-is-empty-net.sh`:
- Line 26: Change the shell options at the top of the test script so failures
abort and avoid pipefail: replace the current set invocation that uses
"pipefail" with one that includes "-euo" (i.e., enable errexit, nounset, and no
word-splitting protections) so that commands like "source \"$LIB\"" and fixture
setup will stop the script on error and you won't introduce pipefail-related
SIGPIPE issues with pipelines such as grep -q; update the line containing the
existing set command accordingly.
In `@tests/scripts/test-ticket-exemption-equivalence.sh`:
- Around line 147-149: _helper_verdict currently collapses all non-zero exit
codes to "not", hiding helper failures; change it to capture the helper's exit
code and return "exempt" for rc=0, "not" for rc=1, and "error" (and exit
immediately with non-zero) for any other rc so helper errors are visible and
tests fail fast; update both occurrences (the function at lines ~147 and the
similar block at ~173-177) to use this tri-state branching based on the captured
exit status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 847142b2-9d47-4075-8f50-c886e38900df
📒 Files selected for processing (8)
plugins/dso/scripts/ci/fp-recovery-audit-sweep.shplugins/dso/scripts/ci/review-coverage-invariant.shplugins/dso/scripts/lib/review-coverage-lib.shplugins/dso/scripts/verify-session-provenance.shtests/scripts/test-rc-diff-is-empty-net.shtests/scripts/test-review-coverage-invariant.shtests/scripts/test-ticket-exemption-equivalence.shtests/scripts/test-verify-session-provenance-cases.sh
| # E6 evil octopus (own content in merge) -> NOT exempt (1) | ||
| # E7 bogus / unknown SHA -> ERROR (2) (fail closed) | ||
|
|
||
| set -uo pipefail |
There was a problem hiding this comment.
Fix shell options: use set -euo for test scripts.
The shell options are incorrect in two ways:
- Missing
-e(errexit), so the script continues after errors — ifsource "$LIB"(line 114) or fixture setup fails, assertions would run against broken state, producing misleading results. - Includes
pipefail, which test scripts should omit per the retrieved learning to avoid SIGPIPE issues withgrep -qpipelines (bug 8142-c280).
🔧 Proposed fix
-set -uo pipefail
+set -euoBased on learnings: "In test scripts under tests/scripts/ (shell scripts), do not add pipefail to the initial set statement; use set -euo instead."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set -uo pipefail | |
| set -euo |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/scripts/test-rc-diff-is-empty-net.sh` at line 26, Change the shell
options at the top of the test script so failures abort and avoid pipefail:
replace the current set invocation that uses "pipefail" with one that includes
"-euo" (i.e., enable errexit, nounset, and no word-splitting protections) so
that commands like "source \"$LIB\"" and fixture setup will stop the script on
error and you won't introduce pipefail-related SIGPIPE issues with pipelines
such as grep -q; update the line containing the existing set command
accordingly.
Source: Learnings
| _helper_verdict() { # <repo> <feat> <helper-fn> | ||
| ( cd "$1" && "$3" "$2" >/dev/null 2>&1 ) && echo exempt || echo not | ||
| } |
There was a problem hiding this comment.
Preserve helper tri-state semantics in _helper_verdict.
_helper_verdict maps all non-zero exits to not. That conflates rc=1 (not exempt) with rc=2 (helper error), so expected-not fixtures can pass even if the helper is failing. Return an explicit error verdict and fail fast on it.
Suggested patch
_helper_verdict() { # <repo> <feat> <helper-fn>
- ( cd "$1" && "$3" "$2" >/dev/null 2>&1 ) && echo exempt || echo not
+ local _rc
+ ( cd "$1" && "$3" "$2" >/dev/null 2>&1 )
+ _rc=$?
+ case "$_rc" in
+ 0) echo exempt ;;
+ 1) echo not ;;
+ *) echo error ;;
+ esac
}
@@
hlp="$(_helper_verdict "$r" "$f" "$_hfn")"
+ if [[ "$hlp" == "error" ]]; then
+ _fail "$name" "helper=$_hfn returned error on fixture '$kind'"
+ return
+ fi
if [[ "$hlp" != "$exp" ]]; then
- _fail "$name" "helper verdict=$hlp != expected=$exp — rc_diff_is_tickets_only wrong on fixture '$kind'"
+ _fail "$name" "helper verdict=$hlp != expected=$exp — $_hfn wrong on fixture '$kind'"
return
fiAs per coding guidelines, tests must include edge-case and error-path coverage; this currently leaves the helper error path unverified.
Also applies to: 173-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/scripts/test-ticket-exemption-equivalence.sh` around lines 147 - 149,
_helper_verdict currently collapses all non-zero exit codes to "not", hiding
helper failures; change it to capture the helper's exit code and return "exempt"
for rc=0, "not" for rc=1, and "error" (and exit immediately with non-zero) for
any other rc so helper errors are visible and tests fail fast; update both
occurrences (the function at lines ~147 and the similar block at ~173-177) to
use this tri-state branching based on the captured exit status.
Source: Coding guidelines
…eticity + docstring accuracy) Neutral opus review of #694 confirmed the production wiring is fail-SAFE (all 3 consumers exempt only on predicate rc==0; rc 1 and rc 2 fall through to fail-closed; diff-tree rc captured separately from emptiness). Two non-production findings: - important (verification): the DD6 equivalence test was non-hermetic — _rci_verdict and _fpa_verdict invoked the consumer scripts without pinning CLAUDE_PLUGIN_ROOT, so off-CI (ambient value set) they resolved a stale review-coverage-lib.sh lacking rc_diff_is_empty_net → "command not found" → false "CONSUMERS DISAGREE" failure. Pin CLAUDE_PLUGIN_ROOT="$REPO_ROOT/plugins/dso" in both (mirrors the sibling test-review-coverage-invariant.sh _run discipline). Verified: now passes with ambient CLAUDE_PLUGIN_ROOT set (the prior failure condition) AND unset. - minor (maintainability): the predicate docstring called the exemption "genuinely- empty-net / no net change over its parents", a misnomer — a clean --no-ff feature merge has an empty --cc yet introduces net change vs its first parent. Reworded to "combined-diff-empty merge (no own/conflict-resolution content)" and documented that safety rests on the content-bearing PARENTS being independently reviewed (T13/t15b), not on the merge having no net change. No logic change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| local r="$1" f="$3" | ||
| ( cd "$r" && DSO_GH_BIN="$_BIN/gh" GH_REPO="o/r" GITHUB_BASE_REF=main \ | ||
| # Pin CLAUDE_PLUGIN_ROOT to the checkout under test so the consumer resolves | ||
| # review-coverage-lib.sh (incl. the newly-added rc_diff_is_empty_net) from THIS | ||
| # tree, not an ambient value leaked from the runner's session that would point | ||
| # at a stale lib and silently fall through to "not exempt". Hermetic regardless | ||
| # of the runner's environment (mirrors test-review-coverage-invariant.sh _run). | ||
| ( cd "$r" && CLAUDE_PLUGIN_ROOT="$REPO_ROOT/plugins/dso" \ | ||
| DSO_GH_BIN="$_BIN/gh" GH_REPO="o/r" GITHUB_BASE_REF=main \ | ||
| DSO_HEAD_SHA="$f" DSO_COVERAGE_INVARIANT_MODE=enforce GH_RETRY_MAX=1 \ | ||
| bash "$RCI" >/dev/null 2>&1 ) && echo exempt || echo not |
There was a problem hiding this comment.
💡 Quality: Hermeticity pin leaves DSO_REVIEW_COVERAGE_LIB override path open
The added comments claim the verdict subshells are now "Hermetic regardless of the runner's environment" after pinning CLAUDE_PLUGIN_ROOT. However, the consumers resolve the library through a higher-precedence override that this commit does not neutralize: review-coverage-invariant.sh:56 uses _REVIEW_COVERAGE_LIB="${DSO_REVIEW_COVERAGE_LIB:-${_PLUGIN_ROOT}/...}" and fp-recovery-audit-sweep.sh:38 uses _FPA_LIB="${DSO_REVIEW_COVERAGE_LIB:-...}". Because DSO_REVIEW_COVERAGE_LIB takes priority over the CLAUDE_PLUGIN_ROOT-derived path, an ambient DSO_REVIEW_COVERAGE_LIB (or DSO_REVIEW_TRISTATE_LIB) leaked from the runner's session would still point the consumer at a stale lib — exactly the failure mode the pin is meant to close. The test's top-of-file unset list does not include these vars. Impact is low (these vars are unlikely to be set in CI), so this is minor, but the hermeticity claim is incomplete. Consider unsetting DSO_REVIEW_COVERAGE_LIB and DSO_REVIEW_TRISTATE_LIB alongside the existing unset, or passing them explicitly empty in the subshells.
Neutralize the higher-precedence lib-path overrides at the top of the test so the CLAUDE_PLUGIN_ROOT pin is actually authoritative.:
unset GITHUB_BASE_REF GITHUB_SHA GITHUB_REPOSITORY DSO_RULESET_BYPASS_USER_IDS DSO_RULESET_BYPASS_USER_ID \
DSO_REVIEW_COVERAGE_LIB DSO_REVIEW_TRISTATE_LIB
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
CI failed: The CI gate 'check-staged-head' failed because the PR is targeting 'main' directly from a 'fix/' branch, violating the repository's required 'staged-*' naming convention.OverviewThe FailuresWorkflow Gate Violation (confidence: high)
Summary
Code Review 👍 Approved with suggestions 0 resolved / 2 findingsImplements a shared 💡 Quality: Hardcoded helper name in failure message misleads debugging📄 tests/scripts/test-ticket-exemption-equivalence.sh:168-176 In the modified Reference the actually-selected helper function ($_hfn) in the failure message.💡 Quality: Hermeticity pin leaves DSO_REVIEW_COVERAGE_LIB override path open📄 tests/scripts/test-ticket-exemption-equivalence.sh:119-128 📄 tests/scripts/test-ticket-exemption-equivalence.sh:139-148 The added comments claim the verdict subshells are now "Hermetic regardless of the runner's environment" after pinning CLAUDE_PLUGIN_ROOT. However, the consumers resolve the library through a higher-precedence override that this commit does not neutralize: Neutralize the higher-precedence lib-path overrides at the top of the test so the CLAUDE_PLUGIN_ROOT pin is actually authoritative.🤖 Prompt for agentsTip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
c9e9 — in-gate empty-net-diff exemption for clean merge commits
Fixes bug c9e9-fd49-a699-4d8f (588e panel's revised fix direction). A 2-parent PR1 merge commit with a genuinely-empty net diff fail-closed the PR2
llm-review(review-coverage-invariant + transitively the dispatcher), forcing redundant bypasses on #634/#671/#678 even though the real code commits were already identity-exempt (ADR-0022). This false wedge gates the 3ee4 enforce-flip.Fix: new shared predicate
rc_diff_is_empty_net(review-coverage-lib.sh) — EXEMPT iff merge (≥2 parents) ANDgit diff-tree --ccrc==0 AND empty file list; NOT-exempt for non-merge/non-empty; ERROR (fail-closed) when diff-tree rc≠0. The exit code, captured separately from emptiness, is the genuinely-empty-net vs uncomputable disambiguator — every uncertain case fails closed. Wired additively + fail-closed into all 3 DD6 consumers (RCI, VSP, FPA — FPA's inline clean-merge check replaced by the shared predicate, removing a parallel impl). Dispatcher inherits transitively (not edited).Security: built ON the shared predicate (not parallel, not content-hash carry-forward). Anti-laundering controls preserved + tested: evil merge stays UNREVIEWED; an unreviewed content parent still blocks; the exemption covers only the content-free merge commit, never content. New unit test (7 fixtures incl. evil-octopus + bad-sha) + DD6 equivalence extended.
Why direct PR + manual bypass-merge
Per epic 588e handoff (two-tier flow FP-amplifies/livelocks). Landing recipe = direct PR + neutral local opus review before merge (the real gate — especially apt: this adds a review exemption to safeguard gates) + human web-UI bypass-merge with all other required checks green. The neutral opus review is attached as the clearance artifact.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests