Narrow files-mode echo carryover by source#57
Merged
Conversation
Stop treating ordinary R prompts as eligible for sideband-first echo carryover in the pending output tape. R-owned readline echo now travels through ordered output_text frames, so late raw stdout that looks like an R prompt should remain visible instead of being trimmed by the old race-tolerant fallback. Keep Python-style raw prompt echo eligible for the fallback, update the carryover tests to use Python prompts, and document the remaining raw output boundary in the active plan and output timeline notes. Validation: - cargo test r_prompt_carryover_does_not_trim_late_raw_stdout (failed before implementation) - cargo test pending_output_tape - cargo +nightly fmt - cargo check - cargo build - cargo clippy --all-targets --all-features -- -D warnings - cargo test - cargo +nightly fmt
Review finding: The narrowed prompt eligibility can leak R-owned echoed input when sideband and output_text land in separate drains, which regresses files-mode timeout/poll behavior. Review comment: - [P2] Preserve R echo carryover across drain boundaries — /Users/tomasz/github/posit-dev/mcp-repl/src/pending_output_tape.rs:592-592 In files mode, a timeout or poll can drain the tape after the IPC reader handles `ReadlineResult` but before it handles the following R `output_text` frame. Since `>`, `+`, and `Browse[...]` no longer populate `pending_echo_prefix`, the later `output_text` echo is appended through the same text path without a matching sideband event in that snapshot, so long-running R requests can leak echoed input on an intermediate poll. Keep this carryover for R-owned IPC echo or make the text/result pair atomic or distinguishable from raw stdout. Resolution: - Distinguished raw pipe text from IPC `output_text` in the files-mode pending output tape. - Kept R-style prompt carryover for IPC-delivered `output_text` only, while leaving R-shaped raw stdout visible. - Added regressions for both late raw stdout and late IPC output_text. - Updated the active plan and output timeline docs. Validation: - cargo test r_prompt_carryover_trims_late_ipc_output_text (failed before implementation) - cargo test r_prompt_carryover - cargo +nightly fmt - cargo test pending_output_tape - cargo check - cargo build - cargo clippy --all-targets --all-features -- -D warnings - cargo test - cargo +nightly fmt
Review finding:
The patch introduces source-aware echo carryover but infers source from prompt text alone and asserts on mixed classifications. That can leak Python input echoes for prompt strings like `> ` and can panic on mixed prompt shapes.
Full review comments:
- [P2] Avoid hard-coding prompt spelling as the output source — /Users/tomasz/github/posit-dev/mcp-repl/src/pending_output_tape.rs:679-685
In Python files mode, prompts passed to `input()` are still echoed through the raw PTY, even when the prompt text is `"> "` or `"+ "`. This branch records those prompts as `PendingTextSource::Ipc`, so if the `ReadlineResult` drains before the PTY echo, `leading_echo_match_progress` rejects the later raw stdout and the echoed input leaks into the next reply; before this change, the same prompt-shaped carryover was trimmed.
- [P2] Avoid panicking on mixed prompt-source carryover — /Users/tomasz/github/posit-dev/mcp-repl/src/pending_output_tape.rs:667-670
If one drain contains two sideband-first readline results that fall on different sides of this prompt heuristic, this assertion can panic the server instead of falling back to visible output. For example, buffered Python input for `input('>>> ')` followed by `input('> ')`, or an R session with a Python-shaped primary prompt and the default `+` continuation, can produce mixed classified sources in a single pending echo prefix.
Resolution:
- Added an explicit `echo_source` to pending `ReadlineResult` sideband events.
- Set the echo source from the active backend: raw for Python, IPC `output_text` for R.
- Reworked pending echo carryover to track source per segment instead of asserting on one source for the whole prefix.
- Added regressions for Python raw prompts shaped like R prompts and for mixed-source carryover.
- Updated docs and the active plan to state that prompt spelling controls eligibility, not source.
Validation:
- cargo test python_r_shaped_prompt_carryover_trims_late_raw_echo (failed before implementation)
- cargo test mixed_prompt_source_carryover_does_not_panic (failed before implementation)
- cargo +nightly fmt
- cargo test pending_output_tape
- cargo check
- cargo build
- cargo clippy --all-targets --all-features -- -D warnings
- cargo test
- cargo +nightly fmt
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
This is one small step in the larger refactor to move output onto synchronous IPC, so output text and output-related events travel through one ordered path instead of racing against stdout/stderr.
This PR updates files-mode prompt echo bookkeeping to remember which output path produced the carried echo text. That keeps existing echo de-duplication working as output moves to the synchronous path, without applying carryover from one output path to another.
Python prompt handling is unchanged.
Testing
review-loopis clean.