Quality audit PR 1/6: correctness batch + compatibility guardrails#319
Merged
Conversation
API-surface snapshot (public names + parameter names/kinds across 14 modules, stable across Python versions), subclass-compat suite covering old-style explicit-parent-call and cooperative-super() third-party widgets and bars, and exact render goldens. The super()-style width kwargs test is a strict xfail documenting the known chain break that the cooperative-super() migration will fix.
Passing fixed_colors/gradient_colors to a single widget instance called dict.update() on the shared class-level mapping, rewriting colors for every other instance and subclass. Merge overrides into a fresh per-instance dict instead, leaving the class default untouched. The two mappings are no longer ClassVar since instances may now legitimately hold their own copy.
The class-level `mapping = dict()` default was aliased by every default-constructed FormatCustomText, so update_mapping() on one instance mutated all others. Build a fresh dict from the argument instead, and fix the update_mapping() value annotation (was Dict, values are arbitrary).
A single JobStatusBar reused across two ProgressBars accumulated its marker history on the widget instance, interleaving the two bars' output. Store the marker list in progress.extra under a per-widget key (mirroring SamplesMixin), so the widget stays stateless and reusable. The job_markers attribute is retained for backward compatibility but no longer holds render state.
create_wrapper, create_marker and MultiRangeBar's render path used bare `assert` for user-facing validation, which is stripped under `python -O`, turning invalid input into silent corruption. Raise ValueError with the same messages instead. Existing AssertionError expectation updated.
The bytes CSI pattern was rebuilt on every no_color() call and the str branch relied on re's internal cache; both run per widget per redraw. Compile both variants once at module import and reuse via Pattern.sub().
int(c / 255) was only 1 at exactly 255, collapsing every mid-intensity colour (e.g. maroon 128,0,0) to black. Threshold each channel at 128 so it sets its own bit.
Two defects in Color.ansi: - `if self.xterm:` was falsy for index 0 (Black), so Black fell through to the RGB fallback and emitted the wrong code. - A registered 256-colour xterm index was emitted even on 16-colour (XTERM) terminals, which cannot address it. Use `self.xterm is not None` and only take the xterm index when COLOR_SUPPORT is XTERM_256 or better; 16-colour terminals now derive the code from to_ansi_16.
r'|/-\\' keeps both backslashes, so the literal was 5 chars and the four-frame cycle depended on a hardcoded `% 4`. Extract a module-level _SPINNER_FRAMES (plain 4-char literal) and index by its length.
The fill-branch check used a bare `assert`, which `python -O` strips, so an invalid multi-char fill would silently corrupt the rendered bar. Raise a ValueError with the offending value instead.
Document that the attribute is kept only for backwards compatibility; per-run marker state now lives in progress.extra (get_job_markers) and this attribute is no longer read or written during rendering.
The width-threshold math ran inside contextlib.suppress(Exception), so any unexpected failure silently disabled redraws. Explicit guards for the known-incomplete states (no value drawn yet, no usable term_width, no nonzero max_value) preserve the exact prior False results while letting genuine bugs propagate. Also drops the per-call context-manager allocation from the hot path.
is_ansi_terminal wrapped fd.isatty() probing in contextlib.suppress(Exception) and is_terminal in a bare except Exception, so genuine bugs (unexpected None, wrong types, programming errors) were silently downgraded to "not a terminal". Narrow both to the failures a stream can legitimately raise while probing: OSError (real I/O), ValueError (closed/detached file objects) and AttributeError (objects without isatty). Anything else now propagates. Drop the stale # pragma: no cover on the is_terminal isatty fallback now that tests exercise it. Update the existing test_utils RuntimeError case to OSError (a tolerated error); RuntimeError propagation is covered in the new tests/test_env_detection.py.
update() guarded value assignment with `value is not None and value is not base.UnknownLength and isinstance(value, (int, float))`. The first two clauses are subsumed by the isinstance check: None fails it, and UnknownLength is a class (type FalseMeta), not an int/float instance. Verified in a REPL: isinstance(base.UnknownLength, (int, float)) is False. Reduce to the isinstance check alone; behavior is unchanged. Add tests/test_update_guard.py asserting update(None) and update(UnknownLength) still leave value/previous_value untouched.
On a clean context-manager exit MultiBar.__exit__ called join() with timeout=None, waiting until every bar finished. A single never-finishing bar hung the program forever. Add a keyword-only join_timeout: timedelta | float | None = None constructor parameter (converted with the same python_utils.delta_to_seconds_or_none used for remove_finished). The clean __exit__ path now passes it to join(timeout=...); once it elapses the still-unfinished bars are abandoned and the daemon render thread is left running so the program can exit. Default None preserves the historical wait-forever behavior. Regenerate tests/api_surface_snapshot.json for the additive keyword-only parameter.
test_multibar_print drove both the inter-iteration sleep and the per-thread print decision off the global random.random(). Under the 100% branch-coverage gate a run where the print guard never fired could flakily fail, and workers outliving the multibar occasionally raised "I/O operation on closed file" from a torn-down fd. Give each worker its own seeded random.Random(seed) so its draws are reproducible regardless of thread scheduling, iterate probabilities (0.0, 0.5, 1.0) so the never/always threads deterministically hit both sides of the print guard every run (verified the 0.5 seeds also hit both), and join the workers before leaving the context so none writes to a closed stream. Shorter, deterministic sleeps keep the run fast.
The join_timeout test (previous commit) intentionally leaves an unfinished bar, so join_timeout abandons the MultiBar render thread. That daemon thread keeps rendering for the rest of the process and raced with later tests -- intermittently surfacing freezegun's threaded IndexError and unrelated TypeError failures. Hold the MultiBar reference and call stop() once the prompt exit is verified so the render loop terminates and cannot pollute other tests. Behavior under test is unchanged.
test_needs_update starts bars (registering them as global capture listeners via streams.start_capturing) but never finish()es them, and one deliberately sets term_width='wide' to prove the width-threshold math now propagates. That poisoned bar lingered in streams.listeners, so when a *later* test wrote a newline to the captured stream -- WrappingIO.write calls update() on every listener -- the abandoned bar hit the int/str division from commit 84b9413 and raised, intermittently failing test_examples and test_no_newlines (the failure surfaced or hid depending on freezegun-driven redraw timing). Add an autouse fixture that deregisters whatever each test started, so a bar driven into an invalid state can never poison another test.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR is the first batch from a larger quality audit, focusing on correctness fixes and adding compatibility guardrails to prevent accidental public API breakage across subsequent refactors.
Changes:
- Add compatibility guardrails: a public API surface snapshot test + a subclass-compat characterization suite (including render goldens and an intentional strict xfail).
- Replace user-facing
assertvalidation with explicitValueErrorin widget marker/fill validation, and tighten terminal-detection exception handling to avoid silently swallowing unexpected errors. - Fix several correctness/performance issues (per-instance color/mapping state, per-bar
JobStatusBarmarker state,_needs_updateexplicit guards, deterministicMultiBartests, precompiled ANSI regexes, spinner frame constant).
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_widgets.py | Updates and adds tests asserting ValueError-based validation for wrapper/marker/fill width rules. |
| tests/test_utils.py | Adjusts terminal detection test to match narrowed tolerated exception set. |
| tests/test_update_guard.py | Adds regression tests ensuring update(None) and update(UnknownLength) preserve historical value semantics. |
| tests/test_subclass_compat.py | Introduces subclass compatibility characterization (old-style explicit parent init vs cooperative super()) plus render goldens. |
| tests/test_needs_update.py | Adds tests covering _needs_update incomplete-state guards and unexpected error propagation. |
| tests/test_multibar.py | Makes multibar print tests deterministic and adds coverage for join_timeout behavior. |
| tests/test_job_status.py | Adds regression test ensuring job markers don’t interleave when a widget is shared across bars. |
| tests/test_fastpath.py | Adds test asserting ANSI regexes are precompiled (and preserves stripping behavior). |
| tests/test_fast_default.py | Adds spinner frame cycle regression test and validates the intended 4-frame behavior. |
| tests/test_env_detection.py | Adds tests asserting only specific isatty() errors are tolerated; unexpected errors propagate. |
| tests/test_custom_widgets.py | Adds regression test ensuring FormatCustomText.mapping is per-instance (no shared class dict mutation). |
| tests/test_color.py | Adds tests for RGB→ANSI16 mapping, color support gating, and per-instance color override copy-on-write. |
| tests/test_api_surface.py | Adds public API surface snapshot test and regeneration mechanism via env var. |
| tests/api_surface_snapshot.json | Adds the baseline snapshot used by the API surface guardrail. |
| progressbar/widgets.py | Converts asserts to ValueError, fixes shared mutable class state via copy-on-write overrides, and moves JobStatusBar marker state to per-bar storage. |
| progressbar/utils.py | Precompiles ANSI-stripping regexes at import time and reuses them in no_color(). |
| progressbar/terminal/base.py | Fixes RGB.to_ansi_16 thresholding and gates use of 256-color xterm indexes by terminal support (including honoring index 0). |
| progressbar/multi.py | Adds join_timeout and uses it on clean context-manager exit to bound render-thread joining. |
| progressbar/fast.py | Replaces raw spinner literal with a 4-character constant and uses its length for indexing. |
| progressbar/env.py | Narrows exception handling for isatty() probing to legitimate stream errors only. |
| progressbar/bar.py | Replaces _needs_update blanket suppression with explicit incomplete-state guards and simplifies update() value-assignment gating. |
💡 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: ea7267d83c
ℹ️ 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".
![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 several quality-audit refactors and bug fixes across the progressbar library. Key changes include replacing blanket exception suppressions with explicit guards or narrowed exception types, optimizing ANSI color stripping via precompiled regexes, fixing color mapping bugs in to_ansi_16 and xterm-256 color support, and introducing a join_timeout parameter to prevent MultiBar from hanging on exit. Additionally, assertions used for user-facing validation are replaced with explicit ValueError exceptions, and state leakage across instances is resolved by copying mutable dictionaries on write. The review feedback correctly points out that the new dictionary initialization in FormatCustomText discards class-level default mappings when no mapping is explicitly provided, and suggests a fix to copy the existing mapping instead.
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.
With COLOR_SUPPORT at NONE, Color.ansi is now None (correct since the support-gating fix) but SGRColor formatted it into the escape code, emitting a malformed '\x1b[38;Nonem' whose tail leaked into visible output as 'onem' in pty-based tests. Without a usable representation the text now passes through completely unstyled.
Describe typing aliases, enums (by member list) and stdlib re-exports with version-stable descriptors: typing.Union aliases changed type in 3.14, enum constructor signatures are metaclass artifacts, and TracebackType's signature availability varies. Snapshot regenerated on 3.14 and verified identical on 3.10. Use a single import style for progressbar in the new test modules so CodeQL's import-and-import-from check stays quiet (same alias pattern as test_fast_default.py).
Color.ansi gated the registered xterm index on detected 256-color support, but callers can force colors (enable_colors=True, FORCE_COLOR) on terminals whose detection reports NONE -- rendering an SGR at all means the caller decided colors are wanted. Prefer the registered xterm index everywhere except true 16-color terminals (which still get the to_ansi_16 translation instead of a leaked 256-color index); only fully unregistered colors with no derivable code return None and pass text through unstyled.
dict(mapping or {}) discarded class-level mapping defaults declared by
subclasses. Fall back to the class attribute when no mapping is passed,
still copying so instance mutation never touches the shared default.
join(timeout) returning with the thread alive left the daemon looping and writing until interpreter exit; signal it to stop instead.
The join_timeout test started its bar inside the multibar context, so a render tick could catch the bar between _started=True and widget population, crash the render thread on the empty-widgets assert, and let join() succeed on a dead thread -- skipping the timeout path the test exists to exercise (seen as a reproducible 99.93% coverage failure on the py312 CI job). Start the bar fully up front; verified stable across 10 consecutive runs and at 100% coverage on 3.12/3.14.
This was referenced Jul 6, 2026
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.
First of six sequential PRs from the 2026-07-02 full-codebase quality audit (93 findings across core, widgets, terminal, and tooling). This PR lands the correctness batch: confirmed bugs and silent-error fixes, plus the backwards-compatibility guardrails that gate every subsequent audit PR.
Compatibility guardrails (new, first commit)
tests/test_api_surface.py+api_surface_snapshot.json): public names + parameter names/kinds across 14 modules; any removal/rename/narrowing fails the suite. Annotation/default changes are deliberately excluded so the snapshot is stable across py3.10-3.15.tests/test_subclass_compat.py): third-party widgets/bars in both historical styles (explicit unbound parent__init__calls, cooperativesuper()) must construct and render identically; includes exact render goldens. One strict xfail documents the known cooperative-chain break that PR 2 will fix.Bug fixes (each TDD, one commit per finding)
fixed_colors/gradient_colorsoverrides mutated the class dict (one instance recolored all others) — now copy-on-write per instance. Same forFormatCustomText.mapping.JobStatusBarmarker state moved toprogress.extraso a widget shared between bars no longer interleaves markers.RGB.to_ansi_16was degenerate:int(c/255)mapped everything but pure 255-channels to black (maroon → black). Channels now threshold at half intensity.Color.ansi: honours xterm index 0 (Black) viais not None, and no longer leaks 256-color indexes to terminals that only report 16-color support. Note: direct.ansireads under WINDOWS/NONE support now returnNoneinstead of an unrenderable5;Nfragment (the publicfg/bg/underlinepaths never hit this).python -O: user-facingasserts increate_marker/create_wrapper/MultiRangeBar(marker + fill) are now explicitValueErrors._needs_updatedropped itssuppress(Exception)for explicit incomplete-state guards (identical redraw cadence, real bugs propagate);env.pyterminal detection narrowed from bareExceptionto(OSError, ValueError, AttributeError).r'|/-\\'literal was 5 chars); frames and cadence unchanged.MultiBar: new keyword-onlyjoin_timeout=Nonebounds the clean context-exit wait (default preserves wait-forever);update()dropped two guard clauses subsumed by its isinstance check.test_multibar_printnow uses seeded per-worker RNGs and hits both print-guard branches every run (was flaky under the 100% branch-coverage gate); ANSI regexes inutils.no_colorprecompiled.Verification
556 passed / 100.00% branch coverage (two consecutive runs), ruff clean, pyright 0 errors. No public name removed, no signature narrowed (snapshot-enforced); the only snapshot delta is the additive keyword-only
join_timeout. Render goldens byte-identical.Full audit findings and the 6-PR remediation plan live in the session notes; PR 2 (cooperative
super()migration) follows once this merges.