fix: skip roles check when slash command not triggered on issue open - #48991
Conversation
For workflows using the `on.slash_command` trigger, the pre_activation job previously ran `check_membership` before `check_command_position`. When any issue was opened without the slash command in its body, both checks ran and both emitted warnings: - "Access denied: User '...' is not authorized" (from check_membership) - "The trigger comment did not start with a required command" (from check_command_position) The roles warning is confusing because it fires even when there was no attempt to trigger the slash command at all. Fix: - Move `check_command_position` to run first for command workflows - Add `if: steps.check_command_position.outputs.command_position_ok == 'true'` to the `check_membership` step for command workflows Now the membership/roles check is only performed when the slash command was actually found in the trigger text. If the slash command is absent, only the "command not found" message appears (or no message at all, since the job-level if: condition typically skips the job entirely). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (66 additions across 3 files). |
There was a problem hiding this comment.
🟢 Ready to approve
The focused change correctly addresses the warning while preserving activation conditions and adding regression coverage.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR prevents unnecessary membership checks when a slash command was not triggered.
Changes:
- Runs command-position validation before membership validation.
- Conditionally skips membership checks for absent commands.
- Adds compilation coverage for ordering and conditional behavior.
File summaries
| File | Description |
|---|---|
pkg/workflow/compiler_pre_activation_job.go |
Reorders pre-activation checks. |
pkg/workflow/role_checks.go |
Guards command-workflow membership checks. |
pkg/workflow/slash_command_output_test.go |
Tests ordering and conditional generation. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Medium
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix correctly reorders check_command_position before check_membership and adds an if: guard so the membership check is skipped when no slash command was present. Code is clean, the YAML step syntax is valid, and the new test covers both the ordering and the conditional. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.2 AIC · ⌖ 5.42 AIC · ⊞ 5.3K
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (2 tests)
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-tested. Approving.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause correctly addressed:
check_command_positionis moved beforecheck_membershipso theif:conditional guard can reference its output - ✅
buildPreActivationActivatedConditionsalready includescommand_position_okin theactivatedexpression — the end-to-end guard is consistent - ✅ Regression test
TestSlashCommandMembershipCheckConditionalvalidates both step ordering and theif:conditional, covering the exact bug scenario - ✅ Comment at the removal site in
buildPreActivationRolesBotsCmdStepsexplains why the step moved, making the intent clear
Minor observation
The test slices membershipSection using membershipSection[1:] as the search base when looking for the next step boundary. This is a cosmetic off-by-one that does not affect correctness for the current compiled output, but worth noting for future readers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.9 AIC · ⌖ 4.84 AIC · ⊞ 7K
Comment /matt to run again
|
@copilot please run the Failed checks to investigate: Run: https://github.com/github/gh-aw/actions/runs/30497974805
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed pr-finisher pass:
The PR is ready for merge pending a human CI re-trigger (close/reopen or workflow_dispatch) to get a fresh run of the PR Code Quality Reviewer. |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/30501214572
|
PR status summary — no code changes needed: ✅ Reviews — All four automated reviews approved (copilot-pull-request-reviewer, Impeccable Skills Reviewer, Test Quality Sentinel, Matt Pocock Skills Reviewer). No unresolved threads. ✅ Checks (local) — ❓ Checks (CI on current HEAD ✅ Branch — Up to date with main (base Human-only actions remaining:
|
|
@copilot please run the
|
Triage: fix: skip roles check when slash command not triggered on issue open
|
No changes since last pass. Status unchanged:
Remaining human-only actions:
|
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/30507870059
|
Status unchanged since last pass. No code changes needed.
Remaining human-only actions:
|
Triage Summary
Fixes confusing 'Access denied' UX bug for slash_command workflows. Large diff (56 files); PR is blocked by merge conflicts — needs rebase before review.
|
When a workflow uses
on.slash_command, every issue-opened event (regardless of body content) rancheck_membershipbeforecheck_command_position, producing a confusing "Access denied: User is not authorized" warning even when no slash command was attempted.Changes
compiler_pre_activation_job.go: For command workflows,check_command_positionis now added inbuildPreActivationCheckStepsbeforecheck_membership, and removed frombuildPreActivationRolesBotsCmdStepsrole_checks.go:check_membershipstep gainsif: steps.check_command_position.outputs.command_position_ok == 'true'for command workflows — the roles check only runs when the slash command was actually found in the trigger textslash_command_output_test.go: AddedTestSlashCommandMembershipCheckConditionalverifying step ordering and the conditional guardCompiled output for a
slash_commandworkflow now looks like:@copilotplease run thepr-finisherskill, review the latest PR status, and rerun any needed checks once the branch is current. If no code changes are needed, summarize why this PR is ready for maintainer review and call out any remaining human-only actions.Run: https://github.com/github/gh-aw/actions/runs/30501214572
Run: https://github.com/github/gh-aw/actions/runs/30503727864