fix(review): dismiss stale request-changes when verdict softens - #2
Merged
Merged
Conversation
Minimizing old reviews via GraphQL minimizeComment hides them in the UI but does not remove the CHANGES_REQUESTED merge block under branch protection. Add DismissPullRequestReview to the forge client interface and call it from submitFormalReview() before minimizing, so the most recent CHANGES_REQUESTED review is dismissed when the new verdict is softer (COMMENT or APPROVE). Fixes: fullsend-ai#414 Assisted-by: Claude Code (Opus 4.6)
ben-alkov
force-pushed
the
worktree-fix+414-dismiss-stale-request-changes
branch
from
May 6, 2026 19:23
b9c0693 to
a81d443
Compare
Deduplicate GetAuthenticatedUser and ListPullRequestReviews calls by lifting them into submitFormalReview and passing user/reviews to the dismiss and minimize helpers. Remove the undocumented "event":"DISMISS" field from the GitHub dismiss-review payload — only "message" is part of the REST API. Strengthen the "multiple CR reviews" test case to use two CHANGES_REQUESTED reviews, properly validating that only the most recent is dismissed. Assisted-by: Claude Code (Opus 4.6)
The dismiss and minimize logic was unreachable for COMMENT events because submitFormalReview returned early at the COMMENT check before reaching the cleanup block. This left stale CHANGES_REQUESTED reviews undismissed for the primary scenario in fullsend-ai#414: verdict softening from request-changes to comment. Move the COMMENT early-return after the cleanup block so dismiss and minimize always run, and only CreatePullRequestReview is skipped for COMMENT events. Assisted-by: Claude Code (Opus 4.6)
ben-alkov
pushed a commit
that referenced
this pull request
May 15, 2026
Medium: - Updated PR description to reflect that pre-existing plan links were removed from README (no longer claims to link agent-execution-environment, vertex-inference-provisioning, and drift-scanner) Low #2: - Removed "Minimal wrapper pattern" section from plan document as it argues against itself - describes a pattern that requires capabilities that don't exist yet, then recommends removing it Low #3: - Updated IsURL function comment to note that url.Parse may not set u.User for all userinfo edge cases, advising implementers to consider additional validation if strict userinfo rejection is required Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ben-alkov
pushed a commit
that referenced
this pull request
May 18, 2026
When a human submits a PR and the review agent requests changes, the fix agent no longer auto-triggers. This is now the default behavior for human-authored PRs. Bot-authored PRs (from the code agent) continue to auto-trigger fixes as before. Humans can opt in to the review-fix loop by adding the `fullsend-fix` label to their PR. The existing `/fs-fix` command for manual invocation is unaffected — it intentionally bypasses this gate so authorized users can always trigger fix regardless of labels. Changes across dispatch paths (reusable dispatcher, per-repo scaffold): - Gate fix dispatch on PR author ending in [bot] OR presence of `fullsend-fix` label - Add defense-in-depth check in reusable-fix.yml that verifies PR author and label before running the fix agent - Add inline comment clarifying /fs-fix bypass (per review feedback) Note: fullsend.yaml shim no longer needs this gate — upstream refactored it to delegate all routing to the reusable dispatch workflow. E2E tested in ascerra-fullsend-lab org (human-pr-fix-test repo): 1. Human PR, no label (PR #1) — fix agent SKIPPED - Review: https://github.com/ascerra-fullsend-lab/.fullsend/actions/runs/25836235094 - Shim skipped dispatch-fix-bot: https://github.com/ascerra-fullsend-lab/human-pr-fix-test/actions/runs/25836370170 2. Human PR, fullsend-fix label (PR #2) — fix agent auto-triggered - Review: https://github.com/ascerra-fullsend-lab/.fullsend/actions/runs/25837485822 - Shim dispatched fix: https://github.com/ascerra-fullsend-lab/human-pr-fix-test/actions/runs/25837620440 - Fix agent succeeded: https://github.com/ascerra-fullsend-lab/.fullsend/actions/runs/25837623950 3. Bot PR, no label (PR fullsend-ai#4, author: ascerra-fullsend-lab-coder[bot]) — fix agent auto-triggered - Code agent created PR: https://github.com/ascerra-fullsend-lab/.fullsend/actions/runs/25838129538 - Review requested changes: https://github.com/ascerra-fullsend-lab/.fullsend/actions/runs/25838396492 - Shim dispatched fix (no label needed): https://github.com/ascerra-fullsend-lab/human-pr-fix-test/actions/runs/25838580565 - Fix agent succeeded: https://github.com/ascerra-fullsend-lab/.fullsend/actions/runs/25838584099 Co-authored-by: Cursor <cursoragent@cursor.com>
ben-alkov
pushed a commit
that referenced
this pull request
Aug 6, 2026
…riables Add HMAC-SHA256 signing of dispatch variables so API-triggered GitLab pipelines can verify the variables were set by the trusted poller, not forged by an attacker. This reduces the residual risk where CI_API_V4_URL override could bypass bot identity verification (mitigation #1). Go poller (dispatch.go): - computeDispatchHMAC() signs canonical key=value pairs using FULLSEND_DISPATCH_SECRET - signedDispatchKeys defines the canonical key list (sorted) - FULLSEND_POLL_JOB_URL included in signed keys - Once-per-run warning when FULLSEND_DISPATCH_SECRET is not set Shell verifier (fullsend-agent.yml): - HMAC verification block for API-triggered pipelines - Secret passed via env var (not command line) to avoid /proc exposure - Constant-time comparison via python3 hmac.compare_digest - Fail-closed: missing HMAC or mismatch aborts the job - Backward compatible: skipped when secret not configured Tests: - Cross-language test executing both Go and python3 HMAC against same test vector to verify identical output - Key list parity test between Go signedDispatchKeys and shell printf - HMAC determinism, tampering detection, missing keys - Integration test covering all signed keys for MR events Closes fullsend-ai#5572 (mitigation #2) Signed-off-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.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.
Summary
DismissPullRequestReviewto theforge.Clientinterface and implement it in the GitHub client usingPUT /repos/{owner}/{repo}/pulls/{number}/reviews/{id}/dismissalsdismissStaleRequestChanges()inpostreview.go— called fromsubmitFormalReview()beforeminimizeStaleReviews()to dismiss the most recentCHANGES_REQUESTEDreview when the new verdict is softer (COMMENT or APPROVE)minimizeStaleReviews()error-tolerance patternFixes: fullsend-ai#414
Test plan
dismissStaleRequestChanges()covering all edge cases: softening, same severity, no prior reviews, different user, multiple reviews, API errors, auth errorssubmitFormalReview()calls dismiss when aCHANGES_REQUESTEDreview exists and the new event is softergo vet ./...clean