diff --git a/CHANGELOG.md b/CHANGELOG.md index d2cee27c..d96ca750 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,15 @@ 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..2cb9974f 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,75 @@ 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 + warn: 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]"), + 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* can be encoded to *encoding*. + + 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 + try: + text.encode(encoding) + except (UnicodeEncodeError, LookupError): + return False + return True + + +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 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 = 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, + ) def run_doctor( @@ -109,14 +164,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 +260,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 +280,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 +314,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) + return styled("{} {}", glyphs.warn, 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 +348,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 +377,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 +394,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"). 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) -> 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 +409,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 +427,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 @@ -380,7 +439,7 @@ def _render_models(providers: list[ProviderDiagnostic], console: Console) -> Non 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") @@ -395,23 +454,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 +488,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..bcbdd980 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 @@ -69,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, @@ -96,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, @@ -859,3 +862,148 @@ 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``: + 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="") + 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 + + @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, + 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, **shape_kwargs)], + registries=RegistryDiagnostic( + default="local", + registries=[ + RegistryInfo(name="local", type="path", source="~/.conductor", is_default=True) + ], + ), + ) + _patch_gather(monkeypatch, report) + result = runner.invoke(app, ["doctor", *cli_args]) + stream.flush() + assert result.exception is None + assert result.exit_code == 0 + # 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 + + 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: + # 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 + 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