Fix TUI waiting conversation overlay - #1061
Conversation
|
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. |
![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: e2686f445a
ℹ️ 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".
|
@codex review Please review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53fb2ce1e1
ℹ️ 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".
| callID: p.CallID, | ||
| generation: m.askUserGeneration, | ||
| } | ||
| cmds = append(cmds, fetchAskUserPendingCmd( |
There was a problem hiding this comment.
When the TUI receives a real run.waiting_for_user event immediately after it is emitted, this GET can race ahead of pending-input registration: runner_step_engine.go:1105 emits the event before runTools.Execute at line 1201 invokes the AskUser broker, whose Ask method installs the pending entry. If the GET wins, /input returns 409, askUserFetchErrorMsg clears the overlay, and no retry or later event re-fetches the question, so the run remains blocked and the user never sees the prompt. Retry the transient no-pending response or render/fetch only after registration is guaranteed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and mapped to existing issue #1054 and PR #1055. That PR changes the owning server invariant so pending input is readable/durable before waiting_for_user status/event publication; its exact-head fast/race checks are green. Issue #1058 and PR #1061 intentionally scope this branch to TUI envelope identity plus late-resume/supersession correlation, so I did not add a second client retry workaround here. Current main does not yet contain #1055, so I am leaving this thread unresolved and recording #1055 as the residual integration dependency rather than claiming the P1 is fixed on this branch.
53fb2ce to
221ab05
Compare
|
Rebase verification evidence:
The late-resume and superseded-call/generation AskUser regressions remain present and green. No baseline failure appeared. @codex review Please review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 221ab05da6
ℹ️ 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".
| m.askUser = askUserState{ | ||
| active: true, |
There was a problem hiding this comment.
When a production run.waiting_for_user event arrives, this marks askUser active before the GET has returned any questions. If that request is slow or unreachable, renderAskUserOverlay displays nothing, while the tea.KeyMsg branch routes every key to handleAskUserKey, which immediately no-ops because questions is empty; even Esc and Ctrl+C cannot dismiss or cancel the run until the HTTP client's 10-second timeout expires. Keep the loading state from claiming overlay key priority, or explicitly handle cancellation keys while the fetch is pending.
Useful? React with 👍 / 👎.
Resolved against main's newer TUI work (context meter, tool-call grouping, provider-key provenance, config persistence). model.go and bridge.go merged cleanly; the one code conflict was messages.go, where main added Conversation and ConversationID to SSEEventMsg after this branch was cut — main's fields are kept. Append-only logs and plan indexes keep both sides.
1 participant
Closes #1058
Summary
Preserves the SSE envelope's top-level
run_idon the TUI's decoded event message and uses that canonical identity whenrun.waiting_for_userfetches pending input. A production-shape acceptance regression now proves the visible AskUserQuestion overlay can submit an answer, dismiss on resume, and render the later assistant continuation.Scope and issue reconciliation
The final diff matches #1058. In scope: TUI envelope decoding, the waiting handler, production-shape bridge/model/input coverage, replay compatibility, corrected synthetic fixtures, and required plans/logs. Out of scope and unchanged: PR #1055's server-side pending-read ordering, native GUI, web, ACP, approvals, callbacks, cron execution, provider/model/tool catalogs, and completion status for epics #1000/#1010. There were no design deviations.
Impact analysis reconciliation
cmd/harnesscli/tui/bridge.gonow decodes top-levelrun_idintoSSEEventMsg.RunID;model.goconsumes it for the existing pending-input fetch./v1/runs/{id}/inputare unchanged.Rawremains payload-only.Last-Event-IDreplay use the same decoder; backpressure, reconnect, cancellation, deadlines, resume, and terminal handling are unchanged.Architecture and duplication check
Searched
StartSSEBridge*,sseEnvelope,decodeSSE,SSEEventMsg, the model event switch, AskUserQuestion fetch/submit/render state, reconnect/Last-Event-ID, server input handlers, server event shape, non-TUI AskUser handling, and open PR ownership. The existingsseEnvelope -> SSEEventMsg -> Model.Updateboundary remains the only normalization point. No payload mutation, fallback decoder, or second run-ID source was introduced. Open PR #1055 owns server ordering and does not touchcmd/harnesscli/tui.Test-first evidence
Red command:
TMPDIR=/private/tmp/gocode-1058-tmp GOCACHE=/private/tmp/gocode-1058-cache go test ./cmd/harnesscli/tui -run TestAskUser_WaitingEnvelopeThroughBridge_ShowsSubmitsAndContinues -count=1 -vObserved failure on the pre-fix decoder:
The event crossed the real SSE bridge, but no input request occurred because top-level run identity was discarded. After retaining
run_idonSSEEventMsgand consuming it in the waiting handler, the same test passed in 0.01s.Green command: the red command above, unchanged.
Refactor/characterization evidence: existing model-only fixtures were corrected from a synthetic nested
run_idtoSSEEventMsg.RunIDplus payload-onlycall_id. A reconnect control asserts the exactLast-Event-ID, canonical decoded run ID, event ID, and absence of a duplicated payload run ID.Verification evidence
PASS:
go test ./cmd/harnesscli/tui -run TestAskUser_WaitingEnvelopeThroughBridge_ShowsSubmitsAndContinues -count=1 -vgo test ./cmd/harnesscli/tui -run 'AskUser|WaitingForUser|WaitingEnvelope|SSEBridge' -count=1go test -race ./cmd/harnesscli/tui -run 'AskUser|WaitingForUser|WaitingEnvelope|SSEBridge' -count=1go test ./cmd/harnesscli/tui -count=1(40.800s)go test -race ./cmd/harnesscli/tui -count=1(40.389s)./scripts/test-regression.sh— normal PASS, complete race PASS,coveragegate: PASS (total=85.6%, min=80.0%, zero-functions=0).Real-path proof: the acceptance uses an actual localhost SSE server and the production
StartSSEBridge, Bubble TeaModel.Update/View, GET pending input, keyboard Enter, POST answer, resume event, assistant delta, and terminal event. It asserts visible question/options, exact request counts/body, and continued transcript.Environment evidence: the first full gate in detached tmux failed both real-Keychain tests because
security(1)waited on the controlling terminal and was killed at 15 seconds. This was not waived. The entire script was rerun in the repository-documented logged-in foreground context;internal/modelstorepassed in both normal and race phases and the full script completed green.Hosted checks are required and will be reported from this PR before handoff.
Rollout and rollback
Ship with the next
harnesscliclient build; no server deployment order, migration, flag, or data repair is required. Monitor stalled-wait and pending-input-fetch reports. Roll back by reverting commite2686f445ab13cc7608d7f750874b5bbed4878a8if unrelated TUI event decoding or reconnect behavior regresses. Already-pending server runs remain durable and answerable through the existing API.Documentation
Added the Issue #1058 plan and one-page impact map; updated the plans index and active plan; recorded command/user intent, symptom/cause/fix, wire-shape/testing observations, system ownership, valid red/green evidence, the Keychain execution-context failure, and the successful full rerun. Public docs, runbooks, release notes, and operator routes are unchanged because this restores already-intended behavior without changing a public contract.
Contract checklist