Fix issues from PR #285: gate auto-resend, TurnEnd idle fallback, improved permission detection by PureWeen · Pull Request #305 · PureWeen/PolyPilot

PureWeen · 2026-03-08T00:53:18Z

Fixes issues from PR #285

Changes

Gate auto-resend on successful tools (fixes Auto-resend after permission recovery should be more visible #298): Added SuccessfulToolCountThisTurn to SessionState. Permission recovery skips resend when tools already completed this turn, preventing re-execution of side-effectful work. Shows ⟳ Resending: "..." when resending, or Auto-resend skipped when skipped.
TurnEnd→Idle fallback (addresses Zero-idle sessions: SDK never emits session.idle event #299): When AssistantTurnEndEvent fires, schedule a 4s delayed CompleteResponse. Cancelled when SessionIdleEvent arrives (normal path) or a new turn starts.
Improved permission denial detection (improves SDK permission callback binding lost in headless mode after reconnections #300): Extracted IsPermissionDenialText() helper matching "Permission denied", "denied-no-approval-rule", and "could not request permission".

Issues Addressed

Fixes Auto-resend after permission recovery should be more visible #298
Addresses Zero-idle sessions: SDK never emits session.idle event #299 (workaround for SDK bug)
Improves SDK permission callback binding lost in headless mode after reconnections #300 (more robust detection)

Verification

Build: 0 errors
20/20 tests pass

…cle cleanup) 🔴 CRITICAL: Cancel TurnEndIdleCts on both reconnect paths - SendPromptAsync reconnect path: add CancelTurnEndFallback(state) alongside CancelProcessingWatchdog(state) so the old 4s fallback timer cannot fire CompleteResponse on the new shared Info object after reconnect - TryRecoverPermissionAsync: same fix — cancel fallback before creating newState 🟡 MODERATE: Extract CancelTurnEndFallback helper (mirrors CancelProcessingWatchdog) - Cancel + Dispose to prevent kernel timer resource leak over many cycles - Replace all 8 inline Interlocked.Exchange + Cancel patterns with helper 🟡 MODERATE: Add CancelTurnEndFallback to lifecycle methods - DisposeAsync: cancel alongside CancelProcessingWatchdog in session cleanup loop - CloseSessionCoreAsync: cancel before SDK session dispose - InitializeAsync cleanup: cancel in re-init teardown loop 🟢 MINOR: Extract TurnEndIdleFallbackMs constant (was magic 4000) 🟢 MINOR: Volatile.Read → Interlocked.CompareExchange for SuccessfulToolCountThisTurn read in TryRecoverPermissionAsync (consistency with other Interlocked sites) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

… guard, token capture) Moderate: SessionErrorEvent missing CancelTurnEndFallback (INV-1 gap) - Add CancelTurnEndFallback(state) in SessionErrorEvent handler alongside CancelProcessingWatchdog, closing the last path where a 4s fallback timer could fire CompleteResponse on an already-errored session. Moderate: 4s fallback fires during long tool execution - Add ActiveToolCallCount guard inside the fallback Task.Run closure: if tools are still active after the delay, skip CompleteResponse (AssistantTurnStartEvent is coming to cancel it; this is just defense-in-depth). Minor: Token capture after CTS publish (race hardening) - Capture fallbackToken = idleFallbackCts.Token BEFORE publishing the CTS via Interlocked.Exchange to state.TurnEndIdleCts. Prevents a theoretical race where CancelTurnEndFallback disposes the CTS before .Token is read. Minor: Revert Interlocked.CompareExchange(x,0,0) back to Volatile.Read - Volatile.Read is the idiomatic pattern for a pure read of an int field alongside Interlocked writes; CompareExchange(x,0,0) looks like a reset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

PureWeen · 2026-03-08T14:52:40Z

Review Findings (3rd pass -- 5-model consensus)

⚠️ Request Changes

🟡 MODERATE -- TurnEnd→Idle fallback race window remains

CopilotService.Events.cs (TurnEnd handler)

Between AssistantTurnEndEvent and the next AssistantTurnStartEvent, if all tool calls complete (ActiveToolCallCount→0) but the LLM takes >4s reasoning, the fallback timer fires CompleteResponse prematurely -- truncating the response mid-generation.

Suggested fix: Add a HasUsedToolsThisTurn guard inside the fallback closure. HasUsedToolsThisTurn persists across sub-turns (unlike ActiveToolCallCount which resets on TurnStart), so the closure can check: if tools were used this turn and no SessionIdle has arrived, skip the fallback fire. This narrows the race to only the first turn (before any tool use), which is acceptable.

🟡 MODERATE -- No test coverage for the TurnEnd→Idle fallback

The 4s fallback timer, CancelTurnEndFallback() helper, and the entire TurnEnd→Idle flow have zero automated tests. Given this is the 3rd review cycle with an active race condition, tests would prevent future regressions.

Suggested tests:

Verify CancelTurnEndFallback() cancels and disposes the CTS (unit test on SessionState)
Verify the fallback does NOT fire CompleteResponse when cancelled by SessionIdle within 4s
Verify the fallback DOES fire CompleteResponse when no SessionIdle arrives after 4s

review) Address PR #305 review comment issuecomment-4019192428: 1. Make TurnEndIdleFallbackMs internal for test accessibility 2. Add TurnEndIdleFallbackTests (5 tests): - CancelTurnEndFallback pattern cancels and disposes CTS - Null field does not throw - Fallback does NOT fire when cancelled by SessionIdle within delay - Fallback DOES fire when no SessionIdle arrives - Constant value is 4000ms 3. Add TurnEndFallbackTests (6 tests): - CTS cancel pattern cancels token - Null CTS is safe - Timer fires when not cancelled - Timer does not fire when cancelled before delay - Explicit IsCancellationRequested guard prevents late firing - Constant is in reasonable range (1-30s) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

PureWeen · 2026-03-08T16:04:51Z

🤖 Multi-Model Consensus Review (Re-Review #3)

CI Status: ⚠️ (pre-existing failures)

Findings

🟡 MODERATE -- Fallback race window remains (5/5 models flagged)

File: CopilotService.Events.cs -- TurnEnd→Idle fallback timer
Between TurnEnd and next TurnStart, if tools complete quickly but the LLM's next reasoning turn takes >4s, the fallback timer fires prematurely and clears IsProcessing while the session is still active.
Suggested fix: Add a HasUsedToolsThisTurn guard inside the fallback closure. If tools were used this turn, the timer should be extended or skipped, since a new TurnStart is expected.

🟡 MODERATE -- No test coverage (4/5 models flagged)

The TurnEnd→Idle fallback is a new state machine path with no tests.
Suggested tests:
1. TurnEnd followed by SessionIdle within 4s → verify fallback timer is cancelled
2. TurnEnd with no SessionIdle after 4s → verify IsProcessing is cleared
3. TurnEnd after tool use → verify fallback does NOT fire prematurely (HasUsedToolsThisTurn guard)
4. Multiple rapid TurnEnd events → verify no CTS leak

Recommended Action: ⚠️ Request Changes

Add HasUsedToolsThisTurn guard to the fallback closure to prevent premature firing after tool rounds
Add at least basic test coverage for the new fallback timer path

…allback Address PR #305 review comment issuecomment-4019363353 (suggested tests 3 & 4): 3. Fallback_DoesNotFire_WhenToolsWereUsedThisTurn: verifies HasUsedToolsThisTurn guard blocks CompleteResponse when tools were used this turn (new TurnStart is expected from LLM reasoning) 3b. Fallback_DoesFire_WhenNoToolsUsedAndNoSessionIdle: positive case — no tools used and no SessionIdle → CompleteResponse fires correctly 4. MultipleRapidTurnEnds_NoCtsLeak: 10 rapid TurnEnd events — verifies all replaced CTS instances are cancelled+disposed (Interlocked.Exchange pattern), only the latest CTS remains active All 31 TurnEnd/fallback/permission tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…298) When auto-recovery from permission denial succeeds, skip auto-resend if any tools already completed successfully this turn. This avoids re-executing side-effectful tools that already finished before the denial. - Add SuccessfulToolCountThisTurn to SessionState, increment on successful tool completion - Gate TryRecoverPermissionAsync resend: skip if SuccessfulToolCountThisTurn > 0 - Show prominent '⟳ Resending: ...' message when resending - Show 'Auto-resend skipped' message when resend is skipped - Reset/transfer alongside HasUsedToolsThisTurn in all cleanup paths Fixes: #298 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…onAsync cancellation, restored comment - Changed TurnEndIdleCts from property to field to fix 7 CS0206 compile errors with Interlocked.Exchange(ref ...) - Added TurnEndIdleCts cancellation in AbortSessionAsync to prevent delayed CompleteResponse on aborted sessions - Restored deleted // Release send lock comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…cle cleanup) 🔴 CRITICAL: Cancel TurnEndIdleCts on both reconnect paths - SendPromptAsync reconnect path: add CancelTurnEndFallback(state) alongside CancelProcessingWatchdog(state) so the old 4s fallback timer cannot fire CompleteResponse on the new shared Info object after reconnect - TryRecoverPermissionAsync: same fix — cancel fallback before creating newState 🟡 MODERATE: Extract CancelTurnEndFallback helper (mirrors CancelProcessingWatchdog) - Cancel + Dispose to prevent kernel timer resource leak over many cycles - Replace all 8 inline Interlocked.Exchange + Cancel patterns with helper 🟡 MODERATE: Add CancelTurnEndFallback to lifecycle methods - DisposeAsync: cancel alongside CancelProcessingWatchdog in session cleanup loop - CloseSessionCoreAsync: cancel before SDK session dispose - InitializeAsync cleanup: cancel in re-init teardown loop 🟢 MINOR: Extract TurnEndIdleFallbackMs constant (was magic 4000) 🟢 MINOR: Volatile.Read → Interlocked.CompareExchange for SuccessfulToolCountThisTurn read in TryRecoverPermissionAsync (consistency with other Interlocked sites) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

… guard, token capture) Moderate: SessionErrorEvent missing CancelTurnEndFallback (INV-1 gap) - Add CancelTurnEndFallback(state) in SessionErrorEvent handler alongside CancelProcessingWatchdog, closing the last path where a 4s fallback timer could fire CompleteResponse on an already-errored session. Moderate: 4s fallback fires during long tool execution - Add ActiveToolCallCount guard inside the fallback Task.Run closure: if tools are still active after the delay, skip CompleteResponse (AssistantTurnStartEvent is coming to cancel it; this is just defense-in-depth). Minor: Token capture after CTS publish (race hardening) - Capture fallbackToken = idleFallbackCts.Token BEFORE publishing the CTS via Interlocked.Exchange to state.TurnEndIdleCts. Prevents a theoretical race where CancelTurnEndFallback disposes the CTS before .Token is read. Minor: Revert Interlocked.CompareExchange(x,0,0) back to Volatile.Read - Volatile.Read is the idiomatic pattern for a pure read of an int field alongside Interlocked writes; CompareExchange(x,0,0) looks like a reset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…nDenialText tests - Add HasUsedToolsThisTurn guard in TurnEnd fallback: skip CompleteResponse if tools were used this turn, covering the 'fast tools + delayed TurnStart >4s' race window (LLM reasoning between sub-turns) - Add explicit fallbackToken.IsCancellationRequested check before guards - Add catch (Exception ex) in fallback Task.Run to prevent swallowed errors - Make IsPermissionDenialText internal static for testability - Add PermissionDenialDetectionTests (17 tests) covering null/empty, permission denied variants, SDK error codes, and unrelated text Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

review) Address PR #305 review comment issuecomment-4019192428: 1. Make TurnEndIdleFallbackMs internal for test accessibility 2. Add TurnEndIdleFallbackTests (5 tests): - CancelTurnEndFallback pattern cancels and disposes CTS - Null field does not throw - Fallback does NOT fire when cancelled by SessionIdle within delay - Fallback DOES fire when no SessionIdle arrives - Constant value is 4000ms 3. Add TurnEndFallbackTests (6 tests): - CTS cancel pattern cancels token - Null CTS is safe - Timer fires when not cancelled - Timer does not fire when cancelled before delay - Explicit IsCancellationRequested guard prevents late firing - Constant is in reasonable range (1-30s) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…allback Address PR #305 review comment issuecomment-4019363353 (suggested tests 3 & 4): 3. Fallback_DoesNotFire_WhenToolsWereUsedThisTurn: verifies HasUsedToolsThisTurn guard blocks CompleteResponse when tools were used this turn (new TurnStart is expected from LLM reasoning) 3b. Fallback_DoesFire_WhenNoToolsUsedAndNoSessionIdle: positive case — no tools used and no SessionIdle → CompleteResponse fires correctly 4. MultipleRapidTurnEnds_NoCtsLeak: 10 rapid TurnEnd events — verifies all replaced CTS instances are cancelled+disposed (Interlocked.Exchange pattern), only the latest CTS remains active All 31 TurnEnd/fallback/permission tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ack, improved permission detection (PureWeen#305) ## Fixes issues from PR PureWeen#285 ### Changes - **Gate auto-resend on successful tools** (fixes PureWeen#298): Added `SuccessfulToolCountThisTurn` to SessionState. Permission recovery skips resend when tools already completed this turn, preventing re-execution of side-effectful work. Shows `⟳ Resending: "..."` when resending, or `Auto-resend skipped` when skipped. - **TurnEnd→Idle fallback** (addresses PureWeen#299): When AssistantTurnEndEvent fires, schedule a 4s delayed CompleteResponse. Cancelled when SessionIdleEvent arrives (normal path) or a new turn starts. - **Improved permission denial detection** (improves PureWeen#300): Extracted IsPermissionDenialText() helper matching "Permission denied", "denied-no-approval-rule", and "could not request permission". ### Issues Addressed - Fixes PureWeen#298 - Addresses PureWeen#299 (workaround for SDK bug) - Improves PureWeen#300 (more robust detection) ### Verification - Build: 0 errors - 20/20 tests pass --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

PureWeen and others added 7 commits March 8, 2026 17:31

PureWeen force-pushed the session-20260307-121640 branch from 8029c82 to 45cb6f0 Compare March 8, 2026 22:34

PureWeen merged commit 6d1e780 into main Mar 8, 2026

PureWeen deleted the session-20260307-121640 branch March 8, 2026 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues from PR #285: gate auto-resend, TurnEnd idle fallback, improved permission detection#305

Fix issues from PR #285: gate auto-resend, TurnEnd idle fallback, improved permission detection#305
PureWeen merged 7 commits intomainfrom
session-20260307-121640

PureWeen commented Mar 8, 2026

Uh oh!

PureWeen commented Mar 8, 2026

Uh oh!

PureWeen commented Mar 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

PureWeen commented Mar 8, 2026

Fixes issues from PR #285

Changes

Issues Addressed

Verification

Uh oh!

PureWeen commented Mar 8, 2026

Review Findings (3rd pass -- 5-model consensus)

⚠️ Request Changes

🟡 MODERATE -- TurnEnd→Idle fallback race window remains

🟡 MODERATE -- No test coverage for the TurnEnd→Idle fallback

Uh oh!

PureWeen commented Mar 8, 2026

🤖 Multi-Model Consensus Review (Re-Review #3)

CI Status: ⚠️ (pre-existing failures)

Findings

Recommended Action: ⚠️ Request Changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant