Skip to content

docs(gap-baseline): record review_admission_controller.py coverage/docstring gap - #1876

Open
seonghobae wants to merge 4 commits into
mainfrom
docs/gap-baseline-review-admission-controller-coverage
Open

docs(gap-baseline): record review_admission_controller.py coverage/docstring gap#1876
seonghobae wants to merge 4 commits into
mainfrom
docs/gap-baseline-review-admission-controller-coverage

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Summary

scripts/ci/review_admission_controller.py (added by #1862/#1863, "bounded durable review admission core") is below this repo's own 100% coverage and 100% docstring hard gates on protected main. Reproduced directly on a clean origin/main@a9aeee8f checkout, independent of any open PR's own diff:

  • coverage run -m pytest tests -q && coverage report --show-missing → 99% (13,107 statements / 5,288 branches; 95 statements and 77 branch-parts missed, effectively all of them this module's validation raise branches — AdmissionRequest.create's type/range checks, the durable-state JSON shape/consistency checks in _read_state, the symlink guards around the state file and its lock/backup paths, and the stale-request/lease-mismatch branches in plan_dispatches/complete_dispatch).
  • interrogate → 98.3% on scripts/ci, missing ~21 docstrings, all on the same module's public surface (WorkerBoundary, AdmissionRequest and its methods, RequestRecord, DispatchLease, ControllerState and its methods, _read_state, DispatchPlan, SchedulerAdmissionGate and its nested closures).

Independently reproduced (same miss counts) while merge-repairing #1812's unrelated Rust-regex fast-path change, confirming this predates that PR. No open PR was found already carrying a fix (checked via search_pull_requests). This means every open PR based on current main inherits both gate failures regardless of its own diff — a local coverage/interrogate run against main cannot be used as clean baseline evidence until this is repaired.

Also resolves a stale note in the adjacent 2026-09-04 gap-baseline entry: the hourly-cron test it flagged as "not yet fixed" (test_review_fix_caller_runs_once_each_hour) was in fact renamed and repaired by #1870's cron-pin work, now on main and passing as test_review_fix_caller_keeps_the_github_daily_recovery_slot.

Scope

Documentation only — one dated entry appended to docs/product-technical-gap-baseline.md, plus a short resolved-note on an adjacent stale entry. No source, test, or workflow change. Repair of the underlying gap (targeted unit tests for the validation error branches, plus docstrings) is left for a dedicated follow-up per this doc's own convention of not bundling live-code changes into a documentation-only entry.

Test plan

  • python3 -m pytest tests/test_product_technical_gap_baseline.py -q → 5 passed

🤖 Generated with Claude Code

https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4


Generated by Claude Code

…cstring gap

scripts/ci/review_admission_controller.py (added by #1862/#1863) is below
this repo's own 100% coverage and 100% docstring hard gates on protected
main, reproduced directly on a clean origin/main@a9aeee8f checkout: 99%
coverage (95 statements / 77 branch-parts missed, effectively all of them
this module's validation raise branches) and 98.3% docstrings (~21 missing
on its public surface). Independently reproduced while merge-repairing
#1812's unrelated fast-path change, confirming it predates that PR. Every
open PR based on current main inherits both gate failures regardless of its
own diff. Also resolves a stale note in the adjacent 2026-09-04 entry: the
hourly-cron test it flagged as "not yet fixed" was in fact renamed and
repaired by #1870's cron-pin work, now on main and passing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 19 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: b9705658-5e83-4804-93c2-d5d473d44271

📥 Commits

Reviewing files that changed from the base of the PR and between f250638 and 9fe4eeb.

📒 Files selected for processing (1)
  • docs/product-technical-gap-baseline.md

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.

Copy link
Copy Markdown
Contributor Author

agent-review-runtime-quality failed with the same ~39 stale test-oracle failures already fixed on main since this PR's base (a9aeee8) — stale hourly-cron schedules, changed-scope byte-identical drift, noema close-cleanup step rename, pr_review_merge_scheduler fixture drift, plus the known local-sandbox-only gh-CLI limitation in test_strix_rerun_job_selection.py. This PR itself only touches docs/product-technical-gap-baseline.md.

Merged current main (8272e4f95) into this branch and validated: full suite coverage run -m pytest tests -q → 2832 passed, 1 skipped, 21 subtests passed, zero failures; git diff --check clean. Pushed as ba30cd1d1e... (merge commit, no force).


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

agent-review-runtime-quality failed on ba30cd1d1 with the same known stale-tests/test_pr_review_merge_scheduler.py-fixture pattern (17 failures, already fixed on main) — but the branch was auto-updated to current main (6d7fbebec) immediately after, landing at 85fdca4bc, before I could act. All checks are now freshly queued against that head. No fix needed here; watching the fresh run.


Generated by Claude Code

Copy link
Copy Markdown
Contributor Author

Retracting this PR's premise — the gap it documents no longer exists on main.

This PR records scripts/ci/review_admission_controller.py as leaving the repo below its 100% coverage / 100% docstring gates. That was measured against origin/main@a9aeee8f. It is no longer true:

So merging this PR as written would append a dated entry to docs/product-technical-gap-baseline.md asserting a deficit that has already been repaired — a false claim in the canonical gap snapshot.

Two further reasons not to merge it in its current shape:

  1. The measurement was taken from a stale checkout. This session's .github working tree was 218 commits behind origin/main at the time (docs/product-technical-gap-baseline.md: 2615 lines locally vs 3234 on main). That is the same failure mode this PR's own subject matter is about.
  2. The gap-baseline file is contended right now. PRs docs(gap-baseline): root-cause item 4's provider-pool stall, 8 data points #1903/docs(items15-17): measure Detect changed scope gate-job runner waste #1904/docs(gap-baseline): measure PR job fan-out, disprove the workflow-waste hypothesis #1905 are appending to the same anchors from other sessions, so an additional append here is a merge-conflict source with no remaining informational value.

Requesting closure rather than closing it myself, per the org convention that supersession is recorded as a comment with SHAs and the author does the closing.

The durable part of this work — the method for catching dated-evidence rot, including the corrected KPI baseline and the working-tree-staleness rule that would have prevented this false reading — is carried in #1913 (docs/doctoring/ci-failure-signature-triage.md) instead, which touches no contended file.


Generated by Claude Code

…coverage

Both sides appended to `docs/product-technical-gap-baseline.md` at the same
anchor. Kept both, in this order, because the order is structurally
constrained rather than arbitrary:

- This branch's 32 lines open with "**Resolved — 2026-09-04.**" and resolve the
  item described in the paragraph immediately above the conflict ("Separately
  found while validating this fix, not yet fixed"). They must stay attached to
  that parent paragraph.
- Main's 118 lines are a new top-level section ("Items 15/16/17 measurement"),
  which follows.

Placing main's section first would have separated the resolution note from the
finding it resolves. Verified after resolution that the note sits directly
after its parent paragraph (lines 3236/3238) and that fenced code blocks are
still balanced.

Verified after resolution: 2890 passed, 1 skipped, coverage 100%,
interrogate 100%; zero conflict markers; `ruff check --select F821` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copy link
Copy Markdown
Contributor Author

Escalating the retraction: this PR is now consuming CI capacity on every main advance

I retracted this PR's premise earlier and asked for closure. Adding one fact I did not have then, because it changes this from "harmless stale PR" to an ongoing cost.

It is being auto-updated. The head moved to 9fe4eeb7e at 2026-09-05T12:57Z — a fresh Merge origin/main roughly thirteen minutes after main reached f25063882. That is the third such merge on this branch today, and it will keep happening as long as the PR is open.

Every one of those updates enqueues the full required-workflow set — opencode-review, strix, noema-review, pr-review-merge-scheduler — into a queue currently sitting at roughly 55 of 60 running jobs organization-wide, where a review run needs about 4.7 hours of wall clock to survive to a verdict. So this PR is spending shared review capacity, once per main advance, on a document whose claim is false.

The premise remains false on current main. The gap this PR documents — scripts/ci/review_admission_controller.py below the 100% coverage and docstring gates — was closed by #1883 (merged 2026-09-05T08:10:23Z, landing as 71dd84d4). My full-suite run against a tree containing current main reports TOTAL … 0 missing … 100% with interrogate at 100.0%; a total of zero missing statements is zero missing for every file in scripts/ci, this one included. That was over four and a half hours ago.

Merging it would write **Status:** found, not yet fixed. into the canonical gap snapshot for a gap that is fixed.

The ask is unchanged and I am not performing it: close without merging. I will not close it myself, and I am not pushing to the branch — a push would only enqueue another round of the checks described above.

One salvageable fragment, if anyone wants it: the adjacent **Resolved — 2026-09-04.** note about test_review_fix_caller_keeps_the_github_daily_recovery_slot is still accurate and would need re-proposing as its own small edit rather than riding along with the obsolete section.

Noting for the record that the auto-updater is doing exactly what it should — the problem is that nothing stops it for a PR whose content has been retracted, so the retraction has to be acted on by a person to take effect.


Generated by Claude Code

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.

2 participants