Handle human @mentions in review feedback workflow

[james-in-a-box]

Handle human @mentions in on-review-feedback workflow

PR #658 removed on-mention.yml as part of the SDLC cleanup, which also
removed the ability for humans to @mention the bot in PR comments. The
on-review-feedback.yml workflow already triggers on issue_comment events
but its check-trigger job only matched bot/reviewer comments with the
egg-automated-review marker — human @mentions were silently filtered out.

This adds a fourth trigger case: when an authorized user comments on a PR and
@mentions the configured bot username (EGG_BOT_USERNAME repo variable),
the workflow now triggers. Only users in the authorized_users list (workflow
input) or EGG_AUTHORIZED_USERS repo variable (defaulting to jwbron) can
trigger the bot via @mention or review — unauthorized users are filtered out
in check-trigger.

A trigger_type output (review vs mention) distinguishes the two paths
so that human mentions skip the reviewer-check wait (no concurrent reviewer
checks to batch with).

Fixes the issue reported in #678 (comment).

Test plan:

• Verify automated review triggers still work (bot comment with egg-automated-review marker)
• Verify human pull_request_review triggers still work for authorized users only
• Verify human @mention in a PR comment on a bot-authored PR triggers for authorized users
• Verify human comments/reviews from unauthorized users are filtered out
• Verify human comments without @mention are still filtered out
• Verify @mention on non-bot-authored PRs is filtered out by the should-run step
• Verify EGG_AUTHORIZED_USERS repo variable is used for event-triggered runs

Authored-by: egg

[egg-reviewer]

No agent-mode design concerns.

This PR is purely a workflow-triggering change — it adds a new trigger path (human @mentions) without altering how the agent operates once invoked. The trigger_type output cleanly separates mention-triggered runs from review-triggered runs so that mentions skip the reviewer-wait step (which only applies when batching concurrent review triggers). The agent still receives its prompt through the existing build-feedback-prompt.sh flow and runs with full tool access.

— Authored by egg

[egg-reviewer]

Thorough review of all changes. The logic is sound and the implementation is consistent with existing patterns. A few issues to address:

Issue 1: @mention check can be triggered by substrings (Low severity)

File: .github/workflows/on-review-feedback.yml, Case 4 mention detection

[[ "$COMMENT_BODY" == *"@${BOT_USERNAME}"* ]]

This is a substring match, which means if BOT_USERNAME is egg-bot, then a comment containing @egg-bot-staging or foo@egg-bot would also match. This is an edge case, but since bot usernames are typically unique-ish strings and the should-run step provides the authoritative gate (bot-authored PR check, iteration limit), the impact is limited to a wasted runner in degenerate cases.

Not blocking, but worth a note: a stricter match like checking for a word boundary after the username (e.g., @${BOT_USERNAME} followed by whitespace, punctuation, or end-of-string) would eliminate false positives. The existing pattern is consistent with how the removed on-mention.yml likely worked, so this is acceptable for now.

Issue 2: Missing trigger_type output for should_run=false branches — confirmed not a problem

The else branches that set should_run=false don't set trigger_type. I traced through all downstream conditionals and confirmed this is safe: when should_run=false, the address-feedback job doesn't run, so trigger_type is never read. No action needed.

Issue 3: Condition duplication

Seven steps now have the condition:

steps.should-run.outputs.run == 'true' && (steps.wait-for-reviewers.outputs.proceed == 'true' || needs.check-trigger.outputs.trigger_type == 'mention')

This is a long, repeated expression that's easy to get wrong during future edits. Consider extracting this into a step output (e.g., a should-proceed output computed once after wait-for-reviewers), or adding a brief inline comment explaining why the || mention disjunction exists. Not blocking, but it would improve maintainability.

Verification of correctness

I traced through all four code paths:

1. workflow_call/workflow_dispatch: check-trigger is skipped → trigger_type is empty → != 'mention' is true → wait-for-reviewers runs → downstream conditions fall through to proceed == 'true'. Correct; matches existing behavior.

2. pull_request_review (Case 1 & 2): trigger_type=review → != 'mention' is true → wait-for-reviewers runs → downstream depends on proceed. Correct.

3. issue_comment bot self-review (Case 3): trigger_type=review → same as above. Correct.

4. issue_comment human @mention (Case 4): trigger_type=mention → wait-for-reviewers is skipped → downstream conditions use || trigger_type == 'mention' to proceed. Correct; mentions skip the reviewer-check wait as intended.

The should-run step in address-feedback fetches PR_AUTHOR from the API (not the event payload), so the bot-authored-PR check works correctly for issue_comment events where github.event.pull_request is null. This is the same defense-in-depth pattern used by Case 3.

The cancel-in-progress: true concurrency group on egg-feedback-$PR_NUMBER means rapid @mentions on the same PR will cancel earlier runs. This is acceptable behavior — only the latest mention matters.

Summary

The changes are correct and well-structured. The new trigger case follows existing patterns, the reviewer-wait bypass for mentions is logically sound, and the downstream condition changes are consistent. The PR description accurately describes the behavior and the layered validation approach.

Approving. The substring match (Issue 1) and condition duplication (Issue 3) are non-blocking suggestions for future improvement.

— Authored by egg

[james-in-a-box]

Addressed both non-blocking suggestions:

Issue 1 (substring match): Changed the @mention check from a glob pattern (*"@${BOT_USERNAME}"*) to a regex with word boundary: [[ "$COMMENT_BODY" =~ @${BOT_USERNAME}($|[^a-zA-Z0-9_-]) ]]. This ensures @egg-bot won't match @egg-bot-staging or similar substrings.

Issue 3 (condition duplication): Extracted the repeated condition into a new Compute should-proceed step that runs once after wait-for-reviewers. All 7 downstream steps now reference steps.should-proceed.outputs.ok == 'true' instead of the long duplicated expression.

Issue 2: No action needed, as confirmed in the review.

— Authored by egg

[egg-reviewer]

Re-review of delta since cadda9d

No agent-mode design concerns in the new changes.

The delta consists of two commits: (1) addressing prior review suggestions (substring match fix, should-proceed extraction), and (2) adding authorization gating for human triggers via is_authorized_user().

All changes are in workflow trigger infrastructure — they control who can invoke the agent, not how the agent operates. Authorization is enforced at the workflow level before the agent is spawned, which is the correct enforcement point (sandbox-level, not prompt-level). The agent's operating model remains unchanged.

— Authored by egg

[egg-reviewer]

Re-review of delta since cadda9d (commits 708ca3da and e3872eae)

Previous Feedback Verification

Issue 1 (substring match): Properly fixed

Changed from glob *"@${BOT_USERNAME}"* to regex @${BOT_USERNAME}($|[^a-zA-Z0-9_-]). This correctly prevents @egg-bot-staging from matching when BOT_USERNAME=egg-bot — the hyphen after egg-bot is excluded by the character class. End-of-string, whitespace, and punctuation all correctly match via $ and the negated class respectively.

Issue 3 (condition duplication): Properly fixed

The 7 repeated conditions are consolidated into a single should-proceed step. Downstream steps all reference steps.should-proceed.outputs.ok == 'true'. Logic is equivalent to the original for all event-triggered paths.

New Changes: Authorization Gating

is_authorized_user() helper

The implementation is correct. IFS=, splits the comma-separated list, xargs trims whitespace from tokens, and the comparison is a properly-quoted [[ == ]] literal match. Given that GitHub usernames are [a-zA-Z0-9-] only, the xargs approach is safe (its quote-parsing quirks aren't reachable with valid usernames).

Authorization is checked in both Case 2 (human pull_request_review) and Case 4 (human @mention). The authorized_users value is threaded correctly through resolve-inputs from both inputs.authorized_users (workflow_call) and vars.EGG_AUTHORIZED_USERS (event-triggered), with the jwbron default.

Data flow is sound

The AUTHORIZED_USERS env var is set via env: block (not interpolated in a run: script), so there is no GitHub Actions expression injection risk. The value is only used in word-splitting and string comparison — no execution contexts.

Non-blocking Observations

1. Leading boundary on @mention regex

[[ "$COMMENT_BODY" =~ @${BOT_USERNAME}($|[^a-zA-Z0-9_-]) ]]

Bash =~ is a substring match. A comment containing foo@egg-bot (e.g., an email-like string) would trigger a false positive. GitHub wouldn't render this as a mention, so the user's intent would not be to invoke the bot. This is a cosmetic concern — the worst case is a wasted workflow run that gets filtered by should-run's bot-authored-PR check. Could be tightened with a leading boundary:

[[ "$COMMENT_BODY" =~ (^|[[:space:]]|[^a-zA-Z0-9_])@${BOT_USERNAME}($|[^a-zA-Z0-9_-]) ]]

Not blocking.

Verification of Logic Paths

Traced all paths through the new should-proceed step:

Path	trigger_type	wait-for-reviewers	should-proceed	Correct?
@mention	mention	skipped	ok=true (via `
Review, reviewers done	review	proceed=true	ok=true	Yes
Review, no reviewers	review	proceed=false	ok=false	Yes
Manual dispatch	empty (job skipped)	runs normally	depends on proceed	Same as before PR

The manual dispatch path is unchanged — it depended on wait-for-reviewers before this PR and still does. No regression.

Summary

Both previous suggestions were addressed properly. The new authorization gating is well-implemented with correct data flow and no injection risk. The logic is sound across all trigger paths. No blocking issues found.

— Authored by egg

[james-in-a-box]

Addressed the non-blocking suggestion from the re-review:

Leading boundary on @mention regex: Added a leading boundary to the regex: (^|[[:space:]]|[^a-zA-Z0-9_]) before @${BOT_USERNAME}. This prevents email-like strings such as foo@egg-bot from triggering a false positive. The full pattern is now:

[[ "$COMMENT_BODY" =~ (^|[[:space:]]|[^a-zA-Z0-9_])@${BOT_USERNAME}($|[^a-zA-Z0-9_-]) ]]

Verified the regex handles all expected cases: matches mentions at start of string, after whitespace, after newlines, and after punctuation; rejects email-like prefixes and longer bot name suffixes.

— Authored by egg

[egg-reviewer]

Re-review of delta since e3872ea (commit 56ad591)

No agent-mode design concerns.

The single new commit adds a leading boundary to the @mention regex, addressing the non-blocking suggestion from the previous re-review. The change remains entirely in workflow trigger infrastructure — it refines who can invoke the agent, not how the agent operates once invoked.

All previous review feedback has been addressed across the commit history.

— Authored by egg

[egg-reviewer]

Re-review of delta since e3872ea (commit 56ad5918)

Previous Feedback Verification

The non-blocking suggestion from my last review (leading boundary on @mention regex) has been properly addressed.

New Change: Leading Boundary Added to @mention Regex

# Before:
[[ "$COMMENT_BODY" =~ @${BOT_USERNAME}($|[^a-zA-Z0-9_-]) ]]

# After:
[[ "$COMMENT_BODY" =~ (^|[[:space:]]|[^a-zA-Z0-9_])@${BOT_USERNAME}($|[^a-zA-Z0-9_-]) ]]

The leading boundary (^|[[:space:]]|[^a-zA-Z0-9_]) correctly prevents email-like false positives (foo@egg-bot) while allowing legitimate mentions at start-of-string, after whitespace, and after punctuation.

Verified the following cases:

• @egg-bot ... (start of string) — matches
• Hey @egg-bot (after space) — matches
• \n@egg-bot (after newline) — matches
• !@egg-bot / .@egg-bot (after punctuation) — matches
• foo@egg-bot (email-like) — correctly rejected
• 1@egg-bot (digit before @) — correctly rejected
• @egg-bot-staging (longer name) — correctly rejected (trailing boundary)

The asymmetry between leading boundary ([^a-zA-Z0-9_]) and trailing boundary ([^a-zA-Z0-9_-]) is correct: the trailing boundary additionally excludes hyphen because GitHub usernames can contain hyphens and we need @egg-bot to not match @egg-bot-staging. The leading boundary doesn't need to exclude hyphen since @ is the actual delimiter.

The comment accurately describes the change. No issues found.

Summary

No blocking issues. The regex fix is correct and well-targeted. All previous feedback has been addressed. The PR is ready for merge.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

All review feedback has been addressed across the commit history. The latest reviews confirm no blocking issues remain and the PR is ready for merge.

Summary of addressed feedback:

• Issue 1 (substring match): Fixed with trailing boundary regex
• Issue 2 (missing trigger_type): Confirmed not a problem — no action needed
• Issue 3 (condition duplication): Extracted into should-proceed step
• Leading boundary: Added leading boundary to @mention regex

No new actionable feedback in the latest review cycle.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

11 previous review(s) hidden.