fix: native review cleanup never matched — match managed reviews by marker, not author - #190
Merged
Merged
Conversation
…arker, not author cleanup_native_reviews resolved its identity via 'gh api user', but /user returns 403 for installation tokens (default GITHUB_TOKEN and GitHub App tokens) — and on HTTP errors gh prints the JSON error body to STDOUT, so CURRENT_ACTOR became a JSON error blob instead of triggering the github-actions[bot] fallback. The author filter then matched no review and cleanup silently did nothing on every production run. Even the fallback, had it fired, was wrong for app-token workflows (its-saffron[bot] etc.). Managed reviews are now matched by the marker their bodies start with — identity-free, safe because cleanup runs before the new review is posted, and robust across token-identity changes. Already-stubbed reviews are skipped unless they still carry a live verdict, and a failed list call now warns instead of masquerading as 'no previous reviews'. The existing functional test mocked /user failure as stderr+exit-1 (the divergence from real gh behavior that hid the bug); the mock now prints the error body to stdout like production, and a new behavioral suite exercises the dismiss/stub data path end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![its-saffron[bot]](https://avatars.githubusercontent.com/in/3901073?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: review@https://litellm.jory.dev/v1 (openai)
Recommendation
Approved. This PR addresses a critical bug where managed native reviews were not being cleaned up due to a flawed author-matching logic that failed when using installation tokens (which return a 403 on /user). The fix shifts to marker-based matching, which is more robust and identity-independent.
Change-by-change findings
scripts/publish_helpers.sh
- Fix: Marker-based matching: Replaced the
gh api userlookup with a direct list of reviews from the PR endpoint. This avoids the 403 error encountered by installation tokens. - Fix: Robust matching logic: The
jqfilter now matches reviews based on the presence of theCOMMENT_MARKERat the start of the body (or the legacy<!-- ai-pr-reviewerprefix), ensuring correct identification regardless of the bot's identity. - Improvement: Error handling: Added a warning and graceful exit if the review list cannot be fetched, preventing silent failures.
- Improvement: Idempotency: The logic now correctly handles reviews that have already been stubbed as
_Outdated: superseded...by checking if they still carry a live verdict (APPROVEDorCHANGES_REQUESTED). - Improvement: Cleanup logic: Added a check to ensure that if a review is already stubbed but still has a live verdict, it is re-processed to ensure consistency.
tests/test_cleanup_native_reviews_behavior.sh (New)
- New: Behavioral tests: Added a comprehensive test suite that mocks the
ghCLI to verify the cleanup logic. It specifically tests:- Identity independence (matching reviews from different bot identities).
- Correct handling of custom markers.
- Prevention of false positives from mid-body marker mentions.
- Correct handling of list failures and disabled flags.
tests/test_cleanup_previous_native_reviews.sh
- Refactor: Test coverage: Updated existing tests to reflect the new identity-independent matching logic and to verify that
/useris no longer called.
Standards Compliance
- Repository Conventions: The implementation follows the existing shell scripting patterns in the repository. The use of
localvariables andset -uo pipefailin tests is consistent with project standards. - Error Handling: The transition from silent failure to explicit
WARNlogging for API failures aligns with the repository's goal of observability.
Linked Issue Fit
- Fixes PR 190: The PR directly addresses the root cause described in the issue: the failure of
gh api userto return a login for installation tokens, which caused the cleanup script to fail to find any reviews to dismiss.
This was referenced Jun 10, 2026
joryirving
added a commit
that referenced
this pull request
Jun 12, 2026
#221) (#236) Every host-forge API call now routes through scripts/platform_api.sh (platform_* functions) or pr_reviewer/platform.py. The github backend reproduces the exact pre-seam gh invocations — the existing test suites, which stub gh via PATH, pass unmodified apart from two static source-grep assertions mechanically pointed at the seam file. - New `platform` input: auto (default, detects from GITHUB_SERVER_URL), github, forgejo. PLATFORM is wired into all six script-running steps. - Forgejo mode delegates implemented operations (core PR I/O from #222) to pr_reviewer/forgejo_backend.py and fails loudly on everything else (reviews/#224, CI status/#225, graphql, collaborator permission) instead of silently misbehaving; missing FORGEJO_API_URL is a hard error. - Linked-source enrichment calls are deliberately a separate class (github_enrich_api): they target github.com-hosted upstreams regardless of the host platform. #227 gates their degradation. - resolve_finding_threads.py builds gh argv via the Python seam; GraphQL thread management degrades to its existing best-effort no-op on forgejo. - The #190 stdout-error-body discipline is preserved: wrappers are thin execs, never capture-and-echo; call-site rc-check semantics unchanged. Tests: 32-case seam suite asserts every github-backend command line byte-for-byte plus resolution logic and loud forgejo failures; pytest suite covers the Python mirror. Full regression: all bash suites + 660 python tests pass. Refs #221 (Forgejo umbrella #229). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
Fixes the bug observed on misospace/dispatch#344: previous native reviews are never hidden/dismissed when a new review is posted.
Root cause
cleanup_native_reviewsresolved the bot identity withgh api user --jq .login. Two compounding failures:/userreturns 403 for installation tokens — both the defaultGITHUB_TOKENand GitHub App tokens (its-saffron), i.e. every deployment.gh apiprints the JSON error body to stdout (the--jqfilter is not applied), soCURRENT_ACTOR=$(gh api user --jq .login 2>/dev/null || echo "")captured{"message": "Resource not accessible by integration"...}— non-empty, so thegithub-actions[bot]fallback never fired, and the author filteruser.login == $actormatched nothing.Result: "No previous managed native reviews found" on every run since the feature shipped (verified in dispatch and pr-reviewer-action run logs). The fallback was doubly broken: even had it fired,
github-actions[bot]≠its-saffron[bot]for all app-token repos.Fix
Match managed reviews by the marker their bodies start with, with no author lookup at all. This is safe because cleanup runs before the new review is posted (it can never touch its own), and it keeps working across token-identity changes (default token → app token migrations). Also:
Tests
/userfailure as stderr+exit-1 — exactly the divergence from realghthat let this pass CI while dismissing nothing in production. The mock now prints the error body to stdout like production does.tests/test_cleanup_native_reviews_behavior.sh: 16 behavioral checks running the real function against a mockedgh(identity-independence, custom markers, mid-body false-positives, stub idempotency, list-failure, disabled flag).