Fix 28 bugs found in full-codebase audit (MultiBar threading, lifecycle, widget math, streams, CLI)#315
Open
wolph wants to merge 8 commits into
Open
Fix 28 bugs found in full-codebase audit (MultiBar threading, lifecycle, widget math, streams, CLI)#315wolph wants to merge 8 commits into
wolph wants to merge 8 commits into
Conversation
Covers lifecycle (A), widget math (B), streams/terminal/env (C), MultiBar concurrency (D) and CLI/platform (E) findings. All tests currently fail against the buggy behavior; fixes follow per subsystem.
multi.py: per-instance locks/events (D1), start() clears instead of sets the closed event (D6), __exit__ stops on exception instead of hanging (D4), flush() resets the buffer position to avoid NUL output (D3), append_label works together with prepend_label (D7), snapshot iteration for cross-thread dict mutation (D2), join(timeout) keeps the thread reference while alive (D8). bar.py: days_elapsed uses the full elapsed time (A1), init() resets the finished/started flags and finish() is idempotent so the global capturing counter stays balanced (A2), f-string in the unknown variable error (A3), __del__ suppresses all exceptions (A4), SIGWINCH handling moved to a shared registry so overlapping bars restore the original handler in any finish order (A5). widgets.py/algorithms.py: ETA and fill computations respect min_value (B3, B9), JobStatusBar drops old markers instead of overflowing (B4), MultiProgressBar treats a zero total as no progress (B5), transfer speed shows the regular format before any data (B6), time-window samples evict on time alone (B7), EMA/DEMA seed from the first value (B8). Legacy tests updated for the corrected semantics.
terminal/base.py: HSL.interpolate no longer swaps saturation and lightness (C1). terminal/stream.py: TextIOOutputWrapper.flush delegates to the wrapped stream and LineOffsetStreamWrapper.write returns the original length (C5, C6). utils.py: WrappingIO._flush clears its buffer before writing so failures cannot duplicate output, and skips closed targets such as the atexit flush at interpreter shutdown (C3, C4); unwrapping restores sys.excepthook once both streams are unwrapped (C7); the unsupported-operation handler keeps the wrap counters as ints (C2). env.py: truthy FORCE_COLOR / PROGRESSBAR_ENABLE_COLORS flags enable full color support (C8).
__main__.py: pass the configured widgets to the bar (E2), track progress in bytes in line mode (E1), suppress BrokenPipeError for early-closing pipes (E3), keep known-size mode for empty files (E8). posix.py: getch() falls back to a plain read when stdin is not a tty (E6). windows.py: explicit argtypes on all kernel32 bindings, invalid console handles are detected and skipped, getch() honors the number of events read and survives non-ASCII keys (E4, E5). base.py: remove the Python 2 era __cmp__/__nonzero__ dead code (E7).
The monitor_progress goldens and the colors test encoded the duplicate final render caused by the double finish() from the iterator plus the context manager; finish() is idempotent now so the bar renders its final state once. The multibar example finishes its bars explicitly since the context manager now genuinely waits for completion. The transfer-speed golden for zero progress uses the regular format. Also: FORCE_COLOR handling returns directly instead of break (avoids an unrecordable coverage arc), SIGWINCH dispatch and reverse-order unwrapping gained coverage, and the repro_bugs.py scratch battery is replaced by the regression tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive bug fixes, stability improvements, and robust test coverage across the progressbar library. Key changes include seeding moving average algorithms with the first observation to avoid zero-bias, implementing a shared _ResizeRegistry for safe multi-bar terminal resizing, resolving several multi-threading and stream-wrapping issues in MultiBar, and correcting calculations for HSL interpolation, elapsed days, and widgets with custom min_value bounds. Additionally, platform-specific terminal handling is hardened for both POSIX and Windows environments. The review feedback highlights three key improvements: explicitly checking for KEY_EVENT in Windows getch() to avoid reading garbage union data, using hasattr to safely check for SIGWINCH availability across platforms, and capping the progress bar's rendered length to prevent visual wrapping when values exceed the maximum.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR addresses a large set of correctness issues found in a full codebase audit, with a strong emphasis on regression coverage. It improves ProgressBar/MultiBar lifecycle behavior, widget math, stream wrapping reliability, terminal/platform handling (POSIX + Windows), and the CLI’s progress accounting.
Changes:
- Fix MultiBar thread lifecycle/state isolation, render-loop safety under concurrent mutation, and label/flush behavior.
- Correct multiple ProgressBar/widget computations (e.g., min_value-relative math, sample eviction, ETA/speed formatting) and improve SIGWINCH handler management.
- Harden stream wrappers and terminal/platform code paths (flush behavior, excepthook restore, getch behavior, Windows ctypes argtypes/handle validity), and fix CLI widget wiring + byte-accurate progress counting.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_windows.py | Adds Windows-only regressions for ctypes argtypes and getch buffer handling. |
| tests/test_widgets.py | Adds regressions for ETA/bar fill respecting min_value and MultiProgressBar zero totals. |
| tests/test_utils.py | Adds regressions for StreamWrapper excepthook restoration and WrappingIO flush correctness. |
| tests/test_stream.py | Adds regressions for LineOffsetStreamWrapper write return value and flush propagation. |
| tests/test_speed.py | Updates expectations for zero-progress transfer speed formatting. |
| tests/test_samples.py | Adds regression for sample eviction when progress stalls. |
| tests/test_progressbar.py | Adds regressions for days_elapsed, restart-after-finish behavior, idempotent finish, del suppression, SIGWINCH restore. |
| tests/test_progressbar_command.py | Adds regressions ensuring CLI passes widgets, counts bytes in line mode, handles BrokenPipe, and keeps size known for empty files. |
| tests/test_os_specific.py | Adds POSIX-only regression for getch() on non-tty stdin. |
| tests/test_multibar.py | Adds extensive MultiBar regressions for threading/lifecycle/mutation/flush/labeling. |
| tests/test_monitor_progress.py | Updates golden expectations for idempotent finish behavior. |
| tests/test_job_status.py | Adds regression ensuring JobStatusBar output doesn’t exceed width. |
| tests/test_failure.py | Adds regression for unexpected update kwarg TypeError message formatting. |
| tests/test_data_transfer_bar.py | Adds regression for speed formatting before any data is transferred. |
| tests/test_color.py | Adds regressions for HSL interpolation component order and FORCE_COLOR truthy handling. |
| tests/test_algorithms.py | Updates EMA/DEMA expectations and adds regressions for proper seeding behavior. |
| progressbar/widgets.py | Fixes min_value-relative computations, sample eviction, speed formatting edge case, MultiProgressBar zero division, JobStatusBar overflow handling. |
| progressbar/utils.py | Fixes WrappingIO flush duplication/closed-target behavior; fixes StreamWrapper excepthook restore & counter typing. |
| progressbar/terminal/stream.py | Fixes flush propagation and write() return length for LineOffsetStreamWrapper. |
| progressbar/terminal/os_specific/windows.py | Adds ctypes argtypes, handle validity checks, safer mode setting, and robust getch reading/decoding. |
| progressbar/terminal/os_specific/posix.py | Handles getch() on non-tty stdin without termios raw mode. |
| progressbar/terminal/base.py | Fixes HSL.interpolate parameter ordering. |
| progressbar/multi.py | Fixes MultiBar instance state isolation, flush buffer handling, render-loop safety, thread lifecycle, and exception behavior in exit. |
| progressbar/env.py | Treats truthy FORCE_COLOR-style flags as enabling truecolor support. |
| progressbar/base.py | Removes legacy Python 2 comparison hooks from FalseMeta. |
| progressbar/bar.py | Makes finish() idempotent, fixes elapsed day calculation, improves del robustness, and adds shared SIGWINCH registry. |
| progressbar/algorithms.py | Seeds EMA/DEMA from first observation rather than biasing toward 0. |
| progressbar/main.py | Fixes CLI widget wiring, byte-accurate progress tracking in line mode, BrokenPipe handling, and known-size handling for empty files. |
| examples.py | Updates MultiBar example to explicitly finish contained bars before context exit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5858b43af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review feedback: - windows.getch() only decodes KEY_EVENT records with bKeyDown set; reading other union members returned garbage for mouse/focus events (Gemini). _valid_handle() normalizes ctypes instances before comparing against INVALID_HANDLE_VALUE (Codex). - _ResizeRegistry checks hasattr(signal, 'SIGWINCH') explicitly instead of relying on suppressed AttributeError (Gemini). - create_marker clamps the fill length to the available width for value > max_value with max_error=False (Gemini). - get_sorted_bars() snapshots the dict values explicitly before sorting (Codex) and MultiBar.flush() keeps the fd write inside the print lock so concurrent output cannot interleave (Codex). - unwrap_stdout/unwrap_stderr reset the wrapper's own stream references so needs_clear()/update_capturing() don't act on a stale WrappingIO (Codex). - CLI line mode opens input and output with newline='' so CRLF files are counted at their true byte size (Codex). CI: - CodeQL: no side effects in assert, no explicit __del__ call (the finalizer test now uses gc + sys.unraisablehook), no ineffectual subscript statements in tests. - The py315 tox job is marked experimental/continue-on-error: Python 3.15 is pre-release and no released typing_extensions survives 'from typing_extensions import *' on it (no_type_check_decorator is still in __all__ after its removal from typing), which breaks the python_utils import chain. Verified against 3.15.0b2 with typing_extensions 4.14.1 and 4.15.0.
Job-level continue-on-error keeps the workflow green but still reports the job check as failed; step-level makes the experimental pre-release job report success while the failing step stays visible in the logs.
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.
Summary
A full audit of the codebase found 28 fixable bugs; every fix in this PR is covered by a regression test that failed before the fix (test-first, commit
190ff5f).stop()permanently killed every later MultiBar;start()set the closed event instead of clearing it; an exception insidewith MultiBar():deadlocked forever;flush()emitted NUL bytes (truncate(0)withoutseek(0));append_labelwas dead code whenprepend_labelwas on; the render thread iterated the live dict while other threads mutated it;join(timeout)dropped the thread reference while alive.days_elapsedwas computed fromtimedelta.seconds(always < 1 day); reused bars never reset_finished, and every extrafinish()corrupted the global stream-capturing counter; SIGWINCH handling now uses a shared registry so overlapping bars restore the original handler in any finish order;__del__no longer leaks non-AttributeError exceptions; the unknown-variableTypeErrorprinted a literal{key!r}.min_value;MultiProgressBarcrashed withZeroDivisionErroron(n, 0)sub-tasks;JobStatusBaroverflowed its width; transfer speed rendered0.0 s/Bbefore any data; time-window samples stopped evicting when progress stalled; EMA/DEMA now seed from the first observation instead of biasing toward zero.HSL.interpolateswapped saturation and lightness (every HSL gradient blend was wrong);FORCE_COLOR=1(the standard CI convention) yielded zero color support;LineOffsetStreamWrapperreturned short write counts and never flushed;sys.excepthookwas never restored after unwrapping; a failed wrapped write duplicated output on the next flush and the atexit flush could hit a closed target.# widgets=widgets,); line mode counted characters against a byte-sized total; early-closing pipes dumped aBrokenPipeErrortraceback; zero-byte files flipped to unknown-length mode; POSIXgetch()crashed on non-tty stdin; the Windows kernel32 bindings had noargtypes(64-bit handle truncation), ignored invalid console handles, andgetch()decoded an unread buffer entry.Behavioral notes:
finish()is now idempotent, so the duplicated final render that severaltest_monitor_progressgoldens encoded is gone (goldens re-derived);MultiBar.__exit__/join()now genuinely wait for bars to finish (the multibar example finishes its bars explicitly) and__exit__stops the render thread instead of hanging when an exception propagates.Test plan
uv run pytest: 437 passed, 2 skipped, 100% statement+branch coverage (enforced gate)ruff checkandruff format --checkcleanpyright0 errorstest_kernel32_argtypes/test_getch_reads_first_event(Windows-only, skipped on POSIX)