@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR

🤖 Copilot Code Review

Holistic Assessment

Motivation: The PR adds a targeted regression test for InheritedHandles / ProcessStartInfo file-handle inheritance behavior. The test exercises both the "not inherited" and "inherited" scenarios, and uses platform-appropriate identity comparisons (inode+dev on Unix, normalized path on Windows). The motivation is clear and the scope is appropriately small.

Approach: Using GetSafeFileHandleId (platform-split) rather than path comparison is the right call given the OS-level path normalization issues that were discovered. The logic in the child process correctly handles three cases: handle is the invalid sentinel, handle resolves to the same file, handle resolves to a different file.

Summary: ⚠️ Needs Human Review. The implementation is generally sound but has one logical gap in the inherited-handle path that could let a test pass incorrectly, plus a minor test-name mismatch. Other potential concerns (buffer size, file deletion order) raised by sub-agent reviewers were investigated and found to be safe.

Detailed Findings

⚠️ Logical gap: inherited handle with mismatched identity is silently accepted

In the child lambda, the "Handle value was reused for a different file" branch returns SuccessExitCode unconditionally, regardless of shouldBeInherited:

// Handle value was reused for a different file — not our handle, acceptable.
return RemoteExecutor.SuccessExitCode;

When shouldBeInherited = true, an explicitly inherited handle should always resolve to the same file. If childId != fileId in this case it means the inheritance silently failed — which the test should report as a failure. Consider:

// Handle value was reused for a different file — only acceptable when not inherited.
return shouldBeInherited ? RemoteExecutor.SuccessExitCode - 1 : RemoteExecutor.SuccessExitCode;

In practice this race cannot occur (an explicitly inherited Windows HANDLE or Unix fd always preserves its identity), but the test would be more robust and self-documenting with an explicit failure.

(Flagged independently by both Gemini and GPT sub-reviews.)

💡 Test name only reflects the negative case

NonInheritedFileHandle_IsNotAvailableInChildProcess describes only the inheritHandle=false scenario. The method also tests the positive case (inheritHandle=true). A more neutral name like FileHandleInheritance_AvailabilityMatchesInheritedHandlesList or simply InheritedHandles_FileHandleAccessibility would better represent both theory cases tested.

💡 Bare catch swallows all exception types

catch
{
    // Handle is not a valid file handle in this process.
    return shouldBeInherited ? ...;
}

This catches OutOfMemoryException, ThreadAbortException, and other unexpected exceptions. For test code this is acceptable, but a narrower catch (Exception) with a comment explaining why broad catching is intentional here would make the intent clearer.

Verified as Safe (no action needed)

✅ Windows GetFinalPathNameByHandle buffer size is correct

Both sub-agent reviewers flagged const int MaxPath = 32_767 as potentially one char short. Investigated and confirmed safe: cchFilePath = 32767 tells the API the buffer holds 32767 chars (including room for the null terminator), so paths up to 32,766 chars can be returned. The Windows extended-path hard limit (UNICODE_STRING.MaximumLength / sizeof(WCHAR) – 1) means path content cannot exceed 32,766 chars in practice. No resize needed.

✅ File deletion ordering in finally

remoteHandle (declared last) disposes before fileHandle (declared first) per C# using-declaration scoping. remoteHandle.Dispose() waits for the child to exit, releasing any inherited handle. fileHandle.Dispose() then closes the parent's handle. File.Delete in finally therefore runs with no open handles on any platform.

✅ Unix file identity is robust

FormattableString.Invariant($"{status.Dev}:{status.Ino}") correctly includes the device number for cross-mount robustness and uses invariant culture for cross-process stability.

✅ FileShare.None comment is accurate

The comment correctly avoids overstating the guarantee ("independent opens that honor .NET/OS sharing semantics are prevented") rather than claiming exclusive access is enforced at the kernel level.

@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR

🤖 Copilot Code Review

Holistic Assessment

Motivation: The PR adds a focused test that verifies file handle inheritance semantics when spawning child processes via InheritedHandles. This fills a real gap in the existing ProcessHandlesTests coverage.

Approach: Using RemoteExecutor to launch a child process and checking file identity via inode+dev on Unix and GetFinalPathNameByHandle on Windows is reasonable and platform-appropriate. No new production code is changed.

Summary: ⚠️ Needs Human Review. The code is largely correct and well-structured. There is one ⚠️ warning about the broad catch block that could silently mask unexpected failures in the non-inherited scenario. No blocking ❌ errors were found.

Detailed Findings

⚠️ Overly broad catch block — ProcessHandlesTests.cs line 592

The catch block catches all exceptions (including NullReferenceException, OutOfMemoryException, etc.), not just the expected Win32Exception thrown by GetSafeFileHandleId when the FD/handle is invalid:

catch
{
    // Handle is not a valid file handle in this process.
    return shouldBeInherited ? RemoteExecutor.SuccessExitCode - 1 : RemoteExecutor.SuccessExitCode;
}

Risk: When inheritHandle=false, an unexpected exception in GetSafeFileHandleId (e.g., a future bug introducing a NullReferenceException) returns SuccessExitCode, masking the failure. When inheritHandle=true, unexpected exceptions would correctly return SuccessExitCode - 1 (failure), so that direction is safe.

Consider narrowing to catch (Win32Exception):

catch (Win32Exception)
{
    // Handle is not a valid file handle in this process.
    return shouldBeInherited ? RemoteExecutor.SuccessExitCode - 1 : RemoteExecutor.SuccessExitCode;
}

Both GetSafeFileHandleId implementations throw only Win32Exception on failure, so this narrower catch covers all expected cases while letting unexpected failures propagate and fail the test clearly.

✅ Test structure — handles edge cases correctly

The test correctly handles the OS handle-value-reuse scenario (a different file coincidentally gets the same FD/handle value in the child). The three-way result logic (invalid handle → no-op, same id → pass/fail based on shouldBeInherited, different id → treat as unrelated file) is sound.

✅ Platform-specific identity — appropriate choices

• Unix: Dev:Ino (with FormattableString.Invariant) correctly identifies files across mounts and is stable across processes. Including Dev alongside Ino avoids cross-device collisions.
• Windows: GetFinalPathNameByHandle with FILE_NAME_NORMALIZED gives a stable canonical path. A 32,767-character buffer covers all practical Windows path lengths.

✅ No new public API surface, no production code changed

All changes are scoped to test files. No API approval verification needed.

💡 Minor — FormattableString.Invariant is technically unnecessary for long values

long.ToString() is already culture-invariant in .NET, so FormattableString.Invariant($"{status.Dev}:{status.Ino}") is defensive but not strictly required. This is a minor nit; keeping it as is is fine as a defensive habit.

@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR

🤖 Copilot Code Review

Holistic Assessment

Motivation: The PR adds a targeted, focused test for ProcessStartInfo.InheritedHandles file-handle inheritance behavior. Both the positive (inherited) and negative (not inherited) cases are exercised — this is a valuable regression guard for a subtle OS-level feature.

Approach: Using platform-specific GetSafeFileHandleId helpers (inode+dev on Unix, normalized path on Windows) rather than comparing file names is the right call given the OS-level path normalization issues that motivated the design change. The RemoteExecutor pattern is correct for this cross-process test.

Summary: ⚠️ Needs Human Review. One genuine concern — the bare catch in the child process lambda (flagged independently by all three reviewer models) — and a minor test-name cosmetic issue. The core logic, platform wiring, and handle lifecycle are all correct.

Detailed Findings

⚠️ Exception Handling — Bare catch in child lambda may mask bugs in new helpers

File: ProcessHandlesTests.cs:582-596

try
{
    string childId = GetSafeFileHandleId(handle);
    ...
}
catch   // ← catches everything, including bugs in the new helper
{
    // Handle is not a valid file handle in this process.
    return shouldBeInherited ? RemoteExecutor.SuccessExitCode - 1 : RemoteExecutor.SuccessExitCode;
}

The only expected exception from GetSafeFileHandleId on an invalid handle is Win32Exception (both Unix FStat returning -1 and Windows GetFinalPathNameByHandle returning 0 explicitly throw it). A bare catch swallows any other exception — including bugs in the new interop helpers — and maps them to a test pass when inheritHandle == false, hiding real failures. Should be catch (Win32Exception).

Flagged by all three reviewer models.

✅ Handle lifecycle — ownsHandle: false is correct

The child wraps the raw handle value with ownsHandle: false for inspection only. Since the parent process owns the handle, the child must not close it. The using with ownsHandle: false correctly disposes the wrapper without closing the underlying handle.

✅ Test logic — all three exit-code paths are now correct

• handle.IsInvalid → fails when shouldBeInherited, passes otherwise ✓
• childId == fileId → passes when shouldBeInherited, fails otherwise ✓
• childId != fileId (after latest fix) → fails when shouldBeInherited, passes otherwise ✓
• catch (invalid file handle) → fails when shouldBeInherited, passes otherwise ✓

✅ Platform wiring — correct compilation scope

ProcessHandlesTests.Unix.cs is included for unix|browser|android|maccatalyst (matching where Interop.Stat.cs is compiled). The class-level [SkipOnPlatform(TestPlatforms.Android | ...MacCatalyst)] and [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] ensure the test doesn't run on unsupported platforms.

💡 Test name — slightly misleading for the inheritHandle=true case

NonInheritedFileHandle_IsNotAvailableInChildProcess describes only the false scenario. A name like FileHandle_ChildProcessInheritanceRespected would cover both cases. Minor — not blocking.

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR

🤖 Copilot Code Review

Holistic Assessment

Motivation: The PR adds a well-motivated regression test for ProcessStartInfo.InheritedHandles file-handle inheritance. It correctly identifies the core challenge: handle numbers can be reused by the OS, so numeric comparison alone is insufficient — file identity (via dev:ino on Unix, final path on Windows) is the right approach.

Approach: The [ConditionalTheory] with both false and true cases, the per-platform GetSafeFileHandleId helpers, and the catch (Win32Exception) when (!shouldBeInherited) filter are all correct and well-designed. The FileShare.None exclusive open prevents ID collision with an independently-opened copy of the same file.

Summary: ✅ LGTM. No blocking correctness issues found across all three models. The findings below are all suggestions/minor notes.

Detailed Findings

💡 Suggestion — Test method name partially describes the scenario (flagged by 2 models)

File: ProcessHandlesTests.cs:547

NonInheritedFileHandle_IsNotAvailableInChildProcess only describes the inheritHandle=false case. The test now also validates that an explicitly-inherited handle is available in the child. A more neutral name (e.g., FileHandle_IsAvailableInChildProcess_OnlyWhenExplicitlyInherited) would reflect both scenarios — but the current name is not wrong, just incomplete. Reviewer discretion.

💡 Suggestion — Windows buffer overflow not fully defended (flagged by 2 models)

File: ProcessHandlesTests.Windows.cs:190-196

GetFinalPathNameByHandle returns result >= MaxPath if the buffer was too small. The current code doesn't guard that case: new string(buffer, 0, (int)result) would throw ArgumentOutOfRangeException rather than a clear error. A defensive guard costs nothing:

if (result == 0 || result >= MaxPath)
{
    throw new Win32Exception();
}

For test code this is extremely low priority (no real path approaches 32,767 chars), but it's a belt-and-suspenders improvement.

✅ Verified — [SkipOnPlatform] removal is correct

The original had [SkipOnPlatform(TestPlatforms.Browser | TestPlatforms.Wasi, ...)] which was redundant with [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]. The new code correctly relies solely on ConditionalTheory — RemoteExecutor.IsSupported returns false on Browser/WASI. Removal is intentional and correct.

✅ Verified — Exception handling logic is correct

catch (Win32Exception) when (!shouldBeInherited) correctly handles the three cases:

1. shouldBeInherited=false, handle invalid → exception caught, returns success ✅
2. shouldBeInherited=false, handle reused for different resource → childId != fileId, Assert.Equal(false, false) passes ✅
3. shouldBeInherited=true, handle valid and same file → childId == fileId, Assert.Equal(true, true) passes ✅
4. shouldBeInherited=true, exception thrown → propagates, test fails ✅

@copilot please invoke the code-review skill and post your analysis/results as a comment on this PR

🤖 Copilot Code Review

Holistic Assessment

Motivation: The PR adds a focused, well-motivated test for ProcessStartInfo.InheritedHandles file-handle inheritance. The test correctly identifies the core challenge: raw handle numbers can be reused by the OS, so numeric comparison alone is insufficient—file identity (via dev:ino on Unix, normalized path on Windows) is the right approach.

Approach: The [ConditionalTheory] with both false and true cases, per-platform GetSafeFileHandleId helpers, catch (Win32Exception) when (!shouldBeInherited) filter, and FileShare.None exclusive open are all correct and well-designed. The RemoteExecutor pattern is the standard isolation mechanism for cross-process tests.

Summary: ✅ LGTM. No blocking correctness issues found across either model. The single finding below is a minor observation, not a blocker.

Detailed Findings

✅ Test Logic — Correct handle identity comparison

Assert.Equal(shouldBeInherited, childId == fileId) covers all cases correctly:

• shouldBeInherited=true: child resolves to same file → childId == fileId → assertion passes ✓
• shouldBeInherited=false, handle invalid → Win32Exception caught → early return success ✓
• shouldBeInherited=false, handle reused for same file → would fail the assertion, but FileShare.None prevents this from happening ✓

✅ Error Handling — Exception narrowing is correct

catch (Win32Exception) when (!shouldBeInherited) is the only exception GetSafeFileHandleId throws on both platforms (both FStat failure on Unix and GetFinalPathNameByHandle failure on Windows). Non-Win32Exception errors propagate unconditionally, preventing them from masking bugs.

💡 Suggestion — Windows buffer size (advisory, test code)

GetFinalPathNameByHandle returns result >= MaxPath if the buffer was too small. The current code would pass an oversized result to new string(buffer, 0, (int)result), which would throw ArgumentOutOfRangeException. In practice this can't happen since MaxPath = 32_767 is the Windows maximum path length, so this is safe as-is for test code. No action needed.