chore: simplify CLI installation detection internals

[hatayama]

Summary

• Keep CLI installation and PATH setup detection behavior unchanged while consolidating child-process execution in one owner.
• Separate detection policy from process startup, output capture, timeout cleanup, and exit reporting.

User Impact

• No user-visible behavior changes are intended.
• Shell-visible CLI detection, package-owned fallback selection, version parsing, timeout behavior, and cancellation cleanup remain unchanged.

Changes

• Add CliDetectionCommandRunner as the sole owner of detection process startup, ordered stdout capture, stderr draining, cancellation registration, timeout kill, output flush, disposal, and exit-code reporting.
• Return ordered stdout lines and the exit code so CliInstallationDetector retains its two existing policies: login-shell marker output preserves line boundaries and ignores the outer shell exit code, while direct version probes concatenate lines and reject non-zero or empty output.
• Move the already-exited process cleanup race test to the runner fixture and add characterization for multiline output, non-zero exit output, and missing executables.
• The direct version probe's existing exception recovery now surrounds runner setup as well as waiting. This is behaviorally equivalent because missing executables already return null, redirected output is constructed as a runner precondition, and the caller-owned cancellation token source remains live during execution; avoiding a mutable start/wait session keeps a single execution path.

Verification

• Red: new runner contract produced 7 expected missing-type/member compile errors before implementation.
• Unity compile: 0 errors, 0 warnings.
• CliDetectionCommandRunnerTests: 4/4 passed.
• CliInstallationDetectorTests: 14/14 passed.
• CliSetupApplicationServiceTests: 4/4 passed.
• CliPathSetupFlowTests: 4/4 passed.
• StaticFacadeStateGuardTests: 20/20 passed.
• git diff --check origin/v3-beta...HEAD: passed.
• Process start, async stdout/stderr capture, wait/flush, timeout cleanup, and kill now occur in one production implementation.

Compatibility

• Shell command text, output markers, parsing, cache behavior, platform/PATH policy, warning text, and public APIs are unchanged.
• No IPC request/response shape or protocol declaration changed; no protocol version bump is required.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR extracts process execution logic from CliInstallationDetector into a new CliDetectionCommandRunner class with a CliDetectionCommandResult data holder, handling process start, async stdout capture, timeout, and cancellation-driven kill. CliInstallationDetector now delegates to this runner, and its associated unit tests are relocated to a new test fixture.

Changes

CLI detection command runner extraction

Layer / File(s)	Summary
CliDetectionCommandResult and Execute implementation Packages/src/Editor/Infrastructure/CLI/CliDetectionCommandRunner.cs	New CliDetectionCommandResult holds stdout lines and exit code; Execute starts a process, captures stdout asynchronously, applies a 5s timeout, kills on cancellation, and returns null on failure/timeout.
CliInstallationDetector migration to runner Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs	Shell-based detection and ExecuteCliVersionCommand now call CliDetectionCommandRunner.Execute; local ExecuteAndGetOutput, KillProcessIfRunning, and PROCESS_TIMEOUT_MS are removed.
Tests relocated to CliDetectionCommandRunnerTests Assets/Tests/Editor/CliDetectionCommandRunnerTests.cs, Assets/Tests/Editor/CliInstallationDetectorTests.cs	New test fixture covers stdout ordering, exit codes, missing executables, and kill-after-exit; equivalent test removed from CliInstallationDetectorTests.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CliInstallationDetector
  participant CliDetectionCommandRunner
  participant Process

  CliInstallationDetector->>CliDetectionCommandRunner: Execute(startInfo, ct)
  CliDetectionCommandRunner->>Process: Start() and redirect stdout/stderr
  Process-->>CliDetectionCommandRunner: OutputDataReceived (stdout lines)
  alt process completes within timeout
    CliDetectionCommandRunner->>Process: WaitForExit() to flush buffers
    CliDetectionCommandRunner-->>CliInstallationDetector: CliDetectionCommandResult(lines, exitCode)
  else timeout or cancellation
    CliDetectionCommandRunner->>Process: KillProcessIfRunning()
    CliDetectionCommandRunner-->>CliInstallationDetector: null
  end

Loading

Possibly related PRs

• hatayama/unity-cli-loop#1131: Both PRs touch the CLI installation detection flow's process execution/termination race handling in CliInstallationDetector, directly related to the new CliDetectionCommandRunner's kill/timeout behavior.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: simplifying CLI installation detection internals.
Description check	✅ Passed	The description is directly related to the changes and accurately summarizes the refactor and test updates.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/hatayama/isolate-cli-detection-command-runner

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs (1)

490-510: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Keep unexpected runner failures fail-fast.

CliDetectionCommandRunner.Execute already maps expected “no result” cases to null; this broad catch now also hides unexpected runner bugs introduced by the extraction. Please narrow this to the expected process/detection exceptions, or let unexpected exceptions propagate.

Based on learnings, avoid broad defensive try-catch for unexpected exceptions and catch only expected, domain-specific exceptions at the appropriate layer.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs` around
lines 490 - 510, The broad try/catch in CliInstallationDetector’s CLI version
detection helper is swallowing unexpected bugs from
CliDetectionCommandRunner.Execute, which should fail fast instead of being
hidden. Narrow the catch in the detection path to only the expected
process/detection exceptions handled by Execute, or remove it so unexpected
exceptions propagate, while keeping the existing null-return handling for normal
“no result” cases.

Source: Learnings

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs`:
- Around line 490-510: The broad try/catch in CliInstallationDetector’s CLI
version detection helper is swallowing unexpected bugs from
CliDetectionCommandRunner.Execute, which should fail fast instead of being
hidden. Narrow the catch in the detection path to only the expected
process/detection exceptions handled by Execute, or remove it so unexpected
exceptions propagate, while keeping the existing null-return handling for normal
“no result” cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: dfbc9111-ab10-449c-adcc-fb1290150dd8

📥 Commits

Reviewing files that changed from the base of the PR and between dde189d and 157fcc6.

⛔ Files ignored due to path filters (2)

• Assets/Tests/Editor/CliDetectionCommandRunnerTests.cs.meta is excluded by none and included by none
• Packages/src/Editor/Infrastructure/CLI/CliDetectionCommandRunner.cs.meta is excluded by none and included by none

📒 Files selected for processing (4)

• Assets/Tests/Editor/CliDetectionCommandRunnerTests.cs
• Assets/Tests/Editor/CliInstallationDetectorTests.cs
• Packages/src/Editor/Infrastructure/CLI/CliDetectionCommandRunner.cs
• Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs

💤 Files with no reviewable changes (1)

• Assets/Tests/Editor/CliInstallationDetectorTests.cs

[hatayama]

CodeRabbit nit disposition: no code change.

The broad catch remains at the external process boundary, where it logs the failure and returns the existing explicit null result. This is the pre-existing direct-version detection contract. The extraction only expands the catch over setup failures that are already prevented by ProcessStartHelper.TryStart, the runner's redirected-output preconditions, and the live caller-owned cancellation token. Narrowing to exception types without evidence of a smaller expected set would change abnormal-path behavior in this behavior-preserving refactor. Fable's independent final review confirmed this disposition and found no blocker or nit.