fix(logging): resolve sys.stderr at emit time so log records stop scribbling over the prompt - #301
Open
Ken Chau (kenotron-ms) wants to merge 1 commit into
Conversation
…ibbling over the prompt Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
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
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.
The defect
_configure_console_logging()installs the process's only root logging handler, and it did so with a stocklogging.StreamHandler(sys.stderr)(main.py:229). CPython binds that stream eagerly:main()calls_configure_console_logging()at process entry (main.py:4621), long before anypatch_stdout()context exists. Andpatch_stdout()works by rebinding the names:Rebinding a name cannot reach an object something else already captured. So every
logger.warning(...)in the process wrote straight past the proxy, into a terminal prompt_toolkit was actively rendering into — the text landed at the cursor, on top of the input box, with none of the erase/redraw thatrun_in_terminalexists to provide.Rich never had this bug, and the reason is already recorded in this repo as a load-bearing assumption —
steering_input.py:10-14:That is exactly right (
rich/console.py:762). It was simply never extended tologging. This PR extends it.Why there is no safe window
A prompt_toolkit
Applicationis live essentially all the time in an interactive session:main.py:3853patch_stdout(raw=True)around the whole turnmain.py:3854->steering_input.py:392SteeringInputManager.run()pins a prompt at the bottom for the turn's entire durationmain.py:4014patch_stdout()aroundprompt_async()between turnsThere is no gap to land a log record in. Nor is there any handler-level mitigation to lean on:
logging.disable,NullHandler,removeHandler,QueueHandler,MemoryHandlerare 0 hits each acrossamplifier_app_cli/andtests/, and the two existing filters act on record content (ui/log_filter.py:32) and exception metadata (main.py:235) — neither touches stream binding.The fix
_LateBoundStderrHandler(logging.StreamHandler)makesstreama property that resolvessys.stderron every access. Five lines of behavior; the rest is the docstring explaining why.The setter is required, not decorative.
StreamHandler.__init__assignsself.stream, andsetStream()assigns it again — a bare read-only property raisesAttributeErrorduring construction. The setter distinguishes the two cases:sys.stderr(all__init__does)setStream(open(...)))StreamHandlersemantics preservedNonesetLevel, the existing_suppress_tracebackfilter andLLMErrorLogFilterare untouched — they act on records, not streams. NoFileHandlerpath is affected: this repo constructs exactly one logging handler anywhere (main.py:229) and defines no otherlogging.Handlersubclass.This fixes every
logger.warningin the process at once, not one message.Evidence: real pty, raw bytes
A mock, a
StringIO, or a captured-stderrassertion passes identically on the broken and the fixed build — both variants log the same string, at the same level, through the same filters. The only thing that differs is where the bytes land relative to the prompt render, which is observable only on a real terminal.tests/test_log_over_prompt_integration.pydrives a real pty, puts a liveprompt_async()prompt up, and emits the warning from a background asyncio task (matching the real trigger — see below).Measured on Linux, prompt_toolkit 3.0.52 — bytes between the completed prompt render and the log text:
The tests are not vacuous — shown by mutation
streamproperty eager again (pin at__init__)test_late_bound_handler_routes_log_text_through_run_in_terminal,test_late_bound_handler_is_transparent_without_patch_stdoutmain.py:229tologging.StreamHandler(sys.stderr)test_configure_console_logging_installs_the_late_bound_handlerThe wiring test exists specifically because the pty tests construct the handler directly and would keep passing if
_configure_console_logging()regressed.test_stock_streamhandler_injects_log_text_at_the_cursordeliberately asserts the broken behavior. If CPython or prompt_toolkit ever changes such that an eagerly-bound handler stops corrupting the prompt, that test fails and this fix gets re-evaluated instead of cargo-culted.Evidence: isolated environment, real installed wheel
Verified in an isolated Incus container (Digital Twin Universe
dtu-30bb4c32, since destroyed) running Amplifier installed the way a user installs it —uv tool install git+https://github.com/microsoft/amplifier— with a URL rewrite pointing theamplifier-app-clidependency at this branch. Not an editable install, not the working tree:A real two-turn interactive
amplifier run --mode chatsession in that container, configured with two different-vendor providers (anthropic at the lowest numeric priority, so it answers the session) androuting.matrix: openai— which makeshooks-session-namingemit its genuine cross-provider refusal on turn 2. Raw bytes off the pty, at the moment the warning lands:The input box is not touched.
What this PR does not do
The session-naming warning is correct and is deliberately left alone. It reports a real routing misconfiguration — a
model_roleresolving to a foreign vendor, which naming correctly refuses to borrow — andhooks-session-namingis right to say so. Nothing here silences, filters, downgrades or defers it. That message's content is tracked separately as microsoft-amplifier/amplifier-support#511; this change is to the display path only, and it applies equally to every otherlogger.warningin the process.Honest limits
amplifier-user-simprofile pattern. Docker was unavailable on the verification host, so the rewrite target was a public fork whosemaincarried this exact commit — structurally the same mechanism (sameurl_rewritesmitmproxy path, same@mainref), different host.@pytest.mark.integrationandpytest.skipon win32, matching every other pty test here and the deliberatematrix.osexclusion inci.yml. The fix itself is platform-independent.Application.cpr_not_supported_callbackwrites it viaself.output(already wrapped inrun_in_terminal), and it only appears because the harness's pty does not answer CPR. Out of scope, unaffected either way.Suite
35ab604)uv run pytest -quv run pytest -m integration -qPass count is unchanged; the 5 new tests are integration-marked, hence
deselected13 -> 18 by default and+5under-m integration.ruff check amplifier_app_cli/main.pyproduces a byte-identical finding set before and after this change;ruff formatwould touch the same 4 lines on both (pre-existing debt, deliberately not swept into this diff). The new test file isruff checkandruff formatclean.