fix: restrict Q workflow triggers to eliminate action_required from PR events

[Copilot]

The Q workflow has been 100% action_required for 10+ days. Root cause: GitHub's bot-approval mechanism fires at the platform level when a workflow is triggered by pull_request or pull_request_review_comment events — producing action_required with 0 jobs/0 duration before any steps run. PR #43527's prompt-level fix had no effect because this is an infrastructure trigger issue.

Changes

• q.md — Adds events: [issues, issue_comment, pull_request_comment, discussion, discussion_comment] to slash_command:, dropping pull_request and pull_request_review_comment from the trigger set.

• q.lock.yml — Recompiled: removes both event types from on:, updates activation.if and pre_activation.if consistently.

• Compiler fix: expression_builder.go — buildCommentAuthorAssociationCondition() previously always emitted guards for both issue_comment and pull_request_review_comment regardless of which events are active, producing dead-code conditions. Now accepts activeCommentEvents []string and only emits guards for events actually present in on:.

  // Before — always emitted both clauses even if pull_request_review_comment isn't a trigger
  (github.event_name != 'issue_comment' && github.event_name != 'pull_request_review_comment') || ...
  
  // After — only emits the clause for the active event
  github.event_name != 'issue_comment' || ...

• Compiler fix: compiler_pre_activation_job.go — Adds activeCommentEventsInOn() helper to extract active comment events from the compiled on: string; passes them to the updated buildCommentAuthorAssociationCondition().

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #45745 does not have the 'implementation' label and has only 65 new lines of code in business logic directories (threshold: 100).

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Test Quality Sentinel skipped.

[Copilot]

Pull request overview

Restricts Q workflow triggers to avoid action_required runs and aligns generated authorization guards with active events.

Changes:

• Removes problematic pull request event triggers.
• Generates event-specific author-association guards.
• Recompiles the Q workflow lockfile.

Show a summary per file

File	Description
pkg/workflow/expression_builder.go	Builds guards only for active comment events.
pkg/workflow/compiler_pre_activation_job.go	Detects active comment triggers.
.github/workflows/q.md	Restricts Q slash-command events.
.github/workflows/q.lock.yml	Applies the recompiled trigger configuration.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Medium

[github-actions]

The changes are correct and well-motivated. Removing pull_request and pull_request_review_comment triggers eliminates the platform-level action_required firings. The pull_request_comment alias in q.md correctly maps to issue_comment in GitHub Actions, so activeCommentEventsInOn returns only issue_comment and the simplified guard is accurate. The buildCommentAuthorAssociationCondition refactor cleanly emits guards only for active trigger events.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 24 AIC · ⌖ 4.27 AIC · ⊞ 5K

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — commenting, no blocking issues.

📋 Key Themes & Highlights

Root Cause

The fix is correct and well-reasoned: pull_request / pull_request_review_comment events trigger GitHub's bot-approval mechanism at the platform level before any job runs, causing 100% action_required failures. Removing them from the trigger set is the right fix.

Compiler Fix Quality

The change to buildCommentAuthorAssociationCondition is clean — accepting activeCommentEvents []string and emitting guards only for present events eliminates the dead-code conditions precisely.

Positive Highlights

• ✅ Good use of slices.Contains for readable guard logic
• ✅ activeCommentEventsInOn helper is well-commented and consistent with the existing hasCommentEventInOn pattern
• ✅ PR description clearly explains the root cause and the before/after expression difference

Suggestions (non-blocking)

1. Missing regression test (see inline): the most important new behaviour — that issue_comment-only config drops pull_request_review_comment from the guard — has no dedicated test.
2. Unreachable case 0 (see inline): silently emits an always-true permissive expression; better to panic or document the precondition.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 50.3 AIC · ⌖ 4.68 AIC · ⊞ 6.7K
_{Comment /matt to run again}

[github-actions]

[/diagnosing-bugs] The case 0 branch (BuildBooleanLiteral(true)) is unreachable in practice — this function is only called when hasCommentEventInOn() returns true, guaranteeing at least one active comment event. If that guard is ever relaxed, case 0 would silently emit a permissive always-true expression, bypassing the author-association check entirely.

💡 Suggestion

Make the invariant explicit with a panic to fail loudly rather than silently permitting all actors:

case 0:
    // Unreachable: callers only invoke this function when hasCommentEventInOn() is
    // true, so activeCommentEvents always has at least one entry.
    panic("buildCommentAuthorAssociationCondition: no active comment events")

Alternatively, add a doc comment on the function signature declaring the precondition.

@copilot please address this.

[github-actions]

Two issues to fix before merge

The core approach is sound — dropping pull_request/pull_request_review_comment triggers and emitting guards only for events actually in on: is correct. The YAML parsing via "<event>:" substring match is safe because data.On is always compiler-generated key-colon YAML (same pattern already used by hasCommentEventInOn).

🔴 Blocking findings

1. Fail-open case 0 in buildCommentAuthorAssociationCondition (expression_builder.go line 209)
When activeCommentEvents is empty the function returns BuildBooleanLiteral(true), which ORed with authorizedAssoc makes the whole guard trivially true — the author-association check is silently compiled away. The outer call-site gate (line 521) currently prevents this, but it's an undocumented precondition enforced at a distance. Any future caller passing an empty slice removes the security guard with no warning. The old code was fail-closed; the new code is fail-open here. The case 0 fallback should preserve the full guard.

2. Duplicated event name list (expression_builder.go line 194 + compiler_pre_activation_job.go lines 592–597)
guardedEvents inside buildCommentAuthorAssociationCondition and the strings.Contains checks in activeCommentEventsInOn independently hardcode the same two event names. Adding a third guarded event requires updating both sites; missing one silently drops the guard for the new event with no compile-time signal. A single shared package-level constant would fix this.

🔎 Code quality review by PR Code Quality Reviewer · 56.1 AIC · ⌖ 4.68 AIC · ⊞ 5.6K
_{Comment /review to run again}

[github-actions]

case 0 silently drops the security guard when activeCommentEvents is empty — any future caller passing an empty slice will compile away the author-association check with no error or warning.

💡 Detail and suggested fix

The outer gate in compiler_pre_activation_job.go (line 521) currently prevents this path, but that's an undocumented precondition enforced at a distance. If any future call site passes an empty slice (e.g. from a different compiler path, a refactor, or a test), the function silently returns BuildBooleanLiteral(true). When ORed with authorizedAssoc, the full expression short-circuits to true — the guard is entirely removed.

The old code was fail-closed: it always emitted the guard. The new code is fail-open for the empty-input case. Security controls should default to the more restrictive posture.

Suggested fix — preserve the full guard as the fallback:

case 0:
    // No active events supplied; stay fail-closed by emitting the full unconditional guard.
    notCommentEvent = BuildAnd(
        BuildNotEquals(BuildPropertyAccess("github.event_name"), BuildStringLiteral("issue_comment")),
        BuildNotEquals(BuildPropertyAccess("github.event_name"), BuildStringLiteral("pull_request_review_comment")),
    )

Or add a panic/error so misuse is caught at test time rather than deployed silently.

[github-actions]

guardedEvents is a hidden dependency on a list maintained in two places — adding a third guarded event type requires updating both this list and activeCommentEventsInOn(), with no compile-time enforcement linking them.

💡 Detail and suggested fix

guardedEvents in buildCommentAuthorAssociationCondition (line 194) and the two strings.Contains checks in activeCommentEventsInOn (lines 592–597) independently hardcode the same event name set. If a developer adds e.g. workflow_run_comment to guardedEvents to guard a future event type, they must also add a corresponding strings.Contains in activeCommentEventsInOn or the new event will never appear in the active list and its guard will silently be omitted.

Suggested: define a single package-level var guardedCommentEvents = []string{"issue_comment", "pull_request_review_comment"} and drive both functions from it, so the two sites cannot diverge.

Done

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.10