Uh oh!
There was an error while loading. Please reload this page.
fix(cli): stop agent text being parsed as Rich markup - #387
Conversation
Agent output is arbitrary text and regularly contains bracketed tokens -- code,
globs, regexes, ARM/URI patterns. Rich parses those as markup, so a closing-tag
form in ordinary technical prose raises MarkupError and kills the run.
The trigger that found this was a plan document describing an Azure RBAC
permission format:
{provider}/{type}[/{nestedType}...]/read
Nothing about that is malformed; it is what the format looks like. It killed a
long-running workflow and then killed both resume attempts at the identical
offset, because the text is checkpointed and replayed verbatim -- the run was
unresumable until the checkpoint was hand-edited.
Two sinks render agent-supplied content, and only one is verbosity-gated:
- The file console (init_file_logging) is live whenever --log-file is passed,
with NO verbosity condition. It now sets markup=False. The log is already
no_color=True plain text so it has no use for markup, and every styled write
in the module either targets _verbose_console or passes a pre-built rich Text,
neither of which this flag affects -- verified across all 20 call sites.
- The verbose console keeps markup=True for conductor's own [cyan] title, so
the agent-supplied content is escaped at the call site instead.
The content is the rendered prompt, which embeds user-supplied plan text, so
this is driven by user data rather than anything conductor controls.
Tests assert both sinks, and pin the samples themselves as genuine triggers so
no case can pass vacuously. The production-console assertions matter: the
behavioural tests build their own Console and would keep passing if someone
dropped markup=False from init_file_logging. Verified by reverting both fixes --
3 of 15 fail, and all 15 pass on the fixed tree.
Full tests/test_cli + tests/test_web matches the pre-existing baseline exactly
(48 failures before and after; my sandbox lacks pytest-asyncio). ruff check and
ruff format --check clean.
Fixesmicrosoft#382Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #387 +/- ##
=======================================
Coverage ? 91.32% =======================================
Files ? 108 Lines ? 17467 Branches ? 0 =======================================
Hits ? 15951 Misses ? 1516 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Nice find, and the reproduction in the issue made this easy to confirm. I appreciate you digging in here! I reproduced the crash, and both sinks you changed are genuinely safe now. Gates are clean here too: 521 passed / 3 skipped in tests/test_cli, plus ruff and ty.
Three things I would want resolved before we merge.
markup=Falseregresses the experimental-provider banner in the log file, and the comment added alongside it states an invariant that this same file already violates.- Three more sinks in
run.pystill raise on the same input, two of which I reproduced as realconductor runfailures with no flags set. - The test fixtures are all closing-tag forms, so the suite passes against an implementation that silently discards agent text.
Two smaller ones that don't fit on a line in the diff.
CHANGELOG. Every commit merged since v0.1.27 updated CHANGELOG.md, and a crash that kills a run and leaves it unresumable is squarely the kind of entry the existing ### Fixed section covers.
Resume coverage. The unresumability is the heart of the issue and nothing exercises it. It would also pin something no current test does: that the checkpoint stores agent text verbatim and escaping happens at render time. A well-meaning alternative fix that escaped at checkpoint-write would pass all 15 tests today and double-escape stored context on every resume.
Worth saying what is good here, because some of it is unusual. TestMarkupTriggersAreRealTriggers guards its own fixtures against passing vacuously, and that earned its keep during review when a reviewer's own draft test passed for the wrong reason. Using the real Azure RBAC string rather than a synthetic [/foo] will age well. And the title versus content split is the right trust boundary, explained rather than just applied.
| # markup=False because this console renders agent-supplied text (prompts, | ||
| # tool output, model responses) verbatim. Rich would otherwise parse a | ||
| # bracketed token such as ``[/nestedType]`` -- ordinary technical prose -- | ||
| # as a closing tag and raise MarkupError, killing the run (see #382). The | ||
| # log is already ``no_color=True`` plain text, so it has no use for markup; | ||
| # every styled write in this module goes to ``_verbose_console`` or passes a | ||
| # pre-built ``rich.text.Text``, neither of which is affected by this flag. |
There was a problem hiding this comment.
The last two lines of this comment are not true of the file as it stands, and they are the argument for why the change is safe.
_maybe_print_experimental_banner (~L906-918) builds a Panel from a markup-bearing plain string and prints that same object to _verbose_consoleand_file_console. I A/B'd it against main: the log file now contains the literal tags rather than styled text.
main: Experimental provider in use: claude-agent-sdk (pin) maintained by @someone
here: Experimental provider in use: [bold]claude-agent-sdk[/bold] ([dim]pin[/dim]) maintained by [dim]@someone[/dim]
Nothing catches it: test_experimental_banner.py swaps _verbose_console for a MagicMock and never calls init_file_logging, so it cannot observe the file sink.
Two separate things to fix. The banner should build its body with Text.from_markup("\n".join(body_lines)), which I verified renders identically on both consoles. And the comment should stop claiming an invariant that nothing enforces.
Also worth dropping the no_color=True clause. no_color suppresses colour only; bold and italic still emit ANSI:
force_terminal=True, no_color=True : '\x1b[1mB\x1b[0m \x1b[3mI\x1b[0m R'
Markup looked harmless in the log because the console writes to a file handle and is_terminal is False, not because of no_color. Anyone who later sets force_terminal=True would be misled.
| # markup=False because this console renders agent-supplied text (prompts, | |
| # tool output, model responses) verbatim. Rich would otherwise parse a | |
| # bracketed token such as ``[/nestedType]`` -- ordinary technical prose -- | |
| # as a closing tag and raise MarkupError, killing the run (see #382). The | |
| # log is already ``no_color=True`` plain text, so it has no use for markup; | |
| # every styled write in this module goes to ``_verbose_console`` or passes a | |
| # pre-built ``rich.text.Text``, neither of which is affected by this flag. | |
| # markup=False because this console renders agent-supplied text (prompts, | |
| # tool output, model responses) verbatim. Rich would otherwise parse a | |
| # bracketed token such as ``[/nestedType]`` -- ordinary technical prose -- | |
| # as a closing tag and raise MarkupError, killing the run (see #382). | |
| # Consequence: a markup-bearing renderable now prints its tags literally | |
| # here, so style file output with ``rich.text.Text`` rather than markup. |
| @@ -376,7 +386,14 @@ def verbose_log_section(title: str, content: str) -> None: | |||
| return | |||
| if should_console: | |||
There was a problem hiding this comment.
Three more sinks in this same file still die on the identical input, and two of them are reachable without any flags at all.
run.py:202 _verbose_console.print(f"[{style}]{message}[/{style}]")
run.py:524 _verbose_console.print(error_msg, style="red dim")
run.py:763 same pattern
style= does not turn markup parsing off, which I checked directly. At 524 and 763 error_msg carries a provider exception message, so a workflow that is already failing has its real error replaced by a MarkupError.
I reproduced two of these as real conductor run failures on this branch with no flags set:
- a
type: scriptstep whose stderr contains the trigger reachesexecutor/script.py:212and exits 1 withclosing tag '[/nestedType]' at position 103 - a
type: waitstep whose renderedreasoncontains it reachesexecutor/wait.py:143and exits 1
Related: verbose_mode and full_mode both default to True (app.py:56,59), so the console path here is live on a bare conductor run. The PR description and the test docstring both describe it as verbose-only and rank it as the safer of the two sinks, which is backwards.
Fixing two of five sites is a reasonable scope call, but the gap should be visible. Either escape at these three sites too, or add them as xfail(strict=True) against a follow-up issue so a green suite doesn't read as "#382 is closed".
| # and kill the run (#382). ``title`` is conductor-controlled, so it keeps | ||
| # its styling. | ||
| _verbose_console.print( | ||
| Panel(escape(content), title=f"[cyan]{title}[/cyan]", border_style="dim") |
There was a problem hiding this comment.
Text(content) would be a better fit here than escape().
escape() does not round-trip input that already contains a backslash before a bracket:
in : 'pattern: \[0-9\]+'
out: 'pattern: [0-9\]+'
Low impact on its own, but Text is byte-exact, it is what the other 13 call sites in this module already do, and it removes the need to explain why escaping is safe. The [cyan] title keeps its styling either way. The escape import at L23 could then go.
| Panel(escape(content), title=f"[cyan]{title}[/cyan]", border_style="dim") | |
| Panel(Text(content), title=f"[cyan]{title}[/cyan]", border_style="dim") |
| ) | ||
| # File always gets full untruncated content | ||
| if _file_console is not None: |
There was a problem hiding this comment.
The file path deliberately does not escape, and that asymmetry is load-bearing but unexplained.
Escaping into a markup=False console writes the backslash out literally, which I confirmed:
| fmt: {p}/{t}\[/{nestedType}...]/read
Sitting eight lines under a comment explaining why content must be escaped, this reads like an oversight, and the obvious "consistency fix" would corrupt every log file. One line prevents that.
| if_file_consoleisnotNone: | |
| if_file_consoleisnotNone: | |
| # Not escaped: ``_file_console`` has ``markup=False``, so escaping here | |
| # would write literal backslashes into the log. |
| MARKUP_TRIGGERS = [ | ||
| "Permission format: {provider}/{type}[/{nestedType}...]/read", | ||
| "[/nestedType]", | ||
| "[/]", | ||
| "[/bold]", | ||
| "text [/foo] more", | ||
| ] |
There was a problem hiding this comment.
Every trigger here is a closing-tag form, so the suite only pins the loud failure.
Opening tags are the quiet one. [bold] and [red] never raise; rich consumes them and the text is gone. Your fix handles this correctly already, which is worth locking in: on main the tags are dropped, and with both changes they survive.
markup=False : [bold] preserved=True
escape() : [bold] preserved=True
no fix : [bold] preserved=False
Without a case for it, a future narrowing of the escaper to a closing-tag-only regex would pass all 15 tests while silently dropping agent text, and nothing would ever raise to say so.
| MARKUP_TRIGGERS= [ | |
| "Permission format: {provider}/{type}[/{nestedType}...]/read", | |
| "[/nestedType]", | |
| "[/]", | |
| "[/bold]", | |
| "text [/foo] more", | |
| ] | |
| MARKUP_TRIGGERS= [ | |
| "Permission format: {provider}/{type}[/{nestedType}...]/read", | |
| "[/nestedType]", | |
| "[/]", | |
| "[/bold]", | |
| "text [/foo] more", | |
| ] | |
| # Opening tags do not raise -- rich consumes them silently. A crash-only suite | |
| # cannot tell a correct fix from one that quietly drops agent text. | |
| OPENING_TAG_TRIGGERS= [ | |
| "Use [bold] to emphasise and [red] for errors", | |
| "Config key [section] then [other]", | |
| ] |
| assert console is not None | ||
| assert console.no_color is True | ||
| # The property that prevents #382. | ||
| assert console._markup is False |
There was a problem hiding this comment.
_markup is private and rich is pinned only >=13.0.0, with no public accessor on Console in 14.3.1.
It fails loudly rather than silently if rich renames it, so it is a maintenance cost rather than a hole. A behavioural equivalent survives a rich upgrade and pins the contract that actually matters:
console.print("[bold]not a style[/bold]")
...
assert"[bold]not a style[/bold]"inlog.read_text(encoding="utf-8")| ``[cyan]`` title. | ||
| """ | ||
| try: | ||
| run_module.init_file_logging(tmp_path / "run.log") |
There was a problem hiding this comment.
This test's docstring says it drives the console path, but the init_file_logging call pulls the file sink in with it.
That costs you the isolating signal. Reverting each fix separately:
revert markup=False only : 3 failed, 12 passed
revert escape() only : 1 failed, 14 passed
The one test that catches escape() is this one, and under the first mutation it dies on the file console before it ever reaches its console assertion. So no test isolates escape(), and the "3 of 15 fail" figure in the description holds only when both changes are reverted together.
Dropping this line gives clean 1:1 discrimination. Asserting all five triggers rather than MARKUP_TRIGGERS[0] at L157 costs nothing either.
… log Addresses the review on microsoft#387. Three more sinks in run.py still died on the identical input, because `style=` does not turn markup parsing off -- verified directly rather than assumed: Console(...).print("...[/nestedType]...", style="red dim") -> MarkupError: closing tag '[/nestedType]' ... doesn't match any open tag `verbose_log` (an f-string interpolation) and the two `error_msg` sinks all now pass `rich.text.Text`. The two error sinks matter most: `error_msg` carries a provider exception, so a workflow that is already failing had its real error replaced by a MarkupError. `verbose_mode` and `full_mode` both default to True, so these are reachable on a bare `conductor run` -- the description called this verbose-only and ranked it the safer sink, which was backwards. The `markup=False` comment claimed an invariant the file violated. `_maybe_print_experimental_banner` built a Panel from a markup-bearing string and printed that same object to both consoles, so the log file gained literal `[bold]`/`[dim]`/`[link]` tags. Resolved once with `Text.from_markup`, which renders identically on both sinks. The comment now states the actual consequence instead, and drops the `no_color` claim: `no_color` suppresses colour only, and bold/italic still emit ANSI under `force_terminal=True`. `Panel(escape(content))` -> `Panel(Text(content))`. `escape()` is not byte-exact for input that already contains a backslash before a bracket: `pattern: \[0-9\]+` renders as `pattern: [0-9\]+`. `Text` is exact, matches the other call sites in the module, and removes the need to argue that escaping is safe. The now-unused `escape` import is gone, and the deliberate non-escaping of the file path is documented so the obvious "consistency fix" does not corrupt every log. Tests. Opening tags are the quiet half of microsoft#382 -- rich consumes them without raising and the text is gone -- so they now have their own triggers and a negative control proving the samples really are eaten. The private `console._markup` assertion is replaced by a behavioural one that survives a rich upgrade, since rich is pinned only >=13.0.0 and exposes no accessor. `init_file_logging` is dropped from the console-path test: it pulled the file sink in, so reverting `markup=False` alone killed that test before it reached its console assertion and no test isolated the console fix. It now asserts all five triggers rather than the first. Verified by reverting src and re-running: 6 of the new tests fail without these fixes (five `verbose_log` cases plus the banner). Three pass either way, because they guard the fix this PR already shipped rather than anything added here -- said plainly so the count is not mistaken for more than it is. 554 passed in tests/test_cli (541 without these tests). ruff check unchanged at 2 pre-existing findings, `ty check src` unchanged at 64, my files ruff-format and `ty` clean. CHANGELOG entry added.
Frank Li (franklixuefei)
commented
Aug 10, 2026
Thanks — all three blockers were real, and I verified each at source before changing anything. Addressed in
|
| Outcome | Tests |
|---|---|
| ❌ fail without these fixes | 6 — five verbose_log cases + the banner |
| ✅ pass either way | 3 — they guard the fix this PR already shipped, not anything added here |
Stating the second row plainly rather than quoting "9 new tests", since three of them are not evidence for this commit.
Gates
554 passed in tests/test_cli (541 without the new tests). ruff check unchanged at 2 pre-existing findings; ty check src unchanged at 64; my files ruff format and ty clean. Merged main first this time, so the CHANGELOG entry did not conflict.
One observation worth recording: test_plugin_commands.py::TestFetch::test_a_second_fetch_reports_a_cache_hit failed once in a full-suite run and passed on an identical rerun, in isolation, and on a pristine tree. It looks flaky rather than related, but flagging it rather than quietly re-running until green.
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM. Approved!
Uh oh!
There was an error while loading. Please reload this page.
The "Workflow Inputs" verbose panel serialised inputs with the default ensure_ascii=True, so a Cyrillic, CJK or emoji input value was displayed as \uXXXX escapes rather than the text the user typed. Every other JSON display path in the repo already passes ensure_ascii=False (issue microsoft#356); this was the last one that did not. This branch previously also carried a fix for Rich parsing bracketed prompt text as console markup (microsoft#371). That landed independently on main in microsoft#387, which arrived at the same Panel(Text(content), ...) console fix and additionally set markup=False on the file-log console. The rest of this branch is therefore rebased away as redundant, leaving only the encoding change and its regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(cli): stop parsing runtime data as rich console markup Rich parses `[...]` in a plain str as markup, and conductor interpolated runtime data straight into those strings. A bracketed token is a tag when its first character is lowercase, `#`, `/` or `@`, so `[0]` renders fine, `[task1]` is silently deleted, and `[/etc/x]` raises MarkupError out of the print call. `style=` does not disable parsing. `conductor validate` died with an unhandled traceback on a workflow named `probe [/bold] name`, and printed `probe name` for `[dim]`. Two further consequences were already shipping: every for-each iteration's verbose panel read the same, because the engine qualifies a member's name as `<agent>[<key>]` and a `key_by:` key of `task1` erased exactly the identity that name exists to carry -- while a key starting with `/`, which `key_by:` over paths produces, killed the run from a logging call. That needed no flags; verbose and full mode both default on. `conductor status` (#389) and `conductor plugin list` (#398) were written against the same unfixed pattern in files #387 never touched, and #398 made these strings third-party rather than the author's own YAML. This is the third occurrence: #382 was the original, #387 fixed cli/run.py and still left `title=` in the function it changed. So invert the default rather than escape ~450 call sites. Every console is built by the new `conductor.console.make_console()` with `markup=False`, making a plain string literal unless it asks to be styled -- which fixes 119 sites with no code churn -- and conductor's own styling goes through `styled("<template>", value)`, which parses the template but inserts values verbatim and byte-exact. Panel titles and Prompt prompts are handled separately: rich calls `Text.from_markup` on those unconditionally, so the console setting never reaches them. That is the trap that left #387 incomplete one line from the code it changed. `rich.markup.escape` is dropped everywhere, since it cannot round-trip a value containing a backslash before a bracket. Rendering is byte-identical to main, ANSI codes included, across ten commands: rich highlights a plain str but not a Text, so the console also re-applies the ReprHighlighter to keep this a pure safety change. Five AST guards read src/conductor and fail with file:line when a new call site reintroduces any of these shapes, each with negative controls -- a source scan that quietly matches nothing reports "all clear" forever. The convention is documented in AGENTS.md rather than only in comments. Closes#406 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(cli): restore dropped styling and close the remaining markup gaps Review follow-ups to the markup-safety change. Two real defects. The dry-run plan built its loop-target marker as a `Text` and then interpolated it into an f-string, which renders a `Text` as its plain form -- so `conductor run --dry-run` lost the yellow that distinguishes a loop target from the agent names around it. That is the fourth time this exact shape has shipped in this change, so it is now a guard rule rather than a fixed call site. And the dialog test asserted `str(title)` on a mocked console, which stays true when a regressed f-string title deletes the agent name from the rendered output; it now renders for real and requires the name in both the body and the title. `markup` is no longer overridable per call. Rich lets `print(..., markup=True)` override the instance setting, so one line could reopen both original failure modes -- and it is the obvious-looking fix for the visible `[green]` that forgetting `styled` now produces. The refusal moved onto the class so subclasses inherit it, and `MarkupFreeConsole` is public because `cli/run.py` subclasses it and AGENTS.md documents doing so. `Console.input` forwards `markup=` to `print` unconditionally, so the refusal is of a *truthy* value only and `input` forces it off; otherwise every `Prompt.ask(console=...)` would raise. Typer renders help through its own rich console, so the previous "outside this convention" carve-out was wrong: `[@registry][@Version]` was being deleted from `conductor run/resume/validate/show --help`, and that syntax appears nowhere else in the help output. The strings are escaped and rule G now enforces it. Guard corrections. Rules F (a `Text` in an f-string), G (typer help) and H (`rich.markup.escape`) are new -- the last two were documented as rules while nothing checked them. `", ".join(...)` was being treated as a `Text` producer, and `str | Text` as a Text-bearing annotation, which between them blinded rule B across a whole module; name resolution is now per-function with closure inheritance. Every rule is a shared predicate called by both the source scan and its negative control, which had already drifted apart. `styled` now refuses rather than silently loses: a format spec or conversion on a `Text` flattened away the styling the caller passed a `Text` to keep, and a field inside a tag raised `'\x00' is not in list`, naming an internal detail. Field resolution goes through `Formatter.get_field`, so dotted and indexed fields work. Rendering stays byte-identical to main across ten commands, now including a dry-run with a loop target. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Jason Robert <jasonrobert@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes#382.
The bug
Rich parses
[...]as markup. Agent output is arbitrary text and regularly contains bracketed tokens — code, globs, regexes, ARM/URI patterns — so a closing-tag form in ordinary technical prose raisesMarkupErrorand kills the run.The trigger that found this was a plan document describing an Azure RBAC permission format:
Nothing about that is malformed; it is what the format looks like.
It killed a long-running workflow mid-execution, then killed both resume attempts at the identical byte offset, because the offending text is checkpointed and replayed verbatim. Until I hand-edited the checkpoint the run was permanently unresumable.
It is not verbose-only
Two sinks render agent-supplied content, and only one of them is gated on verbosity:
init_file_loggingis called from a bareif log_file is not None:in bothrunandresume. So--log-filealone is sufficient to arm the crash, including under--silent. Since--log-file autois the natural way to keep a record of a long background run, this is reachable from the configuration you would most want for exactly the runs where losing the work hurts most.The content is the rendered prompt, which embeds user-supplied plan/workflow text — so the crash is driven by user data, not by anything conductor controls.
The change
File console →
markup=False. It is alreadyno_color=Trueplain text, so it has no use for markup interpretation.I checked this is safe rather than assuming it: all 20
_file_console.print(...)call sites pass either a pre-builtrich.text.Text(13 of them) or a plain string.Textis not markup-parsed, so the flag cannot change it. Every markup-bearing print in the module targets_verbose_consoleinstead.Console path →
escape()on the content. That console deliberately keepsmarkup=Truefor conductor's own[cyan]title, so the fix there is to escape the untrusted part rather than de-feature the console.titleis conductor-controlled and keeps its styling.Both were verified against a standalone repro to raise nothing and preserve the literal text byte-for-byte.
Tests
tests/test_cli/test_markup_safety.py, 15 tests. Two things I want to call out because they are what makes them worth having:The samples are pinned as genuine triggers.
test_sample_raises_when_markup_is_enabledasserts each fixture really does raise with markup on. Without that, a sample that happens not to trigger would make its cases pass vacuously — the same trap as the em-dash in #342, which turned out to be encodable in cp1252 and would have been a dud.The production consoles are asserted, not just hand-rolled ones. The behavioural tests construct their own
Consoleand would keep passing if someone droppedmarkup=Falsefrominit_file_logging. So there are assertions against the real_file_consoleand a test that drivesverbose_log_sectionend-to-end through both sinks.Verified by reverting both fixes: 3 of 15 fail, and all 15 pass on the fixed tree.
Minimal triggers covered, all confirmed raising without the fix:
{provider}/{type}[/{nestedType}...]/read[/nestedType]doesn't match any open tag[/]has nothing to close[/bold]doesn't match any open tagtext [/foo] moreValidation
tests/test_cli+tests/test_webmatches the pre-existing baseline exactly — 48 failures before and after, verified by stashing the change and re-running. (My sandbox has no network access to PyPI sopytest-asynciois unavailable and those async tests error identically either way; I diffed the failure sets rather than trusting the count.)ruff check src testsandruff format --check src testsare clean.Not in this PR
Panel(...)/print(...)fed agent- or user-derived text has the same shape. I fixed the two sinks I could demonstrate; a sweep is worth doing separately rather than bundled here.Workaround, for anyone hitting this before it lands
The offending text is in the checkpoint, so a plain
resumere-crashes at the same offset. Rewrite the bracket form in the checkpoint JSON ([/x]→(/x)) and then resume.Environment
Not Windows-specific — nothing in the parse path is platform-dependent — but I have only reproduced it there.
Related
Same run, different boundaries: #342 / #381 (
UnicodeEncodeErrorat the byte-write boundary) and #344 / #383 (conductor stoporphaning runs). Three separate failures, three separate fixes; this one is a parse failure and neither of the others prevents it.