Add review workflow

[JanKrivanek]

Motivation

Mount the expert-reviewer agent into PR process - triggered on PR open (for contributors only) and on /review slash command (by contributors only)

This PR intends to only mount the pre-existing reviewing logic (not to change it in any way)

[Copilot]

Pull request overview

This PR adds two GitHub Agentic Workflows to run the existing expert-reviewer agent: automatically on PR open and manually via a /review slash command, with shared configuration factored into a reusable import.

Changes:

• Added shared workflow configuration/prompt content for “expert review” runs.
• Added a /review command-triggered workflow and its compiled lockfile.
• Added a PR-open-triggered workflow and its compiled lockfile.

Show a summary per file

File	Description
.github/workflows/shared/review-shared.md	Introduces shared imported config/prompt body and attempts to centralize PAT rotation logic.
.github/workflows/review.agent.md	Defines the slash-command entrypoint workflow importing the shared config.
.github/workflows/review.agent.lock.yml	Compiled workflow for the slash-command review run.
.github/workflows/review-on-open.agent.md	Defines the on-open entrypoint workflow importing the shared config.
.github/workflows/review-on-open.agent.lock.yml	Compiled workflow for the on-open review run.

Copilot's findings

• Files reviewed: 5/5 changed files
• Comments generated: 3

[github-actions]

Expert Code Review

This PR adds CI/CD infrastructure to automatically trigger the existing expert-reviewer agent on PR open and on /review slash command. The change is purely additive (no MSBuild source modifications), so most of the 24 review dimensions are not applicable. The analysis below focuses on the dimensions that are relevant.

---

✅ No BLOCKING Issues

No MSBuild behavioral changes. No ChangeWave concerns. No backwards-compatibility risk.

---

✅ No MAJOR Issues

No performance, API surface, test coverage, or error message concerns (pure CI infrastructure).

---

MODERATE Observations

1. Permission mismatch between source and compiled output

Both review-on-open.agent.md and review.agent.md declare:

permissions:
  contents: read
  pull-requests: read

But the compiled lock files grant pull-requests: write (plus issues: write, discussions: write) on the conclusion and agent jobs — correctly needed to post review comments via add-comment. The source-level declarations are intentionally underspecified (the compiler injects the actual grants); this is consistent with how the framework works. However, the agent.md files could be misleading to maintainers who expect them to reflect the actual permissions in use.

2. review-on-open trigger fires on all opens — fork check is in the compiled layer only

review-on-open.agent.md unconditionally sets on: pull_request: types: [opened]. The fork protection (github.event.pull_request.head.repo.id == github.repository_id) and the membership check (GH_AW_REQUIRED_ROLES: "admin,maintainer,write") live only in the compiled lock file. This is correct by design, but means the security guarantee isn't visible in the human-readable source file. A comment in the front-matter noting this would help future maintainers.

---

MINOR Observations

3. PAT rotation comment — acknowledged temporary workaround

The review-shared.md comment explicitly calls the 10-token pool a stop-gap until organization-level billing is available. This is well-documented and appropriate. No action needed beyond tracking the eventual cleanup.

4. review-on-open does not retrigger on synchronize or reopened

The trigger covers only opened, not reopened or synchronize. This appears intentional (one review per PR lifecycle opening), but worth confirming it matches the desired UX — subsequent pushes won't automatically re-review. The /review slash command covers the on-demand case.

5. Lock files are auto-generated (DO NOT EDIT header present)

Both .agent.lock.yml files correctly carry the # This file was automatically generated by gh-aw ... DO NOT EDIT. header with the strict: true metadata hash. The lock files appear consistent with their .agent.md sources. No action needed.

---

Summary

The change is well-scoped and follows security best practices: all actions are pinned to full commit SHAs, fork PRs are rejected at the compiled activation level, contributor membership is gated to write/maintainer/admin roles, and permissions are minimal. The observations above are informational and do not block merging.

✅ Approved with the minor suggestion to add a comment in review-on-open.agent.md's front-matter noting that fork/membership guards are enforced in the compiled lock file.

Generated by Expert Code Review (on open) for issue #13503 · ● 610.5K · ◷

[JanProvaznik]

from description

triggered on PR open

this does not align with (at minimum my) workflow, I think the trigger should be "on PR moved from draft to Ready-for-review"

[JanKrivanek]

from description

triggered on PR open

this does not align with (at minimum my) workflow, I think the trigger should be "on PR moved from draft to Ready-for-review"

Adjusted to "on opened (draft PRs already filtered out) or on moved to ready_for_review"
In any case - the explicit /review can be used to ammend (e.g. for external contributors, for re-reviewing the PR upon changes, etc.)

[JanProvaznik]

/review

[JanProvaznik]

or does it work only if it's in main 😄 ?

[JanProvaznik]

fork PRs are rejected at the compiled activation level

this also does not align with typical team workflow, but if I get it correctly, that's a fundamental limitation so we may consider changing workflow

or am I missing the point of that comment and it's only about fork-targeting PRs, and upstream-targeting PRs from forks work?

[github-actions]

Expert Code Review — PR #13503: Add review workflow

Summary: This PR adds two Copilot Agentic Workflow (gh-aw) files that automatically invoke the expert-reviewer agent — one triggered by pull_request open/ready events and one by /review slash command — along with a shared configuration fragment. The change is CI-infrastructure only and touches no MSBuild source code.

---

BLOCKING

None.

---

MAJOR

None.

---

MODERATE

None.

---

NIT

NIT — Severity label mismatch in review-shared.md
FILE: .github/workflows/shared/review-shared.md

The shared body instructs the agent to organize findings by severity as BLOCKING > MAJOR > MODERATE > MINOR, but the expert-reviewer.md methodology uses NIT (not MINOR) for the lowest level. When the generated review uses NIT labels (as it does today in this very PR), any downstream tooling or humans parsing the structured output by severity name will see an undeclared category.

Recommendation: Change MINOR to NIT in the shared template:

-3. Post a single review comment summarizing findings, organized by severity (BLOCKING > MAJOR > MODERATE > MINOR).
+3. Post a single review comment summarizing findings, organized by severity (BLOCKING > MAJOR > MODERATE > NIT).

---

Security & Infrastructure observations (informational)

The key security properties are all correctly implemented:

• ✅ Fork safety: uses pull_request (not pull_request_target); lock file additionally guards with github.event.pull_request.head.repo.id == github.repository_id, so secrets are never exposed to fork-originated PRs.
• ✅ Contributor gate: pre_activation checks admin,maintainer,write membership before activating. The PR description says "contributors only" — this is actually write-permission members and above, which is intentional and appropriate.
• ✅ Least privilege: top-level permissions: {} at the workflow; pull-requests: write granted only in the conclusion/safe_outputs jobs that need it.
• ✅ Secret isolation: COPILOT_GITHUB_TOKEN is explicitly excluded from the agent container env (--exclude-env COPILOT_GITHUB_TOKEN), preventing accidental leakage into agent context.
• ✅ Action pinning: actions/checkout is pinned to a full commit SHA.
• ✅ Slash command injection protection: review.agent.lock.yml pre_activation includes a check_command_position step to prevent body-injection attacks via comment-embedded commands.

The lock files are compiled/generated by the gh-aw framework and should not be edited manually — this is clearly documented in the file headers.

---

Overall: LGTM with one NIT. The NIT is worth fixing for consistency but is not a blocker.

Generated by Expert Code Review (on ready) for issue #13503 · ● 609.6K · ◷

[JanProvaznik]

I suppose it's better than classic copilot code review but still a lot of space to improve. It uses sonnet model and from the logs there is no evidence of spawning subagents.

When I run the review workflow locally with opus and use /fleet I'd actually do the subagent review as intended.

[JanKrivanek]

@JanProvaznik

• IIRC only the PR triggers are allowed to trigger workflow that's being added by the PR. For slash commands it needs to be in main (here it was triggered by flipping to ready status, not slash command)
• to trigger on forks as well - there is a dedicated trigger for that pull_request_target. But let's start with explicit triggering for forks now - we can then reconsider based on outcomes.
• the appetite to call subagents is very low in my experience. I was fighting with that quite a bit on unrelated workflow (for adding tests - obviously :D) - and there I was able to get it to call at least the first level of agents (e.g.: https://github.com/JanKrivanek/machinelearning/actions/runs/24122439131/job/70379385136#step:24:236) - lm try to improve the prompt here a bit

[github-actions]

Expert Code Review — PR #13503 "Add review workflow"

Overall assessment: Well-structured PR that follows the established pattern from close-stale-prs.agent.md. The PR correctly reuses the existing select-copilot-pat action, the expert-reviewer agent, and introduces a shared import to reduce duplication. The lock files are auto-generated and consistent with the .md sources.

No BLOCKING or MAJOR issues found.

---

MODERATE

1. review-on-open.agent.md — pull-requests: read may be insufficient for posting comments

• Location: review-on-open.agent.md:66-67 and shared/review-shared.md:13-14
• Issue: Both the workflow and the shared import declare permissions: pull-requests: read. The shared instructions say to "Post a single review comment summarizing findings" via safe-outputs: add-comment. In the compiled lock file, the agent job (line 321) only has pull-requests: read, while the conclusion job (line 847) has pull-requests: write. If the safe-outputs add-comment tool runs during the agent job (not the conclusion job), the comment will fail due to insufficient permissions. The slash-command variant (review.agent.lock.yml) gives pull-requests: write to the activation job (line 97), so this asymmetry is notable.
• Suggestion: Verify that safe-outputs comments are posted exclusively in the conclusion job (which has write). If the agent job itself needs to post comments, you'll need pull-requests: write on the agent job or in the source .md permissions. This may be "working as designed" by the gh-aw compiler, but worth confirming with a dry-run on a test PR.

2. review.agent.md — No draft PR filter for the /review slash command

• Location: review.agent.md (entire file — no if: condition)
• Issue: The review-on-open.agent.md has if: github.event.pull_request.draft == false (line 54), which sensibly skips draft PRs. The /review command variant has no such guard. This means /review will run on draft PRs. This may be intentional (allowing authors to get early review feedback), but it's worth calling out since draft PRs often have incomplete code.
• Suggestion: If intentional, add a comment to review.agent.md noting this is by design. If not, add a similar if: guard or consider if the agent should emit a softer disclaimer on draft PRs.

---

MINOR

3. review-on-open.agent.md — Potential for noisy reviews on every PR open

• Location: review-on-open.agent.md:9-10 — types: [opened, ready_for_review]
• Issue: This triggers on every non-draft PR opened by a contributor. For a busy repo like MSBuild, this could generate many automated review comments, potentially creating noise. The close-stale-prs workflow runs weekly on schedule, so it's inherently bounded. This workflow has no rate-limiting beyond token pool rotation.
• Suggestion: Consider whether a debounce/cooldown mechanism or a label-based opt-in might be appropriate if the review volume becomes excessive. The timeout-minutes: 60 and cancel-in-progress: true concurrency do help — just noting for operational awareness.

4. shared/review-shared.md — Model pinned to claude-opus-4.6

• Location: shared/review-shared.md:33
• Issue: The instruction hardcodes model: "claude-opus-4.6" for the sub-agent call. This will become stale as newer models are released. The lock files reference vars.GH_AW_MODEL_AGENT_COPILOT || 'auto' for the top-level model selection, but the sub-agent is always pinned.
• Suggestion: Consider whether this should reference a repo variable or use "auto" to stay current, or add a comment noting this should be updated periodically.

5. shared/review-shared.md — PR number resolution for slash commands

• Location: shared/review-shared.md:28
• Issue: The expression ${{ github.event.pull_request.number || github.event.issue.number }} is correct for both triggers. For pull_request events, pull_request.number is set. For issue_comment events (slash commands on PRs), issue.number is used. This works correctly — just confirming it was reviewed.
• Suggestion: None — this is correct.

---

NIT

6. review-on-open.agent.md:5 — Comment says "Fork protection and contributor membership checks are enforced in the compiled lock file"

• Location: review-on-open.agent.md:5-7
• Issue: This is helpful context but could be confusing for someone unfamiliar with gh-aw. The lock file does confirm fork protection (github.event.pull_request.head.repo.id == github.repository_id at lock line 94) and membership checks (check_membership at lock line 1116-1127 requiring admin,maintainer,write roles).
• Suggestion: Consider linking to the gh-aw documentation URL already present in the lock file header.

7. Duplication between the two .agent.md files

• Location: review-on-open.agent.md and review.agent.md
• Issue: The PAT rotation block (~40 lines) is duplicated verbatim. The shared import correctly extracts permissions, tools, and prompt — but the PAT rotation cannot be shared via imports (as noted in shared/review-shared.md:9).
• Suggestion: This is a known gh-aw limitation. The comment on line 9 of the shared file documents it well. No action needed, but worth tracking if gh-aw adds import support for steps/jobs.

---

Security Summary ✅

• Fork protection: Enforced in compiled lock (head.repo.id == repository_id)
• Contributor-only: Membership check requires admin,maintainer,write roles
• PAT handling: Secrets are never logged; the select-copilot-pat action only outputs the index number, not the token value
• Permissions: Minimal (contents: read, pull-requests: read for agent job), elevated only where needed (conclusion job)
• Action pinning: All actions pinned to SHA, not tags ✅
• persist-credentials: false: Set on checkout ✅

Verdict

Approve — This PR correctly mounts the pre-existing expert-reviewer agent into the PR workflow using established patterns from the repository. The moderate findings are worth confirming but are not blockers.

Generated by Expert Code Review (on ready) for issue #13503 · ● 3.2M · ◷

[github-actions]

Expert Code Review — PR #13503: Add review workflow

Reviewed by: GitHub Copilot expert-reviewer agent (claude-opus-4.6) via expert-reviewer.md

This PR adds two agentic workflows (review-on-open.agent.md, review.agent.md) and a shared config (shared/review-shared.md) to automatically trigger the expert-reviewer agent on PR open/ready and on /review slash command. Both compiled lock files are present. No MSBuild C#/targets code is changed.

---

✅ Security — All checks pass

• Fork protection and contributor membership gates are enforced in the compiled lock files ✅
• actions/checkout is SHA-pinned (de0fac2e... labeled v6.0.2) ✅
• PATs passed via env: (not args:), minimizing log exposure ✅
• Minimal permissions: contents: read, pull-requests: read ✅
• Comment posting routes through the Copilot agent's own PAT (safeoutputs MCP), not the workflow's GITHUB_TOKEN, so pull-requests: read on the workflow is correct and sufficient ✅
• PAT rotation fallback is correct: if no pool secret has a value, the case() expression's final fallback returns secrets.COPILOT_GITHUB_TOKEN ✅

---

🟡 MODERATE

1. No draft-PR guard in review.agent.md (slash command workflow)

review-on-open.agent.md correctly skips drafts with:

if: github.event.pull_request.draft == false

review.agent.md has no equivalent guard. A /review comment on a draft PR will trigger the full agent run. This may be intentional (on-demand review of drafts), but the asymmetry is worth a conscious decision. If not desired, add:

if: github.event.issue.pull_request && github.event.issue.draft == false
```
(or document the intentional difference with a comment)

---

### 🟠 MINOR

**2. Model version hardcoded in `review-shared.md` (line 33)**

```
`model: "claude-opus-4.6"`

This will need manual updates as the model evolves. Consider a brief comment noting the intention (use the highest-capability model available) so future maintainers know why it's hardcoded rather than using a symbolic alias.

3. Potential for noisy reviews on every non-draft PR open

review-on-open.agent.md triggers on both opened and ready_for_review. For repositories with high PR velocity, every contributor PR will get an automated review run immediately. This is by design per the PR description, but worth validating against billing/quota expectations for the PAT pool — especially since the PAT pool is a stop-gap until organization-level billing is available.

---

💬 NITs

• PAT rotation duplication: Both .agent.md files duplicate the steps, jobs, and engine PAT rotation block. The shared comment correctly explains this is a gh-aw limitation. No action needed — just confirming it's acknowledged.
• permissions block in shared/review-shared.md (lines 13–15): This repeats pull-requests: read already declared in both parent workflow files. It's harmless (gh-aw uses most-restrictive merge) but redundant — a comment clarifying merge behavior would help future readers.
• ${{ github.event.pull_request.number || github.event.issue.number }}: Correct for both triggers — pull_request events have pull_request.number, slash command events (issue comment) use issue.number, which equals the PR number for PR comments. ✅

---

Summary

No blocking or major issues. The PR correctly follows existing PAT rotation patterns, uses minimal permissions, pins action SHAs, and delegates access control to the compiled lock files. The two moderate/minor items above are worth a quick author decision before merging.

Generated by Expert Code Review (on ready) for issue #13503 · ● 3.2M · ◷