fix: soft steer falls through to hard steer on disposed JsonRpc connection

[PureWeen]

Bug

⚠️ Soft steer failed: Cannot access a disposed object. Object name: 'StreamJsonRpc.JsonRpc'

When the underlying JsonRpc connection is disposed (e.g., CLI process crashed), soft steer's SendAsync throws ObjectDisposedException. Previously this just reported the error and gave up — the user's steering message was lost.

Fix

Connection errors (detected via IsConnectionError) in the soft steer path now:

1. Remove the already-added user message from history (prevents duplicate)
2. Fall through to the hard steer path (AbortSessionAsync + SendPromptAsync)

SendPromptAsync has full reconnection logic: dispose old session → restart server if needed → recreate client → resume session → retry send. Non-connection errors still use the original error-and-cleanup behavior.

Tests

• Added SteerSession_SoftSteerPath_ContainsConnectionErrorFallback — source-structure test verifying the fallback path exists with correct patterns (IsConnectionError, STEER-FALLBACK, History.RemoveAt, softSteerSucceeded).

Verified

• All 2025 passing tests still pass (15 pre-existing failures unrelated)
• Mac Catalyst build succeeds (0 errors)

[PureWeen]

PR #293 Review — "fix: soft steer falls through to hard steer on disposed JsonRpc connection"

CI Status: ⚠️ No checks reported
Prior reviews: None
Models: claude-opus-4.6 (×2), claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex
Consensus filter: 2+ models required

---

Core Logic — ✅ Correct

The control flow is sound:

• softSteerSucceeded = false → SendAsync → softSteerSucceeded = true
• Connection error catch: remove userMsg from History, fall through
• Non-connection error catch: cleanup + return (unchanged behavior)
• if (softSteerSucceeded) return; → hard steer for connection-error path

Reference equality state.Info.History[^1] == userMsg is correct — same object instance just added. Hard steer's AbortSessionAsync correctly resets all processing state. The fundamental fix works.

---

🟡 MODERATE — Unguarded IndexOf crashes test with unintelligible error (5/5 models)

PolyPilot.Tests/SteeringMessageTests.cs — SteerSession_SoftSteerPath_ContainsConnectionErrorFallback

var softSteerBlock = source.Substring(softSteerStart,
    source.IndexOf("// Hard steer:", softSteerStart) - softSteerStart);
//                 ^^^^^^^^^^^^^^^^ no guard — can return -1

If "// Hard steer:" is ever renamed or removed, IndexOf returns -1, making the length -1 - softSteerStart = large negative number → ArgumentOutOfRangeException with no context instead of a clear assertion failure. The first IndexOf has a proper guard; this one doesn't.

Fix:

var hardSteerIdx = source.IndexOf("// Hard steer:", softSteerStart);
Assert.True(hardSteerIdx >= 0, "'// Hard steer:' anchor comment not found in CopilotService.cs");
var softSteerBlock = source.Substring(softSteerStart, hardSteerIdx - softSteerStart);

---

🟡 MODERATE — Duplicate user message in _chatDb on connection-error fallback (5/5 models)

PolyPilot/Services/CopilotService.cs — soft steer + connection-error catch

The soft steer path calls _chatDb.AddMessageAsync(userMsg) before SendAsync. The connection-error catch removes userMsg from state.Info.History only — not from _chatDb. The hard steer path's SendPromptAsync then writes the same message to _chatDb again. On next session load from DB, the steering message appears twice in chat history.

Fix: Either call _chatDb.RemoveMessageAsync(userMsg) (or equivalent) in the connection-error catch block, or pass a flag to suppress the _chatDb write in SendPromptAsync when the message was already persisted by the soft steer path.

---

🟡 MODERATE — History.RemoveAt called from background thread (4/5 models)

PolyPilot/Services/CopilotService.cs — connection-error catch block

catch (Exception ex) when (IsConnectionError(ex))
{
    state.Info.History.RemoveAt(state.Info.History.Count - 1);  // ← background thread
    state.Info.MessageCount = state.Info.History.Count;

SteerSessionAsync runs on a background thread (fire-and-forget from Dashboard). History is List<ChatMessage> — not thread-safe. The UI thread iterates it during Blazor render. Concurrent RemoveAt while rendering risks ArgumentOutOfRangeException or silent corruption. All other History mutations in the service that happen post-event use Invoke(() => { ... }).

Fix: Wrap the RemoveAt / MessageCount update in InvokeOnUI(() => { ... }) (or the existing Invoke helper used throughout the file).

---

Test Coverage

New test SteerSession_SoftSteerPath_ContainsConnectionErrorFallback is a source-structure test (reads CopilotService.cs at runtime). This validates the pattern is present but cannot test the actual runtime behavior. A behavioral test — mocking IsConnectionError returning true → verify hard steer is called with original message — would give stronger guarantees, but the source-structure test is consistent with the existing pattern in this file.

---

Recommended Action: ⚠️ Request changes

Three small fixes needed. The core fallthrough logic is correct; these are correctness and reliability issues in the error handling path and test.

[PureWeen]

Independent Re-Review — PR #293 (5-Model Consensus)

CI Status: ⚠️ No checks reported
Prior review: One existing review comment covers the same findings (posted earlier this session).
This review: Independent 5-model parallel verification. All three prior findings confirmed unanimously.

---

🟡 MODERATE → 🔴 CRITICAL (5/5 models) — Duplicate message persisted to _chatDb on fallback

PolyPilot/Services/CopilotService.cs — soft steer + connection-error catch

Execution order on connection error:

1. Soft steer writes _ = _chatDb.AddMessageAsync(sessionId, userMsg) ← fire-and-forget, already in-flight
2. SendAsync throws ObjectDisposedException
3. Connection-error catch: History.RemoveAt removes from in-memory History — DB row is NOT removed
4. Falls through to hard steer → SendPromptAsync creates a new ChatMessage with the same content and writes it to _chatDb again

On next session reload, the steering message appears twice in chat history. AddMessageAsync has no dedup logic — it uses maxOrder + 1 → INSERT. The ghost duplicate is permanent.

Fix options:

• Call _chatDb.RemoveMessageAsync (or equivalent) in the connection-error catch before falling through, OR
• Pass the already-persisted userMsg reference into SendPromptAsync via an overload that skips the DB write

---

🟡 MODERATE (5/5 models) — History.RemoveAt on background thread races with UI render

PolyPilot/Services/CopilotService.cs — connection-error catch block

SteerSessionAsync is invoked fire-and-forget from Dashboard:

_ = CopilotService.SteerSessionAsync(sessionName, ...).ContinueWith(...)

History is List<ChatMessage> (not thread-safe). The Blazor render thread iterates History for display; concurrent RemoveAt can throw InvalidOperationException during enumeration, or corrupt indices.

Sonnet model also flagged a subtler TOCTOU: the guard History[^1] == userMsg and the subsequent RemoveAt(History.Count - 1) are two separate index reads — a concurrent FlushCurrentResponse or tool-result Add between those two calls could delete the wrong message.

Fix: Wrap the RemoveAt + MessageCount update in InvokeOnUI(() => { ... }), consistent with all other History mutations in this file.

---

🟡 MODERATE (5/5 models) — Unguarded IndexOf crashes test with unintelligible error

PolyPilot.Tests/SteeringMessageTests.cs — SteerSession_SoftSteerPath_ContainsConnectionErrorFallback

var softSteerBlock = source.Substring(softSteerStart,
    source.IndexOf("// Hard steer:", softSteerStart) - softSteerStart);
//  ^^^ returns -1 if comment renamed/removed → length = (-1 - softSteerStart) → ArgumentOutOfRangeException

If the // Hard steer: anchor comment is ever renamed, this throws with no diagnostic context.

Fix:

var hardSteerIdx = source.IndexOf("// Hard steer:", softSteerStart);
Assert.True(hardSteerIdx >= 0, "'// Hard steer:' anchor not found in CopilotService.cs");
var softSteerBlock = source.Substring(softSteerStart, hardSteerIdx - softSteerStart);

---

Core Logic Assessment

✅ The softSteerSucceeded flag logic is correct. All 5 models agreed: success → return, connection error → fall through to hard steer, non-connection error → cleanup + return. No path can slip through incorrectly. The fundamental fix works.

---

Recommended Action: ⚠️ Request changes

Three targeted fixes required:

1. Remove persisted user message from _chatDb in the connection-error catch (or prevent the double-write)
2. Wrap History.RemoveAt / MessageCount in InvokeOnUI()
3. Add Assert.True(hardSteerIdx >= 0, ...) guard before Substring in new test

Independent 5-model consensus review (claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex) — confirms all three findings from prior review

[PureWeen]

PR #293 Round 2 Re-review (commits bf0da69, d622d44)

CI Status: ⚠️ No checks reported
Models: claude-opus-4.6 (×2), claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex
Consensus filter: 2+ models required

---

Previous Findings — All 3 FIXED ✅

Finding	Status
🟡 Duplicate _chatDb write on fallback	✅ FIXED (5/5) — AddMessageAsync moved inside try after softSteerSucceeded = true; connection-error path never writes
🟡 History.RemoveAt on background thread	✅ FIXED (5/5) — wrapped in await InvokeOnUIAsync(), ensuring removal completes on UI thread before hard steer runs
🟡 Unguarded IndexOf in test	✅ FIXED (5/5) — both tests now assert hardSteerIdx >= 0 before Substring

---

Additional Fix Applied

During re-review, 4/5 models identified that InvokeOnUI (fire-and-forget) introduced a secondary race: the removal callback could fire after SendPromptAsync added the new steering message, causing History[^1] == userMsg to mismatch and leaving userMsg stranded as a duplicate.

The fix (d622d44) upgrades to await InvokeOnUIAsync() — the awaitable variant already present in the codebase — which guarantees the removal completes before the hard steer path executes. All 20 SteeringMessage tests pass.

---

No New Issues Found

The fix is correct and complete:

• On success: DB write after SendAsync succeeds — no orphaned entries
• On connection error: History removal awaited before hard steer — no duplicates
• On other errors: unchanged cleanup + return path — no regression
• Test anchors guarded — clear failure messages if structure changes

Recommended Action: ✅ Approve