fix: codespace "New Session" button silently fails

[btessiau]

Summary

Fixes #314 — Clicking "➕ New Session" on a connected codespace group (green dot) silently does nothing.

Note: This PR depends on #308 (codespace integration). Merge #308 first, then this cleanly applies on top.

Root Cause

Three independent bugs combine to create this silent failure:

#	Bug	Impact
1	Fire-and-forget async lambda — @onclick handler is void-returning, so Blazor never awaits QuickCreateSessionForCodespace	Exception silently swallowed
2	Error in wrong UI section — createError only renders in CreateSessionForm at top of sidebar, not codespace section	Error invisible to user
3	Stale client undetected — Health check only checks tunnel.IsAlive + ContainsKey(), never probes remote port	Green dot stays after remote copilot dies

Changes

SessionSidebar.razor

• Changed void lambda to async () => { ... await QuickCreateSessionForCodespace(...) } so Blazor properly awaits and re-renders
• Added codespace-create-error banner below codespace group sessions (click to dismiss)

SessionSidebar.razor.css

• Added .codespace-create-error style

CopilotService.Codespace.cs

• Health check now does a TCP probe to the tunnel's local port when tunnel + client + Connected all pass
• If probe fails: disposes stale client, removes from _codespaceClients, falls through to reconnect path
• The green dot will correctly transition to reconnecting when the remote copilot dies

CodespaceSessionCreationTests.cs (new, 8 tests)

• Missing client → descriptive error (not "Service not initialized")
• No session state leaked on failure
• Non-codespace groups skip codespace guard
• TCP probe detects stale client (old code would skip)
• Healthy group still skips correctly
• All disconnected states block creation
• Connected state allows creation
• Error message mentions health check + retry guidance

Test Results

2,223 pass (8 new), 3 pre-existing CLI binary failures (unrelated).

Risk

• TCP probe overhead: One extra TCP connect per health check cycle (15s) per connected codespace group. Sub-millisecond for localhost.
• No behavior change for healthy connections: Probe succeeds instantly → continue as before.
• Backward compatible: No API changes.

[btessiau]

PR #315 Review — Codespace "New Session" silent failure fix

Clean, focused fix for a real three-bug interaction. The incremental diff (4 files, ~266 additions over #308) is well-scoped.

✅ All three root causes addressed

1. Fire-and-forget lambda — void → async lambda on the @onclick handler. Blazor now properly awaits and catches exceptions. Correct fix.

2. Error visibility — New codespace-create-error banner renders inline below codespace group sessions. Click-to-dismiss is good UX.

3. Stale client detection — TCP probe added to the health check continue path. When tunnel is alive but the remote port is unreachable, the stale CopilotClient is disposed and removed, falling through to reconnect.

🟡 TCP probe is simpler than IsCopilotListeningAsync — intentional?

The health check probe uses a bare TcpClient.ConnectAsync() (line 387-389), while CodespaceService.IsCopilotListeningAsync() does the more sophisticated connect-then-read-with-timeout approach to distinguish "gh accepts locally but nothing at remote end" from "copilot is truly listening."

For SSH tunnels, the simpler probe is actually correct — SSH tunnels refuse the TCP connect outright when the remote end isn't listening (unlike gh cs ports forward which accepts locally). But it's worth a one-line comment explaining why the simpler approach suffices here (SSH tunnel semantics), since the existing IsCopilotListeningAsync already documents the nuance.

🟢 Minor: shared createError variable

createError is shared between CreateSessionForm, HandleCreateSession, QuickCreateSessionForCodespace, and the new codespace error banner. A regular session creation error would briefly flash in the codespace section (and vice versa) until cleared. Not a real issue since each code path clears it at the start, but if it ever becomes annoying, a separate codespaceCreateError variable would isolate them.

🟢 Tests are well-structured

8 new tests cover the right conditions: missing client → descriptive error, no session state leak, non-codespace skip, stale client model logic, healthy skip, disconnected states block, connected allows, error message content. The HealthCheck_StaleClientCondition test validating the model logic rather than real TCP is pragmatic and correct for a unit test.

LGTM — no blocking issues. 👍

[btessiau]

Thanks for the review! Addressed both findings in 1109976:

🟡 TCP probe comment — Added a 3-line comment explaining why the bare TcpClient probe suffices for SSH tunnels (they refuse connections outright when the remote end dies), while CodespaceService.IsCopilotListeningAsync is needed for non-SSH tunnels like gh cs ports forward which accept locally regardless.

🟢 Shared createError — Split into separate codespaceCreateError variable so codespace errors only show in the codespace section and regular session errors only show in CreateSessionForm. No cross-section flashing.

Also resolved merge conflicts with origin/main. 2,226 tests pass.

[btessiau]

Re-review after 1109976

Both suggestions from my first review are addressed:

✅ TCP probe comment — Clear explanation of why bare TcpClient.ConnectAsync suffices for SSH tunnels vs. the IsCopilotListeningAsync read-probe needed for gh cs ports forward
✅ Error variable isolation — codespaceCreateError is now separate from createError, preventing cross-section error flashing

Merge conflict with #311 (collapsible unpinned sessions) resolved cleanly — codespace UI section repositioned correctly.

One housekeeping note: review.md was accidentally committed in #308 and is now on main. Not part of this PR's diff, but should be removed in a follow-up.

No new issues. LGTM 👍

[btessiau]

Review — merge conflict introduced duplicate codespace section

The merge-conflict resolution in 1109976 accidentally left two back-to-back codespace sections in SessionSidebar.razor, both rendered under identical conditions (@if (CopilotService.CodespacesEnabled && codespaceGroups.Any())):

Lines 752–937 — original section from #308, still present verbatim. The ➕ New Session button at line 833 still uses the broken void lambda:

@onclick="() => { openGroupMenuId = null; QuickCreateSessionForCodespace(gId); }"

Lines 939–1130 — new section added by this PR with the correct async/await lambda at line 1020.

Since both blocks share the same guard, every codespace group renders twice in the sidebar. The first copy's button still fire-and-forgets, so exceptions are silently swallowed from that path — and the codespaceCreateError banner only lives in the second copy, so those errors are invisible.

Fix: Remove lines 752–937 (the old duplicate). The section added at 939 is the correct one.

---

Two additional moderate issues (consensus across all 5 review models):

codespaceCreateError renders under every expanded codespace group (line 1122): it's a single string? field inside a @foreach loop — an error from group A appears under group B as well. Suggest Dictionary<string, string?> keyed by group ID.

TCP probe swallows OperationCanceledException (Codespace.cs line 395): bare catch {} around ConnectAsync(..., ct) treats app shutdown/reconnect cancellation as "port unreachable", disposing a healthy CopilotClient unnecessarily. Fix: catch (OperationCanceledException) { throw; } before the general catch.

[btessiau]

All three review items addressed in a20dad1:

1. Duplicate codespace section removed — Deleted the first copy (lines 752–937) with the broken void lambda. The second copy with async/await on QuickCreateSessionForCodespace is the sole remaining section.

2. Per-group error dictionary — Replaced the shared codespaceCreateError string with Dictionary<string, string?> _codespaceErrors keyed by group ID. Each codespace group now has independent error state; click-to-dismiss clears only that group's error.

3. TCP probe cancellation — Added catch (OperationCanceledException) { throw; } before the bare catch {} around ConnectAsync, so app shutdown/reconnect cancellation propagates instead of being swallowed as port-unreachable.

Build passes (0 errors, Mac Catalyst). Tests: 2226 passed, 3 pre-existing CliPath failures (bundled binary not present in test env).

[btessiau]

Multi-model re-review — commit a20dad1

5 models reviewed the current diff (claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex). Below is the consensus report against the previous findings.

---

Previous Findings Status

#	Severity	Finding	Status
1	🔴 CRITICAL	Duplicate codespace section + surviving void lambda	✅ FIXED
2	🟡 MODERATE	codespaceCreateError bleeds across all codespace groups	✅ FIXED
3	🟡 MODERATE	TCP probe swallows OperationCanceledException	✅ FIXED
4	🟡 MODERATE	Shared isCreating silently blocks cross-group creation	⚠️ STILL PRESENT
5	🟢 MINOR	Health check tests are tautological	⚠️ STILL PRESENT

---

Fix Details

✅ #1 — Duplicate section removed — The net diff (base → a20dad1) shows a single codespace section with the correct async () => { …; await QuickCreateSessionForCodespace(gId); } lambda. The intermediate duplicate introduced in 1109976 was cleanly reverted in a20dad1.

✅ #2 — Per-group error dictionary — codespaceCreateError: string? replaced by _codespaceErrors: Dictionary<string, string?>. Error display uses _codespaceErrors.GetValueOrDefault(group.Id) inside the loop, correctly isolating each group's error state. Click-to-dismiss clears only the target group's entry.

✅ #3 — Cancellation rethrow — catch (OperationCanceledException) { throw; } now precedes the bare catch {} in CopilotService.Codespace.cs:388–391. App-shutdown and reconnect cancellation now propagates correctly instead of being treated as port-unreachable.

---

Outstanding Issues

�� MODERATE — Shared isCreating silently blocks cross-group creation (4/5 models)

SessionSidebar.razor — isCreating field declaration + QuickCreateSessionForCodespace

isCreating remains a single component-level bool. If a user clicks ➕ New Session for group B while group A is still creating (e.g. slow SSH tunnel), the method returns at the isCreating guard with no error message and no visual feedback — the click silently does nothing. This is especially awkward because the fix to finding #3 (TCP probe) means the healthy check now has a latency of up to the TCP connect timeout.

Suggested fix:

// Replace
private bool isCreating = false;
// With
private readonly HashSet<string> _creatingGroups = new();

Then gate QuickCreateSessionForCodespace on _creatingGroups.Contains(groupId) and add/remove the specific group ID in the try/finally. This mirrors the same per-group isolation pattern already applied to _codespaceErrors.

---

🟢 MINOR — Health check unit tests are tautological (4/5 models)

CodespaceSessionCreationTests.cs:87–150 (HealthCheck_StaleClientCondition_* and HealthCheck_HealthyGroup_*)

These tests operate entirely on local bool variables that manually mirror the service logic. They never call CopilotService or RunCodespaceHealthCheckAsync. A bug in the actual health check loop (e.g. wrong condition order, wrong field name) would leave both tests green. The inline comment acknowledges this is intentional for unit testing, but the tests provide zero regression protection for the actual service code.

Not a blocker, but worth replacing the model-logic tests with a real integration test using a mock TcpListener on a free port to exercise the actual ConnectAsync path.

---

Verdict

⚠️ Request changes — the three critical/blocking issues are resolved and the PR is substantially cleaner. One moderate user-visible bug remains (isCreating silently blocks cross-group creation). The minor test gap is not a blocker but should be tracked.

Specific ask before merge: Replace the component-level isCreating: bool with a per-group _creatingGroups: HashSet<string> (or Dictionary<string, bool>) following the same pattern used for _codespaceErrors.

[btessiau]

Addressed in 99fb185:

#4 — Per-group isCreating guard — Added HashSet<string> _creatingGroups for QuickCreateSessionForCodespace. The guard now checks _creatingGroups.Contains(groupId) and adds/removes the specific group ID in try/finally. Creating a session in group B while group A is mid-create now works correctly. The shared isCreating bool is retained for non-codespace paths (main create form, resume).

#5 — Tautological health check tests — Acknowledged; not addressed in this PR. Will track separately.

Build: 0 errors (Mac Catalyst). Tests: 2226 passed, 3 pre-existing CliPath failures.

[btessiau]

Final re-review — commit 99fb185

All consensus findings from the multi-model review are resolved. Updated status:

#	Severity	Finding	Status
1	🔴 CRITICAL	Duplicate codespace section + surviving void lambda	✅ FIXED (a20dad1)
2	🟡 MODERATE	Error bleeds across all codespace groups	✅ FIXED (a20dad1 — _codespaceErrors dict)
3	🟡 MODERATE	TCP probe swallows OperationCanceledException	✅ FIXED (a20dad1 — rethrow added)
4	🟡 MODERATE	Shared isCreating silently blocks cross-group creation	✅ FIXED (99fb185 — _creatingGroups: HashSet<string>)
5	🟢 MINOR	Health check tests are tautological	⚠️ Acknowledged, tracked separately

Fix quality check on 99fb185: QuickCreateSessionForCodespace now gates on _creatingGroups.Contains(groupId) and uses Add/Remove in try/finally — correctly mirrors the per-group pattern from _codespaceErrors. The shared isCreating bool is correctly retained for non-codespace paths (main create form, session resume). Clean.

✅ Approve

All blocking issues resolved. The only open item (#5 tautological tests) is non-blocking and tracked separately. LGTM.

[btessiau]

Re-review — commit 99fb185 (5-model consensus)

Previous Findings Status

#	Severity	Finding	Status
1	🔴	Duplicate codespace section with surviving void lambda	✅ FIXED
2	🟡	codespaceCreateError bleeds across all codespace groups	✅ FIXED
3	🟡	TCP probe swallows OperationCanceledException	✅ FIXED
4	🟡	Shared isCreating silently blocks cross-group creation	✅ FIXED
5	🟢	Health check tests are tautological	⚠️ Acknowledged, deferred

---

Fix Verification

#1 Duplicate section — Confirmed via direct file inspection: grep -n "CodespacesEnabled" returns exactly one occurrence (line 752). The old duplicate introduced in 1109976 was cleanly removed in a20dad1. The single remaining section uses the correct async () => { …; await QuickCreateSessionForCodespace(gId); } lambda.

#2 Per-group errors — private readonly Dictionary<string, string?> _codespaceErrors added. Error banner renders _codespaceErrors.GetValueOrDefault(group.Id) inside the loop, correctly isolating each group. Dismiss clears only the target group's entry via _codespaceErrors.Remove(dismissId).

#3 Cancellation — catch (OperationCanceledException) { throw; } now precedes the generic catch {} in CopilotService.Codespace.cs:395. App-shutdown propagates correctly.

#4 Per-group guard — private readonly HashSet<string> _creatingGroups replaces the component-level isCreating flag for codespace creation. isCreating is retained for the main create form and resume paths (still used at 14 sites).

---

Edge Cases Verified (clean)

• _codespaceErrors[group.Id] at render line 938 after GetValueOrDefault check at 935: safe — Blazor renders single-threaded, no mutation possible mid-render.
• _creatingGroups HashSet thread safety: safe — only accessed from @onclick handlers on the UI SynchronizationContext.
• Early return before any await in QuickCreateSessionForCodespace: safe — Blazor re-renders automatically after async event handler completion, even for synchronous early returns.

---

One minor new finding (1/4 models)

Does not meet the 2+ consensus threshold but worth tracking: when the health-check loop reconnects a codespace group (state → Connected), _codespaceErrors is not cleared for that group. A "not connected" error banner set before reconnection will persist until the user dismisses it or retries creation. Harmless and click-to-dismiss works, but RefreshSessions could clear entries for groups whose state is now Connected to auto-clean.

---

Verdict

✅ All blocking issues resolved. The three original bugs (#314) are fixed, all four review findings are addressed. The only open item (#5 tautological tests) is explicitly acknowledged as a follow-up. LGTM.

[btessiau]

Re-Review — commit 165b95f (5-model consensus)

CI Status: ⚠️ No checks configured
Models: claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex

---

Previous Findings Status

#	Severity	Finding	Status
1	🔴	Duplicate codespace section with surviving void lambda	✅ FIXED
2	🟡	codespaceCreateError bleeds across all codespace groups	✅ FIXED
3	🟡	TCP probe swallows OperationCanceledException	✅ FIXED
4	🟡	Shared isCreating silently blocks cross-group creation	✅ FIXED
5	🟢	Health check tests are tautological	⚠️ Deferred (acknowledged)

---

New Finding (5/5 models flagged)

🟡 MODERATE — GetAllSessions() hidden-session gap in name dedup

SessionSidebar.razor:1371

The new commit changes the dedup from checking same-group Organization.Sessions to checking GetAllSessions(). This is the right direction — _sessions is the authoritative store that CreateSessionAsync validates against. However, GetAllSessions() filters !s.IsHidden, while CreateSessionAsync's uniqueness guard does not:

// GetAllSessions() — excludes hidden sessions:
public IEnumerable<AgentSessionInfo> GetAllSessions()
    => _sessions.Values.Select(s => s.Info).Where(s => !s.IsHidden);

// CreateSessionAsync uniqueness check — includes ALL sessions:
if (_sessions.ContainsKey(name))
    throw new InvalidOperationException($"Session '{name}' already exists.");

Realistic scenario: A squad preset defines a worker named "Main" (IsHidden = true). GetAllSessions() won't include it, so the while-loop exits and picks sessionName = "Main". CreateSessionAsync then throws "Session 'Main' already exists". The exception IS caught and displayed as a dismissible error toast — not a crash — but the message is opaque with no hint of the real cause.

Fix: Expose session names including hidden from the service (one line), then use it in the dedup:

// Add to CopilotService.cs alongside GetAllSessions():
public IEnumerable<string> GetAllSessionNames() => _sessions.Keys;

// SessionSidebar.razor:1371 — replace GetAllSessions() call:
var existingNames = CopilotService.GetAllSessionNames().ToHashSet();

This makes the dedup use exactly the same source as CreateSessionAsync.

---

Recommendation: ⚠️ Request changes (minor)

One-line service method + one-line component fix. All previous blocking issues remain resolved. Ready to merge after this fix.

[btessiau]

Fixed in b77829c:

Hidden-session gap — Added GetAllSessionNames() (exposes _sessions.Keys directly) and replaced GetAllSessions().Select(s => s.Name) in the dedup. Now matches CreateSessionAsync's uniqueness check exactly, including hidden worker sessions.

[btessiau]

PR #315 Re-Review -- Final Report (commit b77829c)

CI Status: ⚠️ No checks configured
Models used: claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex
Review rounds: 4

---

All Previous Findings: ✅ FIXED

#	Severity	Finding	Fixed In	Status
1	🔴 CRITICAL	Duplicate codespace UI section with surviving void lambda	a20dad1	✅ FIXED
2	🟡 MODERATE	codespaceCreateError renders under all expanded groups	a20dad1	✅ FIXED -- now per-group _codespaceErrors dict
3	🟡 MODERATE	TCP probe swallows OperationCanceledException	a20dad1	✅ FIXED -- catch (OperationCanceledException) { throw; } added
4	🟡 MODERATE	Shared isCreating silently blocks cross-group creation	99fb185	✅ FIXED -- now per-group _creatingGroups HashSet
5	🟢 MINOR	Health check tests are tautological	--	Tracked separately
6	🟡 MODERATE	GetAllSessions() hidden-session dedup gap	b77829c	✅ FIXED -- new GetAllSessionNames() exposes _sessions.Keys directly

---

New Findings

None. All 5 models independently found no new issues or regressions.

---

Recommendation: ✅ Approve

All consensus findings from 4 review rounds have been addressed. The PR is ready to merge.

[btessiau]

Re-review — commit b77829c (Round 3, 5-model consensus)

CI Status: ⚠️ No checks configured (draft PR)

---

Previous Findings Status

#	Severity	Finding	Status
1	🔴	Duplicate codespace section with surviving void lambda	✅ FIXED
2	🟡	codespaceCreateError bleeds across all codespace groups	✅ FIXED
3	🟡	TCP probe swallows OperationCanceledException	✅ FIXED
4	🟡	Shared isCreating silently blocks cross-group creation	✅ FIXED
5	🟢	Health check tests are tautological	⏸️ Deferred (acknowledged)
6	🟡	GetAllSessions() hidden-session gap in name dedup	✅ FIXED

---

Fix #6 Verification

GetAllSessionNames() at CopilotService.cs:3163 returns _sessions.Keys — the exact same ConcurrentDictionary that CreateSessionAsync line 1607 guards against with _sessions.ContainsKey(name). The dedup namespace and the throw condition now use the same source. Correct and complete.

Global dedup scope: The old per-group dedup (Organization.Sessions.Where(m => m.GroupId == groupId)) would have picked "Main" for group B when group A already held "Main", then CreateSessionAsync would have thrown "Session 'Main' already exists" — a confusing error. Global dedup is actually more correct: group B gets "Main 2" (surprising but functional) rather than a failed create.

TOCTOU window (2/5 models noted, below consensus threshold): Two simultaneous quick-creates across different groups could both snapshot "Main" as free, both pick "Main", and the second CreateSessionAsync throws. The _creatingGroups guard only prevents re-entry per group. Exception is caught and surfaced as a dismissible error — graceful degradation. This pre-existed this PR and is not introduced by the fix.

---

No new consensus findings.

All 3 models that completed agree: the diff is clean, all 6 previous findings are resolved or acknowledged, no new bugs.

---

Verdict

✅ LGTM — all blocking issues resolved across 3 rounds of review. Ready to merge once #308 lands on main.

[PureWeen]

Code Review — PR #315 (fix: codespace "New Session" button silently fails)

CI: ⚠️ No checks on fix/codespace-session-creation-silent-failure branch (pre-existing gap)
Prior reviews: Multiple rounds completed. All blocking issues previously resolved. This is a final pass.

---

✅ Confirmed fixed

• void @onclick lambda → async () => await (fire-and-forget fixed)
• Shared createError → per-group _codespaceErrors (error isolation fixed)
• Shared isCreating → per-group _creatingGroups HashSet (concurrent groups unblocked)
• Cancellation rethrow in TCP probe (catch (OperationCanceledException) { throw; })
• GetAllSessionNames() replacing group-scoped name lookup — this is correct: _sessions is keyed by name globally, so checking only the group was a latent duplicate-key bug

---

🟡 MODERATE — TCP probe has no per-probe timeout

CopilotService.Codespace.cs:~385 (2/5 models)

ConnectAsync("127.0.0.1", tunnel.LocalPort, ct) uses only the health-check-wide ct with no per-probe deadline. If the SSH tunnel is half-open (SYN accepted but handshake stalls — e.g. remote firewall silently dropping packets), the OS TCP timeout applies: ~20–75s on macOS/Linux. This blocks the entire health check foreach loop, so every subsequent codespace group goes uninspected until the OS rejects the connection.

Fix: Use a short-lived linked CTS for the probe only:

using var probeCts = CancellationTokenSource.CreateLinkedTokenSource(ct);
probeCts.CancelAfter(TimeSpan.FromSeconds(3));
await probe.ConnectAsync("127.0.0.1", tunnel.LocalPort, probeCts.Token);

Catch OperationCanceledException from the probe CTS, but only rethrow if the outer ct is cancelled — a probe timeout should be treated as unreachable, not as a loop abort.

---

Test coverage gaps

• No test validates that the TCP probe correctly handles a half-open tunnel (SYN accepted, no RST) — the existing test simulates only the portReachable=false outcome via boolean logic, not an actual socket.
• No test for the async/await fix specifically (confirming exceptions from QuickCreateSessionForCodespace surface as _codespaceErrors rather than being swallowed).

---

Verdict

⚠️ Request changes — one MODERATE finding: add a 3–5s per-probe timeout to prevent the health check loop from stalling on half-open SSH tunnels. All other fixes are correct and well-structured.

[PureWeen]

🔍 PR Review Squad — Round 5 Re-review

Commit reviewed: 3a740df5 fix: add 3s per-probe timeout to TCP health check

Previous Finding Status

Finding	Status
🟡 TCP probe lacks per-probe timeout (Round 4)	✅ FIXED
6 findings from Rounds 1-3	✅ All confirmed FIXED in Round 4

Fix Verification

The implementation correctly addresses the timeout gap:

• CreateLinkedTokenSource(ct) + CancelAfter(3s) bounds each probe to 3 seconds while still honoring service shutdown
• Both TcpClient and the linked CancellationTokenSource are properly using-disposed (prevents callback registration leaks on the parent token)
• Catch chain correctly ordered: when (!ct.IsCancellationRequested) catches probe-only timeouts → plain OperationCanceledException re-throws on service shutdown → bare catch handles SocketException/connection-refused
• If both cancellations fire simultaneously, the service-shutdown path wins — correct behavior

New Findings

None.

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

Verdict: ✅ Approve

All findings across 5 review rounds have been addressed. The codespace health check is now robust against half-open SSH tunnels. Ship it. 🚢