fix(#5536): use is_bot for bot-author detection in fix eligibility - #6250
fix(#5536): use is_bot for bot-author detection in fix eligibility#6250ggallen wants to merge 1 commit into
Conversation
|
🤖 Finished Review · ✅ Success · Started 1:48 PM UTC · Completed 2:00 PM UTC Commit: |
PR Summary by QodoUse GraphQL __typename to detect bot-authored PRs in fix eligibility
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
Previous run (5)ReviewFindingsMedium
Previous run (6)ReviewFindingsMedium
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Previous run (9)ReviewFindingsMedium
Low
Previous run (10)ReviewFindingsMedium
Labels: PR modifies fix-agent eligibility logic in dispatch workflows |
waynesun09
left a comment
There was a problem hiding this comment.
HIGH: Third location named in issue #5536 (org-level dispatch.yml) is left unfixed despite "Closes #5536"
Issue #5536's "Affected locations" list names three files with the same bug: reusable-fix.yml and reusable-dispatch.yml (both addressed by this PR) plus .fullsend/.github/workflows/dispatch.yml at the org level (in the fullsend-ai/.fullsend repo, out of tree for this PR — no inline location available here). That file still hardcodes BOT_LOGIN="fullsend-ai[bot]" / CODER_BOT_LOGIN="fullsend-ai-coder[bot]" (around lines 271-272) and compares them against .author.login from gh pr list --json number,author (around lines 275-276). gh pr list --json author also returns app/fullsend-ai-coder, not fullsend-ai-coder[bot] — the identical root-cause mismatch — so this comparison also never matches a real bot PR.
This PR's body says "Closes #5536", which will auto-close the issue on merge even though one of its three named locations remains broken (and, per the inline CRITICAL finding, the other two aren't actually fixed either).
Failure scenario: Merging this PR auto-closes #5536. The org-level dispatch routing in .fullsend/.github/workflows/dispatch.yml continues to misidentify bot-authored PRs as human because BOT_LOGIN/CODER_BOT_LOGIN are compared against a login format ([bot] suffix) that gh pr list --json author never returns, so the same class of bug silently persists there with no tracking issue left open.
Suggestion: Either fix .fullsend/.github/workflows/dispatch.yml in this PR (or a linked companion PR) before closing #5536, or scope this PR's title/body to only the two files it actually touches and open a follow-up issue for the org-level file so it isn't lost when #5536 auto-closes.
Two additional findings were left as inline comments on .github/workflows/reusable-dispatch.yml (a CRITICAL issue with the __typename field not being populated by gh pr view, and a MEDIUM precision gap relative to the existing-PR guard it's modeled on).
|
Re: HIGH (org-level |
83a98f2 to
a37adcc
Compare
|
🤖 Finished Review · ✅ Success · Started 3:21 PM UTC · Completed 3:36 PM UTC Commit: |
a37adcc to
e3553d3
Compare
|
🤖 Finished Review · ✅ Success · Started 3:42 PM UTC · Completed 3:55 PM UTC Commit: |
e3553d3 to
5e3e64f
Compare
Site previewPreview: https://1e23914e-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 4:00 PM UTC · Completed 4:13 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
One additional finding that has no inline location in this PR's diff (the file isn't touched here):
MEDIUM: docs/agents/fix.md still describes auto-fix as applying to any bot-authored PR, not just the coder app (docs/agents/fix.md, line 122)
The new gate in both reusable-dispatch.yml:1097 and reusable-fix.yml:309 requires PR_IS_BOT == "true" AND PR_LOGIN == "app/fullsend-ai-coder" — only the coder app qualifies for label-free auto-fix. But docs/agents/fix.md (unchanged by this PR) still reads (lines 121-124): "For bot-authored PRs (e.g., PRs opened by the code agent), automatic fixing happens with no extra setup" — phrased as any bot-authored PR being eligible, not scoped to the coder app specifically. Since dispatch routing (unchanged by this PR) still admits any [bot]$-suffixed webhook login into the fix stage, a PR from a different bot (e.g. renovate-fullsend[bot], listed in docs/contributing/bot-identities.md) reaches this gate and is silently skipped with the misleading "Human-authored PR #N ... skipping bot-triggered fix" warning (reusable-fix.yml:312) — an operator reading fix.md would not expect that skip.
Suggestion: Update docs/agents/fix.md to scope the wording to the coder app specifically (e.g. "For PRs authored by the fullsend code agent") now that this PR makes the eligibility check precise, since this is the PR that changes/clarifies the actual gating behavior.
|
Re: MEDIUM (fix.md wording) — updated |
5e3e64f to
4ec2d3a
Compare
|
🤖 Finished Review · ✅ Success · Started 4:22 PM UTC · Completed 4:37 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Two additional findings that don't have a commentable inline diff line in this PR (the surrounding lines aren't part of the diff hunk / no file changed):
MEDIUM: Control-labels table still says "Bot-authored PRs do not need this label," contradicting the updated prose above it (docs/agents/fix.md, line 141 — outside this PR's diff hunk for that file)
The prose earlier in the same file (~lines 119-124, updated by this PR) now says only PRs from fullsend-ai-coder[bot] auto-fix without the label, and "PRs from other bots (e.g., Renovate) are treated like human-authored PRs and require the fullsend-fix label." But the separate "Control labels" table (## Control labels, line 137) still has the row: fullsend-fix | ... Bot-authored PRs do not need this label. — a blanket statement no longer true after this change. This table wasn't touched by this PR's diff (unlike the prose paragraph), so the two sections of the same doc now directly contradict each other.
Suggestion: Update the table row to match the prose, e.g. "Enables automatic bot-triggered fix runs on human-authored PRs, and on bot-authored PRs other than the fullsend coder bot, which is always eligible without this label."
MEDIUM: Test-plan checkboxes for this CI-gating change remain unchecked (PR description, "Test plan" section)
Confirmed in the current PR body: all four test-plan checkboxes are still unchecked (verifying gh pr view --json author output for a bot PR, for a human PR, confirming auto-fix fires on a bot PR after CHANGES_REQUESTED, and confirming the label gate still applies to human PRs). This changes production CI eligibility gating for an automated write path (the fix agent auto-committing to PRs); it's already been live-verified via review comments that the is_bot/app/-prefix schema assumption holds (PR #6252 -> is_bot:true, login:app/fullsend-ai-coder), but the PR's own listed end-to-end verification steps were never marked done.
Suggestion: Check off the test-plan boxes after actually running the listed gh pr view commands against a live bot PR and a live human PR, and confirm the auto-fix trigger fires end-to-end before merging.
|
Responding to waynesun09's review on commit 4ec2d3a: Control-labels table — Already fixed in a prior commit. The table row now says: "Enables automatic bot-triggered fix runs on human-authored PRs and PRs from bots other than the fullsend code agent. Without this label, the fix agent only runs when explicitly invoked via Test-plan checkboxes — Acknowledged. Will check off items 1-2 (the Skip-log message — Fixed in shared script. Log now includes No test / duplication — Fixed by extracting into Hardcoded literal — Acknowledged, intentionally scoped to hosted app-set. Not a regression (old regex never matched any bot). Added scope note to PR description. |
4ec2d3a to
6972772
Compare
|
🤖 Finished Review · ✅ Success · Started 7:48 PM UTC · Completed 8:27 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
One additional finding that has no inline location in this PR's diff (Makefile is not touched by this PR):
HIGH: New check-fix-eligibility-test.sh fixture suite is not wired into the Makefile's script-test target, so CI never runs it (Makefile, script-test target, ~line 180)
Verified on HEAD 6972772: Makefile's script-test target explicitly lists each shell-script test file (scripts/check-e2e-authorization-test.sh, .github/scripts/redact-behaviour-artifacts-test.sh, internal/scaffold/fullsend-repo/scripts/reconcile-repos-test.sh, internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review-test.sh, plus two python test files) and was not updated to add .github/scripts/check-fix-eligibility-test.sh. .github/workflows/lint.yml runs make script-test in CI. The new 8-fixture test suite added by this PR therefore never executes in CI — it is dead code from CI's perspective, so a future regression in the eligibility predicate (including the deployment gap raised inline on reusable-fix.yml) would not be caught automatically.
Suggestion: Add bash .github/scripts/check-fix-eligibility-test.sh to the script-test target in the Makefile, and confirm a CI run actually exercises it before merge.
6972772 to
046675b
Compare
|
🤖 Review · Commit: |
046675b to
02ca85c
Compare
|
🤖 Finished Review · ✅ Success · Started 2:18 AM UTC · Completed 2:36 AM UTC Commit: |
02ca85c to
dcb3ce2
Compare
|
🤖 Finished Review · ✅ Success · Started 2:40 AM UTC · Completed 2:57 AM UTC Commit: |
dcb3ce2 to
f82a8b6
Compare
|
🤖 Finished Review · ✅ Success · Started 3:00 AM UTC · Completed 3:17 AM UTC Commit: |
f82a8b6 to
a588425
Compare
|
🤖 Finished Review · ✅ Success · Started 3:21 AM UTC · Completed 3:38 AM UTC Commit: |
…gibility The fix agent's eligibility check used gh pr view --json author which returns app/fullsend-ai-coder format, but the code tested against a [bot]$ regex that never matched. Switch to .author.is_bot (bool) plus .author.login to precisely identify coder-bot PRs, following the same identity-check pattern as fullsend-ai#5974 adapted for the gh pr view API surface. Extract the eligibility logic into a shared script (.github/scripts/check-fix-eligibility.sh) called by both reusable-fix.yml and reusable-dispatch.yml, with 12 fixture tests. Source the script from the trusted .defaults/ tree (upstream scaffold checkout) to ensure it exists for consumer repos and runs with the correct trust boundary. Signed-off-by: Greg Allen <gallen@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
a588425 to
066c6b1
Compare
|
🤖 Finished Review · ✅ Success · Started 3:43 AM UTC · Completed 3:58 AM UTC Commit: |
Summary
gh pr view --json author(GraphQL), which returnsapp/fullsend-ai-coder— not the RESTfullsend-ai-coder[bot]format. The[bot]$regex never matched, so bot-authored PRs were misidentified as human-authored and skipped..author.is_bot(bool) and.author.loginto precisely identify coder-bot PRs. This uses the fields actually exposed bygh pr view --json, following the same identity-check logic as the existing-PR guard (see Code dispatch existing-PR guard uses bare substring search, causing false-positive blocks #5974 anddocs/contributing/bot-identities.md) adapted for thegh pr viewAPI surface..github/scripts/check-fix-eligibility.sh) with 8 fixture tests, called by bothreusable-fix.ymlandreusable-dispatch.yml.dispatch.yml) does not have a fix eligibility check — it only has the existing-PR guard which already uses__typenamecorrectly via raw GraphQL. No change needed there.fullsend-ai-coder). Self-hosted--app-setdeployments with custom app names remain in the same state as before (requiring thefullsend-fixlabel) — not a regression since the old[bot]$regex never matched any bot.Closes #5536
Test plan
gh pr view <bot-pr> --json author --jq '{is_bot: .author.is_bot, login: .author.login}'returns{"is_bot":true,"login":"app/fullsend-ai-coder"}for a coder-bot-authored PR (verified via PR fix(#6246)!: preserve SHA pinning only for already-pinned repos during upgrade #6252){"is_bot":false,...}for a human-authored PR/fs-fixneeded)fullsend-fixlabel for human-authored PRs when bot-triggered🤖 Generated with Claude Code