Allow IPC named pipe directory to be overridden on Unix#9846
Conversation
On Unix the IPC named pipe is backed by a Unix domain socket file that was always created under /tmp. Sandboxed environments (e.g. Claude Code on macOS) that block /tmp cannot run `dotnet test`. Resolve the directory as: 1. TESTINGPLATFORM_PIPE_DIRECTORY (explicit opt-in override) 2. Path.GetTempPath() (honors TMPDIR on Unix) 3. /tmp (previous default) Add a sun_path length guard (always) and a writability probe (override only), both failing with actionable, localized messages. Keep the logic self-contained in NamedPipeServer.cs so it flows into dotnet/sdk on the next vendor sync, and document the "creator resolves, full path is passed, peers never recompute" invariant so the directory never becomes a shared convention that couples SDK and test-host versions. Fixes #9821 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae434eba-3cad-4c61-a1da-ef252ae87f48
There was a problem hiding this comment.
Pull request overview
Adds configurable Unix IPC socket directories for sandbox compatibility while preserving Windows behavior.
Changes:
- Adds override/temp-directory resolution and safety guards.
- Adds localized diagnostics and environment-variable support.
- Adds unit tests and regenerated localization files.
Show a summary per file
| File | Description |
|---|---|
test/UnitTests/Microsoft.Testing.Platform.UnitTests/IPC/IPCTests.cs |
Tests directory resolution and path-length limits. |
src/Platform/Microsoft.Testing.Platform/IPC/NamedPipeServer.cs |
Implements Unix pipe-directory selection and validation. |
src/Platform/Microsoft.Testing.Platform/Helpers/EnvironmentVariableConstants.cs |
Defines the pipe-directory override variable. |
src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.cs |
Adds resource accessors. |
src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx |
Adds localized diagnostic source strings. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.cs.xlf |
Updates Czech localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.de.xlf |
Updates German localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.es.xlf |
Updates Spanish localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.fr.xlf |
Updates French localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.it.xlf |
Updates Italian localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ja.xlf |
Updates Japanese localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ko.xlf |
Updates Korean localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pl.xlf |
Updates Polish localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pt-BR.xlf |
Updates Brazilian Portuguese localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ru.xlf |
Updates Russian localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.tr.xlf |
Updates Turkish localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hans.xlf |
Updates Simplified Chinese localization. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hant.xlf |
Updates Traditional Chinese localization. |
Review details
- Files reviewed: 18/18 changed files
- Comments generated: 6
- Review effort level: Medium
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary
Overall: Solid, well-motivated change. The design is sound — the single-resolver invariant (creator resolves, peer uses verbatim) is the right architecture and the XML doc captures it well. The guards (path length + writability probe) are practical and the error messages are actionable.
Findings by severity
| Severity | Dimension | Finding |
|---|---|---|
| MAJOR | Test Completeness (#13) | EnsureDirectoryIsWritable has no unit tests — it has meaningful branching worth covering |
| MODERATE | Data-Driven (#14) | Missing " " (whitespace) [DataRow] for tempPath in the /tmp fallback test |
| NIT | Algorithmic (#1) | The /tmp fallback in ResolvePipeDirectory is effectively dead code in production (only reachable via the testable overload) — worth a clarifying comment |
| NIT | Security (#3) | Consider Path.GetFullPath() normalization on the explicit override for path hygiene |
Dimensions checked and clean
✅ Threading & Concurrency — no shared mutable state; all new methods are static/pure or local I/O
✅ Public API — class is internal [Embedded]; constants are public on an internal class (effectively internal); no PublicAPI.Unshipped.txt update needed
✅ IPC Wire Compatibility — no wire format changes; directory resolution is local-only
✅ Performance — GetPipeName is not a hot path; probe I/O only on explicit override
✅ Resource/IDisposable — probe file uses using + File.Delete
✅ Localization — .resx entries added correctly; .xlf regenerated; accessors placed in the !IS_CORE_MTP block (correct — these are needed by extension projects that link the file)
✅ Naming & Conventions — test names describe scenario and outcome
✅ Scope & PR Discipline — focused change, no unrelated refactoring
✅ Build Infrastructure — no dependency changes
No blocking issues found.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 61.6 AIC · ⌖ 4.9 AIC · ⊞ 7.3K · ◷
…ests - Replace banned System.Environment / string.IsNullOrWhiteSpace with SystemEnvironment / RoslynString to fix RS0030. - Declare the four new internal symbols in InternalAPI.Unshipped.txt (RS0016). - Normalize an explicit override with Path.GetFullPath so a relative TESTINGPLATFORM_PIPE_DIRECTORY resolves to a rooted socket path and honors the "full path handed to peers" invariant (also collapses '..'). - Clarify that the '/tmp' fallback is only reachable via the test overload. - Add unit tests for EnsureDirectoryIsWritable (create-missing happy path and non-creatable error path) and a whitespace DataRow for the tempPath fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae434eba-3cad-4c61-a1da-ef252ae87f48
This comment has been minimized.
This comment has been minimized.
…ed-train # Conflicts: # src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt
This comment has been minimized.
This comment has been minimized.
NamedPipeServer.cs is glob-compiled (IPC\*.cs) into HangDump, Retry, TrxReport, and both MSBuild projects, so the earlier fixes must build there: - Read TESTINGPLATFORM_PIPE_DIRECTORY via System.Environment with a scoped RS0030 suppression instead of `new SystemEnvironment()`. Only the MSBuild task project links SystemEnvironment.cs; the other shared-source consumers do not, so the wrapper would not compile there. Reading directly also keeps the file self-contained for the dotnet/sdk vendor copy. - Normalize the selected directory with Path.GetFullPath in every precedence branch (not just the explicit override): Path.GetTempPath() can surface a relative TMPDIR, which NamedPipeServerStream rejects and which would violate the fully-resolved-path invariant. The write probe stays override-only. - Declare the new EnvironmentVariableConstants / NamedPipeServer / PlatformResources members in the HangDump, Retry, TrxReport, and Extensions.MSBuild InternalAPI.Unshipped baselines (they compile these shared files and keep their baselines in sync with core), so RS0016 stays green. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae434eba-3cad-4c61-a1da-ef252ae87f48
…consumers The earlier baseline update missed some projects that link the shared files: - EnvironmentVariableConstants.cs is also linked into HotReload -> add the new TESTINGPLATFORM_PIPE_DIRECTORY const there. - PlatformResources.cs (the !IS_CORE_MTP accessors) is linked into nine API-tracked extensions -> add the two new NamedPipe* accessor signatures to HtmlReport, CtrfReport, JUnitReport, VideoRecorder, and CrashDump (the other four were done previously). Verified each affected project builds clean with -warnaserror:RS0016,RS0017,RS0025,RS0037. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae434eba-3cad-4c61-a1da-ef252ae87f48
This comment has been minimized.
This comment has been minimized.
…unting The existing path-length tests used only ASCII, so a buggy character-count implementation would still pass. Add a test with 40 '好' (3-byte) chars plus a prefix: 45 characters (under the 103 limit) but 125 UTF-8 bytes (over it), proving the guard measures sun_path bytes rather than characters. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae434eba-3cad-4c61-a1da-ef252ae87f48
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
❌ Build Failure AnalysisProject: Errors (4 total, 2 unique locations)
The errors appear twice because the project targets two frameworks (both evaluations fail). FixReplace the
|
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · 70.5 AIC · ⌖ 5.18 AIC · ⊞ 7.4K · ◷
Replace Assert.IsTrue(a < b)/Assert.IsTrue(a > b) with Assert.IsLessThan / Assert.IsGreaterThan in the multibyte path-length setup assertions. Note the argument order is (bound, value) - the raw analyzer codefix suggestion had the operands reversed, which would have inverted the assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ae434eba-3cad-4c61-a1da-ef252ae87f48
🧪 Test quality grade — PR #9846
This advisory comment was generated automatically. Grades are heuristic Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Fixes #9821
Problem
On Unix, the Microsoft.Testing.Platform IPC named pipe is backed by a Unix domain socket file on disk, and
NamedPipeServer.GetPipeNamehardcoded that file to live under/tmp. In sandboxed environments that block/tmp(the reporter hit this with Claude Code on macOS),dotnet testfails and the only workaround is to open up the whole/tmpfolder to the sandbox.Change
On Unix, the pipe directory is now resolved with this precedence:
TESTINGPLATFORM_PIPE_DIRECTORY— new explicit opt-in override (guaranteed escape hatch).Path.GetTempPath()— honorsTMPDIRon Unix, which is the standard POSIX knob and is usually a per-user, sandbox-allowed directory (so many setups are fixed with no extra configuration)./tmp— preserves the previous default when neither is set.Windows is unchanged (it uses the kernel pipe namespace, not the filesystem).
Guards
Because the location is now user-influenced:
sun_path(~104 bytes on macOS, ~108 on Linux). We validate against the smaller 103-byte budget and fail with an actionable, localized message. This matters most for a deepTMPDIRon macOS (/var/folders/…/T/+ a GUID).TMPDIR//tmpare OS-managed and skipped to avoid extra I/O and a behavior change on every pipe creation.Compatibility notes
The design deliberately keeps the existing invariant: the process that creates the pipe resolves the directory locally and hands the fully-resolved path to the peer (via env var, command-line arg, or the dotnet-test handshake); peers use that path verbatim and never recompute it. As a result:
TESTINGPLATFORM_PIPE_DIRECTORY/TMPDIRdifference between two processes is harmless because only the creator's resolution is ever used.NamedPipeServer.cs, whichdotnet/sdkvendors, so thedotnet testentrypoint picks this up on the next vendor sync with no separate SDK design work.An XML-doc note on
GetPipeNamecaptures this invariant so the directory is never turned into a shared convention that both sides derive independently (which would couple SDK and test-host versions).Localization
Two new strings added to
PlatformResources.resxwith hand-maintained accessors in the!IS_CORE_MTPblock;.xlfregenerated for all 14 languages viaUpdateXlf.Tests
Added 8 MSTest cases in
IPCTests.cscovering override precedence,TMPDIRfallback,/tmpfallback, and the length guard (within/exceeds). Resolution and length-check logic was split into internal seams so they are testable on Windows CI, where the Unix branch ofGetPipeNamenever runs. FullIPCTestsclass passes locally (net8.0).