Fix Fix/Feature button to use CLI path from settings

[PureWeen]

The Fix / Feature button's \LaunchCopilotInTerminal\ method hardcoded \copilot\ as the executable name, which fails when copilot isn't installed globally on the system PATH.

Fix: Resolve the CLI path via \CopilotService.ResolveCopilotCliPath(settings.CliSource)\ using the user's configured setting, matching the pattern already used by \OpenInCopilotConsole\ in \SessionListItem.razor. On Windows the path is PS-escaped; on macOS/Linux it uses \PlatformHelper.ShellEscape().

[PureWeen]

🔍 PR #326 Review — Multi-Model Consensus (5 models, 2+ agreement filter)

PR: Fix Fix/Feature button to use CLI path from settings
CI: No checks reported
Existing reviews: None (first review)

---

🔴 CRITICAL — CopilotService.cs:777 — Compile error: ResolveCopilotCliPath is private

3/5 models flagged this.

// CopilotService.cs:777
private static string? ResolveCopilotCliPath(CliSourceMode source = CliSourceMode.BuiltIn)

The PR calls CopilotService.ResolveCopilotCliPath(settings.CliSource) from SessionSidebar.razor (a different class), but the method is private static. This will not compile.

Fix: Change visibility to internal static (minimum required for cross-class access within the assembly).

---

⚠️ Claim in PR Description is Inaccurate

The PR description states the fix "matches the pattern already used by OpenInCopilotConsole in SessionListItem.razor". Verified: OpenInCopilotConsole still hardcodes copilot (line ~384: $"copilot --resume '{psId}' --yolo\n"). It does not use ResolveCopilotCliPath. The fix approach is correct; the justification is not.

---

🟡 MODERATE — SessionSidebar.razor:~2089 — Silent failure after user action

3/5 models flagged this.

if (string.IsNullOrEmpty(cliPath)) return;

The caller sets footerStatus = "⏳ Launching copilot…" before calling LaunchCopilotInTerminal. If cliPath is null/empty, the method returns silently and the status stays frozen (or updates to "✓ Copilot launched" depending on control flow), giving the user no indication that the launch failed.

Fix: Either set an error in footerStatus before returning, or throw so the caller's catch block handles it.

---

✅ Correct Aspects (All 5 models agree)

• PowerShell escaping: cliPath.Replace("'", "''") + & '...' invocation is correct for PS single-quoted strings. Handles spaces, $, backticks, backslashes.
• Unix escaping: PlatformHelper.ShellEscape(cliPath) correctly single-quotes and handles embedded ' via '\"'\"' substitution. Safe for all shell metacharacters.
• Core logic: Resolving the path from settings is the correct fix — prevents failure on systems where copilot isn't on the global PATH.
• Pre-existing issues (workingDir, promptFile unescaped): Not introduced by this PR.

---

Recommended Action: ⚠️ Request Changes

Required before merge:

1. Make ResolveCopilotCliPath (currently — compile error)
2. Surface an error to the user when cliPath is null/empty instead of silently returning

[PureWeen]

🔍 PR Review Squad — Round 2 Re-review

New commit reviewed: dc30d596 Make git fetch best-effort in Fix/Feature flow

Previous Findings Status

#	Finding	Round 1	Status
1	🔴 ResolveCopilotCliPath is private — compile error	CRITICAL (3/5)	✅ FIXED — now internal static (line 857)
2	🟡 Silent failure when CLI path not found	MODERATE (3/5)	⚠️ STILL PRESENT
3	✅ Shell escaping correctness (PS + Unix)	CLEAN (5/5)	✅ Still clean

Finding #2 Detail

LaunchCopilotInTerminal (line ~2514) does if (string.IsNullOrEmpty(cliPath)) return; silently. But the caller at line ~2441 unconditionally proceeds to footerStatus = "✓ Copilot launched in new terminal" — so the user sees a false success message when the CLI binary cannot be found.

Suggested fix (either approach works):

• Option A: Have LaunchCopilotInTerminal throw when cliPath is null, so the existing catch block at line ~2454 handles it with footerStatus = "✗ ...".
• Option B: Return a bool from LaunchCopilotInTerminal and check it before setting the success status.

New Commit Analysis

dc30d596 wraps git fetch in try/catch in both SessionSidebar.razor (Fix/Feature flow) and RepoManager.cs (worktree creation). This makes the feature work offline from cached refs. Clean implementation — no new issues.

CI Status: ⚠️ Pre-existing failures (not PR-specific)

Verdict: ⚠️ Request changes

One remaining fix: add user feedback when CLI path resolution fails (Finding #2).

[PureWeen]

🔍 PR Review Squad — Round 3 Re-review (post-fix)

Fix commit: 95102f19 fix: throw when CLI path not found in LaunchCopilotInTerminal

Previous Findings Status

#	Finding	Round	Status
1	🔴 ResolveCopilotCliPath is private — compile error	R1	✅ FIXED (author, earlier commit — now internal static)
2	🟡 Silent failure when CLI path not found	R1-R2	✅ FIXED (this commit — throws InvalidOperationException)
3	✅ Shell escaping correctness (PS + Unix)	R1	✅ Clean across all rounds

Fix Verification

LaunchCopilotInTerminal now throws InvalidOperationException("Copilot CLI binary not found. Check Settings → CLI Source.") when cliPath is null/empty. The caller's existing catch (Exception ex) block at line ~2475 catches this and sets footerStatus = "✗ {ex.Message}" — giving the user a clear error message instead of a false success.

Verdict: ✅ Approve

All findings from Rounds 1-3 are resolved. Ship it. 🚢