fix(#7384): skip automatic review-handoff label dispatch - #7403
fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
Code-agent PR creation currently fires both pull_request_target.opened and labeled (ready-for-review) for the same head SHA. Those two events share a per-PR review concurrency group with cancel-in-progress, so one review cancels the other after setup has started. This commit implements the non-workflow half of the fix: - HarnessRouter skips ready-for-review when the event snapshot includes the fullsend-auto-review-handoff provenance label. Distinction is that label, not a bot username, [bot] suffix, or a prior review of the SHA. - Authorization is unchanged and independent of the skip. The GitHub [bot] collaborator-API exemption on opened|synchronize|ready_for_review is preserved in the maintainer patch. - Behavioral tests cover both creation/label orders, concurrent delivery, subsequent and fix-agent synchronize, and explicit same-revision /fs-review and labeled requests. - Maintainer patch for reusable-dispatch.yml and scaffold dispatch.yml is in docs/contributing/patches/ (coder app cannot push workflows). - Agents-repo post-code must apply provenance before ready-for-review; that change is not in this repository. Note: pre-commit could not fetch remote hook repos (git fetch HTTP 403). Local hooks plus shellcheck, gitleaks, lychee, gofmt, and go vet were run directly on the changed files. Related to #7384
|
🤖 Finished Review · ✅ Success · Started 1:58 PM UTC · Completed 2:23 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.00 |
Site previewPreview: https://5349171c-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Risk Assessment: moderate (2/5) DetailsRe-review of a bot-authored 'fix: address review feedback' commit that only adjusted existing content within the same 14 files (Tier1 2.630.5 + Tier2 2.430.3 + Tier3 2.0*0.2 ~= 2.44 -> 2); Tier 1 signals (file count, 2 protected .github/scripts paths, no dependency changes, bot author) are unchanged from the prior assessment, git history shows moderately churny, multi-author, fix-heavy files but no reverts or hack/workaround sentiment, and the linked high-priority issue remains well-scoped with no open discussion and an explicitly rollback-safe, currently-inert partial fix — none of which provides an articulable reason to move off the prior moderate score. Previous runRisk Assessment: moderate (2/5) DetailsWeighted composite (Tier1 2.630.5 + Tier2 2.430.3 + Tier3 2.0*0.2 ~= 2.44 -> 2) reflects a sizeable, bot-authored change touching two protected .github/scripts files and a moderately churny, multi-author area of the codebase, offset by zero security-sensitive paths, no dependency changes, no revert/hack history, a well-scoped high-priority issue with no open discussion, and an explicitly rollback-safe/inert partial fix that cannot affect production behavior until two companion changes land. |
ReviewFindingsHigh
Medium
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 2:25 PM UTC · Completed 2:41 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $3.02 |
Addresses the review-handoff dedup review findings: - Fix the labeled-arm logic error in review-handoff.sh: dispatch decisions now require the triggering label to be ready-for-review before evaluating the provenance skip, matching the maintainer patch's gating on TRIGGERING_LABEL; other labeled events now skip instead of dispatching. Updated all review-handoff-test.sh call sites for the new parameter and added a regression test for a non-ready-for-review labeled event. - Remove the test-local githubLikeReviewDecision helper and its tests from review_handoff_test.go: it duplicated (and shared the bug in) the shell decision table without exercising real routing, and the same scenarios are already covered by review-handoff-test.sh against the actual shell helper and the patched YAML. - Unexport AutoReviewHandoffLabel/IsAutomaticReviewHandoff to autoReviewHandoffLabel/isAutomaticReviewHandoff for consistency with sibling routing helpers in router.go; no external package references existed. - Rename the seven tests that exercise HarnessRouter.Route directly to the established TestHarnessRouter_ prefix convention. - Drop the shebang, include guard, and executable bit from review-handoff.sh (now 0644): it is sourced-only and never invoked directly, matching hack/gitlab-runner-vm/lib.sh's convention. - Consolidate review-handoff-test.sh's scratch temp directories under a single top-level TMPDIR_ROOT with an EXIT trap so a failed assertion can't leak temp dirs across the ~32 concurrent-count iterations or the patch-apply test. - Document two residual-risk edge cases in docs/contributing/review-handoff-dedup.md: GitLab leaving provenance in place until pre-review can shadow a deliberate re-labeling, and a lost/failed `opened` delivery combined with a leftover provenance label means no automatic review dispatches (new "Known limitations" section, referenced from the patch file header). Disagreements / could not fix: - [premature-closure-claim] (high): confirmed the PR body's "Closes #7384" contradicts its own "Remaining integration" section and the rollout doc's "not fixed in production" caveat. Attempted to replace it with "Refs #7384" via `gh pr edit --body-file`, but the sandbox network policy blocks PR-metadata GraphQL mutations for this agent (403 policy_denied on POST /graphql). A maintainer or the post-script needs to make this edit before merge. - [protected-path] (medium): this finding only mandates human approval for .github/scripts/ changes; it prescribes no content edit, so no code change was made for it. Verification: go test ./internal/dispatch/... -race and ./internal/scaffold/... pass; bash .github/scripts/review-handoff-test.sh passes; gofmt/go vet clean; go build ./... succeeds; scan-secrets clean. pre-commit could not fetch remote hook repos in this sandbox (HTTP 403); ran the applicable hooks directly instead (end-of-file-fixer, trailing-whitespace, merge-conflict/CRLF checks, lint-broken-symlinks, lint-docs-links, lychee) — all clean. shellcheck is not installed in this sandbox and pre-commit could not fetch it, so that hook did not run. Addresses #7403
🔧 Fix agent — iteration 1 (bot-triggered)Fixed 8 of 10 review findings on PR #7403: corrected the labeled-arm triggering-label logic error in review-handoff.sh (with updated tests and a new regression case), removed the redundant/misleading Go test helper duplicating that bug, renamed 7 tests to the TestHarnessRouter_ convention, unexported two dispatch-package symbols, converted review-handoff.sh to a proper sourced-only helper (0644, no shebang/guard), consolidated review-handoff-test.sh's scratch temp dirs under one EXIT-trapped TMPDIR_ROOT, and documented two residual-risk edge cases in the rollout doc (referenced from the patch file). Left the protected-path finding as a no-op (it only mandates human approval, prescribing no edit) and could not apply the PR-body 'Closes #7384' -> 'Refs #7384' fix because the sandbox blocks PR-metadata GraphQL mutations for this agent -- that one needs a maintainer or the post-script. Fixed (8):
Disagreed (2):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 2:43 PM UTC · Completed 3:00 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $4.76 |
|
🤖 Finished Fix · ✅ Success · Started 3:02 PM UTC · Completed 3:07 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $0.41 |
🔧 Fix agent — iteration 2 (bot-triggered)Both findings in the current review are outside this agent's ability to address with a code commit. The high-severity finding requires editing the PR body to remove the "Closes #7384" closing keyword, which needs a PR-metadata GraphQL mutation; that call was attempted and confirmed blocked by sandbox endpoint policy (403 policy_denied), so it must be done by a human maintainer or the post-script. The medium-severity protected-path finding is the mandatory merge-gate marker for .github/ changes and only requires human approval, not a content edit. No commit was made this iteration. Disagreed (2):
Tests: passed Decision points
Next steps:
|
1 participant
Summary
Stops duplicate automatic review of a code-agent PR's initial revision by treating
fullsend-auto-review-handoffas explicit provenance on the automaticready-for-reviewhandoff.ready-for-reviewremains a supported state/handoff label. This PR does not change.github/workflows/(coder app cannot push those files) and does not close the issue.Related Issue
#7384
Changes
Completed in this PR (locally verified):
internal/dispatchHarnessRouter skipsready-for-reviewwhen the event snapshot includesfullsend-auto-review-handoff. Distinction is that provenance label, not a bot username,[bot]suffix, or a previous review of the same SHA.[bot]collaborator-API exemption onopened|synchronize|ready_for_reviewis preserved.synchronize, and explicit same-revision/fs-reviewand labeled requests.reusable-dispatch.ymland scaffolddispatch.ymlatdocs/contributing/patches/7384-review-handoff-dedup.patch.docs/contributing/review-handoff-dedup.md.Remaining integration (not in this PR):
workflowspermission. Instructions are in the patch header and the contributing doc (git apply --check, thengit apply, then the listed tests/pre-commit).fullsend-ai/agents) post-code appliesfullsend-auto-review-handoffbeforeready-for-review, strips it immediately on GitHub, and leaves it for GitLab until pre-review. Do not removeready-for-reviewapplication.Until both land, production still double-dispatches. Independent deploy is safe: each half is a no-op without the other.
Testing
go test ./internal/dispatch/(including-race) andgo test ./internal/scaffold/bash .github/scripts/review-handoff-test.sh(includesgit apply --checkand apply-to-temp-copies)gofmt,go vet,shellcheck,gitleaks(changed files),lychee --offline, ADR lintsmake lint—pre-commitcould not fetch remote hook repos in this sandbox (HTTP 403); local equivalents were run insteadLive GitHub
opened+labeledone-second-apart delivery is not exercised here; it needs the maintainer patch plus the agents pin.Checklist
!for breaking changes)Closes #7384
Post-script verification
agent/7384-dedupe-review-handoff)fc364841eb01c3847e0368ca31dd5fe000faa19b..HEAD)