fix(scheduler): require independent exact-head approval - #828
fix(scheduler): require independent exact-head approval#828seonghobae wants to merge 11 commits into
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Comment |
|
@opencode-agent address Repair only the six stale positive scheduler fixtures on exact current Draft head Production approval policy is already the intended security fix and must not be weakened. Strix Changed Path Quality CI run
Update only the semantically authorized positive fixtures used by those paths so they model the real post-repair authorization state: Do not change the default After the test-only repair, rerun the focused independent-approval gate, complete central suite, Strix quick gate, compilation, exact 100% production statement/branch coverage and public-docstring gate, plus all exact-head security/supply-chain checks. Keep Draft; no self-approval, synthetic approval, branch-protection bypass, or temporary/write-capable repair workflow. |
|
@opencode-agent Fix only the current exact-head Strix regression on PR #828 at head The six observed failures to repair are:
Use an explicit small positive-fixture helper only if it makes the intent clearer; it must not affect default/negative fixtures. Before pushing, re-read the live PR head and target blob and abort/re-plan if either moved. Validate the focused independent-approval regression, all affected legacy scheduler tests, full suite, exact 100% production statement/branch coverage and public-docstring gate, Strix quick gate, compilation/syntax, and clean diff. Keep the PR Draft and do not merge or synthesize approval. |
|
@coderabbitai review Review-only on exact current head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
expected_head='c10607aec1f1f23f2125422310101e03de9bd86d'
repo='ContextualWisdomLab/.github'
pr='828'
echo '== Live PR metadata =='
gh pr view "$pr" --repo "$repo" --json number,isDraft,headRefOid,baseRefOid,url \
--jq '{number, isDraft, headRefOid, baseRefOid, url}'
echo '== Checked-out commit =='
git rev-parse HEAD
git status --short
echo '== Exact-head commit verification =='
test "$(git rev-parse HEAD)" = "$expected_head"
test "$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq .headRefOid)" = "$expected_head"
echo 'exact-head verified'
echo '== Changed files at exact head relative to merge base =='
base="$(git merge-base HEAD origin/main)"
git diff --name-status "$base"...HEAD
echo '== Relevant source/test file locations =='
fd -t f -a '(^|/)(pr_review_merge_scheduler|_pr_review_merge_scheduler_core|test_scheduler_independent_approval_gate|test_pr_review_merge_scheduler)\.py$' .
echo '== Python structure maps =='
ast-grep outline scripts/ci/pr_review_merge_scheduler.py --items all || true
ast-grep outline scripts/ci/_pr_review_merge_scheduler_core.py --items all || true
ast-grep outline tests/test_scheduler_independent_approval_gate.py --items all || true
ast-grep outline tests/test_pr_review_merge_scheduler.py --items all || true
echo '== Policy identifiers with context =='
rg -n -C 5 --type py \
'reviewDecision|headRefOid|author|OpenCode|APPROVED|independent|formal.*review|reviews' \
scripts/ci/pr_review_merge_scheduler.py \
scripts/ci/_pr_review_merge_scheduler_core.py \
tests/test_scheduler_independent_approval_gate.py \
tests/test_pr_review_merge_scheduler.py
echo '== Exact-head diff for relevant files =='
git diff --unified=80 "$base"...HEAD -- \
scripts/ci/pr_review_merge_scheduler.py \
scripts/ci/_pr_review_merge_scheduler_core.py \
tests/test_scheduler_independent_approval_gate.py \
tests/test_pr_review_merge_scheduler.pyLength of output: 101369 🏁 Script executed: #!/bin/bash
set -euo pipefail
expected_head='c10607aec1f1f23f2125422310101e03de9bd86d'
test "$(git rev-parse HEAD)" = "$expected_head"
facade='scripts/ci/pr_review_merge_scheduler.py'
core='scripts/ci/_pr_review_merge_scheduler_core.py'
legacy='tests/test_pr_review_merge_scheduler.py'
regression='tests/test_scheduler_independent_approval_gate.py'
echo '== Facade full map and bounded source =='
nl -ba "$facade" | sed -n '1,360p'
echo '== Core approval-related definitions and entrypoint call sites =='
rg -n -C 12 --type py \
'^(def (pull_request_author_login|has_independent_current_head_approval|merge_approval_block_reason|inspect_pr|enable_auto_merge|merge_pr|fetch_pr|fetch_open_prs)|PULL_REQUEST_FIELDS_FRAGMENT)|\bmerge_approval_block_reason\(|\bhas_independent_current_head_approval\(|Decision\(.*"(merge|auto_merge)"' \
"$core"
echo '== Core exact source regions around each relevant definition/call =='
for line in $(rg -n --type py \
'^(def (pull_request_author_login|has_independent_current_head_approval|merge_approval_block_reason|inspect_pr|enable_auto_merge|merge_pr|fetch_pr|fetch_open_prs)|PULL_REQUEST_FIELDS_FRAGMENT)|\bmerge_approval_block_reason\(' \
"$core" | cut -d: -f1 | sort -nu); do
start=$(( line > 25 ? line - 25 : 1 ))
end=$(( line + 100 ))
echo "--- $core:$start-$end ---"
nl -ba "$core" | sed -n "${start},${end}p"
done
echo '== Merge and auto-merge return sites, with enclosing context =='
rg -n -C 20 --type py 'Decision\([^)]*"(merge|auto_merge)"|action="(merge|auto_merge)"' "$core"
echo '== Approval regression test, complete =='
nl -ba "$regression" | sed -n '1,280p'
echo '== Legacy fixture definitions and all modified authorized/negative-policy regions =='
rg -n -C 16 --type py \
'^(def (make_pr|merge_approved_pr|test_body_head_sha_approval_prevents_same_run_opencode_rerun|test_current_head_approval_cleans_previous_head_change_gate_before_merge|test_inspect_pr_blocks_and_waits_for_policy_states|test_inspect_pr_handles_approved_reviews_and_dispatch|test_direct_or_auto_falls_back_to_auto_merge_when_branch_policy_blocks_direct_merge|test_direct_or_auto_attempts_direct_merge_when_mergeability_is_blocked))|reviewDecision|independent-reviewer|pull-request-author|COMMENTED|anonymous|stale|previous-head' \
"$legacy"
echo '== Test-only diff, minimal context =='
base="$(git merge-base HEAD origin/main)"
git diff --unified=12 "$base"...HEAD -- "$legacy" "$regression"Length of output: 2832 🏁 Script executed: #!/bin/bash
set -euo pipefail
expected_head='c10607aec1f1f23f2125422310101e03de9bd86d'
test "$(git rev-parse HEAD)" = "$expected_head"
numbered() { awk '{printf "%6d\t%s\n", NR, $0}' "$1"; }
facade='scripts/ci/pr_review_merge_scheduler.py'
core='scripts/ci/_pr_review_merge_scheduler_core.py'
legacy='tests/test_pr_review_merge_scheduler.py'
regression='tests/test_scheduler_independent_approval_gate.py'
echo '== Facade, complete (expected small policy facade) =='
numbered "$facade"
echo '== Core approval definitions and calls =='
rg -n -C 10 --type py \
'^(def (pull_request_author_login|has_independent_current_head_approval|merge_approval_block_reason|inspect_pr|enable_auto_merge|merge_pr|fetch_pr|fetch_open_prs)|PULL_REQUEST_FIELDS_FRAGMENT)|\bmerge_approval_block_reason\(|\bhas_independent_current_head_approval\(' \
"$core"
echo '== Core exact regions =='
for line in $(rg -n --type py \
'^(def (pull_request_author_login|has_independent_current_head_approval|merge_approval_block_reason|inspect_pr|enable_auto_merge|merge_pr|fetch_pr|fetch_open_prs)|PULL_REQUEST_FIELDS_FRAGMENT)' \
"$core" | cut -d: -f1 | sort -nu); do
start=$(( line > 20 ? line - 20 : 1 ))
end=$(( line + 120 ))
echo "--- $core:$start-$end ---"
sed -n "${start},${end}p" "$core" | awk -v n="$start" '{printf "%6d\t%s\n", n++, $0}'
done
echo '== All core merge/auto-merge decision sites =='
rg -n -C 24 --type py 'Decision\([^)]*"(merge|auto_merge)"|action="(merge|auto_merge)"' "$core"
echo '== Independent-gate regression test =='
numbered "$regression"
echo '== Legacy fixture helpers =='
sed -n '1,115p' "$legacy" | awk '{printf "%6d\t%s\n", NR, $0}'
echo '== Legacy tests named in the request, with sufficient bounded context =='
for name in \
test_body_head_sha_approval_prevents_same_run_opencode_rerun \
test_current_head_approval_cleans_previous_head_change_gate_before_merge \
test_inspect_pr_blocks_and_waits_for_policy_states \
test_inspect_pr_handles_approved_reviews_and_dispatch \
test_direct_or_auto_falls_back_to_auto_merge_when_branch_policy_blocks_direct_merge \
test_direct_or_auto_attempts_direct_merge_when_mergeability_is_blocked; do
line="$(rg -n "^def ${name}\b" "$legacy" | head -1 | cut -d: -f1)"
echo "--- $legacy:$line ($name) ---"
sed -n "${line},$((line + 300))p" "$legacy" | awk -v n="$line" '{printf "%6d\t%s\n", n++, $0}' \
| sed -n '1,/^ *[0-9][0-9]*[[:space:]]def /{ /^ *[0-9][0-9]*[[:space:]]def /!p; }'
done
echo '== Direct negative-policy test cases in legacy suite =='
rg -n -C 8 --type py \
'REVIEW_REQUIRED|CHANGES_REQUESTED|COMMENTED|author.*login|author.*\{\}|author.*None|commit.*old|previous-head|anonymous|independent' \
"$legacy"
echo '== Test-only exact-head diff =='
base="$(git merge-base HEAD origin/main)"
git diff --unified=16 "$base"...HEAD -- "$legacy" "$regression"Length of output: 178950 🏁 Script executed: #!/bin/bash
set -euo pipefail
expected_head='c10607aec1f1f23f2125422310101e03de9bd86d'
repo='ContextualWisdomLab/.github'
test "$(git rev-parse HEAD)" = "$expected_head"
test "$(gh pr view 828 --repo "$repo" --json headRefOid --jq .headRefOid)" = "$expected_head"
core='scripts/ci/_pr_review_merge_scheduler_core.py'
facade='scripts/ci/pr_review_merge_scheduler.py'
echo '== Exact live head remains verified =='
gh pr view 828 --repo "$repo" --json headRefOid,isDraft --jq '{headRefOid,isDraft}'
echo '== Complete core decision segment =='
sed -n '2340,2650p' "$core" | awk -v n=2340 '{printf "%6d\t%s\n", n++, $0}'
echo '== Production direct-core imports, file execution, and scheduler entrypoints =='
rg -n -C 4 \
'_pr_review_merge_scheduler_core|pr_review_merge_scheduler(\.py)?|--self-test|inspect_pr\(' \
--glob '*.py' --glob '*.yml' --glob '*.yaml' --glob '!tests/**' .
echo '== Workflow references to scheduler files =='
rg -n -C 5 \
'scripts/ci/(pr_review_merge_scheduler|_pr_review_merge_scheduler_core)\.py' \
.github
echo '== Core module export or runtime alias changes =='
rg -n -C 4 \
'__all__|sys\.modules|importlib|exec_module|from scripts\.ci\._pr_review_merge_scheduler_core|import scripts\.ci\._pr_review_merge_scheduler_core' \
scripts tests .github
echo '== Final changed file list and production diff summary =='
base="$(git merge-base HEAD origin/main)"
git diff --stat "$base"...HEAD
git diff --name-status "$base"...HEAD
git diff --check "$base"...HEAD
git diff --unified=8 "$base"...HEAD -- "$facade" "$core"Length of output: 108137
I verified live PR The facade delegates scheduler behavior to the authoritative core. It adds the approval boundary without duplicating privileged scheduler implementation. The merge gate fails closed for:
The positive fixtures use a narrow authorized fixture helper. The default fixture remains No branch mutation, approval, merge, release, or repair automation was performed.
|
|
@opencode-agent address Reconcile this Draft with the exact current protected Before writing, refetch and require those head/main/blob identities to remain exact. Merge the protected-main tip into the existing branch with a normal non-forced merge. Resolve After reconciliation, ensure the effective feature diff remains the scheduler approval-policy facade/core, its permanent quality workflow/tests, doctoring, and changelog only. Rerun Scheduler Independent Approval Quality CI, the complete central suite, Strix Changed Path Quality CI, Python compilation, exact 100% production statement/branch and public-docstring gates, plus security/supply-chain checks. Keep Draft until fresh exact-head automated review, zero valid unresolved findings, qualifying independent non-author approval, and branch protection all pass. |
|
@opencode-agent address Reconcile this scheduler separation-of-duties repair onto current protected Use a trusted checkout and normally merge exact protected main into this existing branch. Preserve all newer main behavior and the narrow authorization invariant already implemented on this branch:
The current branch diff already updates positive scheduler fixtures toward Do not force-push, use |
1 participant
Safety gap
The organization merge scheduler could previously enter direct-merge/auto-merge paths from a same-head OpenCode
APPROVEDreview even when GitHub still reportedreviewDecision: REVIEW_REQUIRED. That lets automation policy run ahead of the repository's independent-review gate and is unsafe if a routine scheduler credential ever acquires a ruleset bypass path.Test-first repair
Exact RED head
c7011f23356eee92c3569709a893790a6b0893c4added onlytests/test_scheduler_independent_approval_gate.pyfrom protectedmain1131b1bbafb24e455fc8619cdf316813e8721861.The production repair now requires, before merge/auto-merge entrypoints:
reviewDecision == APPROVED;APPROVEDreview from a non-author, non-OpenCode identity; andStale, predecessor-head, dismissed, comment-only, anonymous, author-self, OpenCode-only, status-only, or synthetic-merge evidence remains non-authorizing. No approval is synthesized and no branch/ruleset protection is weakened.
The mature scheduler engine remains in
scripts/ci/_pr_review_merge_scheduler_core.py;scripts/ci/pr_review_merge_scheduler.pyis the auditable approval-policy facade. This is permanent reviewed source structure, not a temporary repair workflow.Exact-current-head state
Authoritative head:
8abddc8df9ebddcb14e40e818593e71e1c4ca40a.Protected base:
mainat1131b1bbafb24e455fc8619cdf316813e8721861.The current head already repaired the stale source-layout contract so
test_privileged_review_retries_use_default_branch_repository_dispatchfollows the authoritative scheduler core rather than requiring duplicated production literals in the facade.Current exact-head workflow evidence:
31229459086: failed.That Strix quality run checked out exact head
8abddc8d...and completed the central suite with 978 passed, 16 subtests passed, 6 failed. All six failures are stale positive merge-path fixtures intests/test_pr_review_merge_scheduler.py: they still modelreviewDecision: REVIEW_REQUIREDand/or OpenCode-only approval while expecting merge/auto-merge behavior.The remaining test repair must update only those positive fixtures to model the real post-repair authorization state:
reviewDecision: APPROVED, authoritative pull-request author identity, exact-head OpenCode approval, and an exact-head formal approval from a different non-OpenCode reviewer. Negative review-policy fixtures must remain fail-closed. Do not weaken production logic or globally inject approvals into the legacy helper merely to make the suite green.The authoritative legacy test blob observed at this head is
3e421e9039654aa669bf147db3be63fd1386d461. Any head/base/blob movement invalidates this repair plan and requires a fresh read before mutation.Required next validation
After the six fixtures are corrected, rerun the focused independent-approval regression, complete central suite, Strix quick gate, Python compilation, exact 100% production statement/branch coverage and public-docstring coverage, then regenerate every exact-head security/supply-chain and automated-review surface. Keep Draft until zero valid unresolved findings, qualifying independent non-author approval, repository policy, and branch protection all pass without bypass.
Documentation and governance boundary
docs/doctoring/scheduler-independent-current-head-approval.mdandCHANGELOG.mdrecord the incident boundary, exact-head approval semantics, rollback, non-claims, and current GitHub protected-branch/ruleset references. This PR closes #771 only after protected merge; it does not itself solve the organization-wide counted-human-review availability gap tracked separately in #772.