fix(#5188): re-trigger review via label after fix-agent push#5551
fix(#5188): re-trigger review via label after fix-agent push#5551ggallen wants to merge 1 commit into
Conversation
PR Summary by QodoRe-trigger review by relabeling ready-for-review after fix-agent push
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
🤖 Finished Review · ✅ Success · Started 10:06 PM UTC · Completed 10:22 PM UTC |
Site previewPreview: https://33428969-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
ReviewFindingsLow
Previous runReviewFindingsHigh
Low
Labels: PR fixes dispatch re-triggering for review agent after fix-agent push; modifies post-fix script in scaffold |
![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.
fullsend-ai-review
Bot
added
component/dispatch
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad pass (Claude x2 + Grok + Gemini, 4 agents). fullsend-ai-review[bot] already correctly flagged a HIGH bug (label re-trigger calls run before GH_TOKEN is exported to PUSH_TOKEN, so they silently use the wrong ambient token and the whole fix no-ops) — not re-reported here.
Five new findings this round:
- HIGH: a second, even-quieter failure mode — if --remove-label fails for a transient reason (not just "label already absent"), the subsequent --add-label is idempotent and succeeds silently, so the warning never fires and there's zero log signal that re-dispatch didn't happen.
- HIGH: the new tests (perform_relabel_retrigger) reimplement the label-call logic in an isolated stub rather than exercising the real script, so they pass green today despite the real script having the already-confirmed GH_TOKEN bug — the test suite cannot catch this class of bug.
- MEDIUM (premature-decision): the core claim that GitHub doesn't refire the labeled webhook when re-adding an already-present label is asserted with no citation and no test; the cited precedents (post-code.sh, #2679) only ever add a label that was absent, so they don't actually validate this specific remove-then-add-when-present behavior.
- MEDIUM: the cancel-in-progress concurrency guard on the fix workflow can interrupt the remove/add sequence mid-flight, dropping the label with no automatic recovery until the next fix push.
- MEDIUM: the test stub doesn't validate argument construction, so an argument-order/quoting bug would pass silently.
Full detail inline. Given the compounding failure modes around the already-known GH_TOKEN bug, recommend addressing at least the two HIGH findings before merge.
pull_request_target.synchronize fires when the fix agent pushes, but its actor-identity authorization check is gated on PR_USER_LOGIN — the PR's original author, which for agent-authored PRs is the code agent's bot account regardless of who triggered this fix run. GitHub App bots have no collaborator role, so that check always fails closed and review is never re-dispatched after a fix-agent push (fullsend-ai#5188). post-fix.sh now removes then re-adds the ready-for-review label after a successful push, forcing a fresh labeled webhook event. That path has no actor-authorization gate at all — label application itself already requires write access, so it needs no separate identity check — mirroring post-code.sh's identical handling of the PR-open case. GitHub does not fire a new labeled event when a label already present is re-added, hence the remove-then-add sequence. This supersedes fullsend-ai#5415, which attempted the same fix via an actor-identity-recognition function (is_org_bot()) extended across several dispatch-authorization gates. That approach was closed after review: it reintroduced a design (recognizing bots by name for dispatch authorization) that issue fullsend-ai#2669 had already evaluated and rejected in favor of label-based gating — "actors can be spoofed and the approach is fragile across workflow changes" — a decision PR fullsend-ai#2679 already implemented and shipped for the retro-to-triage handoff (closing fullsend-ai#2636). The label-based fix here needs no changes to the CEL-based dispatch path (internal/harnessdispatch), which already trusts label-added events unconditionally, and needs no forge-specific bot-identity logic, since labels work identically across GitHub and GitLab. A separate, unrelated bug that fullsend-ai#5415 also touched — the fix agent's own review-body content-attribution lookups missing the shared fullsend-ai-review[bot] identity — is tracked independently as fullsend-ai#5550, since it is not a dispatch-authorization question and is unaffected by this change. Signed-off-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Greg Allen <gallen@redhat.com>
6644a49 to
de825b7
Compare
|
🤖 Finished Review · ✅ Success · Started 1:23 AM UTC · Completed 1:37 AM UTC |
Superseded by updated review
fullsend-ai-review
Bot
added
the
ready-for-merge
|
Which is the relation with #5536? |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review squad round 2 (Claude x2 + Grok + Gemini, 4 agents). All 6 round-1 findings verified genuinely fixed via direct re-inspection and mutation testing — GH_TOKEN export ordering, the sanitized ::notice:: on remove-label failure, the new structural ordering test, the honest unverified-assumption comment, the documented concurrency window, and the exact-match gh-invocation test assertions all hold up.
This round's mutation testing (deliberately mutating the fixed code to see if the new tests would catch it) surfaced:
- CRITICAL (test-only): the new run_gh_token_ordering_test is a static grep-based line-position check, not execution-based — commenting out the GH_TOKEN export, or wrapping it in a subshell where export doesn't propagate, both preserve the detected line ordering and pass the test while leaving the real authentication bug in place.
- MEDIUM: sanitization of the remove-label error message handles literal newlines and :: but misses percent-encoded %0A/%0D escapes, inconsistent with three sibling scripts in the same directory (post-retro.sh, install-precommit-tools.sh, extract-transcript-error.sh) that already strip these.
- Two LOW items: missing \r stripping in the same sanitization, and the new structural test's greps are unscoped to the relabel section specifically (works today only because the matched strings happen to be unique in the file).
Since the CRITICAL is a gap in test rigor rather than the shipped fix itself, and this PR already has two approvals, I don't think this needs to block merge — but the test gap is worth closing in a fast follow-up given it directly concerns the regression test for the bug this very PR fixed. Full detail inline.
|
|
||
| local remove_line token_export_line | ||
| remove_line="$(grep -n -- '--remove-label "ready-for-review"' "${post_fix_sh}" | head -1 | cut -d: -f1)" | ||
| token_export_line="$(grep -n 'export GH_TOKEN="\${PUSH_TOKEN}"' "${post_fix_sh}" | head -1 | cut -d: -f1)" |
There was a problem hiding this comment.
[critical] test-coverage
Confirmed via direct mutation: run_gh_token_ordering_test is a pure line-position grep check, not an execution-based test. It can be satisfied while GH_TOKEN never actually reaches the gh calls:
- Commenting out the export (
# export GH_TOKEN="${PUSH_TOKEN}") — grep doesn't care about#, sotoken_export_linestill resolves to the same line, ordering check passes, and the test reportsPASS: gh-token-orderingeven thoughGH_TOKENis never exported and everyghcall runs unauthenticated (the exact class of bug this test exists to catch). - Wrapping the export in a subshell (
( export GH_TOKEN="${PUSH_TOKEN}" )) — same result: line position unchanged, test passes, butexportinside( ... )never propagates to the parent shell, so downstreamghcalls are still unauthenticated.
Both mutations reproduce the real-world failure this test was added to prevent, and both get a clean pass.
Suggested fix: make this execution-based rather than static. E.g., source or bash -c the relevant script region with PUSH_TOKEN set to a sentinel and assert [ "$GH_TOKEN" = "$PUSH_TOKEN" ] is true in the same shell right before the relabel block would run. At minimum, tighten the grep to ^export GH_TOKEN= (rejecting comments) and reject matches inside parens/braces — though execution-based verification is the robust fix, since a static grep will always have some blind spot to control-flow that doesn't change line position.
| # Expected when the label wasn't present (e.g. a human-authored PR that | ||
| # never went through post-code.sh); still worth a breadcrumb since an | ||
| # unexpected API/permission failure looks identical from here. | ||
| SANITIZED_ERR="$(tr '\n' ' ' < "${REMOVE_LABEL_ERR}" | sed 's/::/: /g')" |
There was a problem hiding this comment.
[medium] sanitization-gap
SANITIZED_ERR="$(tr '\n' ' ' < "${REMOVE_LABEL_ERR}" | sed 's/::/: /g')" handles literal \n and literal ::, but doesn't strip percent-encoded %0A/%0D (or lowercase %0a/%0d) sequences. Confirmed: printf '%0A::error::fake injected error%0A' | tr '\n' ' ' | sed 's/::/: /g' → %0A: error: fake injected error%0A, i.e. the percent-encoded escapes pass through untouched.
This is inconsistent with the established pattern in this same directory — post-retro.sh, install-precommit-tools.sh, and extract-transcript-error.sh all already strip all four case variants of %0A/%0D as part of their GHA workflow-command sanitization, specifically because API error bodies can legitimately contain percent-encoded control characters that downstream consumers may reinterpret as real newlines/CRs — the same ::-prefix injection class this sanitization otherwise defends against. This PR's sanitization leaves that line incomplete relative to its own siblings.
Suggested fix: strip the same four variants used elsewhere, e.g. via SAFE:=${SAFE//%0A/} / ${SAFE//%0a/} / ${SAFE//%0D/} / ${SAFE//%0d/} before or alongside the existing tr/sed pipeline, and add a test case with a poison string like "HTTP 500%0A::error::spoofed%0D" to lock it in.
| # Expected when the label wasn't present (e.g. a human-authored PR that | ||
| # never went through post-code.sh); still worth a breadcrumb since an | ||
| # unexpected API/permission failure looks identical from here. | ||
| SANITIZED_ERR="$(tr '\n' ' ' < "${REMOVE_LABEL_ERR}" | sed 's/::/: /g')" |
There was a problem hiding this comment.
[low] sanitization-completeness
The pipeline strips \n but not \r. A crafted gh stderr containing a bare carriage return would pass through unmodified into the ::notice:: line. This doesn't create a new workflow-command injection (the runner's parser keys off \n as the line delimiter, and :: pairs are already neutralized by the adjacent fix), but some terminal/log renderers interpret bare \r as "return to column 0," which could let attacker-controlled stderr visually overwrite/obscure the ::notice:: prefix in a rendered log view. Minor readability/spoofing concern, not a security boundary.
Suggested fix: extend to tr -d '\r' | tr '\n' ' ' | sed 's/::/: /g' (or a broader control-character scrub) to make the sanitization conceptually complete, alongside the %0A/%0D fix above.
| post_fix_sh="${script_dir}/post-fix.sh" | ||
|
|
||
| local remove_line token_export_line | ||
| remove_line="$(grep -n -- '--remove-label "ready-for-review"' "${post_fix_sh}" | head -1 | cut -d: -f1)" |
There was a problem hiding this comment.
[low] test-robustness
Both greps match against the whole file, not scoped to the relabel section specifically — they work today only because each literal string (--remove-label "ready-for-review" and export GH_TOKEN="${PUSH_TOKEN}") happens to be unique in post-fix.sh right now (confirmed via grep -c, both count 1). If a future unrelated change adds a second occurrence of either string elsewhere in the file (e.g., an earlier idempotent cleanup step, or a narrower-scoped second GH_TOKEN export), head -1 would silently start comparing the wrong pair of lines and could pass even if the real relabel-step ordering regressed.
Suggested fix: scope the greps to the # 5. Re-trigger review section explicitly (e.g. extract the line range via awk '/^# 5\. Re-trigger review/,/^# 6\./' first, then grep within that slice) so the test stays correct if the file grows additional matches of either literal elsewhere.
3 participants
What this fixes
After the fix agent pushes a commit to a PR, the review agent is never re-dispatched (#5188).
pull_request_target.synchronizefires on that push, but the dispatch routing's actor-identity check is gated onPR_USER_LOGIN— the PR's original author, which for agent-authored PRs is always the code agent's bot account, regardless of who actually triggered this fix run. GitHub App bots have no collaborator role, so that check always fails closed.The fix
post-fix.shnow removes then re-adds theready-for-reviewlabel after a successful push, forcing a freshlabeledwebhook event. That path has no actor-authorization gate at all — applying a label already requires write access, so no separate identity check is needed — mirroringpost-code.sh's existing handling of the PR-open case. GitHub does not fire a newlabeledevent when a label already present is simply re-added, hence the remove-then-add sequence.Why this approach, not identity recognition
This supersedes #5415, which attempted the same fix by extending an actor-identity-recognition function (
is_org_bot()) across several dispatch-authorization gates. @ifireball closed that approach out during review: it reintroduced a design — recognizing bots by name for dispatch authorization — that issue #2669 had already evaluated and explicitly rejected in favor of label-based gating ("actors can be spoofed and the approach is fragile across workflow changes"). PR #2679 already implemented that decision and shipped it for the retro→triage handoff, closing #2636.The label-based fix here has some concrete advantages the identity-based approach didn't:
internal/harnessdispatch) — itsIsAuthorized()already trustslabel-addedevents unconditionally, so this fix is already consistent there with zero Go changes.[bot]-suffixed App-login convention has no GitLab equivalent at all, so an identity-matching approach would need an entirely separate mechanism per forge.#2636 needs no further work — it's already fixed by #2679.
What's not in this PR
A separate, unrelated bug that #5415 also touched — the fix agent's own review-body content-attribution lookups (in
reusable-fix.yml,reusable-dispatch.yml, andpre-fetch-prior-review.sh) missing the sharedfullsend-ai-review[bot]identity — is tracked independently as #5550 (found by @waynesun09 during #5415's review). It's a content-attribution question ("whose review text do I trust"), not dispatch authorization, so it's unaffected by the labels-vs-identity discussion here and doesn't belong in this PR.#5463and#5480(identity-hardening follow-ups filed during #5415's review) are no longer relevant to dispatch authorization now that this PR doesn't useis_org_bot()for that purpose; they may still be relevant once #5550 is addressed.Test plan
bash internal/scaffold/fullsend-repo/scripts/post-fix-test.sh— new test cases cover the remove/add label sequence tolerating either call failing without the script exiting nonzero.go build ./...andgo test ./internal/scaffold/...— no regressions.Fixes #5188. References #2636, #2669, #2679, #5463, #5480, #5550. Supersedes #5415.
cc @ifireball @waynesun09 — this replaces #5415 per the discussion there; see the closing comment on that PR for the full rationale.