Uh oh!
There was an error while loading. Please reload this page.
fix(cli): resolve doctor's glyph set per console encoding (#401) - #469
fix(cli): resolve doctor's glyph set per console encoding (#401)#469Amir Fathi (AmirF194) wants to merge 2 commits into
Conversation
) 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 (microsoft#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. Fixesmicrosoft#401
00ee432 to
72bef58Compare
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Nice fix, and the shape is right: resolve once from the stream encoding and thread it explicitly instead of reaching for module globals. Probing per glyph rather than using rich's ascii_only is a better call than it might look, too. I checked gb18030, which encodes all of these but reports ascii_only=True, so rich's prefix test would have downgraded it for nothing. Your _bind_console harness is genuinely good: binding the real console to a strict TextIOWrapper makes the stream itself the assertion.
The blocker is that the glyph set isn't closed. _connection_cell still prints a bare ⚠ (U+26A0), which cp1252 can't encode either, so conductor doctor --check fails exactly the way #401 describes:
conductor doctor --check (cp1252 console)
-> UnicodeEncodeError: 'charmap' codec can't encode character '\u26a0' in position 376
That's a shipped path, not a corner case. providers/claude.py:456 sets _connection_probe_note on an inconclusive probe (#455), providers/diagnostics.py:617-618 copies it to connection_note, and the branch renders it. The result is also worse than the bug you started from: the exception reaches cli/app.py:182, whose error panel title carries ❌ (also not cp1252), so that write fails as well and the user gets zero bytes and exit 1 rather than a partial table.
No test could have caught it. _prov has no connection_note parameter, and grep -rn connection_note tests/ comes back empty across the whole repo.
A second one is sitting at line 433, Table(title=f"Models — {diag.name}"). cp1252 handles the em dash so #401's scenario is fine, but --models still dies on ascii, latin-1 and cp437 while _resolve_glyphs is busy correctly downgrading dash to - two hundred lines away.
That pattern is worth a moment. test_markup_guards.py opens by saying the behavioural tests can't prove the next call site is correct, and that #387 fixed cli/run.py thoroughly and still left a Panel(title=) in the function it changed. This is the same story with a different codepoint. A guard rule rejecting non-ASCII literals outside the glyph constants would have flagged both lines with file and line the day it was written, and it fits the existing rule A-H idiom exactly. Worth considering as a follow-up if not here.
Everything else below is smaller: a few docstrings that now claim more than the code delivers, and some test assertions that pass for the wrong reason.
Gates are all clean on my end: ruff check, ruff format --check, ty check src (fully green, no pre-existing warnings), 1106 passed in tests/test_cli, 164 markup-guard tests passed. No Console Output convention violations in the diff, and the removal half left no orphans behind, which is the usual failure mode for a change of this shape.
| """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 |
There was a problem hiding this comment.
This reads as an inventory of the module's status glyphs, but it isn't one. _connection_cell prints a ⚠ that never comes through here, and cp1252 can't encode that either. Adding it as a field is the smallest change that makes the docstring true.
| """The status glyphs a table render uses, resolved for one console.""" | |
| .. note:: | |
| ``✓``, ``✗``and``○``arenotencodableincp1252, sothedefault*table* | |
| outputof``conductordoctor``stillfailsonsuchaconsole — see#401. | |
| Thismodule's``--json``pathissafe (``ensure_ascii=True``); thetablepath | |
| isdeliberatelyoutofscopeherebecauseeveryglyphconsumerwouldneedthe | |
| consolethreadedthroughit. Theem-dashisencodableincp1252. | |
| """ | |
| check: Text | |
| cross: Text | |
| dash: Text | |
| optional: str | |
| """The status glyphs a table render uses, resolved for one console.""" | |
| check: Text | |
| cross: Text | |
| dash: Text | |
| warn: Text | |
| optional: str |
| _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", | ||
| ) |
There was a problem hiding this comment.
Matching warn entries for the field above.
Worth a look while you're here: OK is two cells wide where o and X are one, so the multi-line Credentials cell goes ragged on exactly the console this is meant to serve. Single-character ASCII (+/x/o/-) would keep the names lined up.
| _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", | |
| ) | |
| _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", | |
| ) |
| """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. |
There was a problem hiding this comment.
Two small corrections. io.StringIO does have an .encoding attribute, it is just None (your own test on line 934 asserts exactly that). The genuinely attribute-less case is rich's NULL_FILE. Also nothing here does a round trip, and none of these glyphs are full-width in the Unicode sense.
The there is nothing to protect against claim is the part I'd most want changed. codecs.getwriter("cp1252")(...) returns a real, lossy stream that reports no encoding at all, and this branch hands it ✓. Harmless at today's call site, but it is the reasoning a future reader will lean on.
| """Whether*text*survivesaroundtripthrough*encoding*. | |
| ``encoding``of``None`` (thestreamhasno``.encoding``attribute, e.g. | |
| anin-memorybuffer) istreatedascapable: thereisnothingtoprotect | |
| against, soitisbettertorenderfull-widthUnicodethantoneedlessly | |
| downgradeeveryinvocationtoASCII. | |
| """Whether*text*canbeencodedto*encoding*. | |
| Afalsy``encoding``istreatedascapablesoanin-memorybufferisnot | |
| needlesslydowngraded. ``io.StringIO``hasan``.encoding``of``None``; | |
| rich's``NULL_FILE``hasnosuchattributeatall. Thisisadeliberate | |
| fail-open: astreamthatislossy*and*silentaboutitsencoding (e.g. | |
| ``codecs.getwriter``) willstillraise. | |
| """ |
| 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, | ||
| ) |
There was a problem hiding this comment.
Three things folded together here.
The annotation should be MarkupFreeConsole. console.py:168-171 calls that "the one place the guarantee can enter the type system", and rule A of the AST guard only catches construction, not annotations. It is already imported at line 20.
console.encoding is rich's public property and does the same getattr(self.file, "encoding", "utf-8") internally. I checked it against all three cases your tests cover and it agrees on every one, including StringIO. It is present in rich>=13.0.0, the floor in pyproject.toml, so there is no pin risk. The outer getattr(console, "file", None) can only fire if the property getter itself raises, which means a half-constructed Console.
And used to die isn't true yet, per the ⚠ on line 325.
Separately, and not something I'd hold the PR for: each glyph is written twice, once inside Text.from_markup and once as the probe argument 40 lines away. Change one and the other silently guards the wrong character. Deriving the probe from the constant would also mean a fifth glyph can't be added without being probed, which is how this one got through.
| def_resolve_glyphs(console: Console) ->_Glyphs: | |
| """PickUnicodeorASCII-safeglyphsfor*console*'sstreamencoding. | |
| Richhandsarenderedlinestraighttotheunderlyingfile's``write()``; | |
| itdoesnotcheckwhetherthetargetencodingcanrepresentit. Alegacy | |
| Windowsconsole (``cp1252``) cannotencode``✓``/``✗``/``○``, sothetable | |
| usedtodiemid-write, part-printed (issue#401). Resolved once per | |
| ``run_doctor``callandpasseddown, ratherthanre-checkedpercell: the | |
| stream'sencodingcannotchangemid-render. | |
| """ | |
| encoding=getattr(getattr(console, "file", None), "encoding", None) | |
| return_Glyphs( | |
| check=_UNICODE_GLYPHS.checkif_encodable("✓", encoding) else_ASCII_GLYPHS.check, | |
| cross=_UNICODE_GLYPHS.crossif_encodable("✗", encoding) else_ASCII_GLYPHS.cross, | |
| dash=_UNICODE_GLYPHS.dashif_encodable("—", encoding) else_ASCII_GLYPHS.dash, | |
| optional=_UNICODE_GLYPHS.optionalif_encodable("○", encoding) else_ASCII_GLYPHS.optional, | |
| ) | |
| def_resolve_glyphs(console: MarkupFreeConsole) ->_Glyphs: | |
| """PickUnicodeorASCII-safeglyphsfor*console*'sstreamencoding. | |
| Richhandsarenderedlinestraighttotheunderlyingfile's``write()``; | |
| itdoesnotcheckwhetherthetargetencodingcanrepresentit. Alegacy | |
| Windowsconsole (``cp1252``) cannotencode``✓``/``✗``/``○``/``⚠``, sothe | |
| tablediesmid-write, part-printed (issue#401). Resolved once per | |
| ``run_doctor``callandpasseddownratherthanre-checkedpercell, so | |
| everycellinonereportagrees. | |
| Probedperglyphratherthanthroughrich's``ConsoleOptions.ascii_only``, | |
| whichisa``startswith("utf")``prefixtest: ``gb18030``encodesallof | |
| theseandthatcheckwoulddowngradeitfornothing. | |
| """ | |
| encoding=console.encoding | |
| return_Glyphs( | |
| check=_UNICODE_GLYPHS.checkif_encodable("✓", encoding) else_ASCII_GLYPHS.check, | |
| cross=_UNICODE_GLYPHS.crossif_encodable("✗", encoding) else_ASCII_GLYPHS.cross, | |
| dash=_UNICODE_GLYPHS.dashif_encodable("—", encoding) else_ASCII_GLYPHS.dash, | |
| warn=_UNICODE_GLYPHS.warnif_encodable("⚠", encoding) else_ASCII_GLYPHS.warn, | |
| optional=_UNICODE_GLYPHS.optionalif_encodable("○", encoding) else_ASCII_GLYPHS.optional, | |
| ) |
| return _DASH | ||
| return glyphs.dash | ||
| if diag.connection_ok and diag.connection_note: | ||
| return styled("[yellow]⚠[/yellow] {}", diag.connection_note) |
There was a problem hiding this comment.
This is the one blocking item. ⚠ is U+26A0, cp1252 can't encode it, and it never goes through _Glyphs even though glyphs is in scope and used on the line below. conductor doctor --check still fails the way #401 describes:
conductor doctor --check (cp1252 console)
-> UnicodeEncodeError: 'charmap' codec can't encode character '\u26a0' in position 376
It's a live path. providers/claude.py:456 sets _connection_probe_note whenever the probe is inconclusive (#455), providers/diagnostics.py:617-618 copies it into connection_note, and this branch renders it. So it fires for any Claude-compatible endpoint that doesn't implement /v1/models.
The outcome is also worse than the original bug rather than equal to it. The exception reaches cli/app.py:182, whose error panel title carries ❌ (U+274C, also not cp1252), so that write fails too and the user gets zero bytes and exit 1 instead of a partial table.
| returnstyled("[yellow]⚠[/yellow] {}", diag.connection_note) | |
| returnstyled("{} {}", glyphs.warn, diag.connection_note) |
| - **`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. |
There was a problem hiding this comment.
As written this is false for --check until line 325 is fixed, and a user who hits that crash will have read here that it was solved. The fallback list also omits — to -, which matters on ascii and latin-1 even though it doesn't on cp1252.
If you take the warn fix, the original headline becomes accurate and you can drop the last sentence of the suggestion.
| -**`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. | |
| -**`conductor doctor`'s status glyphs degrade to ASCII on a console that | |
| cannot encode them** (#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. |
| 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. |
There was a problem hiding this comment.
The harness itself is the best part of this PR: binding the real console to a strict TextIOWrapper makes the stream do the asserting. The docstring describes it backwards though. A leaked glyph raises UnicodeEncodeError on the way in, at write() time, and the buffer stays empty, so the decode on line 911 never sees it. And the UTF-8 capture that would otherwise swallow it belongs to Click's CliRunner, not pytest.
| Returnstherawbytebufferratherthanrelyingon``result.output``: | |
| theCLI'srealconsoleisbounddirectlytoit, decodedwiththesame | |
| *encoding*itwaswrittenwith, soacharacterneithersidecan | |
| representsurfacesasadecodeerrorratherthanbeingsilently | |
| swallowedbypytest'sownUTF-8capture. | |
| Returnstherawbytebufferratherthanrelyingon``result.output``: | |
| Click's``CliRunner``capturesthroughaUTF-8streamthatwouldaccept | |
| aglypharealcp1252consolerejects, soatestcouldpassonoutput | |
| theusernevergets. BindingtheCLI'sconsoletoa``TextIOWrapper`` | |
| inthetarget*encoding*makesaleakedglyphraise | |
| ``UnicodeEncodeError``atwritetime, surfacingvia | |
| ``result.exception``. |
| ), | ||
| ) | ||
| _patch_gather(monkeypatch, report) | ||
| result = runner.invoke(app, ["doctor"]) |
There was a problem hiding this comment.
This is where the ⚠ slipped through, and I don't think the assertions are to blame. assert result.exception is None on line 909 already is a complete "every glyph survives" check, enforced by the codec at write time, with nobody having to guess which character is at fault. It just only ever runs against one fixture under one invocation.
Two gaps behind that. _prov (line 62) forwards eleven of ProviderDiagnostic's twelve fields and the one it omits is connection_note, so no test in this file can construct the crashing branch. grep -rn connection_note tests/ returns nothing for the whole repo. And --check and --models, the two flags that add the columns this PR rewrote, never touch the cp1252 stream at all.
I'd add connection_note to _prov and parametrize this test over diagnostic shapes crossed with [], --check and --models. Enumerating shapes rather than characters is what makes it catch the next one. Note that once connection_ok=False shapes are in the matrix the assertion needs to become assert not isinstance(result.exception, UnicodeEncodeError), since those exit 1 legitimately.
| output = buffer.getvalue().decode("cp1252") # raises if a raw glyph leaked through | ||
| assert "OK" in output | ||
| assert "✓" not in output |
There was a problem hiding this comment.
The comment credits the wrong line. The decode can't raise: only five byte values are undefined in cp1252 and a cp1252 encode can't produce any of them. Line 909 is what actually catches a leak, so a future cleanup that drops it as redundant would quietly remove the only real check. assert "✓" not in output is a tautology for the same reason.
Separately, assert "OK" not in output on line 925 is a whole-report negative on a two-letter string. I confirmed it breaks on a fixture as ordinary as note="HTTP 200 OK from probe". Asserting on _resolve_glyphs(...).check.plain, or scoping to the Installed cell, would be sturdier.
| output=buffer.getvalue().decode("cp1252") # raises if a raw glyph leaked through | |
| assert"OK"inoutput | |
| assert"✓"notinoutput | |
| # 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"inoutput |
| 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 |
There was a problem hiding this comment.
This one passes whether or not the branch it names exists. With _prov's defaults the Credentials and Notes cells also render —, so the assertion holds with tier="stable" too. I checked. Deleting _tier_cell's if tier is None branch wouldn't fail it.
Filling the other two cells makes the tier cell the only possible dash. Worth noting the class docstring above describes these as driven by the same glyph set as the encoding tests, but both run on UTF-8 against result.output; moving them onto the cp1252 stream would turn them into real regression tests for free.
| deftest_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"]) | |
| assertresult.exit_code==0 | |
| assert"—"inresult.output | |
| deftest_missing_tier_renders_dash(self, monkeypatch: pytest.MonkeyPatch) ->None: | |
| 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"]) | |
| assertresult.exit_code==0 | |
| assert"—"inresult.output |
…dash jrob5756's review on microsoft#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 microsoft#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.
Amir Fathi (AmirF194)
commented
Aug 27, 2026
Thanks for the thorough pass. Took the blocking fix and every suggestion: added a warn glyph to _Glyphs (falls back to !), routed the connection-note branch through it, switched _resolve_glyphs to MarkupFreeConsole.encoding, corrected the _encodable and _PRICING_SOURCE_CELLS docstrings, added connection_note to the test helper and parametrized the crash test over --check/--models (that's the actual gap, since those are the only flags that render the Connection column), fixed the misattributed comment and the tautological assert, and tightened the missing-tier test so it can only pass on the branch under test. One divergence: for the Models title I kept it dash-resolving rather than hardcoding an ASCII hyphen, using glyphs.dash.plain so it's still a plain str (no title_style loss, verified). Preserves the unicode em dash on consoles that can take it while still downgrading where needed. Left the AGENTS.md rule-A/H guard and the app.py:182 panel-title glyph as follow-ups, not in this PR. Full suite plus the doctor.py coverage report both green, pushed to 0ef11e3. |
Root cause
conductor doctor's table output hardcodesText.from_markup("[green]✓[/green]")/[red]✗[/red]/[dim]—[/dim]and the literal glyph○(src/conductor/cli/doctor.py:38-51), consumed across_render_providers,_render_registries, and six_*_cellhelpers, none of which take a console/encoding parameter. Rich hands a rendered line straight to the underlying file'swrite()without checking whether the target encoding can represent it, so a run on a legacy Windows console (cp1252, which cannot encode✓/✗/○) raisesUnicodeEncodeErrormid-table, after the Environment section has already printed. The report is truncated and the process exits non-zero for a run that actually succeeded.--jsonalready guards this withensure_ascii=True(#381); the table path was deliberately left out of scope there and filed as this issue.Fix
Resolve a
_Glyphstuple once perrun_doctor()call from the output console's actual stream encoding (_resolve_glyphs,_encodable), falling back toOK/X/o/-when the Unicode set isn't encodable, and thread it as an explicit parameter through every cell helper instead of reaching for a module-level constant. A console with no.encodingattribute (e.g. an in-memory buffer) is treated as capable, matching the existing "nothing to protect against" default. The--jsonpath and the credential-rendering logic from #319 are unchanged.Verification
TestDoctorEncodingFallback) binds the CLI's console to acp1252-encoded stream and asserts the run completes and rendersOK/Xinstead of raising; a UTF-8 counterpart asserts the Unicode glyphs are still used there. Confirmed failing onmain(UnicodeEncodeErrorat the same character the issue describes) and passing on this branch, in Docker.tests/test_cli/test_doctor.py(44 tests) green on Python 3.12 and 3.13 in Docker;ruff check,ruff format --check, andty check srcall clean (the 4 pre-existingtywarnings inproviders/claude_agent_sdk.pyare unrelated to this diff, confirmed by runningty checkagainst unmodifiedmain).pytest --cov, 7671 passed) is green except 6 failures I traced to running as root in a container (permission/unreadable-file tests that assume a non-root user, e.g.test_init_file_logging_permission_denied) and one order-dependent flake (test_renders_the_wordmark, passes in isolation): reproduced identically against unmodifiedmain, so none are caused by this change.conductor doctorCLI invocation underPYTHONIOENCODING=cp1252; the regression test binds the module's own console object directly to acp1252-encodedTextIOWrapper, which exercises the same write path.