fix(dispatch): log why comment-triggered dispatches are skipped - #5214
Closed
Roming22 wants to merge 1 commit into
Closed
fix(dispatch): log why comment-triggered dispatches are skipped#5214Roming22 wants to merge 1 commit into
Roming22 wants to merge 1 commit into
Conversation
Emit notice annotations when bot or unauthorized comments are ignored, and centralize those checks in shared helpers. Signed-off-by: Romain Arnaud <rarnaud@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
Thank you for your interest in contributing to fullsend, @Roming22. This project uses a vouch system for first-time contributors. Before submitting a pull request, you need to be vouched by a maintainer. To get vouched:
See CONTRIBUTING.md for details. |
PR Summary by QodoDispatch: log reasons for skipping comment-triggered stage dispatches
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Bad bash return value
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+110
to
+123
| comment_from_user() { | ||
| if [[ "${COMMENT_USER_TYPE}" == "Bot" ]]; then | ||
| echo "::notice::Skipping dispatch for bot comment" | ||
| return 1 | ||
| fi | ||
| return 0 | ||
| } | ||
| comment_from_authorized_user() { | ||
| if ! is_authorized; then | ||
| echo "::notice::Skipping dispatch for unauthorized comment from ${COMMENT_USER_LOGIN}" | ||
| return 1 | ||
| fi | ||
| return comment_from_user | ||
| } |
There was a problem hiding this comment.
1. Dispatch routing checks out-of-sync 📘 Rule violation ⚙ Maintainability
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml centralizes bot/authorization checks into comment_from_user/comment_from_authorized_user with notice annotations, but .github/workflows/reusable-dispatch.yml still uses the older inline COMMENT_USER_TYPE != "Bot" + is_authorized checks. This violates the requirement to keep dispatch routing logic in sync across the two workflows and can cause different behavior/diagnostics between scaffolded repos and the reusable workflow.
Agent Prompt
## Issue description
The bot/authorization gating and skip-notice behavior was updated in `internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml` (via `comment_from_user` and `comment_from_authorized_user` helpers), but the reusable workflow `.github/workflows/reusable-dispatch.yml` still uses the previous inline checks. This breaks the compliance requirement that routing/dispatch logic stays aligned across both workflows.
## Issue Context
PR intent is to "Emit notice annotations when bot or unauthorized comments are ignored, and centralize those checks in shared helpers." Only the scaffold dispatch workflow was updated.
## Fix Focus Areas
- .github/workflows/reusable-dispatch.yml[180-243]
- internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml[110-190]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+117
to
+123
| comment_from_authorized_user() { | ||
| if ! is_authorized; then | ||
| echo "::notice::Skipping dispatch for unauthorized comment from ${COMMENT_USER_LOGIN}" | ||
| return 1 | ||
| fi | ||
| return comment_from_user | ||
| } |
There was a problem hiding this comment.
2. Bad bash return value 🐞 Bug ≡ Correctness
comment_from_authorized_user() uses return comment_from_user, but return requires a numeric status, so the function errors and returns non-zero even for authorized human comments. As a result, every if comment_from_authorized_user; then ... branch is skipped and comment-triggered stages won’t dispatch.
Agent Prompt
## Issue description
`comment_from_authorized_user()` attempts to delegate to `comment_from_user()` via `return comment_from_user`, which is invalid in bash because `return` accepts only an integer exit code. This causes authorized human comments to fail the `if comment_from_authorized_user; then` checks and prevents `STAGE` from being set.
## Issue Context
This is inside the `Determine stage` script (`set -euo pipefail`). The helper is used as the gate for multiple slash commands.
## Fix Focus Areas
- internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml[110-123]
## Suggested change
Replace the invalid return with actually calling the function and returning its exit status, e.g.:
```bash
comment_from_authorized_user() {
if ! is_authorized; then
echo "::notice::Skipping dispatch for unauthorized comment from ${COMMENT_USER_LOGIN}"
return 1
fi
comment_from_user
return $?
}
```
(or make `comment_from_user` the last command without an explicit `return` argument).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+110
to
+123
| comment_from_user() { | ||
| if [[ "${COMMENT_USER_TYPE}" == "Bot" ]]; then | ||
| echo "::notice::Skipping dispatch for bot comment" | ||
| return 1 | ||
| fi | ||
| return 0 | ||
| } | ||
| comment_from_authorized_user() { | ||
| if ! is_authorized; then | ||
| echo "::notice::Skipping dispatch for unauthorized comment from ${COMMENT_USER_LOGIN}" | ||
| return 1 | ||
| fi | ||
| return comment_from_user | ||
| } |
There was a problem hiding this comment.
3. Bot notice precedence wrong 🐞 Bug ◔ Observability
comment_from_authorized_user() checks is_authorized before the bot filter, so an unauthorized bot comment emits the “unauthorized comment” notice and never emits the intended bot-skip notice. This also performs an unnecessary permission check (gh api) for bot comments that could be rejected immediately.
Agent Prompt
## Issue description
For comment-triggered dispatches, the new helper logs skip reasons. However, `comment_from_authorized_user()` checks authorization first and only then calls the bot filter, which means bot comments that aren’t authorized get logged as “unauthorized” instead of “bot comment”, and still incur a permission API call.
## Issue Context
`is_authorized()` calls `has_write_permission()`, which uses `gh api` to query collaborator permissions.
## Fix Focus Areas
- internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml[60-86]
- internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml[110-123]
## Suggested change
Reorder the checks so bot filtering happens first:
```bash
comment_from_authorized_user() {
if ! comment_from_user; then
return 1
fi
if ! is_authorized; then
echo "::notice::Skipping dispatch for unauthorized comment from ${COMMENT_USER_LOGIN}"
return 1
fi
return 0
}
```
This ensures bot comments always produce the bot notice and avoids calling the permission API for bots.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Emit notice annotations when bot or unauthorized comments are ignored,
and centralize those checks in shared helpers.
Signed-off-by: Romain Arnaud rarnaud@redhat.com
Co-authored-by: Cursor cursoragent@cursor.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Summary
When a comment does not trigger a stage, the reason is logged to the output.
Related Issue
N/A
Changes
Determine stageinfullsend-ai_fullsend/internal/scaffold/fullsend-repo/.github/workflowshas been updated to output why a comment may not trigger a given stage.Testing
make lintpasses (stage changes first, then run): not working in my envChecklist
!for breaking changes)