diff --git a/AGENTS.md b/AGENTS.md index f393c57f..1a06fc22 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,7 +95,7 @@ step-by-step checklist. - `fleet.py` - `fleet` group (`list`, `prune`). The **one deliberate deviation** from the `checkpoint`/`gate`/`registry` sub-app pattern: `fleet_app` sets `invoke_without_command=True` rather than `no_args_is_help=True`, because the bare `conductor fleet` (no subcommand) launches the interactive Textual TUI (Fleet Manager E7) — the TUI *is* the feature here, not a missing default. Do not "fix" this to match the other three sub-apps. The TUI is behind the optional `tui` extra; a `TEXTUAL_AVAILABLE` flag (checked only in the bare-invocation callback, mirroring `providers/aca.py`'s `AZURE_IDENTITY_AVAILABLE`) prints an install hint and exits non-zero rather than raising `ImportError` when `textual` isn't installed. That hint comes from `install_hint.py::install_command("tui")` and is printed with `soft_wrap=True` so rich never breaks the copy-pasteable command across lines — do not re-hardcode `pip install 'conductor-cli[tui]'`, which cannot work (issue #441). `fleet list`/`fleet prune` need no optional dependency. See `src/conductor/fleet/` below for the TUI itself. - `doctor.py` - `doctor` diagnostics rendering (thin presentation layer over `providers/diagnostics.py`) - `run.py` - Workflow execution command with verbose logging helpers - - `bg_runner.py` - Background process forking for `--web-bg` mode. Captures the detached child's stdout/stderr to `$TMPDIR/conductor/conductor---.bg.{stderr,stdout}.log` so silent crashes (uncaught Python exceptions, `faulthandler` dumps) leave a forensic trail — DEVNULL is **not** used for stdout/stderr. Passes `CONDUCTOR_RUN_ID`, `CONDUCTOR_BG_STDERR_LOG`, and `CONDUCTOR_BG_STDOUT_LOG` to the child via env so the child's `EventLogSubscriber` shares a run id with the bg log files and surfaces both paths in `workflow_started` system metadata. Returns a `BackgroundLaunch` dataclass (`url`, `stderr_log`, `stdout_log`, `run_id`, `workflow_started`, `still_running`, `run_record_written`). **The launch health gate's run-record poll is a readiness signal, not a kill switch** (Fleet Manager D2, hardened by issue #435): `_finalize_background_launch` waits for the dashboard to become reachable and then polls `conductor.fleet.records.read_run_record(run_id)` until the **child** has written its own record (matched on `mode`/`port`, and either `pid` equality or freshness — see below). If that poll's own 15s deadline passes while the child is confirmed alive and its dashboard is still reachable (a fresh 1s `_wait_for_server` re-probe), the gate logs a warning naming the run id and stderr log and lets the launch proceed with `run_record_written=False` rather than terminating a healthy workflow over a failed diagnostic write — `cli/app.py::_print_web_bg_no_run_record_notice` surfaces this to the user (verbose mode only, alongside the other `--web-bg` notices) and the `fleet` TUI's New Run screen does the same via a warning `notify()`. Only a child that is actually dead, or whose dashboard has gone unreachable, still fails the launch (terminate + raise), unchanged from before. The parent no longer writes a `.pid` file itself — `write_pid_file` (`cli/pid.py`) was removed as part of the original D2 change, since that was its only call site. Do not restore parent-side PID writing; a future bg-launch change belongs in this poll gate, not a reinstated `write_pid_file`. `launch_background_resume` adopts the run id from the resolved checkpoint (`_peek_resume_run_id`) instead of minting a fresh one, so the run record, `/api/info`, the events JSONL, and the capture-log filenames all agree on one id across a resume. **Readiness is three-staged.** Stage one, `_wait_for_server`, checks `proc.poll()` on every iteration of its socket-connect loop (keyword-only `proc` param), so a child that dies before binding the port is reported in well under a second instead of after the full 15s timeout. Stage one-and-a-half is the run-record poll above — a *stronger* signal than the `write_pid_file` it replaced, since the child only writes the record once it is executing. Stage two, `_wait_for_workflow_start`, polls `GET /api/info` for up to `CONDUCTOR_WEB_BG_START_TIMEOUT` seconds (default 30, `0` disables the probe) until the payload carries a `started_at` **key** (not truthiness — it can legitimately be `0`), proving the engine actually emitted `workflow_started` rather than just its HTTP server coming up (issue #410). `StartProbe` enumerates the four outcomes (`STARTED` / `CHILD_EXITED` / `PORT_CONFLICT` / `TIMED_OUT`), mirroring the `Liveness`/`Identity` enum pattern in `cli/pid.py`/`cli/app.py`. A `CHILD_EXITED` with a non-zero code (or `PORT_CONFLICT`, when `/api/info` reports an identity that positively does not match this launch's *confirmed* identity — see below) removes the child's run record via `_remove_dead_child_record` (identity-checked on `pid`, since a resumed launch can carry a checkpoint's original `run_id`) and raises `RuntimeError` with a bounded stderr-log tail (`_tail_log`); a clean exit-0 or a `TIMED_OUT` with the child still alive both return normally — the latter as `workflow_started=False`, which `cli/app.py` surfaces as a "still initializing" note rather than a failure. `still_running` is re-polled after the gate returns so a sub-second run is never advertised with a live dashboard URL. **Identity, not `Popen.pid`, is what stage two compares against (issue #444).** `Popen.pid` is not always the pid of the process that ends up running the workflow: a trampoline `sys.executable` (e.g. a Windows `uv tool install`, the documented install path) re-execs into a different one, so comparing stage two's `/api/info` payload against `proc.pid` produced a false `PORT_CONFLICT` on *every* port under that install path — and, one stage earlier, made the run-record poll's `pid == proc.pid` check never match the child's own record either, which is why the two symptoms (the spurious "did not report a run record" note, then the bogus port error) appeared together. `_confirmed_pid_from_record` accepts a record whose `pid` differs from `proc.pid` when the record is *fresh* (`_record_is_fresh`: its `started_at` parses to a timezone-aware timestamp at or after `launched_at`, captured immediately before `_spawn_detached`) — freshness is what a stale-record concern actually needs, and `proc.pid` equality was never it under a trampoline. Once a record is confirmed, its `pid` (not `proc.pid`) is carried forward as `confirmed_child_pid` and handed to `_wait_for_workflow_start`, which classifies the dashboard's reported identity (pid first, `run_id` as fallback) via `_classify_dashboard_identity`/`_DashboardIdentity` (mirroring `cli/app.py`'s `Identity`/`_confirm_identity`). A mismatch (`FOREIGN`) is only fatal when `confirmed_child_pid` is not `None`; an *unconfirmed* mismatch (`confirmed_child_pid is None` — the issue #435 downgrade path, or a resume whose predicted run id never matched) keeps polling instead of killing a possibly-healthy run on unproven suspicion, degrading at worst to the existing non-fatal `TIMED_OUT` note. The `PORT_CONFLICT` error message now names the foreign pid captured *before* `_terminate_child` runs (via the probe loop's own last-seen payload) rather than probing `/api/info` after the child is already dead, which previously always rendered `(PID unknown)`. **Termination reaches the whole process tree, not just `Popen.pid` (issue #447).** On Windows, `_spawn_detached_windows` creates the child suspended (`CREATE_SUSPENDED`) and assigns it to a fresh job object (`_create_job_object`, `JOB_OBJECT_LIMIT_BREAKAWAY_OK` set, deliberately **not** `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` so the tree outlives the launcher) *before* resuming its primary thread (`ResumeThread`, always called from a `finally` so a failed job assignment can never leave the child permanently suspended) — this closes the window a trampoline `sys.executable` could otherwise re-exec through before the job could be created. `_WindowsDetachedProcess` (the `Popen`-shaped wrapper this requires, since CPython's own `Popen` closes the child's thread handle before `__init__` returns, making suspend-then-assign impossible on top of it) exposes `terminate_tree()` (`TerminateJobObject`, reaching every process in the job regardless of exec depth). On POSIX, `start_new_session=True` already makes the child a process-group leader, so `os.killpg` is the equivalent — gated by `_SPAWNED_GROUP_LEADERS`, a registry of pids this module actually spawned, so it is never called against an arbitrary pid. `_terminate_child` now returns a `_TerminationOutcome` (`confirmed: bool`, `surviving_pids: tuple[int, ...]`) rather than a bare `None`: after the tree kill and the original single-handle terminate/wait/kill ladder, a final identity-checked sweep (`conductor.cli.pid.is_process_alive`/`terminate_process`) over every pid this call knows about (`proc.pid` and, if different, `confirmed_child_pid`) actually confirms the outcome instead of assuming it. `_remove_dead_child_record` is now keyed on the confirmed pid (via the shared `_cleanup_record_after_termination` helper) — but only fires once that pid is confirmed dead by the sweep, so a surviving orphan keeps the run record that is `conductor stop`'s only remaining handle on it. `_finalize_background_launch` has five terminate-and-raise branches; only the three with a pid to key on (dashboard-unreachable, `CHILD_EXITED`, `PORT_CONFLICT`) call `_cleanup_record_after_termination`. The other two — both inside the run-record-poll wait, one on a read failure and one on the poll's own 15s deadline going fatal — terminate without removing anything, because neither ever had a confirmed pid to begin with: no record was ever read successfully (or ever confirmed) on those paths, so there is nothing yet keyed to a pid for the helper to clean up. `_termination_note` renders the corresponding half of each failure message: "The background process was terminated." only when `confirmed` is true; otherwise a warning naming the surviving pid(s) and pointing at `conductor status` / `conductor stop --port`. + - `bg_runner.py` - Background process forking for `--web-bg` mode. Captures the detached child's stdout/stderr to `$TMPDIR/conductor/conductor---.bg.{stderr,stdout}.log` so silent crashes (uncaught Python exceptions, `faulthandler` dumps) leave a forensic trail — DEVNULL is **not** used for stdout/stderr. Passes `CONDUCTOR_RUN_ID`, `CONDUCTOR_BG_STDERR_LOG`, and `CONDUCTOR_BG_STDOUT_LOG` to the child via env so the child's `EventLogSubscriber` shares a run id with the bg log files and surfaces both paths in `workflow_started` system metadata. Returns a `BackgroundLaunch` dataclass (`url`, `stderr_log`, `stdout_log`, `run_id`, `workflow_started`, `still_running`, `run_record_written`). **The launch health gate's run-record poll is a readiness signal, not a kill switch** (Fleet Manager D2, hardened by issue #435): `_finalize_background_launch` waits for the dashboard to become reachable and then polls `conductor.fleet.records.read_run_record(run_id)` until the **child** has written its own record (matched on `mode`/`port`, and either `pid` equality or freshness — see below). If that poll's own 15s deadline passes while the child is confirmed alive and its dashboard is still reachable (a fresh 1s `_wait_for_server` re-probe), the gate logs a warning naming the run id and stderr log and lets the launch proceed with `run_record_written=False` rather than terminating a healthy workflow over a failed diagnostic write — `cli/app.py::_print_web_bg_no_run_record_notice` surfaces this to the user (verbose mode only, alongside the other `--web-bg` notices) and the `fleet` TUI's New Run screen does the same via a warning `notify()`. Only a child that is actually dead, or whose dashboard has gone unreachable, still fails the launch (terminate + raise), unchanged from before. The parent no longer writes a `.pid` file itself — `write_pid_file` (`cli/pid.py`) was removed as part of the original D2 change, since that was its only call site. Do not restore parent-side PID writing; a future bg-launch change belongs in this poll gate, not a reinstated `write_pid_file`. `launch_background_resume` adopts the run id from the resolved checkpoint (`_peek_resume_run_id`) instead of minting a fresh one, so the run record, `/api/info`, the events JSONL, and the capture-log filenames all agree on one id across a resume. **Readiness is three-staged.** Stage one, `_wait_for_server`, checks `proc.poll()` on every iteration of its socket-connect loop (keyword-only `proc` param), so a child that dies before binding the port is reported in well under a second instead of after the full 15s timeout. Stage one-and-a-half is the run-record poll above — a *stronger* signal than the `write_pid_file` it replaced, since the child only writes the record once it is executing. Stage two, `_wait_for_workflow_start`, polls `GET /api/info` for up to `CONDUCTOR_WEB_BG_START_TIMEOUT` seconds (default 30, `0` disables the probe) until the payload carries a `started_at` **key** (not truthiness — it can legitimately be `0`), proving the engine actually emitted `workflow_started` rather than just its HTTP server coming up (issue #410). `StartProbe` enumerates the four outcomes (`STARTED` / `CHILD_EXITED` / `PORT_CONFLICT` / `TIMED_OUT`), mirroring the `Liveness`/`Identity` enum pattern in `cli/pid.py`/`cli/app.py`. A `CHILD_EXITED` with a non-zero code (or `PORT_CONFLICT`, when `/api/info` reports an identity that positively does not match this launch's *confirmed* identity — see below) removes the child's run record via `_remove_dead_child_record` (identity-checked on `pid`, since a resumed launch can carry a checkpoint's original `run_id`) and raises `RuntimeError` with a bounded stderr-log tail (`_tail_log`); a clean exit-0 or a `TIMED_OUT` with the child still alive both return normally — the latter as `workflow_started=False`, which `cli/app.py` surfaces as a "still initializing" note rather than a failure. `still_running` is re-polled after the gate returns so a sub-second run is never advertised with a live dashboard URL. **Identity, not `Popen.pid`, is what stage two compares against (issue #444).** `Popen.pid` is not always the pid of the process that ends up running the workflow: a trampoline `sys.executable` (e.g. a Windows `uv tool install`, the documented install path) re-execs into a different one, so comparing stage two's `/api/info` payload against `proc.pid` produced a false `PORT_CONFLICT` on *every* port under that install path — and, one stage earlier, made the run-record poll's `pid == proc.pid` check never match the child's own record either, which is why the two symptoms (the spurious "did not report a run record" note, then the bogus port error) appeared together. `_confirmed_pid_from_record` accepts a record whose `pid` differs from `proc.pid` when the record is *fresh* (`_record_is_fresh`: its `started_at` parses to a timezone-aware timestamp at or after `launched_at`, captured immediately before `_spawn_detached`) — freshness is what a stale-record concern actually needs, and `proc.pid` equality was never it under a trampoline. Once a record is confirmed, its `pid` (not `proc.pid`) is carried forward as `confirmed_child_pid` and handed to `_wait_for_workflow_start`, which classifies the dashboard's reported identity (pid first, `run_id` as fallback) via `_classify_dashboard_identity`/`_DashboardIdentity` (mirroring `cli/app.py`'s `Identity`/`_confirm_identity`). A mismatch (`FOREIGN`) is only fatal when `confirmed_child_pid` is not `None`; an *unconfirmed* mismatch (`confirmed_child_pid is None` — the issue #435 downgrade path, or a resume whose predicted run id never matched) keeps polling instead of killing a possibly-healthy run on unproven suspicion, degrading at worst to the existing non-fatal `TIMED_OUT` note. The `PORT_CONFLICT` error message now names the foreign pid captured *before* `_terminate_child` runs (via the probe loop's own last-seen payload) rather than probing `/api/info` after the child is already dead, which previously always rendered `(PID unknown)`. **Termination reaches the whole process tree, not just `Popen.pid` (issue #447).** On Windows, `_spawn_detached_windows` creates the child suspended (`CREATE_SUSPENDED`) and assigns it to a fresh job object (`_create_job_object`, `JOB_OBJECT_LIMIT_BREAKAWAY_OK` set, deliberately **not** `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` so the tree outlives the launcher) *before* resuming its primary thread (`ResumeThread`, always called from a `finally` so a failed job assignment can never leave the child permanently suspended) — this closes the window a trampoline `sys.executable` could otherwise re-exec through before the job could be created. `_WindowsDetachedProcess` (the `Popen`-shaped wrapper this requires, since CPython's own `Popen` closes the child's thread handle before `__init__` returns, making suspend-then-assign impossible on top of it) exposes `terminate_tree()` (`TerminateJobObject`, reaching every process in the job regardless of exec depth). On POSIX, `start_new_session=True` already makes the child a process-group leader, so `os.killpg` is the equivalent — gated by `_SPAWNED_GROUP_LEADERS`, a registry of pids this module actually spawned, so it is never called against an arbitrary pid. `_terminate_child` now returns a `_TerminationOutcome` (`confirmed: bool`, `surviving_pids: tuple[int, ...]`) rather than a bare `None`: after the tree kill and the original single-handle terminate/wait/kill ladder, a final identity-checked sweep (`conductor.cli.pid.is_process_alive`/`terminate_process`) over every pid this call knows about (`proc.pid` and, if different, `confirmed_child_pid`) actually confirms the outcome instead of assuming it. `_remove_dead_child_record` is now keyed on the confirmed pid (via the shared `_cleanup_record_after_termination` helper) — but only fires once that pid is confirmed dead by the sweep, so a surviving orphan keeps the run record that is `conductor stop`'s only remaining handle on it. `_finalize_background_launch` has five terminate-and-raise branches; only the three with a pid to key on (dashboard-unreachable, `CHILD_EXITED`, `PORT_CONFLICT`) call `_cleanup_record_after_termination`. The other two — both inside the run-record-poll wait, one on a read failure and one on the poll's own 15s deadline going fatal — terminate without removing anything, because neither ever had a confirmed pid to begin with: no record was ever read successfully (or ever confirmed) on those paths, so there is nothing yet keyed to a pid for the helper to clean up. `_termination_note` renders the corresponding half of each failure message: "The background process was terminated." only when `confirmed` is true; otherwise a warning naming the surviving pid(s) and pointing at `conductor status` / `conductor stop --port`. A keyword-only `cwd: Path | None = None` (issue #477, default `None`) threads through `_spawn_detached_posix`/`_spawn_detached_windows`/`_spawn_detached`/`_spawn_bg_child`/`launch_background`/`launch_background_resume` to `subprocess.Popen(cwd=...)` (POSIX) / `_winapi.CreateProcess`'s `current_directory` argument (Windows, both call sites) -- the child's own `os.getcwd()` is what `engine/workflow.py` stamps as `system.cwd`. `None` on every existing CLI path, so `--web-bg`/`resume --web-bg` are unaffected; only the Fleet TUI's New Run screen (`fleet/launch.py`) supplies one. `_spawn_bg_child` validates `cwd` is an existing directory *before* opening the bg log files, raising a named `RuntimeError` rather than letting a bad path reach `Popen` as an indistinguishable `FileNotFoundError`. Both `launch_background` and `launch_background_resume` always invoke the child with `-P` (not a conditional env var), so the interpreter never puts the child's cwd on `sys.path[0]` -- a chosen directory containing a stray `conductor/` package would otherwise shadow the installed one -- without leaking a `PYTHONSAFEPATH` env var to the workflow's own `type: script` steps, which rebuild their environment from `os.environ` (see `executor/script.py`). The workflow path reaching `launch_background` must be absolute once a caller sets `cwd` -- see `fleet/launch.py` below, which is what actually absolutises it. - `pid.py` - **Legacy** PID file utilities, retained only so a still-running pre-upgrade background process (one that wrote its `.pid` file before Fleet Manager D2 removed `write_pid_file`) can still be discovered and cleaned up by `stop`/`fleet list`. Every current run path uses `conductor.fleet.records` (`run_id`-keyed JSON records) instead — see `fleet/records.py` below. - `self_run.py` - Answers "is this run record the run I am executing inside?" for `conductor stop`'s self-exclusion (issue #399: an agent smoke-testing `stop` must not terminate its own workflow). Three signals, first match wins: (1) `CONDUCTOR_RUN_ID` **or** `CONDUCTOR_SELF_RUN_ID` matching the record's `run_id` — the former is set on a `--web-bg` child, the latter is exported by *every* run (`cli/run.py`) into its own environment so descendants inherit it, which is the only working signal for a foreground run off Linux (no port for signal 2, no `/proc` for signal 3); the two are separate names because `engine/event_log.py` reads `CONDUCTOR_RUN_ID` as "adopt this run id", which a nested `conductor run` must not do; (2) `CONDUCTOR_WEB_BG`/`CONDUCTOR_WEB_PORT` matching the record's port, but *only* when the record has no `run_id` (the pre-#411 compatibility path — a record with a present-but-different id never falls back to this signal); (3) process ancestry (`/proc//status` `PPid:` walk + `os.getsid(0)`), POSIX-only — Windows relies on signals 1–2 alone. `partition_own_run` splits run records into `others`/`own`; `stop` targets `others` unless `--allow-self` is passed. Since the Fleet Manager it operates on `RunRecord`s rather than PID-file dicts (`read_run_records()` already surfaces legacy `.pid` files in that shape), and `reasons` is keyed by PID rather than port because a foreground run has no port. - `update.py` - Update check and version comparison. Upgrades are delegated to the install script (`install.ps1`/`install.sh`); in-process self-upgrade was removed because on Windows the running Python interpreter sits inside the venv `uv tool install --force` is trying to recreate, which fails with "Access is denied". `conductor update` prints the OS-appropriate install-script one-liner; `conductor update --apply` spawns the installer detached (Windows: new console window; POSIX: `os.execvpe` replace) and exits the current process so file locks release. It also prints `_print_extras_note` — the extras `install_hint.py::installed_extras()` reads out of the uv receipt — because `--force` rewrites the tool's entire requirement set and an upgrade that named no extras used to silently uninstall `[tui]`/`[aca]` (issue #441); both install scripts read the same receipt and rebuild the source as `conductor-cli[] @ `. The startup hint is suppressed by `CONDUCTOR_NO_UPDATE_CHECK=1`, `--silent`, `--help`/`--version`, and the `update` subcommand itself. @@ -167,9 +167,9 @@ step-by-step checklist. - `resume.py` - The fleet layer's first checkpoint consumer (issue #460): `correlate_checkpoints(entries)` joins History's `HistoryEntry` rows against `engine/checkpoint.py::CheckpointManager.list_checkpoints`'s on-disk checkpoints for the History screen's Resume action. The join key is the **event log path** (normalized via `os.path.realpath`, since both sides ultimately derive from `tempfile.gettempdir()`), with `run_id` as a documented fallback that is refused whenever that id is ambiguous across the scanned entries (a nested `conductor` invocation inherits `CONDUCTOR_RUN_ID`, so two logs can share one id). Gating a row's Resume availability is **checkpoint existence + the checkpoint's recorded `workflow_path` existing on disk — never `outcome`**: an `unknown` row from a crash is exactly the case a periodic checkpoint exists for, and a `completed` row can correlate to a stale checkpoint too (re-executing already-finished work is accepted, mitigated only by surfacing the checkpoint's provenance in the UI, not by hiding the key). Kept out of `fleet/history.py` deliberately — that module's docstring is emphatic that History is derived from event logs alone, and `build_history_entries` is patched by name in several existing tests. - `summary.py` - Derives a `RunSummary` (status/current-step/elapsed/tokens/cost/gate) from a **bounded tail** read of a run's JSONL event log (cheap enough to call once per row on the Runs screen's ~2s poll) plus a separate bounded **full**-log read (capped at 8 MiB from the start of the file) for the run-detail screen's per-agent history — an unusually large log's trailing history can fall outside that cap and be omitted, the same accepted trade-off the bounded tail read makes at the other end. Status is derived from explicit event markers (`gate_presented`/`gate_resolved`/`workflow_completed`/`workflow_failed`), never inferred from timing alone — the design's own measurement found that unreliable (228 false positives, 0 true positives). `gate_resolvable` (Fleet Manager D4) is computed once here: `True` whenever the record has a dashboard port (`fg-web`/`bg`), `False` for `mode == "fg"`. - `history.py` - Enumerates every retained run directly from `$TMPDIR/conductor/*.events.jsonl` files (not run records, which are already gone by the time a run is history) for the History screen, regardless of outcome. Classifies each log by its terminal event (`workflow_completed`/`workflow_failed`); a log with neither is `"unknown"`, **never** `"running"` — the same non-inference constraint `summary.py` follows, sharpened here since there is no run record to fall back on at all. The returned list is bounded by `[fleet.retention].keep_last` and, independently, by a fixed 200-entry display cap (so a `keep_last` configured below 1 — "unbounded" for the pruning sweep — still cannot grow the History screen without limit). Each log is read through a single-pass generator (`_read_full_log` yields one parsed event at a time to `_scan_history_events`) with no byte cap, unlike `summary.py`'s bounded readers — deliberately, because History reads a completed log exactly once and a truncated read would misclassify a finished run as `"unknown"` with incomplete totals; `_scan_history_events` must therefore stay a single forward pass over its `Iterable` argument (issue #436). - - `launch.py` - Resolves a file path or registry reference (reusing `registry/resolver.py::resolve_ref` + `registry/cache.py::resolve_and_fetch`, the same pair `conductor show` uses) and calls `cli.bg_runner.launch_background()` directly for the New Run screen — never re-implements detached process spawning, per the design's explicit warning that doing so would make a TUI-launched run die with the TUI. `launch_resume(checkpoint_path, ...)` (issue #460) is the same pattern for the History screen's Resume action: it calls `cli.bg_runner.launch_background_resume(workflow_path=None, checkpoint_path=...)` directly rather than re-implementing detached spawning, since a `HistoryEntry` carries no workflow path of its own and the checkpoint is the only route to one. + - `launch.py` - Resolves a file path or registry reference (reusing `registry/resolver.py::resolve_ref` + `registry/cache.py::resolve_and_fetch`, the same pair `conductor show` uses) and calls `cli.bg_runner.launch_background()` directly for the New Run screen — never re-implements detached process spawning, per the design's explicit warning that doing so would make a TUI-launched run die with the TUI. `launch_resume(checkpoint_path, ...)` (issue #460) is the same pattern for the History screen's Resume action: it calls `cli.bg_runner.launch_background_resume(workflow_path=None, checkpoint_path=...)` directly rather than re-implementing detached spawning, since a `HistoryEntry` carries no workflow path of its own and the checkpoint is the only route to one. Both `resolve_workflow` and `launch_workflow`/`launch_resume` take the Fleet Manager's launch directory as an explicit argument (issue #477) -- `base_dir` on the former, `cwd` on the latter two, forwarded to `cli.bg_runner.launch_background()`/`launch_background_resume()` -- rather than ever reading the process cwd, so this module stays a pure function of its arguments; `fleet/tui/app.py::FleetApp.launch_dir` is the one place that directory is actually decided. `resolve_workflow` joins a relative *file* reference onto `base_dir` (a registry reference and an absolute reference both ignore it) and always returns an absolute `ResolvedWorkflow.path` -- `Path(os.path.abspath(...))`, not `.resolve()`, matching `_resolve_agent_working_dir`'s "normpath, not resolve" convention so a symlinked project directory stays the alias the user typed. Absolutising is not optional: `launch_background` puts `str(workflow_path)` straight into a detached child's argv, and a relative path there resolves against whatever cwd that child happens to inherit, not the directory it was typed against. - `retention.py` - `prune_event_logs(keep_last, dry_run)` bounds `$TMPDIR/conductor/*.events.jsonl` (Fleet Manager D3), mirroring `CheckpointManager.rotate_periodic_checkpoints`'s `keep_last` vocabulary. Never deletes the `checkpoints/` subdirectory or an event log a live/resuming run still references; a retained/live log's `.bg.stderr.log`/`.bg.stdout.log` companions are kept or pruned alongside it. `maybe_prune_event_logs()` is the opportunistic-startup-sweep wrapper `cli/run.py` calls, gated by `[fleet.retention].enabled` and never raising. - - `tui/` - The Textual app (`app.py`, `Screen` push/pop stack) and its screens (`screens/runs.py` home; `screens/run_detail.py`; `screens/step_detail.py`; `screens/providers.py`; `screens/registries.py`; `screens/new_run.py`; `screens/history.py`; `screens/splash.py`), plus `actions.py` (shared stop/kill/gate-respond logic, reusing `cli/app.py::stop_records` and `cli/gate.py::_gate_respond_impl` rather than duplicating either), `theme.py` (the single status glyph/label/colour vocabulary — screens must not define local glyph maps), `dag.py` (step-status chips), `anim.py` (pure frame→glyph functions; animation is off by explicit `CONDUCTOR_FLEET_NO_ANIM`, on by explicit `CONDUCTOR_FLEET_ANIM`, off by default on a detected RDP session (`SESSIONNAME` starting `RDP-Tcp`) via `is_remote_session()`, and on otherwise — `CONDUCTOR_FLEET_NO_ANIM` always wins. **SSH is deliberately not detected**: it ships the ANSI byte stream for the local terminal to render (a few hundred bytes per frame), where RDP renders remotely and diffs/encodes/ships changed *pixel regions*, so only the latter is costly — and the real trigger is a *slow* link, for which there is no signal, only "SSH at all", which is usually fast. `CONDUCTOR_FLEET_NO_ANIM` is the remedy there and for VNC/Citrix/xrdp; a negative test pins this so it is not re-added as an oversight; any path that disables animation — explicit `CONDUCTOR_FLEET_NO_ANIM` or detection — also sets Textual's own `App.animation_level = "none"` in `app.py::on_mount`, issue #462), `art.py`, `widgets.py`, and `notify.py` (terminal bell / OSC 9 notifications, debounced to fire once per status transition). Optional: imported only by `conductor fleet`'s bare invocation, gated behind the `tui` extra (floor `textual>=8.0` — `widgets.py::BlockFooter` is written against 8.x `Footer` group rendering). The install command for that extra is resolved per install context by `install_hint.py`, never hardcoded. **No blocking I/O on the event loop** (issue #437): every screen that reads the filesystem *on a timer, or on a path that can be slow*, does it in a worker (`@work async def` + `await asyncio.to_thread()`), with the render half running on the event loop after the `await` — the pattern `screens/new_run.py::action_resolve`/`action_launch`, `screens/step_detail.py::load_step` and `screens/registries.py::load_workflows` established first, now also `screens/runs.py::_refresh_worker`/`_open_dashboard_worker`, `screens/run_detail.py::_refresh_worker`, `screens/history.py::load_history`, and (not a screen, but the same reasoning) `actions.py::kill_runs`. Two deliberate exceptions, both documented at their definition: `registries.py::load_registries` reads one local `registries.toml` inline, and `providers.py::load_providers` is a `@work` method awaiting a natively async `gather()` — note that `gather()` still does its offline SDK-availability imports synchronously, so it is *not* yet an instance of this pattern. The **two polled** screens (`runs.py`, `run_detail.py`) additionally keep their `refresh_runs()`/`refresh_detail()` entry point as a **synchronous dispatcher** (still callable from `on_mount`, `set_interval`, and action handlers) guarded by a plain `_refreshing` boolean, so a tick arriving mid-scan is **dropped** rather than started alongside the in-flight one — deliberately not `@work(exclusive=True)`, which would cancel the in-flight worker ("newest wins") instead of skipping the newer tick. Load-once screens (`history.py::load_history`) need no guard and are `@work` methods directly. The flag is set in the *dispatcher*, not as the worker's first line, because a `@work` body doesn't start until Textual schedules it; and it is released in a `finally`, because without that one transient error stops the screen refreshing for the rest of the session, silently and indistinguishably from a calm fleet. An **explicit** refresh (`_kill_and_refresh`, `action_resolve_gate`'s `finally`) passes `explicit=True` and is *coalesced* via `_refresh_pending` rather than dropped: those callers promise the table reflects what they just did, and the scan they collide with started before it. Every screen shows `theme.py::loading_text()` in `compose()` until its first result lands. **A failed load is surfaced, never degraded into an empty state** — rendering "No runs" or "No run history yet" for an unreadable directory is a positive claim of absence that reads as success, so `runs.py` distinguishes an empty fleet from one whose summaries all failed (`RunScan.failed`/`seen_run_ids`) and `history.py` follows `step_detail.py`/`registries.py`'s red-error-line convention. `RunScan.seen_run_ids` carries *every* record read, not just the rows that rendered, because the notifier is pruned against it — pruning against the rendered rows made a run whose summary briefly failed look brand-new and re-fire its gate notification. Test caution: a pilot test that presses a key opening a modal (kill confirm, gate options), or that deliberately suspends a worker on a `threading.Event`, and then needs a second keypress to resolve it, must use a plain `await pilot.pause()` between the two, never `tests/test_fleet/conftest.py::settle` — `settle` awaits `app.workers.wait_for_complete()`, and the suspended `@work` method won't finish until that second keypress, so awaiting it first deadlocks the test. `history.py` also correlates checkpoints in the same `load_history` worker as a second thread hop (issue #460, `conductor.fleet.resume.correlate_checkpoints`), gating its `r`/Resume binding via `check_action` the same way `runs.py` gates `g`/Gate -- hidden outright when the highlighted row has no correlated checkpoint, refreshed on `on_data_table_row_highlighted` so the footer never shows yesterday's answer as the cursor moves. **The frame timer (~10fps, `runs.py::_tick`) may only repaint what actually moves** (issue #462): the animated table cells and the preview pane's `#run-preview-score` widget. Everything else in the preview pane (the gate section, the `Progress N/M` header), and the footer's `refresh_bindings()`, belong to the ~2s data poll (`_update_gate_detail`) and to selection changes — that is the invariant a future edit is most likely to break by re-adding a convenient `_update_gate_detail()` call to `_tick`. + - `tui/` - The Textual app (`app.py`, `Screen` push/pop stack) and its screens (`screens/runs.py` home; `screens/run_detail.py`; `screens/step_detail.py`; `screens/providers.py`; `screens/registries.py`; `screens/new_run.py`; `screens/history.py`; `screens/splash.py`), plus `actions.py` (shared stop/kill/gate-respond logic, reusing `cli/app.py::stop_records` and `cli/gate.py::_gate_respond_impl` rather than duplicating either), `theme.py` (the single status glyph/label/colour vocabulary — screens must not define local glyph maps), `dag.py` (step-status chips), `anim.py` (pure frame→glyph functions; animation is off by explicit `CONDUCTOR_FLEET_NO_ANIM`, on by explicit `CONDUCTOR_FLEET_ANIM`, off by default on a detected RDP session (`SESSIONNAME` starting `RDP-Tcp`) via `is_remote_session()`, and on otherwise — `CONDUCTOR_FLEET_NO_ANIM` always wins. **SSH is deliberately not detected**: it ships the ANSI byte stream for the local terminal to render (a few hundred bytes per frame), where RDP renders remotely and diffs/encodes/ships changed *pixel regions*, so only the latter is costly — and the real trigger is a *slow* link, for which there is no signal, only "SSH at all", which is usually fast. `CONDUCTOR_FLEET_NO_ANIM` is the remedy there and for VNC/Citrix/xrdp; a negative test pins this so it is not re-added as an oversight; any path that disables animation — explicit `CONDUCTOR_FLEET_NO_ANIM` or detection — also sets Textual's own `App.animation_level = "none"` in `app.py::on_mount`, issue #462), `art.py`, `widgets.py`, and `notify.py` (terminal bell / OSC 9 notifications, debounced to fire once per status transition). Optional: imported only by `conductor fleet`'s bare invocation, gated behind the `tui` extra (floor `textual>=8.0` — `widgets.py::BlockFooter` is written against 8.x `Footer` group rendering). The install command for that extra is resolved per install context by `install_hint.py`, never hardcoded. **No blocking I/O on the event loop** (issue #437): every screen that reads the filesystem *on a timer, or on a path that can be slow*, does it in a worker (`@work async def` + `await asyncio.to_thread()`), with the render half running on the event loop after the `await` — the pattern `screens/new_run.py::action_resolve`/`action_launch`, `screens/step_detail.py::load_step` and `screens/registries.py::load_workflows` established first, now also `screens/runs.py::_refresh_worker`/`_open_dashboard_worker`, `screens/run_detail.py::_refresh_worker`, `screens/history.py::load_history`, and (not a screen, but the same reasoning) `actions.py::kill_runs`. Two deliberate exceptions, both documented at their definition: `registries.py::load_registries` reads one local `registries.toml` inline, and `providers.py::load_providers` is a `@work` method awaiting a natively async `gather()` — note that `gather()` still does its offline SDK-availability imports synchronously, so it is *not* yet an instance of this pattern. The **two polled** screens (`runs.py`, `run_detail.py`) additionally keep their `refresh_runs()`/`refresh_detail()` entry point as a **synchronous dispatcher** (still callable from `on_mount`, `set_interval`, and action handlers) guarded by a plain `_refreshing` boolean, so a tick arriving mid-scan is **dropped** rather than started alongside the in-flight one — deliberately not `@work(exclusive=True)`, which would cancel the in-flight worker ("newest wins") instead of skipping the newer tick. Load-once screens (`history.py::load_history`) need no guard and are `@work` methods directly. The flag is set in the *dispatcher*, not as the worker's first line, because a `@work` body doesn't start until Textual schedules it; and it is released in a `finally`, because without that one transient error stops the screen refreshing for the rest of the session, silently and indistinguishably from a calm fleet. An **explicit** refresh (`_kill_and_refresh`, `action_resolve_gate`'s `finally`) passes `explicit=True` and is *coalesced* via `_refresh_pending` rather than dropped: those callers promise the table reflects what they just did, and the scan they collide with started before it. Every screen shows `theme.py::loading_text()` in `compose()` until its first result lands. **A failed load is surfaced, never degraded into an empty state** — rendering "No runs" or "No run history yet" for an unreadable directory is a positive claim of absence that reads as success, so `runs.py` distinguishes an empty fleet from one whose summaries all failed (`RunScan.failed`/`seen_run_ids`) and `history.py` follows `step_detail.py`/`registries.py`'s red-error-line convention. `RunScan.seen_run_ids` carries *every* record read, not just the rows that rendered, because the notifier is pruned against it — pruning against the rendered rows made a run whose summary briefly failed look brand-new and re-fire its gate notification. Test caution: a pilot test that presses a key opening a modal (kill confirm, gate options), or that deliberately suspends a worker on a `threading.Event`, and then needs a second keypress to resolve it, must use a plain `await pilot.pause()` between the two, never `tests/test_fleet/conftest.py::settle` — `settle` awaits `app.workers.wait_for_complete()`, and the suspended `@work` method won't finish until that second keypress, so awaiting it first deadlocks the test. `history.py` also correlates checkpoints in the same `load_history` worker as a second thread hop (issue #460, `conductor.fleet.resume.correlate_checkpoints`), gating its `r`/Resume binding via `check_action` the same way `runs.py` gates `g`/Gate -- hidden outright when the highlighted row has no correlated checkpoint, refreshed on `on_data_table_row_highlighted` so the footer never shows yesterday's answer as the cursor moves. **The frame timer (~10fps, `runs.py::_tick`) may only repaint what actually moves** (issue #462): the animated table cells and the preview pane's `#run-preview-score` widget. Everything else in the preview pane (the gate section, the `Progress N/M` header), and the footer's `refresh_bindings()`, belong to the ~2s data poll (`_update_gate_detail`) and to selection changes — that is the invariant a future edit is most likely to break by re-adding a convenient `_update_gate_detail()` call to `_tick`. `app.py::FleetApp.launch_dir` is an app-level, **process-lifetime-only** Textual `reactive[Path]` (issue #477) -- no `config.toml` key, no state file, reset to the process's cwd on the next `conductor fleet` -- read by `screens/new_run.py` (the base a relative workflow reference resolves against, and the `cwd` a launched run's detached child inherits) and mutated only through `FleetApp.set_launch_dir`, matching this module's `push_*`/`return_to_runs` one-named-mutation-site convention. `actions.py::DirectoryPickerModal` (opened via the shared `change_launch_directory`, bound to `d` on Runs and `ctrl+d` -- `priority=True`, since `Input` binds it first -- on New Run) is the only way to change it; a bad path is rejected in place (a red message line, modal stays on the stack) rather than dismissed. `widgets.py::BlockFooter` passes `show_command_palette=False` to `Footer.__init__` (a real 8.x kwarg) so the Runs footer's docked `^p palette` key -- not `ctrl+p` itself, which still opens it -- is hidden, reclaiming the columns `d Dir` needed once `Providers`/`Registries` were also shortened to `Prov`/`Regs`. - **web/**: Real-time web dashboard for workflow visualization - `auth.py` - `OriginHostGuard` (issue #397): a pure-ASGI middleware (not Starlette's `BaseHTTPMiddleware` / `@app.middleware("http")`, neither of which ever sees WebSocket scopes — a decorator-style middleware would leave `/ws` completely unguarded) registered via `app.add_middleware(...)` on both `server.py` and `replay.py`. Enforces `Host`/`Origin` validation on every `http`/`websocket` scope (a present `Origin` must match; an *absent* one is allowed, since httpx/curl/`conductor gate respond` send none), then token auth + a JSON `Content-Type` on mutating HTTP routes, then token auth on the `/ws` handshake — closing it via `websocket.close` in reply to `websocket.connect`, before `accept()`, so a rejected socket can never send any message type (`gate_response`, `dialog_message`, `dialog_decline`, `iteration_limit_response` all included). `CONDUCTOR_WEB_ALLOW_ORIGINS` (comma-separated full origins) extends the allowlist for a dev server (e.g. Vite's `http://localhost:5173`) without disabling the check for anything else. A per-run token is minted automatically (`mint_token()`) so the protected configuration is the default; `resolve_expected_token(minted)` lets `CONDUCTOR_GATE_TOKEN` override it, preserving the pre-#397 escape hatch. `write_token_file`/`read_token_file`/`remove_token_file` persist that token at `~/.conductor/runs/dashboard-.token` (mode `0600` on POSIX, atomic temp-file + `os.replace`; on Windows the mode bits are not honoured, so the file is protected by the user-profile NTFS ACL instead, issue #425) so a separate CLI invocation (`conductor gate respond`, `conductor guide`, `conductor stop`'s graceful-kill rung) can discover it without a flag or env var; `resolve_cli_token(port, token)` is the shared `--token` > `CONDUCTOR_GATE_TOKEN` > token-file resolver all three call. `token_from_scope(scope)` reads `Authorization: Bearer` first, then the `token` query param — the latter exists only because a browser cannot set handshake headers, and is an acceptable exposure only because both dashboard apps run uvicorn with `log_level="warning"` (no access logs to leak the query string into). diff --git a/docs/fleet.md b/docs/fleet.md index 590ab0c1..a22bccb2 100644 --- a/docs/fleet.md +++ b/docs/fleet.md @@ -13,6 +13,7 @@ same run-discovery mechanism. - [Animation and remote sessions](#animation-and-remote-sessions) - [Screens](#screens) - [Key bindings](#key-bindings) +- [Launch directory](#launch-directory) - [Status vocabulary](#status-vocabulary) - [Gates: display vs. resolve](#gates-display-vs-resolve) - [Division of labor: TUI vs. dashboard](#division-of-labor-tui-vs-dashboard) @@ -213,9 +214,11 @@ stack rather than each managing its own navigation state. - **New run** (`n`) — enter a file path or registry reference, resolve it, and fill in a form generated from the workflow's declared `input:` block (required fields marked, defaults pre-filled, descriptions shown). - Submitting shells out to `conductor run --web-bg` via the same - `launch_background()` the CLI itself uses, so a launched run outlives - the TUI rather than dying with it. Once the launch succeeds, this screen pops + A relative reference resolves against the current + [launch directory](#launch-directory) (`ctrl+d` to change it, from either + this screen or Runs). Submitting shells out to `conductor run --web-bg` + via the same `launch_background()` the CLI itself uses, so a launched run + outlives the TUI rather than dying with it. Once the launch succeeds, this screen pops back to Runs, where the new run appears on the next poll tick; the TUI never tracks a launched run's lifecycle beyond that (**viewer, not supervisor**). @@ -264,12 +267,17 @@ Bindings shown are the Runs (home) screen's; each drill-down screen binds | `k` | Kill the selected run (confirms first) | | `K` | Kill every displayed run (confirms once) | | `g` | Resolve the selected run's open gate (see [below](#gates-display-vs-resolve)) | +| `n` | New run | +| `d` | Change the [launch directory](#launch-directory) | | `p` | Providers | | `r` | Registries | -| `n` | New run | | `h` | History | | `q` | Quit | +The Runs footer also hides the docked `^p palette` key to make room for the +above (`ctrl+p` still opens the command palette — only the footer key is +hidden, not the palette itself). + Screens with a row-scoped `enter` advertise it in their own footer: | Screen | `enter` | @@ -301,6 +309,32 @@ run discards in-flight progress unless periodic checkpoints are enabled for it (the same warning `conductor stop`'s own confirmation shows — one policy, two presentations, sharing the same underlying implementation). +## Launch directory + +`d` (Runs) / `ctrl+d` (New run) opens a directory picker — type a path, or +browse a tree rooted at the current directory's parent (so a sibling +checkout is one keypress away) — and sets the TUI's **launch directory** +for the rest of this `conductor fleet` session. + +The launch directory affects two things: + +- A **relative** workflow reference on the New Run screen resolves against + it, not against wherever `conductor fleet` happened to be started. +- A launched run's detached child inherits it as its working directory, + which is what its Directory column shows and what a `type: script` step + without an explicit `working_dir:` defaults to. + +It does **not** affect `runtime.working_dir` / `agent.working_dir` or a +sub-workflow's own workflow-file reference — those always resolve against +the *workflow file's* directory, unrelated to where the TUI itself was +launched from or later pointed at. It is also not a filter: the Runs and +History screens always show the whole fleet, regardless of the current +launch directory. + +**Process-lifetime only** — there is no `config.toml` key and no state file +behind it. It starts at `conductor fleet`'s own working directory every +time, and resets the moment this process exits. + ## Status vocabulary Status is a small explicit state machine, not a boolean: diff --git a/src/conductor/cli/bg_runner.py b/src/conductor/cli/bg_runner.py index ec46dd41..34962ada 100644 --- a/src/conductor/cli/bg_runner.py +++ b/src/conductor/cli/bg_runner.py @@ -522,6 +522,7 @@ def _spawn_detached_windows( stdout: Any, stderr: Any, stdin: Any, + cwd: Path | None = None, ) -> _WindowsDetachedProcess: """Windows ``_spawn_detached``: suspend, job-assign, then resume (issue #447). @@ -547,6 +548,8 @@ def _spawn_detached_windows( stdout: ``subprocess.DEVNULL`` or an open file-like object. stderr: ``subprocess.DEVNULL`` or an open file-like object. stdin: ``subprocess.DEVNULL`` or an open file-like object. + cwd: Working directory for the child, or ``None`` to inherit the + parent's (issue #477). Returns: A :class:`_WindowsDetachedProcess` wrapping the running child, @@ -560,6 +563,7 @@ def _spawn_detached_windows( _resolve_stdio_handle(stdout, for_write=True), _resolve_stdio_handle(stderr, for_write=True), ] + current_directory = str(cwd) if cwd is not None else None try: si = _StartupInfo() si.dwFlags |= _winapi.STARTF_USESTDHANDLES @@ -570,7 +574,7 @@ def _spawn_detached_windows( creationflags = _CREATE_NEW_PROCESS_GROUP | _CREATE_BREAKAWAY_FROM_JOB | _CREATE_SUSPENDED try: hp, ht, pid, _tid = _winapi.CreateProcess( - None, cmd_line, None, None, True, creationflags, env, None, si + None, cmd_line, None, None, True, creationflags, env, current_directory, si ) except OSError as exc: if not _is_breakaway_denied(exc): @@ -583,7 +587,7 @@ def _spawn_detached_windows( ) creationflags &= ~_CREATE_BREAKAWAY_FROM_JOB hp, ht, pid, _tid = _winapi.CreateProcess( - None, cmd_line, None, None, True, creationflags, env, None, si + None, cmd_line, None, None, True, creationflags, env, current_directory, si ) finally: for handle in handles: @@ -616,6 +620,7 @@ def _spawn_detached_posix( stdout: Any, stderr: Any, stdin: Any, + cwd: Path | None = None, ) -> subprocess.Popen[Any]: """POSIX ``_spawn_detached`` -- unchanged ``subprocess.Popen`` call. @@ -626,8 +631,19 @@ def _spawn_detached_posix( child. ``proc.pid`` is recorded in :data:`_SPAWNED_GROUP_LEADERS` so that call is only ever made against a pid this module spawned as a group leader. + + ``cwd`` (issue #477) is passed straight through to ``Popen`` -- the + child's own ``os.getcwd()`` is what ``engine/workflow.py`` stamps as + ``system.cwd``, so this is the only seam that needs to change for the + Directory column and a ``type: script`` step's default cwd to follow. """ - base: dict[str, Any] = {"stdout": stdout, "stderr": stderr, "stdin": stdin, "env": env} + base: dict[str, Any] = { + "stdout": stdout, + "stderr": stderr, + "stdin": stdin, + "env": env, + "cwd": cwd, + } proc = subprocess.Popen(cmd, **base, start_new_session=True) # noqa: S603 _SPAWNED_GROUP_LEADERS.add(proc.pid) return proc @@ -640,6 +656,7 @@ def _spawn_detached( stdout: Any = subprocess.DEVNULL, stderr: Any = subprocess.DEVNULL, stdin: Any = subprocess.DEVNULL, + cwd: Path | None = None, ) -> _DetachedChild: """Launch a fully-detached child process for ``--web-bg`` mode. @@ -661,6 +678,8 @@ def _spawn_detached( stderr: Popen ``stderr`` argument; defaults to ``DEVNULL``. Pass an open file handle to capture the child's stderr. stdin: Popen ``stdin`` argument; defaults to ``DEVNULL``. + cwd: Working directory for the child, or ``None`` to inherit the + parent's (issue #477 -- the Fleet Manager TUI's ``launch_dir``). Returns: The running detached child -- a :class:`subprocess.Popen` on @@ -672,8 +691,8 @@ def _spawn_detached( missing executable). Callers wrap this in a ``RuntimeError``. """ if sys.platform == "win32": - return _spawn_detached_windows(cmd, env, stdout=stdout, stderr=stderr, stdin=stdin) - return _spawn_detached_posix(cmd, env, stdout=stdout, stderr=stderr, stdin=stdin) + return _spawn_detached_windows(cmd, env, stdout=stdout, stderr=stderr, stdin=stdin, cwd=cwd) + return _spawn_detached_posix(cmd, env, stdout=stdout, stderr=stderr, stdin=stdin, cwd=cwd) @dataclass(frozen=True, slots=True) @@ -1874,6 +1893,7 @@ def _spawn_bg_child( web_port: int, pid_workflow_ref: Path, forced_run_id: str | None = None, + cwd: Path | None = None, ) -> BackgroundLaunch: """Open the bg log files, spawn the detached child, and finalize the launch. @@ -1894,17 +1914,35 @@ def _spawn_bg_child( ``_open_bg_log_files`` so the bg log filenames, the ``CONDUCTOR_RUN_ID`` env var, and the run-record poll key in ``_finalize_background_launch`` all agree on one id. + cwd: Working directory for the detached child (issue #477 -- the + Fleet Manager TUI's ``launch_dir``). ``None`` preserves the + child's inherited cwd. Validated to be an existing directory + *before* the bg log files are opened -- a bad ``cwd`` reaching + ``subprocess.Popen`` directly would otherwise raise a bare + ``FileNotFoundError`` indistinguishable from a missing + interpreter. Returns: ``BackgroundLaunch`` describing the live launch. Raises: - RuntimeError: If the log files cannot be created, the child fails to - start, the dashboard doesn't become reachable, or the child is - found to have exited with a non-zero code in the narrow window - between ``_finalize_background_launch`` reporting success and - this function's own final liveness check. + RuntimeError: If ``cwd`` is given and is not a directory, the log + files cannot be created, the child fails to start, the + dashboard doesn't become reachable, or the child is found to + have exited with a non-zero code in the narrow window between + ``_finalize_background_launch`` reporting success and this + function's own final liveness check. """ + if cwd is not None: + try: + ok = cwd.is_dir() + except OSError as exc: + raise RuntimeError(f"Working directory is not accessible: {cwd} ({exc})") from exc + if not ok: + if cwd.exists(): + raise RuntimeError(f"Working directory is not a directory: {cwd}") + raise RuntimeError(f"Working directory does not exist: {cwd}") + try: run_id, stderr_path, stdout_path, stderr_handle, stdout_handle = _open_bg_log_files( pid_workflow_ref, forced_run_id=forced_run_id @@ -1930,6 +1968,7 @@ def _spawn_bg_child( _build_bg_env(run_id, web_port, stderr_path, stdout_path), stdout=stdout_handle, stderr=stderr_handle, + cwd=cwd, ) except Exception as exc: raise RuntimeError( @@ -2010,6 +2049,7 @@ def launch_background( workspace_instructions: bool = False, cli_instructions: list[str] | None = None, print_loaded_instructions: bool = False, + cwd: Path | None = None, ) -> BackgroundLaunch: """Fork a detached child process running the workflow with a web dashboard. @@ -2032,6 +2072,9 @@ def launch_background( print_loaded_instructions: Whether to forward ``--print-loaded-instructions`` to the background child. Output goes to the child's captured stderr log, not to the parent's TTY. + cwd: Working directory for the detached child (issue #477); becomes + the run's recorded ``system.cwd``. ``None`` (every CLI path) + preserves the child's inherited cwd. Returns: A ``BackgroundLaunch`` describing the launch (dashboard URL, @@ -2052,6 +2095,7 @@ def launch_background( # enabled (see issue #196). cmd: list[str] = [ sys.executable, + "-P", "-m", "conductor", "run", @@ -2093,7 +2137,7 @@ def launch_background( if print_loaded_instructions: cmd.append("--print-loaded-instructions") - return _spawn_bg_child(cmd=cmd, web_port=web_port, pid_workflow_ref=workflow_path) + return _spawn_bg_child(cmd=cmd, web_port=web_port, pid_workflow_ref=workflow_path, cwd=cwd) def _peek_resume_run_id(workflow_path: Path | None, checkpoint_path: Path | None) -> str | None: @@ -2175,6 +2219,7 @@ def launch_background_resume( web_port: int = 0, metadata: dict[str, str] | None = None, guidance: list[str] | None = None, + cwd: Path | None = None, ) -> BackgroundLaunch: """Fork a detached child process resuming the workflow with a web dashboard. @@ -2198,6 +2243,9 @@ def launch_background_resume( metadata: Optional CLI metadata key=value pairs. guidance: Optional mid-run guidance text(s) to apply before the resumed agent runs. Forwarded as repeated ``--guidance`` flags. + cwd: Working directory for the detached child (issue #477); becomes + the run's recorded ``system.cwd``. ``None`` (every CLI path) + preserves the child's inherited cwd. Returns: A ``BackgroundLaunch`` describing the launch (dashboard URL, @@ -2225,6 +2273,7 @@ def launch_background_resume( # enabled (see issue #196). cmd: list[str] = [ sys.executable, + "-P", "-m", "conductor", "resume", @@ -2282,6 +2331,7 @@ def launch_background_resume( web_port=web_port, pid_workflow_ref=pid_workflow_ref, forced_run_id=forced_run_id, + cwd=cwd, ) diff --git a/src/conductor/fleet/launch.py b/src/conductor/fleet/launch.py index 3dfa5378..6facf669 100644 --- a/src/conductor/fleet/launch.py +++ b/src/conductor/fleet/launch.py @@ -28,11 +28,19 @@ #460, the History screen's Resume action) by calling :func:`conductor.cli.bg_runner.launch_background_resume` directly, for the exact same never-re-implement-spawning reason as ``launch_workflow``. + +The Fleet Manager's launch directory (issue #477, ``FleetApp.launch_dir``) is +threaded into this module as an argument -- ``base_dir`` on +:func:`resolve_workflow`, ``cwd`` on :func:`launch_workflow`/:func:`launch_resume` +-- and never read off the process itself (no ``os.getcwd()`` here). That +keeps this module a pure function of its arguments; the one place the +directory is actually decided is ``fleet/tui/app.py``. """ from __future__ import annotations import json +import os from dataclasses import dataclass from pathlib import Path from typing import TYPE_CHECKING, Any @@ -71,7 +79,7 @@ class ResolvedWorkflow: the same shape ``conductor show`` renders (``cli/app.py``).""" -def resolve_workflow(ref: str) -> ResolvedWorkflow: +def resolve_workflow(ref: str, *, base_dir: Path | None = None) -> ResolvedWorkflow: """Resolve a file path or registry reference to a launchable workflow. Mirrors ``conductor show``'s resolution exactly (``cli/app.py``): a @@ -81,9 +89,20 @@ def resolve_workflow(ref: str) -> ResolvedWorkflow: Args: ref: A local file path or registry reference (``name[@registry][#version]``). + base_dir: The directory a *relative file* reference is resolved + against (Fleet Manager E12/issue #477 -- the TUI's + ``FleetApp.launch_dir``, not the process cwd). + ``None`` preserves the prior behaviour of resolving relative to + the process's current working directory. Ignored for an + absolute reference or a registry reference, neither of which + are relative to anything. Returns: A :class:`ResolvedWorkflow` with the local path and declared inputs. + ``path`` is always absolute -- ``launch_background`` puts it + straight into a detached child's argv, so a relative path would + resolve against whatever cwd that child happens to inherit rather + than the directory it was actually typed against. Raises: LaunchError: If the reference cannot be resolved, the file does not @@ -98,13 +117,23 @@ def resolve_workflow(ref: str) -> ResolvedWorkflow: if resolved.kind == "file": assert resolved.path is not None workflow_path = resolved.path + if base_dir is not None and not workflow_path.is_absolute(): + workflow_path = base_dir / workflow_path if not workflow_path.exists(): - raise LaunchError(f"Workflow file not found: {ref}") + raise LaunchError(f"Workflow file not found: {ref} (looked for {workflow_path})") else: workflow_path = resolve_and_fetch(resolved) except RegistryError as e: raise LaunchError(str(e)) from e + # `Path(os.path.abspath(...))`, not `.resolve()`: matches this repo's + # existing "normpath, not resolve" convention + # (`_resolve_agent_working_dir`, `skills/registry.py`) so a symlinked + # project directory stays the alias the user typed rather than being + # collapsed to its real path. A registry-fetched cache path is already + # absolute, so this is a no-op for it. + workflow_path = Path(os.path.abspath(workflow_path)) + try: from conductor.config.loader import load_config as load_workflow_config @@ -217,6 +246,7 @@ def launch_workflow( provider_override: str | None = None, skip_gates: bool = False, metadata: dict[str, str] | None = None, + cwd: Path | None = None, ) -> BackgroundLaunch: """Validate/coerce inputs and launch the workflow in the background. @@ -238,6 +268,9 @@ def launch_workflow( provider_override: Optional provider name override. skip_gates: Whether to auto-select first option at human gates. metadata: Optional CLI metadata key=value pairs. + cwd: Working directory for the detached child (issue #477 -- + the TUI's ``FleetApp.launch_dir``). ``None`` preserves the + child's inherited cwd (today's behaviour). Returns: The ``BackgroundLaunch`` describing the launch. See above for the @@ -262,6 +295,7 @@ def launch_workflow( provider_override=provider_override, skip_gates=skip_gates, metadata=metadata, + cwd=cwd, ) except Exception as e: # noqa: BLE001 - surfaced as a LaunchError, not a traceback raise LaunchError(str(e)) from e @@ -273,6 +307,7 @@ def launch_resume( provider_override: str | None = None, skip_gates: bool = False, metadata: dict[str, str] | None = None, + cwd: Path | None = None, ) -> BackgroundLaunch: """Resume a workflow from an on-disk checkpoint in the background (issue #460). @@ -300,6 +335,9 @@ def launch_resume( provider_override: Optional provider name override. skip_gates: Whether to auto-select first option at human gates. metadata: Optional CLI metadata key=value pairs. + cwd: Working directory for the detached child (issue #477 -- + the TUI's ``FleetApp.launch_dir``). ``None`` preserves the + child's inherited cwd (today's behaviour). Returns: The ``BackgroundLaunch`` describing the launch. See above for the @@ -320,6 +358,7 @@ def launch_resume( provider_override=provider_override, skip_gates=skip_gates, metadata=metadata, + cwd=cwd, ) except Exception as e: # noqa: BLE001 - surfaced as a LaunchError, not a traceback raise LaunchError(str(e)) from e diff --git a/src/conductor/fleet/tui/actions.py b/src/conductor/fleet/tui/actions.py index 35241037..e8b84018 100644 --- a/src/conductor/fleet/tui/actions.py +++ b/src/conductor/fleet/tui/actions.py @@ -51,20 +51,22 @@ import subprocess import threading import webbrowser +from collections.abc import Iterable from dataclasses import dataclass from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast import typer from rich.console import Console from rich.text import Text from textual.containers import Vertical, VerticalScroll from textual.screen import ModalScreen -from textual.widgets import OptionList, Static +from textual.widgets import DirectoryTree, Input, OptionList, Static, Tree +from textual.widgets._directory_tree import DirEntry from textual.widgets.option_list import Option from conductor.cli.app import StopOutcome, _foreground_stop_warning_lines, stop_records -from conductor.console import make_console +from conductor.console import make_console, styled from conductor.fleet.records import RunRecord from conductor.fleet.summary import GateInfo @@ -72,6 +74,7 @@ from textual.app import App, ComposeResult from conductor.cli.bg_runner import BackgroundLaunch + from conductor.fleet.tui.app import FleetApp logger = logging.getLogger(__name__) @@ -705,3 +708,211 @@ async def resolve_gate(app: App, record: RunRecord, gate: GateInfo) -> GateResol port = record.port worker = app.run_worker(lambda: _resolve_gate_sync(port, choice, gate.agent_name), thread=True) return await worker.wait() + + +# --------------------------------------------------------------------------- +# Launch directory (Fleet Manager, issue #477) +# --------------------------------------------------------------------------- + + +class _DirectoryOnlyTree(DirectoryTree): + """A :class:`~textual.widgets.DirectoryTree` that lists directories only. + + :class:`DirectoryPickerModal` is a *directory* picker -- listing files + too would let one be highlighted and mirrored into the ``Input`` as if + it were a valid launch directory, which is only caught later at accept + time. Filtering here means the tree never offers a choice that could + not be accepted. + """ + + def filter_paths(self, paths: Iterable[Path]) -> Iterable[Path]: + return [path for path in paths if path.is_dir()] + + +class DirectoryPickerModal(ModalScreen[Path | None]): + """Pick a new launch directory -- the footer's ``d``/``ctrl+d`` action + (issue #477). + + Modelled on :class:`ConfirmKillModal` and :class:`GateOptionsModal`: a + centered, bordered dialog with a docked hint line and an ``escape`` + binding that cancels. Two controls that stay in sync -- typing (or + editing) a path in ``Input#dir-path``, pre-filled with the current + launch directory and focused on mount, or browsing + ``DirectoryTree#dir-tree`` -- but exactly **one** way to accept: pressing + Enter in the input. The tree's highlighted node is mirrored into the + input rather than accepted directly, so the input stays the single + source of truth for what Enter will submit. + + A bad path -- one that does not exist, or names a file rather than a + directory -- is rejected *in place*: a red message line appears and the + modal stays on the stack, dismissed only by a valid directory or + cancellation. This is deliberate: silently dismissing on a bad path + would either apply no change with no explanation, or (worse) look like + it accepted something it didn't. + """ + + DEFAULT_CSS = """ + DirectoryPickerModal { + align: center middle; + } + #dir-dialog { + /* Fixed width, `max-height` bounded -- same reasoning as + `ConfirmKillModal`/`GateOptionsModal`: an `auto`-width container + whose children are all `1fr` resolves to 0 (#449), and a tree + listing a large directory is routinely taller than the terminal. */ + width: 70; + max-width: 90%; + height: auto; + max-height: 90%; + border: thick $primary; + padding: 1 2; + } + #dir-path { + margin-bottom: 1; + } + #dir-tree { + height: 1fr; + max-height: 16; + } + #dir-message { + height: auto; + } + #dir-hint { + /* Docked, matching `ConfirmKillModal`/`GateOptionsModal`: the keys a + user needs can never be pushed off the bottom by a long tree or a + rejection message. */ + dock: bottom; + height: 1; + } + """ + + BINDINGS = [ + ("escape", "cancel", "Cancel"), + ] + + def __init__(self, current: Path) -> None: + """ + Args: + current: The launch directory to pre-fill the input with and + to derive the tree's root from. + """ + super().__init__() + self._current = current + + @staticmethod + def _tree_root(current: Path) -> Path: + """The directory tree's root: ``current``'s parent, so sibling + projects -- the common reason to switch -- are one keypress away. + + Falls back to ``current`` itself at a filesystem root, which has no + parent to climb to (``Path("/").parent == Path("/")``). + """ + parent = current.parent + return current if parent == current else parent + + def compose(self) -> ComposeResult: + yield Vertical( + Input(value=str(self._current), id="dir-path"), + _DirectoryOnlyTree(self._tree_root(self._current), id="dir-tree"), + Static(id="dir-message"), + Static( + Text.from_markup("[dim]enter accept · esc cancel[/dim]"), + id="dir-hint", + ), + id="dir-dialog", + ) + + def on_mount(self) -> None: + """Focus the input -- this is what Enter submits (E13-style: one + widget owns the accept action, matching ``GateOptionsModal`` + focusing its option list).""" + self.query_one("#dir-path", Input).focus() + + def on_tree_node_highlighted(self, event: Tree.NodeHighlighted[DirEntry]) -> None: + """Mirror the highlighted tree node's path into the input. + + The input remains the single source of truth for what Enter + accepts -- the tree is a browsing aid, not a second accept path. + """ + data = event.node.data + if data is not None: + self.query_one("#dir-path", Input).value = str(data.path) + + def on_directory_tree_directory_selected(self, event: DirectoryTree.DirectorySelected) -> None: + """Double-click / Enter-on-the-tree accepts that directory directly.""" + self._accept(str(event.path)) + + def on_input_submitted(self, event: Input.Submitted) -> None: + """Enter in the input accepts its current value (the one meaning + of Enter on this screen).""" + if event.input.id == "dir-path": + self._accept(event.value) + + def _accept(self, raw: str) -> None: + """Validate ``raw`` and dismiss with it, or reject in place. + + ``expanduser()`` then ``Path(os.path.abspath(...))`` -- not + ``.resolve()`` -- matching this repo's "normpath, not resolve" + convention (``_resolve_agent_working_dir``, ``skills/registry.py``) + so a symlinked project directory stays the alias the user typed. + A relative ``text`` is anchored on the tree's root (the parent of + ``self._current``) rather than the process cwd, so typing a bare + sibling name means the directory the tree is displaying. + """ + try: + text = raw.strip() + if not text: + self._reject("Enter a directory path, or press esc to cancel.") + return + expanded = Path(os.path.expanduser(text)) + if not expanded.is_absolute(): + expanded = self._tree_root(self._current) / expanded + candidate = Path(os.path.normpath(expanded)) + is_dir = candidate.is_dir() + except OSError as e: + self._reject(f"Cannot access {raw!r}: {e}") + return + if not is_dir: + self._reject(f"Not a directory: {candidate}") + return + self.dismiss(candidate) + + def _reject(self, message: str) -> None: + """Render ``message`` in place, leaving the modal on the stack. + + ``styled(...)``, never an f-string into ``Text.from_markup``: the + message carries a runtime path (guard rule C, + ``tests/test_cli/test_markup_guards.py``). + """ + self.query_one("#dir-message", Static).update(styled("[red]{}[/red]", message)) + + def action_cancel(self) -> None: + self.dismiss(None) + + +async def change_launch_directory(app: App) -> Path | None: + """Prompt for and apply a new launch directory (issue #477). + + Presents :class:`DirectoryPickerModal` -- awaited with + ``app.push_screen_wait`` -- pre-filled with the app's current + ``launch_dir``. On a chosen directory, calls ``FleetApp.set_launch_dir`` + (the one named mutation site) and notifies the user of the change. + Shared by the Runs screen's ``d`` binding and the New Run screen's + ``ctrl+d`` binding, exactly as :func:`kill_runs` / :func:`resolve_gate` + are already shared between screens. + + Args: + app: The running Textual app (used to push the modal and read/set + ``launch_dir``). + + Returns: + The newly chosen directory, or ``None`` if the user cancelled -- + ``launch_dir`` is left unchanged in that case. + """ + fleet_app = cast("FleetApp", app) + chosen = await app.push_screen_wait(DirectoryPickerModal(fleet_app.launch_dir)) + if chosen is None: + return None + fleet_app.set_launch_dir(chosen) + app.notify(f"Launch directory: {chosen}", markup=False) + return chosen diff --git a/src/conductor/fleet/tui/app.py b/src/conductor/fleet/tui/app.py index a1c31d30..fe859bf8 100644 --- a/src/conductor/fleet/tui/app.py +++ b/src/conductor/fleet/tui/app.py @@ -10,7 +10,11 @@ from __future__ import annotations +import os +from pathlib import Path + from textual.app import App +from textual.reactive import reactive from conductor.fleet.records import RunRecord from conductor.fleet.tui.anim import animations_enabled, disabled_reason @@ -24,6 +28,19 @@ from conductor.fleet.tui.screens.step_detail import StepDetailScreen +def _initial_launch_dir() -> Path: + """Return the process's current working directory at app startup. + + Falls back to the home directory on ``OSError`` -- the process cwd can + be deleted out from under a running process, and ``engine/workflow.py`` + guards ``os.getcwd()`` the same way for the same reason. + """ + try: + return Path(os.getcwd()) + except OSError: + return Path.home() + + class FleetApp(App): """Conductor Fleet Manager TUI application (``conductor fleet``).""" @@ -38,6 +55,30 @@ class FleetApp(App): nothing. Set it on the instance instead, which goes *through* the reactive and leaves the palette working.""" + launch_dir: reactive[Path] = reactive[Path](_initial_launch_dir, init=False) + """The directory new runs are launched from (issue #477): the base a + relative workflow reference on the New Run screen resolves against, and + the working directory a launched run's detached child inherits (hence + its Directory column and any ``type: script`` step's default cwd). + + **Process-lifetime only.** There is no ``config.toml`` key and no state + file backing this -- it starts at the process's cwd + (:func:`_initial_launch_dir`) every time ``conductor fleet`` runs, and a + change made via the footer's ``d``/``ctrl+d`` picker (see + ``fleet/tui/actions.py::DirectoryPickerModal``) is gone the moment this + process exits. It does **not** affect ``runtime.working_dir`` / + ``agent.working_dir`` or sub-workflow references, which always resolve + against the *workflow file's* directory, and it is not a filter on what + Runs/History display -- the Fleet Manager always shows the whole fleet. + + A Textual ``reactive`` (not a plain attribute) so screens can + ``self.watch(self.app, "launch_dir", callback)`` the same way + ``textual.widgets._header.Header`` watches ``app.title``/ + ``screen.sub_title``. ``init=False`` because the callable default is + evaluated once regardless, and re-running every registered watcher at + startup (Textual's ``init=True`` default) would fire before a screen's + own ``on_mount`` has registered anything to receive it.""" + CSS = """ /* ------------------------------------------------------------------ App-level design system. @@ -153,6 +194,17 @@ def on_mount(self) -> None: markup=False, ) + def set_launch_dir(self, path: Path) -> None: + """Set :attr:`launch_dir` -- the one named mutation site (issue #477). + + Called by ``fleet/tui/actions.py::change_launch_directory`` once the + directory picker modal dismisses with a chosen, validated directory. + Matches this module's ``push_*``/``return_to_runs`` convention of + centralizing state mutation in one named method rather than having + screens assign ``self.app.launch_dir`` directly. + """ + self.launch_dir = path + def push_run_detail(self, record: RunRecord) -> None: """Push the run-detail screen (E9) for ``record`` onto the screen stack. diff --git a/src/conductor/fleet/tui/screens/history.py b/src/conductor/fleet/tui/screens/history.py index 391d7940..7938245b 100644 --- a/src/conductor/fleet/tui/screens/history.py +++ b/src/conductor/fleet/tui/screens/history.py @@ -49,6 +49,7 @@ import asyncio import logging +from functools import partial from pathlib import Path from typing import TYPE_CHECKING, cast @@ -466,8 +467,12 @@ async def action_resume(self) -> None: markup=False, ) + cwd = cast("FleetApp", self.app).launch_dir + try: - launch = await asyncio.to_thread(launch_resume, target.checkpoint_path) + launch = await asyncio.to_thread( + partial(launch_resume, target.checkpoint_path, cwd=cwd) + ) except LaunchError as e: logger.warning("Failed to resume checkpoint %s", target.checkpoint_path, exc_info=True) self.notify(str(e), severity="error", markup=False) diff --git a/src/conductor/fleet/tui/screens/new_run.py b/src/conductor/fleet/tui/screens/new_run.py index 73aa508a..838e8e31 100644 --- a/src/conductor/fleet/tui/screens/new_run.py +++ b/src/conductor/fleet/tui/screens/new_run.py @@ -37,14 +37,16 @@ from rich.text import Text from textual import work from textual.app import ComposeResult +from textual.binding import Binding from textual.containers import Horizontal, Vertical, VerticalScroll from textual.screen import Screen from textual.widgets import Checkbox, Footer, Header, Input, Label, Static from conductor.config.schema import InputDef -from conductor.console import styled +from conductor.console import join, styled from conductor.fleet.launch import LaunchError, ResolvedWorkflow, launch_workflow, resolve_workflow -from conductor.fleet.tui.actions import report_background_launch +from conductor.fleet.tui.actions import change_launch_directory, report_background_launch +from conductor.fleet.tui.theme import shorten_home if TYPE_CHECKING: # The app module imports this screen module, so a top-level import of @@ -183,6 +185,13 @@ class NewRunScreen(Screen): ("escape", "back", "Back"), ("ctrl+r", "resolve", "Resolve"), ("ctrl+s", "launch", "Launch"), + # `priority=True` is required for the same reason `enter` on Runs' + # `open_detail` needs it: `Input` binds `ctrl+d` itself + # (`delete_right`, `show=False`), and as the focused widget it sits + # ahead of the screen in the binding chain. Verified empirically -- + # with priority the screen action fires and the Input's value is + # untouched; `delete` still deletes-right (issue #477). + Binding("ctrl+d", "change_dir", "Dir", priority=True), ] def __init__(self, initial_ref: str | None = None) -> None: @@ -216,6 +225,10 @@ def __init__(self, initial_ref: str | None = None) -> None: """Bumped at the start of every ``action_resolve`` call so an out-of-order (slower, superseded) resolve worker can detect it is stale and discard its result instead of overwriting a newer one.""" + self._changing_dir = False + """Guards against a second, concurrent ``ctrl+d`` press opening a + second directory-picker modal while one is already in flight, + matching ``RunsScreen``'s ``_changing_dir`` guard (issue #477).""" def action_back(self) -> None: """Pop back to the Runs screen -- bound to ``escape``.""" @@ -228,17 +241,29 @@ def _update_hint(self) -> None: ``disabled`` state: with no button, "you cannot launch yet" has to be said in words, and saying *why* is more useful than a greyed-out control was. + + Appends the current launch directory (issue #477) -- exactly where + relative references resolve against, so this is where a reader + needs to see it. That directory is runtime data, so it is built + with ``styled()``/``join()`` rather than interpolated into the + ``Text.from_markup`` literal above it (guard rules C/F, + ``tests/test_cli/test_markup_guards.py``). """ hint = self.query_one("#form-hint", Static) if self._resolved is None: - hint.update( - Text.from_markup( - "[dim]Enter a workflow reference above, then press " - "[/dim]enter[dim] to resolve it.[/dim]" - ) + first_line = Text.from_markup( + "[dim]Enter a workflow reference above, then press " + "[/dim]enter[dim] to resolve it.[/dim]" ) - return - hint.update(Text.from_markup("[dim]Press [/dim]ctrl+s[dim] to launch this workflow.[/dim]")) + else: + first_line = Text.from_markup( + "[dim]Press [/dim]ctrl+s[dim] to launch this workflow.[/dim]" + ) + directory_line = styled( + "[dim]Relative paths resolve against {} ([/dim]ctrl+d[dim] to change).[/dim]", + shorten_home(cast("FleetApp", self.app).launch_dir), + ) + hint.update(join("\n", [first_line, directory_line])) def compose(self) -> ComposeResult: yield Header() @@ -258,6 +283,7 @@ def compose(self) -> ComposeResult: def on_mount(self) -> None: """Focus the reference field, pre-filling and resolving it when the caller supplied one (the Registries drill-down's ``n``).""" + self.sub_title = shorten_home(cast("FleetApp", self.app).launch_dir) self._update_hint() ref_input = self.query_one("#workflow-ref", Input) if self._initial_ref: @@ -280,6 +306,41 @@ def on_checkbox_changed(self, event: Checkbox.Changed) -> None: if name is not None: self._checkbox_touched.add(name) + # ----------------------------------------------------------------- + # Launch directory (issue #477) + # ----------------------------------------------------------------- + + @work + async def action_change_dir(self) -> None: + """Open the directory picker -- bound to ``ctrl+d`` (``priority``, + since ``Input`` holds focus for most of this screen's life and + binds ``ctrl+d`` itself as ``delete_right``). + + Re-resolves the reference field on success, when it holds + something, so a relative reference immediately re-resolves against + the new base rather than silently pointing at the old one until + the next explicit ``ctrl+r``/Enter. Also refreshes the hint, which + names the current directory. + + Guarded by :attr:`_changing_dir` against a second, concurrent + ``ctrl+d`` press opening a second modal while one is already in + flight. + """ + if self._changing_dir: + return + self._changing_dir = True + try: + chosen = await change_launch_directory(self.app) + finally: + self._changing_dir = False + + if chosen is None: + return + self.sub_title = shorten_home(chosen) + self._update_hint() + if self.query_one("#workflow-ref", Input).value.strip(): + self.action_resolve() + @work async def action_resolve(self) -> None: """Resolve the entered workflow reference and (re)build the input @@ -317,8 +378,13 @@ async def action_resolve(self) -> None: return message.update(Text.from_markup("[dim]Resolving…[/dim]")) + # Read on the event loop *before* the thread hop -- `launch_dir` is + # a Textual reactive and the resolve itself runs in a worker + # thread, where reading app state directly would be a cross-thread + # access. + base_dir = cast("FleetApp", self.app).launch_dir try: - resolved = await asyncio.to_thread(resolve_workflow, ref) + resolved = await asyncio.to_thread(resolve_workflow, ref, base_dir=base_dir) except LaunchError as e: if generation != self._resolve_generation: return @@ -464,10 +530,11 @@ async def action_launch(self) -> None: message = self.query_one("#launch-message", Static) message.update(Text.from_markup("[dim]Launching…[/dim]")) raw_values = {name: self._raw_value_for(name) for name in resolved.inputs} + cwd = cast("FleetApp", self.app).launch_dir try: launch = await asyncio.to_thread( - launch_workflow, resolved.path, raw_values, resolved.inputs + launch_workflow, resolved.path, raw_values, resolved.inputs, cwd=cwd ) except LaunchError as e: logger.warning("Failed to launch workflow %s", resolved.path, exc_info=True) diff --git a/src/conductor/fleet/tui/screens/runs.py b/src/conductor/fleet/tui/screens/runs.py index 5ff366c0..197589aa 100644 --- a/src/conductor/fleet/tui/screens/runs.py +++ b/src/conductor/fleet/tui/screens/runs.py @@ -52,6 +52,7 @@ ) from conductor.fleet.tui.actions import ( GateResolveOutcome, + change_launch_directory, dashboard_disabled_reason, dashboard_url, gate_resolve_disabled_reason, @@ -76,6 +77,7 @@ loading_text, mode_label, muted, + shorten_home, status_badge, status_style, ) @@ -207,9 +209,7 @@ def _directory_cell(cwd: str | None) -> Text: """ if not cwd: return empty_cell() - home = str(Path.home()) - shown = f"~{cwd[len(home) :]}" if cwd.startswith(home) else cwd - return Text(shown, style="dim") + return Text(shorten_home(cwd), style="dim") def _dim_if_empty(value: str) -> Text: @@ -638,9 +638,15 @@ class RunsScreen(Screen): # longer wording ("Dashboard", "Resolve Gate") overflowed it, and an # overflowing footer does not wrap -- it truncates mid-word and drops # whatever came after, which is how `h History` disappeared entirely. - # The full set currently needs ~91 columns; `test_footer_fits_without_ - # truncation` pins that, so adding a binding fails a test rather than - # silently dropping the last one off the edge again. + # The full set now needs ~97 columns at the *gated* worst case (`g Gate` + # visible); `test_footer_fits_without_truncation` pins that against a + # gated record, so adding a binding fails a test rather than silently + # dropping the last one off the edge again. What bought the room for + # `d Dir` (issue #477): `BlockFooter` hides the docked `^p palette` key + # (`ctrl+p` still opens it -- see `BlockFooter`'s own docstring), and + # `Providers`/`Registries` were shortened to `Prov`/`Regs`. The tank is + # nearly empty after this -- the next binding will need to reclaim + # columns of its own before it can land. # # Ordered in two blocks, because these keys are not one flat set: the # first acts on whichever run the cursor is on, the second navigates the @@ -650,7 +656,9 @@ class RunsScreen(Screen): # invisible when every key has identical styling and spacing. `K` (kill # *all*) sits in the fleet block despite pairing visually with `k`: it is # fleet-scoped, and leaving the two adjacent put "kill everything" one - # stray Shift away from "kill this one". + # stray Shift away from "kill this one". `d` (change directory, issue + # #477) sits in the fleet block too -- it commands the app's launch + # directory, not the highlighted row. BINDINGS = [ # Row-scoped -- operate on the highlighted run. # @@ -665,8 +673,9 @@ class RunsScreen(Screen): ("g", "resolve_gate", "Gate"), # Fleet-scoped -- navigation and whole-fleet commands. ("n", "open_new_run", "New"), - ("p", "open_providers", "Providers"), - ("r", "open_registries", "Registries"), + ("d", "change_dir", "Dir"), + ("p", "open_providers", "Prov"), + ("r", "open_registries", "Regs"), ("h", "open_history", "History"), ("K", "kill_all", "Kill all"), ("q", "quit", "Quit"), @@ -740,12 +749,50 @@ def __init__(self) -> None: """Debounces the "could not read run records" notification to once per run of consecutive failures, so a persistently unreadable directory doesn't emit one toast per ~2s tick.""" + self._changing_dir = False + """Guards against a second, concurrent ``d`` press opening a second + directory-picker modal while one is already in flight (issue #477), + matching the ``_resolving_gate``/``_opening_dashboard`` "first one + wins" convention.""" def action_quit(self) -> None: """Quit the app -- bound to ``q`` (Textual dispatches bindings to the focused screen, not the App, so this must live here to take effect).""" self.app.exit() + # ----------------------------------------------------------------- + # Launch directory (issue #477) + # ----------------------------------------------------------------- + + @work + async def action_change_dir(self) -> None: + """Open the directory picker and apply a chosen launch directory -- + bound to ``d``. Not tied to the currently-selected run row, since + the launch directory is an app-wide setting, not a per-run action. + + Guarded by :attr:`_changing_dir` against a second, concurrent ``d`` + press opening a second modal while one is already in flight. + """ + if self._changing_dir: + return + self._changing_dir = True + try: + await change_launch_directory(self.app) + finally: + self._changing_dir = False + + def _update_sub_title(self, _old: Path | None = None, _new: Path | None = None) -> None: + """Refresh the header sub-title from ``app.launch_dir``. + + Registered with ``self.watch(...)`` in :meth:`on_mount` so a + directory change made from this screen (or from New Run) updates + the header without waiting for the next poll tick. The optional + old/new parameters match Textual's watch-callback signature but are + unused -- the sub-title is always rebuilt from the app's current + value, not from the deltas. + """ + self.sub_title = shorten_home(cast("FleetApp", self.app).launch_dir) + # ----------------------------------------------------------------- # Providers drill-down (E10-T4) # ----------------------------------------------------------------- @@ -807,6 +854,11 @@ def compose(self) -> ComposeResult: yield BlockFooter(first_block_actions=self._ROW_SCOPED_ACTIONS | {"resolve_gate"}) def on_mount(self) -> None: + self._update_sub_title() + # `init=False`: the sub-title is already set by the line above, so + # rerunning the callback immediately on registration would just + # repeat that same work. + self.watch(self.app, "launch_dir", self._update_sub_title, init=False) table = self.query_one(DataTable) # The first key is kept because `_tick` repaints that column in # place: `update_cell` addresses a column by key, not by index. diff --git a/src/conductor/fleet/tui/theme.py b/src/conductor/fleet/tui/theme.py index 6c828cc6..bfb19030 100644 --- a/src/conductor/fleet/tui/theme.py +++ b/src/conductor/fleet/tui/theme.py @@ -24,6 +24,7 @@ from __future__ import annotations from dataclasses import dataclass +from pathlib import Path from rich.text import Text @@ -139,3 +140,23 @@ def empty_cell() -> Text: def loading_text() -> Text: """Render the standard dim "still loading" placeholder.""" return Text(LOADING, style="dim") + + +def shorten_home(path: str | Path) -> str: + """Render ``path`` with the user's home directory shortened to ``~``. + + One presentation vocabulary shared across five call sites (issue + #477): the Runs screen's Directory column (previously inlined in + ``runs.py::_directory_cell``), its sub-title, and the New Run screen's + launch-directory hint and sub-title (two spots). Falls back to + ``path`` unchanged when it isn't rooted under the home directory -- + compared by path component (``Path.is_relative_to``), not a bare + string prefix, so a sibling directory that merely shares the home + directory's string prefix (e.g. ``/home/jasonx`` under + ``HOME=/home/jason``) is not mangled into ``~x``. + """ + candidate = Path(path) + home = Path.home() + if not candidate.is_relative_to(home): + return str(candidate) + return str(Path("~", candidate.relative_to(home))) diff --git a/src/conductor/fleet/tui/widgets.py b/src/conductor/fleet/tui/widgets.py index d6542643..f6ba1437 100644 --- a/src/conductor/fleet/tui/widgets.py +++ b/src/conductor/fleet/tui/widgets.py @@ -40,7 +40,7 @@ def highlighted_row_key(table: DataTable) -> str | None: class BlockFooter(Footer): """A :class:`~textual.widgets.Footer` that draws a rule between two - groups of keys. + groups of keys, with the docked command-palette key hidden (issue #477). The Runs screen's bindings are two different kinds of thing -- keys that act on the highlighted run, and keys that navigate the app or command the @@ -60,6 +60,14 @@ class BlockFooter(Footer): The divider is attached to the *first* key of the second block rather than emitted as its own widget, so it costs no additional footer columns -- the footer is a single non-wrapping line with no room to spare. + + ``show_command_palette=False`` is passed to the parent constructor for + the same reason: it is only the docked ``^p palette`` *key* that is + hidden, not the palette itself -- ``Footer.compose`` guards just the + ``FooterKey`` yield, so ``ctrl+p`` remains in ``screen.active_bindings`` + and still opens it. The reclaimed columns are what let the Runs footer's + ``d Dir`` binding (issue #477) fit at 100 columns without truncating the + keys after it. """ DEFAULT_CSS = """ @@ -87,7 +95,7 @@ def __init__( id: The ID of the widget in the DOM. classes: The CSS classes for the widget. """ - super().__init__(id=id, classes=classes) + super().__init__(id=id, classes=classes, show_command_palette=False) self._first_block_actions = frozenset(first_block_actions) def compose(self) -> ComposeResult: diff --git a/tests/test_cli/test_bg_runner.py b/tests/test_cli/test_bg_runner.py index 1182c4ea..98a255bc 100644 --- a/tests/test_cli/test_bg_runner.py +++ b/tests/test_cli/test_bg_runner.py @@ -205,8 +205,27 @@ def _fake_popen(cmd: list[str], **kwargs: Any) -> MagicMock: assert kwargs["stderr"] is subprocess.DEVNULL assert kwargs["stdin"] is subprocess.DEVNULL assert kwargs["env"] == {"X": "1"} + assert kwargs["cwd"] is None assert proc.pid in bg_runner._SPAWNED_GROUP_LEADERS + def test_posix_cwd_is_passed_through_to_popen(self, tmp_path: Path) -> None: + """Issue #477: a supplied ``cwd`` reaches ``subprocess.Popen`` -- + the child's own ``os.getcwd()`` is what ``engine/workflow.py`` + stamps as ``system.cwd``, so this is the honest seam to assert on.""" + captured: dict[str, Any] = {} + + def _fake_popen(cmd: list[str], **kwargs: Any) -> MagicMock: + captured["kwargs"] = kwargs + return MagicMock(pid=1234) + + with ( + patch.object(bg_runner.sys, "platform", "linux"), + patch.object(bg_runner.subprocess, "Popen", side_effect=_fake_popen), + ): + bg_runner._spawn_detached(["python", "-c", "pass"], {"X": "1"}, cwd=tmp_path) + + assert captured["kwargs"]["cwd"] == tmp_path + def test_windows_happy_path_includes_breakaway_and_suspended(self) -> None: winapi, kernel32 = _make_windows_mocks() @@ -220,6 +239,21 @@ def test_windows_happy_path_includes_breakaway_and_suspended(self) -> None: assert creationflags & bg_runner._CREATE_NEW_PROCESS_GROUP assert creationflags & bg_runner._CREATE_BREAKAWAY_FROM_JOB assert creationflags & bg_runner._CREATE_SUSPENDED + # `current_directory` (the 8th positional arg) is `None` when no + # `cwd` was supplied. + assert call.args[7] is None + + def test_windows_cwd_is_passed_as_current_directory(self, tmp_path: Path) -> None: + """Issue #477: a supplied ``cwd`` reaches ``_winapi.CreateProcess``'s + ``current_directory`` argument (the 8th positional).""" + winapi, kernel32 = _make_windows_mocks() + + with _patched_windows_platform(winapi, kernel32): + bg_runner._spawn_detached(["python", "-c", "pass"], {"X": "1"}, cwd=tmp_path) + + winapi.CreateProcess.assert_called_once() + call = winapi.CreateProcess.call_args + assert call.args[7] == str(tmp_path) def test_windows_happy_path_assigns_job_and_resumes_in_order(self) -> None: winapi, kernel32 = _make_windows_mocks() @@ -493,6 +527,169 @@ def test_launch_background_resume_wraps_spawn_failure_in_runtimeerror( ) +# --------------------------------------------------------------------------- +# cwd (issue #477): threading the Fleet Manager's launch directory through +# to the detached child. +# --------------------------------------------------------------------------- + + +class TestLaunchBackgroundCwd: + """``launch_background``/``launch_background_resume(cwd=...)`` reach + ``_spawn_detached``, and the child is always launched with ``-P`` so + the interpreter never puts the child's cwd on ``sys.path[0]`` + (issue #477).""" + + def test_launch_background_forwards_cwd_to_spawn_detached(self, tmp_path: Path) -> None: + wf_path = tmp_path / "wf.yaml" + wf_path.write_text("workflow: {name: x, entry_point: a}\nagents: []\n") + launch_dir = tmp_path / "launch-dir" + launch_dir.mkdir() + + fake_proc = MagicMock(pid=11) + fake_proc.poll.return_value = None + + with ( + patch.object(bg_runner, "_spawn_detached", return_value=fake_proc) as mock_spawn, + patch.object(bg_runner, "_wait_for_server", return_value=True), + patch( + "conductor.fleet.records.read_run_record", + return_value=MagicMock(pid=11, mode="bg", port=9311), + ), + patch.object(bg_runner, "_resolve_start_timeout", return_value=0.0), + ): + bg_runner.launch_background( + workflow_path=wf_path, + inputs={}, + web_port=9311, + cwd=launch_dir, + ) + + mock_spawn.assert_called_once() + assert mock_spawn.call_args.kwargs["cwd"] == launch_dir + cmd = mock_spawn.call_args.args[0] + assert "-P" in cmd + + def test_launch_background_without_cwd_still_uses_dash_p(self, tmp_path: Path) -> None: + wf_path = tmp_path / "wf.yaml" + wf_path.write_text("workflow: {name: x, entry_point: a}\nagents: []\n") + + fake_proc = MagicMock(pid=12) + fake_proc.poll.return_value = None + + with ( + patch.object(bg_runner, "_spawn_detached", return_value=fake_proc) as mock_spawn, + patch.object(bg_runner, "_wait_for_server", return_value=True), + patch( + "conductor.fleet.records.read_run_record", + return_value=MagicMock(pid=12, mode="bg", port=9312), + ), + patch.object(bg_runner, "_resolve_start_timeout", return_value=0.0), + ): + bg_runner.launch_background( + workflow_path=wf_path, + inputs={}, + web_port=9312, + ) + + assert mock_spawn.call_args.kwargs["cwd"] is None + cmd = mock_spawn.call_args.args[0] + assert "-P" in cmd + + def test_launch_background_resume_forwards_cwd_to_spawn_detached(self, tmp_path: Path) -> None: + wf_path = tmp_path / "wf.yaml" + wf_path.write_text("workflow: {name: x, entry_point: a}\nagents: []\n") + launch_dir = tmp_path / "launch-dir" + launch_dir.mkdir() + + fake_proc = MagicMock(pid=13) + fake_proc.poll.return_value = None + + with ( + patch.object(bg_runner, "_spawn_detached", return_value=fake_proc) as mock_spawn, + patch.object(bg_runner, "_wait_for_server", return_value=True), + patch( + "conductor.fleet.records.read_run_record", + return_value=MagicMock(pid=13, mode="bg", port=9313), + ), + patch.object(bg_runner, "_resolve_start_timeout", return_value=0.0), + ): + bg_runner.launch_background_resume( + workflow_path=wf_path, + checkpoint_path=None, + web_port=9313, + cwd=launch_dir, + ) + + mock_spawn.assert_called_once() + assert mock_spawn.call_args.kwargs["cwd"] == launch_dir + cmd = mock_spawn.call_args.args[0] + assert "-P" in cmd + + def test_bad_cwd_raises_before_any_spawn_is_attempted(self, tmp_path: Path) -> None: + """A ``cwd`` that is not a directory is rejected *before* the bg log + files are opened or ``_spawn_detached`` is called (issue #477) -- + otherwise a POSIX ``Popen`` with a bad cwd raises a bare + ``FileNotFoundError`` indistinguishable from a missing interpreter.""" + wf_path = tmp_path / "wf.yaml" + wf_path.write_text("workflow: {name: x, entry_point: a}\nagents: []\n") + bad_cwd = tmp_path / "does-not-exist" + + with ( + patch.object(bg_runner, "_spawn_detached") as mock_spawn, + pytest.raises(RuntimeError, match=re.escape(str(bad_cwd))), + ): + bg_runner.launch_background( + workflow_path=wf_path, + inputs={}, + web_port=9314, + cwd=bad_cwd, + ) + + mock_spawn.assert_not_called() + + def test_cwd_naming_a_file_is_also_rejected(self, tmp_path: Path) -> None: + wf_path = tmp_path / "wf.yaml" + wf_path.write_text("workflow: {name: x, entry_point: a}\nagents: []\n") + file_cwd = tmp_path / "not-a-dir.txt" + file_cwd.write_text("") + + with ( + patch.object(bg_runner, "_spawn_detached") as mock_spawn, + pytest.raises(RuntimeError, match="is not a directory"), + ): + bg_runner.launch_background( + workflow_path=wf_path, + inputs={}, + web_port=9315, + cwd=file_cwd, + ) + + mock_spawn.assert_not_called() + + def test_cwd_is_dir_raising_permission_error_is_wrapped(self, tmp_path: Path) -> None: + """Recommendation 3 (issue #477 review): ``Path.is_dir()`` can raise + ``PermissionError`` (an unsearchable ancestor directory) rather + than returning ``False`` -- that must surface as the documented + ``RuntimeError``, not an undocumented ``OSError`` subclass.""" + wf_path = tmp_path / "wf.yaml" + wf_path.write_text("workflow: {name: x, entry_point: a}\nagents: []\n") + bad_cwd = tmp_path / "unreachable" + + with ( + patch.object(Path, "is_dir", side_effect=PermissionError(13, "Permission denied")), + patch.object(bg_runner, "_spawn_detached") as mock_spawn, + pytest.raises(RuntimeError, match="is not accessible"), + ): + bg_runner.launch_background( + workflow_path=wf_path, + inputs={}, + web_port=9316, + cwd=bad_cwd, + ) + + mock_spawn.assert_not_called() + + # --------------------------------------------------------------------------- # Module-level constants # --------------------------------------------------------------------------- diff --git a/tests/test_fleet/test_launch.py b/tests/test_fleet/test_launch.py index 3dcc1a4c..fbac630e 100644 --- a/tests/test_fleet/test_launch.py +++ b/tests/test_fleet/test_launch.py @@ -19,6 +19,7 @@ from __future__ import annotations +import os from pathlib import Path from unittest.mock import patch @@ -122,6 +123,15 @@ def test_resolves_existing_file(self, workflow_file: Path) -> None: assert resolved.inputs["question"].required is True assert resolved.inputs["verbose"].default is False + def test_resolved_path_is_absolute(self, workflow_file: Path) -> None: + """Issue #477: ``launch_background`` puts ``str(workflow_path)`` + straight into a detached child's argv, so a relative path would + resolve against whatever cwd that child happens to inherit rather + than the directory it was actually typed against.""" + resolved = resolve_workflow(str(workflow_file)) + + assert resolved.path.is_absolute() + def test_nonexistent_file_raises_launch_error(self, tmp_path: Path) -> None: missing = tmp_path / "does-not-exist.yaml" @@ -136,6 +146,80 @@ def test_malformed_workflow_raises_launch_error(self, tmp_path: Path) -> None: resolve_workflow(str(bad)) +class TestResolveWorkflowBaseDir: + """``base_dir`` (issue #477): the Fleet Manager's launch directory a + relative *file* reference resolves against, instead of the process cwd.""" + + def test_relative_reference_resolves_against_base_dir(self, tmp_path: Path) -> None: + base_dir = tmp_path / "project" + base_dir.mkdir() + (base_dir / "workflow.yaml").write_text(_WORKFLOW_YAML) + + resolved = resolve_workflow("workflow.yaml", base_dir=base_dir) + + assert resolved.path == Path(os.path.abspath(base_dir / "workflow.yaml")) + assert resolved.path.is_absolute() + assert resolved.name == "test-workflow" + + def test_relative_reference_with_base_dir_none_uses_process_cwd( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """``base_dir=None`` preserves the prior behaviour: relative to the + process's current working directory.""" + (tmp_path / "workflow.yaml").write_text(_WORKFLOW_YAML) + monkeypatch.chdir(tmp_path) + + resolved = resolve_workflow("workflow.yaml") + + assert resolved.path == Path(os.path.abspath(tmp_path / "workflow.yaml")) + assert resolved.path.is_absolute() + + def test_absolute_reference_ignores_base_dir(self, workflow_file: Path, tmp_path: Path) -> None: + unrelated_base = tmp_path / "unrelated" + unrelated_base.mkdir() + + resolved = resolve_workflow(str(workflow_file), base_dir=unrelated_base) + + assert resolved.path == workflow_file + + def test_relative_reference_missing_under_base_dir_raises_launch_error( + self, tmp_path: Path + ) -> None: + base_dir = tmp_path / "project" + base_dir.mkdir() + + with pytest.raises(LaunchError, match="not found"): + resolve_workflow("does-not-exist.yaml", base_dir=base_dir) + + def test_absolute_missing_reference_with_base_dir_does_not_claim_it_was_resolved( + self, tmp_path: Path + ) -> None: + """Recommendation 5 (issue #477 review): an absolute reference is + never joined onto ``base_dir`` (``workflow_path.is_absolute()`` is + ``True``), so the not-found error must not claim it was resolved + against it.""" + base_dir = tmp_path / "project" + base_dir.mkdir() + missing_absolute = tmp_path / "nope" / "missing.yaml" + + with pytest.raises(LaunchError, match="not found") as exc_info: + resolve_workflow(str(missing_absolute), base_dir=base_dir) + + assert str(base_dir) not in str(exc_info.value) + + def test_registry_reference_ignores_base_dir( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + _registry_env(tmp_path, monkeypatch) + _configure_registry(_write_registry(tmp_path), name="my-reg") + unrelated_base = tmp_path / "unrelated" + unrelated_base.mkdir() + + resolved = resolve_workflow("test-workflow@my-reg", base_dir=unrelated_base) + + assert resolved.name == "test-workflow" + + class TestResolveWorkflowRegistry: def test_resolves_registry_reference( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch @@ -334,6 +418,24 @@ def test_forwards_optional_kwargs(self, workflow_file: Path) -> None: assert kwargs["skip_gates"] is True assert kwargs["metadata"] == {"source": "fleet-tui"} + def test_forwards_cwd(self, workflow_file: Path, tmp_path: Path) -> None: + """Issue #477: the Fleet Manager's launch directory reaches + ``launch_background`` as ``cwd``.""" + launch_dir = tmp_path / "launch-dir" + launch_dir.mkdir() + with patch("conductor.cli.bg_runner.launch_background") as fake_launch: + launch_workflow(workflow_file, {}, {}, cwd=launch_dir) + + _args, kwargs = fake_launch.call_args + assert kwargs["cwd"] == launch_dir + + def test_cwd_defaults_to_none(self, workflow_file: Path) -> None: + with patch("conductor.cli.bg_runner.launch_background") as fake_launch: + launch_workflow(workflow_file, {}, {}) + + _args, kwargs = fake_launch.call_args + assert kwargs["cwd"] is None + def test_required_field_rejected_before_launch_background_is_called( self, workflow_file: Path ) -> None: @@ -414,6 +516,26 @@ def test_forwards_optional_kwargs(self, tmp_path: Path) -> None: assert kwargs["skip_gates"] is True assert kwargs["metadata"] == {"source": "fleet-tui"} + def test_forwards_cwd(self, tmp_path: Path) -> None: + """Issue #477: the Fleet Manager's launch directory reaches + ``launch_background_resume`` as ``cwd``.""" + checkpoint_path = tmp_path / "checkpoint.json" + launch_dir = tmp_path / "launch-dir" + launch_dir.mkdir() + with patch("conductor.cli.bg_runner.launch_background_resume") as fake_launch: + launch_resume(checkpoint_path, cwd=launch_dir) + + _args, kwargs = fake_launch.call_args + assert kwargs["cwd"] == launch_dir + + def test_cwd_defaults_to_none(self, tmp_path: Path) -> None: + checkpoint_path = tmp_path / "checkpoint.json" + with patch("conductor.cli.bg_runner.launch_background_resume") as fake_launch: + launch_resume(checkpoint_path) + + _args, kwargs = fake_launch.call_args + assert kwargs["cwd"] is None + def test_launch_background_resume_failure_surfaces_as_launch_error( self, tmp_path: Path ) -> None: diff --git a/tests/test_fleet/test_tui_actions.py b/tests/test_fleet/test_tui_actions.py index e923f9b8..6626bbba 100644 --- a/tests/test_fleet/test_tui_actions.py +++ b/tests/test_fleet/test_tui_actions.py @@ -1275,3 +1275,260 @@ async def test_cancel_still_works_while_the_scroll_holds_focus( await pilot.pause() mock_kill.assert_not_called() + + +# --------------------------------------------------------------------------- +# DirectoryPickerModal (issue #477) +# --------------------------------------------------------------------------- + + +class TestDirectoryPickerModal: + """The launch-directory picker: reject-in-place validation, ``~`` + expansion, directories-only tree filtering, and dismiss-with-Path on a + valid directory.""" + + @staticmethod + async def _push(app: FleetApp, current: Path) -> dict[str, object]: + """Push :class:`DirectoryPickerModal` via ``push_screen_wait`` in a + worker, mirroring ``TestGateOptionsModalMarkupSafety``'s pattern -- + a plain ``push_screen`` wouldn't let the test read the eventual + dismiss result.""" + result_holder: dict[str, object] = {} + + async def _push_modal() -> None: + result_holder["result"] = await app.push_screen_wait( + tui_actions.DirectoryPickerModal(current) + ) + + app.run_worker(_push_modal()) + return result_holder + + async def test_nonexistent_path_is_rejected_in_place( + self, fleet_env: Path, tmp_path: Path + ) -> None: + from textual.widgets import Input, Static + + app = FleetApp() + async with app.run_test() as pilot: + result_holder = await self._push(app, tmp_path) + await pilot.pause() + + modal = app.screen + bad_path = tmp_path / "does-not-exist" + modal.query_one("#dir-path", Input).value = str(bad_path) + await pilot.press("enter") + await pilot.pause() + + # Rejected in place: still on the stack, dismiss not called. + assert app.screen is modal + assert "result" not in result_holder + message = str(modal.query_one("#dir-message", Static).render()) + assert str(bad_path) in message + + await pilot.press("escape") + await settle(pilot) + assert result_holder["result"] is None + + async def test_empty_input_is_rejected_in_place(self, fleet_env: Path, tmp_path: Path) -> None: + """Blocking finding 3 (issue #477 review): clearing the pre-filled + input and pressing Enter must not silently dismiss with the + *process* cwd (``os.path.abspath("")`` -- what an empty string + maps to).""" + from textual.widgets import Input + + app = FleetApp() + async with app.run_test() as pilot: + result_holder = await self._push(app, tmp_path) + await pilot.pause() + + modal = app.screen + modal.query_one("#dir-path", Input).value = "" + await pilot.press("enter") + await pilot.pause() + + # Rejected in place: still on the stack, dismiss not called. + assert app.screen is modal + assert "result" not in result_holder + + await pilot.press("escape") + await settle(pilot) + assert result_holder["result"] is None + + async def test_path_naming_a_file_is_rejected_in_place( + self, fleet_env: Path, tmp_path: Path + ) -> None: + from textual.widgets import Input, Static + + a_file = tmp_path / "not-a-dir.txt" + a_file.write_text("") + + app = FleetApp() + async with app.run_test() as pilot: + result_holder = await self._push(app, tmp_path) + await pilot.pause() + + modal = app.screen + modal.query_one("#dir-path", Input).value = str(a_file) + await pilot.press("enter") + await pilot.pause() + + assert app.screen is modal + assert "result" not in result_holder + message = str(modal.query_one("#dir-message", Static).render()) + assert str(a_file) in message + + await pilot.press("escape") + await settle(pilot) + + async def test_tilde_is_expanded( + self, fleet_env: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + from textual.widgets import Input + + home = tmp_path / "home" + home.mkdir() + sub = home / "project" + sub.mkdir() + monkeypatch.setenv("HOME", str(home)) + + app = FleetApp() + async with app.run_test() as pilot: + result_holder = await self._push(app, tmp_path) + await pilot.pause() + + modal = app.screen + modal.query_one("#dir-path", Input).value = "~/project" + await pilot.press("enter") + await settle(pilot) + + assert result_holder["result"] == Path(os.path.abspath(sub)) + + async def test_relative_input_resolves_against_current_directory( + self, fleet_env: Path, tmp_path: Path + ) -> None: + """Recommendation 2 (issue #477 review): a relative path typed into + the picker must anchor on the directory the picker is currently + showing (``self._current``), not the process cwd -- otherwise a + sibling name typed while browsing ``/work/a`` resolves against + wherever ``conductor fleet`` happened to be started from.""" + from textual.widgets import Input + + work = tmp_path / "work" + work.mkdir() + sibling_a = work / "a" + sibling_a.mkdir() + sibling_b = work / "b" + sibling_b.mkdir() + + app = FleetApp() + async with app.run_test() as pilot: + result_holder = await self._push(app, sibling_a) + await pilot.pause() + + modal = app.screen + modal.query_one("#dir-path", Input).value = "b" + await pilot.press("enter") + await settle(pilot) + + assert result_holder["result"] == Path(os.path.abspath(sibling_b)) + + async def test_valid_directory_dismisses_with_absolute_path( + self, fleet_env: Path, tmp_path: Path + ) -> None: + from textual.widgets import Input + + chosen = tmp_path / "chosen" + chosen.mkdir() + + app = FleetApp() + async with app.run_test() as pilot: + result_holder = await self._push(app, tmp_path) + await pilot.pause() + + modal = app.screen + modal.query_one("#dir-path", Input).value = str(chosen) + await pilot.press("enter") + await settle(pilot) + + assert app.screen is not modal + + result = result_holder["result"] + assert isinstance(result, Path) + assert result == chosen + assert result.is_absolute() + + async def test_escape_dismisses_with_none(self, fleet_env: Path, tmp_path: Path) -> None: + app = FleetApp() + async with app.run_test() as pilot: + result_holder = await self._push(app, tmp_path) + await pilot.pause() + + await pilot.press("escape") + await settle(pilot) + + assert result_holder["result"] is None + + async def test_filter_paths_yields_directories_only(self, tmp_path: Path) -> None: + a_dir = tmp_path / "a_dir" + a_dir.mkdir() + a_file = tmp_path / "a_file.txt" + a_file.write_text("") + + tree = tui_actions._DirectoryOnlyTree(tmp_path) + filtered = list(tree.filter_paths([a_dir, a_file])) + + assert filtered == [a_dir] + + +# --------------------------------------------------------------------------- +# change_launch_directory (issue #477) +# --------------------------------------------------------------------------- + + +class TestChangeLaunchDirectory: + """The shared helper both the Runs and New Run screens' ``d``/``ctrl+d`` + bindings dispatch through.""" + + async def test_applies_the_chosen_directory(self, fleet_env: Path, tmp_path: Path) -> None: + from textual.widgets import Input + + chosen = tmp_path / "chosen" + chosen.mkdir() + + app = FleetApp() + async with app.run_test() as pilot: + result_holder: dict[str, object] = {} + + async def _call() -> None: + result_holder["result"] = await tui_actions.change_launch_directory(app) + + app.run_worker(_call()) + await pilot.pause() + + modal = app.screen + modal.query_one("#dir-path", Input).value = str(chosen) + await pilot.press("enter") + await settle(pilot) + + assert app.launch_dir == chosen + assert result_holder["result"] == chosen + + async def test_cancelling_leaves_launch_dir_unchanged( + self, fleet_env: Path, tmp_path: Path + ) -> None: + app = FleetApp() + async with app.run_test() as pilot: + original = app.launch_dir + result_holder: dict[str, object] = {} + + async def _call() -> None: + result_holder["result"] = await tui_actions.change_launch_directory(app) + + app.run_worker(_call()) + await pilot.pause() + + await pilot.press("escape") + await settle(pilot) + + assert app.launch_dir == original + assert result_holder["result"] is None diff --git a/tests/test_fleet/test_tui_history.py b/tests/test_fleet/test_tui_history.py index 7759a66b..814fbb8f 100644 --- a/tests/test_fleet/test_tui_history.py +++ b/tests/test_fleet/test_tui_history.py @@ -713,6 +713,45 @@ async def test_pressing_r_launches_resume_and_returns_to_runs( assert kwargs["workflow_path"] is None assert kwargs["checkpoint_path"] == cp_path + async def test_pressing_r_forwards_app_launch_dir_as_cwd( + self, fleet_env: Path, event_log_dir: Path, resume_workflow_file: Path + ) -> None: + """Blocking finding 1 (issue #477 review): ``launch_resume``'s + ``cwd`` parameter is only useful if the History screen actually + supplies it -- assert the app's current ``launch_dir`` reaches the + detached child's spawn call.""" + log_path = _write_log( + event_log_dir, + name="resumable-workflow", + run_id="00002222", + lines=[_event("workflow_started", {"name": "resumable-workflow"}, ts=1000.0)], + ) + _write_checkpoint(resume_workflow_file, event_log_path=str(log_path), run_id="00002222") + + launch = BackgroundLaunch( + url="http://127.0.0.1:8080", + stderr_log=event_log_dir / "00002222.bg.stderr.log", + stdout_log=event_log_dir / "00002222.bg.stdout.log", + run_id="00002222", + ) + fake_launch = Mock(return_value=launch) + launch_dir = resume_workflow_file.parent + + app = FleetApp() + async with app.run_test() as pilot: + with patch("conductor.cli.bg_runner.launch_background_resume", fake_launch): + app.set_launch_dir(launch_dir) + await _goto_history(pilot) + table = app.screen.query_one(DataTable) + table.move_cursor(row=0) + + await pilot.press("r") + await settle(pilot) + + fake_launch.assert_called_once() + _args, kwargs = fake_launch.call_args + assert kwargs["cwd"] == launch_dir + async def test_pressing_r_resumes_the_highlighted_rows_checkpoint( self, fleet_env: Path, event_log_dir: Path, resume_workflow_file: Path ) -> None: diff --git a/tests/test_fleet/test_tui_new_run.py b/tests/test_fleet/test_tui_new_run.py index 58b9156c..006b099d 100644 --- a/tests/test_fleet/test_tui_new_run.py +++ b/tests/test_fleet/test_tui_new_run.py @@ -768,7 +768,7 @@ async def test_out_of_order_resolve_does_not_overwrite_newer_result( real_resolve_workflow = launch_module.resolve_workflow second_call_started = threading.Event() - def _tracking_resolve_workflow(ref: str): + def _tracking_resolve_workflow(ref: str, *, base_dir: Path | None = None) -> object: if ref == str(fixture_workflow): # The first (slower) resolve waits for the second call to # start before returning, so it finishes *after* it -- @@ -776,7 +776,7 @@ def _tracking_resolve_workflow(ref: str): second_call_started.wait(timeout=2) else: second_call_started.set() - return real_resolve_workflow(ref) + return real_resolve_workflow(ref, base_dir=base_dir) with pytest.MonkeyPatch.context() as mp: mp.setattr( @@ -799,3 +799,89 @@ def _tracking_resolve_workflow(ref: str): assert app.screen._resolved is not None assert app.screen._resolved.name == "dotted-name-workflow" + + +# --------------------------------------------------------------------------- +# Launch directory (issue #477) +# --------------------------------------------------------------------------- + + +class TestNewRunLaunchDirectory: + """The New Run screen threads ``FleetApp.launch_dir`` through to + ``resolve_workflow``/``launch_workflow`` and offers ``ctrl+d`` to + change it.""" + + async def test_ctrl_d_opens_picker_while_input_has_focus_and_leaves_it_untouched( + self, fleet_env: Path + ) -> None: + """Pins ``priority=True``: a bare ``d`` cannot work here because the + reference ``Input`` holds focus for most of this screen's life and + binds ``ctrl+d`` itself (``delete_right``, ``show=False``).""" + from conductor.fleet.tui.actions import DirectoryPickerModal + + app = FleetApp() + async with app.run_test() as pilot: + await _goto_new_run(pilot) + ref_input = app.screen.query_one("#workflow-ref", Input) + ref_input.value = "hello" + assert app.focused is ref_input + + await pilot.press("ctrl+d") + await pilot.pause() + + assert isinstance(app.screen, DirectoryPickerModal) + await pilot.press("escape") + await settle(pilot) + + assert isinstance(app.screen, NewRunScreen) + assert app.screen.query_one("#workflow-ref", Input).value == "hello" + + async def test_resolve_uses_app_launch_dir_as_base_dir( + self, fleet_env: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + launch_dir = tmp_path / "launch-dir" + launch_dir.mkdir() + (launch_dir / "relative.yaml").write_text(_FIXTURE_WORKFLOW_YAML) + + app = FleetApp() + async with app.run_test() as pilot: + app.set_launch_dir(launch_dir) + await _goto_new_run(pilot) + await _resolve(pilot, Path("relative.yaml")) + + assert app.screen._resolved is not None + assert app.screen._resolved.path == Path(os.path.abspath(launch_dir / "relative.yaml")) + + async def test_launch_forwards_app_launch_dir_as_cwd( + self, fleet_env: Path, fixture_workflow: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + launch_dir = fixture_workflow.parent + fake_result = Mock(url="http://127.0.0.1:8080") + launch_mock = Mock(return_value=fake_result) + monkeypatch.setattr("conductor.fleet.tui.screens.new_run.launch_workflow", launch_mock) + + app = FleetApp() + async with app.run_test() as pilot: + app.set_launch_dir(launch_dir) + await _goto_new_run(pilot) + await _resolve(pilot, fixture_workflow) + + question_input = app.screen._input_widgets["question"] + question_input.value = "What is Python?" + + await pilot.press("ctrl+s") + await pilot.pause(0.3) + + launch_mock.assert_called_once() + _args, kwargs = launch_mock.call_args + assert kwargs["cwd"] == launch_dir + + async def test_hint_names_the_current_launch_directory(self, fleet_env: Path) -> None: + from conductor.fleet.tui.theme import shorten_home + + app = FleetApp() + async with app.run_test() as pilot: + await _goto_new_run(pilot) + + hint = str(app.screen.query_one("#form-hint", Static).render()) + assert shorten_home(app.launch_dir) in hint diff --git a/tests/test_fleet/test_tui_runs.py b/tests/test_fleet/test_tui_runs.py index 0795e69b..a2714259 100644 --- a/tests/test_fleet/test_tui_runs.py +++ b/tests/test_fleet/test_tui_runs.py @@ -1674,7 +1674,7 @@ async def test_kill_all_is_not_adjacent_to_kill(self) -> None: assert keys.index("K") > keys.index("k") + 1 row_scoped = ["enter", "w", "k", "g"] - fleet_scoped = ["n", "p", "r", "h", "K", "q"] + fleet_scoped = ["n", "d", "p", "r", "h", "K", "q"] assert keys == row_scoped + fleet_scoped async def test_a_rule_is_drawn_between_the_two_blocks( @@ -1712,7 +1712,9 @@ async def test_no_leading_rule_on_an_empty_fleet(self, fleet_env: Path) -> None: assert _BLOCK_RULE not in line[: line.index("n New")], line async def test_footer_fits_without_truncation(self, fleet_env: Path, tmp_path: Path) -> None: - """Every visible footer key must fit inside the footer. + """Every visible footer key must fit inside the footer, at the + *gated* worst case -- the widest the row-scoped block gets, since + `g Gate` is the one row-scoped key that isn't always shown. The footer is a single non-wrapping line: once the keys overrun it, Textual clips the tail rather than wrapping, which is how `h History` @@ -1722,13 +1724,29 @@ async def test_footer_fits_without_truncation(self, fleet_env: Path, tmp_path: P Checked at 100 columns -- comfortably under the width the 12-column runs table itself needs, so this is a floor on the footer, not an - assertion about the terminal anyone actually uses. + assertion about the terminal anyone actually uses. What buys the + room at this width (issue #477): the Runs footer hides the docked + `^p palette` key (`BlockFooter(show_command_palette=False)`) and + `Providers`/`Registries` are shortened to `Prov`/`Regs`. """ - _write_record(tmp_path, "aaaa0016", workflow_name="plain") + log = tmp_path / "gated.events.jsonl" + _write_events( + log, + [ + _event("workflow_started", {"workflow_name": "plain"}), + _event("agent_started", {"agent_name": "planner"}), + _event("gate_presented", {"agent_name": "planner", "prompt": "Approve?"}), + ], + ) + _write_record(tmp_path, "aaaa0016", workflow_name="plain", event_log_path=str(log)) app = FleetApp() async with app.run_test(size=(100, 30)) as pilot: await settle(pilot) + # Exercise the actual worst case rather than assuming it: `g` + # must be genuinely visible here, or this test isn't pinning + # anything. + assert app.screen.check_action("resolve_gate", ()) is True footer = app.screen.query_one(Footer) overflowing = [ (child.region.x, child.region.right, getattr(child, "description", "?")) @@ -1737,6 +1755,22 @@ async def test_footer_fits_without_truncation(self, fleet_env: Path, tmp_path: P ] assert not overflowing, f"footer keys clipped at 100 cols: {overflowing}" + async def test_footer_has_no_docked_command_palette_key( + self, fleet_env: Path, tmp_path: Path + ) -> None: + """The Runs footer hides the docked `^p palette` `FooterKey` to make + room for `d Dir` (issue #477) -- but `ctrl+p` must still open the + palette; only the footer key is hidden, not the binding itself.""" + _write_record(tmp_path, "aaaa0018", workflow_name="plain") + + app = FleetApp() + async with app.run_test(size=(140, 30)) as pilot: + await settle(pilot) + footer = app.screen.query_one(Footer) + descriptions = [getattr(child, "description", "") for child in footer.children] + assert not any("palette" in d.lower() for d in descriptions) + assert "ctrl+p" in app.screen.active_bindings + async def test_detail_binding_survives_datatables_own_enter( self, fleet_env: Path, tmp_path: Path ) -> None: @@ -2147,3 +2181,66 @@ def _tracking_derive(record: RunRecord): assert seen_main_thread, "the gate re-read never ran" assert not any(seen_main_thread), "the gate re-read blocked the event loop" + + +# --------------------------------------------------------------------------- +# Change launch directory (issue #477) +# --------------------------------------------------------------------------- + + +class TestChangeDirBinding: + """``d`` opens the directory picker (``RunsScreen.action_change_dir``); + a chosen directory lands in ``app.launch_dir`` and the screen's + ``sub_title``, and cancelling leaves both unchanged.""" + + async def test_d_opens_picker_and_applies_chosen_directory( + self, fleet_env: Path, tmp_path: Path + ) -> None: + from textual.widgets import Input + + from conductor.fleet.tui.actions import DirectoryPickerModal + from conductor.fleet.tui.theme import shorten_home + + _write_record(tmp_path, "aaaa0022", workflow_name="plain") + chosen = tmp_path / "chosen" + chosen.mkdir() + + app = FleetApp() + async with app.run_test() as pilot: + await settle(pilot) + await pilot.press("d") + # Two keypresses resolving one modal -- must use a plain + # `pilot.pause()`, never `settle()` (AGENTS.md test caution): + # `settle` awaits `workers.wait_for_complete()`, and the + # suspended `action_change_dir` worker cannot finish until the + # second keypress below. + await pilot.pause() + + assert isinstance(app.screen, DirectoryPickerModal) + app.screen.query_one("#dir-path", Input).value = str(chosen) + await pilot.press("enter") + await settle(pilot) + + assert app.launch_dir == chosen + assert isinstance(app.screen, RunsScreen) + assert app.screen.sub_title == shorten_home(chosen) + + async def test_cancelling_leaves_launch_dir_unchanged( + self, fleet_env: Path, tmp_path: Path + ) -> None: + _write_record(tmp_path, "aaaa0023", workflow_name="plain") + + app = FleetApp() + async with app.run_test() as pilot: + await settle(pilot) + original = app.launch_dir + original_sub_title = app.screen.sub_title + + await pilot.press("d") + await pilot.pause() + await pilot.press("escape") + await settle(pilot) + + assert app.launch_dir == original + assert isinstance(app.screen, RunsScreen) + assert app.screen.sub_title == original_sub_title diff --git a/tests/test_fleet/test_tui_theme.py b/tests/test_fleet/test_tui_theme.py index d000483a..3f08c359 100644 --- a/tests/test_fleet/test_tui_theme.py +++ b/tests/test_fleet/test_tui_theme.py @@ -8,6 +8,7 @@ from __future__ import annotations +import pytest from rich.text import Text from conductor.fleet.tui.theme import ( @@ -16,6 +17,7 @@ empty_cell, mode_label, muted, + shorten_home, status_badge, status_label, status_style, @@ -91,3 +93,24 @@ def test_renderers_return_text_not_markup_strings(self) -> None: codebase-wide rule these screens have to honour.""" for value in (status_badge("failed"), status_label("failed"), mode_label("bg"), muted("x")): assert isinstance(value, Text) + + +class TestShortenHome: + """Recommendation 9 (issue #477 review): ``shorten_home`` must compare + by path component, not a bare string prefix -- a string-prefix test + mangles a sibling home directory that merely shares a prefix (e.g. + ``/home/jasonx`` under ``HOME=/home/jason``) into ``~x``.""" + + def test_path_under_home_is_shortened(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("HOME", "/home/jason") + assert shorten_home("/home/jason/src/proj") == "~/src/proj" + + def test_path_outside_home_is_unchanged(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("HOME", "/home/jason") + assert shorten_home("/tmp/a") == "/tmp/a" + + def test_sibling_directory_sharing_home_prefix_is_not_mangled( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("HOME", "/home/jason") + assert shorten_home("/home/jasonx/proj") == "/home/jasonx/proj"