Skip to content

[worktree-20260606-211401] fix(coverage): narrow A3b self-merge guard to genuine merge nodes (bug 374f)#701

Merged
joeoakhart merged 3 commits into
mainfrom
staged-e90a943c6116-1780846417
Jun 7, 2026
Merged

[worktree-20260606-211401] fix(coverage): narrow A3b self-merge guard to genuine merge nodes (bug 374f)#701
joeoakhart merged 3 commits into
mainfrom
staged-e90a943c6116-1780846417

Conversation

@joeoakhart

@joeoakhart joeoakhart commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Commits

  • feat(enforce-flip): swap merge-pipeline-checks -> review-gate; review-gate enforce
  • fix(coverage): narrow A3b self-merge guard to genuine merge nodes (bug 374f)
  • chore: bump version to v1.17.143

Auto-generated by merge-to-main-pr.sh from git log --no-merges origin/main..HEAD.

Summary by CodeRabbit

  • New Features

    • Review gate now operates in enforce mode, actively blocking merges when violations are detected (previously in warn mode).
  • Bug Fixes

    • Improved edge-case handling in review coverage detection to correctly distinguish between rebase/squash commits and genuine merge nodes.
  • Chores

    • Updated required status checks configuration to use review-gate instead of previous check.
    • Plugin version bumped to 1.17.143.
    • Added and updated test coverage for review gate and required checks logic.

Test and others added 3 commits June 7, 2026 08:58
…-gate enforce

Story 3ee4 (Option A enforce-flip) + cca8 DD2 riding the go-live.

- required-checks.txt: merge-pipeline-checks OUT, review-gate IN (llm-review retained)
- review-gate.yml: DSO_REVIEW_GATE_MODE warn -> enforce
- update-required-checks-manifest.sh / promote-ruleset-required.sh: MAIN staged
  check swapped to review-gate (+ their tests). The merge-pipeline-checks JOB is
  retained in ci.yml (still fires on the sub-PR path); it is removed only from the
  MAIN ruleset required set.

Live ruleset 15629023 provisioned (admin, atomic surgical PATCH): review-gate IN /
merge-pipeline-checks OUT, required_linear_history added, allowed_merge_methods=[rebase],
bypass identity 207596960 preserved. R5/R8 roundtrip green post-provision;
ruleset-design-invariants green under the non-admin identity (I4/I7 = never).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…g 374f)

rc_sha_is_reviewed wrongly marked the rebased version-bump tip UNREVIEWED under
enforce: after cca8 DD1 (rebase-not-merge) GitHub sets a rebase-merged PR's
merge_commit_sha to the 1-parent rebased tip, which the blanket A3b guard excluded
-> every promotion wedged (surfaced on the 3ee4 enforce-flip go-live PR2).

- Extract shared rc_a3b_should_exclude(sha, mcs_match): exclude a covering PR whose
  merge_commit_sha == sha ONLY for a genuine >=2-parent merge node; a proven 1-parent
  rebase/squash tip keeps its covering-PR evidence (G3 still re-verifies the review).
  Fail-closed on unknown topology (0 parents / shallow boundary / absent SHA).
- Route BOTH Goal-1 covering-PR filters through it: rc_sha_is_reviewed AND the G3
  routine in verify-session-provenance.sh (bug 98d9) -> the A3b decision cannot
  diverge between the twins by construction.
- Tests: T14 (1-parent tip == merge_commit_sha -> reviewed), T15 (>=2-parent evil
  merge -> still excluded), new test-rc-a3b-should-exclude.sh (6 cases incl.
  fail-closed). 4-lens panel + convergence APPROVED (epic 588e protocol).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DSO-Story: worktree-20260606-211401
@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 91e9d930-6b07-4a0f-957c-01d0b7f10fa5

📥 Commits

Reviewing files that changed from the base of the PR and between 0e636a3 and 530f088.

📒 Files selected for processing (11)
  • .github/required-checks.txt
  • .github/workflows/review-gate.yml
  • plugins/dso/.claude-plugin/plugin.json
  • plugins/dso/scripts/lib/review-coverage-lib.sh
  • plugins/dso/scripts/promote-ruleset-required.sh
  • plugins/dso/scripts/update-required-checks-manifest.sh
  • plugins/dso/scripts/verify-session-provenance.sh
  • tests/scripts/test-promote-ruleset-required.sh
  • tests/scripts/test-rc-a3b-should-exclude.sh
  • tests/scripts/test-review-coverage-invariant.sh
  • tests/scripts/test-validate-required-checks.sh

Walkthrough

This PR migrates the merge-gate mechanism from merge-pipeline-checks to review-gate in enforce mode, introducing a topology-aware self-merge exclusion predicate that allows rebase/squash-merged PR tips while excluding true merges. Configuration, coverage library, provenance scripts, workflow mode, and tests are updated cohesively.

Changes

Merge-gate transition with topology-aware self-merge handling

Layer / File(s) Summary
Topology-aware self-merge exclusion predicate
plugins/dso/scripts/lib/review-coverage-lib.sh, tests/scripts/test-rc-a3b-should-exclude.sh
rc_a3b_should_exclude function keeps covering PRs matching the SHA under review only when the SHA has exactly one parent (rebase/squash), excluding true merges and unknown topologies in fail-closed mode. Unit test verifies 1-parent keep vs. 2-parent exclude behavior.
Coverage library integration of topology predicate
plugins/dso/scripts/lib/review-coverage-lib.sh
rc_sha_is_reviewed updated to defer self-merge exclusion from Python extraction to Bash filtering. Python emits mcs_match flag per covering PR; Bash loop calls rc_a3b_should_exclude to decide whether to skip, replacing blanket exclusion on exact SHA match.
Provenance verification script topology-aware filtering
plugins/dso/scripts/verify-session-provenance.sh
Python extraction emits mcs_match alongside covering PR numbers; G3 verification loop parses new tab-delimited format and calls rc_a3b_should_exclude instead of blanket filtering on merge_commit_sha match. Changes which covering PRs are eligible for provenance classification.
Required-checks configuration migration to review-gate
.github/required-checks.txt, plugins/dso/scripts/promote-ruleset-required.sh, plugins/dso/scripts/update-required-checks-manifest.sh
Replaces merge-pipeline-checks with review-gate as the required check-context. Ownership comment documents that merge-pipeline-checks is retained in ci.yml but removed from MAIN ruleset. Promotion and manifest-update scripts stage and maintain review-gate instead.
Review-gate workflow mode flip to enforce
.github/workflows/review-gate.yml
Rollout documentation updated to reflect MODE=enforce (previously warn). DSO_REVIEW_GATE_MODE environment variable changed from warn to enforce so review-gate.sh runs in fail-closed mode when sub-check violations occur.
Test coverage for new predicate and configuration changes
tests/scripts/test-promote-ruleset-required.sh, tests/scripts/test-rc-a3b-should-exclude.sh, tests/scripts/test-review-coverage-invariant.sh, tests/scripts/test-validate-required-checks.sh
Unit test validates topology-based exclusion decisions across mcs_match values and commit parent counts. Coverage-invariant tests T14 and T15 verify 1-parent tips are allowed and evil merges are blocked (bug 374f). Configuration tests assert review-gate is added and merge-pipeline-checks is not re-added by the update script.
Plugin manifest version bump
plugins/dso/.claude-plugin/plugin.json
Version incremented from 1.17.142 to 1.17.143.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • navapbc/digital-service-orchestra#259: Both modify verify-session-provenance.sh to change self/near-self covering PR filtering logic around merge_commit_sha to prevent llm-review bypass.
  • navapbc/digital-service-orchestra#648: Both modify plugins/dso/scripts/lib/review-coverage-lib.sh and alter rc_sha_is_reviewed() covering-PR matching behavior, though this PR adds topology-aware exclusion while the other changes retry/backoff logic.
  • navapbc/digital-service-orchestra#438: Both update required-checks tooling by modifying promote-ruleset-required.sh and update-required-checks-manifest.sh to change which check-contexts are treated as required/staged per ruleset.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: fixing a bug in the A3b self-merge guard to narrow its scope to genuine merge nodes, rather than incorrectly excluding rebase-merged tips.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch staged-e90a943c6116-1780846417

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@joeoakhart joeoakhart enabled auto-merge (rebase) June 7, 2026 15:59
@joeoakhart joeoakhart merged commit a5e7799 into main Jun 7, 2026
28 checks passed
joeoakhart pushed a commit that referenced this pull request Jun 7, 2026
…ic 588e SC6)

DD1: PR1 title is derived from the branch's meaningful commit subject via
  _derive_pr_title (staged: prefix retained), not the generic branch-arrow form.
DD2: sprint draft umbrella body documents the long-lived lifecycle (closed at
  /dso:end-session or epic completion; not itself merged; retained under Option A).
DD3: re-baselined the merge-to-main-pr.sh PR1-title edit against current main.
DD4: docs reflect the enforced architecture — CI-INTEGRATION.md + HOOKS-REFERENCE.md
  (merge-pipeline-checks no longer a main required check; review-gate is the
  always-runs fail-closed gate), WORKFLOW-STABILITY-CHECKS.md (admin go-live
  COMPLETED), and check-ruleset-preflight.sh now asserts review-gate IN +
  required_linear_history on main (the prior MAIN_HAS_LINEAR assertion was inverted
  by the cca8 cutover) + new MAIN_MISSING_REVIEW_GATE / MAIN_MISSING_LINEAR verdicts.
DD5: ADR-0020 gains an Execution results section (enforce-flip landed PR #701,
  E6'/E2/E3/E8 outcomes, exit-78 fix, linear-history cutover, bug 374f + FP-recovery,
  ADR-0022 identity exemption, panel/cost record).

Tests: t_pr1_title_is_descriptive (merge-to-main), preflight MAIN_MISSING_REVIEW_GATE
  + MAIN_MISSING_LINEAR, sprint-draft umbrella body, staged-intermediate source guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
JoeOakhartNava pushed a commit that referenced this pull request Jun 7, 2026
…ic 588e SC6)

DD1: PR1 title is derived from the branch's meaningful commit subject via
  _derive_pr_title (staged: prefix retained), not the generic branch-arrow form.
DD2: sprint draft umbrella body documents the long-lived lifecycle (closed at
  /dso:end-session or epic completion; not itself merged; retained under Option A).
DD3: re-baselined the merge-to-main-pr.sh PR1-title edit against current main.
DD4: docs reflect the enforced architecture — CI-INTEGRATION.md + HOOKS-REFERENCE.md
  (merge-pipeline-checks no longer a main required check; review-gate is the
  always-runs fail-closed gate), WORKFLOW-STABILITY-CHECKS.md (admin go-live
  COMPLETED), and check-ruleset-preflight.sh now asserts review-gate IN +
  required_linear_history on main (the prior MAIN_HAS_LINEAR assertion was inverted
  by the cca8 cutover) + new MAIN_MISSING_REVIEW_GATE / MAIN_MISSING_LINEAR verdicts.
DD5: ADR-0020 gains an Execution results section (enforce-flip landed PR #701,
  E6'/E2/E3/E8 outcomes, exit-78 fix, linear-history cutover, bug 374f + FP-recovery,
  ADR-0022 identity exemption, panel/cost record).

Tests: t_pr1_title_is_descriptive (merge-to-main), preflight MAIN_MISSING_REVIEW_GATE
  + MAIN_MISSING_LINEAR, sprint-draft umbrella body, staged-intermediate source guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
joeoakhart pushed a commit that referenced this pull request Jun 7, 2026
…ic 588e SC6)

DD1: PR1 title is derived from the branch's meaningful commit subject via
  _derive_pr_title (staged: prefix retained), not the generic branch-arrow form.
DD2: sprint draft umbrella body documents the long-lived lifecycle (closed at
  /dso:end-session or epic completion; not itself merged; retained under Option A).
DD3: re-baselined the merge-to-main-pr.sh PR1-title edit against current main.
DD4: docs reflect the enforced architecture — CI-INTEGRATION.md + HOOKS-REFERENCE.md
  (merge-pipeline-checks no longer a main required check; review-gate is the
  always-runs fail-closed gate), WORKFLOW-STABILITY-CHECKS.md (admin go-live
  COMPLETED), and check-ruleset-preflight.sh now asserts review-gate IN +
  required_linear_history on main (the prior MAIN_HAS_LINEAR assertion was inverted
  by the cca8 cutover) + new MAIN_MISSING_REVIEW_GATE / MAIN_MISSING_LINEAR verdicts.
DD5: ADR-0020 gains an Execution results section (enforce-flip landed PR #701,
  E6'/E2/E3/E8 outcomes, exit-78 fix, linear-history cutover, bug 374f + FP-recovery,
  ADR-0022 identity exemption, panel/cost record).

Tests: t_pr1_title_is_descriptive (merge-to-main), preflight MAIN_MISSING_REVIEW_GATE
  + MAIN_MISSING_LINEAR, sprint-draft umbrella body, staged-intermediate source guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant