Tolerate prior-finding dispositions for externally resolved findings - #11
Merged
Merged
Conversation
A reviewer who resolves a bot thread on GitHub while a review round is queued or running makes prepare's sync_manifest_threads close that finding, but the manifest handed to the model still contains it. The model reads the thread, agrees it is fixed, and returns a `resolved` disposition -- and compile rejected the whole round because the ID was not in the open set, discarding an otherwise clean review pass. Validate prior_findings IDs against manifest membership rather than open status, so only a genuinely unknown ID is a contract violation. For a finding already resolved with `thread_resolution: confirmed`, ignore the model's disposition entirely: a concordant `resolved` is a no-op, and an `open` must not reopen a thread a human deliberately closed. Tell the model in the prompt to disposition only findings whose status is `open`, so the tolerance path is a backstop rather than the normal case.
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.
Summary
A human resolving a bot review thread while a review round is queued or running fails the entire
pr-reviewcheck withPRIOR_FINDING_INVALID, discarding a clean review pass. Hit live on megaeth-labs/specification#3, reproduced on rerun.Sequence:
preparecallssync_manifest_threads, seesisResolved: trueon the thread, and sets the finding tostatus: resolved/thread_resolution: confirmed.review-input.json, so the model still sees the finding, its thread, and the reply. It correctly assesses it as fixed and returnsdisposition: resolved.compilebuiltexpected_prior_idsfrom open findings only and raisedPRIOR_FINDING_INVALIDfor anything outside that set.The model agreed with the pipeline's own state and the pipeline failed the run for saying so.
Changes
prior_findingsIDs against manifest membership rather than open status, so only a genuinely unknown (e.g. hallucinated) ID is a contract violation. Error text updated to match.resolvedwiththread_resolution: confirmed, ignore the model's disposition and log it. A concordantresolvedbecomes a no-op, and anopencannot reopen a thread a reviewer deliberately closed — external human resolution stays authoritative.action.yml): state that dispositions are expected only for findings whose status isopen, and that already-closed findings appear as context so they are not re-raised. Keeps the tolerance path a backstop rather than the normal case.opencannot reopen an externally-resolved finding; unknown ID still fails. Existing unknown-ID test updated for the new message. 85 tests pass.prior_questionsvalidation is unchanged — it has no equivalent external-resolution path.Notes
__pycache__/*.pycfor this action are tracked in the repo and get dirtied by any local test run; I reverted that churn out of this commit. Worth removing and gitignoring separately.