fix: Ctrl+C cascade state machine (clear → abort → exit) - #941
Conversation
|
📚 No documentation changes were needed for this PR. |
Kimchi Code Review
Summary📊 Review Score: 85/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Unit tests added for the pure 📝 Found 1 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 85/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 2/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Unit tests added for the pure ctrlCCascadeDecision function cover all three outcomes and the priority ordering. The integration path inside onTerminalInput is not directly tested, which is reasonable given the current test style, but the decision logic itself is well covered.
📝 Found 1 issue(s). See inline comments for details.
5ca1fd9 to
e49ffb0
Compare
Previously, Ctrl+C while the agent was streaming would simultaneously abort the agent AND clear the editor text (because the terminal input listener called abort() but returned undefined, letting the event flow through to upstream's app.clear handler). This meant the user lost their in-progress text without any way to just clear it first. Now Ctrl+C follows a cascading state machine: 1. Text exists → clear text (let upstream handle it) 2. No text, streaming → abort agent (consume event) 3. No text, idle → let upstream handle double-press-to-exit A status hint 'Ctrl+C again to abort' is shown after the first press clears text while the agent is streaming, and is cleared on turn_end. The decision logic is extracted into a pure function ctrlCCascadeDecision() for testability. ESC behavior is unchanged (immediate abort, no text clearing). Test: added ctrlCCascadeDecision unit tests covering all state transitions.
…Time The review identified a severe issue: the abort stage consumes the event, preventing upstream's handleCtrlC from updating lastSigintTime. This meant the exit stage's 500ms window was measured from the clear press (stage 1), not the abort press (stage 2), causing: 1. Unintended exit on rapid triple-press (clear → abort → exit within 500ms measured from clear, not abort) 2. Unreliable '3rd press exits' for slower users (>500ms from clear to exit) Fix: the exit stage now checks its own lastCtrlCTime (updated on every press, including abort) and calls shutdown() directly. The event is consumed in the exit stage so upstream's handleCtrlC never fires, preventing a competing lastSigintTime that could cause unintended exits later.
The codebase uses brief comments explaining why (1-5 lines), not what. Trimmed the listener block comment from 14 to 3 lines, the JSDoc from 10 to 1 line, and each switch case comment to 1 line.
2582970 to
433a61c
Compare
2 participants
Problem
When the agent is working and the user navigates through previous messages with up/down arrows, pressing Ctrl+C to clean the input box also aborts the agent — requiring a "please continue" prompt afterward.
Root cause
In
src/extensions/ui.ts, the globalonTerminalInputlistener intercepted Ctrl+C (\x03) and calledcurrentCtx.abort()when the agent was streaming, but returned plainundefinedinstead of{ consume: true }. Sinceundefineddoesn't stop propagation in pi-tui'shandleInput, the event flowed through to upstream'sapp.clearhandler (handleCtrlC()) which cleared the editor text. So Ctrl+C simultaneously aborted the agent AND cleared text.Solution
Replaced the single-shot abort with a cascading state machine:
app.clear{ consume: true })The exit timing is managed entirely in the extension layer via
lastCtrlCTime(updated on every press), not upstream'slastSigintTime, to avoid a timing gap where the abort press consumes the event without updating upstream's timer.A status hint "Ctrl+C again to abort" is shown after the first press clears text while the agent is streaming, and is cleared on
turn_end.The decision logic is extracted into a pure function
ctrlCCascadeDecision(hasText, isStreaming)for testability. ESC behavior is unchanged (immediate abort, no text clearing).Test plan
ctrlCCascadeDecisioncovering all state transitionsCloses #944