feat(review): say when a review is still running, and refuse to submit until it is not - #23
Merged
Conversation
…t until it is not A reader cannot tell "nothing found" from "not finished looking". On a real review here the findings arrived five minutes after the browser opened, and the pull request had already been approved by then. An agent now declares the run: `agent review-start [--note]` and `agent review-done`, kept in a review_runs table so existing databases migrate by creating it. While a run is open the diff page carries a loud banner with the finding count so far, and the forge dialog disables submission and says why. An unfinished run follows the session when a commit moves HEAD, so committing mid-review does not silently clear the warning. Also fixes three ways a tab could quietly show a stale review: - carrying threads into a new session left a tab asking about the old one, which truthfully answered "no threads". Session ids are now resolved server-side, following a superseded session to the one that took its threads, and the parameter is optional. - /api/info never refetched, so a tab kept a dead session id and would have filed new comments against it. It polls. - the staleness fingerprint hashed only the diffstat, which counts lines without reading them, so a commit rewriting the same number of lines looked identical. It includes the head commit. And PR mode no longer fails on a merged pull request: gh can only check out a branch, which is usually deleted on merge, so it falls back to refs/pull/<n>/head. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs
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.
A reader cannot tell "nothing found" from "not finished looking". This happened for real today:
on admin3#671 the browser opened at setup, the findings arrived five minutes later, and the pull
request had already been approved in between.
An agent now declares the run.
agent review-start [--note]andagent review-done, kept in areview_runstable so existing databases migrate by simply gaining it — the schema here iscreate-only, with no ALTER path. While a run is open:
An unfinished run follows the session when a commit moves HEAD, so committing mid-review does not
silently clear the warning.
review-doneis required even when nothing was found, or the guard staysup — the skill says so explicitly.
Three ways a tab could quietly show a stale review, all found while chasing "the previous
comments were hidden on update":
?session=is optional — the client should not be the authority on session identity/api/infonever refetched, so a tab kept a dead session id indefinitely and would have filed new comments against it, invisible. It polls every 5sone\ntwo→one\nTWOand asserts the fingerprint changesAnd PR mode no longer dies on a merged pull request.
gh pr checkoutcan only check out a branch,and merging usually deletes it —
fatal: couldn't find remote ref refs/heads/DEV-25255, which isexactly what happened when #671 merged mid-session. It falls back to
refs/pull/<n>/head, verifiedagainst that real merged PR.
Written test-first throughout: the guard test was red on "submit is refused while a review runs" and
green on "allowed once finished" before the implementation existed. 283 tests pass.
Stacked on #22.
🤖 Generated with Claude Code
https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs