Make PR review routing data-driven and harden review preparation

[PureWeen]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Make PR review routing data-driven and harden review preparation

Description

Follow-up to #69502. The producer and hosted workflow changes make bundle preparation and publication more robust, and replace hard-coded guide routing with a trusted routing table. The SKILL.md changes clarify how workers read bundled guidance and how many workers run, allow an optional local-only repro, and shorten findings. Review criteria in the guides are unchanged.

Bundle producer (scripts/prepare-review.cs)

• Bounded process output: stdout and stderr are now counted while the child process runs. Once either passes the existing 64 MiB limit, the producer kills the whole process tree and fails with the existing BLOCKED error. Exit-code and stderr reporting are unchanged. Previously both streams were read fully into memory before the size check.
• --check re-freeze: after validating a prepared bundle, --check re-freezes the target and fails closed if the PR head or base moved during validation. It now does what the normal preparation path already does before writing its manifest.
• One bounded retry: when the target or base branch moves during preparation, the producer deletes its partial output and rebuilds from scratch, exactly once. A second move, or any other failure, still returns BLOCKED. --check never retries.
• Data-driven routing: a new .github/skills/review-pull-request/routing.md table (| Changed path prefix | Guide |) replaces the hard-coded guide list and Components path regex. Preparation and --check share one strict parser: * applies to every PR, other prefixes end in / and match at a directory boundary on both filename and previous_filename, several rows may match, and guides are de-duplicated. The table is read from the trusted guidance snapshot, never the PR head, and its path and SHA-256 are recorded in the manifest. A missing, empty, or malformed table, a missing * row, a duplicate row, or a missing/invalid guide returns BLOCKED. Today's two routes are unchanged. Adding an area is one row plus one guide file.
• Manifest version stays 2: the routing object is additive, and updated consumers require it, so stale bundles fail closed.

The output-limit and --check re-freeze changes address the Copilot review comments on #69502: bounding output while reading, and re-checking identity after bundle validation.

Skill (SKILL.md)

• When a bundle read fails with Copilot CLI's generic Permission denied and could not request permission from user, the reviewer tells the user to rerun interactively or with --allow-all-paths. It doesn't claim the error proves a long-path cause.
• Guidance file paths: guides, policies, and context documents under guidance.root use the manifest's .source suffix, matching how the producer writes them while retaining unsuffixed logical manifest paths.
• Single worker per guide: each routed guide gets exactly one fresh worker. A coordinator may correct or clarify only with that same worker; it must record the guide as incomplete rather than relaunching or replacing the worker.
• Routing: the skill consumes manifest.routing and guides[] instead of naming the two guides.
• Optional local repro: a native local coordinator may create a temporary detached worktree at the frozen head, write and run a minimal test there, and then remove it. A failing repro confirms a candidate; a passing one is evidence against it, not an automatic discard. If the worktree, build, or test is unavailable, the review stays source-only. Workers and hosted runs remain source-only.
• Shorter findings: each NEW_FINDINGS entry is a one-line claim, file:line, severity, a minimal consumer repro, at most two lines of impact, and a fix snippet when possible.

The .source and single-worker fixes address defects that invalidated local evaluation runs: workers reading guidance without the .source suffix, and a coordinator relaunching a worker.

Hosted workflow (pull-request-review.md / .lock.yml)

• Live-head re-check inside publication: gh-aw v0.89.21's supported jobs.safe_outputs.pre-steps hook re-reads the live PR inside the publishing job, before agent output is downloaded or processed. Publication is blocked if the head moved, the PR closed, or the base repository changed. The existing verify_live_head job stays as an earlier fail-fast gate. The read still isn't atomic with the API writes; the trusted commit-id keeps review attribution pinned to the reviewed SHA.

• Visible BLOCKED/INCOMPLETE status: one capped, PR-only add-comment safe output posts:

  Review not published (<STATUS>): <reason>
  
  No partial findings were published.
  

  The reason is the same single-line reason (at most 240 characters) given to report_incomplete. The trusted preflight rejects a status comment combined with findings, a review, or noop; a malformed one; or one whose reason doesn't match. Status is only reported for failures inside the agent; a bundle-preparation failure still ends the run before the agent starts.

• Inline comment format: hosted inline comments use the same short finding shape as the skill. Hosted runs still never execute target code.

Validation

• git diff --check: passed.
• dotnet run --project .github/skills/review-pull-request/scripts/PrepareReview.Tests.csproj at 3d478449d8 (Windows): 58/58 passed (Total: 58, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0). The two Unix-only process tests return early on Windows; they passed on macOS in the earlier 44/44 run at 101fccea8c and are unchanged.
• Routing red/green: restoring only prepare-review.cs from 101fccea8c failed exactly the 11 routing tests (all 8 MalformedRoutingTableReturnsBlocked rows, PreparesDistinctCompleteSidesInertTargetInstructionsLargeFilesAndDirtyGuidance, ReleaseBaseUsesRoutingFromTrustedGuidanceSnapshot, and the hash row of CheckRejectsAChangedOrTamperedRoutingTable); restoring the implementation returned 58/58.
• Routing parity on real PRs: prepare-review and --check were ready for [release/11.0] Avoid sending JSInterop calls when component is disposed #69522 (release/11.0), Prevent multiple browser file uploads from hanging #69505 (Components), and Fix BindNever for record primary constructor parameters #69585 (MVC only). The manifest guides, policies, context, skippedLinks, and exclusions were byte-identical to the origin/main producer for all three. Routed guides were cross-cutting plus Blazor for [release/11.0] Avoid sending JSInterop calls when component is disposed #69522/Prevent multiple browser file uploads from hanging #69505 and cross-cutting only for Fix BindNever for record primary constructor parameters #69585.
• Earlier red/green: with each fix temporarily removed, its new tests failed for the intended reason:
  • the --check re-freeze tests and the retry tests (CheckRejectsATargetThatMovesDuringValidation, RetriesOneMovedTargetFromScratch, BlocksWhenTheTargetMovesTwice) failed: 3 tests;
  • the output-limit test (KillsAChildWhenProcessOutputExceedsTheLimit) failed: the child kept running for about 5 s instead of being killed.
• Workflow compile: gh-aw v0.89.21 compile pull-request-review --strict passed twice with byte-identical locks (SHA-256 a5cdd59d0066b77ad20f7d497a2e7c4382d4a3bb764179295481b1ad588f4036). .github/aw/actions-lock.json is unchanged.
• Local native check of the two SKILL.md fixes (Windows, four runs on Order root component operations without serializing circuit batches #69484, Preserve modified state when InputBase parsing fails #68365, [Blazor] Preserve null values for <option value="@null"> in single select binding #67616, and docs(azure): fix placeholder title in README.md #65504): no unsuffixed guidance reads, and exactly one worker per routed guide with no relaunch or replacement, verified from raw session events. These runs predate the routing, local-repro, and finding-format changes and used an earlier head that also contained evidence-contract wording, which has since been removed from this PR. One run correctly ended INCOMPLETE after a worker misjudged a recovered path probe; it was reported as incomplete, not as a clean review.
• Hosted, on a fork only: two runs of the producer and workflow changes at 9be41d75dd, before the routing commit. Both were cleaned up afterwards.
  • Normal path (run 36919666671): every job succeeded, including the new in-job head re-check. Exactly one COMMENT review was posted, pinned to the frozen head, with the planted finding. There was no status comment or incomplete signal.
  • Forced INCOMPLETE (run 36916516447): verify_live_head, detection, and safe_outputs succeeded. Exactly one neutral status comment was posted, with no review or inline findings. The run concluded failure as designed, because report_incomplete fails closed.
• Not yet run at this head: a native review or hosted run exercising the new routing table, local repro, or finding format end to end.

No product code changes.

Follow-up to #69502

[Copilot]

Copilot review overview

🟡 Changes recommended

The publication preflight accepts an empty output set, allowing blocked or incomplete reviews to remain invisible.

Review effort: Balanced
Findings: 1

Open (1)

• Empty output.items bypasses validation and publishes no status · New

What changed in this PR

Hardens the automated PR-review pipeline without changing product code.

Changes:

• Bounds producer output and retries once on moved targets.
• Clarifies guidance paths and worker reuse.
• Revalidates PR identity and reports incomplete reviews visibly.

File	Description
.github/​workflows/​pull-request-review.md	Adds publication checks and status comments.
.github/​workflows/​pull-request-review.lock.yml	Regenerates the compiled workflow.
.github/​skills/​review-pull-request/​tests/​PrepareReviewTests.cs	Tests output limits and target movement.
.github/​skills/​review-pull-request/​SKILL.md	Clarifies bundle reading and worker handling.
.github/​skills/​review-pull-request/​scripts/​prepare-review.cs	Implements bounded output, revalidation, and retry.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

The publication gate permits empty terminal output, the finding format excludes tooling repros, and process-limit coverage remains incomplete.

Review effort: Balanced
Findings: 1 · 2

Open (3)

• Empty output.items bypasses validation and publishes no status
• Allow tool commands or workflow inputs as minimal reproductions · New
• Permit tooling and workflow inputs as minimal reproductions · New

Previously missed (2)

In code that hasn't changed since last review

Test oversized stderr as well as stdout for process termination

.github/​skills/​review-pull-request/​scripts/​prepare-review.cs:186

The new limit is implemented independently for stderr, but the added process test only writes oversized stdout. A regression in this StreamReader/UTF-8 byte-counting branch would therefore pass the suite even though the PR promises that either stream triggers termination. Parameterize the process test to emit the oversized payload to stdout and to stderr.

Replace scheduler-dependent timeout assertion with child marker check

.github/​skills/​review-pull-request/​tests/​PrepareReviewTests.cs:558

The two-second wall-clock assertion can fail on a loaded test agent even when the process tree is killed correctly, contrary to the repository's deterministic-test guidance (docs/CrossCuttingGuidance.md:123). Let a nested child create a marker after the delay and assert that it never does; because the child inherits the output pipe, this still detects failure to kill the whole tree without a scheduler-dependent threshold.