fix: stop writing to stdout, which carries the stdio JSON-RPC protocol - #359
Merged
Conversation
The default transport is stdio, where stdout is the JSON-RPC channel --
as __main__.py already notes: "Written to stderr because stdout carries
the stdio protocol". Two live code paths wrote to it anyway, interleaving
plain text into the framing the client is parsing.
tree/service.py printed every word of every EditControl, DocumentControl
and ImageControl encountered during traversal:
word_elements.append((word,box))
print(word,box) # <- removed
This runs on any snapshot containing an editable control, once per word,
so a single text field can emit dozens of lines into the protocol stream.
It is debug leftover from the word-bounding-box work: the value is
already accumulated on the line above and the print does nothing else.
infrastructure/analytics.py printed a per-tool-call summary that the very
next line already sends to the logger, so removing the print loses
nothing -- the logger writes to stderr, where this belongs.
Adds tests/test_no_stdout_writes.py, a static AST scan of the source
tree. It counts only ast.Call nodes, so prints inside docstring examples
are excluded for free, and it ignores print(..., file=...) since that is
not a stdout write. Two narrow exemptions are encoded: prints lexically
inside an `if ...debug...:` block, and threadFunc inside RunByHotKey, the
upstream uiautomation hotkey utility the server never invokes. Those
cover the three remaining prints in uia/controls.py; verified that an
ungated print added anywhere in that same file is still caught.
Not run locally: the test suite needs pywin32, which has no macOS wheels,
so this was verified by static analysis and by exercising the scanner
directly. CI runs on windows-latest.
PR Summary by QodoStop stdout writes that corrupt the stdio JSON-RPC channel
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
16 rules 1. Single-quoted '<module>' literal
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| def test_no_bare_print(self, path: Path): | ||
| offenders = _find_stdout_prints(path) | ||
| assert offenders == [], "\n".join( | ||
| f"{path.relative_to(SRC_ROOT)}:{line} in {func or '<module>'} " |
There was a problem hiding this comment.
1. Single-quoted '' literal 📘 Rule violation ✧ Quality
A new string literal uses single quotes ('<module>') instead of the required double quotes. This
violates the repository-wide string literal quoting rule and may lead to inconsistent formatting
across files.
Agent Prompt
## Issue description
A string literal uses single quotes (`'<module>'`) instead of double quotes.
## Issue Context
The codebase enforces double quotes for all string literals.
## Fix Focus Areas
- tests/test_no_stdout_writes.py[101-101]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+90
to
+99
| class TestNoStdoutWrites: | ||
| def test_source_tree_was_found(self): | ||
| """Fail loudly rather than passing vacuously on a bad path.""" | ||
| assert _source_files(), f"no Python sources under {SRC_ROOT}" | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "path", _source_files(), ids=lambda p: str(p.relative_to(SRC_ROOT)) | ||
| ) | ||
| def test_no_bare_print(self, path: Path): | ||
| offenders = _find_stdout_prints(path) |
There was a problem hiding this comment.
2. Missing docstrings in tests 📘 Rule violation ✧ Quality
The public class TestNoStdoutWrites and its public method test_no_bare_print were added without Google-style docstrings. This reduces readability and violates the required documentation standard for public APIs in changed files.
Agent Prompt
## Issue description
Public class/method definitions in the newly added test file are missing required Google-style docstrings.
## Issue Context
The compliance checklist requires Google-style docstrings on public functions and classes in changed files.
## Fix Focus Areas
- tests/test_no_stdout_writes.py[90-99]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+91
to
+99
| def test_source_tree_was_found(self): | ||
| """Fail loudly rather than passing vacuously on a bad path.""" | ||
| assert _source_files(), f"no Python sources under {SRC_ROOT}" | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "path", _source_files(), ids=lambda p: str(p.relative_to(SRC_ROOT)) | ||
| ) | ||
| def test_no_bare_print(self, path: Path): | ||
| offenders = _find_stdout_prints(path) |
There was a problem hiding this comment.
3. Test methods lack type hints 📘 Rule violation ✧ Quality
Newly added test methods do not fully annotate their signatures (missing return types and unannotated parameters). This violates the requirement that all function signatures include explicit type hints.
Agent Prompt
## Issue description
New `def` methods in the added test file are missing required type annotations (e.g., return types like `-> None`, and parameter annotations as required by the rule).
## Issue Context
The compliance checklist requires explicit type hints for all function signatures, including methods.
## Fix Focus Areas
- tests/test_no_stdout_writes.py[91-99]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+64
to
+76
| if ( | ||
| isinstance(node, ast.Call) | ||
| and isinstance(node.func, ast.Name) | ||
| and node.func.id == "print" | ||
| ): | ||
| # `print(..., file=sys.stderr)` is not a stdout write. | ||
| writes_elsewhere = any(kw.arg == "file" for kw in node.keywords) | ||
| function = _enclosing_function(ancestors) | ||
| if ( | ||
| not writes_elsewhere | ||
| and not _is_debug_gated(ancestors) | ||
| and function not in EXEMPT_FUNCTIONS | ||
| ): |
There was a problem hiding this comment.
4. Print(file=stdout) bypass 🐞 Bug ≡ Correctness
The new stdout-print guard treats any print(..., file=...) as non-stdout, so `print(..., file=sys.stdout) (or file=None`) will not be flagged even though it still writes to stdout and can corrupt the stdio JSON-RPC stream. This weakens the regression test’s ability to prevent reintroducing the original bug class.
Agent Prompt
## Issue description
The static scan exempts *any* `print()` call that passes a `file=` keyword, but `file=sys.stdout` and `file=None` still write to stdout.
## Issue Context
This test is meant to prevent corruption of the stdio JSON-RPC protocol by stdout writes.
## Fix Focus Areas
- tests/test_no_stdout_writes.py[64-77]
## Implementation notes
- Change the logic to treat `file=` as safe only when it is clearly **not** stdout.
- At minimum, flag these as offenders:
- `file=None`
- `file=sys.stdout`
- `file=sys.__stdout__`
- Keep allowing `file=sys.stderr` / `file=sys.__stderr__` and (optionally) other non-stdout streams.
- Add small unit-like cases (in this same test module) that build/parse a tiny snippet and assert the scanner catches `file=sys.stdout` / `file=None` and does not catch `file=sys.stderr`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+37
to
+44
| def _is_debug_gated(ancestors: list[ast.AST]) -> bool: | ||
| """True if any enclosing `if` tests something named like a debug flag.""" | ||
| for node in ancestors: | ||
| if isinstance(node, ast.If): | ||
| test_source = ast.dump(node.test).lower() | ||
| if "debug" in test_source: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
5. Overbroad debug exemption 🐞 Bug ☼ Reliability
The debug-gating exemption triggers if the enclosing if condition’s AST dump contains the substring "debug", which also exempts prints in cases like if not debug: where the print would run during normal (non-debug) operation. This can allow stdout prints to slip through the guard despite being active in production paths.
Agent Prompt
## Issue description
`_is_debug_gated()` uses a substring search over `ast.dump(node.test)`, which can mark non-debug (or inverted) conditions as "debug gated" and wrongly exempt stdout prints.
## Issue Context
This is a regression guard; false negatives reduce confidence that stdout stays clean for the JSON-RPC protocol.
## Fix Focus Areas
- tests/test_no_stdout_writes.py[37-44]
## Implementation notes
- Replace the substring heuristic with a structural check that only returns True for clearly-positive debug flags, e.g.:
- `if debug:` / `if DEBUG:`
- `if self.debug:` / `if ctx.debug:`
- Ensure it does **not** treat `if not debug:` or other negated forms as gated.
- Consider limiting matches to `ast.Name`/`ast.Attribute` identifiers containing `debug` rather than arbitrary dumped text.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
The default transport is stdio, where stdout is the JSON-RPC channel.
__main__.py:189already says as much — "Written to stderr because stdout carries the stdio protocol" — but two live code paths write to it anyway, interleaving plain text into the framing the client is parsing.tree/service.pyThis runs inside
tree_traversal, for everyEditControl/DocumentControl/ImageControlencountered, once per word. A single text field can emit dozens of lines into the protocol stream; a document control can emit hundreds.It looks like leftover debugging from the word-bounding-box work — the value is already accumulated on the line above, and the print does nothing else. The enclosing
try/except Exception: passdoesn't help, since the write has already happened by the time anything could raise.infrastructure/analytics.pyThe line immediately below sends the identical message to the logger, which writes to stderr. Removing the print loses nothing.
Regression guard
tests/test_no_stdout_writes.pystatically scans the source tree withast. Design notes:ast.Callnodes, so prints inside docstring examples (controls.py:486,patterns.py:1587, and others) are excluded for free — they're strings, not calls;print(..., file=...), which isn't a stdout write;Two narrow exemptions are encoded rather than blanket-skipping any file:
if ...debug...:blockthreadFuncinsideRunByHotKeyuiautomationhotkey utility the server never invokesTogether those cover the three remaining prints in
uia/controls.py(5421, 5442, 5912). I verified the exemptions aren't over-broad by injecting an ungatedprintintoGetWordBoundingBoxAtPointin that same file — still caught. Reintroducing the exact removed line atservice.py:571is also caught.Testing caveat
I could not run the suite locally.
pywin32has no macOS wheels, souv syncfails outright on this machine. Verification was:file=sys.stderrcorrectly ignored).CI runs on
windows-latestand will exercise the real suite. Worth a reviewer with a Windows box confirming the protocol noise is actually gone from a snapshot on an editable control — that's the observable symptom I couldn't reproduce from here.