Skip to content

fix(cli): stop parsing runtime data as rich console markup - #408

Merged
Jason Robert (jrob5756) merged 2 commits into
mainfrom
fix/406-rich-markup-injection
Aug 11, 2026
Merged

fix(cli): stop parsing runtime data as rich console markup#408
Jason Robert (jrob5756) merged 2 commits into
mainfrom
fix/406-rich-markup-injection

Conversation

@jrob5756

Copy link
Copy Markdown
Collaborator

Closes#406.

The bug

Rich parses [...] in a plain str as console markup, and Conductor interpolated runtime data — workflow names, agent names, plugin metadata read out of git-cloned repos, str(e), gate payloads — straight into those strings. A bracketed token is a tag when its first character is lowercase, #, / or @, so the behaviour splits three ways and only one is loud:

first charexampleresult
digit / uppercase[0], [KPI_3]renders literally
lowercase, #, @[task1], [#42]silently deleted
/[/bold], [/etc/x]MarkupError, out of the print call

The quiet half matters more: a listing that drops part of a name looks like it worked. style= does not disable parsing.

What was actually broken

Four sites, all reproduced against main before the fix:

  1. conductor validate crashed on workflow.name: "probe [/bold] name", and printed probe name for [dim].

  2. Every for-each iteration's verbose panel read the same. The engine qualifies a member's name as <agent>[<key>] so interleaved output can be attributed to one iteration; key_by: keys are agent-derived, so:

    key='0' -> Prompt for 'review[0]' ok
    key='task1' -> Prompt for 'review' IDENTITY ERASED
    key='docs/a.md' -> Prompt for 'review' IDENTITY ERASED
    key='/etc/x' -> MarkupError RUN DIES
    

    This needed no flags — verbose and full mode both default on. The comment directly above it claimed "title is conductor-controlled, so it keeps its styling", which was false.

  3. conductor status (feat(cli): add a read-only \conductor status\ command #389) corrupted its listing — the name a user reads to pick a port for conductor stop.

  4. gates/dialog.py built a Panel title from the same for-each qualified name. Not in the issue; found while working.

This is the third occurrence. #382 was the original; #387 fixed cli/run.py thoroughly and still left title= in the very function it changed; #389 and #398 then reintroduced the pattern next door, in files #387 never touched. #398 also made these strings third-party rather than the author's own YAML.

Approach

A per-site escape pass across ~450 literals demonstrably does not hold, so the default is inverted instead.

New leaf module src/conductor/console.py — top-level rather than under cli/, because gates/ and providers/ need it and must not import from cli/:

  • make_console() locks markup=False. A plain string is now literal unless it asks to be styled, which fixes 119 sites with zero code churn and covers plain prints, Panel bodies, Table cells/headers/titles/captions and Rule titles. It rejects a markup= kwarg rather than allowing an override.
  • styled("<template>", value) parses conductor's own template but inserts values verbatim and byte-exact. It replaces each field with a length-matched filler run, parses once, then substitutes by offset — matching the width is what keeps the parsed template's spans valid, so nested styling around a placeholder ([bold][red]{}[/red][/bold]) is correct. Reading spans[0].style instead would collapse it. A value that is already a Text splices in with its own spans re-anchored.
  • join(sep, parts)Text.join requires every part to already be a Text, but the common shape here is a content_lines list mixing styled fragments with plain runtime values.

Panel(title=)/(subtitle=) and Prompt/Confirm/IntPrompt are handled separately. Rich calls Text.from_markup on those unconditionally (rich/panel.py:113,129, rich/prompt.py:67), so markup=False never reaches them. Measured, not assumed — and it is exactly the trap that left #387 incomplete one line from the code it changed.

rich.markup.escape is dropped everywhere. It is not byte-exact: the parser treats \[ as an escaped bracket, so an ordinary regex \[0-9\]+ renders as [0-9\]+ whether escaped first or not.

Typer's help=/epilog= are deliberately left alone — Typer renders those through its own console.

Rendering is byte-identical to main

Verified across ten commands, ANSI codes included. That took one extra step worth calling out: rich applies its ReprHighlighter to a plain str but not to a Text, so the conversion would otherwise have silently de-coloured every number, path and quoted value across the whole CLI — a cosmetic regression bundled into a safety fix. _MarkupFreeConsole.print re-applies the highlighter in rich's own order (highlight first, conductor's spans copied on top).

Keeping it fixed

tests/test_cli/test_markup_guards.py reads src/conductor with ast and fails with file:line:

  • A — every Console goes through make_console (or explicitly passes markup=False)
  • B — no interpolated string to a Panel title/subtitle or a Prompt
  • C — no f-string to Text.from_markup
  • D — no bare markup literal at a print/cell sink (this is what makes the conversion verifiable rather than eyeballed)
  • E — no Rich Text through the builtinprint, which renders its plain form and drops anything rich parsed as a tag

Each rule has negative controls, because a source scan that quietly matches nothing reports "all clear" forever. _safe_text_names does small intra-function dataflow so a genuinely Text-typed local is not pushed onto an allowlist. I verified the guards fail when the original bug is reintroduced.

The convention is written up in AGENTS.md (new Console Output section under Code Style, plus an architecture entry), not only in code comments.

Review notes

A code review of this branch caught two regressions I had introduced, both now fixed and both covered by new tests:

  • the codemod wrapped a stderr log label in Text.from_markup, and because [workspace-instructions] starts with a lowercase letter rich deleted the prefix. The existing test passed, because it asserted on a substring after it. Rule E exists because of this.
  • f"{mark} ..." flattened a Text and dropped the ✓/⚠ colour from conductor plugin fetch — the marker column on a command CI gates on.

One styled() hardening also came out of it: a NUL in the template is now rejected up front, since it would capture the search that locates each value.

Validation

  • make check (ruff lint + format + ty) clean
  • 5766 tests pass, 47 skipped (+88 new)
  • 44 example workflows validate
  • All four repros verified fixed; conversion audit at zero
  • Rendered output byte-identical to main across ten commands

Jason Robertand others added 2 commits August 11, 2026 11:50
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>
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>
@jrob5756
Jason Robert (jrob5756) marked this pull request as ready for review August 11, 2026 18:14
@jrob5756
Jason Robert (jrob5756) merged commit c017ec7 into mainAug 11, 2026
9 checks passed
@jrob5756
Jason Robert (jrob5756) deleted the fix/406-rich-markup-injection branch August 11, 2026 18:15
Jason Robert (jrob5756) pushed a commit to franklixuefei/conductor that referenced this pull request Aug 12, 2026
`test_plugin_path_survives_the_report` interpolated `tmp_path` straight
into a YAML *double-quoted* scalar. Those process backslash escapes, so
on Windows the runner's `C:\Users\runneradmin\...` reached the parser as
`\U` -- a Unicode escape -- and the fixture failed with "expected escape
sequence of 8 hexdecimal numbers, but found 's'" before the assertion
under test ever ran.
Same class as the `_format_toml` bug this branch already fixed, one
layer out: a value with meaning to the serialiser, interpolated as if it
were inert.
YAML is a superset of JSON, so `json.dumps` produces a scalar that is
valid on every platform and round-trips the path exactly. Verified
against the literal path from the failing CI run: the old form raises
ScannerError, the new form round-trips.
Arrived with microsoft#408, so it is not a regression from this branch -- but it
is only visible because of the Windows leg this branch adds, and the
suite has to be green for that leg to mean anything.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756) pushed a commit that referenced this pull request Aug 12, 2026
* ci: run the test suite on Windows as well as Linux
CI ran ubuntu-latest only. Conductor has a substantial amount of Windows-
specific code that nothing executed: cli/pid.py is an entire
OpenProcess/GetExitCodeProcess ctypes implementation, cli/bg_runner.py handles
CREATE_NEW_PROCESS_GROUP, DETACHED_PROCESS and job-object breakaway, and there
are sys.platform branches in cli/app.py and cli/update.py.
Three bugs found in one week of Windows use all reached a release: #342 (JSON
truncation via a WINDOWS-only chunked-write path), #344 (conductor stop
orphaning runs because CTRL_BREAK_EVENT needs a shared console) and #382
(MarkupError through the console path).
The gap is bidirectional, which is the part worth stressing. While fixing #344 I
wrote a test that patched conductor.cli.app.os.kill; because os is one shared
module object, on POSIX that also patched pid.py's liveness probe and turned
DEAD into UNKNOWN. It passed on Windows and would have failed on Ubuntu. In the
other direction, the Windows liveness logic is covered everywhere by patching a
_kernel32 mock -- good design, but it means no job anywhere validates the real
ctypes argtypes/restype, so an ABI mistake would be CI-green and fail only for
users.
Deliberately ONE Python version on Windows rather than a full 2x2: the platform
is the variable, not the interpreter, and the install-scripts job already
provisions a Windows runner on every PR, so this is incremental cost rather than
a new platform commitment.
Two things this required beyond the matrix:
- The 'Remove bundled Copilot CLI binary' step used ind, which is not a
native Windows command. It now runs under shell: bash (Git Bash is
preinstalled on the runner) and also matches the Windows venv layout, where
scripts live under Scripts/ and the binary carries a .exe suffix.
- Coverage upload is now gated on ubuntu-latest as well as 3.12, so a second
runner cannot upload a partial report for the same commit and skew the totals.
I expect the first Windows run to surface failures rather than be green --
tests that fake a platform via sys.platform, and any assertions on POSIX-shaped
paths or rendered console output, are the likely candidates. Better to see them
than to keep not knowing.
Fixes#385
* fix(registry): escape TOML strings, and make the suite pass on Windows
The Windows CI job added in the previous commit found a real product defect on its
first run, alongside 29 test-level POSIX assumptions.
Product fix — `_format_toml` built TOML by interpolating values straight into *basic*
strings, which process backslash escapes. A `path` registry whose source is a Windows
path (`C:\Users\alice\...`) therefore serialised `\U`, which TOML reads as a Unicode
escape and rejects with "Invalid hex value" — so `conductor registry add` wrote a
`registries.toml` that could never be read back. Not Windows-only: any source
containing `\` or `"` corrupted the file on every platform. Values are now escaped per
the TOML v1.0 spec. This single fix cleared all 10 `test_registry/test_integration.py`
failures.
Regression coverage: the existing round-trip test used `/dev/workflows`, a POSIX path
with nothing to escape, which is why this was never caught. Added a round-trip over
backslashes plus a parametrised case for each TOML metacharacter — `\U` (invalid hex),
`\t`/`\n` (silent corruption rather than an error), `\x`, an embedded quote, and a
trailing backslash. Verified they fail without the fix: reverting `_toml_escape`
turns all 6 red and nothing else.
Test-level fixes, by cause:
- POSIX permission bits (`test_manager_truncation`) — `stat.S_IMODE` reports 0o666/0o777
on Windows whatever mode was requested. The mode assertions are now POSIX-guarded;
the surrounding behaviour still runs everywhere.
- `test_spill_os_error_falls_back_to_marker_without_path` simulated failure with
`mkdir(mode=0o500)`, a no-op on Windows, so the spill silently succeeded and produced
the wrong marker. It now patches `os.open` to raise, exercising the same
`except (OSError, ValueError)` branch deterministically on every platform — better
coverage than the permission side-effect it replaces.
- Path separators — `.copilot/skills`, `.hermes/profiles/chloe` and the spill marker
were compared against literal forward slashes. The discovery warning embeds `repr()`
of the search list, which doubles backslashes on Windows, so that assertion accounts
for both forms rather than being weakened.
- `Path("/plug")` and `Path("/skills/acme")` are absolute on POSIX but not on Windows,
which needs a drive. Anchored to the current drive root via a helper.
- `~` expansion — Windows resolves `~` from `USERPROFILE`, not `HOME`, so setting `HOME`
alone left `expanduser()` pointing at the real profile.
- Line endings — the child's text-mode stdout emits `\r\n`; asserted via `splitlines()`.
- `chmod(0o000)` blocks neither a Windows owner nor root; those tests now skip on
Windows using the same idiom already present in `test_discovery.py`.
- `Skill.md` vs `SKILL.md` cannot express a mis-cased filename on a case-insensitive
filesystem, so that scenario skips on Windows.
Verified on Windows: 4,717 passed. The 3 remaining failures (`test_aca_runner`,
`test_agent_iteration_limits`) reproduce identically with this commit's production
change reverted, so they are pre-existing and environmental, not introduced here.
`ruff check` and `ruff format --check` are clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ba2dfe2e-2c85-4d70-88f9-a604a700a0c4
* fix(providers): detect the bundled Claude CLI on Windows, and stop probing POSIX paths there
`validate_connection` carries a comment saying to keep it in sync with the SDK's
`_find_cli`. It had drifted, in two ways — both surfaced by the new Windows CI job.
1. The bundled-CLI probe only looked for `_bundled/claude`. The SDK names that binary
per-platform (`claude.exe` on Windows, `claude` elsewhere — see `_find_bundled_cli`),
so on Windows conductor reported "Claude CLI not found" even with the bundled binary
present and usable. Also probes `claude.exe` via `shutil.which`, since PATH may hold
npm's `claude.cmd` shim while the native executable lives elsewhere.
2. The hardcoded fallback list probed POSIX paths on every platform. `Path("/usr/local/bin/claude")`
is rooted but driveless, so on Windows it resolves against the current drive —
`C:\usr\local\bin\claude` — a location any unprivileged local user can create. A planted
file there would make conductor report the CLI as available. The SDK refuses these on
Windows for exactly this reason, describing it as a binary-planting probe; conductor now
matches, probing only `~/.local/bin/claude.exe` there.
The remaining test fixes follow the same patterns as the previous commit: POSIX mode
assertions guarded (`S_IMODE` reports 0o666 on Windows whatever mode was requested), and
the explicit-spill-dir assertion compares against the *resolved* directory, since the
manager resolves before writing and `resolve()` also expands 8.3 short components on
Windows.
Windows CI went 30 failures -> 4 with the previous commit; these are those 4.
Verified locally on Windows: 4,716 passed. The 3 pre-existing failures (`test_aca_runner`
x2, `test_agent_iteration_limits`) reproduce with the production change reverted;
`test_performance` is `pytestmark = pytest.mark.performance` and is deselected in CI.
`ruff check` and `ruff format --check` clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ba2dfe2e-2c85-4d70-88f9-a604a700a0c4
* fix(tests): match the JSON-escaped spill path on Windows
result.output is a JSON document, so a Windows path inside it carries JSON-escaped
backslashes (custom-spill\\\\mcp-...). Comparing the raw path could therefore never
match on Windows. Accept either encoding rather than weakening the assertion to a
loose substring.
Last of the 30 Windows failures: the job now runs 5,106 tests green.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ba2dfe2e-2c85-4d70-88f9-a604a700a0c4
* fix: correct the Windows CLI probe and stop unloadable registry names
Addresses the review on #390.
The SDK claim was wrong, and I verified it against the wheels rather than
the changelog. claude-agent-sdk 0.2.87 -- the version uv.lock pins and CI
installs -- has no platform branch in _find_cli at all: it probes all six
POSIX paths on every OS, Windows included. The comment told a future
maintainer the hole was closed upstream when at the pinned version it is not.
Worse, the narrowing caused a false negative. Only "/usr/local/bin/claude"
is driveless; the other five are Path.home()-anchored, so dropping them made
validate_connection() return False on Windows for a CLI at
~/.claude/local/claude -- where Claude Code's own local installer puts it,
and which the SDK would find and run. The user saw "Failed to connect ...
check your credentials" plus advice to npm-install a CLI they already had.
Now only the driveless entry is skipped, the comment names the audited
version, and it says plainly that this narrows conductor's report only.
Dropped the claude.exe branch. shutil.which applies PATHEXT on Windows, so a
claude.exe on PATH is already found by the line above and the branch was
dead. 0.2.134 makes the equivalent probe reachable by vetting the hit first;
there is nothing to port from 0.2.87.
platform.system() -> sys.platform == "win32", the house idiom (11 other
sites; cli/pid.py calls it out). That is what makes the branch testable from
Linux, since the repo fakes Windows by patching sys.platform and nothing
patches platform.system.
Registry names could still corrupt the config. Escaping the values left the
table key unescaped and the charset unchecked, so 'has"quote', 'with space',
'a=b' and 'a#b' all wrote successfully and then failed to parse -- and
because add_registry, remove_registry and get_registry all load first, the
user could not remove the entry that broke it and unrelated registries went
down with it. Names are now restricted to [A-Za-z0-9._-], which also keeps
them legal as cache directory names on Windows.
The charset check alone was not enough: the key is written unquoted, so
"my.team" -- which the suggested regex allows -- became [registries.my.team],
a nested table, and the entry silently vanished on load rather than failing
loudly. The key is quoted now as well, and a round-trip test pins it.
Tests. The Windows branch had none: on Linux is_windows is always False, and
on the Windows runner the bundled claude.exe short-circuits before the
fallbacks, so the block ran on no CI leg and could have been deleted with all
three staying green. It now asserts the probe *set* per platform, which is
what forces ~/.claude/local/claude to be written down as a literal
expectation. Registry names get charset and round-trip coverage.
CHANGELOG entries added for both production fixes; the PR had none.
2,152 passed across test_registry, test_providers, test_cli and
test_executor. ruff check and `ty check src` both unchanged from this
branch's baseline (2 and 68).
* fix(plugins): keep file:// checkouts inside the plugin cache on Windows
The Windows CI leg this PR adds went red on 7 tests. All of them are real
platform bugs in \conductor\, not test artefacts, so they are fixed here
rather than skipped -- a CI leg that has to be read past is worse than none.
Three defects, all reachable only from a \ile://\ source:
1. The cache key is built from the URL path split on \/\, so a native
Windows path arrived as one segment with its backslashes intact. The key
kept them and the checkout landed at a drive-absolute location outside
the cache root -- the escape the \..\ check exists to prevent, through a
door it does not cover.
2. A drive colon survived into a segment, so an owner of \C:_src\ read as a
drive, or mid-name as an NTFS alternate data stream.
3. Flattening a deep path into a single owner segment produced a name long
enough that \git\ could not create \.git\ inside it (260-char cap).
Fixed by folding separators for \ile://\, substituting the characters that
change a path's meaning on Windows, and replacing an over-long segment with
a digest of itself. Applied on every platform: one workflow file must
resolve to the same cache layout wherever it runs, and a Windows-gated
branch would never execute on the Linux legs.
Separately, \_publish\ tolerated losing a race to a concurrent fetch only
for the POSIX errnos. Windows reports a non-empty destination directory as
ERROR_ACCESS_DENIED, so the tolerance never applied and a second source
resolving to the same SHA failed outright. Safe to accept because the
readiness sentinel is written after publishing, so a winner that died
mid-clone leaves no sentinel and the tree is re-fetched.
And \_is_local_path\ asked \pathlib.Path\, the running platform's flavour,
so \/srv/plugins\ was not a local path on Windows. Both conventions now.
Tests: \shutil.rmtree\ of a git repo fails on Windows because git marks
objects read-only, so the four sites that delete a checkout share a helper
that clears the bit. New assertions cover containment directly rather than
leaving it to whichever test happened to notice -- reverting any of the
three fixes fails them.
* fix(plugins): keep the cache path inside Windows' 260-character limit
The containment fix traded one Windows failure for another, and CI caught
what my box did not: my home directory is shorter than the runner's, so the
same path came to 264 characters there and 240 here.
Worth stating plainly, because it is the reason this was not visible until
now: the escaped path was *short*. Landing the checkout at a drive-absolute
location outside the cache had been masking a length problem the whole time.
Containing it exposed the second bug rather than causing it.
Two changes:
- Bound a key segment at 24 characters rather than 48, keeping the tail
instead of the head -- what survives is the directory nearest the
repository, which is the part someone browsing the cache can recognise,
where the head is a drive letter and \Users\. Sized against CI's actual
worst case: 264 at 48, 240 at 24, against a 260 limit.
- Pass \core.longpaths=true\ to every git invocation. That is git-for-
Windows' opt-in to the extended-length API, and without it git refuses any
path over 260 regardless of what Conductor generates. Set per-invocation
because the depth is a property of the paths Conductor builds, not a
preference of the user's. A no-op on POSIX.
Neither alone is enough. The bound keeps Conductor's own paths reasonable;
\core.longpaths\ covers a user whose cache legitimately sits somewhere deep.
* ci: verify the bundled-CLI removal, and correct its rationale
Review follow-ups on the Windows test matrix, all on the CI change itself.
The removal step could not fail and never checked that it worked. `find`
exits 0 when it matches nothing, so `2>/dev/null || true` masked genuine
errors rather than the no-match case. If the SDK layout ever changes the
binary survives, tests taking the real CLI path hang on auth, and the job
dies at the 10-minute timeout with an error naming neither the CLI nor
this step. It now fails immediately with an actionable annotation.
The second `find` deleted nothing the first missed, and the comment
justifying it was wrong. Verified against both wheels: the binary ships
inside the package (`copilot/bin/copilot`, `copilot/bin/copilot.exe`)
with no console-script entry point, so it never lands in `Scripts/`. One
pattern covers both layouts -- the leading `*/` absorbs `lib/pythonX.Y/`
or `Lib/`, the trailing `*` absorbs `.exe`.
`find` is also not absent on Windows: `find.exe` ships in System32. It
collides with the DOS search tool, which is a different debugging trail
than a missing binary, so the comment now says so. The header block
listed the test job as Ubuntu-only, while naming platforms for
install-scripts a few lines down.
Both paths exercised locally against a real venv: present -> removed,
exit 0; absent -> annotation, exit 1.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* test(skills): run the skills-root diagnostic on Windows too
`test_subdirectory_without_skill_md_is_reported` was skipped entirely on
Windows, but the reason described the fixture rather than the behaviour.
`expand_skills_root` reports any immediate subdirectory without a
`SKILL.md`; a mis-cased `Skill.md` is only one way to reach it, and it is
the one way that cannot exist on a case-insensitive filesystem.
Using a plain non-manifest filename keeps the general assertion live on
every platform. The mis-cased case stays as its own test with the
existing skip, so nothing is lost.
Worth having on Windows specifically: mis-casing `SKILL.md` is a Windows
authoring hazard -- it resolves locally and then vanishes for everyone
else -- so Windows is where the diagnostic most wants covering.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* test(cli): build the plugin-path fixture as JSON so it parses on Windows
`test_plugin_path_survives_the_report` interpolated `tmp_path` straight
into a YAML *double-quoted* scalar. Those process backslash escapes, so
on Windows the runner's `C:\Users\runneradmin\...` reached the parser as
`\U` -- a Unicode escape -- and the fixture failed with "expected escape
sequence of 8 hexdecimal numbers, but found 's'" before the assertion
under test ever ran.
Same class as the `_format_toml` bug this branch already fixed, one
layer out: a value with meaning to the serialiser, interpolated as if it
were inert.
YAML is a superset of JSON, so `json.dumps` produces a scalar that is
valid on every platform and round-trips the path exactly. Verified
against the literal path from the failing CI run: the old form raises
ScannerError, the new form round-trips.
Arrived with #408, so it is not a regression from this branch -- but it
is only visible because of the Windows leg this branch adds, and the
suite has to be green for that leg to mean anything.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* test(cli): skip the closing-tag path case where a path cannot hold it
`test_plugin_path_survives_the_report` puts a rich markup marker inside a
directory name. `/` is a path separator on Windows, so `plug[/etc/x]`
becomes three nested components -- `plug[`, `etc`, `x]` -- and the literal
string the assertion looks for cannot appear in the rendered path.
Rich's crashing form is a *closing* tag, so it needs the slash. There is
no Windows-expressible variant to substitute, which makes this a scenario
the platform cannot host rather than an assertion to loosen.
Scoped to the one parametrized case rather than the test: `[dim]` is a
legal Windows directory name, so the silent-deletion half still runs
there. The crashing form keeps its coverage too -- ten other cases in this
module put it through error text, agent names, descriptions and manifest
messages, none of which touch the filesystem.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Frank Li <xuefl@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Jason Robert <jasonrobert@microsoft.com>
Copilot-Session: ba2dfe2e-2c85-4d70-88f9-a604a700a0c4
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.

Rich markup injection across the CLI: validate crashes on a bracketed workflow name, for-each panel titles are corrupted

1 participant

@jrob5756