From 72bef5811757c648cd372f9969b541e083f19014 Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Thu, 20 Aug 2026 05:18:54 +0000 Subject: [PATCH 1/2] fix(cli): resolve doctor's glyph set per console encoding (#401) conductor doctor's table output hardcodes the U+2713/U+2717/U+25CB glyphs for its Installed/Credentials/Connection/Models columns. None of those are encodable in cp1252, so a run on a legacy Windows console raises UnicodeEncodeError mid-table, after the Environment section has already printed: the report is truncated and the process exits non-zero for a run that actually succeeded. The --json path already guards this with ensure_ascii=True (#381); the table path did not. Fix: resolve a Unicode-or-ASCII glyph set once per run_doctor() call, from the output console's actual stream encoding, and thread it through every cell helper (_tier_cell, _credentials_cell, _connection_cell, _models_cell, _format_tokens, _default_effort_cell, _rate_cell, _render_registries) instead of reaching for a module-level _CHECK/_CROSS/_DASH/_OPTIONAL_MARK constant. A console with no encoding attribute (e.g. an in-memory buffer) is treated as capable, matching the existing "nothing to protect against" default. Fixes #401 --- CHANGELOG.md | 8 ++ src/conductor/cli/doctor.py | 164 ++++++++++++++++++++++------------ tests/test_cli/test_doctor.py | 108 ++++++++++++++++++++++ 3 files changed, 223 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2cee27c..d7e194c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 as fatal. - `runtime.default_reasoning_effort` was silently dropped at run time for every provider and is now forwarded through `ProviderRegistry`. +- **`conductor doctor`'s table output no longer dies part-written on a + `cp1252` console** (#401). The Installed/Credentials/Connection/Models + columns hardcoded `✓`/`✗`/`○`, none of which cp1252 can encode, so a run + on a legacy Windows console raised `UnicodeEncodeError` mid-table, after + the Environment section had already printed. `conductor doctor` now + resolves each glyph once per invocation against the output console's + stream encoding, falling back to `OK`/`X`/`o` when the Unicode glyphs + cannot be encoded; the `--json` path was already safe and is unchanged. - **MCP tool discovery and structured tool results no longer break with MCP 2.0** (#419). MCP 2.0 renamed the Python field on `mcp.types.Tool` from `inputSchema` to `input_schema` and on `mcp.types.CallToolResult` from diff --git a/src/conductor/cli/doctor.py b/src/conductor/cli/doctor.py index ed5a02da..25b0593c 100644 --- a/src/conductor/cli/doctor.py +++ b/src/conductor/cli/doctor.py @@ -12,7 +12,7 @@ import contextlib import logging from collections.abc import Iterator -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, NamedTuple from rich.table import Table from rich.text import Text @@ -35,20 +35,66 @@ from conductor.providers.diagnostics import Section -_CHECK = Text.from_markup("[green]✓[/green]") -_CROSS = Text.from_markup("[red]✗[/red]") -_DASH = Text.from_markup("[dim]—[/dim]") -_OPTIONAL_MARK = "○" -"""Neutral glyph for an absent *optional* credential — deliberately not the -red ``✗`` used for a genuinely missing required credential (issue #319). +class _Glyphs(NamedTuple): + """The status glyphs a table render uses, resolved for one console.""" -.. note:: - ``✓``, ``✗`` and ``○`` are not encodable in cp1252, so the default *table* - output of ``conductor doctor`` still fails on such a console — see #401. - This module's ``--json`` path is safe (``ensure_ascii=True``); the table path - is deliberately out of scope here because every glyph consumer would need the - console threaded through it. The em-dash is encodable in cp1252. -""" + check: Text + cross: Text + dash: Text + optional: str + """Neutral glyph for an absent *optional* credential — deliberately not + ``cross``, which is reserved for a genuinely missing required credential + (issue #319).""" + + +_UNICODE_GLYPHS = _Glyphs( + check=Text.from_markup("[green]✓[/green]"), + cross=Text.from_markup("[red]✗[/red]"), + dash=Text.from_markup("[dim]—[/dim]"), + optional="○", +) +_ASCII_GLYPHS = _Glyphs( + check=Text.from_markup("[green]OK[/green]"), + cross=Text.from_markup("[red]X[/red]"), + dash=Text.from_markup("[dim]-[/dim]"), + optional="o", +) + + +def _encodable(text: str, encoding: str | None) -> bool: + """Whether *text* survives a round trip through *encoding*. + + ``encoding`` of ``None`` (the stream has no ``.encoding`` attribute, e.g. + an in-memory buffer) is treated as capable: there is nothing to protect + against, so it is better to render full-width Unicode than to needlessly + downgrade every invocation to ASCII. + """ + if not encoding: + return True + try: + text.encode(encoding) + except (UnicodeEncodeError, LookupError): + return False + return True + + +def _resolve_glyphs(console: Console) -> _Glyphs: + """Pick Unicode or ASCII-safe glyphs for *console*'s stream encoding. + + Rich hands a rendered line straight to the underlying file's ``write()``; + it does not check whether the target encoding can represent it. A legacy + Windows console (``cp1252``) cannot encode ``✓``/``✗``/``○``, so the table + used to die mid-write, part-printed (issue #401). Resolved once per + ``run_doctor`` call and passed down, rather than re-checked per cell: the + stream's encoding cannot change mid-render. + """ + encoding = getattr(getattr(console, "file", None), "encoding", None) + return _Glyphs( + check=_UNICODE_GLYPHS.check if _encodable("✓", encoding) else _ASCII_GLYPHS.check, + cross=_UNICODE_GLYPHS.cross if _encodable("✗", encoding) else _ASCII_GLYPHS.cross, + dash=_UNICODE_GLYPHS.dash if _encodable("—", encoding) else _ASCII_GLYPHS.dash, + optional=_UNICODE_GLYPHS.optional if _encodable("○", encoding) else _ASCII_GLYPHS.optional, + ) def run_doctor( @@ -109,14 +155,15 @@ def run_doctor( console.print_json(data=report.to_dict(), ensure_ascii=True) return _compute_exit_code(report.providers, check=check, provider=provider) + glyphs = _resolve_glyphs(console) if report.env is not None: _render_env(report.env, console) if report.providers is not None: - _render_providers(report.providers, console, check=check, models=models) + _render_providers(report.providers, console, glyphs, check=check, models=models) if models: - _render_models(report.providers, console) + _render_models(report.providers, console, glyphs) if report.registries is not None: - _render_registries(report.registries, console) + _render_registries(report.registries, console, glyphs) return _compute_exit_code(report.providers, check=check, provider=provider) @@ -204,6 +251,7 @@ def _render_env(env: EnvDiagnostic, console: Console) -> None: def _render_providers( providers: list[ProviderDiagnostic], console: MarkupFreeConsole, + glyphs: _Glyphs, *, check: bool, models: bool, @@ -223,30 +271,30 @@ def _render_providers( for diag in providers: row = [ diag.name, - _CHECK if diag.installed else _CROSS, - _tier_cell(diag.tier), - _credentials_cell(diag), + glyphs.check if diag.installed else glyphs.cross, + _tier_cell(diag.tier, glyphs), + _credentials_cell(diag, glyphs), ] if check: - row.append(_connection_cell(diag)) + row.append(_connection_cell(diag, glyphs)) if models: - row.append(_models_cell(diag)) - row.append(diag.note or _DASH) + row.append(_models_cell(diag, glyphs)) + row.append(diag.note or glyphs.dash) table.add_row(*row) console.print(table) -def _tier_cell(tier: str | None) -> Text: +def _tier_cell(tier: str | None, glyphs: _Glyphs) -> Text: """Format the tier cell.""" if tier is None: - return _DASH + return glyphs.dash if tier == "experimental": return Text.from_markup("[yellow]experimental[/yellow]") return Text(tier) -def _credentials_cell(diag: ProviderDiagnostic) -> Text: +def _credentials_cell(diag: ProviderDiagnostic, glyphs: _Glyphs) -> Text: """Format credential env-var presence (presence only, never values). A present credential is a green ``✓``. An absent credential renders as a @@ -257,32 +305,32 @@ def _credentials_cell(diag: ProviderDiagnostic) -> Text: accompanying auth-path note is surfaced in the Notes column (issue #319). """ if not diag.credential_env_vars: - return _DASH + return glyphs.dash lines: list[Text] = [] for cred in diag.credential_env_vars: if cred.present: - lines.append(styled("{} {}", _CHECK, cred.name)) + lines.append(styled("{} {}", glyphs.check, cred.name)) elif diag.credentials_optional: - lines.append(styled("[dim]{} {}[/dim]", _OPTIONAL_MARK, cred.name)) + lines.append(styled("[dim]{} {}[/dim]", glyphs.optional, cred.name)) else: - lines.append(styled("[dim]{} {}[/dim]", _CROSS, cred.name)) + lines.append(styled("[dim]{} {}[/dim]", glyphs.cross, cred.name)) return join("\n", lines) -def _connection_cell(diag: ProviderDiagnostic) -> Text: +def _connection_cell(diag: ProviderDiagnostic, glyphs: _Glyphs) -> Text: """Format the connection-check result cell.""" if not diag.checked or diag.connection_ok is None: - return _DASH + return glyphs.dash if diag.connection_ok and diag.connection_note: return styled("[yellow]⚠[/yellow] {}", diag.connection_note) if diag.connection_ok: - return styled("{} connected", _CHECK) + return styled("{} connected", glyphs.check) if diag.connection_error: - return styled("{} [dim]{}[/dim]", _CROSS, diag.connection_error) - return styled("{} [dim]connection failed[/dim]", _CROSS) + return styled("{} [dim]{}[/dim]", glyphs.cross, diag.connection_error) + return styled("{} [dim]connection failed[/dim]", glyphs.cross) -def _models_cell(diag: ProviderDiagnostic) -> Text: +def _models_cell(diag: ProviderDiagnostic, glyphs: _Glyphs) -> Text: """Format the models cell in the Providers summary table. Shows a count/status only — per-model reasoning-effort and @@ -291,19 +339,19 @@ def _models_cell(diag: ProviderDiagnostic) -> Text: models is ``None`` (not enumerated), ``(none)`` for an empty list. """ if diag.models_error: - return styled("{} [dim]{}[/dim]", _CROSS, diag.models_error) + return styled("{} [dim]{}[/dim]", glyphs.cross, diag.models_error) if diag.models is None: return Text.from_markup("[dim]n/a[/dim]") count = len(diag.models) if not count: return Text.from_markup("[dim](none)[/dim]") - return styled("{} {} model{}", _CHECK, count, "s" if count != 1 else "") + return styled("{} {} model{}", glyphs.check, count, "s" if count != 1 else "") -def _format_tokens(value: int | None) -> Text: +def _format_tokens(value: int | None, glyphs: _Glyphs) -> Text: """Format a token-limit value with grouped digits, or ``—`` when unknown.""" if value is None: - return _DASH + return glyphs.dash return Text(f"{value:,}") @@ -320,10 +368,10 @@ def _efforts_cell(model: ModelDiagnostic) -> Text: return Text(", ".join(model.supported_reasoning_efforts)) -def _default_effort_cell(model: ModelDiagnostic) -> Text: +def _default_effort_cell(model: ModelDiagnostic, glyphs: _Glyphs) -> Text: """Format the default-reasoning-effort cell.""" if model.default_reasoning_effort is None: - return _DASH + return glyphs.dash return Text(model.default_reasoning_effort) @@ -337,12 +385,14 @@ def _default_effort_cell(model: ModelDiagnostic) -> Text: (issue #386), plus the synthetic ``"error"`` key used for ``None`` (pricing resolution itself failed). Built as module-level constants to avoid re-parsing the same markup literal on every table row (matching the -``_CHECK``/``_CROSS``/``_DASH`` constants above) — each markup argument is -still a literal template, not an interpolated value, keeping this inside the -repo's console rules (see AGENTS.md "Console Output").""" +``_UNICODE_GLYPHS``/``_ASCII_GLYPHS`` constants above) — each markup argument +is still a literal template, not an interpolated value, keeping this inside +the repo's console rules (see AGENTS.md "Console Output"). None of these +values are drawn from the encoding-sensitive glyph set, so they need no +ASCII fallback.""" -def _rate_cell(value: float | None) -> Text: +def _rate_cell(value: float | None, glyphs: _Glyphs) -> Text: """Format a per-Mtok rate, or ``—`` when unknown. Deliberately never renders ``0.00`` for ``None`` — a zero would read as @@ -350,7 +400,7 @@ def _rate_cell(value: float | None) -> Text: #386 is about. """ if value is None: - return _DASH + return glyphs.dash return Text(f"{value:,.2f}") @@ -368,7 +418,7 @@ def _pricing_source_cell(model: ModelDiagnostic) -> Text: return _PRICING_SOURCE_CELLS.get(model.pricing_source, Text(model.pricing_source)) -def _render_models(providers: list[ProviderDiagnostic], console: Console) -> None: +def _render_models(providers: list[ProviderDiagnostic], console: Console, glyphs: _Glyphs) -> None: """Render a per-provider Models detail table (``--models`` only). One table per provider that returned at least one model, with columns @@ -395,23 +445,23 @@ def _render_models(providers: list[ProviderDiagnostic], console: Console) -> Non table.add_row( model.id, _efforts_cell(model), - _default_effort_cell(model), - _format_tokens(model.max_prompt_tokens), - _format_tokens(model.max_output_tokens), - _format_tokens(model.max_context_window_tokens), - _rate_cell(model.input_per_mtok), - _rate_cell(model.output_per_mtok), + _default_effort_cell(model, glyphs), + _format_tokens(model.max_prompt_tokens, glyphs), + _format_tokens(model.max_output_tokens, glyphs), + _format_tokens(model.max_context_window_tokens, glyphs), + _rate_cell(model.input_per_mtok, glyphs), + _rate_cell(model.output_per_mtok, glyphs), _pricing_source_cell(model), ) console.print(table) -def _render_registries(registries: RegistryDiagnostic, console: Console) -> None: +def _render_registries(registries: RegistryDiagnostic, console: Console, glyphs: _Glyphs) -> None: """Render the registries section.""" if registries.error is not None: console.print( - styled("{} [dim]failed to load registries: {}[/dim]", _CROSS, registries.error) + styled("{} [dim]failed to load registries: {}[/dim]", glyphs.cross, registries.error) ) return if not registries.registries: @@ -429,7 +479,7 @@ def _render_registries(registries: RegistryDiagnostic, console: Console) -> None reg.name, reg.type, reg.source, - _CHECK if reg.is_default else _DASH, + glyphs.check if reg.is_default else glyphs.dash, ) console.print(table) diff --git a/tests/test_cli/test_doctor.py b/tests/test_cli/test_doctor.py index 51a667f2..59edfcb4 100644 --- a/tests/test_cli/test_doctor.py +++ b/tests/test_cli/test_doctor.py @@ -8,6 +8,7 @@ from __future__ import annotations import importlib +import io import json from unittest.mock import AsyncMock @@ -859,3 +860,110 @@ def test_registries_load_error_renders(self, monkeypatch: pytest.MonkeyPatch) -> assert "failed to load registries" in result.output assert "line 3" in result.output assert "No registries configured" not in result.output + + +# --------------------------------------------------------------------------- +# Encoding fallback (issue #401) +# --------------------------------------------------------------------------- + + +class TestDoctorEncodingFallback: + """The table output degrades to ASCII glyphs on a stream that cannot + encode the Unicode ones, instead of dying part-written.""" + + def _bind_console( + self, monkeypatch: pytest.MonkeyPatch, encoding: str + ) -> tuple[io.BytesIO, io.TextIOWrapper]: + """Point the CLI's output console at a fresh buffer with *encoding*. + + Returns the raw byte buffer rather than relying on ``result.output``: + the CLI's real console is bound directly to it, decoded with the same + *encoding* it was written with, so a character neither side can + represent surfaces as a decode error rather than being silently + swallowed by pytest's own UTF-8 capture. + """ + buffer = io.BytesIO() + stream = io.TextIOWrapper(buffer, encoding=encoding, newline="") + monkeypatch.setattr(_app_module, "output_console", make_console(file=stream, width=200)) + monkeypatch.setattr( + _app_module, "console", make_console(file=stream, stderr=True, width=200) + ) + return buffer, stream + + def test_cp1252_console_renders_ascii_glyphs_without_crashing( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + buffer, stream = self._bind_console(monkeypatch, "cp1252") + report = DoctorReport( + providers=[_prov("copilot", installed=True)], + registries=RegistryDiagnostic( + default="local", + registries=[ + RegistryInfo(name="local", type="path", source="~/.conductor", is_default=True) + ], + ), + ) + _patch_gather(monkeypatch, report) + result = runner.invoke(app, ["doctor"]) + stream.flush() + assert result.exception is None + assert result.exit_code == 0 + output = buffer.getvalue().decode("cp1252") # raises if a raw glyph leaked through + assert "OK" in output + assert "✓" not in output + + def test_utf8_console_keeps_unicode_glyphs(self, monkeypatch: pytest.MonkeyPatch) -> None: + buffer, stream = self._bind_console(monkeypatch, "utf-8") + report = DoctorReport(providers=[_prov("copilot", installed=True)]) + _patch_gather(monkeypatch, report) + result = runner.invoke(app, ["doctor"]) + stream.flush() + assert result.exception is None + assert result.exit_code == 0 + output = buffer.getvalue().decode("utf-8") + assert "✓" in output + assert "OK" not in output + + def test_stream_with_no_encoding_keeps_unicode_glyphs( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A console file with no encoding (``.encoding is None``, e.g. an + in-memory ``StringIO``) is treated as capable of anything, not + downgraded to ASCII (#401).""" + stream = io.StringIO() + assert stream.encoding is None + monkeypatch.setattr(_app_module, "output_console", make_console(file=stream, width=200)) + monkeypatch.setattr( + _app_module, "console", make_console(file=stream, stderr=True, width=200) + ) + report = DoctorReport(providers=[_prov("copilot", installed=True)]) + _patch_gather(monkeypatch, report) + result = runner.invoke(app, ["doctor"]) + assert result.exception is None + assert result.exit_code == 0 + assert "✓" in stream.getvalue() + assert "OK" not in stream.getvalue() + + +class TestDoctorTierAndModelsErrorCells: + """Two more per-cell branches driven by the same glyph set as the + encoding-fallback tests above, exercised on the default UTF-8 path.""" + + def test_missing_tier_renders_dash(self, monkeypatch: pytest.MonkeyPatch) -> None: + report = DoctorReport(providers=[_prov("copilot", tier=None)]) + _patch_gather(monkeypatch, report) + result = runner.invoke(app, ["doctor"]) + assert result.exit_code == 0 + assert "—" in result.output + + def test_models_error_renders_cross_with_message(self, monkeypatch: pytest.MonkeyPatch) -> None: + report = DoctorReport( + providers=[ + _prov("copilot", checked=True, connection_ok=True, models_error="rate limited") + ] + ) + _patch_gather(monkeypatch, report) + result = runner.invoke(app, ["doctor", "--models"]) + assert result.exit_code == 0 + assert "rate limited" in result.output + assert "✗" in result.output From 0ef11e330209b3642f423f8955b8f29f2bd2deca Mon Sep 17 00:00:00 2001 From: Amir Fathi Date: Thu, 27 Aug 2026 07:21:40 +0000 Subject: [PATCH 2/2] fix(cli): address doctor review, add warn glyph and fix Models title dash jrob5756's review on #469 found the fix left one path open: the connection-note branch of _connection_cell still printed a bare warning glyph outside the resolved set, so conductor doctor --check still crashed on cp1252 for any provider whose probe came back inconclusive. Added a warn entry to _Glyphs (falls back to "!") and routed that branch through it. Also fixed the Models detail table title, which embedded a bare em dash and crashed on ascii/latin-1/cp437 independent of #401. Uses the already-resolved dash glyph's plain text instead. Corrected the _encodable and _resolve_glyphs docstrings per the review (StringIO.encoding is None, not absent; switched to MarkupFreeConsole.encoding instead of reaching through console.file). Reworded a docstring near _PRICING_SOURCE_CELLS that implied ASCII literals in general need no fallback rather than these four specifically. Test changes: added connection_note to the _prov helper and parametrized the cp1252 crash test over two diagnostic shapes crossed with no flags, --check and --models, since --check/--models are the only paths that render the Connection column at all. Fixed a misattributed comment and a tautological assertion in the same test. Filled in the Credentials and Notes cells in the missing-tier test so the dash assertion can only pass because of the branch under test. --- CHANGELOG.md | 13 ++++---- src/conductor/cli/doctor.py | 41 ++++++++++++++---------- tests/test_cli/test_doctor.py | 60 +++++++++++++++++++++++++++++------ 3 files changed, 82 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7e194c5..d96ca750 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,12 +42,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 provider and is now forwarded through `ProviderRegistry`. - **`conductor doctor`'s table output no longer dies part-written on a `cp1252` console** (#401). The Installed/Credentials/Connection/Models - columns hardcoded `✓`/`✗`/`○`, none of which cp1252 can encode, so a run - on a legacy Windows console raised `UnicodeEncodeError` mid-table, after - the Environment section had already printed. `conductor doctor` now - resolves each glyph once per invocation against the output console's - stream encoding, falling back to `OK`/`X`/`o` when the Unicode glyphs - cannot be encoded; the `--json` path was already safe and is unchanged. + columns hardcoded `✓`/`✗`/`○`/`⚠`, none of which cp1252 can encode, so a + run on a legacy Windows console raised `UnicodeEncodeError` mid-table, + after the Environment section had already printed. `conductor doctor` + now resolves each glyph once per invocation against the output console's + stream encoding, falling back to `OK`/`X`/`o`/`!` when the Unicode + glyphs cannot be encoded; the `--json` path was already safe and is + unchanged. - **MCP tool discovery and structured tool results no longer break with MCP 2.0** (#419). MCP 2.0 renamed the Python field on `mcp.types.Tool` from `inputSchema` to `input_schema` and on `mcp.types.CallToolResult` from diff --git a/src/conductor/cli/doctor.py b/src/conductor/cli/doctor.py index 25b0593c..2cb9974f 100644 --- a/src/conductor/cli/doctor.py +++ b/src/conductor/cli/doctor.py @@ -41,6 +41,7 @@ class _Glyphs(NamedTuple): check: Text cross: Text dash: Text + warn: Text optional: str """Neutral glyph for an absent *optional* credential — deliberately not ``cross``, which is reserved for a genuinely missing required credential @@ -51,23 +52,26 @@ class _Glyphs(NamedTuple): check=Text.from_markup("[green]✓[/green]"), cross=Text.from_markup("[red]✗[/red]"), dash=Text.from_markup("[dim]—[/dim]"), + warn=Text.from_markup("[yellow]⚠[/yellow]"), optional="○", ) _ASCII_GLYPHS = _Glyphs( check=Text.from_markup("[green]OK[/green]"), cross=Text.from_markup("[red]X[/red]"), dash=Text.from_markup("[dim]-[/dim]"), + warn=Text.from_markup("[yellow]![/yellow]"), optional="o", ) def _encodable(text: str, encoding: str | None) -> bool: - """Whether *text* survives a round trip through *encoding*. + """Whether *text* can be encoded to *encoding*. - ``encoding`` of ``None`` (the stream has no ``.encoding`` attribute, e.g. - an in-memory buffer) is treated as capable: there is nothing to protect - against, so it is better to render full-width Unicode than to needlessly - downgrade every invocation to ASCII. + A falsy ``encoding`` is treated as capable so an in-memory buffer is not + needlessly downgraded. ``io.StringIO`` has an ``.encoding`` of ``None``; + rich's ``NULL_FILE`` has no such attribute at all. This is a deliberate + fail-open: a stream that is lossy *and* silent about its encoding (e.g. + ``codecs.getwriter``) will still raise. """ if not encoding: return True @@ -78,21 +82,26 @@ def _encodable(text: str, encoding: str | None) -> bool: return True -def _resolve_glyphs(console: Console) -> _Glyphs: +def _resolve_glyphs(console: MarkupFreeConsole) -> _Glyphs: """Pick Unicode or ASCII-safe glyphs for *console*'s stream encoding. Rich hands a rendered line straight to the underlying file's ``write()``; it does not check whether the target encoding can represent it. A legacy - Windows console (``cp1252``) cannot encode ``✓``/``✗``/``○``, so the table - used to die mid-write, part-printed (issue #401). Resolved once per - ``run_doctor`` call and passed down, rather than re-checked per cell: the - stream's encoding cannot change mid-render. + Windows console (``cp1252``) cannot encode ``✓``/``✗``/``○``/``⚠``, so the + table dies mid-write, part-printed (issue #401). Resolved once per + ``run_doctor`` call and passed down rather than re-checked per cell, so + every cell in one report agrees. + + Probed per glyph rather than through rich's ``ConsoleOptions.ascii_only``, + which is a ``startswith("utf")`` prefix test: ``gb18030`` encodes all of + these and that check would downgrade it for nothing. """ - encoding = getattr(getattr(console, "file", None), "encoding", None) + encoding = console.encoding return _Glyphs( check=_UNICODE_GLYPHS.check if _encodable("✓", encoding) else _ASCII_GLYPHS.check, cross=_UNICODE_GLYPHS.cross if _encodable("✗", encoding) else _ASCII_GLYPHS.cross, dash=_UNICODE_GLYPHS.dash if _encodable("—", encoding) else _ASCII_GLYPHS.dash, + warn=_UNICODE_GLYPHS.warn if _encodable("⚠", encoding) else _ASCII_GLYPHS.warn, optional=_UNICODE_GLYPHS.optional if _encodable("○", encoding) else _ASCII_GLYPHS.optional, ) @@ -322,7 +331,7 @@ def _connection_cell(diag: ProviderDiagnostic, glyphs: _Glyphs) -> Text: if not diag.checked or diag.connection_ok is None: return glyphs.dash if diag.connection_ok and diag.connection_note: - return styled("[yellow]⚠[/yellow] {}", diag.connection_note) + return styled("{} {}", glyphs.warn, diag.connection_note) if diag.connection_ok: return styled("{} connected", glyphs.check) if diag.connection_error: @@ -387,9 +396,9 @@ def _default_effort_cell(model: ModelDiagnostic, glyphs: _Glyphs) -> Text: re-parsing the same markup literal on every table row (matching the ``_UNICODE_GLYPHS``/``_ASCII_GLYPHS`` constants above) — each markup argument is still a literal template, not an interpolated value, keeping this inside -the repo's console rules (see AGENTS.md "Console Output"). None of these -values are drawn from the encoding-sensitive glyph set, so they need no -ASCII fallback.""" +the repo's console rules (see AGENTS.md "Console Output"). Every value here +is pure ASCII, so no fallback applies: a property of these four literals, +not a rule that anything outside :class:`_Glyphs` is safe to print.""" def _rate_cell(value: float | None, glyphs: _Glyphs) -> Text: @@ -430,7 +439,7 @@ def _render_models(providers: list[ProviderDiagnostic], console: Console, glyphs for diag in providers: if not diag.models: continue - table = Table(title=f"Models — {diag.name}", show_lines=True) + table = Table(title=f"Models {glyphs.dash.plain} {diag.name}", show_lines=True) table.add_column("Model", style="cyan", no_wrap=True) table.add_column("Reasoning efforts") table.add_column("Default") diff --git a/tests/test_cli/test_doctor.py b/tests/test_cli/test_doctor.py index 59edfcb4..bcbdd980 100644 --- a/tests/test_cli/test_doctor.py +++ b/tests/test_cli/test_doctor.py @@ -70,6 +70,7 @@ def _prov( checked: bool = False, connection_ok: bool | None = None, connection_error: str | None = None, + connection_note: str | None = None, models: list[str] | list[ModelDiagnostic] | None = None, models_error: str | None = None, note: str | None = None, @@ -97,6 +98,7 @@ def _prov( checked=checked, connection_ok=connection_ok, connection_error=connection_error, + connection_note=connection_note, models=model_diagnostics, models_error=models_error, note=note, @@ -877,10 +879,12 @@ def _bind_console( """Point the CLI's output console at a fresh buffer with *encoding*. Returns the raw byte buffer rather than relying on ``result.output``: - the CLI's real console is bound directly to it, decoded with the same - *encoding* it was written with, so a character neither side can - represent surfaces as a decode error rather than being silently - swallowed by pytest's own UTF-8 capture. + Click's ``CliRunner`` captures through a UTF-8 stream that would + accept a glyph a real cp1252 console rejects, so a test could pass + on output the user never gets. Binding the CLI's console to a + ``TextIOWrapper`` in the target *encoding* makes a leaked glyph + raise ``UnicodeEncodeError`` at write time, surfacing via + ``result.exception``. """ buffer = io.BytesIO() stream = io.TextIOWrapper(buffer, encoding=encoding, newline="") @@ -890,12 +894,34 @@ def _bind_console( ) return buffer, stream + @pytest.mark.parametrize( + "shape_kwargs", + [ + pytest.param({}, id="no-connection-data"), + pytest.param( + { + "checked": True, + "connection_ok": True, + "connection_note": "probe was inconclusive; endpoint may lack /v1/models", + }, + id="connection-note", + ), + ], + ) + @pytest.mark.parametrize("cli_args", [[], ["--check"], ["--models"]]) def test_cp1252_console_renders_ascii_glyphs_without_crashing( - self, monkeypatch: pytest.MonkeyPatch + self, + monkeypatch: pytest.MonkeyPatch, + shape_kwargs: dict[str, object], + cli_args: list[str], ) -> None: + # The "connection-note" shape crossed with "--check"/"--models" is + # what exercises _connection_cell's warning branch: the two flags + # that add the Connection column are the only paths that ever touch + # the cp1252 stream with this data (#401 follow-up review). buffer, stream = self._bind_console(monkeypatch, "cp1252") report = DoctorReport( - providers=[_prov("copilot", installed=True)], + providers=[_prov("copilot", installed=True, **shape_kwargs)], registries=RegistryDiagnostic( default="local", registries=[ @@ -904,13 +930,14 @@ def test_cp1252_console_renders_ascii_glyphs_without_crashing( ), ) _patch_gather(monkeypatch, report) - result = runner.invoke(app, ["doctor"]) + result = runner.invoke(app, ["doctor", *cli_args]) stream.flush() assert result.exception is None assert result.exit_code == 0 - output = buffer.getvalue().decode("cp1252") # raises if a raw glyph leaked through + # Decode is exact, not lossy; a leaked glyph is caught by the + # result.exception assertion above, which fails at write time. + output = buffer.getvalue().decode("cp1252") assert "OK" in output - assert "✓" not in output def test_utf8_console_keeps_unicode_glyphs(self, monkeypatch: pytest.MonkeyPatch) -> None: buffer, stream = self._bind_console(monkeypatch, "utf-8") @@ -950,7 +977,20 @@ class TestDoctorTierAndModelsErrorCells: encoding-fallback tests above, exercised on the default UTF-8 path.""" def test_missing_tier_renders_dash(self, monkeypatch: pytest.MonkeyPatch) -> None: - report = DoctorReport(providers=[_prov("copilot", tier=None)]) + # Credentials and note are filled so the tier cell is the only one + # that can render a dash; with _prov's defaults both of those cells + # dash too, and the assertion holds even without the tier=None + # branch under test (caught by review on #469). + report = DoctorReport( + providers=[ + _prov( + "copilot", + tier=None, + creds=[CredentialEnvVar(name="COPILOT_TOKEN", present=True)], + note="see docs", + ) + ] + ) _patch_gather(monkeypatch, report) result = runner.invoke(app, ["doctor"]) assert result.exit_code == 0