fix(ci): close the admission-controller coverage/docstring gap on main - #1883
Merged
seonghobae merged 6 commits intoSep 5, 2026
Merged
Conversation
The admission-controller feature burst (#1859-#1869) shipped review_admission_controller.py, pr_review_merge_scheduler_core.py's SchedulerAdmissionGate, and (separately, pre-existing) a coverage gap in audit_codeql_default_setup_rollout.py without full test coverage or docstrings, breaking the required 100% coverage/docstring gate for every PR in this repository regardless of that PR's own diff. The original fix for this landed on .github#1871, which was later closed in favor of narrower successors (#1877 for the stale schedule oracles, #1879 for HTTP error response bodies) -- but the coverage and docstring portion of #1871's delta was dropped in that narrowing and never reached main. This PR recovers exactly that portion from #1871's still-present branch (fix/hourly-review-repair-callers-cron- format-drift) and completes it: - review_admission_controller.py: 85% -> 100% coverage (new tests/test_review_admission_controller.py), 14 missing docstrings added across its WorkerBoundary/AdmissionRequest/RequestRecord/ DispatchLease/ControllerState/DispatchPlan dataclasses and methods. - audit_codeql_default_setup_rollout.py: 79% -> 100% coverage (new tests/test_codeql_default_setup_rollout.py), 2 missing docstrings added (parse_args, main). - pr_review_merge_scheduler_core.py's SchedulerAdmissionGate: 3 missing docstrings added (__init__ and its two nested closures, lease/reconcile_state). Additionally closed pr_review_merge_scheduler_core.py's own separate, longer-standing coverage gap (98% -> 100%, unrelated to the admission-controller work) discovered while verifying this fix would actually bring main to a green gate rather than a differently-shaped 99%: the durable admission gate's own bounded-budget/stale-head branches across every dispatch call site (9 "admission_deferred" checks across post_update_branch_followup/dispatch_draft_review_only/ inspect_pr, plus dispatch_strix_evidence's own two "admission_deferred"/ "stale_head" pairs), reconcile()'s live-head-moved and still-running branches, rotating_pr_window's/dispatch_draft_review_only's/the workflow-run classifier's/the empty-PR-close path's/main()'s own --admission-state-path wiring's remaining gaps, and two untestable package-import fallback lines marked `# pragma: no cover - package import path` matching this file's established convention for that exact pattern. Full local triad: 2875 passed, 1 skipped; coverage 100%; interrogate 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 46 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
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 |
…controller-coverage-docstrings
Contributor
Author
|
Independently reproduced and verified against a fresh, unmodified
No issues found. This closes a real, verified coverage/docstring gate gap with tests that genuinely exercise the branches they claim to. |
…controller-coverage-docstrings
…controller-coverage-docstrings
…s revert #1895 reverted #1889/#1892's model-inference wall-clock caps, restoring opencode-review-dispatch.yml's pre-#1892 content. That content change left REVIEW_DISPATCH_BLOB_SHA in test_pr_review_autofix_nvidia_nim_contract.py pinned to the intermediate #1894 resync value (fca5bfc...), not the now-current content (recomputed directly via git hash-object rather than reasoned from history: ade10b3). Same-day stale-pin pattern already seen this session (#1810, #1874) -- a workflow content change updates most-but-not-all of the tests asserting the old content's exact hash. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…controller-coverage-docstrings
seonghobae
deleted the
fix/review-admission-controller-coverage-docstrings
branch
September 5, 2026 08:10
This was referenced Sep 5, 2026
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
…pline (#1909) Two rules from mistakes this session actually made and corrected, per the per-session lane split agreed with the other concurrent sessions (peer 3 took verification discipline in #1907; peer 2 has gate/merge mechanics; host 1 has close-time diff comparison and noema concurrency; host 2 has CI failure diagnosis). - Narrowing a PR does not carry its delta. #1871 was closed in favor of #1877 plus #1879; both successors were green, but neither carried the coverage/docstring delta, leaving main's required 100% gate broken until #1883 recovered it. "Each piece works" and "the pieces together cover the original's scope" are different questions. - Compare content, not ancestry. main mixes squash and merge commits (last 200: 153 single-parent, 47 two-parent, counted directly), so `git merge-base --is-ancestor` gives false negatives for squashed deltas and false positives for reverted merge-commit deltas. - Never endorse a timeout or retry constant on a model-invocation path without reading docs/product-goal-directive.md section 8, which accepts more than two hours per model and states speed is not a core consideration. #1889/#1890/#1892 each capped a model step at 900s on real multi-hour-hang evidence and were all reverted (#1891, #1895). Every PR number, the section-8 quotes, the parent-count distribution, and the 100% gate values were verified against the repository directly. An earlier draft of the timeout bullet cited a section number that does not exist and attributed a sentence to that file which appears only in #1891's PR body; both were caught by grepping rather than trusting the summary that introduced them, and that failure is recorded in the text. Full suite: 2883 passed, 1 skipped. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
seonghobae
added a commit
that referenced
this pull request
Sep 5, 2026
…uest routing The bash self-test still described the pre-migration architecture and reported 949 failures on this branch's head. Drive it to zero without letting the rewrite bless behaviour the migration did not intend to remove. Retargeted, not deleted, wherever the subject survives: - 119 invocations moved to the gateway contract (orchestrator/free, the pinned loopback base, and the single-attempt model/api-base sequences). - 16 provider-routing scenarios now assert the policy boundary itself: a direct provider route exits 2 with "direct provider/model routes are forbidden". These cover the new boundary, which nothing else tested. - preserve-existing-api-base and the untrusted-api-base case had their expected outcome inverted, because the gate got stricter: it now pins the loopback rather than keeping a caller-supplied base, and refuses an api-base file outside the trusted root rather than ignoring it. - Six new fail-closed scenarios replace the deleted fallback cluster's signal coverage (rate limit, resource-exhausted, quota, token limit, timeout), plus a negative control so the provider-unavailable and generic-failure branches cannot silently collapse into one. Deleted only where the mechanism is gone: 86 multi-attempt scenarios, 11 severity-threshold and fallback-config scenarios, the GitHub Models HTTP 410 cluster (unreachable behind is_github_models_api_compatible_model), and five assert_model_requires_vertex_auth calls whose helper normalises internally and can no longer return 0. Also restores three pull-request-scope calls in run_current_target_scan. 15027c1 rewrote that function by hand right after a merge from main introduced evaluate_pull_request_findings, and dropped all of its call sites along with fail_unmapped_threshold_report and should_fail_pull_request_infra_zero_findings. None of those belong to the "repository-authored retry, model fallback, or severity-threshold admission" this change set out to remove; without them the gate fails a pull request on findings confined to files it never modified, and a zero-findings result produced while the provider was failing reads as a clean scan. load_pull_request_head_sha is left alone: it is orphaned on main too. Verified against an unmodified clone of this same branch head: the self-test goes 949 -> 0, and the rest of the suite is unchanged at 22 failed / 2815 passed, coverage 99%, interrogate 98.3%. Those pre-existing failures are branch staleness (34 commits behind main, missing #1883's coverage and docstring repair) and are not touched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 5, 2026
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
main's required 100% coverage/docstring gate broken for every PR in this repository regardless of that PR's own diff.review_admission_controller.py: 85% → 100% coverage, 14 missing docstrings added.audit_codeql_default_setup_rollout.py: 79% → 100% coverage, 2 missing docstrings added.pr_review_merge_scheduler_core.py: closes both itsSchedulerAdmissionGatedocstring gap (3 items) and a separate, longer-standing 98%→100% coverage gap discovered while verifying this fix would actually bringmainto a genuinely green gate — the durable admission gate's bounded-budget/stale-head branches across every dispatch call site,reconcile()'s live-head-moved/still-running branches, and a few other scattered gaps (see commit message for the full list).Why this exists as a new PR
#1871 (the original fix) was closed in favor of narrower successors whose combined file lists never included these three files. Verified live against a fresh
mainclone before opening this: the exact same 85%/79%/98% gaps are still present onmaintoday.Test plan
coverage run -m pytest tests -q— 2875 passed, 1 skipped, 0 failedcoverage report --fail-under=100— 100%interrogate— 100%🤖 Generated with Claude Code