Repository navigation
Delegate git process invocation to ktsu.RunCommand #27
Description
Activity
matt-edmondson commented
on Sep 15, 2026 ContributorAuthorMore actionsTriage
Category: Improvement
Priority: Low
Area:GitBranchStateCache— git process invocationLow: nothing broken, consolidation onto
ktsu.RunCommand.Suggested assignment: none specific.
Related:
ktsu-dev/KtsuBuild#136delegates process execution to the same library in the same audit rotation — separate repos, not a duplicate. Tracked underktsu-dev/.github#15.Worth knowing before starting:
ktsu-dev/SvnToGit#71is an open build failure fromCS0618on the obsoleteRunCommand.ExecuteAsyncoverload — evidence that this library's API is mid-deprecation. Target the current overload so this repo doesn't acquire the same break.Also consider whether
ktsu.GitIntegrationis the better fit thanktsu.RunCommandfor some of these call sites. This repo shells out to git specifically, not to arbitrary processes, and the org already has a git-operations library (seektsu-dev/KtsuTools#163, adopting it for exactly that reason).RunCommandis the right answer if the invocations are ad-hoc plumbing git porcelain thatGitIntegrationdoesn't model; it is the wrong one ifGitIntegrationalready exposes the operations being shelled out for. Worth five minutes checking which before committing, since redoing it later is more work than deciding now.Open PR covering this: none.
Suggested next step: enumerate the git invocations in this repo and check them against
ktsu.GitIntegration's surface first, then fall back toktsu.RunCommandfor whatever remains.
Generated by Claude Code
matt-edmondson commented
on Sep 21, 2026 ContributorAuthorMore actionsTriage
Not actionable as a clean swap yet. Two of the guarantees
GitRunnerdocuments as load-bearing cannot currently be preserved throughktsu.RunCommand's surface. Verified empirically againstktsu.RunCommand1.5.23 on net10.0, not read off the source.What does work, so it is not in doubt:
CommandOptions.EnvironmentVariablesisIReadOnlyDictionary<string, string?>and behaves exactly as the issue describes — the child inherits the parent environment, listed keys are overlaid, and anullvalue removes an inherited key. So theGIT_*stripping and theGIT_CONFIG_*protocol, which are this class's whole security posture, are expressible. Valid non-ASCII UTF-8 also round-trips correctly throughOutputHandler.Encoding.1. stdin is neither redirected nor closed
GitRunnersetsRedirectStandardInput = trueand callsprocess.StandardInput.Close(), with the comment that a child holding an open stdin it is waiting on "is a hang rather than an error".RunCommanddoes not redirect stdin, so the child inherits the service's.A child that reads stdin blocks until the token is cancelled:
/bin/sh -c "read x; echo got:[$x]" → blocked until cancellation (TaskCanceledException)CommandOptionsexposesWorkingDirectory,EnvironmentVariablesandElevation— no stdin knob.GIT_TERMINAL_PROMPT=0andGCM_INTERACTIVE=nevercover the prompting case but not a generic read, so adopting this as-is reintroduces the hang the current code deliberately guards against, in a long-running service.2. Strict UTF-8 reports a decode failure only sometimes
This is the more serious one, because the failure mode is silence. With
new UTF8Encoding(false, throwOnInvalidBytes: true)passed asOutputHandler.Encoding, four runs each:git output result invalid bytes surrounded by valid text ( before\n\xFF\xFE\nafter\n)AggregateExceptionwrappingDecoderFallbackException— 4/4invalid bytes only ( \xFF\xFE)exit=0, empty output, no exception — 4/4Today
ReadToEndAsyncwithStandardOutputEncoding = StrictUtf8throwsDecoderFallbackExceptionin both cases, andGitRunnerturns that into an explicitGitResult: "git produced output that is not valid UTF-8, so it cannot be read without guessing."After the swap, the second row becomes an empty successful result. That is precisely the outcome the
StrictUtf8comment says the strict encoding exists to prevent — output this service cannot represent being reported as something that "looks fine and matches nothing". An undecodable branch or path name would read as no branches rather than as an error.Note also that even the detected case arrives wrapped in
AggregateException, so the existingcatch (DecoderFallbackException)would not catch it unchanged.On
ktsu.GitIntegrationChecked as the previous triage note asked. It is not the alternative here: the point of
GitRunneris how git is invoked — the credential never touching a command line, system and global config switched off, inheritedGIT_*dropped — not which porcelain is run. A library that models git operations would have to expose that same environment protocol to be usable, so it does not remove the need for this layer.What would unblock it
Either an upstream
ktsu.RunCommandchange — a stdin option onCommandOptions, and a decode failure raised consistently regardless of whether any valid text accompanied the bad bytes — or an explicit decision here to accept both differences. The second is a judgement call about a credential-handling service's failure modes, so it wants a maintainer rather than a drive-by refactor.The
DrainTimeoutcaveat the issue already raises is still open too, and is the least of the three.Leaving this unassigned. The reuse itself is still worth doing once the stdin and decode gaps are settled; nothing above argues against the direction, only against doing it blind today.
Generated by Claude Code
matt-edmondson commented
on Sep 25, 2026 ContributorAuthorMore actionsBoth upstream blockers are now addressed
The 2026-09-21 triage named two things that would have to change in
ktsu.RunCommandbefore this swap could preserveGitRunner's guarantees. Re-checked both against currentmain(d06609e, v1.6.2):The decode gap is already fixed. It was measured against 1.5.23; v1.6.0's "Honour
OutputHandler.Encodingregardless of a byte order mark" addressed it.AsyncProcessStreamReadernow builds its readers withdetectEncodingFromByteOrderMarks: false, and its own comment names exactly the case recorded here — output startingFF FEbeing decoded as UTF-16LE whateverOutputHandler.Encodingsaid, so a strict encoding reported no error on bytes it should have rejected. Nothing left to do for this one.The stdin gap was still live, and now has a PR. Confirmed it reproduces: with the caller's standard input a pipe held open with no data, a command that reads standard input waits there and the call ends only on cancellation —
TaskCanceledExceptionafter 8.0s against an 8-second token. With standard input at/dev/nullit returns in 0.1s, which is why this does not show up on a developer's machine.Filed as
ktsu-dev/RunCommand#81and fixed inktsu-dev/RunCommand#82:CommandOptions.StandardInputtakes aStandardInputModeofInherit(the default, unchanged behaviour) orClosed, which redirects standard input and closes it so a read reports end of stream. Same thingGitRunnerdoes by hand today. Measured after the change: 8.0s hang → 0.1s completion.Worth noting one detail from that work, because it bears on how this repo would test its own adoption: redirecting without closing is not a fix. It leaves the command holding a pipe nobody writes to, which is the same wait as inheriting.
GitRunner's existingprocess.StandardInput.Close()is doing real work, not tidying up.What remains before this issue is actionable. The
DrainTimeoutcaveat the issue raises is untouched and is still the least of the three. Once #82 lands and ships, the swap needsCommandOptions { StandardInput = StandardInputMode.Closed }at each call site to keep the current no-hang guarantee — it is not the default, deliberately, since changing that would break callers relying on inheritance. Whether it should be the default is raised in #82 and left as a maintainer's call.Not assigning myself; nothing branched here.
Generated by Claude Code
matt-edmondson commented
on Sep 26, 2026 ContributorAuthorMore actionsTriage (re-run after 2026-09-25 update)
- Category: Improvement (cross-library reuse)
- Priority: Low
- Assignment: Repo maintainer. This depends on the RunCommand release cadence.
- Status: One upstream blocker is fixed and the other has a fix up:
- The strict-UTF-8 decode gap is fixed in RunCommand v1.6.0.
- The stdin hang was filed as RunCommand#81, and Let a caller close a command's standard input RunCommand#82 fixes it with
StandardInputMode.Closed.
- Remaining caveat: The
DrainTimeoutbehaviour is still open. - Duplicates / related: Part of the reuse audit in Cross-library reuse audit log .github#15.
- In progress: No open PR in this repo.
Next step: Once a RunCommand release ships #82, bump the dependency and delegate git process invocation to RunCommand. Decide whether
DrainTimeoutblocks that or can be a follow-up.
Generated by Claude Code
matt-edmondson commented
on Sep 28, 2026 ContributorAuthorMore actionsDecision (maintainer, 2026-09-28)
- Adopt
ktsu.RunCommandonce a release containing Let a caller close a command's standard input RunCommand#82 (StandardInputMode.Closed) ships. DrainTimeoutis a follow-up, not a blocker. If bounded post-kill draining turns out to matter, fix it upstream in RunCommand rather than keeping local plumbing. This follows the org-wide reuse rule "prefer reuse, accept costs; fix gaps upstream" (Cross-library reuse audit log .github#15).
Next reader: blocked on a RunCommand release that includes #82. Then bump the dependency and delegate
GitRunner.RunAsynctoRunCommand.ExecuteAsync, settingStandardInput = StandardInputMode.Closedat each call site. Keep theGIT_*environment overlay and the strict UTF-8 handling, and re-run the timeout and cancellation tests.
Generated by Claude Code
- Adopt
What's hand-rolled
GitRunner.RunAsyncstarts git as a child process by hand: builds aProcess/ProcessStartInfo, redirects stdout/stderr, racesWaitForExitAsyncagainst a timeout, kills the whole process tree on timeout or cancellation, and drains the output streams with a bounded grace period afterward:GitBranchStateCache/GitBranchStateCache/Git/GitRunner.cs
Lines 64 to 118 in 41b0d95
The kill/drain/timeout machinery (
Kill,DrainAsync, the linked-cancellation-token dance) is generic process-lifecycle plumbing, not anything specific to git:GitBranchStateCache/GitBranchStateCache/Git/GitRunner.cs
Lines 120 to 157 in 41b0d95
What ktsu.RunCommand provides
ktsu.RunCommand.RunCommand.ExecuteAsync(string fileName, IEnumerable<string> arguments, OutputHandler outputHandler, CommandOptions options, CancellationToken cancellationToken):https://github.com/ktsu-dev/RunCommand/blob/cbf667de10ee14073e1198ce5125ad176a7903df/RunCommand/RunCommand.cs#L262-L271
It already does exactly the same core sequence
GitRunnerhand-rolls: starts the process with an argument list (no shell, no quoting), redirects and reads stdout/stderr concurrently (AsyncProcessStreamReader), and on cancellation kills the entire process tree (TryKill,entireProcessTree: trueon non-netstandard2.x targets) before rethrowingOperationCanceledException:https://github.com/ktsu-dev/RunCommand/blob/cbf667de10ee14073e1198ce5125ad176a7903df/RunCommand/RunCommand.cs#L346-L389
CommandOptionsalso covers the two other knobsGitRunnerneeds:WorkingDirectory(AbsoluteDirectoryPath) — https://github.com/ktsu-dev/RunCommand/blob/cbf667de10ee14073e1198ce5125ad176a7903df/RunCommand/CommandOptions.cs#L20-L27EnvironmentVariables(an overlay dictionary,nullvalue removes a key, matchingProcessStartInfo.Environmentsemantics) — https://github.com/ktsu-dev/RunCommand/blob/cbf667de10ee14073e1198ce5125ad176a7903df/RunCommand/CommandOptions.cs#L29-L38OutputHandler.Encodingaccepts a customEncoding, so the strict, throw-on-invalid-bytesUTF8EncodingGitRunnerbuilds today can be passed straight through:https://github.com/ktsu-dev/RunCommand/blob/cbf667de10ee14073e1198ce5125ad176a7903df/RunCommand/OutputHandler.cs#L11-L14
Why it's worth it
RunCommand's cancellation path was specifically hardened against the exact raceGitRunnerguards against by hand: its own CLAUDE.md documents that killing the process can let the "normal exit" path win the race against a cancelled wait, which would return the killed process's exit code and throw nothing — the fix is aThrowIfCancellationRequested()re-check after the await, covered by a regression test that repeats a 1 ms cancellation 50 times.GitRunnerreimplements the same shape (kill, then decide what to report) without that test coverage backing it. Delegating removes ~90 lines of process start/kill/drain plumbing (RunAsync,Kill,DrainAsync,BuildStartInfo's non-environment parts) and leavesGitRunnerholding only what's actually git-specific: theGIT_*/GIT_CONFIG_*environment protocol and the git-vs-caller timeout/cancellation distinction, built as aCommandOptions.EnvironmentVariablesoverlay and a linkedCancellationTokenSourcearound theExecuteAsynccall.Compatibility
ktsu.GitBranchStateCache) targets:net10.0only (its.csprojpins<TargetFramework>net10.0</TargetFramework>deliberately, as an ASP.NET Core component).ktsu.RunCommandtargets:net10.0;net9.0;net8.0;net7.0;net6.0;net5.0;netstandard2.0;netstandard2.1(per its own CLAUDE.md) — coversnet10.0, and the process-tree kill (entireProcessTree: true) is available on exactly the non-netstandard2.xtargets, i.e. it applies onnet10.0.ktsu.RunCommand'sDirectory.Packages.propsreferences onlyktsu.Semantics.Paths,ktsu.Semantics.Strings,Polyfill,System.Memory,System.Threading.Tasks.Extensions— no dependency onktsu.GitBranchStateCache, so no cycle.Sketch
Before (
GitRunner.RunAsync, abbreviated):After (sketch —
ApplyEnvironment'sGIT_*protocol stays as a helper building the overlay dictionary):Caveats
RunCommand'sOutputHandlerdelivers raw, undelimited chunks rather than the single joined stringGitRunnerbuilds withReadToEndAsync. Reassembling the full text needs aStringBuilderper stream in the caller, as sketched above — a small but real difference from today's shape.DecoderFallbackException) from the strict UTF-8 encoding would need to be caught around theExecuteAsynccall instead of around twoTask<string>awaits;GitRunner's current message for that case ("git produced output that is not valid UTF-8...") would need to move to that catch block.RunCommanddoes not expose a bounded post-kill drain timeout (DrainTimeoutinGitRunnertoday) as a separate knob — itsAsyncProcessStreamReaderreads until the process's pipes close after being killed, which is normally immediate but isn't independently time-boxed the wayGitRunner's explicit 5-secondDrainAsyncis. Worth confirming this doesn't reintroduce the grandchild-holds-the-pipe-open case the currentDrainTimeoutcomment calls out.GitRunner/IGitRunnerarepublictypes but this repo is a service, not a library consumed by other ktsu packages), so no downstream break, but it is worth re-running the existingGitRunner/GitBranchStateCache.Testssuite (particularly around timeout and cancellation) after the swap given the subtlety noted above.