Track authoritative terminal state for reconciliation - #1033
Track authoritative terminal state for reconciliation#1033dennisonbertram wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b5a9cf434
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| transcript.reconcile( | ||
| messages: messages, | ||
| authoritativeTerminalState: latestAuthoritativeTerminalState) | ||
| connectionError = nil |
There was a problem hiding this comment.
When the per-run SSE fails before harnessd reaches a terminal state, markFailed() makes the session non-busy, so ProjectSession.syncCurrentConversation() can immediately fetch messages and reach this call with latestAuthoritativeTerminalState == nil. For an existing conversation, that endpoint may still return the previous terminal snapshot while the current run continues; Transcript.reconcile then calls load, marks the run completed, and this method clears the connection error. The UI consequently reports success and permits another submission without any completed event or run-status evidence. Absence of authoritative terminal provenance must not be interpreted as completion.
Useful? React with 👍 / 👎.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Addressed the P1 at Verification is green: strict Swift format; 179 Swift tests / 40 suites; full normal + race + coverage regression gate at 85.6% with zero uncovered functions. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 039acd3a47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| streamTask?.cancel() | ||
| transcript.load(messages: messages) | ||
| latestAuthoritativeTerminalState = nil | ||
| awaitingAuthoritativeTerminalState = false |
There was a problem hiding this comment.
When an accepted run's per-run SSE fails, the transcript becomes non-busy while awaitingAuthoritativeTerminalState intentionally remains true. The user can then click the already-selected conversation in the rail, whose ProjectSession.openConversation path calls load; this assignment clears the lock even though the conversation did not change, while transcript.load reports the potentially older snapshot as completed and the method clears the transport error. canSubmit is consequently re-enabled before any authoritative terminal event arrives, recreating the false-success/overlapping-run scenario through the normal conversation-reload path. Only discard unresolved provenance here when self.conversationID != conversationID.
Useful? React with 👍 / 👎.
|
Addressed the same-conversation reload P1 at Verification remains green: strict Swift format; 179 Swift tests / 40 suites; full normal + race + coverage regression gate at 85.6% with zero uncovered functions. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7234c8afa8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| let started = try await client.startRun(request) | ||
| currentRunID = started.runID | ||
| awaitingAuthoritativeTerminalState = true |
There was a problem hiding this comment.
When submitting into an already-open conversation, its conversation-wide SSE remains live while startRun awaits the POST response, and harnessd dispatches the run before writing the 202 response. A fast run can therefore deliver its terminal event first, causing apply to clear the lock, only for this assignment to set it again afterward. The subsequent run-scoped replay is discarded by seenEventIDs, so no event clears the lock and the completed conversation permanently rejects further submissions until it is reset or switched. Set the accepted-run lock without overwriting terminal provenance that may already have arrived for started.runID.
Useful? React with 👍 / 👎.
|
Addressed the third P1 at Verification is green: strict Swift format; 180 Swift tests / 40 suites; full normal + race + coverage regression gate at 85.6% with zero uncovered functions. @codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
54756e9 to
b6eba2e
Compare
|
Rebased the previously clean-reviewed tree onto current @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6eba2e81e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // MARK: - Conversation switching | ||
|
|
||
| public func load(messages: [StoredMessage], conversationID: String) { | ||
| if self.conversationID == conversationID { |
There was a problem hiding this comment.
This same-ID fast path also handles the reloads after ProjectSession.undo and rewind, not only rail refreshes. If the removed run failed or was cancelled, reconcilePersistedMessages preserves latestAuthoritativeTerminalState and can re-add that run's error after loading the truncated history, leaving the restored conversation incorrectly marked failed/cancelled. Those destructive callers need a full load that clears terminal provenance, or a distinct refresh API.
AGENTS.md reference: AGENTS.md:L57-L61
Useful? React with 👍 / 👎.
1 participant
Closes #1031
Related #1000
Related #1028
Related #1029
Summary
Distinguishes authoritative harness terminal events from local transport/control placeholders during macOS durable transcript reconciliation. A temporary per-run SSE failure no longer becomes a false success from an older message snapshot, yet it still recovers once the conversation stream delivers the authoritative terminal event. Authoritative
run.failedandrun.cancelledstates remain preserved.Design
RunSessionowns terminal provenance and the accepted-run-to-terminal lock because it owns the async server-event boundary.canSubmitremains false during that unresolved interval even after a local stream failure stops the spinner.Transcript.markFailed/markCancelledremain UI fallbacks and intentionally do not update authority.Transcript.reconcilereceives explicit authoritative state instead of inferring it from renderedrunState.No server, SSE/message wire, persistence, config, provider/model, tool schema, or layout changes.
TDD evidence
Deterministic failing-first regressions observed across the review loop:
.failed; an older durable snapshot then reported.completed, cleared the transport error, and enabled another prompt without any terminal event.RunSession.loadand recreated the false success.Green:
transportFailureWaitsForAuthoritativeCompletiondrives the same-conversation GUI reload path and preserves.failed, the transport error, andcanSubmit == falseuntil delayed authoritative completion arrives, then recovers.terminalBeforeStartResponseDoesNotRelockCompletedRunproves an early authoritative completion stays submit-ready after the delayed 202 and deduped per-run replay.Verification
./scripts/test-regression.sh: normal, full race, 85.6% coverage, zero uncovered production functions.Impact / rollout / rollback
Native client-only state provenance; no migration or durable repair. Older clients can retain false failure or false success after a transient disconnect; new clients wait for the authoritative terminal event. Revert if authoritative failure/cancellation stops surviving reconciliation or completed runs cannot recover.
Documentation
Added plan, full impact map, long-term intent, plan index, and required engineering-log symptom/cause/fix/TDD/verification record.
Contract checklist