Skip to content

Fix logger terminal state for colorama - #3263

Open
ICOM725 wants to merge 1 commit into
Comfy-Org:mainfrom
ICOM725:codex/fix-logger-terminal-state
Open

ICOM725 wants to merge 1 commit into
Comfy-Org:mainfrom
ICOM725:codex/fix-logger-terminal-state

Conversation

@ICOM725

@ICOM725 ICOM725 commented Sep 10, 2026

Copy link
Copy Markdown

Fixes #3259.

ComfyUIManagerLogger now delegates isatty() and closed to the original stdout or stderr. This lets colorama preserve ANSI colors on terminals while continuing to strip them from redirected output.

Thanks to @hwprinz for identifying the stream-state mismatch and providing the initial fix direction. Added tests using the real logger and colorama to cover stdout/stderr, terminal/redirected output, and closed streams.

Validation on Windows: python -X utf8 -m pytest tests -q (55 passed, 14 server E2E tests skipped), ruff check ., and git diff --check. The 10 new cases exercise both colorama platform branches and fail on the original code (6 failed, 4 passed). Test dependencies are listed in tests/requirements.txt.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: a7b85cef-f364-4321-aa3f-f5b3a30fdc8a

📥 Commits

Reviewing files that changed from the base of the PR and between f82970b and cc3909b.

📒 Files selected for processing (3)
  • prestartup_script.py
  • tests/requirements.txt
  • tests/test_logger_stream.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Changes

Logger stream fix

Layer / File(s) Summary
Delegate stream state
prestartup_script.py
ComfyUIManagerLogger.isatty() delegates to the selected original stream. The new closed property reports that stream’s closed state.
Validate Colorama stream behavior
tests/requirements.txt, tests/test_logger_stream.py
Adds pytest and colorama. Tests cover stream selection, ANSI preservation or stripping, logging, stream isolation, isatty(), and closed state tracking.

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to cc390

The logger now reflects its wrapped stream’s terminal and closed state, preserving ANSI colors in terminals while keeping redirected output plain. The intended behavior is covered without an identified remaining merge risk.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation satisfies issue #3259. It delegates isatty() and closed to the selected original stream, preserving ANSI colors for TTY output and allowing colorama to strip colors for redirected o…
Out of Scope Changes check ✅ Passed All changes support issue #3259. The production change fixes stream state reporting, and the test dependency and test additions validate the required colorama behavior. No unrelated code changes are p…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

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

@socket-security

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedpypi/​pytest@​9.1.187100100100100
Addedpypi/​colorama@​0.4.6100100100100100

View full report

This branch has not been deployed

No deployments
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.

ComfyUIManagerLogger breaks colorama: isatty() hardcoded to False and missing closed attribute strip ANSI colors from print() on Windows

1 participant