Skip to content

fix(tui): clarify Ctrl+C status when run is still active#38502

Closed
mwfj wants to merge 28 commits intoopenclaw:mainfrom
mwfj:fix/tui-ctrlc-cancel-hint
Closed

fix(tui): clarify Ctrl+C status when run is still active#38502
mwfj wants to merge 28 commits intoopenclaw:mainfrom
mwfj:fix/tui-ctrlc-cancel-hint

Conversation

@mwfj
Copy link
Copy Markdown

@mwfj mwfj commented Mar 7, 2026

Summary

  • clarify TUI Ctrl+C status messaging when a run is active
  • keep existing behavior unchanged (Ctrl+C still clears input / exits; Esc or /abort stops runs)

Problem

Users can press Enter then Ctrl+C quickly and see local status text that reads like cancellation, while the run still completes and emits a reply.

This is a UX mismatch, not a backend run-cancel bug.

Root cause

Ctrl+C is wired to resolveCtrlCAction (clear/warn/exit) in src/tui/tui.ts; it does not call chat.abort.

Fix

When activeChatRunId exists, append this hint to Ctrl+C status:

  • ; run still active (Esc or /abort to stop)

So users immediately know the run is still in progress and how to stop it.

Scope

  • Only src/tui/tui.ts
  • No changes to gateway RPC, abort semantics, or keybindings

Security impact

  • No new permissions/capabilities
  • No auth/token/network/data-path changes
  • Text-only UX clarification

Linked

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 7, 2026

Greptile Summary

This PR makes a targeted UX-only fix to src/tui/tui.ts: when a user presses Ctrl+C while a backend run is still in progress (indicated by activeChatRunId being set), the "cleared input" and "press ctrl+c again to exit" status messages now include an appended hint — ; run still active (Esc or /abort to stop) — so users immediately understand that Ctrl+C did not cancel the run and how to properly stop it.

Key implementation details:

  • The activeRunHint string is evaluated at handler call-time (line 853), ensuring it always reflects the live value of activeChatRunId
  • The hint is correctly appended only to the "clear" (line 856) and "warn" (line 864) branches
  • The "exit" branch (lines 860–862) intentionally receives no change; the user will have seen the hint on the preceding "warn" Ctrl+C press
  • No behavioral, network, auth, or abort-path changes — this is a pure UX clarification as described

Safe to merge.

Confidence Score: 5/5

  • This PR is safe to merge — it is a text-only UX clarification with no behavioral, network, auth, or abort-path changes.
  • The change is minimal, focused, and correct. It modifies only the Ctrl+C handler to append a hint message when a run is active. The activeRunHint logic correctly reads activeChatRunId at call-time, ensuring the hint reflects the current state. The hint is properly scoped to only the "clear" and "warn" branches, with the "exit" branch intentionally unaffected. No new code paths, permissions, side-effects, or backend changes are introduced.
  • No files require special attention.

Last reviewed commit: 6e23473

@mwfj
Copy link
Copy Markdown
Author

mwfj commented Mar 7, 2026

Update on the recurring secrets CI failure:

  • This failure is not caused by the auth-choice code change in this PR.
  • Root cause is in shared workflow logic on main (.github/workflows/ci.yml): PR secrets scan diffs against github.event.pull_request.base.sha while checkout is shallow (fetch-depth: 1). When the base SHA is missing locally, it falls back to full-repo detect-secrets scan.

Tracking + fix are now split out as requested:

Expect the wait the secrets CI failure fixed by upstream

@mwfj
Copy link
Copy Markdown
Author

mwfj commented Mar 7, 2026

Code is ready to review

@mwfj
Copy link
Copy Markdown
Author

mwfj commented Mar 9, 2026

CI follow-up after conflict resolution:
Two checks are currently failing, and they appear to be upstream/baseline issues rather than specific to this PR's changes:

  • Fails on src/cli/daemon-cli/lifecycle.test.ts formatting drift.
  • Error signature: SecretProviderResolutionError ... path ACL verification unavailable on Windows ...`
    • I couldn't find a clear dedicated tracking PR/issue for this exact failure yet and wait for upstream fixed.

Given this, the issue is not introduced by this PR, and we are Ready to review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f846eb8bcd

ℹ️ 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".

Comment thread src/tui/tui.ts Outdated
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 26, 2026

Codex review: needs changes before merge.

Summary
This PR appends a /abort run-still-active hint to TUI Ctrl+C clear/warn status text when activeChatRunId is set.

Reproducibility: yes. Source inspection shows sendMessage marks a pending optimistic message before activeChatRunId is assigned, so a quick Ctrl+C can still miss this PR's active-run hint.

Next step before merge
A narrow repair can update the hint predicate, add focused coverage, and add the changelog entry without changing cancellation semantics.

Security
Cleared: The diff is limited to TUI status text and does not touch CI, dependencies, package metadata, secrets/auth, network, or code-execution paths.

Review findings

  • [P2] Include pending submissions in the active-run hint — src/tui/tui.ts:1007
  • [P3] Add the required changelog entry — src/tui/tui.ts:1007
Review details

Best possible solution:

Repair this text-only PR so the hint covers both pending optimistic sends and bound active runs, or choose #44223 if maintainers want Ctrl+C to abort active runs instead.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows sendMessage marks a pending optimistic message before activeChatRunId is assigned, so a quick Ctrl+C can still miss this PR's active-run hint.

Is this the best way to solve the issue?

No as written. A status-only clarification is a narrow maintainable fix under the current documented policy, but the predicate must include pending optimistic submissions and the PR needs changelog coverage.

Full review comments:

  • [P2] Include pending submissions in the active-run hint — src/tui/tui.ts:1007
    The hint is gated only on activeChatRunId, but sendMessage sets pendingOptimisticUserMessage before the first chat event assigns activeChatRunId. The quick Enter/Ctrl+C path can still show the old clear/exit status while the run continues, so include the pending state or bind the local run id before deriving the hint.
    Confidence: 0.9
  • [P3] Add the required changelog entry — src/tui/tui.ts:1007
    This changes user-visible TUI status text, but the PR only edits src/tui/tui.ts. Repo policy requires user-facing fix changes to add a single-line CHANGELOG.md entry under the active Fixes section with appropriate human contributor credit.
    Confidence: 0.83

Overall correctness: patch is incorrect
Overall confidence: 0.88

Acceptance criteria:

  • pnpm test src/tui/tui.test.ts src/tui/tui-command-handlers.test.ts src/tui/tui-event-handlers.test.ts src/tui/tui-session-actions.test.ts
  • pnpm exec oxfmt --check --threads=1 src/tui/tui.ts src/tui/tui.test.ts src/tui/tui-command-handlers.test.ts src/tui/tui-event-handlers.test.ts CHANGELOG.md
  • pnpm check:changed

What I checked:

Likely related people:

  • vignesh07: TUI history ties the responsive Ctrl+C flow and optimistic active-run handling to Vignesh Natarajan's merged work, including Ctrl+C exit behavior and active-run preservation. (role: introduced behavior / adjacent owner; confidence: high; commits: b4cdffc7a429, 61a0b0293104, f56f799990a9; files: src/tui/tui.ts, src/tui/tui-command-handlers.ts, src/tui/tui-event-handlers.ts)
  • steipete: Peter Steinberger has multiple recent TUI maintenance commits in local history around phase handling, keybindings, and test seams. (role: recent maintainer; confidence: medium; commits: aa6f6135dbc9, a27624437e96, 97297049e7f1; files: src/tui, src/tui/tui.ts, src/tui/tui.test.ts)
  • Alex Knight: Current main's shallow checkout blames the current TUI Ctrl+C block to the recent squash commit that touched the TUI command/event/test surface. (role: recent adjacent maintainer; confidence: low; commits: 3f732aee83d5; files: src/tui/tui.ts, src/tui/tui-command-handlers.ts, src/tui/tui-event-handlers.ts)

Remaining risk / open question:

  • Merging as-is leaves the quick Enter/Ctrl+C path unresolved because the hint is absent before the first gateway event assigns activeChatRunId.
  • fix(tui): correct Ctrl+C abort handling #44223 proposes a different cancellation policy, so maintainers should avoid landing both directions without choosing the intended Ctrl+C behavior.

Codex review notes: model gpt-5.5, reasoning high; reviewed against bc0b54e84485.

@mwfj
Copy link
Copy Markdown
Author

mwfj commented May 4, 2026

Close PR due to this fix: #77199

@mwfj mwfj closed this May 4, 2026
@mwfj mwfj deleted the fix/tui-ctrlc-cancel-hint branch May 4, 2026 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: TUI Ctrl+C can imply cancellation while active run continues

1 participant