Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions .github/required-checks.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,13 @@
#
# Ownership:
# ci.yml — Actionlint, ShellCheck, Lint Python (ruff), Hook Tests,
# Script Tests, llm-review, merge-pipeline-checks
# Script Tests, llm-review
# review-gate.yml — review-gate (the always-runs, fail-closed per-SHA
# coverage + dangling summary gate; story 3ee4 swapped
# it IN, replacing the no-op merge-pipeline-checks
# umbrella. merge-pipeline-checks the *job* is retained
# in ci.yml — it still fires on the sub-PR path — it is
# only removed from the MAIN ruleset required set here.)
# ticket-platform-matrix.yml — Ticket tests (linux-bash4), Ticket tests (macos-bash3),
# Ticket tests (alpine-busybox)

Expand All @@ -26,7 +32,7 @@ Lint Python (ruff)
Hook Tests
Script Tests
llm-review
merge-pipeline-checks
review-gate

# ── ticket-platform-matrix.yml checks ────────────────────────────────────────
Ticket tests (linux-bash4)
Expand Down
29 changes: 15 additions & 14 deletions .github/workflows/review-gate.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,17 @@ name: review-gate
# success only if the script decides it, never the GH scheduler. The job has NO
# code_changed/path skip: it runs on every base==main PR (DD1).
#
# ROLLOUT (current): MODE=warnit LOGS sub-check violations (::warning::) but does
# NOT block, and is NOT yet a required status check. It SHADOWS the existing
# advisory review-coverage-invariant / dangling-references workflows so warn-mode
# behavior can be compared against the real outcome on live PR2 traffic before
# cutover. GO-LIVE is the enforce-flip (story 3ee4), an admin step:
# 1. flip DSO_REVIEW_GATE_MODE to `enforce` below;
# 2. add `review-gate` to .github/required-checks.txt and REMOVE the superseded
# merge-pipeline-checks (+ the standalone coverage/dangling contexts);
# 3. provision `review-gate` into the live MAIN ruleset (admin token) — the W1
# round-trip drift test then keeps required-checks.txt == live in sync.
# Until step 2/3, this check is advisory (visible, non-blocking).
# ROLLOUT (current): MODE=enforcea sub-check violation or precondition (exit 78)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 5/5

[important] .github/workflows/review-gate.yml:13 .github/workflows/review-gate.yml:82 (design)

ARCHITECTURAL READINESS CONCERN: Enforce mode activation without pre-deployment validation. The diff flips DSO_REVIEW_GATE_MODE to 'enforce' (.github/workflows/review-gate.yml line 82) and makes review-gate a required check (.github/required-checks.txt line 29). This enables fail-closed blocking of all staged→main merges. However, the critical awk bug in rc_a3b_should_exclude means every rebase-merged PR will be incorrectly marked UNREVIEWED and blocked. The diff's own tests (T14/T15) are designed to verify that rebase tips are KEPT under the narrowed A3b (they should pass the covering-PR filter). If those tests are run pre-merge, they WILL FAIL due to the awk bug, serving as a canary. However, if the tests are not run or if the awk error is not caught before merge, enforce mode will activate with a broken implementation, wedging all promotions. The diff should include a pre-activation checklist or canary test that explicitly validates rc_a3b_should_exclude behavior before flipping the mode.

Posted by dso_ci_review.runner; resolve this comment when addressed.

# makes the gate RED and blocks merge. The enforce-flip (story 3ee4) completed the
# three-step go-live:
# 1. DSO_REVIEW_GATE_MODE flipped to `enforce` below;
# 2. `review-gate` added to .github/required-checks.txt and the superseded
# merge-pipeline-checks removed from the MAIN ruleset required set (the
# standalone coverage/dangling contexts were never individually required — I-4);
# 3. `review-gate` provisioned into the live MAIN ruleset (admin token) — the W1
# round-trip drift test (R5/R8) keeps required-checks.txt == live in sync.
# The prior warn-mode shadow showed ZERO false BLOCKs on live PR2 traffic before
# this cutover.

on:
pull_request:
Expand Down Expand Up @@ -77,9 +77,10 @@ jobs:
# (Admin exemption is identity-based per ADR-0022: a covering PR merged by
# a designated bypass-actor — ruleset.bypass_user_ids — is reviewed-
# equivalent inside rc_sha_is_reviewed. No ledger env, no signing key.)
# ROLLOUT KNOB — flip to `enforce` at go-live (see header). This single
# knob is propagated to BOTH sub-checks by review-gate.sh.
DSO_REVIEW_GATE_MODE: warn
# ROLLOUT KNOB — flipped to `enforce` at go-live (story 3ee4). This single
# knob is propagated to BOTH sub-checks by review-gate.sh. In enforce mode
# a sub-check precondition (exit 78) or violation makes the gate RED.
DSO_REVIEW_GATE_MODE: enforce
run: |
# review-gate.sh maps each sub-check's precondition (exit 78) through
# precondition-gate.sh with the gate mode internally, so in warn mode the
Expand Down
2 changes: 1 addition & 1 deletion plugins/dso/.claude-plugin/plugin.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "dso",
"version": "1.17.142",
"version": "1.17.143",
"description": "Workflow infrastructure plugin for Claude Code projects",
"commands": "./commands/",
"skills": "./skills/",
Expand Down
63 changes: 57 additions & 6 deletions plugins/dso/scripts/lib/review-coverage-lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,42 @@ print('failed' if fail else ('passed' if ok else 'not_found'))
" 2>/dev/null
}

# ── A3b self-merge guard — SHARED decision (bug 374f) ─────────────────────────
# rc_a3b_should_exclude <sha> <mcs_match>
#
# A3b excludes a covering MERGED PR from providing review provenance for <sha> when
# that PR's merge_commit_sha == <sha> — but ONLY for a GENUINE merge node. A3b was
# written for merge-commit semantics: a >=2-parent merge node whose combined diff the
# sub-PR review never saw cannot be self-proven by its producing PR. After cca8 DD1
# (rebase-not-merge) GitHub sets a rebase/squash-merged PR's merge_commit_sha to the
# rebased 1-parent TIP — a real commit patch-identical to reviewed sub-PR content
# (under merge-to-main, always the version-bump tip). Excluding THAT wrongly marks it
# UNREVIEWED and wedged every promotion under enforce (bug 374f). So: exclude ONLY when
# <sha> is NOT a proven 1-parent commit. A proven 1-parent rebase/squash tip keeps its
# covering evidence; the caller's G3 still re-verifies the review actually passed, so
# this cannot launder.
#
# DIVERGENCE-PROOF: this is the SINGLE source of the A3b decision. BOTH Goal-1
# covering-PR filters route through it — rc_sha_is_reviewed (below) and the G3 routine
# in verify-session-provenance.sh (which sources this lib). Do not re-inline the guard
# in either caller; that is exactly the twin-divergence hazard bug 374f closed.
#
# Parent count is local git topology (authoritative offline; no API/rate-limit cost,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 2/5

[important] plugins/dso/scripts/lib/review-coverage-lib.sh:254 plugins/dso/scripts/lib/review-coverage-lib.sh:259 (verification)

INCOMPLETE TEST COVERAGE FOR PARALLEL CODE PATHS. The diff modifies rc_sha_is_reviewed (review-coverage-lib.sh) AND verify-session-provenance.sh G3 routine to both call the shared rc_a3b_should_exclude function (lines 254-259 explicitly warn this is a DIVERGENCE-PROOF requiring single-sourcing). New tests T14/T15 in test-review-coverage-invariant.sh exercise rc_sha_is_reviewed's A3b path. However, verify-session-provenance.sh's G3 routine (which also calls rc_a3b_should_exclude at the new code ~line 640 in the diff) has NO new or existing test coverage. The G3 path is exercised only implicitly through integration tests of verify-session-provenance.sh, not through targeted unit or integration tests that would catch deviations. Since the A3b guard is explicitly documented as a SHARED decision that cannot diverge, leaving one of the two callsites without dedicated test coverage creates a maintenance hazard: a future modification to the G3 path could diverge from rc_sha_is_reviewed without detection.

Posted by dso_ci_review.runner; resolve this comment when addressed.

# consistent with rc_diff_is_*). FAIL-CLOSED: only an EXACTLY-1-parent result keeps the
# evidence; >=2 parents (true merge), 0 parents (root / shallow-clone boundary where a
# real merge can report 0), or empty output (commit absent) all EXCLUDE — preserving the
# pre-374f exclusion rather than loosening the guarantee.
#
# args: <sha> <mcs_match: 1 if covering PR's merge_commit_sha == sha, else 0>
# exit: 0 = EXCLUDE this covering PR ; 1 = KEEP it
rc_a3b_should_exclude() {
local _sha="${1:-}" _mcs_match="${2:-0}" _pc
[[ "$_mcs_match" == "1" ]] || return 1 # not a self-merge match -> keep
_pc="$(git rev-list --parents -n1 "$_sha" 2>/dev/null | awk 'NR==1{print NF-1}')"
[[ "$_pc" == "1" ]] && return 1 # exactly 1 parent (rebase/squash tip) -> keep
return 0 # >=2 / 0 / unknown -> exclude (fail-closed)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 1/5

[critical] plugins/dso/scripts/lib/review-coverage-lib.sh:267 (correctness)

FATAL AWK LOGIC ERROR in rc_a3b_should_exclude (line 267). The awk expression `NF==1{print NF-1}` is incorrect. When git rev-list outputs a normal 1-parent commit as 'SHA parent1', that is 2 fields (NF=2), so the condition NF==1 NEVER MATCHES. The output is empty, _pc defaults to empty, and the subsequent check `[[ "$_pc" == "1" ]]` fails, causing return 0 (EXCLUDE) for every 1-parent commit. This inverts the intended semantics: 1-parent rebase/squash tips (which should be KEPT per the spec at line 259) are always EXCLUDED. Correct awk is: `_pc="$(git rev-list --parents -n1 "$_sha" 2>/dev/null | awk '{print NF-1}')"` (no conditional; unconditional NF-1 computes parent count). With this bug, every rebase-merged PR's merge_commit_sha points to a 1-parent tip that is incorrectly excluded, marking it UNREVIEWED and blocking merge in enforce mode. The diff activates enforce mode and makes review-gate required, so this bug will wedge EVERY staged→main promotion.

Posted by dso_ci_review.runner; resolve this comment when addressed.

}

rc_sha_is_reviewed() {
local repo="$1" sha="$2" pr_under_review="${3:-0}"
local pulls_json covering check_json verdict cov_pr cov_head
Expand Down Expand Up @@ -263,27 +299,42 @@ for pr in prs:
# A1 self-exclusion: the PR under review cannot provide its own provenance.
if pur > 0 and pr.get('number') == pur:
continue
# A3b self-merge guard only. The blanket A3a (head == sha_u) is intentionally
# A3b self-merge guard. The blanket A3a (head == sha_u) is intentionally
# NOT applied: a DIFFERENT merged sub-PR whose head IS sha_under_review is the
# VALID covering evidence (the common case — a sub-PR's own head commit).
# Excluding it would return false-UNREVIEWED for every sub-PR head SHA and,
# in enforce mode, block every legitimate staged->main merge. G3 below
# independently re-verifies the covering PR's review check actually PASSED, so
# keeping it cannot launder. (Mirrors the W4 fix in verify-session-provenance.sh.)
if pr.get('merge_commit_sha') == sha_u:
continue
# keeping it cannot launder.
#
# A3b itself (bug 374f) is NOT decided in Python: GitHub sets a rebase/squash-
# merged PR's merge_commit_sha to the rebased 1-parent TIP (under merge-to-main,
# the version-bump tip), so an unconditional merge_commit_sha==sha exclusion here
# wrongly marks that tip UNREVIEWED and wedges every promotion under enforce. We
# only emit a per-PR mcs_match flag; the bash caller routes it through the SHARED
# rc_a3b_should_exclude (single source of truth, also used by the G3 routine in
# verify-session-provenance.sh), which excludes only genuine >=2-parent merge
# nodes and fails closed on unknown topology. G3 still re-verifies the review.
mcs_match = 1 if pr.get('merge_commit_sha') == sha_u else 0
# NOTE: merged_by is intentionally NOT read here — the /commits/{sha}/pulls list
# representation does not include it (null). The identity-exemption check (ADR-0022)
# fetches merged_by from the single-PR GET, on the bypass path only.
if head:
print(f\"{pr.get('number','')}\t{head}\")
print(f\"{pr.get('number','')}\t{head}\t{mcs_match}\")
" 2>/dev/null)"
local _filter_rc=$?
[[ $_filter_rc -eq 3 ]] && return 2 # parse error -> fail closed
[[ -z "$covering" ]] && return 1 # no covering merged PR -> not reviewed

while IFS=$'\t' read -r cov_pr cov_head; do
while IFS=$'\t' read -r cov_pr cov_head cov_mcs_match; do
[[ -z "$cov_head" ]] && continue
# A3b (narrowed, bug 374f): exclude this covering PR only if its merge_commit_sha
# == the SHA under review AND that SHA is a genuine merge node. Shared decision —
# see rc_a3b_should_exclude (single source of truth; the twin in
# verify-session-provenance.sh calls the same function).
if rc_a3b_should_exclude "$sha" "$cov_mcs_match"; then
continue
fi
check_json="$(_rc_gh_with_backoff api "repos/${repo}/commits/${cov_head}/check-runs")" || return 2
verdict="$(printf '%s' "$check_json" | rc_review_check_verdict)"
case "$verdict" in
Expand Down
12 changes: 7 additions & 5 deletions plugins/dso/scripts/promote-ruleset-required.sh
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,17 @@ NON_INTERACTIVE=0
# The check-contexts that this script manages, partitioned by target ruleset.
# Each ruleset gets only the checks that are emitted on PRs it covers:
#
# merge-pipeline-checks → fires on session→main PRs, belongs on the
# main-branch ruleset.
# review-sub-pr → fires only on PRs targeting sub-PR branches,
# belongs on the sub-PR ruleset.
# review-gate → the always-runs, fail-closed per-SHA coverage + dangling
# summary gate; fires on every base==main PR, belongs on the
# main-branch ruleset. Swapped IN for the no-op
# merge-pipeline-checks umbrella at the story-3ee4 enforce-flip.
# review-sub-pr → fires only on PRs targeting sub-PR branches, belongs on the
# sub-PR ruleset.
#
# Adding review-sub-pr to the main-branch ruleset would deadlock every PR
# to main (the check never reports on those PRs).
MAIN_STAGED_CHECKS=(
"merge-pipeline-checks"
"review-gate"
)
SUB_PR_STAGED_CHECKS=(
"review-sub-pr"
Expand Down
8 changes: 7 additions & 1 deletion plugins/dso/scripts/update-required-checks-manifest.sh
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,14 @@ CHECKS_FILE="$REPO_ROOT/.github/required-checks.txt"
# ruleset would deadlock every PR to main. Enforcement of review-sub-pr lives
# on the sub-PR ruleset (16961402), populated by provision-ruleset.sh from
# the sub-PR branch-patterns source-of-truth file (config/sub-pr-branch-patterns.txt).
#
# review-gate (story 3ee4) is the always-runs, fail-closed per-SHA coverage +
# dangling summary gate that replaced the no-op merge-pipeline-checks umbrella on
# the MAIN ruleset. merge-pipeline-checks is intentionally NOT in this list: the
# job is retained in ci.yml (it still fires on the sub-PR path) but is no longer a
# required context on the main ruleset, so this bootstrap must not re-add it.
REQUIRED_ENTRIES=(
"merge-pipeline-checks"
"review-gate"
)

# Parse arguments
Expand Down
25 changes: 20 additions & 5 deletions plugins/dso/scripts/verify-session-provenance.sh
Original file line number Diff line number Diff line change
Expand Up @@ -605,13 +605,21 @@ for pr in pr_list:
# CANNOT launder, because G3 below independently verifies that each kept
# covering PR review check actually PASSED. Do NOT re-add the blanket A3a.
# A3b: self-merge guard (a SHA cannot be provenanced by the merge commit it
# itself produced).
if pr.get('merge_commit_sha') == sha_under_review:
continue
# itself produced) — but NOT decided here (bug 374f). GitHub sets a rebase/
# squash-merged PR's merge_commit_sha to the rebased 1-parent TIP (under
# merge-to-main, the version-bump tip); an unconditional exclusion here would
# falsely drop that reviewed tip and force a needless re-review. Emit a per-PR
# mcs_match flag instead; the bash loop routes it through the SHARED
# rc_a3b_should_exclude (single source of truth, also used by
# review-coverage-lib.sh::rc_sha_is_reviewed) which excludes only genuine
# >=2-parent merge nodes, fail-closed on unknown topology. G3 below still
# verifies each kept covering PR's review actually passed, so this cannot launder.
# NOTE: merged_by is NOT read here — the /commits/{sha}/pulls list representation
# omits it (null). The identity check (ADR-0022) fetches merged_by from the
# single-PR GET, on the bypass path only.
print(number if number is not None else '')
if number is not None:
mcs_match = 1 if pr.get('merge_commit_sha') == sha_under_review else 0
print(f'{number}\t{mcs_match}')
" 2>/dev/null)" || covering_prs=""

# G3 fix: verify that each covering PR's review-sub-pr check actually
Expand All @@ -620,8 +628,15 @@ for pr in pr_list:
# failed review would incorrectly count as "covered."
_verified_covering=0
if [[ -n "$covering_prs" ]]; then
while IFS= read -r _cov_pr; do
while IFS=$'\t' read -r _cov_pr _cov_mcs_match; do
[[ -z "$_cov_pr" ]] && continue
# A3b (bug 374f, SHARED): exclude a covering PR whose merge_commit_sha ==
# this SHA only when the SHA is a genuine merge node. rc_a3b_should_exclude
# (review-coverage-lib.sh) is the single source of truth — identical to
# rc_sha_is_reviewed's A3b so the two Goal-1 filters cannot diverge.
if rc_a3b_should_exclude "$sha" "${_cov_mcs_match:-0}"; then
continue
fi
Comment on lines +631 to +639

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Bug: A3b helper ignores GIT_REPO_PATH; runs git in cwd

rc_a3b_should_exclude (review-coverage-lib.sh:262-268) determines the parent count with a bare git rev-list --parents -n1 "$_sha", which operates on the current working directory rather than a passed repo path. In verify-session-provenance.sh every other local git operation is scoped with git -C "$GIT_REPO_PATH", and the two sibling cwd-dependent lib helpers (rc_diff_is_tickets_only, rc_diff_is_empty_net) are deliberately invoked inside a ( cd "$GIT_REPO_PATH" 2>/dev/null && ... ) subshell precisely because they use bare git. The new A3b call at verify-session-provenance.sh:637 is NOT wrapped that way.

When DSO_REPO_PATH is set to a path other than the process cwd (a supported override, default .), git rev-list will not find $sha, producing an empty parent count. The function then takes the fail-closed branch and EXCLUDEs the covering PR, marking a legitimate 1-parent rebase/squash tip as UNREVIEWED — reintroducing the exact promotion-wedge regression that bug 374f set out to fix. In CI today cwd happens to equal the repo, so it works, but this is a latent inconsistency with the established pattern.

Fix: pass the repo path through and run git with -C, or wrap the call in the same ( cd "$GIT_REPO_PATH" && ... ) subshell used for the other shared helpers.

Invoke the shared A3b helper with cwd == GIT_REPO_PATH, matching the existing pattern used for rc_diff_is_tickets_only / rc_diff_is_empty_net so the bare git rev-list resolves the SHA in the correct repository.:

if ( cd "$GIT_REPO_PATH" 2>/dev/null && rc_a3b_should_exclude "$sha" "${_cov_mcs_match:-0}" ); then
    continue
fi
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

if (( _api_call_count >= GH_BUDGET )); then
if (( _budget_exhausted == 0 )); then
echo "BUDGET_EXHAUSTED during G3 review-check verification" >&2
Expand Down
13 changes: 7 additions & 6 deletions tests/scripts/test-promote-ruleset-required.sh
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ assert_pass_if_clean "test_unknown_arg_exits_1"
_snapshot_fail
_dr_tmpdir="$(_make_tmpdir)"
mkdir -p "$_dr_tmpdir/.github"
printf 'review-sub-pr\nmerge-pipeline-checks\n' > "$_dr_tmpdir/.github/required-checks.txt"
printf 'review-sub-pr\nreview-gate\n' > "$_dr_tmpdir/.github/required-checks.txt"

# Minimal gh stub (only repo view for auto-detect; not used when --repo is explicit)
_gh_stub="$_dr_tmpdir/gh"
Expand All @@ -112,10 +112,11 @@ _dr_out="$(DSO_DRY_RUN=1 PATH="$_dr_tmpdir:$PATH" bash "$SCRIPT" \

assert_eq "test_dry_run_stage_outputs: exits 0" "0" "$_dr_exit"
# The observation-window ruleset targets main, so it stages only MAIN_STAGED_CHECKS
# (merge-pipeline-checks). review-sub-pr is staged into the sub-PR ruleset directly
# via --promote-to-required (no observation window for sub-PR checks).
assert_contains "test_dry_run_stage_outputs: mentions merge-pipeline-checks" \
"merge-pipeline-checks" "$_dr_out"
# (review-gate — swapped IN for merge-pipeline-checks at the story-3ee4 enforce-flip).
# review-sub-pr is staged into the sub-PR ruleset directly via --promote-to-required
# (no observation window for sub-PR checks).
assert_contains "test_dry_run_stage_outputs: mentions review-gate" \
"review-gate" "$_dr_out"
# Negative assertion: review-sub-pr must NOT appear in the main observation payload,
# or it will deadlock every PR to main once enforcement promotes.
_review_subpr_absent="yes"
Expand All @@ -135,7 +136,7 @@ assert_pass_if_clean "test_dry_run_stage_outputs"
_snapshot_fail
_dp_tmpdir="$(_make_tmpdir)"
mkdir -p "$_dp_tmpdir/.github"
printf 'review-sub-pr\nmerge-pipeline-checks\n' > "$_dp_tmpdir/.github/required-checks.txt"
printf 'review-sub-pr\nreview-gate\n' > "$_dp_tmpdir/.github/required-checks.txt"

# Write ruleset JSON fixtures to files (avoids heredoc variable-expansion issues)
_ruleset_list_file="$_dp_tmpdir/ruleset-list.json"
Expand Down
72 changes: 72 additions & 0 deletions tests/scripts/test-rc-a3b-should-exclude.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
#!/usr/bin/env bash
# tests/scripts/test-rc-a3b-should-exclude.sh — bug 374f unit test
#
# Drives the PRODUCTION rc_a3b_should_exclude (review-coverage-lib.sh) over REAL git
# fixtures. This is the SHARED A3b self-merge decision: BOTH Goal-1 covering-PR filters
# route through it (review-coverage-lib.sh::rc_sha_is_reviewed AND the G3 routine in
# verify-session-provenance.sh), so the A3b semantics cannot diverge between them.
#
# SEMANTICS (preserve exactly):
# - mcs_match==0 (covering PR's merge_commit_sha != sha) -> KEEP (1)
# - mcs_match==1 AND sha is a 1-parent commit (rebase/squash tip) -> KEEP (1)
# (patch-identical to reviewed content; G3 still verifies the review passed)
# - mcs_match==1 AND sha is a >=2-parent genuine merge node -> EXCLUDE (0)
# - mcs_match==1 AND topology UNKNOWN (absent / 0-parent boundary) -> EXCLUDE (0)
# FAIL-CLOSED: only a PROVEN 1-parent tip is kept.
#
# This is the divergence-guard the panel (epic 588e) required: the single shared
# decision is unit-tested here; rc_sha_is_reviewed's T14/T15 and the vsp suites
# exercise it through each caller.

set -uo pipefail
export GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null GIT_TERMINAL_PROMPT=0
REPO_ROOT="$(git rev-parse --show-toplevel)"
LIB="$REPO_ROOT/plugins/dso/scripts/lib/review-coverage-lib.sh" # shim-exempt: test sources the lib under test

PASS=0; FAIL=0
_pass() { echo "PASS: $1"; PASS=$((PASS+1)); }
_fail() { echo "FAIL: $1 ($2)"; FAIL=$((FAIL+1)); }

_W="$(mktemp -d "${TMPDIR:-/tmp}/dso-a3b.XXXXXX")"; trap 'rm -rf "$_W"' EXIT
R="$_W/repo"; mkdir -p "$R"
(
cd "$R" || exit 1
git init -q -b main
git config user.email t@e.st; git config user.name t; git config commit.gpgsign false
echo base > README.md; git add README.md; git commit -q -m base

# A 1-parent commit (rebase/squash tip class).
echo tip > tip.txt; git add tip.txt; git commit -q -m "1-parent tip"
git rev-parse HEAD > "$_W/ONE"

# A genuine 2-parent merge node.
git checkout -q -b side HEAD~1
echo s > side.txt; git add side.txt; git commit -q -m side
git checkout -q main
git merge -q --no-ff -m "2-parent merge node" side
git rev-parse HEAD > "$_W/TWO"
) || { echo "FIXTURE SETUP FAILED"; exit 1; }

# shellcheck source=/dev/null

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 3/5

[important] tests/scripts/test-rc-a3b-should-exclude.sh:50 tests/scripts/test-rc-a3b-should-exclude.sh:53 (verification)

TEST FILE FUNCTION-AVAILABILITY GAP. test-rc-a3b-should-exclude.sh (line 50) sources review-coverage-lib.sh but does not validate that rc_a3b_should_exclude was successfully defined before invoking it. The test has `set -uo pipefail` but NOT `set -e`, so if the function definition fails to source (e.g., syntax error, parse failure in review-coverage-lib.sh), the _verdict helper (line 53) will silently invoke a non-existent function. In bash without `set -e`, a command not found returns 0 in a subshell, so `_verdict "$ONE" 1` would echo 0, and assertions like `_assert "self-merge match + 1-parent tip -> keep" "$(_verdict ...)" 1` would compare 0 vs 1 and fail—but the root cause (function not sourced) would be masked. If the awk error in review-coverage-lib.sh breaks the function definition, this test could silently pass or fail for the wrong reason.

Posted by dso_ci_review.runner; resolve this comment when addressed.

source "$LIB"

_verdict() { ( cd "$R" && rc_a3b_should_exclude "$1" "$2" >/dev/null 2>&1; echo $? ); }
_assert() { if [[ "$2" == "$3" ]]; then _pass "$1"; else _fail "$1" "rc=$2 want $3"; fi; }

ONE="$(cat "$_W/ONE")"; TWO="$(cat "$_W/TWO")"

# mcs_match==0 -> always KEEP (1), regardless of topology.
_assert "no-merge-match (1-parent) -> keep" "$(_verdict "$ONE" 0)" 1
_assert "no-merge-match (2-parent) -> keep" "$(_verdict "$TWO" 0)" 1
# mcs_match==1 -> decide on parent count.
_assert "self-merge match + 1-parent tip -> keep" "$(_verdict "$ONE" 1)" 1
_assert "self-merge match + 2-parent node -> EXCLUDE" "$(_verdict "$TWO" 1)" 0
# Fail-closed: unknown topology (absent SHA reports empty parent count) -> EXCLUDE.
_assert "self-merge match + absent SHA -> EXCLUDE (fail closed)" \
"$(_verdict "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" 1)" 0
# Default mcs_match (missing arg -> 0) -> keep.
_assert "missing mcs_match arg defaults to keep" "$( cd "$R" && rc_a3b_should_exclude "$ONE" >/dev/null 2>&1; echo $? )" 1

echo ""
echo "=== test-rc-a3b-should-exclude.sh: PASS=$PASS FAIL=$FAIL ==="
[[ $FAIL -eq 0 ]]
Loading
Loading