Repository navigation
fix(tools): drop a failed version probe's stderr - #2498
Conversation
A tool that crashes leaves on stderr the warnings of whatever it loaded. ansible-lint exited 3 with only ansible-core's UNSUPPORTED warning to read, and the probe reported it installed at ansible-core's 2.21.5. ProbeRunner keeps stdout then stderr, but gives a failed run's stdout alone. It is the default probe for dotf tools and, through a new System.VersionOutput seam, for doctor's semverOf, which read a merged CombinedOutput stream. Closes #2494
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| out, err := sys.CommandOutput(n, args...) | ||
| return []byte(out), err | ||
| }) | ||
| v := tools.ProbeVersion(name, sys.VersionOutput) |
There was a problem hiding this comment.
Possible Issue
semverOf now passes sys.VersionOutput to tools.ProbeVersion, which substitutes tools.ProbeRunner when the runner is nil — so a System built without VersionOutput bypasses the CommandOutput fake and shells out to the real binary. In this PR only realSystem() and newSys populate the field; any other doctor test helper or inline System literal that fakes CommandOutput with a version banner but omits VersionOutput will read the machine's actual installed version (test passes with the wrong value) or get "" → "unknown" instead of the injected banner. THEORETICAL: the diff shows no such second constructor, so this cannot be confirmed from the hunks alone — grep for System{ literals outside realSystem/newSys before merging.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
⚙️ Agent run details
|
A nil VersionOutput made ProbeVersion fall back to the real ProbeRunner, so a test System that faked CommandOutput would run the machine's binary. Nil now means CommandOutput.
Review triageReviewers: PR-Agent reviewed
|
1 participant
Summary
The from-zero run behind BUG-138 reported
ansible-lint: reports 2.21.5 after install, below the pin 26.9.0. ansible-lint had crashed with exit 3, and2.21.5came from ansible-core'sUNSUPPORTEDwarning on stderr. The probe read stdout then stderr and kept the output of a failed run.tools.ProbeRunner: stdout then stderr, as before, but a failed run gives its stdout alone. It is the default probe ofProbeVersionArgs, sodotf tools install's three probe sites use it, anddotf tools versiontoo.tools.ExecRunneris unchanged. herdr formats a failed command's stderr into its errors, and that stays.semverOf(copilot, opencode, pi) went throughCommandOutput, aCombinedOutput. That stream was also interleaved, against the stdout-first rule. It now goes through a newSystem.VersionOutputseam,tools.ProbeRunnerin production. ASystemwithout the seam uses itsCommandOutput, so a test never runs the real binary.Closes #2494
Not in this PR:
MiseSync.runsAtPinprobes through the mergedmiserunner, the same class with a boolean result. Filed as BUG-142 (#2496), sinceMiseSyncneeds its own probe seam.Verification
New cases in
TestProbeRunner_ReadsStdoutBeforeStderrrun realsh -cscripts:echo 'UserWarning: UNSUPPORTED: ansible-core 2.21.5 ...' >&2; exit 3gives"", andecho 'tool 2.3.4'; echo 'error: ...' >&2; exit 1gives2.3.4.Mutations, each caught:
ProbeRunnerkeeps stderr on failure:TestProbeRunner_ReadsStdoutBeforeStderrandTestRealSystem_VersionOutputDropsAFailedRunsStderrfail.toolsVersionRunnerback totools.ExecRunner:TestToolsVersion_DropsAFailedRunsStderrfails.VersionOutput:TestSemverOf_WithoutTheSeamUsesCommandOutputfails.Production diff: 49 lines.
Knowledge