Skip to content

fix(cli): render verbose log content literally instead of as Rich markup - #403

Closed
Jason Robert (jrob5756) wants to merge 1 commit into
mainfrom
fix/verbose-log-section-markup-error
Closed

fix(cli): render verbose log content literally instead of as Rich markup#403
Jason Robert (jrob5756) wants to merge 1 commit into
mainfrom
fix/verbose-log-section-markup-error

Conversation

@jrob5756

Copy link
Copy Markdown
Collaborator

Fixes#402.

The bug

verbose_log_section passed its content straight to Panel, and verbose_log interpolated its message into f"[{style}]{message}[/{style}]". Rich parses a plain str as console markup, so any square-bracket tag in that text was interpreted rather than displayed.

That content is untrusted — verbose_log_section is called with the rendered prompt (executor/agent.py:435), which carries agent output, plan text and tool arguments verbatim. An agent that writes [/bold] in a code span produces an unbalanced closing tag, Rich raises MarkupError, and the exception propagates out of the log call and kills the workflow.

It killed a real run at iteration 45, after seven epics had been committed. The trigger is worth reading twice: a review agent had filed a finding about unescaped markup crashing a Textual screen, quoting [/bold] as the example. Rendering the prompt that carried that finding to the next agent raised the exact error the finding described.

The file-console path makes this more than a verbosity-flag bug — file logging is on for every --web-bg run regardless of console verbosity, so background runs are exposed even with --quiet.

The fix

Wrap both in rich.text.Text, which renders literally. Text takes a style directly, so the console output keeps its formatting and the file output is unchanged apart from no longer being parsed.

Tests

Three regression tests in tests/test_cli/test_logging.py, each asserting the literal tag survives to the output:

  • test_verbose_log_section_rejects_markup_interpretation — console path
  • test_verbose_log_section_file_output_rejects_markup — file path, reached with console verbosity off (the --web-bg case)
  • test_verbose_log_rejects_markup_interpretationverbose_log

Verified they fail on main with MarkupError and pass with the fix.

Verification

  • make check — ruff check, ruff format, ty: all pass
  • uv run pytest tests/test_cli/ — 529 passed, 3 skipped

Note on scope

I fixed verbose_log alongside verbose_log_section even though only the latter caused the crash: it is the same one-line flaw in the adjacent function in the same file, and its callers interpolate plugin warnings and MCP server names that conductor does not control either. Happy to split it out if you would rather keep the change to the reproduced path.

rich.markup.escape is already used correctly in cli/doctor.py and gates/interrupt.py, so these two call sites look like they simply predate that pattern. #402 suggests a broader audit as follow-up.

`verbose_log_section` passed its content straight to `Panel`, and
`verbose_log` interpolated its message into `f"[{style}]{message}[/{style}]"`.
Rich parses a bare `str` as console markup, so any square-bracket tag in that
text was interpreted rather than displayed.
The content is untrusted. Rendered prompts carry agent output, plan text and
tool arguments verbatim, so an agent that merely writes `[/bold]` in a code
span produces an unbalanced closing tag and Rich raises MarkupError — which
takes down the whole workflow, not just the log line.
This is not hypothetical: a review agent filed a finding about unescaped
markup crashing a Textual screen, quoting `[/bold]` as the trigger. Rendering
the prompt that carried that finding to the next agent killed the run at
iteration 45, with the error the finding described.
The file-console path makes it worse than a verbosity-only bug: file logging
is enabled for every `--web-bg` run regardless of console verbosity, so a
background run is exposed even with `--quiet`.
Wrap both in `rich.text.Text`, which renders literally. The console keeps its
style, since `Text` accepts one directly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jrob5756

Copy link
Copy Markdown
CollaboratorAuthor

Closing this as superseded by #387, which landed in a58c88f while this branch was open.

#387 fixes the same defect in both functions using the same rich.text.Text mechanism, and goes further:

  • markup=False on _file_console in init_file_logging, which protects the whole file sink structurally rather than at each call site
  • the two error_msg sinks in verbose_log_parallel_agent_failed and verbose_log_for_each_item_failed, where a provider exception was replacing the real error with a MarkupError
  • _maybe_print_experimental_banner, resolved once via Text.from_markup so the panel renders correctly on both sinks
  • tests/test_cli/test_markup_safety.py, which covers opening tags and closing tags with negative controls proving the samples are genuine triggers

Worth recording why this was not obvious: this branch was cut before #387 merged, so the PR diff GitHub shows (and git diff main...branch) compares against the merge base and looks like a clean fix. Diffing against current main instead shows this branch would revert all four of the changes above, including removing markup=False and restoring the two crashing error sinks. Rebasing would leave nothing behind, so there is nothing to salvage here.

One finding from reviewing this does survive on main and is not covered by #387: the panel title is still interpolated into markup at src/conductor/cli/run.py:357, and it is not conductor-controlled the way the comment above it claims. Filed separately.

@jrob5756

Copy link
Copy Markdown
CollaboratorAuthor

The residual title finding referenced above is now #406, filed as the wider CLI markup issue rather than a run.py-only fix, since conductor validate turned out to crash outright on the same input and conductor status corrupts its listing.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MarkupError in verbose logging kills the workflow when agent output contains a Rich tag like [/bold]

1 participant

@jrob5756