Conversation
Three related findings from a review:
1. TextItem(password=True) was masked in view mode but the edit-mode
BufferControl had no input_processors, so the secret was visible
while editing. README and demos advertise password masking, so this
silently undermined the contract for any user dropping in an API-key
field. Fix: pass [PasswordProcessor()] to BufferControl when
item.password is True.
2. DialogConfig.width was a public field that _ConfigBasedDialog never
copied to BaseDialog.width, so the dimension never reached the
widget. Fix: copy it in __init__.
3. ButtonConfig had focused and style fields but neither was applied
when building Button widgets. Fix:
- Introduce a new BaseDialog._build_buttons() override hook that
defaults to today's behavior (calls get_buttons()) so user
subclasses are unaffected.
- _ConfigBasedDialog overrides _build_buttons to construct each
Button directly, applying ButtonConfig.style by wrapping
Button.window.style (preserves focus class toggling) and
recording the focused button on dialog._initial_focus.
- DialogManager.show focuses dialog._initial_focus after the default
focus is set, so the configured button gets initial focus.
Also masks API-key values in demo_showcase.py and settings_dialog_demo.py
output so demos don't print the field a user just entered as a password.
Adds five tests (settings_dialog: 2; dialog: 3) covering each fix.
fix: close public-contract gaps in settings dialog and DialogConfig
Rich's Console treats force_terminal and no_color as orthogonal: with force_terminal=True we still get a real terminal, but NO_COLOR=1 in the environment strips every ANSI escape sequence Rich would have emitted. That's the wrong layer to honor NO_COLOR. _renderable_to_ansi (and the public APIs that build on it — rich_to_ansi, StreamingContent.append_rich, StreamingContent.set_line_rich) advertise ANSI-formatted output for the thinking box. Silently stripping styling whenever NO_COLOR is set leaves those callers with the literal text and no signal that anything was dropped, and made four tests environment-sensitive (they pass with the env var unset, fail with NO_COLOR=1). Pass no_color=False to the Rich Console so the converter is deterministic regardless of the user's environment. NO_COLOR belongs at the terminal- output layer, not at a converter that exists specifically to emit ANSI. prompt_toolkit's output layer is the right place for any future NO_COLOR-aware rendering decisions. Adds a regression test that monkeypatches NO_COLOR=1 and asserts ANSI escapes are still present in the output.
fix: ignore NO_COLOR in _renderable_to_ansi
…d exception cleanup Three coupled gaps in ThinkingPromptSession's input lifecycle: 1. Ctrl+C did not cancel the running handler. The key binding finished active thinking boxes and cancelled the pending-input future, but the handler coroutine was awaited inline with no task handle, so a long- running handler kept executing after the UI signaled "cancel" and could still emit a final response. 2. Input submitted while a handler was running was silently dropped. accept_handler echoed the text to history, cleared the buffer, and tried to deliver it via _pending_input — but _pending_input was already done (the previous prompt had resolved it), so the new text never reached input_loop. The user lost the command with no signal that anything had gone wrong. 3. Handler exceptions left thinking boxes stuck active. The except path logged the error but didn't finish boxes the failing handler had created; for callers using start_thinking() (manual API) the UI was permanently in "thinking" state until restart. Solution: - Add ThinkingPromptSession._run_handler(handler, text) that runs the handler as a tracked asyncio.Task stored on _current_handler_task. CancelledError flagged via _user_cancelled_handler is treated as a Ctrl+C (swallowed, boxes finished, loop continues); other Cancellations propagate so the input loop can exit cleanly. Exceptions trigger _cleanup_after_handler() before being logged. - Ctrl+C key binding cancels _current_handler_task (after marking _user_cancelled_handler), finishes boxes, cancels pending input, and only falls through to app.exit() when nothing is in flight. - accept_handler refuses to deliver input while _current_handler_task is running: leaves the buffer intact, sets a "Busy — press Ctrl+C to cancel" status hint, and returns False so prompt_toolkit does not echo or clear. - input_loop simplifies to "await self._run_handler(...)"; all retry / cleanup / cancellation logic now lives in one place. Adds tests/test_session_lifecycle.py with 9 tests covering each path: async/sync handler tracking, exception cleanup, user vs outer cancel, busy-input drop, and the Ctrl+C key binding (cancels handler, exits when idle).
refactor: session handler lifecycle (cancel, busy-gate, exception cleanup)
Two follow-up bugs in the session-lifecycle refactor:
1. Ctrl+C exited the app after clearing an orphan thinking box.
The Ctrl+C binding finished active boxes via finish_all() and then
re-checked self._manager.has_active_boxes for the "idle, exit"
fallback. After finish_all the manager is empty, so the same Ctrl+C
that cleared the boxes also called event.app.exit().
Snapshot handler_running / had_active_boxes / had_pending_input
before mutating any of them, and only fall through to exit when none
of them were in flight at keypress time. has_active_boxes is also
the right idle signal for boxes opened outside a handler (which the
reviewer flagged), since previously such boxes would be cleared but
the user would also lose their session.
2. _user_cancelled_handler stayed True if the inner cancel did not
propagate as CancelledError out of `await task`.
The flag was only reset inside `except asyncio.CancelledError`, so:
- if user code caught CancelledError and returned normally, the
outer await returned a value (no exception) and the flag stuck;
- if cancel raced against natural completion and lost, same thing.
In both cases the *next* handler invocation that received an outer
cancellation (e.g. session shutdown) would have the leftover True
flag, _run_handler would treat it as Ctrl+C, swallow CancelledError,
and the input loop could not exit.
Reset _user_cancelled_handler in `finally` so it's always cleared
regardless of whether CancelledError propagated. The except branch
no longer needs to reset it explicitly.
Tests: extends test_session_lifecycle.py with one Ctrl+C test
(orphan box must not also exit) and three flag-reset tests:
- handler swallows CancelledError (uses a `started` event so cancel
arrives during await rather than before first tick — otherwise the
task is cancelled before its body runs and the bug is masked);
- cancel races against natural completion;
- outer cancel after a swallowing handler must propagate.
…y-cancel-flag fix: Ctrl+C double-action and sticky _user_cancelled_handler flag
Bug-fix release covering Ctrl+C and handler lifecycle hardening, settings dialog public-contract gaps, and NO_COLOR handling in the ANSI converter. See CHANGELOG.md for details.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bug-fix release. Highlights:
_user_cancelled_handlerflag reset infinallyso it can't stay sticky.TextItem(password=True)now masks during editing;DialogConfig.widthactually applied;ButtonConfig.styleandButtonConfig.focusedhonored._renderable_to_ansi(andrich_to_ansi,StreamingContent.append_rich/set_line_rich) ignoreNO_COLOR=1— these APIs explicitly emit ANSI for the thinking box.See
CHANGELOG.mdfor the full list.Test plan
pytest— 368/368 passingv0.3.2on main