Skip to content

fix(panel-workflow): only fire on label, add dispatch path for fork PRs#836

Merged
danielmeppiel merged 1 commit intomainfrom
fix/panel-review-trigger-and-fork-support
Apr 22, 2026
Merged

fix(panel-workflow): only fire on label, add dispatch path for fork PRs#836
danielmeppiel merged 1 commit intomainfrom
fix/panel-review-trigger-and-fork-support

Conversation

@danielmeppiel
Copy link
Copy Markdown
Collaborator

@danielmeppiel danielmeppiel commented Apr 22, 2026

Problems

1. Triggers on every push (cost / spam)

The panel review workflow re-runs on every push to a labelled PR. Reproduced repeatedly on PR #832 -- each push burned a full panel run.

Root cause: the activation gate only enforces the label check on the labeled action. synchronize bypasses it.

# pr-review-panel.lock.yml (current)
if: needs.pre_activation.outputs.activated == 'true' && (
      github.event_name != 'pull_request'
      || github.event.action != 'labeled'
      || github.event.label.name == 'panel-review'
    )

For action = 'synchronize', the second clause is true -> gate passes regardless of label. Bug, not by design.

2. Fork PRs fail (no secrets)

Run on PR #815 failed because pull_request from a fork does not pass repository secrets (COPILOT_GITHUB_TOKEN etc.). gh-aw correctly blocks pull_request_target on public repos -- that pattern executes the PR's code with full write tokens.

There is no pull_request-based path that gives fork PRs full secrets on a public repo. The portable, GHES-compatible answer is workflow_dispatch (always runs in the base/trusted context, full secrets, regardless of where the PR head lives).

Fix

Two surgical changes to .github/workflows/pr-review-panel.md (lock recompiled with gh aw compile):

  1. Drop synchronize from pull_request types -- the panel runs once when the maintainer applies panel-review. To re-run after addressing findings: remove + re-apply the label.
  2. Add workflow_dispatch with a required pr_number input. Maintainers trigger the review manually for fork PRs from the Actions tab (or via gh workflow run pr-review-panel.lock.yml -f pr_number=NNN).
  3. Wire ${{ inputs.pr_number }} as fallback for github.event.pull_request.number throughout the prompt body so both paths target the same PR.

Trust model (unchanged, and strictly safer than pull_request_target for forks)

workflow_dispatch runs from the dispatched ref (default main), NOT the PR head. For this workflow that is the desired property:

Concern What dispatch gives us
Workflow YAML that executes The one on main -- trusted, already-merged
Skill / persona definitions Already pinned to microsoft/apm#main via imports:
Where the PR diff comes from gh pr view + gh pr diff -- API calls returning inert text
Whether PR code is executed No. No actions/checkout of the PR head; the prompt explicitly forbids git checkout of PR head. The panel reviews the diff as data, never runs it (no tests, no build, no scripts)
Secrets Full repo secrets available (base/trusted context)

This is strictly safer than the pull_request_target pattern, which can be coerced into running PR-controlled code with write tokens. Here, a malicious PR can put anything in its diff and the worst it can do is mislead the review prose -- it cannot escalate to code execution or token exfiltration.

Other invariants unchanged:

  • Both paths require write access (label gate / dispatch gate).
  • Imports still pinned to microsoft/apm#main.
  • Same READ-only permissions: block; safe-outputs.add-comment mediates the only write surface.

Verification

  • gh aw compile .github/workflows/pr-review-panel.md -> 0 errors, 0 warnings.
  • Lock diff: types: [labeled, synchronize] -> types: [labeled]; workflow_dispatch block added with pr_number input; expression substitution ${{ github.event.pull_request.number || inputs.pr_number }} resolved correctly across 3 use sites.
  • Activation gate (line ~88 of lock.yml) is correct for the reduced trigger set: workflow_dispatch -> first clause true; labeled with panel-review -> third clause true; labeled with other labels -> all false (skipped).

Operator notes

To trigger the panel on a fork PR after this lands:

gh workflow run pr-review-panel.lock.yml -R microsoft/apm -f pr_number=815

Or from the Actions tab: "PR Review Panel" -> "Run workflow" -> enter PR number.

Note: when dispatching, leave the "Use workflow from" branch as main (the default). Dispatching from a feature branch would run that branch's workflow definition against the PR -- not what you want.

The panel review workflow was running on every push to a labelled PR
because the activation gate only enforced the label check on the
'labeled' action; 'synchronize' bypassed it entirely. This burned
agent quota and produced redundant verdicts.

Changes:

1. Drop 'synchronize' from pull_request types. The panel runs once
   when a maintainer applies 'panel-review'. To re-run after
   addressing findings, remove + re-apply the label.

2. Add workflow_dispatch with a pr_number input. This is the
   GitHub.com- and GHES-compatible path for reviewing PRs from
   forks, where 'pull_request' does NOT pass repository secrets
   (COPILOT_GITHUB_TOKEN etc.) and gh-aw blocks
   'pull_request_target' on public repos. workflow_dispatch always
   runs in the base/trusted context with full secrets, regardless
   of where the PR head lives.

3. Wire ${{ inputs.pr_number }} as the fallback for
   github.event.pull_request.number throughout the prompt body so
   both event paths target the same PR.

Refs https://github.com/microsoft/apm/actions/runs/24752294360 (fork
PR #815 lost secrets and failed).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 22, 2026 08:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the PR Review Panel Agentic Workflow to avoid rerunning on every push after labeling, and adds a manual dispatch path so maintainers can run the panel against fork PRs (where pull_request does not receive secrets).

Changes:

  • Remove synchronize from the pull_request trigger so the workflow runs only on label application.
  • Add workflow_dispatch with a required pr_number input for maintainers to run the panel on fork PRs.
  • Thread a PR-number fallback expression through the prompt content so both trigger paths target the same PR.
Show a summary per file
File Description
.github/workflows/pr-review-panel.md Adds workflow_dispatch, documents the trigger model, and uses a PR-number fallback in the prompt.
.github/workflows/pr-review-panel.lock.yml Recompiled lock file reflecting trigger changes and the new PR-number expression wiring.

Copilot's findings

Comments suppressed due to low confidence (1)

.github/workflows/pr-review-panel.lock.yml:193

  • GH_AW_GITHUB_EVENT_PULL_REQUEST_NUMBER is still populated from github.event.pull_request.number, which will be empty on workflow_dispatch. If any downstream steps/tooling rely on this variable (or if the built-in context is expected to show the PR number), dispatch runs may behave differently. Set this env var using the same ${{ github.event.pull_request.number || inputs.pr_number }} fallback expression used elsewhere.
          GH_AW_EXPR_A0E5D436: ${{ github.event.pull_request.number || inputs.pr_number }}
          GH_AW_GITHUB_ACTOR: ${{ github.actor }}
          GH_AW_GITHUB_EVENT_COMMENT_ID: ${{ github.event.comment.id }}
          GH_AW_GITHUB_EVENT_DISCUSSION_NUMBER: ${{ github.event.discussion.number }}
          GH_AW_GITHUB_EVENT_ISSUE_NUMBER: ${{ github.event.issue.number }}
          GH_AW_GITHUB_EVENT_PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number }}
  • Files reviewed: 2/2 changed files
  • Comments generated: 2

Comment on lines +64 to +74
workflow_dispatch:
inputs:
aw_context:
default: ""
description: Agent caller context (used internally by Agentic Workflows).
required: false
type: string
pr_number:
description: Pull request number to review (works for fork PRs)
required: true
type: string
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With workflow_dispatch added, the workflow's concurrency key should incorporate the dispatched PR number as well. Otherwise manual runs typically share the same github.ref (default branch) and can cancel each other across different PRs. Update the concurrency group expression to include the same PR-number fallback used elsewhere, then recompile the lock file.

Copilot uses AI. Check for mistakes.

```bash
PR=${{ github.event.pull_request.number }}
PR=${{ github.event.pull_request.number || inputs.pr_number }}
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workflow_dispatch inputs are user-controlled strings; assigning PR=${{ ... }} unquoted allows shell expansions if the input contains $()/backticks/etc. Quote the assignment and/or validate that pr_number is strictly digits before using it in gh pr view/diff (especially important since this path runs with secrets).

Suggested change
PR=${{ github.event.pull_request.number || inputs.pr_number }}
PR="${{ github.event.pull_request.number || inputs.pr_number }}"
case "$PR" in
''|*[!0-9]*)
echo "[x] Invalid PR number: must contain digits only" >&2
exit 1
;;
esac

Copilot uses AI. Check for mistakes.
@danielmeppiel danielmeppiel merged commit a7d7f01 into main Apr 22, 2026
29 checks passed
@danielmeppiel danielmeppiel deleted the fix/panel-review-trigger-and-fork-support branch April 22, 2026 08:53
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

Pushed 3a65b589 — drop forks: ['*'] to eliminate silent fork-PR failures

Following up on run 24769454293 which appeared to be 'the fix not working' but was actually an old pull_request label-event run firing before this PR merged. The error confirms it:

COPILOT_GITHUB_TOKEN: <empty>
Error: None of the following secrets are set: COPILOT_GITHUB_TOKEN

GitHub strips repo secrets from pull_request events on fork PRs; there is no way to grant them via this trigger on a public repo, regardless of forks: setting.

The previous fix (#826, merged to main earlier today) added forks: ['*'] to the label trigger trying to enable fork-PR review, but that setting only controls whether the workflow fires, not whether secrets are passed. Result: the label path on a fork PR was guaranteed to silently fail.

This commit removes forks: ['*']. Effect on the activation gate:

# Before (had: `forks: ['*']`)
if: needs.pre_activation.outputs.activated == 'true' && (
      github.event_name != 'pull_request'
      || github.event.action != 'labeled'
      || github.event.label.name == 'panel-review'
    )

# After (no `forks:` -> gh-aw injects head-repo-id check)
if: needs.pre_activation.outputs.activated == 'true' && (
      (github.event_name != 'pull_request'
       || github.event.pull_request.head.repo.id == github.repository_id)
      && (github.event_name != 'pull_request'
          || github.event.action != 'labeled'
          || github.event.label.name == 'panel-review')
    )
Trigger context Before this commit After
Same-repo PR + panel-review label runs runs
Same-repo PR + other label skipped skipped
Fork PR + any label runs, fails on missing secret skipped (label is a no-op)
Push to PR (synchronize) runs (bug) n/a -- already removed
workflow_dispatch -f pr_number=N n/a runs (works for forks too)

Maintainers now have one deterministic path for fork PR reviews:

gh workflow run pr-review-panel.lock.yml -R microsoft/apm -f pr_number=815

PR #815 will be the first real test once this lands.

danielmeppiel added a commit that referenced this pull request Apr 22, 2026
PR #836 only dropped synchronize and added workflow_dispatch; the
pull_request trigger still skips fork PRs because GitHub does not pass
secrets to fork-PR workflows. Result: labelling a fork PR (e.g. #815)
silently failed at the secret validation step.

Switch to pull_request_target, which runs in the base repo context
with full secrets regardless of where the PR head lives. Combined
with roles: [admin, maintainer, write] (auto-injected by gh-aw), the
trust gate becomes 'only repo maintainers can trigger' -- which
matches the existing model that applying the panel-review label
already requires write access.

Why pull_request_target is safe for this workflow despite the
well-known pwn-request pattern:
  - permissions are read-only (no write to contents / actions)
  - no actions/checkout of PR head; only gh pr view / gh pr diff
    (returns inert text)
  - imports pinned to microsoft/apm#main (panel skill + persona
    definitions are trusted, not from the PR)
  - single write surface is safe-outputs.add-comment (max 1)

gh-aw does not expose names: on pull_request_target, so label-name
filtering moves into the prompt body (Step 0) -- a one-line bash
guard that exits cleanly when any non-panel-review label fires.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 22, 2026
…#837)

PR #836 only dropped synchronize and added workflow_dispatch; the
pull_request trigger still skips fork PRs because GitHub does not pass
secrets to fork-PR workflows. Result: labelling a fork PR (e.g. #815)
silently failed at the secret validation step.

Switch to pull_request_target, which runs in the base repo context
with full secrets regardless of where the PR head lives. Combined
with roles: [admin, maintainer, write] (auto-injected by gh-aw), the
trust gate becomes 'only repo maintainers can trigger' -- which
matches the existing model that applying the panel-review label
already requires write access.

Why pull_request_target is safe for this workflow despite the
well-known pwn-request pattern:
  - permissions are read-only (no write to contents / actions)
  - no actions/checkout of PR head; only gh pr view / gh pr diff
    (returns inert text)
  - imports pinned to microsoft/apm#main (panel skill + persona
    definitions are trusted, not from the PR)
  - single write surface is safe-outputs.add-comment (max 1)

gh-aw does not expose names: on pull_request_target, so label-name
filtering moves into the prompt body (Step 0) -- a one-line bash
guard that exits cleanly when any non-panel-review label fires.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 22, 2026
- Bump version to 0.9.1 in pyproject.toml and uv.lock
- Move CHANGELOG [Unreleased] entries into [0.9.1] - 2026-04-22
- Audit pass: 1 PR = 1 entry, no bloat
  - Consolidate the seven scattered entries from #832 into a single
    Added line that names the closed issues (#827/#829/#831/#834)
    and keeps the migration warning inline
  - Combine the three fork-PR fixes (#826, #836, #837) into one
    Fixed entry per the multi-PR convention
  - Drop the doc-only consolidation commits from the log

Highlights of 0.9.1:
- Install-time enforcement of org apm-policy.yml (#832)
- pr-review-panel automation now usable from fork PRs (#824, #826,
  #836, #837)
- Docs site publish gated on stable releases only (#822)
- Repository dogfooding via .apm/ as primitive source of truth (#823)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel added a commit that referenced this pull request Apr 22, 2026
- Bump version to 0.9.1 in pyproject.toml and uv.lock
- Move CHANGELOG [Unreleased] entries into [0.9.1] - 2026-04-22
- Audit pass: 1 PR = 1 entry, no bloat
  - Consolidate the seven scattered entries from #832 into a single
    Added line that names the closed issues (#827/#829/#831/#834)
    and keeps the migration warning inline
  - Combine the three fork-PR fixes (#826, #836, #837) into one
    Fixed entry per the multi-PR convention
  - Drop the doc-only consolidation commits from the log

Highlights of 0.9.1:
- Install-time enforcement of org apm-policy.yml (#832)
- pr-review-panel automation now usable from fork PRs (#824, #826,
  #836, #837)
- Docs site publish gated on stable releases only (#822)
- Repository dogfooding via .apm/ as primitive source of truth (#823)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants