fix(grug): stop killing a running review because the author edited the body - #814
Merged
Merged
Conversation
Live on quadseven/infra#2157, a PR breaking a packet-mode bootstrap deadlock with per-leg keepalives: 13:51:12 llm_staged_review_planned cohort_count=2, 43111 chars 13:52:43 (author edits the PR body) 13:52:51 code_review_llm_degraded kind=all_failed error="cancelled: review input changed while the review was running; cohort skipped: review cancelled" 13:52:53 elder_review_stack_upserted -> board CREATED -> email Grug threw away a running review of a hard concurrency change and mailed "Grug eyes cloudy this pass - read for self" carrying one cyclomatic nitpick. A retry at 13:56:51 came back "no bad omens from the Cave", but GitHub does not mail an EDIT, so that inbox keeps the degraded verdict. Two independent causes, both fixed here. 1. The staleness watch cancelled on `review_freshness_id`, which hashes title and body. Editing a description aborted both Cave arms mid-generation even though the diff was byte-identical. Head sha now decides, matching the pre-publish guard fixed under #773 - that guard already said "same sha, publish anyway" and duly logged `code_review_intent_drift_publishing_anyway` on this very PR, but had nothing left to publish because this loop had already killed the work. Same rule, both halves. The watch had NO test coverage; it has three now. 2. `worth_an_email` created the board for any real degradation. But a degradation the durable lane RETRIES is not news yet, and deterministic findings are not lost by waiting - the retry re-runs the same scanners. The retry set now lives in one place, `RETRIED_DEGRADATIONS`, which rerun imports rather than restates, so the retry lane and the email gate cannot drift. Two existing tests pinned `all_failed` as the exemplar of "a degradation must mail". Their guarantee is intact and still asserted - a TERMINAL blackout mails - but the exemplar moved to a reason that is actually the last word. Trade, stated in the docstring: if every retry also fails the author gets no board, and the signal lives in the check-run (published neutral, as this PR's own email shows) plus the DLQ poison monitor. A false "Grug could not see" is worse than a quiet one, because it teaches the author to discount the surface. Suites: 1878 webhook, 288 api, 24 infra. Refs #773
5 tasks
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
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
Grug threw away a running review of a hard concurrency change because the author edited the PR description, then mailed the human a cyclomatic-complexity nitpick in its place.
Live on quadseven/infra#2157, "break the packet-mode bootstrap deadlock with per-leg keepalives":
The email said "Grug eyes cloudy this pass - read for self" and carried one
high-complexitymarking. The retry six minutes later reviewed it properly, but GitHub does not mail an EDIT, so that inbox keeps the degraded verdict permanently while the PR page shows the good one.Summary
Two independent causes, both fixed.
1. The staleness watch cancelled on the wrong identity. It compared
review_freshness_id, which hashes title and body, so editing a description aborted both Cave arms mid-generation on a byte-identical diff. Head sha now decides - killing live work is only justified when the diff changed, because that is the only case where the in-flight answer is worthless.This is the other half of #773. That fix taught the pre-publish guard "same sha, publish anyway" - and the guard duly logged
code_review_intent_drift_publishing_anywayon this very PR. It had nothing to publish, because this loop had already destroyed the generation. Same rule, both halves.The watch had no test coverage at all, which is part of why this survived. It has three tests now.
2. A retried degradation created the board.
all_failedis in the durable lane's retry set, so a better answer was always coming. Deterministic findings are not lost by waiting - the retry re-runs the same scanners and republishes them.RETRIED_DEGRADATIONSnow lives in one place andrerun.pyimports it rather than restating it, so the retry lane and the email gate cannot drift.Acceptance criteria
test_staleness_watch_does_not_cancel_on_an_intent_only_edittest_staleness_watch_still_cancels_on_a_new_committest_staleness_watch_ignores_a_transient_fetch_failuretest_no_board_for_a_degradation_that_will_be_retried, driven with the exact infra#2157 shape (deterministic finding present, LLM halfall_failed)test_terminal_blackout_still_earns_a_boardSize
Size: M
Out of scope
Note on two changed tests
test_worth_an_email_is_findings_or_degraded_onlyandtest_no_diff_is_not_an_alarmboth pinnedall_failedas the exemplar of "a degradation must mail". Their guarantee is unchanged and still asserted - a TERMINAL blackout mails - but the exemplar moved to a reason that is actually the last word. The retried case now has its own assertion in the same test.Refs #773