Make ICommandExecutor's synchronous path a real primitive - #21
Merged
Conversation
Closes #17. Closes #10. The three synchronous `Execute` overloads blocked on `Task<T>.Result`, which wraps whatever the operation threw in an `AggregateException`. A caller writing the obvious try/catch around a synchronous call caught nothing, because the exception they were looking for had become an inner exception. That is observable from outside the library. `ICommandExecutor` now declares a synchronous primitive — `Execute(command, environmentVariables, workingDirectory, cancellationToken)` — that the other two synchronous members compose over, mirroring how the stream providers let an implementer replace a default by declaring the primitive. The default body still bridges to `ExecuteAsync`, but through `GetAwaiter().GetResult()`, so a failure surfaces unwrapped with its stack trace intact. The synchronous overloads also take a `CancellationToken` now, so a caller can bound the wait instead of being pinned to `CancellationToken.None`. `NativeCommandExecutor` declares that primitive itself and drives `Process` synchronously: `BeginOutputReadLine`/`BeginErrorReadLine` for capture (reading one redirected stream to the end while the other fills its buffer deadlocks), and a `WaitForExit(timeout)` poll that honours the token and kills the child on cancellation. No thread-pool thread is held for the process lifetime. The start-info construction is now shared between the two paths. Also renames `IncrementalHashAdapter`'s field to `_inner` and drops the `this.` qualifier, which was the only one in the repository and contradicted both CLAUDE.md and `.editorconfig`'s `dotnet_style_qualification_for_field`. Tests: four new cases over the synchronous path — stderr capture, environment variables, cancellation, and that `ExecuteAndGetOutput` throws an unwrapped `InvalidOperationException` with no inner exception. Full suite: 640 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
Contributor
Author
|
Both jobs fail during the build, before any test runs:
Evidence it is the base branch, not the PR: the scheduled No re-run spent. A re-run would fail identically — main's own scheduled run is stronger evidence than a second attempt on this branch would be. Proposed patch, which I have deliberately not pushed here, since it is <!-- Essentials/Essentials.csproj -->
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.1'">
<PackageReference Include="System.Memory" />
</ItemGroup>// Essentials/AssemblyInfo.cs
[assembly: System.Runtime.CompilerServices.InternalsVisibleTo("ktsu.Essentials.Tests")]
Say the word and I will push either the patch above or a severity change onto this branch; otherwise it wants its own PR against I have not been able to run these analyzers locally at any point on this branch: the sandbox has .NET SDK 10.0.111 (Roslyn 5.0) and Generated by Claude Code |
`main` has been red since at least 3 September — scheduled runs 193 through 196 all fail identically on commit f03a686, so this PR inherited a broken base rather than causing one. No test has run in CI for days: the build dies during compilation, before the suite starts. Two analyzer errors, on every target framework: * KTSU0002 wants internals exposed to `ktsu.Essentials.Tests`. Added via the `InternalsVisibleTo` item on `Essentials` and `Essentials.All`, the two projects that carry internals. * KTSU0001 wants a `System.Memory` reference from every project using `Span<T>`/`Memory<T>` — 41 of them. Declared once in `Directory.Build.props` for netstandard2.1 rather than copied into 41 csproj files. KTSU0001 collides head-on with NuGet's NU1510, which calls that same reference redundant because netstandard2.1 already carries those types in its reference assemblies. One of the two has to be silenced; `NoWarn="NU1510"` on that single item follows ktsu.Sdk's own stated requirement and is as narrow as the suppression gets. Fixing those let the test project compile for the first time in days, which surfaced 70 `CA1859` errors telling the tests to bind to concrete types. That is wrong here by design: these providers are built on default interface implementations, which are only callable through the interface, so following it would either fail to compile or dispatch elsewhere — the suite would stop exercising the contract it exists to cover. `CA1859` joins `CA1062` and `CA1707` in the test project's existing `NoWarn`, which is this repository's established way of turning off rules that do not apply to test code. Verified rather than inferred. The sandbox SDK (10.0.111, Roslyn 5.0) could not load `ktsu.Sdk.Analyzers` (needs 5.9) and failed with CS9057, which is why the earlier push could not see any of this. Overriding the compiler with `Microsoft.Net.Compilers.Toolset` 5.9.0 from NuGet reproduces CI's analyzer set exactly: the original two errors reproduced locally on the same files and target frameworks, and the whole solution now builds with 0 warnings and 0 errors. CLAUDE.md documents the technique and both suppressions. Essentials.Tests: 640 passed, 0 failed, in Release with the analyzers loaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
The previous commit satisfied KTSU0001 by referencing System.Memory for netstandard2.1, with NoWarn="NU1510" on the item. That passed locally and CI still rejected it: error NU1510: PackageReference System.Memory will not be pruned. This package is automatically available and does not need to be referenced explicitly. Remove the PackageReference item. [Essentials.slnx] Two differences from the local run. CI's SDK (10.0.400) words NU1510 as a flat refusal where the sandbox's 10.0.111 calls the package "likely unnecessary", and CI raises it during solution restore, attributed to the .slnx, where NoWarn metadata on the item does not reach it. So the reference cannot be added, and KTSU0001 cannot be satisfied. Of the two, NuGet is the one describing reality: the framework supplies these types on netstandard2.1, which is why it prunes the package. KTSU0001's premise does not hold on this SDK, so it is suppressed instead — scoped to netstandard2.1, the only target it fires on. The suppression lives in a new Directory.Build.targets, not Directory.Build.props: ktsu.Sdk assigns NoWarn outright, and props is imported before the SDK, so the addition was silently discarded. Confirmed with -getProperty:NoWarn, which showed the value without it. That is also why the first attempt at this looked correct and did nothing. Verified with CI's analyzer set reproduced locally via Microsoft.Net.Compilers.Toolset 5.9.0: whole solution builds with 0 warnings and 0 errors, no System.Memory reference anywhere outside Directory.Packages.props' PackageVersion, and Essentials.Tests passes 640/640 in Release. CLAUDE.md records the conflict, the resolution and the props-vs-targets trap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
…patch] CI is green on 386ef7f — tests pass on Linux and Windows, analysis and the build are clean — but the SonarCloud quality gate fails on coverage of new code: 63.0% against a required 80%. Measured locally with the Microsoft coverage collector rather than guessed at. Sonar's coverage metric folds branch coverage into line coverage, and computing it the same way over the diff reproduces the gate's reading closely: 69.0% locally against the 63.0% reported. The gap between them is stable, so the same calculation is a usable check on a fix. What was untested, and now is: * `ICommandExecutor`'s own synchronous defaults. `NativeCommandExecutor` declares the primitive, so it replaces every one of them and no test ever ran the interface's bodies. `AsyncOnlyCommandExecutor` declares only the asynchronous members, which is the shape any other implementer inherits, and two tests drive its bridge: one for the result, one asserting the failure arrives unwrapped. The second is the #17 regression guard for the default body itself — the existing one only covered the native path. * `ExecuteAndGetOutput`'s success path. Only its throwing path was covered. * Cancellation while a process is running, on both paths. The existing test cancelled before the call, so the synchronous poll loop and the kill it performs had never executed. Both new tests assert the call returns well inside the child's 20s lifetime rather than waiting it out. * A working directory that does not exist. This is a behaviour this branch introduced and had not tested: the asynchronous path previously let the resulting `Win32Exception` escape, and now reports it as a failed result like the synchronous one does. Two pieces of the new code were unreachable rather than untested, so they are gone rather than papered over with a test: * The synchronous `catch (OperationCanceledException)`. Nothing in that method throws it — the poll uses `WaitForExit(int)`, which takes no token, and cancellation returns directly from the loop. * `TryKill`'s two empty catch clauses, which are now one filtered clause. Both described the same race and neither did anything. Coverage of the diff, computed as Sonar computes it: 69.0% -> 90.2%. 647 tests pass, and the solution builds with 0 warnings and 0 errors on all six target frameworks with CI's analyzer set loaded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
CodeQL flagged the `Path.Combine` added in the previous commit: a later rooted segment silently discards the earlier ones. The second segment here is a GUID, so it can never be rooted and the two calls produce the same string — but `Path.Join` concatenates unconditionally, which is what this line means, and it costs nothing to say so. 647 tests still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
|
This was referenced Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #17. Closes #10.
#17 —
Executeblocked on.Result, so sync callers sawAggregateExceptionAll three synchronous
Executeoverloads were default interface implementations that blocked onTask<T>.Result. That wraps whatever the operation threw in anAggregateException, so a caller writing the obvious try/catch around a synchronous call caught nothing — the exception they were looking for had become an inner exception. That part is observable from outside the library.The issue framed the fuller fix as a question about which half of the API is the primitive. This takes the same answer the stream providers took: declare a synchronous primitive and let an implementer replace it.
ICommandExecutor.Execute(command, environmentVariables, workingDirectory, cancellationToken)is now the synchronous primitive.Execute(command, workingDirectory, cancellationToken)andExecuteAndGetOutputcompose over it, so overriding one member converts all three.ExecuteAsync, but throughGetAwaiter().GetResult(), so a failure surfaces unwrapped with its stack trace intact.VSTHRD002stays suppressed there, but it is now suppressing thread-blocking alone rather than also hiding an exception bug.CancellationToken(source-compatible), so a caller can bound the wait instead of being pinned toCancellationToken.None.NativeCommandExecutordeclares the primitive itself and drivesProcesssynchronously:BeginOutputReadLine/BeginErrorReadLinefor capture, and aWaitForExit(timeout)poll that honours the token and kills the child on cancellation, then a parameterlessWaitForExit()to flush the read handlers. No thread-pool thread is held for the lifetime of the child process.On the capture strategy: reading one redirected stream to the end while the other fills its buffer deadlocks, which is the
WaitForExit()caveat recorded in ktsu-dev/Sdk#35. The event-based handlers are the documented way around it in a synchronous method. Start-info construction is now shared between the sync and async paths rather than duplicated.ExecuteAndGetOutputno longer bridges at all — it calls the synchronous primitive directly and throwsInvalidOperationExceptionitself.Both paths also now report a working directory that does not exist as a failed
CommandResult. The asynchronous path previously let the resultingWin32Exceptionescape.This is a separate defect class from #8 and does not overlap with it.
#10 — the one
this.qualifier in the repositoryIncrementalHashAdapter's field is renamed to_innerand the qualifier dropped, matching howBitRotateObfuscationProviderresolves the same field/parameter collision. One line, as the issue described.The issue's aside — that
EnforceCodeStyleInBuildis unset, so no.editorconfigrule marked:erroris actually enforced at build time — is left alone here. Turning it on is a separate change with its own blast radius.Unblocking the build
mainhad been red since at least 3 September: scheduled runs 193–196 all fail identically onf03a686, from thektsu.Sdk2.28.0 bump in #20. The build died during compilation, so no test had run in CI for days and this PR inherited a broken base rather than causing one. Two analyzer errors, on every target framework:KTSU0002wants internals exposed toktsu.Essentials.Tests, added viaInternalsVisibleToon the two projects that carry internals.KTSU0001wants aSystem.Memoryreference from every project usingSpan<T>/Memory<T>— all 47 netstandard2.1 projects. That reference cannot be added: NuGet rejects it during solution restore withNU1510("This package is automatically available and does not need to be referenced explicitly. Remove the PackageReference item."), raised against the.slnxwhere item-levelNoWarndoes not reach. The two rules cannot both be satisfied, and NuGet is the one describing reality — the framework supplies the package. SoKTSU0001is suppressed instead, scoped to netstandard2.1.The suppression lives in a new
Directory.Build.targetsrather thanDirectory.Build.props: ktsu.Sdk assignsNoWarnoutright, and props is imported before the SDK, so an addition there is silently discarded. Confirmed with-getProperty:NoWarn.That let the test project compile for the first time in days, which surfaced 70
CA1859errors telling the tests to bind to concrete types. That is wrong here by design — these providers are built on default interface implementations, which are only callable through the interface — soCA1859joins the test project's existingNoWarnalongsideCA1062andCA1707.Testing
dotnet test— 647 passed, 0 failed. The solution builds with 0 warnings and 0 errors on all six target frameworks (net10.0, net9.0, net8.0, net7.0, net6.0, netstandard2.1) with CI's analyzer set loaded.Eleven new cases in
CommandExecutorTests, which previously had a single test for the synchronous path:CommandExecutor_Sync_Execute_Captures_Standard_ErrorCommandExecutor_Sync_Execute_With_Environment_VariablesCommandExecutor_Sync_Execute_Reports_CancellationCommandExecutor_Sync_ExecuteAndGetOutput_Throws_Unwrapped_On_Failure— the regression test for ICommandExecutor.Execute blocks on .Result, so sync callers see AggregateException #17: assertsInvalidOperationExceptionexactly, with a nullInnerException. This failed before the change, because.Resultproduced anAggregateException.CommandExecutor_Sync_ExecuteAndGetOutput_Returns_OutputCommandExecutor_Async_Reports_Cancellation_Before_StartCommandExecutor_Async_Reports_Cancellation_While_RunningCommandExecutor_Sync_Execute_Cancels_A_Running_Process— the earlier cancellation test cancelled before the call, so the poll loop and the kill it performs had never executed. Both mid-run tests assert the call returns well inside the child's 20s lifetime rather than waiting it out.CommandExecutor_Reports_Failure_For_Missing_Working_DirectoryCommandExecutor_Default_Sync_Primitive_Bridges_To_AsyncandCommandExecutor_Default_Sync_Primitive_Throws_Unwrapped—NativeCommandExecutordeclares the primitive, so it replaces every one ofICommandExecutor's synchronous defaults and no test ever ran the interface's own bodies.AsyncOnlyCommandExecutordeclares only the asynchronous members, which is the shape any other implementer inherits, and these two drive its bridge. The second is the ICommandExecutor.Execute blocks on .Result, so sync callers see AggregateException #17 regression guard for the default body itself.Two pieces of the new code turned out to be unreachable rather than untested, so they are gone rather than papered over with a test: the synchronous
catch (OperationCanceledException)(nothing in that method throws it — the poll usesWaitForExit(int), which takes no token), andTryKill's two empty catch clauses, now one filtered clause.Coverage on new code: 63.0% → 92.0%, above the 80% quality gate. Duplication 0.0%.
🤖 Generated with Claude Code
https://claude.ai/code/session_01N1cVfacJWw1uM42DUmPfUg
Generated by Claude Code