diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index fdf873f5..7d3cd44e 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -1,5 +1,34 @@ # Engineering Log +## 2026-07-30 (Authoritative Terminal Provenance — Issue #1031) + +- Symptom: after a per-run SSE transport error, `markFailed()` stopped the + spinner locally. Durable reconciliation later preserved that `.failed` state + even when harnessd had completed and persisted a successful run. +- Cause: #1028 reconciliation inferred authority from `Transcript.runState`, + which intentionally includes local transport/control fallbacks as well as + server terminal events. +- Fix: `RunSession` now tracks both the latest terminal state delivered by a + deduped authoritative harness event and whether an accepted run is still + awaiting one. Only an actual conversation replacement resets that + provenance; clicking the already-selected rail row reconciles in place. An + old durable snapshot cannot report success or enable another prompt after a + local stream failure; the eventual terminal event resolves the provisional + failure while server failure/cancellation remains terminal. Terminal run IDs + are retained per selected conversation so a fast terminal event delivered + before the start response cannot be overwritten by that late 202. +- TDD evidence: `transportFailureWaitsForAuthoritativeCompletion` first changed + a provisional `.failed` state to `.completed`, cleared its transport error, + and enabled a second prompt from an older durable snapshot. It now preserves + all three guards through the normal same-conversation reload path until the + delayed authoritative completion arrives, then recovers. Authoritative + failed/cancelled/completed controls remain green. + `terminalBeforeStartResponseDoesNotRelockCompletedRun` separately failed with + `canSubmit == false` after the late 202 and now remains unlocked. +- Verification: strict Swift formatting and the complete Swift package (180 + tests in 40 suites) pass. `./scripts/test-regression.sh` passes the normal + and full race suites plus 85.6% coverage with zero uncovered functions. + ## 2026-07-30 (Workflow Subscription Cancellation Test — Issue #1035) - Symptom: the full race gate failed in diff --git a/docs/logs/long-term-thinking-log.md b/docs/logs/long-term-thinking-log.md index 9c300e4a..2beb1ad8 100644 --- a/docs/logs/long-term-thinking-log.md +++ b/docs/logs/long-term-thinking-log.md @@ -1,5 +1,18 @@ # Long-Term Thinking Log +## 2026-07-30 (Authoritative Terminal Provenance — Issue #1031) + +- Command intent: correct the post-#1028 transport-recovery regression before + completing the cron/callback GUI release. +- User intent: a transient GUI stream failure must not permanently misreport a + successful deployment watcher; only the server's terminal event is + authoritative. +- Success definition: local failure unblocks the spinner but completed durable + state later recovers; authoritative failure/cancellation remains; all gates + and the eventual native scheduled-conversation acceptance pass. +- Guardrails: issue #1031, failing test first, no server/wire/tool change, and + no heuristic based only on the rendered transcript. + ## 2026-07-30 (Workflow Subscription Cancellation Test — Issue #1035) - Command intent: clear the race-gate blocker exposed while verifying the diff --git a/docs/plans/2026-07-30-issue-1031-terminal-provenance-impact-map.md b/docs/plans/2026-07-30-issue-1031-terminal-provenance-impact-map.md new file mode 100644 index 00000000..f256c92e --- /dev/null +++ b/docs/plans/2026-07-30-issue-1031-terminal-provenance-impact-map.md @@ -0,0 +1,93 @@ +# Cross-Surface Impact Map: Issue #1031 Terminal Provenance + +## Task + +- Task / issue: recover provisional transport failure during durable + reconciliation, #1031. +- Plan link: `2026-07-30-issue-1031-terminal-provenance-plan.md`. +- Owner: Codex. +- Status: implemented; verification and merge pending. + +## Current Ownership, Callers, and Data Flow + +- Entry points: `RunSession.submit`, per-run/conversation SSE, and durable Chat + reconciliation. +- Owners/source of truth: server terminal events are authoritative; + `Transcript.markFailed/markCancelled` are local presentation fallbacks; + `RunSession` owns async event provenance. +- Consumers: transcript status/error UI and callback/cron conversation replay. +- Similar abstractions searched: event dedupe, run start/reset/load/rebind, + transcript load/reconcile, local terminal events. +- Search evidence: + `rg -n 'markFailed|localTerminalEvent|runFailed|reconcilePersistedMessages|seenEventIDs' macapp/Sources macapp/Tests`. +- Ownership conclusion: store provenance in `RunSession`; do not infer it from + the reduced transcript and do not create a second transcript. + +## Config, API, CLI, and Tools + +- Config/defaults/env/saved settings: none. +- Endpoints/request/response/server wiring: none. +- CLI/tools/wire/integrations: none. +- Error states: a local transport failure becomes provisional, blocks another + prompt, ignores older durable completion snapshots, and may recover only + after an authoritative terminal event; server failure/cancellation remains + terminal. + +## Persistence and Compatibility + +- Schemas/migrations/caches/generated data: none. +- Compatibility: native-client-only interpretation of unchanged events and + messages. +- Mixed versions: old clients may retain false failure; new clients recover. + +## Lifecycle, Security, and Reliability + +- Concurrency/lifecycle: mark an accepted/started run unresolved until a + deduped authoritative terminal event arrives; clear provenance when the + conversation changes, but reconcile a same-conversation rail reload in + place without cancelling the current stream. Remember terminal run IDs so a + late start response cannot overwrite an earlier conversation terminal event. +- Auth/security/privacy/secrets: none; no new data. +- Recovery/idempotency: repeated durable reconciliation yields the same state; + no durable data repair. + +## Product and Integration Surfaces + +- Server/runtime: unchanged. +- TUI/web/macOS: macOS only; TUI/API unaffected. +- Provider/model/tool routing: none; downstream client reduction only. +- External automation: cron/callback consume the repaired conversation path. +- UX/accessibility/motion: status correctness only; no layout/input change. + +## Deployment and Operations + +- Order/flags/migrations: native patch, no flag or migration. +- Diagnostics: deterministic transport-failure test plus engineering log. +- Rollback: revert if authoritative failure/cancellation stops persisting. +- Runbooks: none; operator contract unchanged. + +## Regression Tests + +- First red: transport exception -> local failed -> same-conversation rail + reload with an old durable snapshot falsely reports completed, clears the + error, and permits another prompt. +- Acceptance: preserve provisional failure and submission lock until a delayed + authoritative completion, then recover and clear the error. +- Controls: authoritative failed/cancelled and completed replay dedupe. +- Ordering control: terminal conversation SSE released after the start request + but before its 202 response leaves the completed run submit-ready. +- Real path: shared native callback/cron acceptance after #1031, #1032, #1027. +- Commands: + `swift test --package-path macapp --filter transportFailureWaitsForAuthoritativeCompletion`; + `swift test --package-path macapp`; + `./scripts/test-regression.sh`. + +## Documentation and Handoff + +- Before code: plan and impact map. +- After code: engineering/long-term logs and plans index/status. +- Release/training: none. + +## Warning Check + +- All applicable surfaces are mapped; none entries include rationale. diff --git a/docs/plans/2026-07-30-issue-1031-terminal-provenance-plan.md b/docs/plans/2026-07-30-issue-1031-terminal-provenance-plan.md new file mode 100644 index 00000000..03f8f38c --- /dev/null +++ b/docs/plans/2026-07-30-issue-1031-terminal-provenance-plan.md @@ -0,0 +1,69 @@ +# Plan: Distinguish Authoritative and Local Terminal State + +## Context + +- Governing GitHub issue: #1031. +- Problem: durable reconciliation cannot distinguish a server `run.failed` + event from the local failure placeholder used when an SSE transport dies. +- User impact: a completed scheduled or user run can remain permanently failed + in the macOS GUI after a transient disconnect. +- Constraints: preserve authoritative failed/cancelled state, completed replay + deduplication, and current server/wire contracts. + +## Scope + +- In scope: track the latest authoritative terminal event separately from + rendered transcript state and use it during durable reconciliation. +- Out of scope: transport retry redesign, server changes, cron/callback schema + changes, and historical failure retention across later runs (#1032). + +## Documentation Contract + +- Feature status: `implemented`. +- Public docs affected: none. +- Spec docs to update before code: this plan and impact map. +- Implementation notes to add after code: engineering log and plans index. + +## Test Plan (TDD) + +- New failing test: per-run transport failure creates a provisional local + failure; an older durable snapshot cannot report success or enable another + prompt when the user reloads the already-selected rail row before a delayed + authoritative completion event arrives. +- Existing controls: authoritative failed/cancelled replay remains preserved; + completed persisted replay remains deduplicated; a terminal conversation + event arriving before its delayed start response does not re-lock the run. +- Full verification: strict formatter, full Swift package, and repository + normal/race/coverage gate. + +## Cross-Surface Impact Map + +- See `2026-07-30-issue-1031-terminal-provenance-impact-map.md`. + +## Implementation Checklist + +- [x] Define acceptance criteria in tests. +- [x] Link a contract-complete structured GitHub issue before implementation. +- [x] Record current architecture, callers, consumers, and source-of-truth search evidence. +- [x] Document feature status and exact contract before code. +- [x] Complete and reconcile the cross-surface impact map before implementation. +- [x] Add characterization coverage before structural refactors. +- [x] Write failing tests first. +- [x] Review ownership/copy semantics for exported or state-storing types when mutable fields cross boundaries. +- [x] Implement minimal code changes. +- [x] Refactor while tests remain green. +- [x] Update docs, status ledgers, and indexes. +- [x] Update engineering/system/observational logs as needed. +- [x] Run full test suite. +- [ ] Merge branch back to `main` after tests pass. + +## Risks and Mitigations + +- Risk: an old authoritative failure could leak into a later run, or an old + message snapshot could falsely complete a currently unresolved run, or a + delayed start response could re-lock a run already completed by conversation + SSE. +- Mitigation: clear provenance on conversation replacement, track the + accepted-run-to-terminal interval explicitly, treat same-conversation load + as reconciliation, remember terminal run IDs, and test provisional blocking, + eventual recovery, and terminal-before-202 ordering. diff --git a/docs/plans/INDEX.md b/docs/plans/INDEX.md index 33a86e1e..3985ca6a 100644 --- a/docs/plans/INDEX.md +++ b/docs/plans/INDEX.md @@ -1,5 +1,7 @@ # Plans Index +- `2026-07-30-issue-1031-terminal-provenance-plan.md` — Issue #1031 implemented repair distinguishing authoritative terminal events from local transport-failure placeholders. +- `2026-07-30-issue-1031-terminal-provenance-impact-map.md` — Cross-surface impact map for Issue #1031. - `2026-07-30-issue-1035-workflow-subscription-cancel-plan.md` — Issue #1035 planned deterministic workflow subscription-cancellation regression repair. - `2026-07-30-issue-1035-workflow-subscription-cancel-impact-map.md` — Cross-surface impact map for Issue #1035. - `2026-07-30-issue-1026-feedback-direct-publish-plan.md` — Issue #1026 attached-image, publish-by-default GitHub feedback intake (implemented and verified; promotion pending). diff --git a/macapp/Sources/GoCodeUI/RunSession.swift b/macapp/Sources/GoCodeUI/RunSession.swift index a77cafd7..a52678c0 100644 --- a/macapp/Sources/GoCodeUI/RunSession.swift +++ b/macapp/Sources/GoCodeUI/RunSession.swift @@ -49,6 +49,20 @@ public final class RunSession { // scale; revisit with an LRU/bounded cap if a conversation runs long // enough for this to matter. private var seenEventIDs: Set = [] + /// The latest terminal state delivered by harnessd for the current run. + /// Local `markFailed` / `markCancelled` calls only unblock the UI after a + /// transport or local-control failure and intentionally do not update this + /// provenance. + private var latestAuthoritativeTerminalState: RunState? + /// True after harnessd accepts or starts a run and until its terminal event + /// arrives. A local stream failure may stop the spinner, but an older + /// durable snapshot must not turn that unresolved run into a success or + /// enable a second submission. + private var awaitingAuthoritativeTerminalState = false + /// A conversation event can reach the app before `startRun` returns its + /// accepted run id. Remember terminal run ids so that late 202 response + /// cannot re-lock a run the conversation stream already completed. + private var authoritativeTerminalRunIDs: Set = [] public init(client: HarnessClient) { self.client = client @@ -59,7 +73,9 @@ public final class RunSession { } public var isBusy: Bool { transcript.runState.isActive } - public var canSubmit: Bool { !draft.trimmed.isEmpty && !isBusy } + public var canSubmit: Bool { + !draft.trimmed.isEmpty && !isBusy && !awaitingAuthoritativeTerminalState + } /// True while a run is active, so the composer can offer steering instead. public var canSteer: Bool { isBusy && transcript.pendingApproval == nil } @@ -67,10 +83,11 @@ public final class RunSession { public func submit() { let prompt = draft.trimmed - guard !prompt.isEmpty, !isBusy else { return } + guard !prompt.isEmpty, !isBusy, !awaitingAuthoritativeTerminalState else { return } draft = "" connectionError = nil cancelRequested = false + latestAuthoritativeTerminalState = nil promptHistory.append(prompt) transcript.appendUserPrompt(prompt) @@ -101,6 +118,9 @@ public final class RunSession { let started = try await client.startRun(request) currentRunID = started.runID + if !authoritativeTerminalRunIDs.contains(started.runID) { + awaitingAuthoritativeTerminalState = true + } if self.conversationID == nil { self.conversationID = started.runID } // Keyed by conversation, not by this run: on a conversation's // later runs `self.conversationID` is already the first run's @@ -113,11 +133,15 @@ public final class RunSession { await apply(event, runID: started.runID) } } catch let error as HarnessError { - connectionError = error.message - transcript.markFailed() + if currentRunID == nil || awaitingAuthoritativeTerminalState { + connectionError = error.message + transcript.markFailed() + } } catch { - connectionError = error.localizedDescription - transcript.markFailed() + if currentRunID == nil || awaitingAuthoritativeTerminalState { + connectionError = error.localizedDescription + transcript.markFailed() + } } currentRunID = nil } @@ -175,8 +199,20 @@ public final class RunSession { // MARK: - Conversation switching public func load(messages: [StoredMessage], conversationID: String) { + if self.conversationID == conversationID { + // Selecting the already-open rail row is a refresh, not a + // conversation replacement. Preserve an active stream and any + // accepted-run-to-terminal lock; reconciliation already ignores + // incomplete snapshots while either is unresolved. + reconcilePersistedMessages(messages) + trackConversationStream(conversationID) + return + } streamTask?.cancel() transcript.load(messages: messages) + latestAuthoritativeTerminalState = nil + awaitingAuthoritativeTerminalState = false + authoritativeTerminalRunIDs = [] self.conversationID = conversationID currentRunID = nil connectionError = nil @@ -189,8 +225,10 @@ public final class RunSession { /// section was visible. An active user-started run remains event-driven so /// an incomplete persistence snapshot cannot replace streaming state. public func reconcilePersistedMessages(_ messages: [StoredMessage]) { - guard !isBusy else { return } - transcript.reconcile(messages: messages) + guard !isBusy, !awaitingAuthoritativeTerminalState else { return } + transcript.reconcile( + messages: messages, + authoritativeTerminalState: latestAuthoritativeTerminalState) connectionError = nil pendingQuestions = nil } @@ -203,6 +241,9 @@ public final class RunSession { streamTask?.cancel() stopConversationStream() transcript.reset() + latestAuthoritativeTerminalState = nil + awaitingAuthoritativeTerminalState = false + authoritativeTerminalRunIDs = [] conversationID = nil currentRunID = nil connectionError = nil @@ -210,6 +251,11 @@ public final class RunSession { } public func rebind(conversationID: String) { + if self.conversationID != conversationID { + latestAuthoritativeTerminalState = nil + awaitingAuthoritativeTerminalState = false + authoritativeTerminalRunIDs = [] + } self.conversationID = conversationID trackConversationStream(conversationID) } @@ -284,6 +330,25 @@ public final class RunSession { /// would double every message (issue #950 requirement 4). private func apply(_ event: HarnessEvent, runID: String) async { guard seenEventIDs.insert(event.id).inserted else { return } + switch event.type { + case .runQueued, .runStarted, .runResumed: + latestAuthoritativeTerminalState = nil + awaitingAuthoritativeTerminalState = true + case .runCompleted: + latestAuthoritativeTerminalState = .completed + awaitingAuthoritativeTerminalState = false + authoritativeTerminalRunIDs.insert(runID) + case .runFailed: + latestAuthoritativeTerminalState = .failed + awaitingAuthoritativeTerminalState = false + authoritativeTerminalRunIDs.insert(runID) + case .runCancelled: + latestAuthoritativeTerminalState = .cancelled + awaitingAuthoritativeTerminalState = false + authoritativeTerminalRunIDs.insert(runID) + default: + break + } transcript.apply(event) await handleSideEffects(of: event, runID: runID) } diff --git a/macapp/Sources/HarnessKit/Transcript.swift b/macapp/Sources/HarnessKit/Transcript.swift index db33f35e..d81ca137 100644 --- a/macapp/Sources/HarnessKit/Transcript.swift +++ b/macapp/Sources/HarnessKit/Transcript.swift @@ -369,8 +369,10 @@ extension Transcript { /// cancelled terminal event into a successful run. Failure detail exists /// only on the event stream, so retain those rows across the persisted /// message rebuild as well. - public mutating func reconcile(messages: [StoredMessage]) { - let terminalState = runState + public mutating func reconcile( + messages: [StoredMessage], + authoritativeTerminalState terminalState: RunState? + ) { let terminalErrors = items.compactMap { item -> String? in if case .error(let message) = item.kind { return message } return nil diff --git a/macapp/Tests/GoCodeUITests/RunSessionConversationStreamTests.swift b/macapp/Tests/GoCodeUITests/RunSessionConversationStreamTests.swift index f7f1fbd5..aef1f2e5 100644 --- a/macapp/Tests/GoCodeUITests/RunSessionConversationStreamTests.swift +++ b/macapp/Tests/GoCodeUITests/RunSessionConversationStreamTests.swift @@ -16,6 +16,8 @@ private final class ConversationStreamStub: URLProtocol, @unchecked Sendable { var status: Int = 200 var headers: [String: String] = ["Content-Type": "application/json"] var chunks: [Data] = [] + var waitForRequestPath: String? + var delayMilliseconds: UInt64 = 0 } nonisolated(unsafe) private static var handlers: [String: [Response]] = [:] @@ -48,6 +50,29 @@ private final class ConversationStreamStub: URLProtocol, @unchecked Sendable { Self.handlers[path] = queue.isEmpty ? [next] : queue return next } + if response.waitForRequestPath != nil || response.delayMilliseconds > 0 { + DispatchQueue.global().async { [weak self] in + self?.deliver(response) + } + } else { + deliver(response) + } + } + + private func deliver(_ response: Response) { + if let path = response.waitForRequestPath { + let deadline = ContinuousClock.now.advanced(by: .seconds(5)) + while ContinuousClock.now < deadline { + let observed = Self.lock.withLock { + Self.recorded.contains { $0.url?.path == path } + } + if observed { break } + Thread.sleep(forTimeInterval: 0.005) + } + } + if response.delayMilliseconds > 0 { + Thread.sleep(forTimeInterval: Double(response.delayMilliseconds) / 1_000) + } let http = HTTPURLResponse( url: request.url!, statusCode: response.status, httpVersion: "HTTP/1.1", headerFields: response.headers)! @@ -391,6 +416,146 @@ struct RunSessionConversationStreamTests { session.reset() } + + /// Regression for #1031 and PR #1033 review: `markFailed()` is a local + /// transport placeholder, not proof that harnessd ended the run. An older + /// durable snapshot -- including one loaded by clicking the already + /// selected conversation -- must not report success or permit another + /// submission before the conversation stream delivers an authoritative + /// terminal event; that event can then recover the provisional failure. + @Test("transport failure waits for authoritative completion") + func transportFailureWaitsForAuthoritativeCompletion() async throws { + ConversationStreamStub.reset() + ConversationStreamStub.queue( + "/v1/runs", + [ + .init( + status: 202, + chunks: [Data(#"{"run_id":"run_transport","status":"queued"}"#.utf8)]) + ]) + ConversationStreamStub.queue( + "/v1/runs/run_transport/events", + [ + .init( + status: 500, + chunks: [ + Data( + #"{"error":{"code":"stream_failed","message":"connection dropped"}}"# + .utf8) + ]) + ]) + let completedFrame = """ + id: run_transport:1 + event: run.completed + data: {"id":"run_transport:1","run_id":"run_transport","type":"run.completed","payload":{}} + + + """ + ConversationStreamStub.queue( + "/v1/conversations/run_transport/events", + [ + .init(status: 500), + .init( + status: 200, headers: ["Content-Type": "text/event-stream"], + chunks: [Data(completedFrame.utf8)]), + ]) + + let session = makeSession() + session.draft = "check deployment" + session.submit() + + try await wait { + session.connectionError != nil && session.transcript.runState == .failed + } + + let storedJSON = """ + {"messages":[ + {"role":"user","content":"check deployment","step":0}, + {"role":"assistant","content":"deployment passed","step":0} + ]} + """ + ConversationStreamStub.queue( + "/v1/conversations/run_transport/messages", + [.init(status: 200, chunks: [Data(storedJSON.utf8)])]) + let storedMessages = try JSONDecoder().decode( + StoredMessageEnvelope.self, + from: Data(storedJSON.utf8) + ).messages + session.draft = "start another deployment check" + session.load(messages: storedMessages, conversationID: "run_transport") + + #expect(session.transcript.runState == .failed) + #expect(session.connectionError != nil) + #expect(!session.canSubmit) + + try await wait { + session.transcript.runState == .completed && session.connectionError == nil + } + + session.reset() + } + + /// PR #1033 review regression: harnessd dispatches before returning 202. + /// The conversation stream can therefore complete a fast run before the + /// start response arrives; that late response must not re-lock it. + @Test("terminal event before start response does not relock the completed run") + func terminalBeforeStartResponseDoesNotRelockCompletedRun() async throws { + ConversationStreamStub.reset() + let completedFrame = """ + id: run_early_terminal:0 + event: run.completed + data: {"id":"run_early_terminal:0","run_id":"run_early_terminal","type":"run.completed","payload":{}} + + + """ + ConversationStreamStub.queue( + "/v1/conversations/conv_existing/events", + [ + .init( + status: 200, + headers: ["Content-Type": "text/event-stream"], + chunks: [Data(completedFrame.utf8)], + waitForRequestPath: "/v1/runs") + ]) + ConversationStreamStub.queue( + "/v1/runs", + [ + .init( + status: 202, + chunks: [ + Data( + #"{"run_id":"run_early_terminal","status":"queued"}"#.utf8) + ], + delayMilliseconds: 250) + ]) + ConversationStreamStub.queue( + "/v1/runs/run_early_terminal/events", + [ + .init( + status: 200, + headers: ["Content-Type": "text/event-stream"], + chunks: [Data(completedFrame.utf8)]) + ]) + ConversationStreamStub.queue( + "/v1/conversations/conv_existing/messages", + [.init(status: 200, chunks: [Data(#"{"messages":[]}"#.utf8)])]) + + let session = makeSession() + session.load(messages: [], conversationID: "conv_existing") + session.draft = "fast deployment check" + session.submit() + + try await wait { + ConversationStreamStub.requests.contains { + $0.url?.path == "/v1/runs/run_early_terminal/events" + } && session.currentRunID == nil + } + #expect(session.transcript.runState == .completed) + session.draft = "next deployment check" + #expect(session.canSubmit) + + session.reset() + } } private struct StoredMessageEnvelope: Decodable {