Skip to content

Commit cf5b364

Browse files
T-GroCopilotnohwnd
authored
Add expert-reviewer agent with 16 review dimensions extracted from review history (#15775)
* Add expert-reviewer agent, skill, and folder instructions Extract review expertise from 5,313 comments across 1,903 PRs/issues into Copilot-compatible artifacts: - .github/agents/expert-reviewer.md: 16 review dimensions with 79 CHECK items, 10 overarching principles, 5-wave review workflow, and voice personality instructions - .github/skills/expert-review/SKILL.md: folder-to-dimension routing table mapping 15 code hotspots to their top review dimensions - .github/instructions/*.instructions.md: 8 folder-scoped instruction files for the highest-feedback areas (CrossPlatEngine, ObjectModel, CommunicationUtilities, BlameDataCollector, eng/, testhost, vstest console, acceptance tests) - .github/workflows/pr-expert-reviewer.md: updated to delegate deep analysis to @expert-reviewer, passing 5 supplemental generic dimensions (algorithmic correctness, performance, resources, security, defensive coding) that complement the agent's 16 vstest-specific ones Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Jakub Jareš <me@jakubjares.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Jareš <me@jakubjares.com>
1 parent b325a01 commit cf5b364

11 files changed

Lines changed: 682 additions & 52 deletions

‎.github/agents/expert-reviewer.md‎

Lines changed: 315 additions & 0 deletions
Large diffs are not rendered by default.

‎.github/instructions/acceptance-tests.instructions.md‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
applyTo: "test/Microsoft.TestPlatform.Acceptance.IntegrationTests/**"
3+
---
4+
5+
# Acceptance & Integration Tests Rules
6+
7+
These tests exercise vstest end-to-end across runners, TFMs, and platforms. They are slow and expensive — design them carefully.
8+
9+
## Test Coverage Design
10+
11+
- Each test should exercise unique behavior, not duplicate coverage from unit tests.
12+
- Keep matrices focused — move exhaustive compatibility coverage to specialized CI jobs.
13+
- New platform behavior must have an automated acceptance test so regressions are caught.
14+
- Performance claims must be backed by measurements separating platform overhead from adapter overhead.
15+
16+
## Cross-Platform & Cross-TFM
17+
18+
- Run supported scenarios on every target OS; explicitly validate Linux and macOS, not just Windows.
19+
- Avoid APIs and runtime assumptions unavailable on some targeted frameworks or runtimes.
20+
- Use OS-appropriate paths and focus on realistic scenarios rather than mocked impossible inputs.
21+
22+
## Parallel Test Safety
23+
24+
- Tests must not rely on timing, shared mutable state, or specific CPU availability.
25+
- Parallel infrastructure needs synchronization covering both build and post-build phases.
26+
- If tests share processes or state, make sharing decisions explicit and documented.
27+
28+
## Assertions & Diagnostics
29+
30+
- Assertions should verify specific behavior, not incidental output (e.g., don't assert on log line counts).
31+
- Include enough diagnostic context in test names and failure messages to identify the scenario.
32+
33+
## Key Checks
34+
35+
- New tests must pass in CI (`-c Release`) — not just locally with roll-forward enabled.
36+
- Don't add broad `*test*` globs that accidentally pull in infrastructure assemblies.

‎.github/instructions/blame-datacollector.instructions.md‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
applyTo: "src/Microsoft.TestPlatform.Extensions.BlameDataCollector/**"
3+
---
4+
5+
# Blame Data Collector — Dump Reliability Rules
6+
7+
This collector captures crash and hang dumps. It operates under extreme conditions (process dying, concurrent crashes) and must remain robust.
8+
9+
## Crash & Hang Dump Reliability
10+
11+
- Handle concurrent crash and hang scenarios without corruption — a crash during hang-dump is a real scenario.
12+
- Detach the dumper from the testhost process before testhost termination.
13+
- Session-end must not interrupt an in-progress dump write.
14+
- Use agent-managed temporary storage; ensure dump file paths are valid and writable on all platforms.
15+
16+
## Environment Variable Contracts
17+
18+
- Every env-var switch must have documented ownership, precedence, and side-effect contract.
19+
- Follow `VSTEST_DISABLE_*` (opt-out) or very rarely `VSTEST_OPTIN_*` (opt-in) naming conventions for new flags.
20+
- Variable propagation from console → testhost → child processes must be verified.
21+
22+
## Diagnostic Clarity
23+
24+
- When dump collection fails, report WHY — don't just swallow the error.
25+
- Collect dumps with a helper process matching the target process bitness (x86 vs x64).
26+
- Prefer narrowly scoped dump collection in shared CI environments.
27+
28+
## Key Checks
29+
30+
- Timeout configurations must have sensible defaults and be documented.
31+
- Validate on Windows and Linux — dump APIs differ significantly between platforms.
32+
- Confirm that blame changes don't kill slow-starting hosts or confuse early exits with connection timeouts.

‎.github/instructions/communicationutilities.instructions.md‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
applyTo: "src/Microsoft.TestPlatform.CommunicationUtilities/**"
3+
---
4+
5+
# CommunicationUtilities — IPC & Protocol Rules
6+
7+
This layer implements JSON-RPC communication between vstest.console and testhosts. Wire-format stability is paramount.
8+
9+
## Protocol Stability
10+
11+
- New protocol messages MUST be backward-compatible with older testhost/console versions.
12+
- Document every wire-protocol version change — silent regressions here break the entire ecosystem.
13+
- Never expose serializer-specific types (e.g., `JsonProperty` attributes) in public messaging contracts.
14+
- Serializer migrations must verify wire-format compatibility on all supported TFMs.
15+
16+
## Error Handling
17+
18+
- Connection timeouts and broken pipes must be handled without crashing the host process.
19+
- Protocol handlers must convert processing failures into deterministic error paths — never silently terminate.
20+
- Surface process id, exit state, and captured error output when child process connections fail.
21+
22+
## Dependency Management
23+
24+
- Serializer library changes require verifying the full transitive dependency set ships correctly.
25+
- Do not introduce new serializer dependencies without a migration plan from the existing one.
26+
- Version conflicts here propagate to every test project — validate against `eng/expected-*.json`.
27+
28+
## Key Checks
29+
30+
- Interface extensions must not break existing consumers — prefer new interfaces over method additions.
31+
- TranslationLayer methods must return or throw, never silently hang.
32+
- Test both directions: newer console → older host AND older console → newer host.

‎.github/instructions/crossplatengine.instructions.md‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
applyTo: "src/Microsoft.TestPlatform.CrossPlatEngine/**"
3+
---
4+
5+
# CrossPlatEngine — Execution Engine Rules
6+
7+
This is the core execution engine handling parallel scheduling, host management, and test orchestration. Thread safety and deterministic behavior are critical here.
8+
9+
## Parallel Execution & Scheduling
10+
11+
- Protect shared state accessed by parallel workers with minimal lock scopes matching actual shared-state boundaries.
12+
- Never rely on timing or CPU availability for correctness — design lock-free or explicitly synchronized paths.
13+
- When converting percentage-based parallelism to worker counts, round deterministically and clamp to valid bounds.
14+
- Verify cancellation tokens are respected and don't leave orphaned work items.
15+
16+
## Process Architecture & Host Resolution
17+
18+
- Architecture inference must handle all valid combinations (AnyCPU, x86, x64, ARM64).
19+
- For mixed-framework/mixed-architecture runs, centralize workload scheduling and preserve shared-host semantics.
20+
- Propagate `DOTNET_ROOT` and `DOTNET_ROOT_<ARCH>` correctly to child processes.
21+
22+
## Error Reporting
23+
24+
- Include stack traces in adapter/executor failure logs so exceptions are diagnosable without reproduction.
25+
- Log messages must identify component and method (e.g., `ProxyExecutionManager.StartTestRun: ...`).
26+
- Never swallow exceptions silently — trace at verbose level at minimum.
27+
28+
## Key Checks
29+
30+
- New behavior must have a disable flag for rollback.
31+
- Validate changes on all targeted TFMs — especially net462 where assembly loading differs.
32+
- IPC protocol changes from this layer must remain backward-compatible with older testhosts.

‎.github/instructions/eng.instructions.md‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
---
2+
applyTo: "eng/**"
3+
---
4+
5+
# Engineering Infrastructure Rules
6+
7+
Build scripts, packaging validation, and CI infrastructure. Changes here affect every contributor and every build.
8+
9+
## Build Script Hygiene
10+
11+
- Scripts must check exit codes of child processes and fail fast on errors.
12+
- Use portable OS checks — scripts should work on both Windows (PowerShell) and Unix (bash).
13+
- Remove dead code rather than carrying it forward; keep scripts canonical and current.
14+
- Don't break source-build mode with infrastructure changes.
15+
16+
## Package Integrity
17+
18+
- Validate that dependency version changes don't conflict with assemblies shipped to end users.
19+
- Packaging projects must ship assemblies from the current build, not stale artifacts.
20+
21+
## Dependency Management
22+
23+
- Version upgrades must state the exact package and target version in the PR description.
24+
- Transitive dependency versions must align between deps.json and actually-shipped assemblies.
25+
26+
## CI & Testing Infrastructure
27+
28+
- Add end-to-end tests around SDK and MSBuild insertion points to catch packaging regressions.
29+
- Cross-platform tests must use OS-appropriate paths and realistic scenarios.
30+
- Keep test matrices focused on unique behavior; move exhaustive coverage to specialized jobs.
31+
32+
## Key Checks
33+
34+
- Verify changes work in both `Debug` and `Release` configurations.
35+
- Changes to Arcade SDK versions require validating the full build pipeline.

‎.github/instructions/objectmodel.instructions.md‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
applyTo: "src/Microsoft.TestPlatform.ObjectModel/**"
3+
---
4+
5+
# ObjectModel — Public API Surface Rules
6+
7+
This assembly is shipped via NuGet and consumed by all test adapters. Every public change is a permanent commitment.
8+
9+
## Public API Protection
10+
11+
- New public types/members must be intentional, minimal, and declared in `PublicAPI.Unshipped.txt`.
12+
- Never change existing public API signatures without an `[Obsolete]` migration path.
13+
- Do not leak serializer-specific attributes or internal dependencies into public types.
14+
- Prefer new interfaces over adding methods to existing interfaces (external implementers exist).
15+
16+
## Binary Compatibility
17+
18+
- API changes must not break callers built against older ObjectModel versions.
19+
- When extending callbacks with framework-specific data, use expandable versioned contracts.
20+
- Bulk analyzer cleanups must exclude refactorings that alter public contracts or binary compatibility.
21+
22+
## Cross-TFM Considerations
23+
24+
- Ensure APIs work on all targeted TFMs (net462, net8.0+).
25+
- Use `#if` guards or polyfills for APIs unavailable on older frameworks.
26+
- netstandard2.0 dependencies must not pull in assemblies absent from net462 hosts.
27+
28+
## Key Checks
29+
30+
- Run `dotnet build` and verify no PublicAPI analyzer warnings before submitting.
31+
- Any signature change here affects the entire test adapter ecosystem — get explicit approval.
32+
- Validate nullable annotations are correct; this is a boundary assembly.

‎.github/instructions/testhost.instructions.md‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
applyTo: "src/testhost*/**"
3+
---
4+
5+
# Testhost — Assembly Loading & Host Resolution Rules
6+
7+
Testhost processes execute user tests. Assembly loading correctness and framework resolution are the primary concerns here.
8+
9+
## Assembly Loading & Resolution
10+
11+
- Keep `testhost.deps.json` dependency versions aligned with assemblies actually shipped in the CLI package.
12+
- Prefer bin-directory resolution over deps.json parsing for speed and correctness.
13+
- TypeLoadException from version mismatches must produce diagnostic output identifying the missing assembly.
14+
- Test deps.json edge cases: self-contained apps, single-file publish, RID-specific native assets.
15+
16+
## Framework & Architecture Resolution
17+
18+
- Use `COREHOST_TRACE` environment variables to inspect how the .NET host resolved the testhost.
19+
- When older projects fail host resolution under newer tooling, emit actionable guidance (install/update Microsoft.NET.Test.Sdk).
20+
- Architecture must match between testhost and test assemblies — validate on x86, x64, and ARM64.
21+
22+
## Cross-TFM Safety
23+
24+
- Changes must work on all targeted TFMs (net462, net8.0+).
25+
- net462 hosts lack binding redirects in DTA scenarios — test the DtaLikeHost pattern.
26+
- RID-specific native assets must resolve correctly on Windows, Linux, and macOS.
27+
28+
## Key Checks
29+
30+
- After dependency changes, verify `testhost.deps.json` in the built package matches shipped DLLs.
31+
- Provide explicit host-path escape hatches when architecture detection fails on new platforms.

‎.github/instructions/vstest-console.instructions.md‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
applyTo: "src/vstest.console/**"
3+
---
4+
5+
# vstest.console — CLI Entry Point Rules
6+
7+
This is the main entry point for `dotnet test` and `vstest.console.exe`. It handles argument parsing, RunSettings inference, and host process orchestration.
8+
9+
## RunSettings Validation & Inference
10+
11+
- Missing RunSettings nodes must fall back to documented defaults, not crash.
12+
- Invalid setting values must produce actionable errors, not silent misbehavior.
13+
- Multiple settings sources (CLI, runsettings file, project) must have clear, deterministic precedence.
14+
- Account for host-shell parsing differences — CLI fragments transit through dotnet and MSBuild.
15+
16+
## Architecture & Host Resolution
17+
18+
- Choose runner architecture from OS and target-framework compatibility, not the current host process alone.
19+
- Architecture-switch fallbacks must not assume a second SDK exists — locate compatible testhosts from shipped paths.
20+
- When inferring frameworks per source, maintain a compatibility path that reproduces old runsettings behavior.
21+
22+
## Environment Variable Contracts
23+
24+
- New env vars must follow `VSTEST_DISABLE_*` / rarely `VSTEST_OPTIN_*` naming.
25+
- Environment inheritance to child processes must be explicit and predictable.
26+
- Feature flags need a clear default and documented migration path before shipping.
27+
28+
## Key Checks
29+
30+
- Test with `-c Release` to match CI behavior (warnings-as-errors differ).
31+
- Validate on all platforms — path separators and process launching differ between Windows and Unix.

‎.github/skills/expert-review/SKILL.md‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
---
2+
name: expert-reviewing
3+
description: "Route code changes to relevant review dimensions based on affected files and folders. Use when reviewing PRs, analyzing code quality, or performing targeted reviews of specific vstest subsystems."
4+
---
5+
6+
# Expert Review Routing
7+
8+
This skill maps changed folders to review dimensions, enabling focused expert reviews of vstest PRs.
9+
10+
## Folder → Dimension Routing Table
11+
12+
| Folder | Primary Dimensions |
13+
|--------|-------------------|
14+
| `eng/` | Build Script & Infrastructure Hygiene, Dependency & Package Integrity, Source Build & Cross-Platform Compliance |
15+
| `src/Microsoft.TestPlatform.CrossPlatEngine/` | Parallel Execution & Scheduling Safety, Error Reporting & Diagnostic Clarity, Process Architecture & Host Resolution |
16+
| `src/vstest.console/` | RunSettings Validation & Inference, Process Architecture & Host Resolution, Environment Variable & Feature Flag Contracts |
17+
| `test/Microsoft.TestPlatform.Acceptance.IntegrationTests/` | Acceptance Test Coverage Design, Cross-TFM & Framework Resolution, Parallel Execution & Scheduling Safety |
18+
| `src/Microsoft.TestPlatform.Extensions.BlameDataCollector/` | Crash & Hang Dump Reliability, Error Reporting & Diagnostic Clarity, Environment Variable & Feature Flag Contracts |
19+
| `src/testhost/` | Testhost Assembly Loading & Resolution, Cross-TFM & Framework Resolution, Process Architecture & Host Resolution |
20+
| `src/Microsoft.TestPlatform.CommunicationUtilities/` | IPC Transport & Protocol Stability, Error Reporting & Diagnostic Clarity, Dependency & Package Integrity |
21+
| `src/Microsoft.TestPlatform.ObjectModel/` | Public API Surface Protection, Backward Compatibility & Rollback Safety, Cross-TFM & Framework Resolution |
22+
| `src/package/` | Dependency & Package Integrity, Build Script & Infrastructure Hygiene, Cross-TFM & Framework Resolution |
23+
| `src/Microsoft.TestPlatform.Common/` | Backward Compatibility & Rollback Safety, Error Reporting & Diagnostic Clarity, Null Safety & Boundary Validation |
24+
| `src/Microsoft.TestPlatform.VsTestConsole.TranslationLayer/` | IPC Transport & Protocol Stability, Error Reporting & Diagnostic Clarity, Backward Compatibility & Rollback Safety |
25+
| `src/Microsoft.TestPlatform.Client/` | IPC Transport & Protocol Stability, Backward Compatibility & Rollback Safety, Public API Surface Protection |
26+
| `src/datacollector/` | Crash & Hang Dump Reliability, Dependency & Package Integrity, Error Reporting & Diagnostic Clarity |
27+
| `src/Microsoft.TestPlatform.CoreUtilities/` | Process Architecture & Host Resolution, Environment Variable & Feature Flag Contracts, Error Reporting & Diagnostic Clarity |
28+
| `src/Microsoft.TestPlatform.TestHostProvider/` | Process Architecture & Host Resolution, Environment Variable & Feature Flag Contracts, Testhost Assembly Loading & Resolution |
29+
30+
## Quick Reference: Dimension Summaries
31+
32+
| Dimension | Focus |
33+
|-----------|-------|
34+
| Dependency & Package Integrity | Version conflicts, transitive deps, package content correctness |
35+
| Cross-TFM & Framework Resolution | Multi-TFM builds, binding redirects, framework-specific behavior |
36+
| Process Architecture & Host Resolution | x86/x64/ARM64 selection, DOTNET_ROOT propagation, muxer resolution |
37+
| Parallel Execution & Scheduling Safety | Race conditions, lock scopes, worker slot assignment, cancellation |
38+
| IPC Transport & Protocol Stability | Wire compatibility, connection timeouts, protocol versioning |
39+
| Crash & Hang Dump Reliability | Dump capture under compound failures, detach timing, file validity |
40+
| Error Reporting & Diagnostic Clarity | Actionable messages, trace formatting, exception propagation |
41+
| Environment Variable & Feature Flag Contracts | Naming conventions, propagation, stable contract semantics |
42+
| RunSettings Validation & Inference | Defensive XML parsing, defaults, source precedence |
43+
| Backward Compatibility & Rollback Safety | Disable flags, version-guarded behavior, migration paths |
44+
| Public API Surface Protection | Unintentional exposure, PublicAPI.txt management, interface evolution |
45+
| Acceptance Test Coverage Design | Focused matrices, OS coverage, measurement-backed claims |
46+
| Testhost Assembly Loading & Resolution | deps.json alignment, RID assets, TypeLoadException handling |
47+
| Build Script & Infrastructure Hygiene | Exit codes, cross-platform scripts, source-build compatibility |
48+
| Null Safety & Boundary Validation | Boundary checks, nullable annotations, defensive parsing |
49+
| Source Build & Cross-Platform Compliance | Source-build mode, Linux/macOS paths, shell compatibility |
50+
51+
## Common Review Scenarios
52+
53+
**PR touches `src/Microsoft.TestPlatform.CrossPlatEngine/`**
54+
→ Activate: Parallel Execution & Scheduling Safety, Error Reporting & Diagnostic Clarity, Process Architecture & Host Resolution
55+
56+
**PR modifies `eng/` scripts or build infrastructure**
57+
→ Activate: Build Script & Infrastructure Hygiene, Dependency & Package Integrity, Source Build & Cross-Platform Compliance
58+
59+
**PR changes `ObjectModel` public API**
60+
→ Activate: Public API Surface Protection, Backward Compatibility & Rollback Safety, Cross-TFM & Framework Resolution
61+
62+
**PR updates `CommunicationUtilities` or `TranslationLayer`**
63+
→ Activate: IPC Transport & Protocol Stability, Backward Compatibility & Rollback Safety, Error Reporting & Diagnostic Clarity
64+
65+
**PR modifies blame/datacollector components**
66+
→ Activate: Crash & Hang Dump Reliability, Environment Variable & Feature Flag Contracts, Error Reporting & Diagnostic Clarity
67+
68+
**PR adds or bumps package dependencies**
69+
→ Activate: Dependency & Package Integrity, Cross-TFM & Framework Resolution, Backward Compatibility & Rollback Safety
70+
71+
**PR touches testhost or TestHostProvider**
72+
→ Activate: Testhost Assembly Loading & Resolution, Process Architecture & Host Resolution, Cross-TFM & Framework Resolution
73+
74+
**PR modifies RunSettings handling in vstest.console**
75+
→ Activate: RunSettings Validation & Inference, Environment Variable & Feature Flag Contracts, Backward Compatibility & Rollback Safety
76+
77+
## Integration with @expert-reviewer
78+
79+
This skill provides the **routing configuration** that tells the expert-reviewer agent which dimensions to activate for a given PR. The agent owns the full CHECK methodology and review logic.
80+
81+
**Invocation:** Reference `@expert-reviewer` in a PR comment or use the `pr-expert-reviewer` workflow.
82+
83+
**How routing works:**
84+
1. Agent identifies changed files/folders in the PR
85+
2. This skill's routing table maps folders to applicable dimensions
86+
3. Agent activates the matched dimensions and applies their CHECK items
87+
4. Agent produces findings limited to activated dimensions only
88+
89+
**Scope boundaries:**
90+
- This skill: folder-to-dimension mapping, dimension summaries, scenario examples
91+
- The agent: full CHECK items, severity classification, comment formatting, review decisions
92+
- `vstest-build-test` skill: build/test commands (not review logic)
93+
- `trx-analysis` skill: test result file parsing (not code review)

0 commit comments

Comments
 (0)