fix(witan): escape stored text at the render boundary; route session/project/trace show through the tool surface - #271
Open
blarghmatey wants to merge 2 commits into
Conversation
… read commands reaching past the tool surface Two CLI defects, both from a call site doing its own thing where a shared boundary already existed. RENDERING. Rich reads `[...]` in `Console.print` as a style tag, so stored content printed straight into a markup string loses every bracketed substring. Closing a task printed its resolution back as "code_transport is not set on , so code graphs remain local" — `[targets.production]`, the part naming which target was misconfigured, was the part that went. `task_get` showed the stored text intact, so this was display-only throughout. Rich drops an unresolvable style silently; `[rank]`, `[targets.production]` and a markdown `[link]` all just vanish. A bracketed absolute path is worse: `[/var/lib/witan]` parses as a closing tag with nothing open and raises MarkupError, taking the command down. #261 escaped the four `witan serve` startup sites it had just written, one call site at a time, and every other renderer stayed broken — which is the argument for fixing it at the boundary instead: `render_table` escapes per cell, and `esc()`/`print_error()` cover the line-oriented renderers. `witan task show`, `task close`, `project show`, `project status`, `trace show`, `session list`, `migrate`, `whoami` and the pickers print stored text whole again. Dry-run prompt output uses `markup=False` rather than escaping — it exists to show the exact text the agent will receive. REMOTE READS. `witan session list <project>` called `s.client.read("read.gq", …)` directly. That attribute exists only on the in-process server module, so against ci/qa/production the command raised `AttributeError: 'function' object has no attribute 'read'` as a traceback — and it is the only way to inspect a shared deployment's session state, so it failed exactly where it was needed. Found while checking session state during a production migration. `witan project show` and `witan trace show` reached past the tool surface the same way and were equally local-only. All three now dispatch through `workflow_session_list` / `workflow_project_get` / `workflow_trace_get`. A test walks the CLI's AST for `.client.read`, so a fourth one is a test failure rather than a crash on a deployed target, and `CLIENT_READ_ATTRS` is down to `graph_uri` — `witan migrate storage` is the only reach-past left, and migrate commands are in-cluster admin operations anyway (ADR-0005). `workflow_session_list` gains `include_superseded` (default False, so every aggregate caller is unchanged). `witan session list` is the view that shows what `witan migrate dedupe-sessions` did, so it is the one caller that wants the rows the tool otherwise drops; without the parameter the swap would have silently deleted that view. It does mean `witan session list` needs a cluster running 0.21.0 — against an older deployment it fails on the unknown argument instead of the AttributeError. 904 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXkw58FkNQuFUxbab2B5NH
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Witan CLI rendering of bracketed content and enables project, session, and trace reads against remote deployments.
Changes:
- Escapes dynamic Rich-rendered content and errors.
- Routes CLI reads through MCP tools, adding superseded-session support.
- Adds regression tests and releases witan-council 0.21.0.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Updates locked Witan version. |
mcp/servers/witan/witan/server.py |
Adds superseded-session inclusion. |
mcp/servers/witan/witan/cli/traces.py |
Routes and escapes trace output. |
mcp/servers/witan/witan/cli/tasks.py |
Escapes task output. |
mcp/servers/witan/witan/cli/targets.py |
Uses shared error rendering. |
mcp/servers/witan/witan/cli/session.py |
Routes remote session listing. |
mcp/servers/witan/witan/cli/run_helpers.py |
Adjusts prompt rendering. |
mcp/servers/witan/witan/cli/projects.py |
Routes and escapes project output. |
mcp/servers/witan/witan/cli/migrate.py |
Escapes migration output. |
mcp/servers/witan/witan/cli/local_dispatch.py |
Narrows direct client access. |
mcp/servers/witan/witan/cli/auth.py |
Escapes authentication output. |
mcp/servers/witan/witan/cli/_common.py |
Centralizes escaping and errors. |
mcp/servers/witan/witan/cli/__init__.py |
Applies shared error rendering. |
mcp/servers/witan/tests/test_local_dispatch.py |
Updates dispatch expectations. |
mcp/servers/witan/tests/test_cli_remote_reads.py |
Tests remote CLI reads. |
mcp/servers/witan/tests/test_cli_markup.py |
Tests Rich markup preservation. |
mcp/servers/witan/pyproject.toml |
Bumps version to 0.21.0. |
mcp/servers/witan/CHANGELOG.md |
Documents the release. |
Suppressed comments (1)
mcp/servers/witan/witan/cli/run_helpers.py:137
markup=Falsestill leaves Rich's emoji-code replacement enabled. A task title or description containing literal text such as:warning:is transformed in the dry-run display even though the agent receives the original characters. Disable emoji processing here too so the preview remains exact.
console.print(prompt, markup=False)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ape the remaining interpolations From Copilot's review of #271. `markup=False` is only half of "show the exact text the agent will receive": Rich substitutes emoji codes independently of markup, so a prompt saying `:warning:` was displayed as ⚠ while the agent received the eight literal characters — the same class of lie the escaping fixes. Both dry-run prints now pass `emoji=False, highlight=False` as well, pinned by a test asserting the rendered output equals the prompt byte for byte. `project show`'s trace line interpolated `tr.get('phases')` as a raw list; it now joins and escapes, matching how `trace show` renders the same field. `auth.py` escapes the endpoint URL, the IdP's verification URI and the device code — every value there comes from outside witan. Two of the three findings do not reproduce as described, and the fixes are for the reason given below rather than the one reported: - A Python list repr survives Rich intact (`phases=['implementation']` prints unchanged) because the quotes make it an invalid tag; it is the UNQUOTED form (`[implementation]`) that gets eaten. Joined anyway: the raw repr was a worse rendering than the neighbouring line's, and one rule beats a per-value argument about which shapes parse. - An IPv6 endpoint is also safe as-is — `https://[::1]:8000/mcp` renders whole, since a colon cannot appear in a Rich tag. Escaped anyway, for the same one-rule reason. 905 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RXkw58FkNQuFUxbab2B5NH
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
No GitHub issue. Tracked in the witan graph:
tk-witan-cli-renderers-swallow-bracketed-text-as-ri-300035tk-witan-session-list-project-crashes-against-any-r-e15a0eDescription (What does it do?)
[...]inConsole.printas a style tag, sowitan task closeprinted its own resolution back ascode_transport is not set on , so code graphs remain local—[targets.production], the identifier naming which target was misconfigured, was the part removed.task_getshows the stored text intact, so this was display-only throughout.render_table(per cell) and inesc()/print_error()for the line-oriented renderers, not per call site. fix(witan): serve the deployed graph instead of silently opening the local store #261 escaped the fourwitan servestartup sites it had just written, one call site at a time, and every other renderer stayed broken; that is the argument for the boundary.task show,task close,project show,project status,trace show,session list,migrate,whoamiand the run pickers are covered.markup=Falseinstead of escaping — it exists to show the exact text the agent will receive.witan session list <project>works against a remote target. It calleds.client.read("read.gq", …), which exists only on the in-process server module, so against ci/qa/production it raisedAttributeError: 'function' object has no attribute 'read'as a traceback. It is the only way to inspect a shared deployment's session state, so it failed exactly where it was needed (found while checking session state during a production migration).witan project showandwitan trace showreached past the tool surface the same way and were equally local-only. All three now go throughworkflow_session_list/workflow_project_get/workflow_trace_get, andCLIENT_READ_ATTRSdrops tograph_uri(witan migrate storageis the only reach-past left, and migrate commands are in-cluster admin operations per ADR-0005).workflow_session_listgainsinclude_superseded(defaultFalse, so aggregate callers are unchanged).witan session listis the view that shows whatwitan migrate dedupe-sessionsdid, so it is the one caller that wants the rows the tool otherwise drops; without the parameter the swap would have silently removed that view.How can this be tested?
Automated (
just test-witan-council): 904 pass. New coverage:tests/test_cli_markup.pyrenders through a realrich.Console(capturingprint's arguments passes even while the user sees the hole) and asserts[targets.production]survivesesc,print_error, a table cell, andwitan task show's description and resolution; plus that a column's own styles still apply after escaping.tests/test_cli_remote_reads.pydrives a realRemoteServerProxyover a scripted client — the defect lived in the seam, and a stub with aclientattribute would pass against the crashing code. Coverssession list,project show,trace show, and thatsession liststill asks for superseded rows. A final test walks the CLI's AST for.client.readso a fourth instance is a test failure rather than a crash on a deployed target.tests/test_local_dispatch.py's facade test now pinsgraph_urionly.Rich's behaviour was checked rather than assumed: an unresolvable style is dropped silently (
[rank],[targets.production], a markdown[link]all vanish), while a bracketed absolute path ([/var/lib/witan]) parses as a closing tag and raisesMarkupError, killing the command. Both cases are asserted.Manually: not exercised against a live deployment. The remote path is covered by the scripted-proxy tests only.
Additional Context
Deployment ordering: the CLI passes
include_superseded, sowitan session listneeds a cluster running witan-council 0.21.0. Against an older deployment it fails on the unknown argument rather than theAttributeErrorit fails with today. Every other command in this PR is client-side only.