fix: defense-in-depth against duplicate code agent PRs (#1312, #1320, #1321) - #2373
Conversation
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
Site previewPreview: https://0738e782-site.fullsend-ai.workers.dev Commit: |
|
Hello @rh-hemartin . Would it be possible to make me a member of the org so I do not have to wait for |
|
🤖 Finished Review · ✅ Success · Started 7:27 AM UTC · Completed 7:39 AM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Info
Previous runReviewFindingsMedium
Low
Info
Previous run (2)ReviewFindingsMedium
Low
Info
Previous run (3)ReviewFindingsMedium
Low
Info
Previous run (4)ReviewFindingsMedium
Low
Info
|
|
🤖 Review · |
514a886 to
481de7c
Compare
|
🤖 Review · ❌ Terminated · Started 9:47 AM UTC · Ended 9:59 AM UTC |
|
Addressed 2 review comments with 2 additional PRs. |
|
🤖 Finished Review · ✅ Success · Started 9:47 AM UTC · Completed 9:59 AM UTC |
|
Added another commit to fix another review comment. |
|
🤖 Finished Review · ✅ Success · Started 10:29 AM UTC · Completed 10:42 AM UTC |
|
You need to signoff all the commits. |
pre-code.sh correctly detected existing PRs and posted a skip comment, but exited 0 without signaling the workflow to stop — so all downstream steps (GCP setup, bot identity, agent run) executed anyway, producing duplicate PRs. Write skip=true/false to GITHUB_OUTPUT on every exit path and gate all post-validation steps on steps.validate.outputs.skip != 'true'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>
The triage agent correctly identified existing PRs during its search but still emitted action "sufficient", applying ready-to-code and triggering duplicate code agent dispatches. Add a hard constraint in Step 2b: when an open PR already addresses the issue, use action "prerequisites" with the PR URL instead of "sufficient". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>
…ssue The dispatch router had no check for existing PRs that reference an issue without formal closing keywords. Add a pr-check step in both dispatch files (reusable-dispatch.yml and scaffold dispatch.yml) that searches for open PRs mentioning the issue number and skips code dispatch when any are found. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>
…tests The no-token exit path writes skip=false to GITHUB_OUTPUT but the existing test only asserted on stdout. Add a run_test_github_output variant to verify the output file. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>
Align with the existing convention used by role-check steps in the dispatch workflows, which output skipped=true. Rename skip→skipped in pre-code.sh, reusable-code.yml, reusable-dispatch.yml, scaffold dispatch.yml, and corresponding tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>
…r-check The dispatch pr-check step did not filter out fullsend-ai[bot] and fullsend-ai-coder[bot] PRs, which would block re-runs even when only a bot PR existed — making the /fs-code --force escape hatch unreachable. Add --jq filtering to match the logic in pre-code.sh. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>
2c94f53 to
cf544d0
Compare
|
Oh, sorry, missed it. All commits signed now. |
|
🤖 Finished Review · ✅ Success · Started 11:50 AM UTC · Completed 12:04 PM UTC |
| pull-requests: read | ||
| outputs: | ||
| stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }} | ||
| stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }} |
There was a problem hiding this comment.
[low] logic-error
When pr-check sets skipped=true, intermediate steps still run. Harmless read-only validations with minor inefficiency.
| pull-requests: read | ||
| outputs: | ||
| stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }} | ||
| stage: ${{ steps.role-check.outputs.skipped != 'true' && steps.pr-check.outputs.skipped != 'true' && steps.route.outputs.stage || '' }} |
There was a problem hiding this comment.
[low] fail-open
The pr-check guard fails open on API errors. Intentional fail-open for defense-in-depth.
|
🤖 Finished Retro · ✅ Success · Started 1:33 PM UTC · Completed 1:45 PM UTC |
Retro: PR #2373 — defense-in-depth against duplicate code agent PRsTimelineHuman-authored PR by Agent activity: 7 review dispatches → 4 successful, 2 cancelled (rapid pushes), 1 cancelled (superseded). Five distinct review iterations produced findings across the PR's evolution. What went well
Known inefficiencies (all tracked by existing issues)
ConclusionThe workflow delivered good value — the review agent found and the author fixed a real bug. The inefficiencies observed (repeated findings, duplicate runs, late-appearing findings) are well-known patterns with extensive existing issue coverage. No new proposals are warranted. |
The dispatch "Check for existing PRs" guard used --search "N in:title,body", which matches any PR that mentions the issue number anywhere in its body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL's Bot.login omits the REST "[bot]" suffix, so the literal string comparison against "fullsend-ai-coder[bot]" never matched. Replace both with a GraphQL query for closedByPullRequestsReferences, which only returns PRs that actually close the issue via Fixes/Closes/ Resolves keywords, and match the bot exclusion on __typename == "Bot" instead of a REST-formatted login string. The query also returns MERGED PRs regardless of includeClosedPrs, so filter on .state == "OPEN" explicitly — otherwise a long-merged closer would permanently block re-dispatch on a reopened issue. Add issues: read to both jobs' permissions, since the query now resolves through the Issue type rather than pull-requests-only fields. Separate stderr from the query's stdout so an incidental warning on an otherwise-successful call can't pollute the result and trigger a false-positive skip. internal/scaffold/fullsend-repo/scripts/pre-code.sh had the identical bug (same substring search, same broken bot-login comparison) and runs as a second gate in the same code-dispatch pipeline, so the original false-positive-skip failure mode could still recur through that path. Apply the same fix there and correct pre-code-test.sh's mock fixtures, which hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns. Verified against production data: #5569 (false positive: PR #5192 only mentions the issue, now correctly proceeds), #1320 (historical: closed by now-merged PR #2373, now correctly proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR #5578 with "Fixes #5575" in its body, correctly detected as blocking). Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
The dispatch "Check for existing PRs" guard used --search "N in:title,body", which matches any PR that mentions the issue number anywhere in its body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL's Bot.login omits the REST "[bot]" suffix, so the literal string comparison against "fullsend-ai-coder[bot]" never matched. Replace both with a GraphQL query for closedByPullRequestsReferences, which only returns PRs that actually close the issue via Fixes/Closes/ Resolves keywords, and match the bot exclusion on __typename == "Bot" instead of a REST-formatted login string. The query also returns MERGED PRs regardless of includeClosedPrs, so filter on .state == "OPEN" explicitly — otherwise a long-merged closer would permanently block re-dispatch on a reopened issue. Add issues: read to both jobs' permissions, since the query now resolves through the Issue type rather than pull-requests-only fields. Separate stderr from the query's stdout so an incidental warning on an otherwise-successful call can't pollute the result and trigger a false-positive skip. internal/scaffold/fullsend-repo/scripts/pre-code.sh had the identical bug (same substring search, same broken bot-login comparison) and runs as a second gate in the same code-dispatch pipeline, so the original false-positive-skip failure mode could still recur through that path. Apply the same fix there and correct pre-code-test.sh's mock fixtures, which hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns. Verified against production data: #5569 (false positive: PR #5192 only mentions the issue, now correctly proceeds), #1320 (historical: closed by now-merged PR #2373, correctly proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR #5578 with a "Fixes" keyword, correctly detected as blocking). Also bump closedByPullRequestsReferences's first from 20 to 100 (the connection's API max) at all three call sites, since a long-lived, repeatedly-reopened issue could otherwise silently truncate past the 20th closing-PR reference; add null-safety around the nodes array and author login so a missing field degrades gracefully instead of erroring or printing "null"; sanitize captured stderr before interpolating it into a ::warning:: workflow command; fix a pre-code-test.sh case that mocked a query failure while asserting the "no linked PRs" behavior; and document a third bot-login format returned by gh's own --json output (app/<slug> with a separate is_bot flag). Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
The dispatch "Check for existing PRs" guard used --search "N in:title,body", which matches any PR that mentions the issue number anywhere in its body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL's Bot.login omits the REST "[bot]" suffix, so the literal string comparison against "fullsend-ai-coder[bot]" never matched. Replace both with a GraphQL query for closedByPullRequestsReferences, which only returns PRs that actually close the issue via Fixes/Closes/ Resolves keywords, and match the bot exclusion on __typename == "Bot" instead of a REST-formatted login string. The query also returns MERGED PRs regardless of includeClosedPrs, so filter on .state == "OPEN" explicitly — otherwise a long-merged closer would permanently block re-dispatch on a reopened issue. Add issues: read to both jobs' permissions, since the query now resolves through the Issue type rather than pull-requests-only fields. Separate stderr from the query's stdout so an incidental warning on an otherwise-successful call can't pollute the result and trigger a false-positive skip. internal/scaffold/fullsend-repo/scripts/pre-code.sh had the identical bug (same substring search, same broken bot-login comparison) and runs as a second gate in the same code-dispatch pipeline, so the original false-positive-skip failure mode could still recur through that path. Apply the same fix there and correct pre-code-test.sh's mock fixtures, which hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns. Verified against production data: #5569 (false positive: PR #5192 only mentions the issue, now correctly proceeds), #1320 (historical: closed by now-merged PR #2373, correctly proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR #5578 with a "Fixes" keyword, correctly detected as blocking). Also bump closedByPullRequestsReferences's first from 20 to 100 (the connection's API max) at all three call sites, since a long-lived, repeatedly-reopened issue could otherwise silently truncate past the 20th closing-PR reference; add null-safety around the nodes array and author login so a missing field degrades gracefully instead of erroring or printing "null"; sanitize captured stderr before interpolating it into a ::warning:: workflow command; fix a pre-code-test.sh case that mocked a query failure while asserting the "no linked PRs" behavior; and document a third bot-login format returned by gh's own --json output (app/<slug> with a separate is_bot flag). Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
Summary
Prevents duplicate code agent PRs through three defense layers, all addressing the same 2026-05-21 incident where 5 duplicate PRs were created:
pre-code.shnow writesskip=trueto$GITHUB_OUTPUTwhen an existing PR is found, andreusable-code.ymlgates all post-validation steps onsteps.validate.outputs.skip != 'true'action: "prerequisites"instead ofaction: "sufficient", preventing theready-to-codelabel from being appliedpr-checkstep in both dispatch files (reusable-dispatch.ymland scaffolddispatch.yml) that searches for open PRs mentioning the issue number and skips code dispatch entirely when any are foundCloses #1312
Closes #1320
Closes #1321
Test plan
GITHUB_OUTPUTskip signal)shellcheckclean onpre-code.shactionlintclean onreusable-code.ymlandreusable-dispatch.ymlgo test ./internal/scaffold/)🤖 Generated with Claude Code