Skip to content

feat(grug): every review says which commits it read - #817

Merged
quadseven merged 1 commit into
mainfrom
feat/673-living-stack-status
Aug 3, 2026
Merged

feat(grug): every review says which commits it read#817
quadseven merged 1 commit into
mainfrom
feat/673-living-stack-status

Conversation

@quadseven

Copy link
Copy Markdown
Owner

Why

"Looked at: <range>" was emitted only when a Living Hunt delta existed. A full base..head review - the common path, and every first review of any PR - answered "what did you actually read" with nothing.

That is the first question a skeptical reader asks about a review, and on most PRs it had no answer.

Summary

Both cases disclose now, and both name a range rather than a single commit: base..head is what the author pastes into git diff to see exactly what Grug saw.

One implementation, board.review_scope_line. The full-diff branch was briefly written twice - once there, once inline in dispatch.py with its own wording and its own head_sha[:7] - so the same fact appeared in two vocabularies on the same surface.

Acceptance criteria

  • A full base..head run discloses its scope - test_stack_discloses_scope_on_a_full_diff_run_too
  • A Living Hunt delta run still discloses its range unchanged
  • The disclosure is a RANGE, not one commit - asserted as base987..c0ffee1
  • No dangling "Looked at:" when there is no sha to name - test_stack_detail_without_a_head_sha_still_renders
  • One implementation of the fact, not two
  • Suites: 1883 webhook, 288 api, 24 infra

Size

Size: S

Out of scope

#673's in-progress note is deliberately NOT here. It was implemented, reviewed and removed before this PR rather than shipped. Two-axis review found it net-negative:

  1. It omitted app_id, so find_board could match any comment quoting the board marker - including a human's. And because the key is elder, board_client's legacy branch (if key != "elder") discards existing text rather than preserving it, so a human quoting the marker would have had their comment replaced wholesale.
  2. Nothing cleared it. The only terminal header write is gated on not check_publish_failed, so a publish failure, a consumer crash into the DLQ, post-settle ineligibility, or the fail-open path would each leave "Wait" on the board permanently - with the prior findings already overwritten. Strictly worse than the staleness it was meant to fix.

Re-filed with both as hard acceptance criteria.

#673 item 3 (disclose tier/arms/model) is also out. Per operator decision it moves to the dashboard rather than the author's PR comment - it is useful to whoever runs Grug, not to whoever wrote the PR.

Refs #673

"Looked at: <range>" was emitted ONLY when a Living Hunt delta existed.
A full base..head review - the common path, and every FIRST review of any
PR - answered "what did you actually read" with nothing at all. That is
the first question a skeptical reader asks about a review.

Both cases now disclose, and both name a RANGE rather than a single
commit: `base..head` is what the author pastes into `git diff` to see
exactly what Grug saw.

One implementation, `board.review_scope_line`. The full-diff branch was
briefly written twice - once there and once inline in dispatch with its
own wording and its own `head_sha[:7]` - so the same fact appeared in two
vocabularies on the same surface.

Scope note: #673 also asks for an in-progress note on re-review. That was
implemented, reviewed, and REMOVED before this commit rather than shipped:
it omitted `app_id`, so it could match and overwrite a human's comment
(board_client's legacy branch discards content when key == "elder"), and
nothing cleared it when a review died, so a crashed review would have left
"Wait" on the board permanently with the prior findings already erased.
Worse than the staleness it was meant to fix. Re-filed with those two
constraints as acceptance criteria.

Suites: 1883 webhook, 288 api, 24 infra.

Refs #673
@quadseven
quadseven merged commit af66c0c into main Aug 3, 2026
18 checks passed
@quadseven
quadseven deleted the feat/673-living-stack-status branch August 3, 2026 01:29
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