Skip to content

Conversation

@gajzzs
Copy link

@gajzzs gajzzs commented Dec 13, 2025

#430

Summary by CodeRabbit

  • New Features
    • Added an output sanitizer that normalizes line endings and removes stray carriage returns from console output on Windows, improving console readability.
  • Bug Fixes
    • Improved stdout handling during stdio operations to ensure consistent LF newlines and prevent unwanted formatting characters in console output, reducing display glitches.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 13, 2025

Walkthrough

On Windows, stdout is switched to binary mode and replaced with a CRStripper-wrapped TextIOWrapper (line-buffered, newline='\n'); FastMCP started over stdio runs inside a redirected stdout context so all stdout writes have CR (\r) characters removed and use LF newlines.

Changes

Cohort / File(s) Summary
Stdout handling / entrypoint
Server/src/main.py
Adds Windows-only msvcrt.setmode to set stdout binary, wraps sys.stdout with a CRStripper-enabled io.TextIOWrapper (newline='\n', line_buffering=True), and uses redirect_stdout around the stdio startup path so stdout writes have CRs stripped and normalized to LF during stdio operation.
CR stripping utility & export
Server/src/utils/cr_stripper.py, Server/src/utils/__init__.py
New CRStripper class that strips \r from bytes/str on write, forwards flush() and delegates other attributes; package __init__ re-exports CRStripper and adds it to __all__.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Inspect CRStripper.write behavior for bytes vs str inputs and its return value semantics.
  • Verify io.TextIOWrapper wrapping sys.stdout.buffer with newline/line_buffering and encoding interactions.
  • Confirm Windows msvcrt.setmode is correctly guarded and doesn't affect non-Windows runs.

Poem

🐇 I nibble at carriage returns with care,
Swapping clumsy CRs for sleek LF air.
I wrap, I buffer, I hop through each line,
Making stdout tidy, one byte at a time.
🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change: fixing an error related to invalid trailing data in a stream by implementing CRLF stripping on stdout.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0f89329 and 2c09a78.

📒 Files selected for processing (2)
  • Server/src/main.py (2 hunks)
  • Server/src/utils/cr_stripper.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Server/src/main.py (1)
Server/src/utils/cr_stripper.py (1)
  • CRStripper (14-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Sourcery review
🔇 Additional comments (1)
Server/src/main.py (1)

414-423: LGTM: Stdio-only application is correct.

The CRStripper-wrapped stdout is correctly applied only for stdio transport mode, not HTTP. The redirect_stdout context ensures that all stdout writes during mcp.run() go through the wrapper, addressing the Windows CRLF issue in the MCP protocol handshake.

As msanatan noted, broader community testing is the remaining validation step to ensure this doesn't break other stdio clients.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e0ef23 and aa81ee6.

📒 Files selected for processing (1)
  • Server/src/main.py (1 hunks)

@gajzzs
Copy link
Author

gajzzs commented Dec 14, 2025

@dsarno is there any issue in this PR ?

@msanatan
Copy link
Contributor

msanatan commented Dec 15, 2025

Thanks for your help with this @gajzzs, from discussions on the issue and this PR.

Can you help me fully understand your changes and answer these questions?

msvcrt.setmode(sys.stdin.fileno(), os.O_BINARY)

Why does stdin need to change as well?

Wouldn't this only need to be applied for stdio mode, and not HTTP mode?

Is it possible to apply this fix w/o globally rewriting sys.stdout

Can you do the byte replacement on sys.stdout.buffer, and not the entire sys.stdout object?

For this fix to work, we need to patch stdout before running mcp.run right? If that's true, isn't it better to add this code to utils as a new Python module, and then import it to main.py? It looks quite messy in it's current location

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
Server/src/utils/cr_stripper.py (1)

3-5: Consider adding type hints and a docstring.

While the implementation is functionally correct, adding type hints and a proper docstring would improve maintainability.

🔎 View suggested improvements:
+from typing import Any, BinaryIO, TextIO, Union
+
 # This utility strips carriage return characters (\r) from bytes or strings 
 class CRStripper:
+    """
+    Stream wrapper that removes carriage return (CR) characters from output.
+    
+    Wraps any stream and strips \\r characters from both bytes and string data
+    before writing to the underlying stream.
+    """
-    def __init__(self, stream):
+    def __init__(self, stream: Union[BinaryIO, TextIO]) -> None:
         self._stream = stream
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa81ee6 and 0f89329.

📒 Files selected for processing (3)
  • Server/src/main.py (2 hunks)
  • Server/src/utils/__init__.py (1 hunks)
  • Server/src/utils/cr_stripper.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • Server/src/main.py
🧰 Additional context used
🧬 Code graph analysis (1)
Server/src/utils/__init__.py (1)
Server/src/utils/cr_stripper.py (1)
  • CRStripper (3-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Sourcery review
🔇 Additional comments (3)
Server/src/utils/__init__.py (1)

1-6: LGTM! Clean package initialization.

The package structure follows Python conventions with appropriate exports via __all__.

Server/src/utils/cr_stripper.py (2)

14-15: LGTM!

The flush method correctly delegates to the underlying stream.


17-18: The __getattr__ delegation is well-suited for this use case. The implementation wraps sys.stdout.buffer (a standard BufferedWriter) rather than sys.stdout, and only explicitly implements write() and flush()—the critical methods for MCP's stdio protocol. Other buffer methods delegate cleanly via __getattr__ to the underlying BufferedWriter, which has standard io module interfaces. Type checking is not a concern since CRStripper is wrapped inside TextIOWrapper, which is the actual stream interface. This approach already addresses the maintainer's compatibility question by operating on the buffer layer rather than replacing the entire stream object.

@msanatan
Copy link
Contributor

Looking good @gajzzs. I left one more comment style wise. The only thing pending after those changes would be other people testing, just to ensure it doesn't break other clients. I'll ask some in the community

gajzzs and others added 2 commits December 20, 2025 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants