Addresses a Gosec G204 (Command Injection) warning where provider command strings were passed directly into `sh -c` and `cmd /C`. Since these commands originate from the user's trusted local config and explicitly require shell features (like pipes and variable expansions) to function, removing the shell invocation introduces a breaking change. This adds a documented `#nosec` annotation to suppress the false positive instead of degrading functionality.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
providerCommandTimeout bounded only the wait, so the call could take arbitrarily
longer than the 5s its error text promised. Two phases sat outside it.
Starting the process ran before the clock began. On Windows that is
CREATE_SUSPENDED, job-object creation and assignment, and a system-wide thread
snapshot, none of it fast on a cold or contended machine. The deadline is now
armed first and the start races it, so process creation is inside the budget.
Draining after termination was unbounded too: the timer path called Terminate
and then received on the wait with no limit. Terminate normally makes that
immediate, which is exactly why it survived unnoticed, but a tree that resists
termination held the call open long past the deadline that had just expired.
That receive is now bounded, and a call that gives up leaves cmd.Wait running,
so the output buffers are synchronised: the I/O pump keeps writing into buffers
the caller already holds, which a plain bytes.Buffer cannot survive.
The timeout error also carried nothing actionable. It was a bare string with the
underlying error and the command's stderr both dropped, so a user whose provider
command was slow got no clue which phase ran out or what the command had said.
It now names the phase, wraps the cause, and includes the redacted output the
failure path already had.
Both bounds are asserted on extracted helpers rather than end to end, because
end to end they are untestable: a terminated command stops writing and its wait
returns at once, so an unbounded drain and an unsynchronised buffer both pass a
whole-call test. The first version of these tests did exactly that and proved
nothing. Removing either bound now fails, as does arming the deadline after the
start, which reports the wrong phase.
Both background-child tests fail intermittently on the Windows runner, and
have since #690. #800 and #802 each relaxed an assertion, which is why neither
held: the deadline itself was the problem.
The tests drove LoadProviderCommand, so the fixture's own startup was billed to
the production 5s budget. On a loaded runner the outer timer won the select in
runProviderCommand, and both symptoms follow from that. The nonzero-exit test
saw a timeout instead of the exit status it asserts, because the timer
preempted the shell's exit. The other test lost its PID file, because the
timer's Terminate killed the background child before it could record a PID,
putting the file permanently out of reach however long the assertion polled.
Neither test is a claim about the 5s budget. They assert which error each
cmd.Wait path yields and that the leftover descendant is killed, so they now
call runProviderCommand directly with a budget of their own and the production
deadline stops competing with process startup. Making the child outlive that
budget lets both drop their elapsed-versus-lifetime bounds as well, so no
wall-clock assertion is left in either test.
That is only worth doing if the two timeout paths can be told apart, and they
could not: the WaitDelay path and the outer-timer path both returned the same
bare sentinel, so asserting on it would have been satisfied by the very timeout
the tests exist to rule out. The WaitDelay path now wraps rather than replaces.
Callers asking errors.Is for the sentinel are unaffected, so no user-visible
message changes.
The Windows fixture also waited for its ready marker by launching a second
PowerShell. That cold start is pure overhead added after the PID is already
written, so the wait now runs in cmd itself and is bounded.
Verified by looping both tests under CPU contention: 7 failures in 16
iterations before, none in 32 after, with per-iteration time going from a
spread of 2.3s to 66s down to a steady 2.1s. Removing the wrap makes the
WaitDelay assertion fail, so it is load-bearing rather than decorative.
* fix(config): kill provider-command process tree via job object on Windows
taskkill /T walks the process tree by parent PID in user space and can
miss descendants, leaving the sleeping child holding the inherited
stdout/stderr pipes so cmd.Wait() blocked until natural completion
(~10s instead of the 5s timeout).
Assign the started command to a job object with
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE and terminate the job on timeout so
the kernel kills the whole tree atomically; taskkill remains as a
fallback. Also set cmd.WaitDelay so Wait can never hang on pipes held
by an escaped orphan, and add a regression assertion that the sleeping
child is actually gone after the timeout returns.
Fixes#683
Co-Authored-By: Claude Fable 5 <[email protected]>
* fix(config): harden provider-command tree termination
Address code review on PR #690: terminate the process tree when
cmd.Wait() returns exec.ErrWaitDelay (a background descendant kept
inherited pipes open, previously left running); start the Windows
process suspended and assign it to the job object before resuming its
main thread, closing the window where a fast child could escape the
job; drop JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE so a successful command's
detached helpers survive Close() instead of being reaped; escape
single quotes in generated PID-file paths; and correct a misnamed
Windows exit-code constant in a comment.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
* fix(config): avoid gofmt smart-quote rewrite in doc comment
gofmt's doc-comment formatter rewrites a trailing '' into a curly
closing quote, which CI's gofmt -l check flags as unformatted. Reword
to avoid the pattern instead of embedding a raw quote pair.
* fix(config): terminate leaked descendants on nonzero exit, avoid PID reuse
Terminate the process tree on any error returned from cmd.Wait, not only
exec.ErrWaitDelay. Go only returns ErrWaitDelay when the shell itself
exited successfully; a nonzero exit (e.g. sleep 600 & exit 7) yields the
bare *ExitError instead, so a detached background child that held the
inherited pipes open was never terminated on that path.
Also stop falling through to the taskkill/PID fallback once a job handle
successfully contained the tree: by the time Terminate runs on the
ErrWaitDelay path, cmd.Wait has already reaped the root process, so its
PID may have been reused by an unrelated process, and taskkill /T /PID
against a reused PID would force-kill the wrong tree.
Addresses PR #690 review feedback.
* fix(config): surface suspended-thread resume failures instead of hanging
resumeMainThread now reports whether it actually resumed at least one
thread. If Toolhelp/OpenThread/ResumeThread all fail, the launched
process would otherwise sit suspended forever, never exit on its own,
and get misreported as a plain 5s provider-command timeout instead of
the real resume failure. attachCommandProcess now terminates the
process immediately in that case.
Also documents the known, non-regressive limitation that the taskkill
fallback (used only when job creation/assignment itself fails) can
still miss a descendant that has reparented away from the tree it
walks by PID.
Addresses remaining PR #690 review feedback; PR description corrected
separately to drop the outdated JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE claim.
* fix(config): guard taskkill with retained process identity
* fix(config): retain POSIX process group identity
* test(tui): match provider wizard shortcut
* test(tui): pin credential-step env for deterministic footer assertion
The credential step's footer only shows the /→ shortcut once a key
is present, whether typed or inherited from the provider's
AuthEnvVars. The previous commit changed the assertion to
"Enter/→ continue" without accounting for that, so the test only
passed by coincidence when ANTHROPIC_API_KEY happened to be set in the
environment — failing deterministically on a clean CI runner (as
smoke reported on all three OSes). Pin the env var empty and assert
the no-key footer text so the test is correct regardless of ambient
environment.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
* fix(config): preserve provider process attributes
* test(config): keep SysProcAttr coverage portable
---------
Co-authored-by: Claude Fable 5 <[email protected]>
