Uh oh!
There was an error while loading. Please reload this page.
Add parallel-safety audit agentic workflow for MSTest test suites (⚠️ requires lock generation on aligned toolchain) - #10252
Conversation
Adds a gh-aw agentic workflow that audits changed MSTest tests for parallel-safety: process-global state mutation, shared filesystem paths, [ResourceLock]/[DoNotParallelize] declaration reconciliation, and over-serialization. Scope-aware (off/ClassLevel/MethodLevel) and confidence-tagged. Two consumers (PR trigger + /parallel-audit slash command) share one imported body written to lift verbatim into an upstream dotnet-test SKILL.md. Generated .lock.yml + agentic_commands.yml routing intentionally deferred to PR-prep: local gh-aw v0.83.1 injects actions/checkout v7.0.0 into framework steps, conflicting with the repo's aligned v7.0.1 pin (PR #10174); regenerating now would sweep ~30 unrelated lock files. Will regenerate with the aligned toolchain when opening the PR. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
Incorporates the sibling parallel-safety-analyzers session's verified findings: - Culture: rewrite the three-form rule with ExecutionContext-flow wording (not AsyncLocal); make the safe third form (CultureInfo.CurrentCulture) an explicit do-not-flag with rationale; cite MSTEST0076 for the two flagged forms. - Category B division of labour: MSTEST0077 fires only on a literal passed directly to a mutating File.*/Directory.* API (narrowed after 8/8 false positives on pure-value path construction). Everything else - path construction with real colliding I/O, traced paths, fixture fields, config - is the audit's job. Require evidence of colliding I/O; make the 'constructed AND written to' claim visible. - Cross-ref MSTEST0074 (undeclared env/console) and MSTEST0075 (CurrentDirectory) in category A; reframe category-C under-declaration around what the analyzers miss. - Step 0: add the analyzer-coverage gate - MSTEST0074-0077 run only under [assembly: Parallelize] or editorconfig mstest_parallel_safety_mode=always, NOT runsettings/MSBuild-only opt-in - and surface it in the report header banner. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
There was a problem hiding this comment.
Pull request overview
Adds an agentic workflow that audits MSTest suites for parallel-safety.
Changes:
- Adds automatic and
/parallel-auditworkflows. - Introduces shared audit logic and generated workflow locks.
- Registers and documents the slash command.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/shared/parallel-safety-audit-shared.md | Defines extraction, analysis, and reporting logic. |
.github/workflows/parallel-safety-audit.md | Configures automatic PR audits. |
.github/workflows/parallel-safety-audit.lock.yml | Compiled automatic workflow. |
.github/workflows/parallel-safety-audit-command.md | Configures the slash command. |
.github/workflows/parallel-safety-audit-command.lock.yml | Compiled command workflow. |
.github/workflows/agentic_commands.yml | Registers command routing. |
.github/workflows/README.md | Documents the workflows. |
Review details
- Files reviewed: 5/7 changed files
- Comments generated: 10
- Review effort level: Medium
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…trigger, future-gate analyzers, TFM-aware culture Resolve the reviewer comments on the parallel-safety audit sources. No generated files are recompiled here: the local gh-aw v0.83.1 build corrupts two pinned actions on compile (downgrades actions/checkout v7.0.1->v7.0.0 and unpins the setup action from its SHA to a mutable @v0.83.1 tag), so the committed .lock.yml files and the agentic_commands.yml router regen are removed/reverted and lock generation is deferred to the aligned CI toolchain (github/gh-aw-actions/setup @ 8bdba80... in agentics-maintenance.yml). Shipping sources-only keeps the two security-relevant pins intact; the workflow is inert until the lock is generated on CI, which is called out as blocking in the PR. Source fixes: - Dispatch resolution: the centralized /parallel-audit command runs as workflow_dispatch, which the resolver's issue_comment-only case rejected with exit 1. Merge issue_comment|workflow_dispatch and resolve the PR number from aw_context.item_number on dispatch, github.event.issue.number otherwise. - Concurrency: github.event.issue.number is empty on workflow_dispatch, so every dispatched run collapsed into one group and cancelled unrelated PRs' audits. Fall back to aw_context.item_number in the group key. - Changed-line ranges: git diff --name-only scopes to files, not methods, so a one-line edit flagged every legacy method in the file. Emit HEAD-side hunk ranges (test_regions_path) and instruct the agent to treat a call site outside every changed range as pre-existing context, not a PR-introduced finding. - Trigger scope: gh-aw ORs paths, so listing src/** fired a full audit on every source-only PR with no tests changed. Filter test/** only; PRs touching both still match and still receive the changed-src/ list. - Analyzer gating: only MSTEST0073 ships on main; MSTEST0074-0077 are in flight on dev/amauryleve/parallel-safety-analyzers. Future-gate every reference to them and stop describing them as active coverage. - MSTestParallelizeScope coverage: Parallelize.targets emits a real [assembly: Parallelize(...)] via WriteCodeFragment, so the MSBuild-property opt-in IS analyzer-visible. Only a .runsettings-only opt-in is uncovered; correct the header/banner wording accordingly. - Class-level over-serialization (category D): only over-serializes under MethodLevel. Under the ClassLevel default the chunk is the whole class and its locks are unioned, so a class-level lock costs nothing extra. Gate the finding to MethodLevel and cite ResourceLockAttribute remarks. - Read-set coverage gap (category C): the changed-src list is a hint, not a boundary. Production code a test exercises may live in an unchanged file, so follow the call graph from the test into the production methods it invokes. - Culture: make TFM-aware. On all testfx targets (.NET Core 5+/.NET FW >=4.6) Thread.CurrentThread.CurrentCulture and CultureInfo.CurrentCulture share ExecutionContext-flowed storage and do not corrupt siblings/successors -- do not flag as a live race; the pooled-thread forward-leak only applies on .NET FW <4.6. DefaultThreadCurrent[UI]Culture stays flagged (process-wide, all TFMs). Note the audit refines the forthcoming MSTEST0076's TFM-agnostic call. - README catalog: correct the trigger scope and analyzer citation to match. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
a54f997 to
e342682CompareThere was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
.github/workflows/shared/parallel-safety-audit-shared.md:277
- This precedence rule conflates runsettings with MSBuild generation. Runsettings override the assembly
[Parallelize]workers/scope (TestExecutionManager.Parallelization.cs:135-145), but the MSBuild properties emit another assembly attribute rather than overriding a source attribute, and assembly-level[DoNotParallelize]independently keepsCanParallelizeAssemblyfalse. As written, the auditor can report parallelism as enabled when it is actually off or when declarations conflict.
2. **Which scope?** `[Parallelize]`'s default `Scope` is **`ClassLevel`**, not
`MethodLevel`. Record the effective scope: `off`, `ClassLevel`, or
`MethodLevel`. Runsettings / MSBuild overrides win over the attribute.
.github/workflows/shared/parallel-safety-audit-shared.md:428
- Two tests reading the same stable file are not a filesystem collision. Requiring merely that a constructed path is “read from ... by more than one test” will flag shared read-only fixtures and recommend unnecessary isolation/locking. Require convergent paths with at least one mutating operation (write/delete/move/replace), while treating concurrent readers as safe unless mutation is also possible.
- `Path.Combine(Path.GetTempPath(), <constant>)` and other **path construction**
— the classic collision, *but only if the constructed path is then read from
or written to by more than one test.* Trace the value: if it is only hashed,
compared, returned from a mock, or used as a sentinel that is never opened, it
is **not a finding.** Require evidence of **actual colliding I/O** before
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Medium
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…y audit Assembly-level [DoNotParallelize] is not a deferred subset. It sets CanParallelizeAssembly = false (TestAssemblySettingsProvider.cs:55), so the line-147 gate in TestExecutionManager.Parallelization.cs routes the entire source down the sequential else branch (281) without ever building a parallel/non-parallel partition. The shared body previously said "(assembly or method)" both mean mutual exclusion and deferral, which would misclassify an opted-out assembly as category-D serial-tail work instead of effective scope off. - Step 0 now records [assembly: DoNotParallelize] as scope off. - The semantics section splits class/method-level (deferral) from assembly-level (scope off). - Category D scopes the deferral finding to class/method level. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
.github/workflows/shared/parallel-safety-audit-shared.md:282
- An explicit worker count of 1 cannot execute two chunks concurrently: the adapter creates exactly
parallelWorkerstasks (TestExecutionManager.Parallelization.cs:216-220). The current severity rules still classifyMethodLevel, Workers = 1conflicts as High/Critical and report over-serialization, producing false live-risk and throughput findings. Treat one worker as readiness-only while retaining the configured scope for context.
3. **Workers.** `[Parallelize(Workers = 0)]` means "one worker per logical
processor"; a positive N pins the count. Record N (or "CPU count") — you
need it for the speedup arithmetic in category D.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Medium
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…udget Addresses three review findings on PR #10252. TestContainer tests were never collected. The Step 1 filter required a [TestMethod]-derived attribute, but the ForTestingMSTest engine recognizes unattributed public parameterless void/Task methods (TestContainer.cs:7-9, TestFrameworkEngine.cs:73-76). The showcased ExecutableConditionAttributeTests readiness-only case would therefore have fallen through to "nothing to audit". Step 0 now detects the engine first and records a TestContainer suite as scope off / readiness-only, and Step 1 collects its unattributed test methods. The shared body's header comment still advertised a test/** or src/** trigger after the trigger was narrowed to test/** only. The workflow shares grade-tests-on-pr's high-frequency test/** trigger set but left the daily AI-credit budget at the enterprise default of 5K, which that workflow documented crossing on a busy weekend (#9086, #9053). Adds the established max-daily-ai-credits: 20K override. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
… gate Acts on the three low-confidence comments the reviewer suppressed. All three were verified against the adapter source and all three prevent false findings. Scope precedence conflated two different mechanisms. A .runsettings value is a genuine runtime override (TestExecutionManager.Parallelization.cs:141-145), but the MSBuild properties generate the [assembly: Parallelize] attribute at build time via WriteCodeFragment rather than overriding it, so they are the attribute's source and not something that beats it. Workers = 1 cannot execute two chunks concurrently: the adapter creates exactly parallelWorkers tasks (Parallelization.cs:216-220). Such a suite is now readiness-only with no category-D over-serialization, since there is no throughput to recover. Category B required only that a constructed path be "read from or written to by more than one test", which would flag shared read-only fixtures. It now requires convergent paths plus at least one mutating operation, with concurrent readers called out as safe. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Resource locks only coordinate within a single test source. Tests are grouped by AssemblyName and executed one group at a time (TestExecutionManager.Parallelization.cs:33-46), and a fresh ResourceLockManager is constructed per source (:214). The reconciliation step told the auditor to grep the whole test/ tree for shared keys, which compares declarations that can never coordinate and would report cross-project near-misses as findings. Key comparison is now scoped to the owning test project, with a note that a matching key across assemblies is not protection, so a genuine cross-assembly collision needs isolation or external coordination instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
.github/workflows/shared/parallel-safety-audit-shared.md:242
- The delta rule is incorrect when the PR changes scheduling itself. Adding
[assembly: Parallelize]or switching toMethodLevelmakes unchanged test methods newly concurrent, yet all of their hazards are capped at Info here (and an assembly-attribute-only edit may reach the fallback). Treat a parallelization opt-in/scope change as widening the primary audit surface to the affected assembly; keep this narrow rule for ordinary test edits.
modified in each changed test file. A finding is a **primary** finding only when
the unsafe call site falls inside one of these ranges (or inside a test method
whose body the PR touched). An unsafe call site that lies **outside** every
changed range is *pre-existing*: report it at most as **context / Info** ("this
file already contains …"), never as though the PR introduced it. A one-line edit
must not light up every legacy method in the file.
.github/workflows/shared/parallel-safety-audit-shared.md:436
- This fix is not valid for every resource listed above.
[ResourceLock]coordinates only inside the current test-host process (ResourceLockAttribute.cs:22-24), while registry state is shared across processes; serializing one assembly therefore does not protect concurrent test hosts. Separate process-local state from OS-global resources and recommend per-test resource names, process isolation, an interprocess lock, or removal for the latter.
MSTEST0076's blanket call with a TFM judgement.
- `Console.SetOut` / `SetError` / `SetIn`; `Console.OutputEncoding` /
`ForegroundColor` / other console state. (The `Console.Set*` writers will also
be covered by **MSTEST0074** when undeclared, once that analyzer ships — see the
analyzer-gate note; it is not on `main` yet.)
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Medium
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…gger docs Four review findings. The attribute filter matched the attribute NAME, so it missed legal spellings that still opt in: a fully qualified [assembly: Microsoft.VisualStudio.TestTools.UnitTesting.Parallelize(...)], a using-alias, or a line break after "assembly:". Any of those would enable parallelism while leaving CONFIG_OUT empty and skipping the whole-assembly audit. The filter now matches the attribute TARGET instead, which cannot express those variations, and the prompt gained an explicit first step: confirm the attribute really is a parallelization attribute before anything else, since files carrying only InternalsVisibleTo or CLSCompliant now appear as candidates. Verified the pattern against simple, fully qualified, Attribute-suffixed, spaced and line-wrapped forms, and that it rejects method attributes and plain code. The category-D ranking used s > 1/N as a break-even test and told the agent never to recommend splitting a lock covering ~2% of runtime. That is not a break-even rule: it describes when the 1/s ceiling binds, not whether reducing s pays. With 8 workers and s = 0.10 -- below 1/8 -- Amdahl gives 1/(0.10 + 0.90/8) = 4.7 against a possible 8, so the old rule suppressed a 70% gain. The gain also grows with N: s = 0.02 is worth ~14% at N = 8 but ~60% at N = 32, so no fixed cutoff is valid. Findings are now ranked by estimated gain, with the arithmetic stated. The workflow description and the README catalog row both still claimed the trigger was test/** only, after the repository-root MSBuild files were added to the path filter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
.github/workflows/shared/parallel-safety-audit-shared.md:505
- This entry-point filter excludes changes confined to lifecycle/fixture code or class-level declarations. For example, adding
Environment.SetEnvironmentVariableto[TestInitialize], or changing a class-level[ResourceLock], enters the changed-range file but is never audited unless a test body also changed. Start from every changed test-relevant member/declaration, then trace its affected tests.
For each changed test method — either decorated with `[TestMethod]`,
`[DataTestMethod]`, or any attribute deriving from them (testfx has
`[STATestMethod]`, `[UITestMethod]`, `[IterativeTestMethod]`, project-local
`MyTestMethodAttribute : TestMethodAttribute`, etc.), **or**, in a
`TestContainer`-derived class, any public parameterless `void`/`Task` method
.github/workflows/shared/parallel-safety-audit-shared.md:760
- The PR description still says splitting pays only when
s > 1/Nand that a 2% lock should be ignored, while this corrected logic explicitly rejects that threshold and shows it can matter at higher worker counts. Update the PR description to match the implemented Amdahl-based ranking so reviewers are not promised the opposite behavior.
`N = 8` but ~60% at `N = 32`. In particular do **not** use `s > 1/N` as a
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Medium
Uh oh!
There was an error while loading. Please reload this page.
…t test bodies Step 1 entered the taxonomy only from changed test methods, and the fallback condition mirrored that. A PR that touches only a [TestInitialize], a constructor, a fixture field, or a class-level [ResourceLock] therefore reached "nothing to audit" while genuinely changing parallel-safety behaviour. Both are now widened together, since leaving either behind would have them contradict each other. Step 1 enumerates test methods, lifecycle members (TestInitialize/Cleanup, ClassInitialize/Cleanup, AssemblyInitialize/Cleanup, and the TestContainer constructor and Dispose), class- and assembly-level ResourceLock/DoNotParallelize/Parallelize declarations, and fixture state. The fallback lists the same set before concluding there is nothing to report. Lifecycle members also carry a blast-radius note: a mutation added to a [TestInitialize] runs before every test in the class, so the finding is attributed to the class or assembly rather than to a single method. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
.github/workflows/shared/parallel-safety-audit-shared.md:773
- This formula treats the lock under review as the suite's only serialized work. If other locks, fixture setup, or deferred tests contribute a residual serial fraction, eliminating this lock does not raise speedup to
N, so the reported “expected gain” and ranking are overstated. Account for the residual serial fraction, or explicitly label the comparison withNas an upper bound.
- **Rank category D by speedup arithmetic.** Estimate `s`, the fraction of total
suite time the lock serializes, then compare what the suite can reach today,
`1 / (s + (1 - s)/N)`, against `N` if `s` were eliminated. Rank findings by that
**gain** and state it. There is **no fixed cutoff** below which a lock is safe
.github/workflows/shared/parallel-safety-audit-shared.md:807
- Restricting this table to assemblies that have findings leaves a successful no-findings audit with no scope row, even though the rules below require the detected scope to be reported. Include every audited assembly so the report remains scope-aware when the finding count is zero or a configuration change is found safe.
**Parallelization** — one row per test assembly the findings touch (Step 0 resolves this per assembly, so never collapse several projects into one state):
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Medium
…udits Two low-confidence review comments, both correct. The category-D gain compared today's speedup against N, which silently assumes the lock under review is the suite's only serialized work. Any residual serial fraction that survives the fix -- other locks, a [DoNotParallelize] tail, class or assembly fixture setup -- caps the result at 1/(s_r + (1 - s_r)/N) instead, so the stated gain was overstated. N is now labelled an upper bound rather than a prediction, with instructions to quote a range where the residual work is visible and to say "upper bound" where it is not. The report header restricted the per-assembly table to assemblies the findings touch, so a clean audit produced no scope row at all while a rule three sections later requires a no-findings result to state the detected scope. The table is now one row per audited assembly. The short "nothing to audit" fallback had the same hole and gained an optional scope line, omitted only when no test assembly was in scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
Uh oh!
There was an error while loading. Please reload this page.
The range extractor skipped hunks whose HEAD side has no lines (if count == 0 next), so a deletion-only change produced no region at all. Removing a [ResourceLock], a [DoNotParallelize], a finally block that restored an environment variable or culture, or a cleanup that deleted a shared file introduces the hazard just as surely as adding a mutation, yet such a regression would have been downgraded to pre-existing context or missed entirely, since the prompt treats only emitted ranges as primary. Deletion-only hunks now emit a single-line anchor at the preceding new-side line, clamped to 1 so a deletion at the start of a file does not produce 0-0. Verified against modify, deletion-only, start-of-file deletion, pure addition and single-line no-comma hunk headers. The prompt explains the anchors and instructs the agent to run git diff and read the removed side, reporting a removed guard as a primary finding attributable to the PR. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
.github/workflows/shared/parallel-safety-audit-shared.md:625
- The restoration guidance needs a special case for class-lifetime state. Under
MethodLevel, a class-level resource lock is acquired and released for each method chunk (ResourceLockAttribute.cs:40-43), so state set in[ClassInitialize]and restored only in[ClassCleanup]remains exposed between chunks; another test can acquire the same key and observe it. Require per-test restoration, elimination, or non-parallel execution for that pattern.
**A lock alone is only half the fix — it must be paired with restoration.** A
resource lock is released when the scheduling **chunk** ends, so it prevents
*concurrent* interference but does nothing about *sequential* contamination: the
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Medium
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…arget the PR Three review threads plus one suppressed comment, all verified against source. grep -q under set -o pipefail exits on the first match and closes the pipe, so git show can die on SIGPIPE for a file larger than the pipe buffer and fail the whole pipeline, silently dropping a real state-change candidate. Reproduced with a 7.6 MB file whose match is on line 1: the old form exits 141, the new form exits 0. grep now consumes the stream and redirects to /dev/null. The lifecycle list limited constructors and disposal hooks to TestContainer classes. MSTest constructs the test class per test and invokes both IAsyncDisposable.DisposeAsync and IDisposable.Dispose (TestMethodInfo.Lifecycle.cs:78-87), so those hooks are per-test lifecycle code on any test class and a mutation in them races like one in a test body. The report instruction did not pass a PR number. This workflow uses target: "*", so there is no implicit triggering PR, and the repository's own guidance (shared/build-failure-analysis-shared.md:53-57) requires passing it on every safe-output call. It matters most for the command variant, whose runtime event is workflow_dispatch. Restoration guidance gained the class-lifetime case: under MethodLevel a class-level lock is acquired and released per test, so a resource established in [ClassInitialize] is not continuously owned through [ClassCleanup] (ResourceLockAttribute.cs:40-43). State set there and restored only in cleanup is exposed between chunks, so a class-level lock is not an acceptable fix for it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…Directory Three review threads, all fallout from widening the audit to lifecycle members two commits ago -- widening the scope created obligations I did not follow through on. [GlobalTestInitialize] and [GlobalTestCleanup] were missing from the lifecycle inventory. The adapter runs them for every test, so a mutation there is the broadest of all; both are now listed and attributed to the assembly. Assembly fixtures were lumped in with per-test lifecycle code as if they raced the same way. They do not: [AssemblyInitialize] and [AssemblyCleanup] are serialized by a SemaphoreSlim(1,1) (TestAssemblyInfo.cs:185/232 and :275/293), so initialize runs once before any worker proceeds. Treating a mutation there as automatically racing would have produced false live-race findings and could have recommended a method-level lock, which does not protect assembly lifetime at all. They now require a concrete conflicting observer before being reported. TestContext.TestTempDirectory was recommended unconditionally, but it returns null in assembly and class fixture contexts because those are not per-test and are not always disposed (TestContextImplementation.cs:126-134). Since the audit now covers fixture code, that could have produced a non-working fix. The recommendation is limited to per-test code, with fixture alternatives given, and no longer promises deletion, which is best effort and can be skipped when result files are retained. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f
Uh oh!
There was an error while loading. Please reload this page.
…rceLock] Ports microsoft/testfx's parallel-safety audit agentic workflow (microsoft/testfx#10252) into dotnet/sdk and starts using the [ResourceLock] feature it audits for. Workflow (.github/workflows/parallel-safety-audit{,-command}.md and shared/parallel-safety-audit-shared.md): - Adapted to this repo's gh-aw conventions: PAT-pool import, Copilot engine, cli-proxy + github gh-proxy tooling, fork guard, repo-pinned actions/checkout. - Adapted to this repo's test tree: test/TestAssets and test/TestPackages are excluded from the audit surface (they are test inputs, not test code), the MSTestParallelizeScope opt-in model and the test/Directory.Build.props default are described, and TestAssetsManager identifier collisions are called out. - MSTEST0074-0077 are described as shipping analyzers (they are now, see below) rather than in-flight, including that MSTestAnalysisMode=Recommended plus TreatWarningsAsErrors makes them build errors here. Dependencies: bump Microsoft.Testing.Platform, MSTest and MSTest.Sdk from 4.4.0-preview.26376.8 to *.26379.6 (darc), which brings in the parallel-safety analyzers MSTEST0073-MSTEST0077. Findings from the bump: - MSTEST0074 fired in Microsoft.NET.Build.Tests: It_fails_build_on_failed_sdk_resolution set MSBUILDINCLUDEDEFAULTSDKRESOLVER process-wide, which every concurrently started MSBuild inherits. Scoped it to the build under test with TestCommand.WithEnvironmentVariable instead. - DockerDaemonTests only needed DOCKER_HOST serialized in one test, so the class-level [DoNotParallelize] is replaced by [ResourceLock(WellKnownResources.EnvironmentVariables)] on that test, letting the other 10 tests run in parallel. - AuthHandshakeMessageHandlerTests keeps [DoNotParallelize]: it is order-dependent through AuthHandshakeMessageHandler's process-wide static credential cache, which a resource lock cannot cover. Comment updated to explain why. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c54ee239-4f8b-4400-af31-01010123c057
Do not merge expecting
/parallel-auditto work yet. gh-aw runs the compiled*.lock.yml, not the markdown source. This PR intentionally ships sources only — no*.lock.yml, andagentic_commands.ymlis left at the origin/main baseline — so there is currently nothing for gh-aw to execute. The lock must be generated on the aligned CI toolchain (thegithub/gh-aw-actions/setup@8bdba8075360648fe6802302a5b4e016361dc6ac # v0.83.1step used byagentics-maintenance) and committed before this workflow does anything. Merging as-is ships a green-CI, inert workflow that looks complete. No CI gate enforces lock freshness (checkedcheck-vendored-files.yml), so this caveat is the only guard — treat it as blocking.Why the lock is not committed here — the local
gh awbuild corrupts two pinned actions on compileCompiling with this environment's
gh awv0.83.1 build rewrites two security-relevant pins away from the repo-aligned values. Both are confirmed by regenerating a known-good baseline and full-file-diffing it (the reliable falsification —compiler_versionasserts an identity claim, not the emitted bytes):actions/checkoutdowngraded v7.0.1 → v7.0.0. On framework-injected steps (patch-context / threat-detection / create-PR) the build emitsactions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0instead of the aligned3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1([main] Update checkout and align agentic workflows #10174)..github/aw/actions-lock.jsonkeys its override by version string and holds only anactions/checkout@v7.0.1entry, so the injected v7.0.0 request key-misses and the CLI's stale built-in SHA wins.9c091bbappears in 0 of 30 committed baseline locks.github/gh-aw-actions/setupun-pinned from its SHA to a mutable tag. Regenerating the router rewrote the baseline's immutablegithub/gh-aw-actions/setup@8bdba8075360648fe6802302a5b4e016361dc6ac # v0.83.1to a baregithub/gh-aw-actions/setup@v0.83.1— a mutable ref, which is a worse supply-chain regression than a version delta (the resolved commit can change under the tag).Both are legitimate-looking outputs of the toolchain, not hand errors — which is exactly why shipping a locally-generated lock is the wrong move. Editing
actions-lock.jsoncan't fix#1 (the injected version label is baked into the binary), andgh extension upgrade awfails under SAML (HTTP 403), so the local build can't be corrected in place. The correct fix is to generate the lock on the CI-pinned setup action, which self-heals both pins.Repo-wide hazard (bigger than this PR)
Anyone who runs
gh aw compileon any agentic workflow in this repo with this local CLI build will silently (1) downgradeactions/checkoutv7.0.1 → v7.0.0 and (2) un-pingithub/gh-aw-actions/setupfrom its SHA to a mutable@v0.83.1tag in every lock they rewrite — a supply-chain-pin regression that could land unnoticed in an unrelated PR. The mechanism is the version-keyed override in.github/aw/actions-lock.json. One-command reproduction:gh aw compile add-tests --strictthengit diffyields exactly the checkout SHA/version lines changed, byte-identical otherwise. Tracked independently of this change in #10258; the durable fix is to compile only on the CI-pinnedgithub/gh-aw-actions/setup@8bdba80…rather than a local build.What this adds
A parallel-safety audit for .NET/MSTest test suites, delivered as a gh-aw agentic workflow: a PR trigger (
parallel-safety-audit.md, ontest/**) and a/parallel-auditslash command (parallel-safety-audit-command.md), sharing one analysis body (shared/parallel-safety-audit-shared.md).It answers a question no existing skill or analyzer asks: "will this suite break, or fail to speed up, when run in parallel?" The same call site can be perfectly testable, not a smell, and still race the moment
[Parallelize]is enabled.What it does that the analyzers cannot
Today only MSTEST0073 (
PreferConstantForResourceLockAnalyzer) ships onmain; the dedicated parallel-safety analyzers MSTEST0074–0077 are in flight (branchdev/amauryleve/parallel-safety-analyzers) and are cited throughout as a forthcoming compile-time complement, not active coverage. Even once they ship, the analyzers see the call; the audit reasons about the resource and the suite's actual configuration:consts, fixture fields and config to decide whether two tests can actually collide, then flags category B only with evidence of colliding I/O ("constructed and then written to"). MSTEST0077 deliberately owns only the narrow direct-literal→mutating-API case after it produced 8/8 false positives on pure-value paths.[ResourceLock]), over-declaration, ordinal key near-misses ("my-db"vs"my_db", case-only diffs), and read-set coverage gaps followed through the call graph into unchanged production code.1 / (s + (1 - s)/N), againstNif the serialized fraction s were eliminated. There is deliberately no cutoff, because the gain grows with worker count —s = 0.02is worth ~14% atN = 8but ~60% atN = 32. (An earlier revision useds > 1/Nas a break-even test; that is wrong, since it describes when the1/sceiling binds rather than whether fixing s pays — atN = 8,s = 0.10sits below1/Nyet still caps the suite at ~4.7x against a possible 8x.) The class-level-lock finding is gated toMethodLevel, where it actually costs something.off/ClassLevel/MethodLevel+ worker count rather than assuming theClassLeveldefault; aMethodLevelsuite has live intra-class races aClassLevelone does not.TestContainerhost, so it does not cry wolf on code that has no scheduler.It encodes the culture rule TFM-aware: flag process-wide
DefaultThreadCurrent[UI]Cultureon every target; on all testfx TFMs (.NET Core 5+/.NET FW ≥4.6)Thread.CurrentThread.Current[UI]CultureandCultureInfo.Current[UI]Cultureshare ExecutionContext-flowed storage and are not flagged as a live race — the pooled-thread forward-leak only applies on .NET FW <4.6, verified against the suite's actual TFM. Header banner notes that a suite opting in via.runsettingsonly gets zero analyzer coverage even after 0074–0077 ship (no analyzer reads runsettings XML) — making the audit the sole net there; theMSTestParallelizeScopeMSBuild-property opt-in is analyzer-visible becauseParallelize.targetsemits a real[assembly: Parallelize]viaWriteCodeFragment.Placement
.agents/skills/isgh-copilot-curate-managed, so a hand-added skill there would be overwritten bygh copilot-curate update. This ships as a repo-local gh-aw workflow instead. The analysis prose in the shared body is written to be portable upstream to adotnet-testpluginSKILL.md— arguably its best long-term home.Dogfood — four verdicts on live specimens
Verified against real assembly opt-ins:
Microsoft.Testing.Extensions.UnitTestsandMSTest.Acceptance.IntegrationTestsboth carry[assembly: Parallelize(Scope = MethodLevel, Workers = 0)];TestFramework.UnitTestshas no opt-in (TestContainerhost).MSTest.Acceptance.IntegrationTests/TelemetryTests.cs:16[B/D · Warning · High]bin/Release/<tfm>tree (Path.Combine(AssetFixture.MTPProjectPath,…)at 28/68/94). Give each test a unique output root (TestContext.TestTempDirectory), drop[DoNotParallelize]→ full parallelism recovered.Microsoft.Testing.Extensions.UnitTests/AzureFoundryChatClientProviderTests.cs:16[D · Warning · High][DoNotParallelize]defers the class to a serial tail. Swap to[ResourceLock(WellKnownResources.EnvironmentVariables)]— same mutual exclusion on the threeAZURE_OPENAI_*vars, no deferral, bounded read-set.Two non-findings — presented as prominently, because they are the strongest evidence the audit reasons about context:
TestFramework.UnitTests/…/ExecutableConditionAttributeTests.cs:121→[A · Info · readiness-only]. RealPATHmutation (lines 121/152) that would race under MSTestMethodLevel— but the suite isTestContainer-based (line 15) with no parallel scheduler, so there is no live race today and the forthcoming MSTEST0074 wouldn't apply either. Not silently dropped: the report states the inapplicability, the exact trigger that would make it live (ported to an MSTest[assembly: Parallelize]project), and the port-time fix (coarse[ResourceLock(EnvironmentVariables)], because the code shells out to child processes that inherit the wholePATH). A naive API scan would flag this identically to Porting latest changes. #2; engine-awareness is the difference.[assembly: Parallelize]/[ResourceLock]occurrences inParallelExecutionTests/ResourceLockExecutionTestsare synthetic test-asset payloads generated inside the acceptance tests, correctly excluded — a naive scan would report the feature's own fixtures as suite findings.Notes for reviewers
Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com
Copilot-Session: 9081c4a5-25b8-4527-aa8f-db8d6678456f