Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 64 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ step-by-step checklist.

- **duration.py**: `parse_duration(value)` shared helper. Accepts plain `int`/`float` seconds, or strings with `ms`/`s`/`m`/`h` suffix. Raises `ValueError` (nests cleanly inside Pydantic `ValidationError`). Rejects booleans. Bounds enforcement (e.g. > 0, 24h cap) lives in callers so the parser can be reused.

- **console.py**: `make_console()` / `styled()` / `join()` — the markup-safety primitives (issue #406). A leaf module (like `duration.py`) with no conductor imports, deliberately **top-level rather than under `cli/`** because `gates/` and `providers/` need it and must not import from `cli/`. `make_console` locks `markup=False`, inverting Rich's default so an interpolated runtime value is literal unless it asks to be styled; it *rejects* a `markup=` kwarg rather than allowing an override — on the constructor and on `print`/`log`, since rich's per-call `markup=True` would otherwise reopen the defect from a single line. `styled("<template>", ...)` parses the template's markup — conductor's own literal — while inserting values verbatim. It works by replacing each field with a **length-matched filler run**, parsing once, then locating each run to substitute the value back: matching the width keeps the parsed template's spans valid, which is what makes nested styling around a placeholder (`[bold][red]{}[/red][/bold]`) come out right. Reading `spans[0].style` and re-applying it — the obvious alternative — collapses the nesting. A value that is already a `Text` is spliced in with its own spans re-anchored, so pre-styled fragments compose (`styled("{} {}", CHECK, name)`); without that, `format()` would flatten it and silently drop the colour from every doctor table cell. `join(sep, parts)` exists because `Text.join` requires every part to already be a `Text`, while the common shape here is a `content_lines` list mixing conductor-styled fragments with plain runtime values. `rich.markup.escape` is deliberately unused throughout: the parser treats `\[` as an escaped bracket, so `\[0-9\]+` renders as `[0-9\]+` whether escaped or not, whereas a `Text` is byte-exact. See the Console Output section under Code Style.

- **providers/**: SDK provider abstraction
- `base.py` - `AgentProvider` ABC defining `execute()`, `validate_connection()`, `close()`
- `_output_shape.py` - `normalize_agent_output(content, schema)` — the single entry point providers call before `validate_output` (issue #343). It raises `ValidationError` when the parsed response is not a JSON object (a bare `42`/`null`/array), because `validate_output` would otherwise either raise `TypeError` from a membership test (numbers, booleans, null) or report a misleading "missing required field" (strings, arrays). It then applies `unwrap_scalar_wrappers`: fires only when the schema declares `string`/`number`/`boolean`, a `dict` arrived, and **exactly one** candidate slot has the expected type. Candidate slots are the field's own name plus the generic `value`/`result` keys, deduped so a field literally named `value` or `result` isn't rejected as ambiguous against itself. Two matches count as ambiguous; any other key shape is ignored. Both are left untouched (same object identity) so the caller re-prompts rather than guessing — this is what stops `{"answer": {"error": "..."}}` being laundered into an answer. Every unwrap logs a warning, naming discarded sibling keys when there are any. Kept out of `executor/output.py` on purpose — see the note there.
Expand Down Expand Up @@ -221,7 +223,7 @@ log. See issue #116.
## Tests Structure

Tests mirror source structure in `tests/`:
- `test_cli/` - CLI command tests, e2e tests
- `test_cli/` - CLI command tests, e2e tests. `test_markup_guards.py` is the one that keeps issue #406 closed: it reads `src/conductor` with `ast` and fails with file:line across eight rules — a bare `Console` or a `Console` subclass (A), an interpolated `Panel` title or `Prompt` (B), an f-string into `Text.from_markup` (C), a markup literal at a print/cell sink (D), a `Text` through the builtin `print` (E) or into an f-string (F), unescaped brackets in `typer` help text (G), and any use of `rich.markup.escape` (H). Each rule is a shared predicate called by both the source scan and its negative control, so a control cannot pass against a drifted rule — that had already happened once. Each rule has a negative control, because a source-scanning check that quietly matches nothing reports "all clear" forever. `test_markup_injection.py` covers the same ground behaviourally, driving the real commands — the two layers are not redundant: the guard alone cannot prove `styled` renders correctly, and the behavioural tests alone cannot stop the *next* call site, which is the actual failure mode here
- `test_config/` - Schema validation, loader tests
- `test_engine/` - Workflow, router, context, limits tests
- `test_executor/` - Agent, template, output tests
Expand Down Expand Up @@ -258,6 +260,67 @@ When adding new fields to `LimitEnforcer`:
- Pydantic v2 for data validation
- async/await for all provider operations

### Console Output

**Never put a runtime value into a string that Rich will parse as markup.**
Rich reads `[...]` in a plain `str` as a style tag, so a workflow name, an
agent name, a plugin name from a cloned repo, a path, or an `str(e)` that
happens to contain a bracketed token is interpreted as styling. In rich a
token is a tag when its **first character** is lowercase, `#`, `/` or `@`,
which splits three ways: `[0]` renders literally, `[task1]` is **silently
deleted**, and `[/etc/x]` raises `MarkupError` out of the print call. The
quiet half is the dangerous one — a listing that drops half a name looks
like it worked. Note `style=` does **not** disable parsing (issues #382,
#387, #406).

Each rule below is enforced by a matching rule in
`tests/test_cli/test_markup_guards.py`, which reads the source and reports
file:line:

- **Build every console with `conductor.console.make_console()`** (rule A).
It locks `markup=False`, so a plain string is literal unless it asks to be
styled. This covers plain prints, `Panel` bodies, `Table` cells, headers,
titles and captions, and `Rule` titles. `markup` is not overridable —
passing it to the constructor, to `print` or to `log` raises `TypeError`,
because rich's per-call `markup=True` would otherwise reopen the whole
defect from one line. Subclass `MarkupFreeConsole` rather than rich's
`Console` so the refusal is inherited (see `cli/run.py::_SilentAwareConsole`).
- **Style with `conductor.console.styled("<template>", value, ...)`.** The
template is conductor's own literal and is parsed; values are inserted
verbatim and never reach the parser. A value that is already a `Text` is
spliced in with its styling intact, so pre-styled fragments compose. Use
`Text.from_markup("...")` when there is nothing to interpolate, and
`conductor.console.join(sep, parts)` to join a list mixing `str` and
`Text` (`Text.join` requires every part to be a `Text` already).
- **`Panel(title=)`, `Panel(subtitle=)` and `Prompt`/`Confirm`/`IntPrompt`
prompts must be handed a `Text`** (rule B). Rich calls `Text.from_markup`
on those unconditionally (`rich/panel.py`, `rich/prompt.py`), so
`markup=False` never reaches them. This is the trap that made #387
incomplete: it fixed the panel *body* and left the `title=` f-string one
line away.
- **Never let a `Text` reach a plain-string context** — an f-string (rule F),
`str()`, or the builtin `print` (rule E). `str(Text)` is its *plain* form,
so styling is dropped and any text rich already parsed as a tag is gone
outright. This one has the worst record in the codebase: it shipped four
separate times during #406 alone, twice destroying data rather than
colour. Use `styled("{}{}", ...)` or `join(...)` instead.
- **Do not use `rich.markup.escape`** (rule H). It is not
byte-exact — the parser treats `\[` as an escaped bracket, so an ordinary
regex like `\[0-9\]+` renders as `[0-9\]+` whether or not it was escaped
first. Building a `Text` avoids the parser entirely.
- **Typer's `help=` / `epilog=` must escape their brackets** as `\[ ... ]`
(rule G). These are outside the *console* convention — Typer renders them
through its own rich console — but they are still markup-parsed. Escaping
is the remedy here rather than a `Text`, because Typer takes a `str`.
Forgetting cost `conductor run --help` the entire `[@registry][@version]`
syntax, which appears nowhere else in the help output.

The worst outcome of forgetting is now a visible literal `[green]` in the
output rather than a crash or a silent deletion, and rule D of the guard
catches that statically. Each rule is a shared predicate called by both the
source scan and its negative control, so a control cannot pass while the rule
it guards has drifted.

### Provider Parity

All providers must maintain feature parity where applicable. Any change to one provider's behavior, contract, or capabilities must be applied to all providers. This includes:
Expand Down
37 changes: 37 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,43 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- **Bracketed text no longer crashes or corrupts CLI output** (#406). The same
defect as #382, which #387 fixed only in `cli/run.py`. `conductor validate`
died with an unhandled `MarkupError` traceback on a workflow whose `name:`
contained `[/bold]`, and silently deleted the token when it contained
`[dim]`. The quiet half is the more damaging one: a listing that drops part
of a name looks like it worked. Rich treats a bracketed token as a style tag
when its first character is lowercase, `#`, `/` or `@`, so `[0]` is fine,
`[task1]` disappears, and `[/etc/x]` raises — and `style=` does not turn
parsing off, which is what made the earlier fix look complete.

Two consequences shipped unnoticed. Every for-each iteration's verbose panel
read the same, because the engine qualifies a member's name as
`<agent>[<key>]` so interleaved output can be attributed to one iteration,
and a `key_by:` key of `task1` erased exactly that identity — while a key
starting with `/`, which `key_by:` over paths or URLs produces, killed the
run from a logging call. This needed no flags: verbose and full mode both
default on. Separately, `conductor status` (#389) and `conductor plugin
list` (#398) were written against the unfixed pattern in files #387 never
touched, and #398 made these strings third-party rather than the author's
own YAML, since plugin, marketplace, skill and subagent names are now read
out of git-cloned repositories.

Rather than escape ~450 call sites, the default is inverted: every console
is built by the new `conductor.console.make_console()` with `markup=False`,
so a plain string is literal unless it asks to be styled, 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 because rich parses those
regardless of the console setting — that is the trap that left #387
incomplete one line from the code it changed. `rich.markup.escape` is no
longer used anywhere: it cannot round-trip a value containing a backslash
before a bracket, so an ordinary regex came out mangled. Eight static guards
now read the source and fail with file:line if a new call site reintroduces
any of these shapes — including a `Text` flattened back into an f-string,
which is how the defect kept coming back, and unescaped brackets in `typer`
help text, which had silently cost `conductor run --help` the whole
`[@registry][@version]` syntax.
- **Agent text containing bracketed tokens no longer kills a run** (#382). A
step whose output contained ordinary technical prose such as
`{provider}/{type}[/{nestedType}...]/read` was parsed by rich as a closing
Expand Down
Loading
Loading