staged: worktree-20260606-211401 -> staged-e90a943c6116-1780846417#700
Conversation
…-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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
DSO-Review-Cycle: 1 pr_number=700 commit_sha=da931e4bc3fa7cad47c52cf4f87626eb6d1e4b3f findings_hash=3b2b9d6fb39258e1 tuples=[[".github/workflows/review-gate.yml", "", "design"], ["plugins/dso/scripts/lib/review-coverage-lib.sh", "", "correctness"], ["tests/scripts/test-rc-a3b-should-exclude.sh", "", "verification"], ["tests/scripts/test-review-coverage-invariant.sh", "", "verification"], ["tests/scripts/test-review-coverage-invariant.sh", "", "verification"]] |
| [[ "$_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) |
There was a problem hiding this comment.
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.
| # 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, |
There was a problem hiding this comment.
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.
| git rev-parse HEAD > "$_W/TWO" | ||
| ) || { echo "FIXTURE SETUP FAILED"; exit 1; } | ||
|
|
||
| # shellcheck source=/dev/null |
There was a problem hiding this comment.
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.
| _mk_repo "$r" "$sf" "rebased-tip" # single 1-parent reachable commit | ||
| sha="$(sed -n 1p "$sf")" | ||
| # Covering merged PR #50 whose merge_commit_sha IS this 1-parent tip (rebase merge), | ||
| # head=cov50 with a passing review check. |
There was a problem hiding this comment.
DSO llm-review — finding 4/5
[important] tests/scripts/test-review-coverage-invariant.sh:269 (verification)
DANGLING REFERENCE IN NEW TEST FIXTURE. test-review-coverage-invariant.sh T14 fixture (lines 268-269) sets up a covering merged PR #50 with merge_commit_sha == the rebase tip, and a covering check 'cov50'. However, the test does not verify that _checks function actually creates valid check-run output for 'cov50'. The test assumes _checks success > "$md/checks_cov50" will create a file that rc_sha_is_reviewed can parse. If the file is missing or malformed, the test assertion at line 272 (`grep -q 'every SHA proven reviewed'`) may pass or fail for reasons unrelated to the A3b narrowing. The test file should include inline validation of fixture structure before running _run.
Posted by dso_ci_review.runner; resolve this comment when addressed.
| # 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=enforce — a sub-check violation or precondition (exit 78) |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
⚠️ 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 👍 / 👎
|
FP_RECOVERY: pr_number=700 manual_reviewer_hash=eaced51157e67e3e7212bc68b5d90001df73b18cf2d52deeba941d0458bc8388 fp_rationale=review-sub-pr finding-1 [critical] misread |
530f088
into
staged-e90a943c6116-1780846417
CI failed: The CI pipeline failed because the automated LLM-based reviewer identified 1 critical and 4 important code quality findings in the PR.OverviewThe build failed during the FailuresLLM Review Blocked (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
Staged promotion (PR-C two-PR flow)
This is the first PR of a two-PR session→main promotion. The worktree branch
worktree-20260606-211401is being merged into the staged refstaged-e90a943c6116-1780846417, which was created fromorigin/mainHEAD.The sub-PR ruleset's
required_workflowsrule requiresreview-sub-prto run successfully on this PR before merge. Once this PR merges, the merge-to-main script will open PR2 fromstaged-e90a943c6116-1780846417→main.🤖 Generated by merge-to-main-pr.sh
Summary by Gitar
check-ruleset-preflight.shto enforcereview-gateandrequired_linear_historyon the main ruleset.create-sprint-draft-pr.shto provide a descriptive, long-lived umbrella lifecycle body.merge-to-main-pr.shto generate descriptive PR1 titles based on commit subjects rather than generic branch arrows.ADR-0020,WORKFLOW-STABILITY-CHECKS.md, andCI-INTEGRATION.mdto document the completed3ee4enforce-flip and rebase-not-merge cutover.test-merge-to-main-pr.shfor descriptive PR1 titles and intest-check-ruleset-preflight.shfor main ruleset enforcement assertions.plugins/dso/.claude-plugin/plugin.jsonto version1.17.144.This will update automatically on new commits.