feat(dotnet11): add process-api-net11 skill for new process APIs - #866
shaikhsharukh wants to merge 14 commits into
Conversation
|
Note This PR is from a fork and modifies infrastructure files ( Changes to infrastructure typically need to be submitted from a branch in Please consider recreating this PR from an upstream branch. If you don't have push access to |
|
@dotnet-policy-service agree |
bb05fea to
29286e7
Compare
|
A short narrated explainer of this PR (dev testing my tool of public repos) - I hope you find it helpful. https://www.lenzon.ai/viewer/cmrb1zapc000o14os7auramuh?voice=google-chirp3 |
Thanks for this contribution, @shaikhsharukh! 🎉 I verified every API in this skill against Before we run evals and move toward merge, though, the 1.
2. 3. Static method signatures. 4. Example 1 won't compile. 5. Example 3 won't compile. For reference, Example 2 ( Once the signatures and examples are aligned with the ref assembly, we'll kick off evals and continue the review. Thanks again for contributing! 🙏 |
8c1066b to
2ae8051
Compare
|
I have aligned the skill definitions and examples with the
|
There was a problem hiding this comment.
Follow-up review — API accuracy re-verified + eval-hardening suggestions
Thanks for the fixes, @shaikhsharukh. I re-checked everything and ran a deeper evaluation pass. Summary below; concrete suggestions are inline on eval.yaml.
1. API accuracy — all confirmed ✅ (proven)
I re-verified every signature in SKILL.md against the current dotnet/runtime main reference assemblies. All five items from the earlier review are now correct: the tuple-returning ReadAllText/ReadAllTextAsync → (string StandardOutput, string StandardError), streaming ReadAllLinesAsync → IAsyncEnumerable<ProcessOutputLine>, RunAndCaptureText(Async) → ProcessTextOutput, and KillOnParentExit. Content looks accurate.
2. Does the skill actually help? — yes, in plugin/deployment mode (proven, scoped)
I hardened the eval to use intent-only prompts (never naming the API) and ran it with skill-validator (--runs 5, judge = claude-opus-4.6). In the whole-plugin configuration (how the skill actually ships), it produced large, low run-to-run variance (CV 1–6%) gains on its target APIs vs a no-skill baseline:
| Scenario | Baseline | With skill |
|---|---|---|
Run + capture (RunAndCaptureText) |
1.8 / 5 | 4.2 / 5 |
Full read (ReadAllText tuple) |
1.0 / 5 | 4.2 / 5 |
Stream lines (ReadAllLinesAsync) |
1.4 / 5 | 4.2 / 5 |
The no-skill baseline reached for the pre-.NET-11 pattern (StandardOutput.ReadToEndAsync() + Task.WhenAll); the judge noted it "didn't even attempt to research what new APIs .NET 11 offers." So the skill is teaching genuinely non-obvious APIs — that's its value.
3. Honest caveat — the blended eval verdict is not green
Being transparent: the validator's official pass metric doesn't clear the 0.10 bar (this skill scored 0.065). The plugin-mode wins above are real, but the isolated single-skill arm shows only small lift and the skill roughly doubles output tokens — both of which the weighted score penalizes. So it's a strong deployment-mode signal with a failing blended score; flagging for maintainers to weigh rather than claiming a clean pass.
4. Suggested eval improvements (inline suggestions)
- Strengthen the run+capture assertions to require both
.StandardOutputand.StandardErrorand reject the old event model. - Add two scenarios that hit the harder, wrong-signature-prone surface — the tuple-returning
ReadAllTextand streamingReadAllLinesAsync/await foreach. These are where the skill adds the most measurable value. - Add a subtle
net10.0non-activation negative (the new APIs are net11-only). - The existing
KillOnParentExitscenario is fine as coverage, but note it shows ~0 marginal lift — the base model already emits it correctly — so it isn't what demonstrates the skill's value.
cc @lewing @SamMonoRT — would appreciate your read on point 3: the deployment-mode gains are real and stable, but the blended eval score doesn't clear the gate. Is the plugin-mode evidence enough to accept, or do you want the eval reworked to a clean green first?
… tests per reviewer suggestions
|
I have applied all of the suggested evaluation improvements:
|
|
/evaluate |
|
❌ Evaluation did not complete successfully (the evaluate job reported |
|
@eiriktsarpalis could you please review this PR? You did the deepest review on the closely analogous #535 (new dotnet11 plugin skill + evals), so your feedback here would be much appreciated. |
|
It might be best if @adamsitnik took a look. |
|
@adamsitnik will review once back from vacation. Please don't merge without his approval. |
|
@adamsitnik , @SamMonoRT : Gentle ping on reviewing the changes here. |
|
/evaluate |
|
👋 Two ways to run it:
|
|
/evaluate 630086d |
|
🔒 Secret-backed evaluation is disabled for fork PRs. A maintainer must review and promote the change to a trusted repository branch before running |
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
@shaikhsharukh big thanks for your contribution! PTAL at my comments.
| ## When to Use | ||
|
|
||
| - Running or orchestrating external processes in a .NET 11 (or later) project. | ||
| - Needing to start a process, wait for it to exit, and capture its output/error streams without risking deadlocks (`Process.RunAndCaptureTextAsync`). |
There was a problem hiding this comment.
nit: we provide sync and async overloads
| - Needing to start a process, wait for it to exit, and capture its output/error streams without risking deadlocks (`Process.RunAndCaptureTextAsync`). | |
| - Needing to start a process, wait for it to exit, and capture its output/error streams without risking deadlocks (`Process.RunAndCaptureText[Async]`). |
There was a problem hiding this comment.
Updated bullet to mention Process.RunAndCaptureText[Async].
| - Running or orchestrating external processes in a .NET 11 (or later) project. | ||
| - Needing to start a process, wait for it to exit, and capture its output/error streams without risking deadlocks (`Process.RunAndCaptureTextAsync`). | ||
| - Wanting to ensure child processes are automatically terminated when the parent process exits (`KillOnParentExit`). | ||
| - Requiring trimmer-friendly and NativeAOT-compatible process creation via `SafeProcessHandle`. |
There was a problem hiding this comment.
The wording needs a bit more specific. Process itself is NativeAOT-compatible, it's just that with SafeProcessHandle the user gets the smallest possible size on disk (so it's more of an optimization)
There was a problem hiding this comment.
Clarified that SafeProcessHandle is an optimization for minimal disk footprint with NativeAOT.
| ## When Not to Use | ||
|
|
||
| - The project targets .NET 10 or earlier — these APIs are not available before .NET 11. | ||
| - Running simple shells where custom execution code is unnecessary. |
There was a problem hiding this comment.
I am not a native speaker, so please take it with a grain of salt. But overall I do believe that the new APIs simplify the code a lot, so I would recommend them even for simple shells.
There was a problem hiding this comment.
Removed the bullet against simple shells.
| <TargetFramework>net11.0</TargetFramework> | ||
| ``` | ||
|
|
||
| ## New APIs & Convenience Methods |
There was a problem hiding this comment.
Each of these methods is a new API, some are also convenience methods. I would just call it New APIs
| ## New APIs & Convenience Methods | |
| ## New APIs |
There was a problem hiding this comment.
Renamed section heading to ## New APIs.
| ### High-Level Convenience APIs (Static Methods) | ||
|
|
||
| #### `Process.Run` / `Process.RunAsync` | ||
| Starts a process and waits for it to exit, returning the exit status. Does not capture standard output or error. |
There was a problem hiding this comment.
We need to mention that it also allows the users to discard the output/error by providing silent: true and internally redirecting std handles to NUL device.
There was a problem hiding this comment.
Mentioned that silent: true discards output/error by internally redirecting std handles to NUL.
| ``` | ||
|
|
||
| #### `StartDetached` | ||
| Starts the process detached from the parent's terminal or job session, ensuring it survives the parent's exit. |
There was a problem hiding this comment.
When set to true, the std handles are by default redirected to NUL device. So the child process does not keep the parent process console/terminal resources alive.
There was a problem hiding this comment.
Added note explaining standard handles redirection to the NUL device when Silent is true.
|
|
||
| if (result.ExitStatus.ExitCode == 0) | ||
| { | ||
| Console.WriteLine($"Git Output: {result.StandardOutput.Trim()}"); |
There was a problem hiding this comment.
There is no need to use Trim here.
| Console.WriteLine($"Git Output: {result.StandardOutput.Trim()}"); | |
| Console.WriteLine($"Git Output: {result.StandardOutput}"); |
| using System.Threading.Tasks; | ||
|
|
||
| // Run 'git status' and capture output (arguments passed as list) | ||
| ProcessTextOutput result = await Process.RunAndCaptureTextAsync("git", ["status"]); |
There was a problem hiding this comment.
People tend to use async for apps that absolutely don't benefit from it (scalability, cancellation support etc). I think it's better to use non-async overload for such simple examples. So AI-written code is simple and does not pay for the price of using async
| ProcessTextOutput result = await Process.RunAndCaptureTextAsync("git", ["status"]); | |
| ProcessTextOutput result = Process.RunAndCaptureText("git", ["status"]); |
There was a problem hiding this comment.
Fixed. Updated the example to use the synchronous Process.RunAndCaptureText overload.
|
|
||
| var startInfo = new ProcessStartInfo("dotnet", ["run", "--project", "BackgroundWorker.csproj"]) | ||
| { | ||
| KillOnParentExit = true // Auto-teardown when this parent process exits |
There was a problem hiding this comment.
| KillOnParentExit = true // Auto-teardown when this parent process exits | |
| KillOnParentExit = OperatingSystem.IsWindows() || OperatingSystem.IsLinux() // Auto-teardown when this parent process exits |
There was a problem hiding this comment.
Fixed. Added OS platform checks: KillOnParentExit = OperatingSystem.IsWindows() || OperatingSystem.IsLinux().
| ```csharp | ||
| using System.Diagnostics; | ||
|
|
||
| var startInfo = new ProcessStartInfo("dotnet", ["run", "--project", "BackgroundWorker.csproj"]) |
There was a problem hiding this comment.
nit: subjective: no need to use var
| var startInfo = new ProcessStartInfo("dotnet", ["run", "--project", "BackgroundWorker.csproj"]) | |
| ProcessStartInfo startInfo = new("dotnet", ["run", "--project", "BackgroundWorker.csproj"]) |
There was a problem hiding this comment.
Fixed. Updated the snippet to explicitly declare ProcessStartInfo.
|
Thank you @adamsitnik for the thorough review! All feedback has been addressed and pushed in the latest commit: updated argument types to |
| - Running or orchestrating external processes in a .NET 11 (or later) project. | ||
| - Needing to start a process, wait for it to exit, and capture its output/error streams without risking deadlocks (`Process.RunAndCaptureText[Async]`). | ||
| - Wanting to ensure child processes are automatically terminated when the parent process exits (`KillOnParentExit`). | ||
| - Requiring trimmer-friendly and NativeAOT-optimized process creation via `SafeProcessHandle` for the smallest possible disk footprint. |
There was a problem hiding this comment.
SafeProcessHandle is more lightweight API than full Process. It has better performance characteristics on all form-factors. It is not a NativeAOT specific optimization.
…sharukh/skills into contrib/process-api-net11
|
Thank you @jkotas for the clarification! Updated the documentation in the latest commit to accurately reflect that |
|
@jkotas , @adamsitnik : Could you please take another pass at this PR? Looks like the author has addressed all the feedback and this PR has been sitting for a bit. |
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
It's almost ready, but needs some edits first. Thank you for your contribution @shaikhsharukh !
| public readonly record struct ProcessExitStatus(int ExitCode) | ||
| { | ||
| public bool Success => ExitCode == 0; | ||
| } |
There was a problem hiding this comment.
This is wrong:
ProcessExitStatusconsists of 3 properties- We don't provide
Successon purpose, as we can't assume what exit codes are used to represent success/failure for every command line app. For example, somebody can return-1on success.
| public readonly record struct ProcessExitStatus(int ExitCode) | |
| { | |
| public bool Success => ExitCode == 0; | |
| } | |
| public readonly record struct ProcessExitStatus(int ExitCode, bool Canceled, PosixSignal? Signal = null); |
| ``` | ||
| - **`ProcessTextOutput`**: Contains the exit status along with all captured standard output and standard error text. | ||
| ```csharp | ||
| public readonly record struct ProcessTextOutput(ProcessExitStatus ExitStatus, string StandardOutput, string StandardError); |
There was a problem hiding this comment.
| public readonly record struct ProcessTextOutput(ProcessExitStatus ExitStatus, string StandardOutput, string StandardError); | |
| public readonly record struct ProcessTextOutput(ProcessExitStatus ExitStatus, string StandardOutput, string StandardError, int ProcessId); |
|
|
||
| ### Types | ||
|
|
||
| Before using the new convenience methods, note the following return and record structures: |
There was a problem hiding this comment.
None of them is a record.
| Before using the new convenience methods, note the following return and record structures: | |
| Before using the new convenience methods, note the following return and structures: |
| public static ProcessExitStatus Run(ProcessStartInfo startInfo, TimeSpan? timeout = null) | ||
| public static Task<ProcessExitStatus> RunAsync(ProcessStartInfo startInfo, CancellationToken cancellationToken = default) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
We need to add a note saying that on timeout/cancellation the process is killed.
| public static ProcessTextOutput RunAndCaptureText(ProcessStartInfo startInfo, TimeSpan? timeout = null) | ||
| public static Task<ProcessTextOutput> RunAndCaptureTextAsync(ProcessStartInfo startInfo, CancellationToken cancellationToken = default) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
We need to add a note saying that when using the ProcessStartInfo overloads, the user is responsible for setting output and error redirected via the boolean flags (because BCL APIs can't modify the arguments they were given).
| public static int StartAndForget(string fileName, IEnumerable<string>? arguments = null) | ||
| public static int StartAndForget(ProcessStartInfo startInfo) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Another note needed: by default, when output/error redirection was not specified, the StartAndForget method will redirect all standard handles to NUL device.
| ### ProcessStartInfo Properties | ||
|
|
||
| #### `KillOnParentExit` | ||
| Ensures that the spawned child process is terminated when the current (parent) process exits. Works across Windows, Linux, and Android. |
There was a problem hiding this comment.
| Ensures that the spawned child process is terminated when the current (parent) process exits. Works across Windows, Linux, and Android. | |
| Ensures that the spawned child process is terminated when the current (parent) process exits (including fatal crash and being force killed). Works across Windows, Linux, and Android. |
| ``` | ||
|
|
||
| #### `InheritedHandles` | ||
| Provides precise control over which file/kernel handles are inherited by the child process, preventing accidental resource leaks. |
There was a problem hiding this comment.
| Provides precise control over which file/kernel handles are inherited by the child process, preventing accidental resource leaks. | |
| Provides precise control over which handles (file descriptors) are inherited by the child process, preventing accidental resource leaks. |
| #### `Silent` | ||
| When set to `true`, the standard handles are by default redirected to the `NUL` device, ensuring the child process does not keep parent console or terminal resources alive. | ||
| ```csharp | ||
| public bool Silent { get; set; } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
ProcessStartInfo does not offer such property, it's just an argument for the Run[Async] methods
| #### `Silent` | |
| When set to `true`, the standard handles are by default redirected to the `NUL` device, ensuring the child process does not keep parent console or terminal resources alive. | |
| ```csharp | |
| public bool Silent { get; set; } | |
| ``` |
| using Process? process = Process.Start(startInfo); | ||
| if (process != null) | ||
| { | ||
| // Read all output lines safely and asynchronously | ||
| await foreach (ProcessOutputLine line in process.ReadAllLinesAsync()) | ||
| { | ||
| string prefix = line.StandardError ? "[Err]" : "[Out]"; | ||
| Console.WriteLine($"{prefix} > {line.Content}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
It's a PITA, but Process.Start can return null only when we set UseShellExecute = true.
| using Process? process = Process.Start(startInfo); | |
| if (process != null) | |
| { | |
| // Read all output lines safely and asynchronously | |
| await foreach (ProcessOutputLine line in process.ReadAllLinesAsync()) | |
| { | |
| string prefix = line.StandardError ? "[Err]" : "[Out]"; | |
| Console.WriteLine($"{prefix} > {line.Content}"); | |
| } | |
| } | |
| using Process process = Process.Start(startInfo)!; | |
| // Read all output lines safely and asynchronously | |
| await foreach (ProcessOutputLine line in process.ReadAllLinesAsync()) | |
| { | |
| string prefix = line.StandardError ? "[Err]" : "[Out]"; | |
| Console.WriteLine($"{prefix} > {line.Content}"); | |
| } |
This branch has not been deployed
7 participants
Resolves issue #649 by adding a new skill
process-api-net11for the newSystem.Diagnostics.ProcessAPIs introduced in .NET 11.This skill guides coding agents on using the new convenience one-liners, reliable deadlock-free output reading, and parent-child process lifetime properties. Evals have been included and verified using the skill-validator.