fix(#5575): match linked-PR closing keywords instead of substring search - #5578
Conversation
PR Summary by QodoFix dispatch guard to detect issue-closing PRs via GraphQL (no substring matches)
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Site previewPreview: https://4573d87d-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1. Raised lint-workflow-size cap
|
|
🤖 Review · |
2f2eb78 to
59dfbce
Compare
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>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 Finished Review · ✅ Success · Started 4:34 PM UTC · Completed 4:50 PM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewReason: stale-head The review agent reviewed commit Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR modifies dispatch workflow and pre-code scripts — entirely within the dispatch subsystem Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Labels: PR modifies dispatch workflow and pre-code scripts — entirely within the dispatch subsystem |
59dfbce to
b3e57dd
Compare
|
🤖 Finished Review · ✅ Success · Started 8:06 PM UTC · Completed 8:20 PM UTC |
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>
b3e57dd to
b9aa139
Compare
b9aa139 to
f6fea94
Compare
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>
|
🤖 Finished Review · ✅ Success · Started 9:20 PM UTC · Completed 9:37 PM UTC |
|
🤖 Review · Commit: |
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), proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
In org/workflow_call mode neither the scaffold dispatch job nor the shim-workflow-call.yaml caller grants issues: read, so the new closedByPullRequestsReferences query is denied and the guard fails open — dedup still holds via the pre-code.sh gate on a minted app token. Record why the scope must not be added to the called job alone: a called workflow cannot exceed its caller's grant, so repos with un-reconciled shims would hard-fail at job start. Safe rollout order, if ever wanted, is shim template first, then this job. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
4fecd78 to
133ae91
Compare
|
🤖 Finished Review · ✅ Success · Started 2:05 PM UTC · Completed 2:22 PM UTC Commit: |
Superseded by updated review
Commit 4df01b1 removed issues: read from the scaffold dispatch job (bundled in a CI-retrigger commit): the shim-workflow-call.yaml caller does not grant the scope, and a called workflow requesting more than its caller grants fails at job start — breaking every org-mode dispatch until shims are re-reconciled. The rebase that produced 9f9a9ca dropped that commit as presumed-empty, silently restoring the permission and contradicting the fail-open NOTE added in 133ae91. Re-remove it; org mode intentionally fails open here and pre-code.sh enforces dedup on a minted app token. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 2:30 PM UTC · Completed 2:50 PM UTC Commit: |
|
Review skipped — this PR is already merged. The Posted by fullsend post-review check |
|
🤖 Finished Retro · ✅ Success · Started 2:53 PM UTC · Completed 3:11 PM UTC Commit: |
Retro: PR #5578 — fix dispatch existing-PR guardVerdict: workflow went well. No novel proposals needed — all improvement areas are already covered by existing open issues. What happenedPR #5578 was a human-authored fix for #5575, a critical bug in the dispatch workflow's "Check for existing PRs" guard. The guard used a substring search ( The PR was open for 14 days (Jul 24 – Aug 7), received 11 review.yml dispatches (8 completed, 2 cancelled, 1 post-merge skip), human approval from rh-hemartin on Aug 4, and was merged after 7 force-pushes (mostly rebases). Review agent qualityThe fullsend review agent performed excellently on this PR:
Iteration cost11 review workflow runs is high for a 4-file, +77/−20 change. The iteration count was driven by:
Evidence for existing open issues
Autonomy readinessThe review agent's zero-false-positive record on this PR demonstrates high precision for workflow file changes. However, the PR correctly carried the |
Summary
Fixes #5575. The dispatch "Check for existing PRs" guard used
--search "N in:title,body", which matches any open PR that mentions the issue number anywhere in its title/body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL'sBot.loginomits the REST[bot]suffix, so the literal string comparison against"fullsend-ai-coder[bot]"never matched anything.Concretely, on #5569, two
/fs-codecomments both routed toSTAGE="code"but were silently skipped because PR #5192 had a single line referencing #5569 as an unrelated, out-of-scope finding — not an actual fix.Fix
Replace the substring search with a GraphQL query for
closedByPullRequestsReferences, which only returns PRs that actually close the issue viaFixes/Closes/Resolveskeywords — GitHub's own linking mechanism, not a text match. The bot exclusion now matches onauthor.__typename == "Bot"combined with the GraphQL-format login (fullsend-ai-coder, no[bot]suffix).Applied to:
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml(org-mode scaffold).github/workflows/reusable-dispatch.yml(per-repo reusable workflow)internal/scaffold/fullsend-repo/scripts/pre-code.sh— a second, independent existing-PR gate that runs later in the samecodejob with the identical bug (same substring search, same broken bot-login comparison). Left unfixed, the original false-positive-skip failure mode could still recur through this path even after the dispatch-level guard is fixed.pre-code-test.sh's mock fixtures hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns — corrected those too.Additional correctness fixes found during review:
closedByPullRequestsReferencesreturns MERGED PRs regardless ofincludeClosedPrs, so the jq filter now explicitly requires.state == "OPEN"— otherwise a long-since-merged closer would permanently block re-dispatch on a reopened issue.issues: readto thedispatch/routejobs' permissions, since the query now resolves through theIssuetype rather than pull-requests-only fields.docs/contributing/bot-identities.mdabout the REST vs. GraphQL login format discrepancy, since it's now cited from three call sites.dispatch.yml'slint-workflow-sizecap bumped from 560 to 590 to accommodate the larger, more explicit query and permission grant (no size cap applies toreusable-dispatch.yml).Test plan
make lintpasses (yaml checks, actionlint, shellcheck, workflow-size lint all green).pre-code-test.sh— all 23 cases pass, including two new regression tests: a MERGED-PR-doesn't-block case and an other-bot (non-coder) PR correctly still blocks.go test ./internal/scaffold/...— all workflow scaffold/alignment tests pass.