Tell a re-review what the last review already said - #73
Merged
Merged
Conversation
🤖 AI code reviewDecision: Approve Overall PR risk: Medium. This change modifies existing behavior across reviewer and cross-cutting prompts. Every re-review now carries prior-review state from the previous comment, and both CI paths unconditionally read comment state instead of only when caching is active. The most plausible breakage is that reviewers could over-apply the dismissal signal and fail to re-report legitimately-revived issues on later revisions. Recovery is direct: removing the two call sites in src/core/review.ts restores prior behavior exactly.
No findings. This review is advisory — it never blocks a merge and never auto-approves. |
brentvatne
added a commit
that referenced
this pull request
Aug 8, 2026
Two problems the reviewer caught on #73. The answered set was built from raw feedback records, so ANY reply marked a finding as handled — including a third-party commenter's, a quote without the `id:` token, and a reply to a critical/secrets/security finding. LLP 0011 floors those in code precisely so no reply can clear them, and a block telling the reviewer "a human replied to this" hands it a reason to drop the finding — reinstating through the prompt the suppression the floors refuse. It now uses feedbackApplied, the same predicate the reporter uses, injected rather than imported so the two cannot drift into disagreeing about what "answered" means. The routed CI path never passed priorReview, silently disabling the feature for any repo with routing.jsonc. It now resolves per scope, following the same rule feedbackSeam already uses for which comment holds a scope's history and which fingerprint its records are keyed under: the scope's own comment, or in "single" mode the aggregate's scopes entry paired with the root's dismissal records under the scope-namespaced fingerprint. The scoped read is reused by the cache check rather than fetched twice.
brentvatne
added a commit
that referenced
this pull request
Aug 8, 2026
Third issue in the same family, again found by the reviewer on #73: the prior-review status was computed from a record that had not been through dropStaleVerdict, which both existing merge paths apply first. A verdict is a claim about SOURCE, and fingerprintFinding deliberately excludes the line number, so a finding keeps its identity while the code a rebuttal relied on is edited away. Without the staleness rule, an accepted rebuttal from an earlier revision keeps marking the finding "answered" — telling a later reviewer a human had settled something that was judged against code no longer under review. Both call sites now chain dropStaleVerdict against the reviewed head before feedbackApplied, matching mergeFeedback and the aggregate merge. Only reachable under dismiss: "adjudicated", the one path that consults a verdict at all.
A re-review started from nothing. Every pass ran stateless, so the reviewer could not tell a second push from a first look, and a finding a maintainer had already dismissed came back on the next run in slightly different words — fingerprint suppression catches a near-identical repeat, not a reworded re-raise. The state was already in hand. ReviewState round-trips the whole previous CoordinatorOutput — every finding with file, line, severity and category — plus dismissed, feedback and pins, through the reviewer's own PR comment on every run. It was decoded each run and consulted only afterward, for suppression and reply matching, and never shown to the model. summarizePriorReview reduces it to a bounded list and priorReviewSection renders one fenced block for the reviewer and cross-file passes. No new storage, no new fetch: ci.ts now reads that state once and reuses it for both the cache check and the prompt. Carrying status is what earns the block. A maintainer's dismissal and an author's reply both happen AFTER a run ends, so no engine session or replayed transcript could ever contain them — this channel is the only one that can. A pin outranks both, because /undismiss is the human's last word that a finding still stands. Treated as untrusted, because it is: the titles and paths are model output produced by reading an untrusted PR, which is the same reason stripStateMarkers exists. Every entry is flattened onto one line and the assembled block is swept for a forged BEGIN/END PREVIOUS REVIEW fence, exactly as context-file text is. Two deliberate boundaries. The coordinator never receives it: it merges and decides, and handing it the previous decision is how a decision drifts by inheritance rather than by evidence. And the wording frames prior findings as claims to re-check rather than conclusions to carry forward, and says outright that absence from the list means nothing — a reviewer that restates last run's list without re-deriving it has stopped reviewing, and recall is the product. Deliberately NOT in the review-cache key, against that file's own "every input joins the hash" rule. The block derives from the previous result, so including it would change the hash the moment a first review exists and miss on every re-review, disabling the cache exactly where it pays. A hit already means the diff, files, config and metadata are byte-identical, so reusing that run's conclusions is what the block would have told the model to do anyway.
Two problems the reviewer caught on #73. The answered set was built from raw feedback records, so ANY reply marked a finding as handled — including a third-party commenter's, a quote without the `id:` token, and a reply to a critical/secrets/security finding. LLP 0011 floors those in code precisely so no reply can clear them, and a block telling the reviewer "a human replied to this" hands it a reason to drop the finding — reinstating through the prompt the suppression the floors refuse. It now uses feedbackApplied, the same predicate the reporter uses, injected rather than imported so the two cannot drift into disagreeing about what "answered" means. The routed CI path never passed priorReview, silently disabling the feature for any repo with routing.jsonc. It now resolves per scope, following the same rule feedbackSeam already uses for which comment holds a scope's history and which fingerprint its records are keyed under: the scope's own comment, or in "single" mode the aggregate's scopes entry paired with the root's dismissal records under the scope-namespaced fingerprint. The scoped read is reused by the cache check rather than fetched twice.
Third issue in the same family, again found by the reviewer on #73: the prior-review status was computed from a record that had not been through dropStaleVerdict, which both existing merge paths apply first. A verdict is a claim about SOURCE, and fingerprintFinding deliberately excludes the line number, so a finding keeps its identity while the code a rebuttal relied on is edited away. Without the staleness rule, an accepted rebuttal from an earlier revision keeps marking the finding "answered" — telling a later reviewer a human had settled something that was judged against code no longer under review. Both call sites now chain dropStaleVerdict against the reviewed head before feedbackApplied, matching mergeFeedback and the aggregate merge. Only reachable under dismiss: "adjudicated", the one path that consults a verdict at all.
In "single" comment mode one aggregate comment holds every scope, and its state was read only when the result cache was live. But cacheAllowed excludes any run with feedback or stack enabled, so the prior-review block silently vanished for exactly the repos whose dismissals and replies make it worth having. It is now read whenever the mode calls for it. Cache reuse stays gated separately on cacheReadRoot; this only decouples the read from it.
brentvatne
force-pushed
the
prior-review-context
branch
from
August 8, 2026 22:52
64a087b to
e4bbd6a
Compare
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.
First step of roadmap item A (#72). Closes the cheapest gap in cross-run reuse: no prior-run state reached the prompt at all, so every re-review started from zero.
The state was already there
ReviewState(render.ts:518) round-trips the whole previousCoordinatorOutput— every finding with file, line, severity, category — plusdismissed,feedbackandpins, through the reviewer's own PR comment on every run. It was decoded each run and consulted only afterward, for suppression and reply matching. The model never saw it.So this needs no new storage and no new fetch.
ci.tsalready calledreporter.readState()inside the cache check; it now reads once and reuses that for both the cache check and the prompt.What it buys
Fingerprint suppression already catches a near-identical repeat. It does not catch a reworded re-raise, which is the common annoyance: a maintainer dismisses a finding, the next push brings it back phrased differently.
Carrying status is the part that earns the block — and it is something session caching could never do. A maintainer's dismissal and an author's reply both happen after a run ends, so no engine transcript could contain them. This channel is the only one that can.
Trust
Not trusted input, and not treated as such. The titles and paths are model output produced by reading an untrusted PR — the same reason
stripStateMarkersexists. So each entry is flattened onto one line viaflattenUntrustedand the assembled block is swept for a forged----- BEGIN/END PREVIOUS REVIEW -----fence, exactly as context-file text is. Covered by a test that plants a fence-forging title and asserts exactly one real BEGIN/END pair survives.Two deliberate boundaries
One thing worth a second look
priorReviewis deliberately not in the review-cache key, againstreview-cache.ts's own "bump this whenever review inputs gain a new source" rule. It derives from the previous result, so including it would change the hash the moment a first review exists and miss on every re-review — disabling the cache exactly where it pays, right after #71 unblocked it for researched runs. A hit already means diff, files, config and metadata are byte-identical, so reusing that run's conclusions is what the block would have told the model to do. The reasoning is recorded in the type and in LLP 0004 so it doesn't read as an oversight.Verification
tscclean ·oxlintclean ·oxfmtclean ·ref-check0 errors · 705 tests pass (+7).New tests cover status derivation, pin precedence over a reply, the cap and its omitted count, fence forgery, the anti-anchoring wording, and that reviewer + cross-file carry it while both omit it cleanly when there is no prior review.
LLP 0004 gains a Prior-Review Context section in the same commit, per AGENTS.md.
🤖 Generated with Claude Code