Skip to content

fix(grug): editing a PR body no longer throws away a finished Elder review - #810

Merged
quadseven merged 2 commits into
mainfrom
fix/773-intent-edit-stalls-elder
Aug 1, 2026
Merged

fix(grug): editing a PR body no longer throws away a finished Elder review#810
quadseven merged 2 commits into
mainfrom
fix/773-intent-edit-stalls-elder

Conversation

@quadseven

Copy link
Copy Markdown
Owner

Why

Editing a PR body stalls Elder on the same commit.

Elder's pre-publish freshness guard compared only the freshness id. That id also hashes title and body, so a body edit moved it while the code under review was byte-identical, and the guard discarded the finished review as stale_snapshot.

That discard is not one wasted pass. It re-enqueues against the SAME sha, elder_in_progress_check_skipped: already_in_progress then declines to post a check-run because one is still open, and the check sits pending.

Hit live on quadseven/infra#2133 today, and the irony is the point:

03:00:27  code_reviewer_dispatched   head=a5098494  result=pass
03:08     author edits the body to satisfy Grug - Chief's DoR gate
03:08:11  elder_review_enqueued      head=a5098494  snapshot v1:fc9b1775 -> v1:5ff36c54
03:12:31  code_reviewer_dispatched   head=a5098494  result=publish_failed
03:25:23  code_reviewer_dispatched   head=a5098494  result=pass

Same head_sha throughout. A docs-only PR sat pending for 25 minutes because satisfying one grug check invalidated another.

Summary

  • Head sha decides staleness. When the reviewed sha is still current, the review publishes - the findings anchor to identical lines and only the intent blurb the model also saw has changed. Logged as code_review_intent_drift_publishing_anyway.
  • A genuine push (sha actually moved) still discards as before.
  • The cancel path no longer says "superseded by a newer commit" when the watcher fired on a title/body edit and no commit landed.

The trade-off is explicit and not new: review_freshness_id already documents choosing bounded staleness over never publishing, for base_sha. This applies the same reasoning to the other two fields it hashes. A rewritten intent can legitimately change findings, so the re-enqueued pass still runs - it just no longer destroys a good verdict first.

Acceptance criteria

Size

Size: S

Out of scope

Refs #773

The backlog went from ~16 open issues on 2026-07-01 to 85 on 2026-07-31.
Intake measured 159 issues / 30 days against 90 closed, so it grows no
matter how fast work ships - and 34 of the 85 (40%) belonged to no epic.

Filing is frictionless; linking to an epic means knowing the epic map.
So the cheap half of the workflow runs at 5.3/day and the expensive half
does not run at all. That asymmetry, not any neglected epic, is why the
orphan pool tracks the backlog.

The rule goes in the two places a filer actually reads: the work-item
template (humans, with the live epic list inline) and the agent
issue-tracker doc (agents filing via gh, which never see the template).
`orphan-ok` is a real answer, not a loophole - it makes a standalone
item a visible decision instead of a silent omission.

Also refreshes the doc's label list, which had drifted: adds orphan-ok
and moves epic-security / epic-grug-saas to their archived- names.

closes #808
Elder's pre-publish freshness guard compared only the freshness id. That
id also hashes title and body, so editing the PR body moved it while the
code under review was byte-identical - and the guard then discarded the
finished review as stale.

The discard is not one wasted pass. It re-enqueues against the SAME sha,
`elder_in_progress_check_skipped: already_in_progress` declines to post a
check-run because one is still open, and the check sits `pending`.

Live on quadseven/infra#2133 today: Elder passed at 03:00:27, the author
edited the body to satisfy Chief's own DoR gate at 03:08, and a docs-only
PR went back to pending for 25 minutes. Satisfying one grug check should
not stall another.

Head sha now decides. When the reviewed sha is still current the findings
anchor to identical lines, so they publish; only the intent blurb the
model also saw has changed. A rewritten intent can legitimately change
findings, so the re-enqueued pass still runs - it just no longer destroys
a good verdict first, and the check stays green meanwhile. This is the
same bounded-staleness trade-off review_freshness_id already makes for
base_sha, applied to the other two fields it hashes.

Also stops the cancel path saying "superseded by a newer commit" when the
watcher fired on a title/body edit and no commit ever landed. The cancel
is real; naming the wrong cause is what sent operators hunting.

Two tests asserted the old behavior and encoded the bug: one drove the
full dispatch path with an unchanged sha, the other computed a changed
freshness id but returned the OLD sha as current. Both now match reality.

Covers #773 acceptance criteria 1 and 4, and removes the trigger for 2.
Criterion 3 (bound repeated re-enqueues on an unchanged sha) and the
cave-transport question in item 4 are untouched, so this does not close
the issue.

Refs #773
@quadseven
quadseven merged commit 965829f into main Aug 1, 2026
18 checks passed
@quadseven
quadseven deleted the fix/773-intent-edit-stalls-elder branch August 1, 2026 03:42
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.

1 participant