Uh oh!
There was an error while loading. Please reload this page.
feat(cli): add a read-only \conductor status\ command - #389
Conversation
There was no read-only way to see which background workflows are running. The only command that lists them is 'conductor stop' -- which stops one when exactly one is running. So the natural 'what's running?' reflex is destructive precisely when there is a single run to lose. I killed a healthy 40-minute workflow this way, having read the help text as 'lists, and if unambiguous, offers to stop'. The behaviour is documented, so this is not a correctness bug. But documentation is the only thing between the user and data loss, and the destructive path is the shortest one to type. The asymmetry is what makes it sting: with TWO runs it prints a table and does nothing; with ONE it terminates. 'conductor status' lists and never terminates, with --json for automation. It reuses read_pid_files and _print_running_list, so this is mostly wiring existing pieces to a command that does not kill anything. It also shows the dashboard URL, which is the field I most often actually want and which currently has no supported way to be recovered once the launching terminal is gone. Tests assert the property that matters absolutely -- status never terminates, in any configuration -- including the exact single-run scenario that costs a workflow, and that PID files are left in place. The JSON output is asserted ASCII-safe because workflow paths are user data and can contain non-ASCII (see microsoft#342, where exactly that crashed a run after it had succeeded). Deliberately NOT narrowing 'conductor stop' in the same change. Requiring --port/--all there is defensible now that discovery has its own home, but it is a breaking change to a documented behaviour and wants its own decision. tests/test_cli matches the pre-existing baseline exactly (22 failures before and after; sandbox lacks pytest-asyncio). ruff clean. Fixesmicrosoft#384
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #389 +/- ##
=======================================
Coverage ? 91.32% =======================================
Files ? 108 Lines ? 17507 Branches ? 0 =======================================
Hits ? 15989 Misses ? 1518 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.
The concept is right and the scoping is disciplined, great work. Most of what follows is about closing the gap between what the command promises and what it currently does.
Everything below is reproduced against the branch rather than inferred.
The main one: status is not read-only. read_pid_files() unlinks PID files that are corrupt, missing pid, or whose process looks dead, and write_pid_file uses a non-atomic write_text. So a status that lands during a --web-bg launch reads a truncated file and deletes it. I reproduced the end state against a real background run: status printed "No background workflows are currently running", exited 0, and removed the PID file, while the process was still alive and its dashboard was still answering. stop could no longer find it either. That is issue #344's orphan, reached through the command meant to be the safe one.
The rest, briefly:
- A single malformed PID file takes down the whole listing with a traceback and exit 1,
--jsonincluded, with empty stdout. run_idandlog_fileare always empty strings, because the only writer atbg_runner.py:394never passes them.- The Dashboard URL is cropped at 80 columns once real
started_atvalues are involved. - One mutation worth knowing about: changing
pid.py:249to sendSIGTERM, sostatuskills everything it lists, passes all 8 tests.
Not anchorable to a line, so noting here: status is missing from docs/cli-reference.md, CHANGELOG.md, AGENTS.md, the help-panel map in tests/test_cli/test_help_panels.py, and plugins/conductor/skills/conductor/references/execution.md. That last one still tells agents to use conductor stop to inspect background runs, which is the habit this PR is trying to break.
Worth coordinating with #383, which rewrites this same code and introduces a tri-state Liveness. A non-destructive reader here and that refactor are the same change from two directions.
| `conductor stop` also lists running workflows, but it stops one when | ||
| exactly one is running -- so the natural "what's running?" reflex is | ||
| destructive precisely when there is a single run to lose. This command is | ||
| read-only and always safe. |
There was a problem hiding this comment.
This is the sentence a user leans on to decide whether running the command is safe, and it is not accurate. read_pid_files() unlinks PID files on three paths (pid.py:150, :155, :164). Since Typer renders this as --help, the claim ships as product copy.
| read-onlyandalwayssafe. | |
| neversignalsorterminatesaprocess. |
| from conductor.cli.pid import read_pid_files | ||
| running = read_pid_files() |
There was a problem hiding this comment.
This is where the read-only property is actually lost. read_pid_files() deletes any PID file it cannot parse, and write_pid_file writes non-atomically (pid.py:127), so a status racing a --web-bg launch reads a zero-length file and removes it.
I reproduced the outcome against a live run: status reported nothing running, exited 0, deleted the file, and the workflow kept going with its dashboard still serving. After that neither status nor stop can find it.
A cleanup: bool = True parameter on read_pid_files(), called with cleanup=False here, keeps reaping in stop where deregistration is what the user asked for. Making the write atomic (os.replace) closes the other half.
| payload = [ | ||
| { | ||
| "pid": e["pid"], | ||
| "port": e["port"], |
There was a problem hiding this comment.
port is indexed directly while the fields around it use .get(), and read_pid_files only validates pid. A PID file without port raises KeyError here, so the command exits 1 with a traceback and empty stdout. Same on the table path at line 1316.
One bad file takes out the listing for every healthy run beside it, which is the failure mode most likely to send someone relaunching. For --json consumers there is nothing on stdout to parse.
Skipping bad entries and reporting them (a sibling unreadable key in the payload, a warning on stderr for the table) keeps the good rows visible. Valid JSON that is not an object also reaches data.get("pid") at pid.py:153 and raises AttributeError outside the existing guard.
| "pid": e["pid"], | ||
| "port": e["port"], | ||
| "workflow": str(e.get("workflow", "")), | ||
| "run_id": e.get("run_id", ""), |
There was a problem hiding this comment.
run_id and log_file are always empty in practice. The only production writer is _finalize_background_launch() at bg_runner.py:394, which calls write_pid_file(proc.pid, web_port, pid_workflow_ref) and leaves both at their "" defaults (pid.py:99-100). Confirmed on a real launch.
Automation will branch on these and take the else path every time. run_id is in scope at the call site, so threading it through is small; log_file is harder, since only the child knows its events log path, though the parent does have the bg stderr and stdout paths. Dropping them until something writes them is also fine, but shipping fields that are structurally empty is the one option I would avoid.
| table.add_column("Workflow", style="white") | ||
| table.add_column("Started", style="dim") | ||
| if show_url: | ||
| table.add_column("Dashboard", style="blue") |
There was a problem hiding this comment.
The URL gets cropped at the default terminal width. Production started_at is a full datetime.now(UTC).isoformat() at 32 characters, 13 wider than the 19-character value the tests write, and Rich crops rather than wraps. At 80 columns with a realistic workflow name:
| 53941 | 72319 | code-review-pipel... | 2026-08-10T14:02:... | http://127.0.0.1:... |
So the field the command exists to surface is the one that disappears. overflow="fold" on this column, or trimming sub-second precision from Started, would fix it.
| table.add_column("Dashboard", style="blue") | |
| table.add_column("Dashboard", style="blue", overflow="fold") |
| Args: | ||
| entries: List of PID-file dicts. | ||
| con: Rich Console for output. | ||
| show_url: Include the dashboard URL column. On by default for |
There was a problem hiding this comment.
"On by default" reads as though the default were True; it is False, and only one of the three call sites passes True. An Args: entry describing a specific caller's policy will also drift as callers change.
| show_url: IncludethedashboardURLcolumn. Onbydefaultfor | |
| show_url: AppendaDashboardURLcolumn. DefaultstoFalse; | |
| ``conductorstatus``passesTrue. |
| assert result.exit_code == 0 | ||
| assert "8080" in result.output | ||
| stop_one.assert_not_called() | ||
| kill.assert_not_called() |
There was a problem hiding this comment.
These two assertions cannot fail. status only calls read_pid_files() and _print_running_list(), so neither _stop_process nor conductor.cli.app.os is reachable from it. Meanwhile line 69 patches out _is_process_alive, which owns the only signal status actually sends: os.kill(pid, 0) at pid.py:249.
The practical effect is that changing pid.py:249 to send SIGTERM, which makes status terminate every workflow it lists, passes all 8 tests here plus test_stop.py and test_pid.py.
Spying the layer below pins the real property, and test_pid.py:142-160 already uses that patch target:
| kill.assert_not_called() | |
| stop_one.assert_not_called() | |
| kill.assert_not_called() | |
| deftest_liveness_probe_never_sends_a_real_signal(self, pid_tmpdir: Path) ->None: | |
| """The only signal ``status`` sends must be the null probe.""" | |
| _write_pid(pid_tmpdir, 4242, 8080) | |
| withpatch("conductor.cli.pid.os.kill") askill: | |
| runner.invoke(app, ["status"]) | |
| assertall(call.args[1] ==0forcallinkill.call_args_list) |
| "pid": pid, | ||
| "port": port, | ||
| "workflow": workflow, | ||
| "started_at": "2026-03-03T00:00:00", |
There was a problem hiding this comment.
This helper hand-builds the PID JSON instead of calling conductor.cli.pid.write_pid_file(), and the divergence is hiding two real defects. The fixture writes a 19-character started_at where production writes 32, which is why the URL cropping above goes unnoticed. It also sets run_id, so assert entry["run_id"] == _RUN_ID at line 134 passes against a value the product never emits.
Building fixtures through the real writer couples these tests to the actual schema and catches both. #383 does this in its own tests, so it is also the direction the sibling PR is heading.
Addresses the review on microsoft#389. The main one: `status` was not read-only. It called `read_pid_files`, which prunes as it reads -- unlinking any entry that is corrupt, missing `pid`, or whose process looks dead. So the command written to be the safe alternative to `stop` could itself deregister a live run, which is issue microsoft#344's orphan reached through the safe door. Combined with a non-atomic `write_pid_file`, a `status` landing during a `--web-bg` launch reads a half-written file and deletes it. Split the parse from the policy rather than adding a flag. New `scan_pid_files()` reads and classifies without touching the disk; `read_pid_files()` remains the maintenance path for `stop`. One parser, two policies, and the read-only contract is a property of the function a reader picks rather than of a boolean they must remember to pass. Also from the review: - A single malformed PID file took down the whole listing. The payload indexes `e["port"]` directly while the reader only guarded `pid`, so one entry without a port raised KeyError and the command exited 1 having printed nothing -- `--json` included, with empty stdout, hiding every healthy run. `scan_pid_files` skips entries without an integer pid and port, and logs which file it skipped. - Listing order is now filename-sorted. `Path.glob` order is filesystem-dependent and this output is user-facing. - `status` was missing from `docs/cli-reference.md`, `AGENTS.md`, `CHANGELOG.md`, the help-panel map in `test_help_panels.py`, and `plugins/.../references/execution.md`. All added. The last one matters most: it still told agents to use `conductor stop` to inspect background runs, which is exactly the habit this command exists to break. The existing `test_pid_files_are_left_in_place` only covered a *live* process -- which the pruning reader would have kept anyway -- so it passed while `status` was still deleting things. That is why the bug survived. The five new tests cover the paths that actually prune: dead-looking process, unreadable file, missing port, and the same two through `--json`. All five fail against the unfixed code and pass with it. 519 passed in tests/test_cli. ruff check unchanged at 2 pre-existing findings, `ty check src` unchanged at 64, and my files are ruff-format and `ty` clean.
Frank Li (franklixuefei)
commented
Aug 10, 2026
Thanks — the destructive-read finding was exactly right, and the mutation you ran (
|
| Test | Without fix |
|---|---|
test_a_pid_file_is_not_deleted_when_the_process_looks_dead | ❌ fails |
test_an_unreadable_pid_file_is_not_deleted | ❌ fails |
test_one_malformed_file_does_not_hide_the_healthy_runs | ❌ fails |
test_json_still_emits_the_healthy_runs | ❌ fails |
test_malformed_files_are_still_on_disk_afterwards | ❌ fails |
Docs
All five gaps closed: docs/cli-reference.md (full section + ToC), AGENTS.md, CHANGELOG.md, the help-panel map in test_help_panels.py, and plugins/.../references/execution.md. That last one mattered most — it still told agents to use conductor stop to inspect background runs, which is the habit this command exists to break.
Coordination with #383
You called these "the same change from two directions", which is right. #383 now carries the root-cause half: atomic write_pid_file (temp + os.replace, so no reader can see a partial file) and a warning before any unlink. This PR carries the reader half. They are complementary and touch different functions, so whichever lands second is a small rebase.
Still open, deliberately
run_idandlog_fileare always empty. Real, and I left it — the writer atbg_runner.py:394never passes them, so fixing it is a change to the launch path rather than tostatus. Happy to do it here if you would rather not split it.- Dashboard URL cropped at 80 columns. Reproduced. It is a
_print_running_listrendering concern shared withstop, so I would rather fix it in one place than widen this PR.
Gates
519 passed in tests/test_cli. ruff check unchanged at 2 pre-existing findings; ty check src unchanged at 64; my files are ruff format and ty clean.
# Conflicts: # CHANGELOG.md
Frank Li (franklixuefei)
commented
Aug 10, 2026
Correction on my previous comment: CI had not actually run, and that was my doing. The push landed on the right branch — PR head The cause: this PR was The conflict was mine. I added the CHANGELOG entry without merging CI now runs and is green: 10/10, including Lint, Type Check, and Test on Python 3.12 and 3.13. Worth flagging as a general trap rather than just my mistake: a conflicting PR shows a check list that is empty rather than red, so it reads as "nothing to do" instead of "nothing ran". Thanks for pushing on it — I would otherwise have reported this as green off a stale run. |
# Conflicts: # CHANGELOG.md
`TestStatusNeverStops` named the right property but asserted it where it could not be violated. `status` never reaches `_stop_process` or `app.os.kill`, so those two assertions cannot fail, and the one call that can signal a process -- `os.kill(pid, 0)` in `_is_process_alive_posix` -- was stubbed out by the `_is_process_alive` patch. The gap was measurable: changing that probe to send SIGTERM, so `status` terminates every workflow it lists, passed all 13 tests. Assert on the signal value instead. `sys.platform` is forced (as in `test_pid.py`) so the POSIX probe runs on any host, and the non-empty guard on `call_args_list` stops the test passing vacuously if the probe is never reached. Verified against three mutants -- SIGTERM, SIGKILL, and a forced Windows dispatch -- each of which now fails. Also add `status` to the panel-mapping dict in `test_help_panels.py`; it was added to the presence tuple but not the mapping, which is the half that catches a miscategorised command. Verified by moving `status` to the Environment panel, which now fails. Correct the `show_url` docstring, which read "On by default" for a parameter that defaults to False. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)
commented
Aug 11, 2026
Verified the fixes — the destructive-read work holds up. Reproduced each one: dead process and truncated file are both preserved now, and a malformed entry no longer takes down the listing. I also reverted Splitting the parser from the policy was the better call. A One thing did not land, so I pushed it rather than send you round again ( The mutation finding was narrower than the pruning bug. The new test asserts on the signal value instead, which is the one place the property can actually break: with (
patch("conductor.cli.pid.sys.platform", "linux"),
patch("conductor.cli.pid.os.kill") askill,
):
result=runner.invoke(app, ["status"])
assertkill.call_args_list, "the liveness probe never ran, so nothing was asserted"assertall(call.args== (4242, 0) forcallinkill.call_args_list)
Also added 540 passed in Agreed on both deferrals. Approving once CI is green. |
Jason Robert (jrob5756)
commented
Aug 11, 2026
Filed the two deferrals so they do not get lost:
Neither blocks this PR. |
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.
* 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>
The module docstring claimed behavioural coverage held for "all five sinks". That was true when written, but `app.py` now has twelve `print_json` calls: `status --json` (microsoft#389) and the `stop` payloads (microsoft#344, microsoft#399) landed while this PR was in review. All twelve carry `ensure_ascii=True` already, so the convention propagated on its own, but the claim had quietly become a count of the sinks the file happens to test rather than of the sinks that exist. Reworded to say what is actually true: `TestRealSinksThroughTheCli` covers the five sinks microsoft#342 was diagnosed against, and the source guard covers every sink in both modules, which is the check that catches an omission anyway. Also fixed "AST guard at the bottom", which stopped being the bottom when the behavioural tests were added below it. No behavioural change; docstrings and comments only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ec (#381) * fix(cli): emit ASCII-safe JSON so results survive a legacy stdout codec Closes#342. On Windows under a cp1252 locale, conductor run crashed with UnicodeEncodeError *after* the workflow had already completed successfully, while writing its final JSON result to stdout. The document was truncated mid-field so no caller could parse it, and the process exited non-zero, so automation concluded a successful run had failed. json.dumps already emits pure ASCII by default, so conductor hands off a safe string. What reintroduces the unencodable character is rich: Console.print_json defaults to ensure_ascii=False, re-parsing and re-serialising the payload with the literal glyph immediately before the write. The JSON channel is made unsafe by the pretty-printer, not by our serialisation. Passing ensure_ascii=True at all five JSON sinks fixes it losslessly: \\uXXXX escapes (surrogate pairs for non-BMP) are valid JSON and decode back to the original text. errors='replace' would also stop the crash but would silently turn review feedback into '?', and errors='backslashreplace' emits \\U0001f600 for non-BMP, which is not valid JSON at all. Tests simulate rather than detect. CI runs on ubuntu-latest, so anything gated on sys.platform would never execute; instead each test renders through a TextIOWrapper bound to strict cp1252 and asserts both that nothing raises and that the payload round-trips - the property a machine consumer depends on. A guard test asserts the unfixed path still raises, so a future change that drops ensure_ascii cannot leave the suite passing for the wrong reason. One fixture note worth recording: U+2014 EM DASH is excluded from the samples because cp1252 can encode it (0x97), so it would not exercise the failure despite being common in agent prose. That asymmetry is why this bug looked intermittent in the field - whether it fired depended on which glyph an agent happened to emit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ba2dfe2e-2c85-4d70-88f9-a604a700a0c4 * test(cli): guard the production sinks, not just rich's behaviour Addresses post-implementation review feedback on the #342 fix. The first version of these tests constructed their own Console and passed ensure_ascii=True from the test body. That characterised rich but protected nothing: reverting every production change would have left the whole suite green. A regression guard that cannot fail on the regression is worse than no guard, because it manufactures confidence. Added an AST assertion over conductor/cli/app.py and conductor/cli/doctor.py requiring every print_json call to pass ensure_ascii=True. Verified it fails on a simulated revert (offenders reported at app.py:553,566,1008,1014) and passes on the fixed tree. It also catches a NEW sink added without the keyword, which is the likelier future regression - rich defaults it to False. A source assertion is deliberate rather than a mock. Driving the real conductor run command to its JSON sink requires standing up a full workflow execution; that belongs in an end-to-end subprocess test invoked with PYTHONIOENCODING=cp1252, which is tracked as follow-up on the issue rather than faked here. Also removed two tests that invoked the CLI through CliRunner: the command exits before reaching the JSON sink without a fully valid execution context, so they asserted nothing while appearing to cover the production path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ba2dfe2e-2c85-4d70-88f9-a604a700a0c4 * test(cli): resolve JSON sinks by module, not by path arithmetic Fixes the Lint job, and a latent import-order bug underneath it. Ruff flagged four dead imports left behind when the `CliRunner` tests were removed (they asserted nothing once `conductor run` exited before reaching the JSON sink). Removing them exposed the real problem: the module was only importable because something else in the suite had already imported `conductor.cli.app`. `conductor/cli/__init__.py` re-exports the Typer object as `app`, which shadows the submodule of the same name -- so `from conductor.cli import app` yields the Typer instance, and `.__file__` raises AttributeError. Run inside the full suite an earlier import had rebound the attribute to the module and it passed; run on its own, collection failed. Resolve each sink with `importlib.import_module` and read its `__file__` directly. That always returns the module regardless of collection order, and drops the `parents[2]` path arithmetic, so the guard no longer depends on the repository layout either. Verified the guard still fails on a simulated revert (reports app.py:553) and that the module now passes standalone as well as in the full suite. * test(cli): drop the orphaned cp1252 fixture and sort imports Follow-up to the previous commit, which fixed the import-order bug but left two ruff findings behind. `cp1252_output_console` was a fixture left over from the removed CliRunner tests. No test referenced it, and it still called `monkeypatch.setattr` on the deleted `app_module` binding -- an F821 that would only have raised had anything used it. The `_Cp1252Console` helper it wrapped is still live in the negative-control test, so only the fixture is removed. Also applied ruff's import ordering. `ruff check src tests` and `ruff format --check src tests` -- the exact CI commands -- both clean. Module passes standalone (7) and in the full suite; guard still fails on a simulated revert. * test(cli): give the ensure_ascii fix behavioural coverage it was missing Addresses the review on #381. The load-bearing criticism was right: nothing asserted the fix worked. Reverting any of the five sinks failed only the source-level AST guard, never a behavioural test, because every behavioural test built its own Console. The comment claiming a behavioural test was infeasible was also wrong -- tests/test_cli/test_run.py already reaches app.py's success sink by patching conductor.cli.run.run_workflow_async. What the earlier attempt was missing was that mock, not a process boundary. TestRealSinksThroughTheCli adds that. Verified per sink: reverting app.py's success-sink ensure_ascii now fails two tests -- the guard and the behavioural one -- where before it failed only the guard. The doctor `data=` shape gets its own case because it goes through JSON.from_data rather than the string path the negative control exercises, so it is independently vulnerable. `text()` earns its place back as a result, along with the `_Cp1252Console` arguments that only mattered while something decoded the buffer. The module docstring described the version that had the CliRunner tests and was not true of the file as it stood. It now says what the file does, and says plainly that behavioural coverage was absent before this commit rather than quietly becoming true again. `ty` no longer errors on the samples: reading them back out of ParameterSet.values typed them as `object` and reached into _pytest.mark.structures, which is not public. They start life as a dict[str, str] now and the params are built from it. conductor doctor's table path is deliberately still out of scope, which is the option the review offered as an alternative to fixing it here. It hardcodes U+2713, U+2717 and U+25CB -- all unencodable in cp1252, and _render_env / _render_providers are separate print calls, so the report dies part-written. Fixing it means threading the console through ~18 call sites across six helpers that do not currently take one. Filed as #401, and both the module docstring and the glyph definitions now say so, so the AST guard's mention of conductor.cli.doctor reads as "its JSON sink is covered" rather than the whole module. CHANGELOG entry added; the PR had none. 535 passed in tests/test_cli. ruff check unchanged at 2 pre-existing findings, `ty check src` unchanged at 64, my files ruff-format and `ty` clean. * test(cli): make the doctor sink test actually reach doctor.py, and cover the rest The doctor case had the same defect the last round was about. The test built its own Console and passed \�nsure_ascii=True\ from the test body, so it asserted that rich honours the keyword -- which the negative control already covers from the other side -- and never reached \doctor.py\. Reverting the sink there left it green. \ un_doctor\ takes \console=output_console\ from \�pp.py\, so the same patch the success sink uses works: patch only the console, plus \gather\, and drive the real function. While in here, cover the three remaining \�pp.py\ sinks -- \ un\'s terminate handler and \ esume\'s success/terminate pair. With the AST guard disabled so it cannot cover for them, reverting each of the five sinks now fails exactly one behavioural test; before this it was one of five. Docs corrected. The file argued with itself: a comment claimed a behavioural test needed a full workflow execution while the class docstring said that claim was wrong. The module docstring promised a revert fails a test that exercised the sink, which held for one sink. The guard's remaining job is the narrower one it is actually good at -- catching a *new* sink added without the keyword -- and its real blind spot, the hardcoded module list, is now named. * docs(test): correct the sink count after main grew more JSON sinks The module docstring claimed behavioural coverage held for "all five sinks". That was true when written, but `app.py` now has twelve `print_json` calls: `status --json` (#389) and the `stop` payloads (#344, #399) landed while this PR was in review. All twelve carry `ensure_ascii=True` already, so the convention propagated on its own, but the claim had quietly become a count of the sinks the file happens to test rather than of the sinks that exist. Reworded to say what is actually true: `TestRealSinksThroughTheCli` covers the five sinks #342 was diagnosed against, and the source guard covers every sink in both modules, which is the check that catches an omission anyway. Also fixed "AST guard at the bottom", which stopped being the bottom when the behavioural tests were added below it. No behavioural change; docstrings and comments only. 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
Fixes#384.
The problem
There is no read-only way to see which background workflows are running. The only command that lists them is
conductor stop— which stops one when exactly one is running:So the natural "what's running?" reflex is destructive precisely when there is a single run to lose. I killed a healthy 40-minute workflow this way: I ran
conductor stopto check status, having read that as "lists, and if unambiguous, offers to stop".The behaviour is documented, so this is not a correctness bug. But the documentation is the only thing standing between the user and data loss, and the destructive path is the shortest one to type. The asymmetry is what makes it sting — with two runs it prints a table and does nothing; with one it terminates. The safe outcome is the one that requires more runs to be active.
The change
Lists and never terminates. It reuses
read_pid_files()and_print_running_list(), so this is mostly wiring existing pieces to a command that does not kill anything.It also shows the dashboard URL, which is the field I most often actually want and which currently has no supported way to be recovered once the launching terminal is gone:
--jsonemitspid,port,workflow,run_id,started_at,log_file,url— for the same reasonstop --jsonexists in #383: automation should not have to parse a table.Why a flag on
stopisn't the fixconductor stop --dry-runwould work mechanically, but it keeps the hazard one typo away and leaves "list running workflows" filed under a destructive verb. Discovery is a read operation and wants a read verb.Tests
The property under test is absolute —
statusmust never terminate anything, in any configuration — so it is asserted directly rather than inferred from output:_stop_processandos.killnever called — the exact scenario that costs a workflow--jsonshapepid/port/run_id/urlpresent--jsonASCII-safeThe ASCII test is there because workflow paths are user data and can contain non-ASCII — exactly what crashed a completed run in #342.
status --jsonis a new JSON sink, so it passesensure_ascii=Truefrom the start rather than inheriting that bug.Deliberately not in this PR
Narrowing
conductor stop. Now that discovery has its own home, requiring--portor--allthere is defensible — but it is a breaking change to a documented behaviour and wants its own decision. Happy to follow up if you want it.Validation
tests/test_climatches the pre-existing baseline exactly — 22 failures before and after, measured by stashing the change rather than assumed. (Sandbox has no PyPI access sopytest-asynciois unavailable; those async tests error identically either way.)ruff checkandruff format --checkclean. Also exercised the command end-to-end against a real PID directory.Related
#344 / #383 —
conductor stopcould delete the PID file without stopping the process. Once that lands, a run can survivestopand stay registered, which makes a read-only listing more valuable: it becomes the way you find the survivor. The--jsonpayload here is deliberately shaped to line up withstop --jsonfrom that PR.