Add scheduled holistic reviews for all PRs and path-specific review instructions#130339
Conversation
Move the general, language-agnostic review criteria (reviewer mindset, Holistic PR Assessment, PR hygiene, codebase consistency, and documentation) verbatim out of .github/skills/code-review/SKILL.md into a path-specific instruction file scoped to src/**, and point the skill at the new file. Copilot agents -- including the Copilot code review agent -- automatically respect .github/instructions/**/*.instructions.md files whose applyTo glob matches the changed files, so these rules now apply to native Copilot reviews in addition to the code-review skill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the C# rules (error handling, thread safety, security, correctness patterns, performance/allocations, API design, and code style) verbatim out of the skill into a file scoped to **/*.cs, and add the loading reference. Copilot agents -- including the Copilot code review agent -- automatically respect .github/instructions/**/*.instructions.md files whose applyTo glob matches the changed files, so these C# rules now apply to native Copilot reviews of C# files in addition to the code-review skill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the native rules (JIT-specific correctness, C++ style, runtime/VM patterns, native performance, platform defines, and P/Invoke marshalling) verbatim out of the skill into a file scoped to the C/C++/asm globs, and add the loading reference. Copilot agents -- including the Copilot code review agent -- automatically respect .github/instructions/**/*.instructions.md files whose applyTo glob matches the changed files, so these native rules now apply to native Copilot reviews of native files in addition to the code-review skill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move the testing rules verbatim out of the skill into a file scoped to the test globs, and add the loading reference. The skill now contains only the review process and points to the path-specific instruction files. Copilot agents -- including the Copilot code review agent -- automatically respect .github/instructions/**/*.instructions.md files whose applyTo glob matches the changed files, so these test rules now apply to native Copilot reviews of test files in addition to the code-review skill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…trator - Add code-review-orchestrator.md: on a 10-minute schedule (and workflow_dispatch) it polls open PRs, computes the new/updated set deterministically from a reviewed-SHA actions/cache, and dispatches the code-review workflow per PR via the dispatch-workflow safe output. - Rework code-review.md from a pull_request-triggered workflow into a per-PR worker (workflow_dispatch) dispatched by the orchestrator; it reviews one PR using the code-review skill and posts a single holistic comment. - Both import the Copilot PAT pool; update .github/aw/actions-lock.json for the pinned actions the new locks use. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
There was a problem hiding this comment.
Pull request overview
This PR restructures the repo’s customized Copilot-based review automation into a scheduled “orchestrator + per-PR worker” model and moves detailed review guidance into path-scoped .github/instructions/*.instructions.md files so both the custom workflow and GitHub’s built-in Copilot code review can share the same rule source.
Changes:
- Converts
code-reviewinto aworkflow_dispatchper-PR worker and adds a scheduled orchestrator that deterministically selects PRs needing (re)review and dispatches the worker. - Splits language-/area-specific review guidance out of the
code-reviewskill into path-scoped instruction files under.github/instructions/. - Updates the Agentic Workflows action pinning to include
actions/cache/*@v4to support the new orchestrator’s state caching.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/code-review.md | Reworks the workflow into a per-PR workflow_dispatch worker with updated safe-outputs / concurrency and PAT-pool handling. |
| .github/workflows/code-review.lock.yml | Regenerated lockfile reflecting the worker model and updated tool/container configuration. |
| .github/workflows/code-review-orchestrator.md | New scheduled orchestrator that computes a deterministic dispatch list and fans out worker dispatches. |
| .github/workflows/code-review-orchestrator.lock.yml | New lockfile for the orchestrator workflow. |
| .github/skills/code-review/SKILL.md | Slims the skill to process + points at path-scoped instruction files; updates output header text. |
| .github/instructions/review-all-src.instructions.md | New “general” source review criteria for src/**. |
| .github/instructions/review-csharp.instructions.md | New C#-specific review criteria (**/*.cs). |
| .github/instructions/review-native.instructions.md | New native/interop review criteria (C/C++/asm globs). |
| .github/instructions/review-all-tests.instructions.md | New test review criteria (tests globs). |
| .github/aw/actions-lock.json | Adds pinned SHAs for actions/cache/restore@v4 and actions/cache/save@v4. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
# Conflicts: # .github/workflows/code-review.lock.yml # .github/workflows/code-review.md
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…hestration' into jeffhandley/code-review-orchestration
# Conflicts: # .github/skills/code-review/SKILL.md
Regenerate the PR-owned workflows with the pinned v0.82.6 compiler after merging current main. Acquisition command: gh extension install github/gh-aw --pin v0.82.6 Repository-wide compile command: gh aw compile --validate --schedule-seed dotnet/runtime Targeted commands used for this commit: gh aw compile code-review-orchestrator --validate --schedule-seed dotnet/runtime gh aw compile code-review --validate --schedule-seed dotnet/runtime Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b64a66b-759f-412a-80ed-9a73f02b17bd
Consolidate the deterministic scheduled orchestrator and COMMENT-only worker reviews with trusted main-branch agent configuration, schema-v5 comment-backed state, exact run provenance, bounded retries, and draft/manual targeting semantics. Bind reviews to the trusted numeric dispatch input, precompute cumulative patch scope safely across rebases and retargeting, route tools through the firewall CLI and gh proxies, honor CODE_REVIEW_WORKFLOW_MODEL, and retry reviews replaced by threat-detection caution banners. Regenerate holistic-review.lock.yml with gh-aw v0.82.6 and --schedule-seed dotnet/runtime. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b64a66b-759f-412a-80ed-9a73f02b17bd
Consolidate the tannergooding feedback across the split review instructions and code-review skill, including authoring-safe PR gates, precise overflow guidance, modern test conventions, and native-specific assertion and GC-EE interface rules. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b64a66b-759f-412a-80ed-9a73f02b17bd
Call out alphabetized csproj item groups and limit findings to ordering regressions introduced by the pull request. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b64a66b-759f-412a-80ed-9a73f02b17bd
tannergooding
left a comment
There was a problem hiding this comment.
A few structural notes on the instructions/skill/worker split, focused on where content is applied vs. where it should live. Individual points inline.
Note
These review comments were drafted with the assistance of GitHub Copilot.
|
Not for this PR -- flagging as potential future work since it's pre-existing in
Note This comment was drafted with the assistance of GitHub Copilot. |
tannergooding
left a comment
There was a problem hiding this comment.
Generally LGTM and the remaining nits are mostly pre-existing, so I'm not going to block on it. We can handle it in a follow up after this big refactoring
Guard scheduled fork runs and keep dispatched reviews within the worker's available tooling. Scope core-runtime review criteria and clarify authoring and agent-discovery guidance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d4ecd6f-4602-4e1f-9485-e9ac231cdda4
jeffhandley
left a comment
There was a problem hiding this comment.
Thanks @T-Gro, @tannergooding, and @PranavSenthilnathan — I’ve incorporated the in-scope feedback and replied inline on each thread.
The final design now:
- uses a conventional deterministic orchestrator, with scheduled runs skipped on forks while manual dispatch remains available;
- stores durable, retry-bounded state in one managed PR comment while treating submitted reviews as authoritative;
- submits additive, non-blocking
COMMENTreviews with line-anchored findings and never hides earlier feedback; - computes initial and incremental scope deterministically from cumulative PR patches, correctly handling rebases and base retargets;
- restores trusted agent configuration from
main, binds safe outputs to the dispatched PR, and narrows GitHub, network, and tool access; - splits review guidance into general, C#, native, test, and core-runtime scopes, with authoring caveats in the broad instruction files;
- gates area-agent discovery on actual tooling and available agents, while the dispatched worker explicitly skips fan-out.
I validated the workflow with pinned gh-aw v0.82.6, deterministic recompilation, YAML and Bash syntax checks, and the multi-iteration simulation. The simulation also confirmed that pull-requests: write is sufficient for the managed PR state comment; issues: write is not required.
The model-specific reliability guidance called out in the follow-up comment remains intentionally deferred as separate work.
Note
This summary was drafted with GitHub Copilot on behalf of the PR author.
Summary
Reworks the repository's customized code-review automation into two complementary parts:
pull_request-triggeredcode-review.mdThe dispatched agentic workflow submits a single, cumulative "Holistic Review" per commit range rather than a stateless diff-only pass, and it is designed to run alongside — not replace — the built-in Copilot reviewer.
Behavior: draft, ready, and closed pull requests
holistic-review-orchestrator'sworkflow_dispatchaccepts a comma-separatedpr_numbersinput ("including drafts and retry-limited review targets") that bypasses the draft filter for exactly the requested PRs, and an unchanged head with an existing durable review is not reviewed again.workflow_dispatchor when the repository is not a fork, preventing scheduled fork runs without blocking explicit manual dispatch.Coexistence with the built-in Copilot code review agent
The Holistic Review worker and GitHub's built-in Copilot code review agent are fully independent; neither waits on, hides, or edits the other's output, and both remain visible on the PR alongside human reviews. The path-specific instruction files under
.github/instructions/are consumed natively by both: the Holistic Review skill explicitly loads them, and Copilot's built-in reviewer already applies any.instructions.mdfile whoseapplyToglob matches the changed paths. Every Holistic Review ends with an explicit disclosure distinguishing it from the built-in review and linking back to theholistic-review.mdworkflow source.Reviews are strictly additive: the worker's only two safe outputs are up to 10 inline
create_pull_request_review_commentcalls and exactly onesubmit_pull_request_review, whose event is restricted toCOMMENT. It never submitsAPPROVEorREQUEST_CHANGES, and it never modifies, hides, or supersedes an earlier review or comment from this workflow, the built-in reviewer, or a human.Architecture: deterministic orchestrator + gh-aw worker
holistic-review-orchestrator.ymlis a conventional, deterministic GitHub Actions workflow — not an agentic workflow. Its top-levelpermissions: {}grants onlyactions: write/pull-requests: writeinside its single job, and it does nothing but call the GitHub REST API to decide what to dispatch and to read/write one managed state comment per PR. It never checks out PR content and never runs a model, keeping it secure even though it processes untrusted PR metadata.holistic-review.mdis thegh-aw-compiled agentic worker (compiled toholistic-review.lock.ymlwith gh-aw v0.82.6, pinned via.github/aw/actions-lock.jsontogithub/gh-aw-actions/setup@v0.82.6). The orchestrator dispatches it once per PR via the Actions API with the PR number, base ref, head SHA, the previously reviewed head and base SHA (when re-reviewing), a bounded review-history JSON blob, and a complete syntheticaw_context(run_id,repo,workflow_id,item_type,item_number) so gh-aw's safe-output layer has full pull-request context even though the triggering event isworkflow_dispatch, notpull_request.workflow_dispatchinputs at dispatch time and the worker's own submitted review afterward, which the orchestrator discovers by matching the run'sdisplay_title(Holistic Review #<PR> (<head SHA>)) and by recognizing gh-aw's automatic<!-- gh-aw-agentic-workflow: Holistic Review, ... workflow_id: holistic-review -->review-footer marker.See
github/gh-aw's documentation on compiled agentic workflows, safe outputs, andworkflow_dispatch-based dispatch, and GitHub's ownworkflow_dispatchand triggering-a-workflow documentation, for background on the primitives this design builds on.Why review state lives in a pull request comment
Each PR carries exactly one managed, orchestrator-owned issue comment holding a versioned (
version: 5) JSON state object: the last dispatched and last reviewed(commit, base ref, base SHA)pairs, the ID of the worker run that produced the last recorded review, a bounded initial-plus-latest review-history array, and a boundedreview_attempt_count. Comments were chosen over gh-aw's repo-memory/cache mechanisms because gh-aw v0.82.6 supports deterministic reads of repo-memory/comment-memory but not deterministic write-back of durable state from a dispatch-triggered workflow. Apull-requests: write-scoped comment is visible, auditable state that survives reruns, needs nocontents: write, and is trivially inspectable by anyone reading the PR. Crucially, a submitted, marker-tagged worker review is the authoritative record of what has actually been reviewed — not the state comment; the comment only suppresses duplicate in-flight dispatches, and legacy comment formats (including a pre-existing machine-only JSON format and an older HTML-marker format) are recognized once and migrated to the current schema in place.Retries and durable-review authority
A worker run that completes without submitting a review — a transient provider error, a threat-detection replacement, or a safe-output formatting failure — is never treated as "reviewed." The orchestrator only advances
last_reviewed_*when it finds a matching, marker-tagged submitted review for the exact dispatched commit and base; otherwise it clears that dispatch record and redispatches the same target. Retries for the same(head, base)target are bounded atMAX_REVIEW_ATTEMPTS = 5; once exhausted, the PR is reported as retry-limited in the workflow step summary rather than dispatched indefinitely, and it can still be retried explicitly through the manualpr_numbersinput. Because attempts are keyed by(head commit, base branch), a rebase, force-push, or base retarget resets the attempt counter for the new target instead of inheriting an unrelated failure count.Review scope: holistic initial review vs. patch-differential incremental review
A trusted, deterministic pre-agent step — not the model — computes the review scope before the agent starts: it resolves the current merge base, computes a patch ID for the PR's cumulative diff, and (for re-reviews) computes the previous merge base, a previous patch ID, a
git range-diff, and a raw patch diff, writing all of it tometadata.jsonplusrange-diff.txt/patch-diff.txtfor the agent to read.current_merge_base..headrange — the PR's actual base-to-head diff, not its head compared against the current state of the base branch.range-diff/patch-diffbetween the previous and current cumulative patches — not a raw tree diff between the two head commits — so a rebase that pulls in unrelated upstream changes cannot manufacture new findings. If the previous and current heads are identical but the merge base changed (a pure base retarget), the same prepared patch comparison is authoritative: only code whose inclusion or semantics changed because of the retarget is reviewed, and unchanged portions of the PR patch are not rediscovered. If the patch truly has not changed, the worker still submits aCOMMENTreview recording that fact and the refreshed Assessment History, so every successful run leaves a durable record.Model selection
The workflow does not hard-code a model.
engine.modelresolves from the workflow-specificHOLISTIC_REVIEW_MODELActions variable, allowing the model to be selected at environment, repository, or organization scope without editing or recompiling the workflow.Security model
actions: write/pull-requests: writeonly; the worker's top-level permissions arecontents: read,issues: read,pull-requests: read, and its actual review/comment capability comes from gh-aw's safe-output layer rather than a broadly scoped token held by the agent.tools.github.github-tokenis pinned to${{ secrets.GITHUB_TOKEN }}, so the agent'sgh/GitHub-proxy calls cannot be backed by a broaderGH_AW_GITHUB_*secret, and the declared read-only permissions are actually authoritative..githubtree, every supported engine configuration directory, and all recognized root instruction files) frommainbefore loading any guidance, because a plain checkout alone would leave PR-added files in those paths behind. PR versions of those paths — along with PR descriptions, comments, source comments, test data, and other PR-controlled text — are treated strictly as untrusted review content, never as instructions; changed files under a trusted overlay path are still reviewed from an explicit commit read or the PR diff, never from the restored worktree copy.web-fetchtool. The allowlist further excludes general-purpose process-launch surfaces (awk,find,xargs,rg,sed,sort,gh) so an allowlisted command cannot spawn another process or modify the workspace; the prompt separately forbids Git/GitHub CLI aliases, hooks, pagers, external helpers, credential helpers, and other child-process-spawning options, and treats the remaining compiler-injectedgit/gh/sortsurface honestly as defense in depth rather than a strict sandbox.network.allowedis limited todefaults(nodotnetfeed access, since builds/restores are disallowed), andtools.cli-proxy: truealongsidetools.github.mode: gh-proxyensures no native MCP endpoints remain mounted in the CLI — only the CLI-mountedsafeoutputsserver — avoiding a firewall interaction where native endpoints bypassed the intended proxy path and were denied.target: ${{ github.event.inputs.pr_number }}on bothcreate-pull-request-review-commentandsubmit-pull-request-review. A review cannot be redirected to any PR other than the one the orchestrator dispatched.pre-agent-stepsperform a token-scoped fetch of exactly the commits the review needs (current head, prior head, and prior base) while a token is still available, so the agent operates only on local Git objects afterward, including across force-pushes and retargets.Shared review instructions
The review rules that used to live entirely in
.github/skills/code-review/SKILL.mdare split into path-scoped.github/instructions/*.instructions.mdfiles, each with anapplyToglob, so they're consumed identically by the Holistic Review skill and by GitHub's built-in Copilot code review agent:review-all-src.instructions.md(src/**) — reviewer mindset, the Holistic PR Assessment (motivation, evidence, approach, cost-benefit, scope, risk, codebase fit), PR hygiene, code reuse, established conventions (including preserving pre-existing alphabetical ordering in modified.csprojitem groups and similar lists, flagging only ordering regressions the PR introduces, not pre-existing unsorted entries), and documentation/comment rules.review-csharp.instructions.md(**/*.cs) — C#-specific error handling, thread safety, security, correctness, performance/allocation, API design, and style rules.review-native.instructions.md(C/C++/asm globs) — JIT-specific correctness, C++ style, VM/interpreter conventions, native size/offset overflow guarding, platform defines, and P/Invoke marshalling.review-all-tests.instructions.md(test globs) — testing conventions and regression-test expectations.review-core-runtime.instructions.md(src/coreclr/**,src/native/corehost/**) — CoreCLR/compiler/host-specific correctness, collectibility, allocation, and PR-prerequisite guidance that should not load for ordinary managed-library changes.The broad C#, native, and test files explicitly state that PR-level gates are preparation guidance during authoring or local experimentation, not reasons to block exploratory work unless review is requested.
SKILL.mdretains the review process and points at these files for substantive guidance. Its area-agent discovery runs only when sub-agent tooling and matching agents actually exist; an instruction file does not imply an agent. The dispatched worker has no such tooling and explicitly skips agent discovery and multi-model fan-out.Testing: multi-iteration time-travel replay simulation
Because this changes how and when reviews are triggered — not just what they say — it was validated by replaying real PR event timelines (opening commit, subsequent commits, and human review feedback) against the candidate workflow tree in a disposable simulation repository, resetting all simulation PRs and state between rounds and starting the next round only from a clean, freshly deployed tree.
Note
This PR description was drafted with the assistance of GitHub Copilot.