fix: address review findings round 1 (PR #802)#803
Closed
github-actions[bot] wants to merge 4 commits intomainfrom
Closed
fix: address review findings round 1 (PR #802)#803github-actions[bot] wants to merge 4 commits intomainfrom
github-actions[bot] wants to merge 4 commits intomainfrom
Conversation
Before sending a prompt, SendPromptAsync now checks if events.jsonl ends with session.shutdown. If so, it forces a reconnect before sending instead of sending to a dead session and discovering the failure 10+ minutes later via the watchdog. The GetLastEventType helper (tail-read, last 4KB) keeps overhead minimal on the normal send path. Also increases the structural test search window in PrematureIdleSignal_ResetInSendPromptAsync to accommodate the new code. Fixes #397 Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…le-reconnect After lazy-resume, events.jsonl still shows session.shutdown from the old session. The pre-check would read this stale file and unnecessarily reconnect again. Gate the pre-check on whether the session was already connected before entering SendPromptAsync, so freshly resumed sessions skip the check. Also fixes: - OperationCanceledException now caught separately (preserves cancellation semantics) - state.Session = null! (consistent with 12+ other occurrences in codebase) - Error message includes inner exception details for actionable root cause Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Search from 'IsProcessing = true' anchor instead of the method signature. This avoids fragile window sizing that breaks every time code is added to the early part of SendPromptAsync. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace weak behavioral tests (that just re-tested GetLastEventType) with structural tests that verify the actual fix code: - wasAlreadyConnected guard prevents double-reconnect - OperationCanceledException has its own catch with throw - SendingFlag released on both catch paths - Error message includes inner exception details - null! used for Session assignment (not bare null) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
|
Stale fix-round PR — fixes were pushed to the main PR branch. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses 6 expert review findings from round 1.
Findings addressed
🟡 MODERATE — Spurious double-reconnect on first-send-after-restart (2/3 reviewers)
Added
wasAlreadyConnectedguard — pre-check is skipped when lazy-resume already reconnected.🟡 MODERATE —
OperationCanceledExceptionwrapping loses cancellation identity (2/3 reviewers)Added specific
catch (OperationCanceledException)withthrow;before the general catch.🟢 MINOR —
nullvsnull!inconsistency (summary finding)Changed
state.Session = null;→state.Session = null!;for consistency with 12+ other occurrences.🟢 MINOR — Fragile structural test window (2/3 reviewers)
Changed anchor from method signature to
IsProcessing = truewith 2000-char window.🟢 MINOR — Error message attributes all failures to "server shutdown" (2/3 reviewers)
Error now includes
ex.Messagefor actionable root cause visibility.🟡 MODERATE — Tests don't cover actual behavior change (3/3 reviewers)
Added 5 structural tests verifying pre-check code invariants (guard, cancellation, flag release, error message, null!).
Test results
All 3649 tests pass (5 new structural tests added).
Warning
The following domain was blocked by the firewall during workflow execution:
192.0.2.1To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.