fix(grug): Elder reviews what it can instead of giving up on a big diff - #807
Merged
Conversation
A hunk larger than one whole cohort can never be reviewed. The planner
will not truncate it - truncation corrupts the line anchors the
hallucination filter depends on - so it became a solo cohort that
_refuse_oversized_cohort auto-failed WITHOUT calling a model.
That one blob caused three separate harms:
- a guaranteed cohort failure
- a `partial_review` flag that suppressed the whole check
- an inflated total_diff_chars that split the rest of the diff into
more cohorts than the wall-clock budget could run
Measured on the triggering PR: a generated .json rewritten by 22,602 lines
produced a 1,004,156-char hunk against a 48,000 cap. Cohorts 5 and 9
auto-failed and cohorts 15-18 were never attempted - four healthy
cohorts lost to one file. The identical 1,004,156 appears on two later PRs there too, because once it landed on main every later Living Hunt range
re-ingested it.
split_oversized_hunks now drops it before planning and NAMES it, reusing
the honest-omission channel split_reviewable_hunks already established.
Per hunk, not per path, so a hand-edited hunk in the same file still
gets reviewed. A non-positive budget means unbounded, so a misparsed
config value can never drop the whole diff.
The summary says so in its own words: calling a real code change
"data/generated ... no meat for review there" would be a lie.
Claude-Session: https://claude.ai/code/session_01FF4AW61D3JkNQeb6mbhoSQ
plan_review optimized purely for SIZE while the executor was bounded by TIME, and neither knew about the other. Cohorts run SERIALLY and the executor stops once `elapsed + reserve > budget`, where reserve is the full per-call LLM timeout - at the live config (budget 700s, timeout 330s) that leaves ~370s of real cohort time. One downstream PR planned 18 cohorts into that. Cohorts 15-18 were never attempted and surfaced as `failed_cohorts`, which reads as a model outage rather than "this PR is too big to review in full". A second run that day was worse: 14 planned, first_skipped_cohort=2 - ONE cohort ran and the review was 93% blind while still reporting as merely degraded. max_cohorts truncates the plan up front and names the remainder as a `plan-truncated` reviewability concern. The surviving prefix is the layer-ordered head - _ordered_areas already sorts contract -> implementation -> verification -> documentation, so what stays is the most load-bearing content, not an arbitrary slice. total_diff_chars still reports the TRUE size of the change, not the reviewed slice, so a diff that keeps outgrowing the budget is visible in the logs. Default of 10 is derived, not guessed: two runs measured 2026-07-31 completed 14 cohorts (~26s each) and 9 (~41s each) before the budget check tripped. It is a backstop - with oversized hunks now dropped before planning, real diffs land well under it. Claude-Session: https://claude.ai/code/session_01FF4AW61D3JkNQeb6mbhoSQ
_run_staged_cohorts did `responses.append(run_cohort(i))` - exactly one
try. A transient backend blip or a single unparseable completion was
therefore permanent, and ONE permanently-failed cohort flips the whole
check to `partial_review`, suppressing the findings from every cohort
that did succeed.
A cohort now gets two attempts. Retries are skipped when:
- the failure is deterministic - `no_diff`, and the oversized-hunk
refusal, which never called a model and whose input does not shrink
between attempts. The refusal error carries a stable prefix so the
scheduler can tell it from a transient failure instead of matching
free text.
- the review was cancelled
- the budget can no longer afford one, so a retry never eats the time
the remaining cohorts need
Stops at two on purpose: the failure modes that survive one retry (a
sustained outage, a model that cannot produce parseable output for this
input) are not fixed by a third, and further attempts spend budget the
remaining cohorts need.
The scheduling test now pins max_attempts=1 - it exists to prove cohorts
run one at a time, and retry has its own tests.
Claude-Session: https://claude.ai/code/session_01FF4AW61D3JkNQeb6mbhoSQ
_RETRYABLE_SKIP_REASONS omits partial_review, so it fell through to the branch whose own comment reads "Unknown skip: still fail-open" - which re-posted a generic "Elder skipped - partial_review" OVER the complete check Elder had already published, naming which cohorts completed. That is why the triggering PR showed "Elder skipped - partial_review" on the check while the board said "Degraded (partial_review) - advisory only": two surfaces describing one pass in different words, because two different code paths wrote them, and the later one knew less. partial_review joins no_diff and fail_open_freshness in a named _SELF_COMPLETING_SKIP_REASONS set - skips that already published their own terminal check, so the fail-open net must leave them alone. It falls through to the normal claim settlement exactly as no_diff does. The fail-open net still catches reasons nobody modelled; there is now a test pinning that, so removing partial_review from its blast radius cannot quietly remove the net itself. Claude-Session: https://claude.ai/code/session_01FF4AW61D3JkNQeb6mbhoSQ
Three surfaces described one event in three ways: the check said "Elder skipped - partial_review", the board said "Grug eyes cloudy this pass - read for self", and _summary_markdown had a third wording that never rendered because rerun.py overwrote it (fixed in the previous commit). The board wording was the actively harmful one. "Grug eyes cloudy - read for self" means Elder saw NOTHING. On the triggering PR it reviewed 12 of 18 cohorts; using the blackout sentence tells the author to disregard a pass that produced real, diff-anchored evidence. is_real_degradation now splits into is_blackout (saw nothing) and is_partial_coverage (saw most of it). Both are still "news" for worth_an_email - they are simply not the same news. Ordering on the board: a blocking finding outranks the coverage caveat, because it is the most actionable fact there. Partial coverage only displaces "Trail clear", which is the one sentence it would turn into a lie - an all-clear over ground Elder never walked. Claude-Session: https://claude.ai/code/session_01FF4AW61D3JkNQeb6mbhoSQ
![grug-tribe[bot]](https://avatars.githubusercontent.com/in/3589276?s=60&v=4){kind=small}
Grug look hard. Trail clear.fix(grug): Elder reviews what it can instead of giving up on a big diff Grug Elder - Clear - no markings published - check `success`
|
Elder's high-complexity rule caught this PR pushing _review_stack_body from cyclomatic 14 -> 17 against a cap of 15 (cognitive 11 -> 17). True positive: commit d64400a added the partial-coverage branch to both the status line and the closing note. Extracted the two self-contained decisions: - _stack_status_line - coverage vs findings, one line out - _stack_closing_note - what to do next, or why there is nothing Measured with grug's OWN complexity scanner (caps 15 / 25): _review_stack_body 10 cyclo / 5 cog (was 17 / 17) _stack_status_line 5 / 3 _stack_closing_note 4 / 3 Pure refactor - 1867 webhook tests pass unchanged, no behavior touched. AI-REVIEW: grug Elder (qwen3-coder-next:q8_0), grug#807, rule high-complexity, medium. Verified true positive and fixed rather than rebutted - the counts were correct. Claude-Session: https://claude.ai/code/session_01FF4AW61D3JkNQeb6mbhoSQ
quadseven
force-pushed
the
fix/elder-partial-review-resiliency
branch
2 times, most recently
from
July 31, 2026 20:36
b054d55 to
7b017cb
Compare
This was referenced Jul 31, 2026
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.
Why
A downstream private-repo PR published "Grug eyes cloudy this pass - read for self" and zero findings on a 13-file PR. Elder had not broken; it hit a wall it built itself, and five layers each declined to recover.
Root cause: the review-exclusion regex covers
.jsonlbut not.json. A commit there rewrote a generated data table by 22,602 lines, producing a 1,004,156-char single hunk against a 48,000 cap. The same byte count recurs on three later PRs there because once it landed on main, every later Living Hunt range re-ingested it.That one blob cost four unrelated healthy cohorts:
failed_cohorts: [5, 9, 15, 16, 17, 18]- 5 and 9 auto-failed, 15-18 were never attempted once the budget died.Summary
Five commits, one per layer, each independently revertable:
e94c21e9b5f02bplan_reviewtakesmax_cohorts, so the plan is bounded by what one pass can RUN; the remainder becomes aplan-truncatedconcern instead of mystery cohort failures0c9bdf07b72220partial_reviewjoins_SELF_COMPLETING_SKIP_REASONS, so the fail-open net stops overwriting Elder's own check with "Elder skipped"d64400ais_real_degradationsplits intois_blackoutvsis_partial_coverageDeliberately NOT done: excluding
.jsonwholesale.package.jsonandtsconfig.jsonare real review targets, and an existing test pins that. The guard is size-based, so it generalises to any generated blob without a path list to maintain.Acceptance criteria
partial_reviewstops being treated as an unknown skip, so Elder's own check is not overwrittenTest plan
Full suite green, no skips introduced:
make webhook-test- 1867 passed, 2 pre-existing skipsmake api-test- 288 passedmake scripts-test- 32 passedNew coverage: 5 tests for the oversized split, 6 for budget-aware planning, 7 for cohort retry, 2 for skip classification (including one pinning that the fail-open net still catches genuinely unknown reasons), 6 for the vocabulary split.
Replaying the real cohort sizes from the
llm_staged_review_plannedDD log through the new code:Zero guaranteed-failure cohorts, and the plan fits the runnable bound.
Size
Size: M
Out of scope
Dependencies
closes #806