diff --git a/AGENTS.md b/AGENTS.md index 15c8af78..c5c45327 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -92,13 +92,13 @@ step-by-step checklist. - `gate.py` - `gate` group (`gate respond`) + shared `_gate_respond_impl` (modeled on `registry.py`). Also consumed directly by the Fleet Manager TUI's gate-resolve action (`conductor.fleet.tui.actions.resolve_gate`) — see the fleet bullet below. - `registry.py` - `registry` group (`list` / `add` / `remove` / `set-default` / `update` / `show`) - `plugin.py` - `plugin` group (`list` / `fetch`). Deliberately no `update`: a floating ref self-updates and a pinned one is meant not to. `fetch` existing as a separate verb is what keeps `conductor validate` off the network - - `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 (`pip install 'conductor-cli[tui]'`); 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. `fleet list`/`fleet prune` need no optional dependency. See `src/conductor/fleet/` below for the TUI itself. + - `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 `pid`/`mode`/`port`). 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 a foreign `pid`) 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. - `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. The startup hint is suppressed by `CONDUCTOR_NO_UPDATE_CHECK=1`, `--silent`, `--help`/`--version`, and the `update` subcommand itself. + - `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. - **config/**: YAML loading and Pydantic schema validation - `schema.py` - Pydantic models for all workflow YAML structures (WorkflowConfig, AgentDef, ParallelGroup, ForEachDef, etc.) @@ -168,7 +168,7 @@ step-by-step checklist. - `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. - `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, disabled by `CONDUCTOR_FLEET_NO_ANIM`), `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 (`pip install 'conductor-cli[tui]'`, floor `textual>=8.0` — `widgets.py::BlockFooter` is written against 8.x `Footer` group rendering). + - `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, disabled by `CONDUCTOR_FLEET_NO_ANIM`), `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. - **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). @@ -176,6 +176,8 @@ step-by-step checklist. - `frontend/` - React 19 + TypeScript dashboard source (Vite + Tailwind). Renders the workflow DAG with **React Flow** (`@xyflow/react`) laid out via dagre, a Zustand store (`stores/workflow-store.ts`), an agent detail panel, and streaming activity. Build with `make build-frontend` (outputs to `static/`); unit tests via `make test-frontend` (Vitest). Subworkflow nodes support inline expand/collapse (nested React Flow containers, collapsed by default) in addition to double-click drill-down — see issue #314. `lib/auth.ts` (issue #397) reads `window.__CONDUCTOR_TOKEN__` (`getToken()`), builds the `Authorization` header for mutating fetches (`authHeaders()`), and appends `?token=` to the `/ws` URL (`withToken()`, since the WebSocket handshake can't carry custom headers). `ReplayDashboard`'s `/` injects nothing, so `getToken()` returning `undefined` there is expected and harmless — that app has no mutating routes or `/ws` to authenticate. In dev, `vite.config.ts`'s `transformIndexHtml` hook (scoped to `apply: 'serve'`, not the production build) injects `process.env.CONDUCTOR_GATE_TOKEN` the same way, and the `/ws` proxy entry needs `changeOrigin: true` (the `/api` proxy already had it) or the handshake arrives with `Host: localhost:5173` and fails `OriginHostGuard`'s host check. - `static/` - Built dashboard assets served by `server.py` (generated from `frontend/` by `make build-frontend`; committed to the repo). Not edited by hand. +- **install_hint.py**: `install_command(extra)` — resolves a working install command for an optional extra (`tui` / `aca` / `claude-agent-sdk`) from the *detected* install context, plus `read_receipt()` / `installed_extras()` which parse `/uv-receipt.toml`. A stdlib-only leaf (like `duration.py`, `console.py`, `rundir.py`) deliberately **top-level rather than under `cli/`**, because `providers/` needs it and must not import from `cli/`. Three branches: a uv receipt → `uv tool install --force ''`; an editable `direct_url.json` → `uv sync --inexact --extra `; neither → `pip install 'conductor-cli[]'`. The receipt is checked **first** because a uv tool install writes *both* files. Every hint used to hardcode the pip form, which cannot work on the documented install path — a uv tool venv is not pip-managed and `conductor-cli` is not on PyPI, so pip has nothing to resolve against unless the distribution is already installed in the target environment (issue #441). It survives as the last-resort fallback, rendered as ` -m pip install` (a bare `pip` does not manage a pipx venv, where it would *succeed* while installing a second copy the user never runs) with the git URL from `direct_url.json` appended when there is one, so a `pip`/`pipx`-from-git install — which is what actually lands in the `UNKNOWN` branch — still resolves. PEP 610's `archive_info` is deliberately *not* used as a source: a wheel or sdist is one artifact, usually a download since cleaned up, so pinning to it emits a command that fails. Four things are load-bearing: already-recorded extras are **unioned** with the requested one (`--force` rewrites the tool's whole requirement set, and `uv sync` is exact unless given `--inexact`, so naming only the new extra uninstalls the others); the **recorded install source is reused** (the receipt's `git`/`directory`/`url` key, else `direct_url.json`, else upstream pinned to `v`) so the hint never redirects a fork or a locally-built install at upstream's released tag; a receipt that exists but **cannot be read or understood** is `readable=False` rather than an empty extras set, because the two would otherwise render the same command and one of them silently uninstalls the user's extras — that case appends an inline `# WARNING` shell comment naming the receipt (carried on `InstallEnvironment` so the renderer stays pure); the two install scripts draw the same line and **warn and continue** rather than aborting, since a broken receipt is exactly the state a reinstall repairs; and `install_command` **never raises**, since it runs inside a `ProviderError(...)` argument expression where an exception deletes the diagnosis instead of degrading the hint. Branch logic lives in the pure `render_install_command(extra, InstallEnvironment)` so all three contexts are unit-testable without a real install; `detect_environment()` is the impure half, and `InstallEnvironment.__post_init__` drops extras/source on the editable branch so the type cannot hold state the renderer ignores. `install.sh`/`install.ps1` parse the same receipt in shell for the same reason — see `receipt_extras` / `Get-ReceiptExtras`, whose agreement with this module is asserted by a shared-oracle test rather than assumed. + - **rundir.py**: `runs_dir() -> Path` — the single definition of `~/.conductor/runs/` (`mkdir(parents=True, exist_ok=True)`). A stdlib-only leaf (like `duration.py`, `console.py`) so `web/auth.py` can depend on it without importing `conductor.cli.pid`, which would drag in the whole Typer app (`conductor/cli/__init__.py` does `from conductor.cli.app import app`, and `app.py` itself reaches `conductor.web.server` — a genuine import cycle). `cli/pid.py::pid_dir()` delegates to it too, so both the PID-file registry and the dashboard token files (issue #397) share one directory and one home-isolation seam for tests. Callers reference it via `from conductor import rundir; rundir.runs_dir()` rather than importing the bare name, so a test can monkeypatch the one attribute (`conductor.rundir.runs_dir`) and every caller sees the patch — importing the name directly would bind a separate reference that a patch on the defining module can't reach. - **events.py**: Pub/sub event system decoupling workflow execution from rendering (console, web dashboard) @@ -263,6 +265,7 @@ Tests mirror source structure in `tests/`: - `test_gates/` - Human gate tests - `test_skills/` - Skill registry, frontmatter parsing, path entries, injection budget, loader, schema field, and executor/engine-integration tests. `test_engine_integration.py` is load-bearing: an `AgentExecutor` built directly in a test is handed `workflow_dir` and `skill_injection` by the test itself, so only an engine-level test can catch the engine failing to supply them - `test_plugins/` - Manifest parsing (both conventions, all three `mcpServers` forms), agent-definition parsing, name/path/`@marketplace`/ambiguous resolution, component tri-state, schema, validator cross-checks, and provider wiring. Source grammar, catalog anchoring, and fetching live in `test_sources.py` / `test_marketplace.py` / `test_fetch.py` — the fetch tests run **real git against `file://` repositories** built by a fixture, because mocking `subprocess` would test the mock: annotated-tag dereferencing, shallow SHA fetch, and the unreachable-remote fallback are all properties of git itself. `conftest.py` builds every plugin tree on disk and takes `home` as a fixture, so no test reads the developer's real `~`. `test_executor_integration.py` and `test_engine_integration.py` are load-bearing for the same reason as their skills counterparts, and more so: a plugin's subagents and MCP servers have **no fallback delivery path**, so if they do not arrive at `execute` they are simply gone — a negative assertion could not tell a working path from a dropped one +- `test_install_hint.py` - The optional-extra install resolver (issue #441). The three context branches are covered against the pure `render_install_command`, so no test needs a real install; receipt parsing is covered against fixture `uv-receipt.toml` files. `tests/test_integration/test_install_script_extras.py` is its shell counterpart and **executes the real `install.sh` helpers** (sourcing the script with its trailing `main` stripped) rather than grepping them — the failure mode there is a quoting or `sed`-pattern mistake that reading the file does not catch; `install.ps1`'s helpers are extracted with PowerShell's own parser and executed wherever a native `pwsh` exists, with a parity class feeding both implementations the same receipt - `test_fleet/` - Fleet Manager tests: run-record read/write/prune tolerance (`test_records.py`), event-log retention (`test_retention.py`), `RunSummary`/status-derivation and gate payload (`test_summary.py`), History enumeration (`test_history.py`), New Run resolve/launch (`test_launch.py`), the run-record-writing integration into `cli/run.py` (`test_run_record_wiring.py`), terminal bell/OSC 9 notification debouncing (`test_notify.py`), and Textual `App.run_test()` pilot tests per screen (`test_tui_runs.py`, `test_tui_run_detail.py`, `test_tui_step_detail.py`, `test_tui_drilldown.py` for Providers/Registries, `test_tui_new_run.py`, `test_tui_history.py`, `test_tui_splash.py`) plus shared actions (`test_tui_actions.py` — stop/kill/gate-resolve) and the presentation helpers (`test_tui_theme.py`, `test_tui_anim.py`, `test_tui_dag.py`) Use `pytest.mark.performance` for performance tests (exclude with `-m "not performance"`). diff --git a/CHANGELOG.md b/CHANGELOG.md index 67d1c0a9..f429b4a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 and `fleet/retention.py`'s filename parsers, and `fleet/records.py`'s own timestamp parser, now derive their run-id-matching regexes from the same shared pattern. +- **Install hints for optional extras now print a command that works, and + upgrades stop uninstalling the extras you have** (#441). Every hint pointing + at an optional extra hardcoded `pip install 'conductor-cli[]'`, which + cannot work on the documented install path: `install.sh`/`install.ps1` create + a `uv tool` venv, which is not pip-managed, and `conductor-cli` is not + published to PyPI so pip has nothing to resolve against there. `conductor + fleet` without the `tui` extra, and the `aca` / `claude-agent-sdk` provider + errors, now resolve the command from the *detected* install context — `uv + tool install --force ''` for an install-script install, `uv sync + --inexact --extra ` for a source checkout, and `pip install` as the + fallback, carrying the git URL you installed from when there is one so a + `pip`/`pipx`-from-git install resolves too. The suggested command reuses the + install source recorded for your install (so a fork or a local build is not + redirected upstream) and carries the extras you already have, because `uv + tool install --force` replaces the tool's entire requirement set and `uv + sync` is exact by default. A receipt that cannot be read is reported rather + than treated as "no extras" — in the hint, and in both install scripts, + which warn and carry on rather than either dropping the extras silently or + refusing to run. + For the same reason, `install.sh` and `install.ps1` now read the existing + install's `uv-receipt.toml` and rebuild the source as + `conductor-cli[] @ `, so `conductor update` (which drives + them) no longer silently uninstalls `[tui]` or `[aca]` on upgrade — it also + names the extras it found before you commit. New `--extras ` / + `CONDUCTOR_INSTALL_EXTRAS` adds an extra during an install or upgrade + (rejecting one this package does not declare, which uv would otherwise + accept with a warning and a zero exit status), and `--no-preserve-extras` / + `CONDUCTOR_INSTALL_NO_PRESERVE_EXTRAS` drops back to a bare install. + - **Fleet Manager History no longer accumulates an entire retained event log into memory to build one entry** (#436). `_read_full_log` now streams parsed events one at a time instead of materializing them into a list diff --git a/README.md b/README.md index 3a5d4680..d3b48f0f 100644 --- a/README.md +++ b/README.md @@ -76,6 +76,17 @@ conductor update --apply `--apply` launches the install script automatically — on Windows it opens in a new console window so you can watch progress; on macOS/Linux it replaces the current process. Either way, the running `conductor` exits before the installer touches the venv, so file locks release cleanly. +**Optional extras survive the upgrade.** `uv tool install --force` replaces the tool's entire requirement set, so an upgrade that named no extras used to silently uninstall `[tui]` or `[aca]`. Both install scripts now read the existing install's uv receipt and carry those extras forward, and `conductor update` tells you which ones it found. To add one during an upgrade, or to drop back to a bare install: + +```bash +curl -sSfL https://aka.ms/conductor/install.sh | sh -s -- --extras tui +curl -sSfL https://aka.ms/conductor/install.sh | sh -s -- --no-preserve-extras +``` + +```powershell +$env:CONDUCTOR_INSTALL_EXTRAS = 'tui'; irm https://aka.ms/conductor/install.ps1 | iex +``` + The install script handles file-lock safety (process detection, stale-file cleanup, and on Windows a rename-fallback when the venv directory can't be removed), retries with backoff, and verifies the installed version after install. If your shell ever gets into a bad state from a failed update, re-running the install script is always the right next step. Conductor periodically checks GitHub for newer releases (cached for 24 hours under `~/.conductor/update-check.json`) and prints a one-line hint when one is available. To silence the hint permanently — for example when you manage upgrades through a package manager or company-mirrored install — set `CONDUCTOR_NO_UPDATE_CHECK=1` in your shell environment. The check is also skipped automatically for non-TTY invocations, `--silent` mode, the `update` subcommand, and `--help` / `--version`. @@ -217,10 +228,17 @@ conductor stop The dashboard shows you one run in depth. The **Fleet Manager** shows you *every* run at once — and it's where you go when something needs you. Launch it with `conductor fleet`: ```bash -pip install 'conductor-cli[tui]' # one-time: the TUI ships as an optional extra +# One-time: the TUI ships as an optional extra. +curl -sSfL https://aka.ms/conductor/install.sh | sh -s -- --extras tui conductor fleet ``` +> The install command depends on how you installed Conductor. Running `conductor fleet` +> without the extra prints the one that works on your machine — pinned to the version +> you are running and carrying any extras you already have, because `uv tool install +> --force` replaces the tool's whole requirement set. `conductor update` carries them +> forward for the same reason. + ![Fleet Manager](docs/img/fleet-manager.png) > **TUI = breadth. Dashboard = depth.** diff --git a/docs/cli-reference.md b/docs/cli-reference.md index b2959185..5d3f3f6f 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -541,21 +541,34 @@ bindings, and status vocabulary. conductor fleet ``` -The TUI requires the `tui` extra: - -```bash -pip install 'conductor-cli[tui]' -``` - -Without it, the bare invocation prints an install hint and exits non-zero -rather than raising an `ImportError` traceback: +The TUI requires the `tui` extra. The install command depends on how +Conductor itself was installed, so the bare invocation prints the one that +works on your machine rather than guessing: + +| How you installed | Command | +| --- | --- | +| The install script | `uv tool install --force 'conductor-cli[tui] @ git+https://github.com/microsoft/conductor.git@v'` | +| A source checkout (`uv sync`) | `uv sync --extra tui` | +| Anything else — a wheel, `pip`/`pipx` from git, a system package | `pip install 'conductor-cli[tui]'` (with the git URL appended when there is one) | + +`conductor-cli` is not on PyPI, so the `pip` form resolves only where pip +can already see an installed `conductor-cli` — never inside the uv tool venv +the install script creates. Without the extra, the bare invocation prints the +resolved command and exits non-zero rather than raising an `ImportError` +traceback: ```bash $ conductor fleet Error: the interactive fleet manager requires the 'tui' extra. -Install with: pip install 'conductor-cli[tui]' +Install with: uv tool install --force 'conductor-cli[tui] @ git+https://github.com/microsoft/conductor.git@v' ``` +The suggested command pins the running version and includes any extras +already installed, since `uv tool install --force` replaces the tool's +entire requirement set. `conductor update` and the install scripts preserve +them for the same reason — see +[Updating](../README.md#updating). + `conductor fleet list` and `conductor fleet prune` (below) need nothing beyond a normal Conductor install — only the bare, no-subcommand invocation needs `textual`. diff --git a/docs/fleet.md b/docs/fleet.md index 502cef02..a1ca56ef 100644 --- a/docs/fleet.md +++ b/docs/fleet.md @@ -47,19 +47,47 @@ below is a separate, optional layer on top of it. `conductor fleet list` and `conductor fleet prune` (documented in the [CLI reference](cli-reference.md)) need nothing beyond a normal Conductor install. The interactive TUI (`conductor fleet`, invoked with no -subcommand) additionally requires the `tui` extra: +subcommand) additionally requires the `tui` extra. + +The command depends on how Conductor itself was installed, so +`conductor fleet` prints the right one for your machine rather than +guessing: + +| How you installed | Command | +| --- | --- | +| The install script (`curl -sSfL https://aka.ms/conductor/install.sh \| sh`) | `uv tool install --force 'conductor-cli[tui] @ git+https://github.com/microsoft/conductor.git@v'` | +| A source checkout (`uv sync`) | `uv sync --extra tui` | +| Anything else — a wheel, `pip`/`pipx` from git, a system package | `pip install 'conductor-cli[tui]'` (with the git URL appended when there is one) | + +`conductor-cli` is **not** published to PyPI, so the `pip` form resolves +only where pip can already see an installed `conductor-cli` — never inside a +uv tool venv, which is what the install script creates. That is why the hint +is resolved rather than hardcoded (issue #441); for a `pip`/`pipx`-from-git +install it also appends the git URL you installed from, so the command +actually resolves. + +Without the extra, `conductor fleet` prints that command and exits non-zero +rather than raising an `ImportError` traceback: ```bash -pip install 'conductor-cli[tui]' +$ conductor fleet +Error: the interactive fleet manager requires the 'tui' extra. +Install with: uv tool install --force 'conductor-cli[tui] @ git+https://github.com/microsoft/conductor.git@v' ``` -Without it, `conductor fleet` prints an install hint and exits non-zero -rather than raising an `ImportError` traceback: +The suggested command pins the version already running and carries any +extras you already have, because `uv tool install --force` replaces the +tool's whole requirement set — installing `[tui]` on a machine that had +`[aca]` would otherwise remove it. + +For the same reason, `conductor update` (and the install scripts it drives) +preserve the extras recorded in the existing install, so an upgrade never +silently uninstalls the TUI. To install an extra as part of an upgrade, or +to drop back to a bare install: ```bash -$ conductor fleet -Error: the interactive fleet manager requires the 'tui' extra. -Install with: pip install 'conductor-cli[tui]' +curl -sSfL https://aka.ms/conductor/install.sh | sh -s -- --extras tui +curl -sSfL https://aka.ms/conductor/install.sh | sh -s -- --no-preserve-extras ``` `conductor fleet list` and `conductor fleet prune` are unaffected either diff --git a/docs/providers/aca.md b/docs/providers/aca.md index 7a0bd3b1..bc6251db 100644 --- a/docs/providers/aca.md +++ b/docs/providers/aca.md @@ -41,14 +41,31 @@ the source design: ### 1. Install the azure-identity extra +The command depends on how Conductor itself was installed. `conductor run` +and `conductor doctor` print the right one when the extra is missing, so you +can also just run your workflow and copy what it says. (`conductor validate` +does not — the provider is constructed lazily, so the guard only fires once +an `aca`-backed agent actually runs.) + ```bash -# Using uv (recommended) -uv add 'conductor-cli[aca]' +# Installed via the install script (uv tool install) +uv tool install --force 'conductor-cli[aca] @ git+https://github.com/microsoft/conductor.git@v' + +# A source checkout +uv sync --extra aca -# Using pip +# A wheel from a GitHub Release or a private index pip install 'conductor-cli[aca]' ``` +`conductor-cli` is not published to PyPI, so the `pip` form resolves only +where pip can already see an installed `conductor-cli` — never inside a uv +tool venv (issue #441). The `uv tool install` form must name every +extra you want to keep — `--force` replaces the tool's entire requirement +set, so `[aca]` alone would remove an already-installed `[tui]`. Conductor's +own hint builds that list for you, and `conductor update` preserves it +across upgrades. + This pins `azure-identity` plus `azure-core[aio]` (which pulls in `aiohttp`), used to acquire a `dynamicsessions.io` bearer token via the async `DefaultAzureCredential` for the *Session Executor* role — `azure-identity` @@ -780,8 +797,11 @@ except the two that are pure narrowing (binding loopback, the ### `aca provider requires the azure-identity package` -Install the extra: `pip install 'conductor-cli[aca]'` (or `uv add -'conductor-cli[aca]'`). +Install the `aca` extra. The error's own `suggestion` carries the exact +command for how this Conductor was installed — see +[Install the azure-identity extra](#1-install-the-azure-identity-extra) for +the three forms and why a hardcoded `pip install 'conductor-cli[aca]'` +does not work on the documented install path. ### `'pool_endpoint' is required when name='aca'` diff --git a/install.ps1 b/install.ps1 index c8c0894d..4ad064c1 100644 --- a/install.ps1 +++ b/install.ps1 @@ -32,13 +32,25 @@ # a throwaway UV_TOOL_BIN_DIR that must never leak into the real # environment (note: `uv tool update-shell` intentionally modifies the # shell and so ignores UV_NO_MODIFY_PATH). +# -Extras OR $env:CONDUCTOR_INSTALL_EXTRAS +# Comma-separated optional extras to install (tui, aca, +# claude-agent-sdk). These are *added to* the extras already recorded in +# the existing install's uv receipt, which are preserved automatically -- +# `uv tool install --force` rewrites the tool's whole requirement set, so +# an upgrade that named no extras used to silently uninstall +# `[tui]`/`[aca]` (issue #441). +# -NoPreserveExtras OR $env:CONDUCTOR_INSTALL_NO_PRESERVE_EXTRAS = '1' +# Do not carry the existing install's extras forward. Use this to drop +# back to a bare install. [CmdletBinding()] param( [string]$Source, [switch]$AutoStop, [switch]$Force, - [switch]$SkipPathUpdate + [switch]$SkipPathUpdate, + [string]$Extras, + [switch]$NoPreserveExtras ) $ErrorActionPreference = 'Stop' @@ -52,6 +64,8 @@ if (-not $Source -and $env:CONDUCTOR_INSTALL_SOURCE) { $Source if (-not $AutoStop -and $env:CONDUCTOR_INSTALL_AUTO_STOP -eq '1') { $AutoStop = $true } if (-not $Force -and $env:CONDUCTOR_INSTALL_FORCE -eq '1') { $Force = $true } if (-not $SkipPathUpdate -and $env:CONDUCTOR_INSTALL_SKIP_PATH_UPDATE -eq '1') { $SkipPathUpdate = $true } +if (-not $Extras -and $env:CONDUCTOR_INSTALL_EXTRAS) { $Extras = $env:CONDUCTOR_INSTALL_EXTRAS } +if (-not $NoPreserveExtras -and $env:CONDUCTOR_INSTALL_NO_PRESERVE_EXTRAS -eq '1') { $NoPreserveExtras = $true } # --------------------------------------------------------------------------- # Helpers @@ -82,13 +96,103 @@ function Get-UvToolsDir { } function Get-ConductorToolDir { + # Probes both directory names Get-InstalledVersion knows about, so the + # receipt lookup and the verification step agree about where the tool + # lives. $tools = Get-UvToolsDir if (-not $tools) { return $null } + foreach ($name in @('conductor-cli', 'conductor')) { + $dir = Join-Path $tools $name + if (Test-Path -LiteralPath (Join-Path $dir 'uv-receipt.toml')) { return $dir } + } $dir = Join-Path $tools 'conductor-cli' if (Test-Path -LiteralPath $dir) { return $dir } return $null } +function Get-ReceiptExtras { + # Read the extras recorded in the existing install's uv tool receipt. + # + # `uv tool install --force` replaces the tool's entire requirement set, so + # an upgrade that names no extras silently uninstalls [tui]/[aca] -- which + # is exactly what `conductor update` used to do (issue #441). The receipt + # is the only authoritative record of what the current install carries; + # the CLI's `conductor.install_hint` reads the same file for its hints. + # + # Flattens newlines (uv may wrap the requirements array), splits the array + # into one requirement object per chunk, and keeps this distribution's + # entry -- so field order and extra `uv tool install --with` requirements + # are both tolerated. + $dir = Get-ConductorToolDir + if (-not $dir) { return '' } + $receipt = Join-Path $dir 'uv-receipt.toml' + if (-not (Test-Path -LiteralPath $receipt)) { return '' } + try { + $text = (Get-Content -LiteralPath $receipt -Raw) -replace '\r?\n', ' ' + } catch { + # A receipt that exists but cannot be read is NOT a bare install, so + # do not report '' -- that would rebuild the tool without the extras + # it records. Signal unreadability and let the caller warn; aborting + # here would take away the reinstall that repairs this very state. + return $null + } + $found = $false + foreach ($chunk in ($text -split '\}')) { + # Match the whole `name = "conductor-cli"` field rather than the bare + # substring, so a `conductor-cli-plugin` requirement is not mistaken + # for this one. `[-_.]` covers a non-canonical (PEP 503) spelling. + if ($chunk -match 'name\s*=\s*"conductor[-_.]cli"') { + $found = $true + if ($chunk -match 'extras\s*=\s*\[([^\]]*)\]') { + return ($Matches[1] -replace '["'' ]', '') + } + } + } + if (-not $found) { return $null } + return '' +} + +function Merge-Extras { + # Merge comma-separated extras lists, dropping blanks/duplicates and + # sorting so the generated spec is stable between runs (and comparable + # as a string). + # + # Lower-cases before deduplicating so the result matches `install.sh`'s + # `merge_extras`, whose `sort -u` is case-sensitive. Without it the two + # installers disagree on mixed-case input. + param([string]$A, [string]$B) + $parts = @("$A", "$B") -join ',' + $names = $parts -split ',' | + ForEach-Object { $_.Trim().ToLowerInvariant() } | + Where-Object { $_ } | + Sort-Object -Unique + return ($names -join ',') +} + +function Test-ExtrasKnown { + # Refuse an extra this package does not declare. uv treats an unknown + # extra as a *warning* and still exits 0, so without this a typo installs + # nothing and reports success. + param([string]$ExtrasList) + if (-not $ExtrasList) { return } + foreach ($name in ($ExtrasList -split ',')) { + $trimmed = $name.Trim() + if (-not $trimmed) { continue } + if (@('tui', 'aca', 'claude-agent-sdk') -notcontains $trimmed) { + Write-Err "unknown extra '$trimmed' (available: tui, aca, claude-agent-sdk)" + } + } +} + +function Add-ExtrasToSource { + # Wrap an install source in a PEP 508 direct reference carrying the + # extras. uv accepts a git+ URL, a local directory, or a wheel path on + # the right-hand side of the `@`. + param([string]$InstallSource, [string]$ExtrasList) + if (-not $ExtrasList) { return $InstallSource } + return "conductor-cli[$ExtrasList] @ $InstallSource" +} + function Get-RunningConductorProcesses { # Returns a list of pscustomobjects: @{ Pid; Name; Path } # Excludes the current PowerShell process and its ancestors so we don't @@ -132,6 +236,17 @@ function Remove-StaleOldFiles { } } +function Format-ProcessArgument { + # Quote an argument that contains whitespace so Start-Process passes it as + # one argv element. See the note in Invoke-UvInstall. + param([string]$Value) + if ($Value -notmatch '\s') { return $Value } + # Double any trailing backslashes so they escape each other rather than + # the closing quote, and escape embedded quotes. + $escaped = $Value -replace '(\\+)$', '$1$1' -replace '"', '\"' + return '"' + $escaped + '"' +} + function Invoke-UvInstall { param( [string]$InstallSource, @@ -142,8 +257,12 @@ function Invoke-UvInstall { $stdoutFile = Join-Path $LogDir ("uv-stdout-{0}.log" -f ([guid]::NewGuid().ToString('N').Substring(0,6))) $stderrFile = Join-Path $LogDir ("uv-stderr-{0}.log" -f ([guid]::NewGuid().ToString('N').Substring(0,6))) - $argList = @('tool', 'install', '--force', $InstallSource) - if ($ConstraintsFile) { $argList += @('-c', $ConstraintsFile) } + # Start-Process joins -ArgumentList with spaces and does NOT quote the + # elements, so a PEP 508 direct reference ("conductor-cli[tui] @ ") + # would reach uv as three separate arguments and the extras would be + # silently dropped. Same hazard for a temp path containing a space. + $argList = @('tool', 'install', '--force', (Format-ProcessArgument $InstallSource)) + if ($ConstraintsFile) { $argList += @('-c', (Format-ProcessArgument $ConstraintsFile)) } $proc = Start-Process -FilePath 'uv' ` -ArgumentList $argList ` @@ -260,6 +379,33 @@ if ($Source) { $displayVersion = $tagName } +# --- Extras: carry the existing install's extras forward, plus any requested --- +# +# $receiptNow is what is installed; $existingExtras is what we choose to carry. +# They differ under -NoPreserveExtras, and the up-to-date check below has to +# compare against the former -- comparing against the latter made the opt-out +# a no-op, since the switch zeroes it and both sides then match. +Test-ExtrasKnown $Extras +$rawExtras = Get-ReceiptExtras +$receiptReadable = $null -ne $rawExtras +$receiptNow = Merge-Extras $rawExtras '' +if (-not $receiptReadable) { + # Warn and continue rather than aborting: a broken receipt is exactly the + # state a reinstall is meant to repair, so refusing would remove the + # remedy. Proceeding silently is how [tui]/[aca] disappear unnoticed. + Write-Warn "Could not read the existing install's uv receipt; extras cannot be preserved." + Write-Warn "Re-run with -Extras to reinstate any you had." +} +$existingExtras = '' +if (-not $NoPreserveExtras) { $existingExtras = $receiptNow } +$resolvedExtras = Merge-Extras $existingExtras $Extras +if ($resolvedExtras) { + Write-Info "Including extras: $resolvedExtras" + $installSource = Add-ExtrasToSource $installSource $resolvedExtras +} elseif ($receiptNow) { + Write-Info "Dropping extras: $receiptNow" +} + # --- Check existing installation (only meaningful for the GitHub-release path) --- if (-not $Source) { $existingConductor = Get-Command conductor -ErrorAction SilentlyContinue @@ -271,7 +417,12 @@ if (-not $Source) { } catch { } if ($currentVersion) { $latestVersion = $tagName -replace '^v', '' - if ($currentVersion -eq $latestVersion) { + # An already-current version is only a no-op when the extras on + # disk already match what this run would install: `-Extras tui` + # (or -NoPreserveExtras) still has work to do, and reporting "up + # to date" would silently skip it. + if ($currentVersion -eq $latestVersion -and + $resolvedExtras -eq $receiptNow -and $receiptReadable) { Write-Ok "Conductor v$currentVersion is already installed and up to date." Write-Host "" Write-Host " Run 'conductor --help' to get started." @@ -387,6 +538,13 @@ try { $lastStderr = $r.Stderr if ($lastExitCode -eq 0) { $installed = $true + # uv reports an extra it does not recognise as a warning and still + # exits 0, and this output is only shown on failure -- so without + # this the run ends in a green checkmark having installed nothing + # the user asked for. + foreach ($line in (($lastStdout + $lastStderr) -split "`r?`n")) { + if ($line -match 'does not have an extra named') { Write-Warn $line.Trim() } + } break } diff --git a/install.sh b/install.sh index 10425579..5b7ca5f8 100755 --- a/install.sh +++ b/install.sh @@ -24,6 +24,16 @@ # install into a throwaway UV_TOOL_BIN_DIR that must never leak into the # real environment (note: `uv tool update-shell` intentionally modifies # the shell and so ignores UV_NO_MODIFY_PATH). +# --extras OR $CONDUCTOR_INSTALL_EXTRAS +# Comma-separated optional extras to install (tui, aca, +# claude-agent-sdk). These are *added to* the extras already recorded +# in the existing install's uv receipt, which are preserved +# automatically -- `uv tool install --force` rewrites the tool's whole +# requirement set, so an upgrade that named no extras used to silently +# uninstall `[tui]`/`[aca]` (issue #441). +# --no-preserve-extras OR $CONDUCTOR_INSTALL_NO_PRESERVE_EXTRAS=1 +# Do not carry the existing install's extras forward. Use this to drop +# back to a bare install. set -eu @@ -39,6 +49,8 @@ SOURCE="${CONDUCTOR_INSTALL_SOURCE:-}" AUTO_STOP="${CONDUCTOR_INSTALL_AUTO_STOP:-0}" FORCE_FLAG="${CONDUCTOR_INSTALL_FORCE:-0}" SKIP_PATH_UPDATE="${CONDUCTOR_INSTALL_SKIP_PATH_UPDATE:-0}" +EXTRAS="${CONDUCTOR_INSTALL_EXTRAS:-}" +NO_PRESERVE_EXTRAS="${CONDUCTOR_INSTALL_NO_PRESERVE_EXTRAS:-0}" while [ $# -gt 0 ]; do case "$1" in @@ -47,6 +59,9 @@ while [ $# -gt 0 ]; do --auto-stop) AUTO_STOP=1; shift ;; --force) FORCE_FLAG=1; shift ;; --skip-path-update) SKIP_PATH_UPDATE=1; shift ;; + --extras) EXTRAS="$2"; shift 2 ;; + --extras=*) EXTRAS="${1#--extras=}"; shift ;; + --no-preserve-extras) NO_PRESERVE_EXTRAS=1; shift ;; *) shift ;; esac done @@ -163,10 +178,15 @@ uv_install_with_retry() { return "$ec" } +# The directory uv keeps tool venvs in, or empty when uv cannot say. +uv_tools_dir() { + uv tool dir 2>/dev/null | head -n1 || true +} + # Run `conductor --version` from the freshly installed location and return the # version string (or empty on failure). verify_install() { - tools_dir=$(uv tool dir 2>/dev/null | head -n1 || true) + tools_dir=$(uv_tools_dir) if [ -n "$tools_dir" ]; then # uv tool venvs put the entrypoint at //bin/ on POSIX for candidate in "$tools_dir/conductor-cli/bin/conductor" "$tools_dir/conductor/bin/conductor"; do @@ -181,6 +201,114 @@ verify_install() { fi } +# Path to the existing install's uv tool receipt, or empty if there isn't one. +# Probes both directory names verify_install knows about, so the two functions +# agree about where the tool lives. +receipt_path() { + tools_dir=$(uv_tools_dir) + [ -n "$tools_dir" ] || return 0 + for name in conductor-cli conductor; do + if [ -f "${tools_dir}/${name}/uv-receipt.toml" ]; then + printf '%s' "${tools_dir}/${name}/uv-receipt.toml" + return 0 + fi + done +} + +# Read the extras recorded in the existing install's uv tool receipt. +# +# `uv tool install --force` replaces the tool's entire requirement set, so an +# upgrade that names no extras silently uninstalls `[tui]`/`[aca]` (issue +# #441). The receipt is the only authoritative record of what the current +# install carries; the CLI's `conductor.install_hint` reads the same file to +# build its install hints, so the two must agree. +# +# Flattens newlines (uv may wrap the requirements array), splits the array +# into one requirement object per line, keeps this distribution's entry -- +# matched on the whole `name = "conductor-cli"` field, not as a substring, so +# a `conductor-cli-plugin` requirement is not mistaken for it -- and extracts +# its extras. Field order inside the object and additional `uv tool install +# --with` requirements are both tolerated. +# Returns 0 with the extras (possibly empty) when the receipt was understood, +# and non-zero when one exists but could not be read. That distinction is the +# whole point: an unreadable receipt is NOT a bare install, and treating it as +# one rebuilds the tool without the extras it records -- silently causing the +# very data loss this change exists to prevent. `conductor.install_hint`'s +# `read_receipt` draws the same line with `ReceiptContents.readable`. +# +# `-i` on the name match because the Python reader normalises per PEP 503; a +# case-sensitive match here would find nothing for a receipt recording +# `Conductor-CLI` and drop the extras. +receipt_extras() { + receipt=$(receipt_path) + [ -n "$receipt" ] || return 0 + [ -r "$receipt" ] || return 1 + flat=$(tr '\n' ' ' < "$receipt") || return 1 + entry=$(printf '%s' "$flat" | tr '}' '\n' \ + | grep -Ei 'name[[:space:]]*=[[:space:]]*"conductor[-_.]cli"' \ + | head -n1) || return 1 + [ -n "$entry" ] || return 1 + printf '%s' "$entry" \ + | sed -n 's/.*extras[[:space:]]*=[[:space:]]*\[\([^]]*\)\].*/\1/p' \ + | tr -d "\"' " \ + || true +} + +# Merge comma-separated extras lists, dropping blanks/duplicates and sorting +# so the generated spec is stable between runs (and comparable as a string). +# +# Lower-cases before deduplicating to match `install.ps1`'s Merge-Extras, +# whose Sort-Object -Unique is case-insensitive. Without this the two +# installers disagree, and `--extras TUI` against a recorded `tui` would make +# the up-to-date comparison below never converge. +merge_extras() { + printf '%s,%s' "$1" "$2" \ + | tr ',' '\n' \ + | tr '[:upper:]' '[:lower:]' \ + | sed 's/^[[:space:]]*//; s/[[:space:]]*$//' \ + | grep -v '^$' \ + | sort -u \ + | tr '\n' ',' \ + | sed 's/,$//' \ + || true +} + +# Refuse an extra this package does not declare. uv treats an unknown extra as +# a *warning* and still exits 0, so without this a typo installs nothing and +# reports success -- the same "confident command that does not work" this +# whole change exists to remove. +# +# Split with parameter expansion rather than a `... | while` pipeline: the +# loop body would run in a subshell there, so `error`'s `exit 1` would kill +# only the subshell and the install would carry on with the bad extra. +validate_extras() { + _rest="$1" + while [ -n "$_rest" ]; do + _name="${_rest%%,*}" + case "$_rest" in + *,*) _rest="${_rest#*,}" ;; + *) _rest="" ;; + esac + [ -n "$_name" ] || continue + _name=$(printf '%s' "$_name" | tr '[:upper:]' '[:lower:]') + case "$_name" in + tui|aca|claude-agent-sdk) ;; + *) error "unknown extra '${_name}' (available: tui, aca, claude-agent-sdk)" ;; + esac + done +} + +# Wrap an install source in a PEP 508 direct reference carrying the extras. +# uv accepts a git+ URL, a local directory, or a wheel path on the right-hand +# side of the `@`. +apply_extras() { + if [ -n "$2" ]; then + printf 'conductor-cli[%s] @ %s' "$2" "$1" + else + printf '%s' "$1" + fi +} + # --------------------------------------------------------------------------- # Main # --------------------------------------------------------------------------- @@ -227,12 +355,48 @@ main() { display_version="$tag_name" fi + # --- Extras: carry the existing install's extras forward, plus any requested --- + # + # `receipt_now` is what is installed; `existing_extras` is what we choose + # to carry. They differ under --no-preserve-extras, and the up-to-date + # check below has to compare against the former -- comparing against the + # latter made the opt-out a no-op, since the flag zeroes it and both sides + # then match. + validate_extras "$EXTRAS" + receipt_readable=1 + raw_extras=$(receipt_extras) || receipt_readable=0 + receipt_now=$(merge_extras "$raw_extras" "") + if [ "$receipt_readable" = "0" ]; then + # Warn and keep going rather than aborting: a broken receipt is + # exactly the state a reinstall is meant to repair, so refusing would + # take away the remedy. But say plainly that extras cannot be carried, + # because the alternative -- proceeding silently -- is how [tui]/[aca] + # disappear without anyone noticing. + warn "Could not read the existing install's uv receipt; extras cannot be preserved." + warn "Re-run with --extras to reinstate any you had." + fi + existing_extras="" + [ "$NO_PRESERVE_EXTRAS" = "1" ] || existing_extras="$receipt_now" + resolved_extras=$(merge_extras "$existing_extras" "$EXTRAS") + if [ -n "$resolved_extras" ]; then + info "Including extras: ${resolved_extras}" + install_source=$(apply_extras "$install_source" "$resolved_extras") + elif [ -n "$receipt_now" ]; then + info "Dropping extras: ${receipt_now}" + fi + # --- Existing-install check (only for the GitHub-release path) --- if [ -z "$SOURCE" ] && need_cmd conductor; then current_version=$(conductor --version 2>/dev/null | grep -oE '[0-9]+\.[0-9]+\.[0-9]+[^ ]*' | head -1 || true) if [ -n "$current_version" ]; then latest_version=$(printf '%s' "$tag_name" | sed 's/^v//') - if [ "$current_version" = "$latest_version" ]; then + # An already-current version is only a no-op when the extras on + # disk already match what this run would install: `--extras tui` + # (or --no-preserve-extras) still has work to do, and reporting + # "up to date" would silently skip it. + if [ "$current_version" = "$latest_version" ] \ + && [ "$resolved_extras" = "$receipt_now" ] \ + && [ "$receipt_readable" = "1" ]; then success "Conductor v${current_version} is already installed and up to date." printf '\n Run \033[1mconductor --help\033[0m to get started.\n\n' return 0 @@ -315,6 +479,14 @@ main() { printf '\n' >&2 error "uv tool install failed after retries" fi + # uv reports an extra it does not recognise as a warning and still exits + # 0, and the log is only shown on failure -- so without this the run ends + # in a green checkmark having installed nothing the user asked for. + if grep -q 'does not have an extra named' "$log_file" 2>/dev/null; then + grep 'does not have an extra named' "$log_file" | while IFS= read -r line; do + warn "$line" + done + fi success "Conductor ${display_version} installed" # --- Update shell PATH --- diff --git a/src/conductor/cli/fleet.py b/src/conductor/cli/fleet.py index 06e8e80d..010b2534 100644 --- a/src/conductor/cli/fleet.py +++ b/src/conductor/cli/fleet.py @@ -9,11 +9,17 @@ calls out: the other three sub-apps (``checkpoint`` / ``registry`` / ``gate``) set ``no_args_is_help=True`` since they have no sensible default action, whereas the TUI *is* the feature here and is the hot path. The TUI -requires the optional ``tui`` extra (``pip install 'conductor-cli[tui]'``); -when ``textual`` isn't installed, the bare invocation prints an install -hint and exits non-zero rather than raising an ``ImportError`` traceback — -mirroring the established availability-flag pattern used for other optional -SDK dependencies (see ``providers/aca.py``'s ``AZURE_IDENTITY_AVAILABLE``). +requires the optional ``tui`` extra; when ``textual`` isn't installed, the +bare invocation prints an install hint and exits non-zero rather than +raising an ``ImportError`` traceback — mirroring the established +availability-flag pattern used for other optional SDK dependencies (see +``providers/aca.py``'s ``AZURE_IDENTITY_AVAILABLE``). + +That hint is resolved from the detected install context +(:func:`conductor.install_hint.install_command`) rather than hardcoded: a +uv tool venv is not pip-managed and ``conductor-cli`` is not on PyPI, so a +hardcoded ``pip install`` string cannot work on the documented install +path (issue #441). """ from __future__ import annotations @@ -26,6 +32,7 @@ from rich.text import Text from conductor.console import make_console, styled +from conductor.install_hint import install_command # `textual` is an optional dependency (the `tui` extra) — this module is # imported unconditionally at every `conductor` invocation (via @@ -54,8 +61,8 @@ def fleet_main(ctx: typer.Context) -> None: r"""Manage the fleet of running Conductor workflows. With no subcommand, this launches the interactive TUI. Requires the - `tui` extra (`pip install 'conductor-cli\[tui]'`); without it, prints an - install hint and exits rather than launching. + `tui` extra; without it, prints the install command for how this + Conductor was installed and exits rather than launching. \b Examples: @@ -67,10 +74,16 @@ def fleet_main(ctx: typer.Context) -> None: console.print( Text.from_markup( "[bold red]Error:[/bold red] the interactive fleet manager requires " - "the 'tui' extra.\n" - "Install with: [cyan]pip install 'conductor-cli\\[tui]'[/cyan]" + "the 'tui' extra." ) ) + # soft_wrap so rich never inserts a hard newline mid-command: the + # whole point of this line is that it can be copied and pasted, + # and the resolved uv spec is longer than a default terminal. + console.print( + styled("Install with: [cyan]{}[/cyan]", install_command("tui")), + soft_wrap=True, + ) raise typer.Exit(code=1) from conductor.fleet.tui.app import FleetApp diff --git a/src/conductor/cli/update.py b/src/conductor/cli/update.py index 551eff94..88642674 100644 --- a/src/conductor/cli/update.py +++ b/src/conductor/cli/update.py @@ -30,6 +30,7 @@ from conductor import __version__ from conductor.console import styled +from conductor.install_hint import installed_extras logger = logging.getLogger(__name__) @@ -37,18 +38,11 @@ _CACHE_TTL_SECONDS = 86_400 # 24 hours _API_URL = "https://api.github.com/repos/microsoft/conductor/releases/latest" _FETCH_TIMEOUT_SECONDS = 2 -_REPO_GIT_URL = "https://github.com/microsoft/conductor.git" -_RELEASE_DL_URL = "https://github.com/microsoft/conductor/releases/download" - # Install-script entry points. Kept as module-level constants so a future # redirect change is a one-line edit. _INSTALL_PS1_URL = "https://aka.ms/conductor/install.ps1" _INSTALL_SH_URL = "https://aka.ms/conductor/install.sh" -# Retry settings for `uv tool install` — mirrors install.ps1 -_INSTALL_MAX_ATTEMPTS = 3 -_INSTALL_RETRY_DELAY_SECONDS = 2 - # --------------------------------------------------------------------------- # Cache helpers @@ -323,7 +317,7 @@ def _print_hint(console: Console, remote_version: str) -> None: # --------------------------------------------------------------------------- -def _install_command() -> str: +def _install_script_command() -> str: """Return the OS-appropriate one-line install/upgrade command. The install script is the single, canonical upgrade path. ``conductor @@ -407,7 +401,7 @@ def _spawn_installer_and_exit(console: Console) -> None: console.print( styled( "Run this manually in a new shell: [bold cyan]{}[/bold cyan]", - _install_command(), + _install_script_command(), ) ) raise SystemExit(1) from None @@ -437,10 +431,42 @@ def _spawn_installer_and_exit(console: Console) -> None: os.execvpe("sh", ["sh", "-c", sh_command], env) except OSError as e: console.print(styled("[bold red]Could not exec installer:[/bold red] {}", e)) - console.print(styled("Run this manually: [bold cyan]{}[/bold cyan]", _install_command())) + console.print( + styled("Run this manually: [bold cyan]{}[/bold cyan]", _install_script_command()) + ) raise SystemExit(1) from None +def _print_extras_note(console: Console) -> None: + """Tell the user which optional extras the upgrade will carry forward. + + ``uv tool install --force`` rewrites the tool's entire requirement set, + so an upgrade that names no extras used to silently uninstall ``[tui]`` + or ``[aca]``. Both install scripts now read the same uv receipt this + does and rebuild the spec with those extras (issue #441); saying so here + is what makes that visible before the user commits to the upgrade. + + Prints nothing when no extras are recorded, which is the common case. + + Args: + console: Rich console for output. + """ + try: + extras = installed_extras() + except Exception: # noqa: BLE001 - a note must never end the update command + logger.debug("Could not read installed extras", exc_info=True) + return + if not extras: + return + console.print( + styled( + "[dim]Your installed extras ({}) should be carried forward by the " + "install script.[/dim]", + ", ".join(sorted(extras)), + ) + ) + + def run_update(console: Console, force: bool = False, apply: bool = False) -> None: """Check for a newer release and either print or run the install command. @@ -486,17 +512,19 @@ def run_update(console: Console, force: bool = False, apply: bool = False) -> No console.print() if apply: + _print_extras_note(console) # Hand off to the installer and exit; this call does not return. _spawn_installer_and_exit(console) return # pragma: no cover - _spawn_installer_and_exit never returns - cmd = _install_command() + cmd = _install_script_command() console.print( Text.from_markup("To upgrade, run this in a [bold]new shell[/bold] (not inside conductor):") ) console.print() console.print(styled(" [bold cyan]{}[/bold cyan]", cmd)) console.print() + _print_extras_note(console) console.print( Text.from_markup( "[dim]Or re-run with [bold]--apply[/bold] to launch the installer " diff --git a/src/conductor/install_hint.py b/src/conductor/install_hint.py new file mode 100644 index 00000000..178041d2 --- /dev/null +++ b/src/conductor/install_hint.py @@ -0,0 +1,487 @@ +"""Resolve a working install command for one of Conductor's optional extras. + +Conductor ships three optional extras (``tui``, ``aca``, +``claude-agent-sdk``) and every hint that pointed at one used to hardcode +``pip install 'conductor-cli[]'``. That command cannot work on the +documented install path: ``install.sh`` / ``install.ps1`` run ``uv tool +install`` against a git reference, and a uv tool venv is not pip-managed — +pip has nothing there to resolve ``conductor-cli`` against, because it is +not published to PyPI (``release.yml`` attaches artifacts to a GitHub +Release and has no publish step). See issue #441. + +The command is therefore resolved from the *detected* install context: + +============================== ================== ======================== +signal context command +============================== ================== ======================== +``/uv-receipt.toml`` uv tool install ``uv tool install --force`` +``dir_info.editable`` in source checkout ``uv sync --inexact`` +``direct_url.json`` +neither anything else ``pip install`` +============================== ================== ======================== + +The pip form survives as the last-resort fallback because it *does* work +wherever pip can already see an installed ``conductor-cli`` — a wheel from +a GitHub Release, for instance, where pip resolves the extra against the +installed distribution. Where the install came from a git URL, +``direct_url.json`` still holds that URL and it is put back into the +command, so a ``pip``/``pipx``-from-git user gets something that resolves +rather than the dead form this module exists to delete. + +Three properties of the rendered command are load-bearing: + +* **Extras already installed are carried forward.** ``uv tool install + --force`` replaces the tool's whole requirement set, and ``uv sync`` is + exact unless given ``--inexact``, so a command naming only the requested + extra *uninstalls* the others. This module exists to add an extra, not to + trade one for another. +* **The install source is preserved.** The receipt records what the tool + was actually installed from — a fork, a local checkout, a wheel — so + reusing it stops the hint silently redirecting a developer's build at + upstream's released tag. +* **Failures are visible.** A receipt that exists but cannot be read is not + the same as a bare install, even though both yield "no extras found". + Reporting the first as the second produces a confident, copy-pasteable + command that deletes the user's TUI. That case renders a command carrying + an inline warning instead. + +The install scripts read the same receipt for the same reason, so an +upgrade preserves extras too — see ``install.sh``'s ``receipt_extras`` and +``install.ps1``'s ``Get-ReceiptExtras``. + +This is a stdlib-only leaf module (like :mod:`conductor.duration` and +:mod:`conductor.console`) because ``providers/`` needs it and must not +import from ``cli/``. +""" + +from __future__ import annotations + +import json +import logging +import re +import sys +import tomllib +from dataclasses import dataclass +from enum import StrEnum +from importlib.metadata import PackageNotFoundError, distribution, version +from pathlib import Path +from typing import Any, Final, Literal + +logger = logging.getLogger(__name__) + +#: The distribution name on the index — *not* the ``conductor`` command. +DISTRIBUTION: Final = "conductor-cli" + +#: Clone URL used when no install source can be recovered. +REPO_GIT_URL: Final = "https://github.com/microsoft/conductor.git" + +_UV_RECEIPT_NAME: Final = "uv-receipt.toml" + +#: The extras declared in ``pyproject.toml``'s ``[project.optional-dependencies]``. +ExtraName = Literal["tui", "aca", "claude-agent-sdk"] + +# A PEP 508 extra name. Checked at the parse boundary because these values +# are interpolated into a single-quoted command the user is told to paste, +# and this module cannot know what wrote the receipt. Mirrors +# `fleet/records.py::is_valid_run_id`, which exists for the same reason. +_EXTRA_NAME_RE: Final = re.compile(r"^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$") + +# PEP 503 normalization, so a receipt recording `conductor_cli` still +# matches. uv writes the canonical form today; this costs one regex and +# removes a way for the match to fail silently. +_NAME_SEPARATORS: Final = re.compile(r"[-_.]+") + + +def _canonical(name: str) -> str: + """Return *name* in PEP 503 canonical form.""" + return _NAME_SEPARATORS.sub("-", name).lower() + + +class InstallContext(StrEnum): + """How the running Conductor was installed.""" + + UV_TOOL = "uv-tool" + """Installed by ``uv tool install`` — the documented install-script path.""" + + EDITABLE = "editable" + """An editable install from a source checkout (``uv sync``).""" + + UNKNOWN = "unknown" + """Anything else: a wheel, ``pip``/``pipx`` from git, a system package.""" + + +def uv_receipt_path(prefix: Path | None = None) -> Path: + """Return the path where ``uv tool install`` records its requirements. + + Args: + prefix: Environment prefix whose receipt to locate. Defaults to + ``sys.prefix``. + + Returns: + ``/uv-receipt.toml``, which only exists for a uv tool venv. + """ + return (Path(sys.prefix) if prefix is None else prefix) / _UV_RECEIPT_NAME + + +def _direct_url() -> dict[str, Any]: + """Return the parsed PEP 610 ``direct_url.json``, or ``{}`` if unavailable. + + ``Distribution.read_text`` suppresses only a handful of ``OSError`` + subclasses, so a file that is present but not valid UTF-8 raises + ``UnicodeDecodeError`` — a ``ValueError``. That and ``JSONDecodeError`` + are both caught here, so this never raises into the error path it is + called from. + """ + try: + raw = distribution(DISTRIBUTION).read_text("direct_url.json") + except PackageNotFoundError: + return {} + except (OSError, ValueError): + logger.warning("Could not read direct_url.json for %s", DISTRIBUTION, exc_info=True) + return {} + if not raw: + return {} + try: + parsed = json.loads(raw) + except ValueError: + logger.warning("Unparseable direct_url.json for %s", DISTRIBUTION, exc_info=True) + return {} + return parsed if isinstance(parsed, dict) else {} + + +def _is_editable() -> bool: + """Return ``True`` when this distribution was installed in editable mode. + + Reads the PEP 610 ``direct_url.json`` recorded in the ``.dist-info`` + directory. A uv tool install also writes that file (with ``vcs_info`` + rather than ``dir_info``), which is why the receipt is checked first. + """ + dir_info = _direct_url().get("dir_info") + return isinstance(dir_info, dict) and bool(dir_info.get("editable")) + + +def _direct_url_source() -> str | None: + """Return an installable source recovered from ``direct_url.json``. + + A ``pip``/``pipx`` install from git records the clone URL and the ref that + was asked for, which is exactly what a working reinstall command needs. + + PEP 610 makes ``vcs_info`` / ``dir_info`` / ``archive_info`` mutually + exclusive, and the branch is chosen on which is present rather than on + what the URL looks like. That distinction matters for ``archive_info``: a + wheel or sdist is one *artifact*, usually a download that no longer + exists, so pinning to it produces a command that fails with ``No such + file or directory``. Returning ``None`` there lets the caller emit a bare + name, which pip resolves against the installed distribution. + """ + info = _direct_url() + url = info.get("url") + if not isinstance(url, str) or not url: + return None + + vcs_info = info.get("vcs_info") + if isinstance(vcs_info, dict): + vcs = vcs_info.get("vcs") or "git" + # `requested_revision` is what the user asked to install; prefer it + # over `commit_id` so an install from a branch stays on that branch + # rather than being re-pinned to a single commit. + ref = vcs_info.get("requested_revision") or vcs_info.get("commit_id") + source = f"{vcs}+{url}" + return f"{source}@{ref}" if isinstance(ref, str) and ref.strip() else source + + # A directory is a project that is still there; an archive is not. + return url if isinstance(info.get("dir_info"), dict) else None + + +@dataclass(frozen=True) +class ReceiptContents: + """What this install's uv receipt says about ``conductor-cli``. + + ``readable`` is separate from an empty ``extras`` on purpose: a receipt + that could not be read is indistinguishable from a bare install by its + contents alone, and the two must not produce the same command — one of + them silently uninstalls the user's extras. + """ + + extras: frozenset[str] = frozenset() + source: str | None = None + readable: bool = True + + +def _requirement_source(req: dict[str, Any]) -> str | None: + """Recover an installable source from one uv receipt requirement entry. + + uv records the origin under a key naming its kind — ``git`` (with the + ref in a ``?rev=`` query), ``directory``/``path``/``editable`` for a + local install, or ``url``. Reusing it is what keeps the hint pointed at + the fork or local checkout the tool was actually built from. + """ + git = req.get("git") + if isinstance(git, str) and git: + url, _, query = git.partition("?") + for part in query.split("&"): + key, _, value = part.partition("=") + if key in {"rev", "tag", "branch"} and value: + return f"git+{url}@{value}" + return f"git+{url}" + + for key in ("directory", "path", "editable", "url"): + value = req.get(key) + if isinstance(value, str) and value: + return value + return None + + +def read_receipt(prefix: Path | None = None) -> ReceiptContents: + """Read this install's uv tool receipt. + + Never raises. A *missing* receipt reports ``readable=True`` with no + extras — there is nothing to lose. A receipt that exists but cannot be + read or understood reports ``readable=False``, which the rendered + command turns into a visible warning rather than a silent omission. + + Args: + prefix: Environment prefix whose receipt to read. Defaults to + ``sys.prefix``. + + Returns: + The extras and install source recorded for this distribution. + """ + receipt = uv_receipt_path(prefix) + if not receipt.is_file(): + return ReceiptContents() + + try: + data = tomllib.loads(receipt.read_text(encoding="utf-8")) + except (OSError, ValueError): + # Permission denied, an I/O error, a half-written file, an encoding + # change: each would otherwise render as "no extras", i.e. as a + # command that removes them. + logger.warning("Could not read uv receipt at %s", receipt, exc_info=True) + return ReceiptContents(readable=False) + + tool = data.get("tool") + requirements = tool.get("requirements") if isinstance(tool, dict) else None + if not isinstance(requirements, list): + logger.warning("uv receipt at %s has no [tool] requirements list", receipt) + return ReceiptContents(readable=False) + + for req in requirements: + # A tool venv can carry several requirements (`uv tool install + # --with ...`); only this distribution's entry belongs in the spec. + if not isinstance(req, dict): + continue + name = req.get("name") + if not isinstance(name, str) or _canonical(name) != DISTRIBUTION: + continue + raw_extras = req.get("extras") + extras = frozenset( + e + for e in (raw_extras if isinstance(raw_extras, list) else []) + if isinstance(e, str) and _EXTRA_NAME_RE.match(e) + ) + return ReceiptContents(extras=extras, source=_requirement_source(req)) + + logger.warning("uv receipt at %s records no %s requirement", receipt, DISTRIBUTION) + return ReceiptContents(readable=False) + + +def installed_extras(prefix: Path | None = None) -> frozenset[str]: + """Return the extras recorded in this install's uv tool receipt. + + ``uv tool install --force`` replaces the tool's entire requirement set, + so any extra missing from the reinstall command is removed. These are + the extras that have to be carried forward. + + Never raises; an unreadable receipt yields an empty set. Callers that + must distinguish "none recorded" from "could not tell" should use + :func:`read_receipt` instead. + + Args: + prefix: Environment prefix whose receipt to read. Defaults to + ``sys.prefix``. + + Returns: + The recorded extra names, or an empty set. + """ + return read_receipt(prefix).extras + + +def installed_ref() -> str | None: + """Return a git ref to pin a reinstall to when no source was recorded. + + Only reached when neither the uv receipt nor ``direct_url.json`` names + an install source, so this is a best-effort guess: ``v`` from + package metadata assumes a matching tag exists. Returns ``None`` when + even that is unavailable, so the caller emits an unpinned spec rather + than a broken one. + + Returns: + A ref such as ``"v1.2.3"``, or ``None``. + """ + try: + return f"v{version(DISTRIBUTION)}" + except PackageNotFoundError: + logger.debug("No installed metadata for %s; emitting an unpinned spec", DISTRIBUTION) + return None + + +def extras_spec(extras: frozenset[str] | set[str]) -> str: + """Render a sorted, deduplicated PEP 508 extras spec. + + Args: + extras: The extra names to include. + + Returns: + Something like ``"conductor-cli[aca,tui]"``. + """ + return f"{DISTRIBUTION}[{','.join(sorted(extras))}]" + + +@dataclass(frozen=True) +class InstallEnvironment: + """Everything about the current install that the hint depends on. + + Separating this from :func:`render_install_command` keeps the branch + logic a pure function: every context can be unit-tested by constructing + one of these, with no fake ``sys.prefix``, no fake ``dist-info``, and no + real install. + """ + + context: InstallContext + """How Conductor was installed.""" + + extras: frozenset[str] = frozenset() + """Extras already recorded for this install, which must be carried forward.""" + + source: str | None = None + """Where it was installed from — the right-hand side of a PEP 508 ``@``.""" + + extras_known: bool = True + """``False`` when a receipt exists but could not be read or understood.""" + + receipt: str | None = None + """The receipt consulted, named in the warning when it could not be read.""" + + def __post_init__(self) -> None: + # `uv sync` names neither extras nor a source, so carrying them on an + # editable install would be state the renderer silently ignores. The + # type should not be able to hold a value that means nothing. + if self.context is InstallContext.EDITABLE: + object.__setattr__(self, "extras", frozenset()) + object.__setattr__(self, "source", None) + object.__setattr__(self, "extras_known", True) + object.__setattr__(self, "receipt", None) + + +def detect_environment(prefix: Path | None = None) -> InstallEnvironment: + """Inspect the running install and describe it. + + The uv receipt is checked first because a uv tool install *also* records + a ``direct_url.json``; only the receipt distinguishes a managed tool venv + from an ordinary one. + + Note *prefix* scopes the receipt lookup only — the editable check reads + the running distribution's own metadata, which no prefix can redirect. + + Args: + prefix: Environment prefix whose receipt to read. Defaults to + ``sys.prefix``. + + Returns: + The detected :class:`InstallEnvironment`. + """ + receipt_path = uv_receipt_path(prefix) + if receipt_path.is_file(): + receipt = read_receipt(prefix) + return InstallEnvironment( + InstallContext.UV_TOOL, + extras=receipt.extras, + source=receipt.source or _direct_url_source(), + extras_known=receipt.readable, + receipt=str(receipt_path), + ) + if _is_editable(): + return InstallEnvironment(InstallContext.EDITABLE) + return InstallEnvironment(InstallContext.UNKNOWN, source=_direct_url_source()) + + +def _fallback_source() -> str: + """Return a ``git+`` source for an install that recorded none.""" + ref = installed_ref() + return f"git+{REPO_GIT_URL}@{ref}" if ref is not None else f"git+{REPO_GIT_URL}" + + +def render_install_command(extra: ExtraName, env: InstallEnvironment) -> str: + """Render the install command for *extra* under *env*. Pure. + + Args: + extra: The extra to install, e.g. ``"tui"`` or ``"aca"``. + env: The install context to render for. + + Returns: + A single shell command. Quoting suits both POSIX shells and + PowerShell, the two shells the install scripts target. + """ + match env.context: + case InstallContext.UV_TOOL: + # Union, not replace: `--force` rewrites the whole requirement + # set, so omitting an already-installed extra uninstalls it. + spec = f"{extras_spec(env.extras | {extra})} @ {env.source or _fallback_source()}" + command = f"uv tool install --force '{spec}'" + if not env.extras_known: + # A shell comment, so the line stays safe to paste while + # saying plainly that the extras list may be incomplete — + # running it unedited could remove one. + named = f" {env.receipt}" if env.receipt else "" + command += ( + f" # WARNING: could not read{named} —" + " add any other extras you have before running this" + ) + return command + case InstallContext.EDITABLE: + # `uv sync` is exact by default, so without --inexact this would + # remove whatever another extra had installed — the same damage + # the union above exists to prevent. + return f"uv sync --inexact --extra {extra}" + case InstallContext.UNKNOWN: + spec = extras_spec({extra}) + # `python -m pip`, not a bare `pip`: the PATH pip does not manage + # a pipx venv (or any venv this interpreter is not on the PATH + # of), so a bare `pip install` there *succeeds* while installing a + # second copy the user never runs -- a loud failure turned into a + # silent wrong outcome. Naming this interpreter targets the + # environment conductor is actually installed in. + pip = f"{sys.executable} -m pip install" + # A pip/pipx install from git records the URL it came from, and + # putting it back is what makes the command resolve at all. + # Without one, pip resolves the extra against the already- + # installed distribution, which works for a wheel install. + return f"{pip} '{spec} @ {env.source}'" if env.source else f"{pip} '{spec}'" + + +def install_command(extra: ExtraName, prefix: Path | None = None) -> str: + """Return a copy-pasteable command that installs *extra* for this install. + + Never raises. This runs while an error message is being built — for a + missing provider dependency, or a missing TUI — so an exception here + would replace the diagnosis the user needs with a traceback. + + Args: + extra: The extra to install, e.g. ``"tui"`` or ``"aca"``. + prefix: Environment prefix whose receipt to read. Defaults to + ``sys.prefix``. + + Returns: + A single shell command. + """ + try: + return render_install_command(extra, detect_environment(prefix)) + except Exception: # noqa: BLE001 - a hint must never mask the error it explains + logger.warning("Could not resolve an install command for %r", extra, exc_info=True) + if sys.platform == "win32": + return ( + "$env:CONDUCTOR_INSTALL_EXTRAS = " + f"'{extra}'; irm https://aka.ms/conductor/install.ps1 | iex" + ) + return f"curl -sSfL https://aka.ms/conductor/install.sh | sh -s -- --extras {extra}" diff --git a/src/conductor/providers/aca.py b/src/conductor/providers/aca.py index 100e557e..cb35bb05 100644 --- a/src/conductor/providers/aca.py +++ b/src/conductor/providers/aca.py @@ -5,11 +5,17 @@ delegates agent execution to an in-sandbox ``conductor-agent-runner`` process running inside an Azure Container Apps dynamic-sessions pool. -The library is an optional dependency — install with: - pip install 'conductor-cli[aca]' -(pins ``azure-identity`` plus ``azure-core[aio]`` — the latter supplies the -``aiohttp``-based async transport the async ``DefaultAzureCredential`` in -this module requires; ``azure-identity`` alone does not include one.) +The library is an optional dependency. ``conductor run`` and ``conductor +doctor`` print the install command for the detected install context (see +:mod:`conductor.install_hint`); a hardcoded ``pip install`` string cannot +work on the documented install path, where a uv tool venv is not +pip-managed and ``conductor-cli`` is not on PyPI (issue #441). Note the +provider is constructed lazily, so this surfaces when the first agent on +it runs — not at ``conductor validate``. +The extra pins ``azure-identity`` plus ``azure-core[aio]`` — the latter +supplies the ``aiohttp``-based async transport the async +``DefaultAzureCredential`` in this module requires; ``azure-identity`` +alone does not include one. The transport shim (epic E3, issue #284) derives a session ``identifier`` from ``identifier_scope`` (DD5), acquires a cached AAD bearer token, issues a @@ -39,6 +45,7 @@ from pydantic import SecretStr from conductor.exceptions import ProviderError +from conductor.install_hint import install_command from conductor.providers.aca_protocol import ( RUNNER_TOKEN_HEADER, AcaAgentPayload, @@ -193,8 +200,9 @@ class AcaRuntimeProvider(AgentProvider): in-sandbox runner's NDJSON event stream verbatim to ``event_callback``, parsing the terminal ``result`` frame into :class:`AgentOutput`. - Requires the ``azure-identity`` package: - pip install 'conductor-cli[aca]' + Requires the ``azure-identity`` package, shipped by the ``aca`` extra. + See :mod:`conductor.install_hint` for how the install command is + resolved. Example: >>> provider = AcaRuntimeProvider(provider_settings=settings) @@ -297,7 +305,7 @@ def __init__( if not AZURE_IDENTITY_AVAILABLE: raise ProviderError( "aca provider requires the azure-identity package", - suggestion="Install with: uv add 'conductor-cli[aca]'", + suggestion=f"Install with: {install_command('aca')}", ) self._provider_settings = provider_settings diff --git a/src/conductor/providers/claude_agent_sdk.py b/src/conductor/providers/claude_agent_sdk.py index 9842fcdb..f250b179 100644 --- a/src/conductor/providers/claude_agent_sdk.py +++ b/src/conductor/providers/claude_agent_sdk.py @@ -14,6 +14,7 @@ from typing import TYPE_CHECKING, Any, Final, cast from conductor.exceptions import ProviderError +from conductor.install_hint import install_command from conductor.providers._schema import ( SchemaDepthError, build_json_schema_field, @@ -596,7 +597,7 @@ def __init__( if not CLAUDE_AGENT_SDK_AVAILABLE: raise ProviderError( "Claude Agent SDK not installed", - suggestion="Install with: uv add 'claude-agent-sdk>=0.2.82'", + suggestion=f"Install with: {install_command('claude-agent-sdk')}", ) self._default_model = model or _DEFAULT_MODEL diff --git a/src/conductor/providers/factory.py b/src/conductor/providers/factory.py index f6cf0728..3fa96279 100644 --- a/src/conductor/providers/factory.py +++ b/src/conductor/providers/factory.py @@ -10,6 +10,7 @@ from conductor.config.schema import ToolOutputConfig from conductor.exceptions import ProviderError +from conductor.install_hint import install_command from conductor.providers.aca import AZURE_IDENTITY_AVAILABLE, AcaRuntimeProvider from conductor.providers.base import AgentProvider from conductor.providers.claude import ANTHROPIC_SDK_AVAILABLE, ClaudeProvider @@ -178,7 +179,7 @@ async def create_provider( if not CLAUDE_AGENT_SDK_AVAILABLE: raise ProviderError( "Claude Agent SDK provider requires claude-agent-sdk package", - suggestion="Install with: uv add 'claude-agent-sdk>=0.2.82'", + suggestion=f"Install with: {install_command('claude-agent-sdk')}", ) # claude-agent-sdk delegates the agentic loop to the underlying # `claude` CLI, which exposes no hooks for sampling temperature or @@ -208,7 +209,7 @@ async def create_provider( if not AZURE_IDENTITY_AVAILABLE: raise ProviderError( "aca provider requires the azure-identity package", - suggestion="Install with: uv add 'conductor-cli[aca]'", + suggestion=f"Install with: {install_command('aca')}", ) if provider_settings is None or provider_settings.name != "aca": raise ProviderError( diff --git a/tests/test_cli/test_fleet_optional_dep.py b/tests/test_cli/test_fleet_optional_dep.py index d6578de1..b9ce2073 100644 --- a/tests/test_cli/test_fleet_optional_dep.py +++ b/tests/test_cli/test_fleet_optional_dep.py @@ -4,6 +4,10 @@ an actionable install hint and exits non-zero without a traceback; ``conductor fleet list`` (core, no optional dependency) still works in that state. + +Also covers issue #441: the hint is resolved from the detected install +context rather than hardcoding ``pip install 'conductor-cli[tui]'``, which +cannot work for anyone (``conductor-cli`` is not published to PyPI). """ from __future__ import annotations @@ -13,6 +17,7 @@ from typer.testing import CliRunner from conductor.cli.app import app +from conductor.install_hint import InstallContext, InstallEnvironment, render_install_command runner = CliRunner() @@ -20,13 +25,76 @@ class TestFleetWithoutTextual: """Simulates a clean install with no ``[tui]`` extra.""" - def test_bare_invocation_prints_install_hint(self) -> None: - with patch("conductor.cli.fleet.TEXTUAL_AVAILABLE", False): + def test_bare_invocation_prints_the_resolved_install_command(self) -> None: + with ( + patch("conductor.cli.fleet.TEXTUAL_AVAILABLE", False), + patch("conductor.cli.fleet.install_command", return_value="INSTALL-ME-NOW"), + ): result = runner.invoke(app, ["fleet"]) assert result.exit_code != 0 - assert "conductor-cli[tui]" in result.output - assert "pip install" in result.output + assert "tui" in result.output + assert "INSTALL-ME-NOW" in result.output + + def test_the_hint_asks_the_resolver_for_the_tui_extra(self) -> None: + with ( + patch("conductor.cli.fleet.TEXTUAL_AVAILABLE", False), + patch("conductor.cli.fleet.install_command", return_value="x") as resolver, + ): + runner.invoke(app, ["fleet"]) + + resolver.assert_called_once_with("tui") + + def test_a_uv_tool_install_is_never_told_to_use_pip(self) -> None: + """The regression issue #441 is about: `pip install + 'conductor-cli[tui]'` cannot work on the documented install path — + `conductor-cli` is not on PyPI and a uv tool venv is not + pip-managed.""" + env = InstallEnvironment(InstallContext.UV_TOOL, frozenset(), "v0.1.30") + with ( + patch("conductor.cli.fleet.TEXTUAL_AVAILABLE", False), + patch( + "conductor.cli.fleet.install_command", + return_value=render_install_command("tui", env), + ), + ): + result = runner.invoke(app, ["fleet"]) + + assert "pip install" not in result.output + assert "uv tool install --force" in result.output + + def test_brackets_in_the_resolved_command_survive_rich_markup(self) -> None: + """The command is a runtime value containing `[tui]`, and rich would + silently delete a lowercase bracketed token from a plain string. It + must go through `styled()`, which never hands values to the parser.""" + with ( + patch("conductor.cli.fleet.TEXTUAL_AVAILABLE", False), + patch( + "conductor.cli.fleet.install_command", + return_value="uv sync --extra tui # [tui] [aca]", + ), + ): + result = runner.invoke(app, ["fleet"]) + + assert "[tui] [aca]" in result.output + assert "Traceback" not in result.output + + def test_the_command_is_not_wrapped_across_lines(self) -> None: + """The resolved uv spec is longer than a default 80-column terminal. + Rich would word-wrap it, inserting a real newline that turns a + copy-paste into two broken commands.""" + long_command = ( + "uv tool install --force 'conductor-cli[aca,tui] @ " + "git+https://github.com/microsoft/conductor.git@v0.1.30'" + ) + assert len(long_command) > 80 + with ( + patch("conductor.cli.fleet.TEXTUAL_AVAILABLE", False), + patch("conductor.cli.fleet.install_command", return_value=long_command), + ): + result = runner.invoke(app, ["fleet"]) + + assert any(long_command in line for line in result.output.splitlines()) def test_bare_invocation_never_raises_a_traceback(self) -> None: """A missing optional dependency must surface as a clean CLI error, diff --git a/tests/test_cli/test_update.py b/tests/test_cli/test_update.py index 53b75b3a..9a9f6b88 100644 --- a/tests/test_cli/test_update.py +++ b/tests/test_cli/test_update.py @@ -597,19 +597,78 @@ def test_force_kwarg_accepted_for_back_compat(self, cache_dir: Path) -> None: run_update(c, force=True) -class TestRunUpdateApply: - """Tests for ``run_update(apply=True)`` — spawn-and-exit behavior.""" +@pytest.fixture() +def newer_release(cache_dir: Path): + """A cache/network state where a newer release is available.""" + with ( + patch( + "conductor.cli.update.fetch_latest_version", + return_value=("99.0.0", "v99.0.0", "https://example/release"), + ), + patch("conductor.cli.update.__version__", "0.1.0"), + ): + yield + + +class TestRunUpdateExtrasNote: + """Issue #441: `uv tool install --force` rewrites the tool's whole + requirement set, so an upgrade used to silently uninstall `[tui]`/`[aca]`. + Both install scripts now carry them forward; `conductor update` says so + before the user commits to the upgrade.""" + + def test_names_the_extras_that_will_be_preserved(self, newer_release) -> None: + c, buf = _make_console() + with patch("conductor.cli.update.installed_extras", return_value=frozenset({"tui", "aca"})): + run_update(c) + output = buf.getvalue() + assert "carried forward" in output.lower() + assert "aca, tui" in output - @pytest.fixture() - def newer_release(self, cache_dir: Path): + def test_says_nothing_when_no_extras_are_installed(self, newer_release) -> None: + """The common case: a bare install must not be told about a + preservation mechanism that has nothing to preserve.""" + c, buf = _make_console() + with patch("conductor.cli.update.installed_extras", return_value=frozenset()): + run_update(c) + assert "carried forward" not in buf.getvalue().lower() + + def test_the_note_also_appears_before_an_automatic_apply(self, newer_release) -> None: + """`--apply` hands off to the installer and never returns, so the note + has to be printed *before* the spawn or it is never seen. + + The mock raises SystemExit because that is the real contract: with a + plain returning mock this test passes even when the note is printed + after the handoff, which is precisely the regression it guards.""" + c, buf = _make_console() with ( + patch("conductor.cli.update.installed_extras", return_value=frozenset({"tui"})), patch( - "conductor.cli.update.fetch_latest_version", - return_value=("99.0.0", "v99.0.0", "https://example/release"), - ), - patch("conductor.cli.update.__version__", "0.1.0"), + "conductor.cli.update._spawn_installer_and_exit", + side_effect=SystemExit(0), + ) as spawn, + pytest.raises(SystemExit), ): - yield + run_update(c, apply=True) + assert "tui" in buf.getvalue() + spawn.assert_called_once() + + def test_a_failed_receipt_read_does_not_break_the_update(self, newer_release) -> None: + """`installed_extras` never raises by contract, but `conductor update` + must not be resting on that alone — it is the command a user reaches + for when something is already wrong. Simulate a real failure rather + than an empty result, which would just repeat the test above.""" + c, buf = _make_console() + with patch( + "conductor.cli.update.installed_extras", side_effect=OSError("receipt unreadable") + ): + run_update(c) + output = buf.getvalue() + assert "99.0.0" in output + assert "carried forward" not in output.lower() + + +class TestRunUpdateApply: + """Tests for ``run_update(apply=True)`` — spawn-and-exit behavior.""" def test_apply_does_not_print_paste_command(self, newer_release) -> None: """With --apply we should hand off to the installer, not print a manual command.""" diff --git a/tests/test_install_hint.py b/tests/test_install_hint.py new file mode 100644 index 00000000..112bce2e --- /dev/null +++ b/tests/test_install_hint.py @@ -0,0 +1,583 @@ +"""Tests for :mod:`conductor.install_hint` — the optional-extra install resolver. + +Issue #441: every hint pointing at an optional extra used to hardcode +``pip install 'conductor-cli[]'``, which cannot work on the documented +install path — a uv tool venv is not pip-managed, and ``conductor-cli`` is not +on PyPI so pip has nothing to resolve against there. + +The branch logic lives in the pure :func:`render_install_command`, so every +context is exercised by constructing an :class:`InstallEnvironment` rather than +faking a real install. Detection and receipt parsing are tested separately +against a throwaway prefix. + +Two properties get disproportionate attention here because getting them wrong +destroys user state rather than merely printing something unhelpful: the +rendered command must never *drop* an extra the user already has, and this +module must never raise, because it runs while an error message is being +built. +""" + +from __future__ import annotations + +import json +import sys +from importlib.metadata import PackageNotFoundError +from pathlib import Path + +import pytest + +from conductor.install_hint import ( + DISTRIBUTION, + REPO_GIT_URL, + InstallContext, + InstallEnvironment, + ReceiptContents, + detect_environment, + extras_spec, + install_command, + installed_extras, + installed_ref, + read_receipt, + render_install_command, + uv_receipt_path, +) + +RECEIPT_TUI = """\ +[tool] +requirements = [{ name = "conductor-cli", extras = ["tui"], \ +git = "https://github.com/microsoft/conductor.git?rev=v0.1.30" }] +entrypoints = [ + { name = "conductor", install-path = "/home/u/.local/bin/conductor", from = "conductor-cli" }, +] +""" + + +def write_receipt(prefix: Path, body: str) -> Path: + prefix.mkdir(parents=True, exist_ok=True) + receipt = prefix / "uv-receipt.toml" + receipt.write_text(body, encoding="utf-8") + return receipt + + +@pytest.fixture() +def direct_url(monkeypatch: pytest.MonkeyPatch): + """Stand in for the installed distribution's PEP 610 ``direct_url.json``.""" + + def _set(payload: dict) -> None: + monkeypatch.setattr("conductor.install_hint._direct_url", lambda: payload) + + return _set + + +class TestRenderInstallCommand: + """The pure branch logic — one assertion per detected context.""" + + def test_uv_tool_emits_a_pinned_direct_reference(self) -> None: + env = InstallEnvironment(InstallContext.UV_TOOL, source=f"git+{REPO_GIT_URL}@v0.1.30") + + assert render_install_command("tui", env) == ( + "uv tool install --force " + "'conductor-cli[tui] @ git+https://github.com/microsoft/conductor.git@v0.1.30'" + ) + + def test_editable_checkout_keeps_other_extras(self) -> None: + """`uv sync` is exact by default, so without --inexact this command + removes whatever another extra had installed — the same damage the + uv-tool branch unions to avoid.""" + assert ( + render_install_command("tui", InstallEnvironment(InstallContext.EDITABLE)) + == "uv sync --inexact --extra tui" + ) + + def test_unknown_install_falls_back_to_pip(self) -> None: + """Kept as the last-resort fallback: pip resolves an extra against an + already-installed distribution, which works for a wheel install.""" + env = InstallEnvironment(InstallContext.UNKNOWN) + + assert render_install_command("tui", env).endswith("-m pip install 'conductor-cli[tui]'") + + def test_the_pip_fallback_names_this_interpreter(self) -> None: + """A bare `pip` is not the pip that manages a pipx venv (or any venv + this interpreter is not on the PATH of). There it *succeeds*, quietly + installing a second copy the user never runs, while `conductor fleet` + keeps printing the same error — a loud failure turned into a silent + wrong outcome.""" + command = render_install_command("tui", InstallEnvironment(InstallContext.UNKNOWN)) + + assert command.startswith(f"{sys.executable} -m pip install") + + def test_unknown_install_from_git_keeps_the_url_it_came_from(self) -> None: + """A pip/pipx-from-git install is the population that actually lands in + UNKNOWN, and a bare name genuinely does not resolve for it. The URL is + recorded in direct_url.json, so putting it back is what makes the + command work rather than reproducing issue #441.""" + env = InstallEnvironment(InstallContext.UNKNOWN, source=f"git+{REPO_GIT_URL}@v0.1.30") + + assert render_install_command("aca", env).endswith( + "-m pip install 'conductor-cli[aca] @ " + "git+https://github.com/microsoft/conductor.git@v0.1.30'" + ) + + def test_uv_tool_preserves_already_installed_extras(self) -> None: + """`uv tool install --force` rewrites the tool's whole requirement set, + so a command that named only the requested extra would uninstall the + ones already there.""" + env = InstallEnvironment(InstallContext.UV_TOOL, frozenset({"tui"}), "git+x@v1") + + assert "conductor-cli[aca,tui] @" in render_install_command("aca", env) + + def test_requesting_an_already_installed_extra_does_not_duplicate_it(self) -> None: + env = InstallEnvironment(InstallContext.UV_TOOL, frozenset({"tui"}), "git+x@v1") + + assert "conductor-cli[tui] @" in render_install_command("tui", env) + + def test_extras_are_sorted_so_the_command_is_stable(self) -> None: + env = InstallEnvironment( + InstallContext.UV_TOOL, frozenset({"tui", "aca", "claude-agent-sdk"}), "git+x@v1" + ) + + assert "conductor-cli[aca,claude-agent-sdk,tui] @" in render_install_command("tui", env) + + def test_the_recorded_install_source_is_reused(self) -> None: + """Hardcoding the upstream URL would silently redirect a fork or a + locally-built install at microsoft/conductor's released tag.""" + env = InstallEnvironment( + InstallContext.UV_TOOL, source="git+https://github.com/me/fork.git@wip" + ) + + assert render_install_command("tui", env).endswith( + "'conductor-cli[tui] @ git+https://github.com/me/fork.git@wip'" + ) + + def test_an_unreadable_receipt_warns_inside_the_command(self) -> None: + """An unreadable receipt is not a bare install, and rendering it as one + produces a confident command that uninstalls the user's extras. The + caveat is a shell comment, so the line is still safe to paste.""" + env = InstallEnvironment(InstallContext.UV_TOOL, source="git+x@v1", extras_known=False) + + command = render_install_command("tui", env) + + assert "WARNING" in command + assert " #" in command + + def test_a_readable_receipt_carries_no_warning(self) -> None: + env = InstallEnvironment(InstallContext.UV_TOOL, source="git+x@v1") + + assert "WARNING" not in render_install_command("tui", env) + + @pytest.mark.parametrize("context", list(InstallContext)) + def test_every_context_names_the_requested_extra(self, context: InstallContext) -> None: + """A hint that loses the extra it was raised about is useless + regardless of which branch produced it.""" + assert "aca" in render_install_command("aca", InstallEnvironment(context)) + + def test_no_context_emits_the_dead_pypi_command_for_a_uv_tool_install(self) -> None: + """The regression this issue is about: a uv tool install can never be + fixed by `pip install`, because that venv is not pip-managed.""" + env = InstallEnvironment(InstallContext.UV_TOOL, source="git+x@v1") + + assert not render_install_command("tui", env).startswith("pip install") + + +class TestInstallEnvironment: + """The invariant lives in the type, not only in its factory.""" + + def test_an_editable_environment_cannot_carry_uv_tool_state(self) -> None: + """`uv sync` names neither extras nor a source, so holding them would + be state the renderer silently ignores.""" + env = InstallEnvironment( + InstallContext.EDITABLE, + frozenset({"tui"}), + "git+x@v1", + extras_known=False, + receipt="/x/uv-receipt.toml", + ) + + assert env.extras == frozenset() + assert env.source is None + assert env.extras_known is True + assert env.receipt is None + + def test_uv_tool_state_is_preserved(self) -> None: + env = InstallEnvironment(InstallContext.UV_TOOL, frozenset({"tui"}), "git+x@v1") + + assert env.extras == frozenset({"tui"}) + assert env.source == "git+x@v1" + + +class TestDetectEnvironment: + """Detection against a throwaway prefix — no real install involved.""" + + def test_uv_receipt_means_uv_tool(self, tmp_path: Path, direct_url) -> None: + direct_url({}) + write_receipt(tmp_path, RECEIPT_TUI) + + env = detect_environment(tmp_path) + + assert env.receipt == str(tmp_path / "uv-receipt.toml") + assert env.context is InstallContext.UV_TOOL + assert env.extras == frozenset({"tui"}) + assert env.source == "git+https://github.com/microsoft/conductor.git@v0.1.30" + + def test_editable_direct_url_means_editable(self, tmp_path: Path, direct_url) -> None: + direct_url({"url": "file:///src/conductor", "dir_info": {"editable": True}}) + + assert detect_environment(tmp_path).context is InstallContext.EDITABLE + + def test_neither_signal_is_unknown(self, tmp_path: Path, direct_url) -> None: + direct_url({}) + + assert detect_environment(tmp_path).context is InstallContext.UNKNOWN + + def test_a_non_editable_direct_url_is_unknown(self, tmp_path: Path, direct_url) -> None: + """A wheel built from a VCS checkout records `direct_url.json` too; + only `dir_info.editable` means a source checkout.""" + direct_url({"url": "https://x/y.whl", "dir_info": {"editable": False}}) + + assert detect_environment(tmp_path).context is InstallContext.UNKNOWN + + @pytest.mark.parametrize( + ("payload", "expected"), + [ + ( + {"url": "file:///tmp/dl/conductor_cli-1.0-py3-none-any.whl", "archive_info": {}}, + None, + ), + ({"url": "https://x/conductor_cli-1.0.tar.gz", "archive_info": {}}, None), + ({"url": "file:///src/conductor", "dir_info": {}}, "file:///src/conductor"), + ], + ids=["local-wheel", "remote-sdist", "local-directory"], + ) + def test_an_artifact_url_is_not_used_as_a_source( + self, tmp_path: Path, direct_url, payload: dict, expected: str | None + ) -> None: + """A wheel or sdist is one artifact, usually a download that has since + been cleaned up, so pinning to it produces a command that fails with + "No such file or directory". A bare name resolves against the + installed distribution instead. A *directory* is still there, so it is + usable.""" + direct_url(payload) + + assert detect_environment(tmp_path).source == expected + + def test_an_unknown_install_recovers_its_git_source(self, tmp_path: Path, direct_url) -> None: + direct_url( + { + "url": "https://github.com/microsoft/conductor.git", + "vcs_info": {"vcs": "git", "requested_revision": "v0.1.30"}, + } + ) + + assert ( + detect_environment(tmp_path).source + == "git+https://github.com/microsoft/conductor.git@v0.1.30" + ) + + def test_the_receipt_wins_over_an_editable_marker(self, tmp_path: Path, direct_url) -> None: + """A uv tool install writes *both* a receipt and a `direct_url.json`, + so the receipt has to be checked first or every tool install would be + misread.""" + direct_url({"dir_info": {"editable": True}}) + write_receipt(tmp_path, RECEIPT_TUI) + + assert detect_environment(tmp_path).context is InstallContext.UV_TOOL + + def test_an_unreadable_receipt_is_reported_as_such(self, tmp_path: Path, direct_url) -> None: + direct_url({}) + write_receipt(tmp_path, "this is not = valid toml [[[") + + assert detect_environment(tmp_path).extras_known is False + + def test_receipt_path_is_under_the_prefix(self, tmp_path: Path) -> None: + assert uv_receipt_path(tmp_path) == tmp_path / "uv-receipt.toml" + + +class TestReadReceipt: + """Receipt parsing. Never raises: this runs on an error path.""" + + def test_reads_a_single_extra_and_its_source(self, tmp_path: Path) -> None: + write_receipt(tmp_path, RECEIPT_TUI) + + contents = read_receipt(tmp_path) + + assert contents.extras == frozenset({"tui"}) + assert contents.source == "git+https://github.com/microsoft/conductor.git@v0.1.30" + assert contents.readable is True + + def test_reads_multiple_extras(self, tmp_path: Path) -> None: + write_receipt( + tmp_path, + '[tool]\nrequirements = [{ name = "conductor-cli", extras = ["tui", "aca"] }]\n', + ) + + assert read_receipt(tmp_path).extras == frozenset({"tui", "aca"}) + + def test_reads_a_local_directory_source(self, tmp_path: Path) -> None: + write_receipt( + tmp_path, + '[tool]\nrequirements = [{ name = "conductor-cli", directory = "/src/conductor" }]\n', + ) + + assert read_receipt(tmp_path).source == "/src/conductor" + + def test_matches_a_non_canonical_distribution_name(self, tmp_path: Path) -> None: + """PEP 503 says `conductor_cli` and `conductor-cli` are the same + project; a literal comparison would silently find no extras and the + upgrade would drop them.""" + write_receipt( + tmp_path, + '[tool]\nrequirements = [{ name = "Conductor_CLI", extras = ["tui"] }]\n', + ) + + assert read_receipt(tmp_path).extras == frozenset({"tui"}) + + def test_ignores_other_requirements(self, tmp_path: Path) -> None: + """`uv tool install --with X` adds requirements that are not this + distribution; their extras do not belong in the reinstall spec.""" + write_receipt( + tmp_path, + "[tool]\nrequirements = [\n" + ' { name = "other-pkg", extras = ["zzz"] },\n' + ' { name = "conductor-cli", extras = ["tui"] },\n' + "]\n", + ) + + assert read_receipt(tmp_path).extras == frozenset({"tui"}) + + def test_a_similarly_named_requirement_is_not_mistaken_for_this_one( + self, tmp_path: Path + ) -> None: + write_receipt( + tmp_path, + "[tool]\nrequirements = [\n" + ' { name = "conductor-cli-plugin", extras = ["evil"] },\n' + ' { name = "conductor-cli", extras = ["tui"] },\n' + "]\n", + ) + + assert read_receipt(tmp_path).extras == frozenset({"tui"}) + + def test_rejects_an_extra_name_that_would_break_the_quoting(self, tmp_path: Path) -> None: + """These values are interpolated into a single-quoted command the user + is told to paste.""" + write_receipt( + tmp_path, + "[tool]\nrequirements = " + '[{ name = "conductor-cli", extras = ["tui", "a\'; rm -rf ~; \'"] }]\n', + ) + + assert read_receipt(tmp_path).extras == frozenset({"tui"}) + + def test_no_extras_recorded_is_empty_but_readable(self, tmp_path: Path) -> None: + write_receipt(tmp_path, '[tool]\nrequirements = [{ name = "conductor-cli" }]\n') + + contents = read_receipt(tmp_path) + + assert contents.extras == frozenset() + assert contents.readable is True + + def test_missing_receipt_is_empty_and_readable(self, tmp_path: Path) -> None: + """A first-time install has nothing to lose, so this must not render + the unreadable-receipt warning.""" + assert read_receipt(tmp_path) == ReceiptContents() + + def test_malformed_receipt_is_flagged_unreadable(self, tmp_path: Path) -> None: + write_receipt(tmp_path, "this is not = valid toml [[[") + + assert read_receipt(tmp_path) == ReceiptContents(readable=False) + + def test_a_non_utf8_receipt_is_flagged_rather_than_raising(self, tmp_path: Path) -> None: + tmp_path.mkdir(parents=True, exist_ok=True) + (tmp_path / "uv-receipt.toml").write_bytes(b"\xff\xfe not utf-8") + + assert read_receipt(tmp_path).readable is False + + @pytest.mark.parametrize( + "body", + [ + 'tool = "not-a-table"\n', + "[[tool]]\nx = 1\n", + '[tool]\nrequirements = "not-a-list"\n', + '[tool]\nrequirements = [{ name = "something-else" }]\n', + ], + ids=["tool-is-a-string", "tool-is-an-array", "requirements-not-a-list", "no-entry"], + ) + def test_unexpected_shapes_are_flagged_rather_than_raising( + self, tmp_path: Path, body: str + ) -> None: + """A future uv could change the receipt schema. Degrading is + survivable; raising out of an error path is not, and reporting it as a + bare install would silently drop the user's extras.""" + write_receipt(tmp_path, body) + + assert read_receipt(tmp_path).readable is False + + def test_installed_extras_is_the_extras_half_of_read_receipt(self, tmp_path: Path) -> None: + write_receipt(tmp_path, RECEIPT_TUI) + + assert installed_extras(tmp_path) == frozenset({"tui"}) + + +class TestDirectUrl: + """The one function that touches real ``importlib.metadata``.""" + + def _distribution(self, monkeypatch: pytest.MonkeyPatch, payload) -> None: + class _Dist: + def read_text(self, _name: str): + if isinstance(payload, Exception): + raise payload + return payload + + monkeypatch.setattr("conductor.install_hint.distribution", lambda _n: _Dist()) + + def test_reads_and_parses_the_file(self, monkeypatch: pytest.MonkeyPatch) -> None: + from conductor.install_hint import _direct_url + + self._distribution(monkeypatch, json.dumps({"url": "https://x", "dir_info": {}})) + + assert _direct_url()["url"] == "https://x" + + def test_a_missing_distribution_is_empty(self, monkeypatch: pytest.MonkeyPatch) -> None: + from conductor.install_hint import _direct_url + + def _raise(_n: str): + raise PackageNotFoundError(DISTRIBUTION) + + monkeypatch.setattr("conductor.install_hint.distribution", _raise) + + assert _direct_url() == {} + + def test_a_non_utf8_file_does_not_raise(self, monkeypatch: pytest.MonkeyPatch) -> None: + """`Distribution.read_text` suppresses a handful of OSError subclasses + but not UnicodeDecodeError, which is a ValueError — so a corrupt file + used to escape as a traceback in place of the real error message.""" + from conductor.install_hint import _direct_url + + self._distribution(monkeypatch, UnicodeDecodeError("utf-8", b"\xff", 0, 1, "bad")) + + assert _direct_url() == {} + + def test_an_absent_file_is_empty(self, monkeypatch: pytest.MonkeyPatch) -> None: + from conductor.install_hint import _direct_url + + self._distribution(monkeypatch, None) + + assert _direct_url() == {} + + def test_unparseable_json_is_empty(self, monkeypatch: pytest.MonkeyPatch) -> None: + from conductor.install_hint import _direct_url + + self._distribution(monkeypatch, "{not json") + + assert _direct_url() == {} + + def test_non_object_json_is_empty(self, monkeypatch: pytest.MonkeyPatch) -> None: + from conductor.install_hint import _direct_url + + self._distribution(monkeypatch, "[1, 2, 3]") + + assert _direct_url() == {} + + +class TestInstalledRef: + """The last-resort pin, used only when no source was recorded anywhere.""" + + def test_falls_back_to_the_installed_version(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr("conductor.install_hint.version", lambda _dist: "9.9.9") + + assert installed_ref() == "v9.9.9" + + def test_returns_none_when_metadata_is_unavailable( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + def _raise(_dist: str) -> str: + raise PackageNotFoundError(DISTRIBUTION) + + monkeypatch.setattr("conductor.install_hint.version", _raise) + + assert installed_ref() is None + + +class TestInstallCommandNeverRaises: + """The public entry point is called while a ``ProviderError`` is being + constructed. An exception here does not degrade the hint — it deletes the + diagnosis the user needed.""" + + def test_a_broken_receipt_still_yields_a_command(self, tmp_path: Path, direct_url) -> None: + direct_url({}) + write_receipt(tmp_path, 'tool = "not-a-table"\n') + + assert install_command("tui", tmp_path) + + def test_an_exploding_detector_still_yields_a_command( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + def _boom(_prefix=None): + raise RuntimeError("detection blew up") + + monkeypatch.setattr("conductor.install_hint.detect_environment", _boom) + + command = install_command("tui") + + # The last resort is the install script, and which one depends on the + # platform — asserting the POSIX form unconditionally would fail on + # Windows for a command that is correct there. + script = "install.ps1" if sys.platform == "win32" else "install.sh" + assert script in command + assert "tui" in command + + @pytest.mark.parametrize("platform", ["win32", "linux"]) + def test_the_last_resort_names_the_right_installer_per_platform( + self, monkeypatch: pytest.MonkeyPatch, platform: str + ) -> None: + """`curl … | sh` is not runnable in PowerShell, so a POSIX-only last + resort would leave a Windows user with no working command at all — + the failure mode this module exists to remove.""" + + def _boom(_prefix=None): + raise RuntimeError("detection blew up") + + monkeypatch.setattr("conductor.install_hint.detect_environment", _boom) + monkeypatch.setattr(sys, "platform", platform) + + command = install_command("aca") + + expected = "install.ps1" if platform == "win32" else "install.sh" + assert expected in command + assert "aca" in command + + def test_composes_detection_and_rendering(self, tmp_path: Path, direct_url) -> None: + direct_url({}) + write_receipt(tmp_path, RECEIPT_TUI) + + assert install_command("aca", tmp_path) == ( + "uv tool install --force " + "'conductor-cli[aca,tui] @ git+https://github.com/microsoft/conductor.git@v0.1.30'" + ) + + +class TestExtrasSpec: + def test_sorts_and_names_the_distribution(self) -> None: + assert extras_spec({"tui", "aca"}) == "conductor-cli[aca,tui]" + + def test_uses_the_distribution_name_not_the_command_name(self) -> None: + """The command is `conductor`; the distribution is `conductor-cli`. + Naming the command here produces a spec that resolves to an unrelated + project.""" + assert extras_spec({"tui"}).startswith(f"{DISTRIBUTION}[") + + +class TestDeclaredExtrasAreReal: + """A typo in an extra name reproduces #441 exactly — a confidently printed + command that installs nothing.""" + + @pytest.mark.parametrize("extra", ["tui", "aca", "claude-agent-sdk"]) + def test_the_extra_is_declared_by_this_package(self, extra: str) -> None: + import tomllib + + pyproject = Path(__file__).resolve().parents[1] / "pyproject.toml" + declared = tomllib.loads(pyproject.read_text(encoding="utf-8"))["project"][ + "optional-dependencies" + ] + + assert extra in declared diff --git a/tests/test_integration/test_install_script_extras.py b/tests/test_integration/test_install_script_extras.py new file mode 100644 index 00000000..4830cd0e --- /dev/null +++ b/tests/test_integration/test_install_script_extras.py @@ -0,0 +1,738 @@ +"""Extras preservation across an install-script upgrade (issue #441). + +``uv tool install --force`` replaces the tool's *entire* requirement set, so +an upgrade that names no extras silently uninstalls ``[tui]``/``[aca]``. Both +install scripts therefore read the existing install's ``uv-receipt.toml`` and +rebuild the source as a PEP 508 direct reference carrying those extras. + +Both scripts' helpers are executed for real against fixture receipts, +because the failure mode here is a shell-quoting or pattern-matching mistake +that no amount of reading the file catches. ``install.sh`` runs under a POSIX +shell; ``install.ps1``'s helpers are extracted with PowerShell's own parser +and evaluated wherever a native ``pwsh`` exists, and a parity class then +feeds both implementations the same receipt. A handful of static checks cover +the wiring that is awkward to execute (flag names, the up-to-date gate). +""" + +from __future__ import annotations + +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +from conductor.install_hint import installed_extras + +REPO_ROOT = Path(__file__).resolve().parents[2] +INSTALL_SH = REPO_ROOT / "install.sh" +INSTALL_PS1 = REPO_ROOT / "install.ps1" + +# Gate on the platform, not just on `which`: GitHub's windows-latest images +# put Git-Bash's `sh.exe` on PATH, which satisfies `which("sh")` while this +# harness (POSIX `PATH`, a scrubbed env, a Windows path fed to `sh -c`) does +# not work there at all. The repo's precedent for this is a platform skip -- +# see test_install_scripts.py's shell-profile check. +SH = None if sys.platform == "win32" else shutil.which("sh") +requires_sh = pytest.mark.skipif(SH is None, reason="POSIX shell required") + +# Only a *native* PowerShell is usable. Under WSL, `powershell.exe` resolves +# through Windows PATH interop but cannot read Linux paths, so it would run +# the harness against a file it can't see and silently report no extras. +PWSH = shutil.which("pwsh") +if sys.platform == "win32": + PWSH = PWSH or shutil.which("powershell.exe") +requires_pwsh = pytest.mark.skipif(PWSH is None, reason="native PowerShell required") + +RECEIPT_BARE = '[tool]\nrequirements = [{ name = "conductor-cli" }]\n' + +RECEIPT_WITH_OTHER_PKG = ( + "[tool]\nrequirements = [\n" + ' { name = "other-pkg", extras = ["zzz"] },\n' + ' { name = "conductor-cli", extras = ["tui"] },\n' + "]\n" +) + +RECEIPT_WRAPPED = """\ +[tool] +requirements = [ + { name = "conductor-cli", extras = [ + "tui", + "aca", + ], git = "https://github.com/microsoft/conductor.git?rev=v0.1.30" }, +] +""" + +RECEIPT_NONCANONICAL = '[tool]\nrequirements = [{ name = "Conductor_CLI", extras = ["tui"] }]\n' + +RECEIPT_TUI = """\ +[tool] +requirements = [{ name = "conductor-cli", extras = ["tui"], \ +git = "https://github.com/microsoft/conductor.git?rev=v0.1.30" }] +entrypoints = [ + { name = "conductor", install-path = "/home/u/.local/bin/conductor", from = "conductor-cli" }, +] +""" + + +@pytest.fixture() +def sh_helpers(tmp_path: Path) -> Path: + """``install.sh`` with its ``main`` invocation stripped, so it can be sourced. + + Sourcing the real file (rather than a copy of the helper bodies) is the + point: a copy would drift from the script it claims to test. + """ + lines = INSTALL_SH.read_text(encoding="utf-8").splitlines() + assert lines[-1].strip() == "main", ( + "install.sh no longer ends with a bare `main` invocation; this fixture " + "strips that line so the script can be sourced without installing anything." + ) + stub = tmp_path / "install_sourceable.sh" + stub.write_text("\n".join(lines[:-1]) + "\n", encoding="utf-8") + return stub + + +def run_sh(stub: Path, snippet: str, uv_tool_dir: Path | None = None) -> str: + """Source the stripped install script and evaluate *snippet*. + + A fake ``uv`` is put on PATH so ``uv tool dir`` resolves to the fixture + directory without needing a real install. + """ + assert SH is not None + env = {"PATH": str(stub.parent / "bin") + ":/usr/bin:/bin", "HOME": str(stub.parent)} + fake_bin = stub.parent / "bin" + fake_bin.mkdir(exist_ok=True) + fake_uv = fake_bin / "uv" + fake_uv.write_text( + "#!/bin/sh\n" + 'if [ "$1" = "tool" ] && [ "$2" = "dir" ]; then\n' + f' printf "%s\\n" "{uv_tool_dir or ""}"\n' + " exit 0\n" + "fi\n" + "exit 1\n", + encoding="utf-8", + ) + fake_uv.chmod(0o755) + + proc = subprocess.run( + [SH, "-c", f". {stub}\n{snippet}"], + capture_output=True, + text=True, + env=env, + timeout=30, + ) + assert proc.returncode == 0, f"snippet failed:\n{proc.stdout}\n{proc.stderr}" + return proc.stdout.strip() + + +@requires_sh +class TestInstallShExtras: + """The real ``install.sh`` helpers, executed.""" + + def test_reads_the_extras_recorded_in_the_receipt(self, sh_helpers: Path) -> None: + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text(RECEIPT_TUI, encoding="utf-8") + + assert run_sh(sh_helpers, "receipt_extras", tools) == "tui" + + def test_reads_several_extras(self, sh_helpers: Path) -> None: + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text( + '[tool]\nrequirements = [{ name = "conductor-cli", extras = ["tui", "aca"] }]\n', + encoding="utf-8", + ) + + assert run_sh(sh_helpers, "receipt_extras", tools) == "tui,aca" + + def test_ignores_extras_belonging_to_other_requirements(self, sh_helpers: Path) -> None: + """`uv tool install --with X` adds requirements that are not this + distribution; folding their extras into the spec would install + something the user never asked for.""" + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text( + "[tool]\nrequirements = [\n" + ' { name = "other-pkg", extras = ["zzz"] },\n' + ' { name = "conductor-cli", extras = ["tui"] },\n' + "]\n", + encoding="utf-8", + ) + + assert run_sh(sh_helpers, "receipt_extras", tools) == "tui" + + def test_a_bare_install_reports_no_extras(self, sh_helpers: Path) -> None: + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text( + '[tool]\nrequirements = [{ name = "conductor-cli" }]\n', encoding="utf-8" + ) + + assert run_sh(sh_helpers, "receipt_extras", tools) == "" + + def test_a_missing_receipt_is_not_an_error(self, sh_helpers: Path) -> None: + """A first-time install has no receipt. `set -e` is active, so a + non-zero exit here would abort the whole installer.""" + assert run_sh(sh_helpers, "receipt_extras", sh_helpers.parent / "nope") == "" + + def test_an_unusable_uv_tool_dir_is_not_an_error(self, sh_helpers: Path) -> None: + """`uv tool dir` can print nothing (or fail) on an older uv; that must + degrade to "no extras", not abort the installer under `set -e`.""" + assert run_sh(sh_helpers, "receipt_extras", None) == "" + + def test_merge_sorts_and_deduplicates(self, sh_helpers: Path) -> None: + assert run_sh(sh_helpers, 'merge_extras "tui,aca" "tui"') == "aca,tui" + + def test_merge_of_nothing_is_empty(self, sh_helpers: Path) -> None: + assert run_sh(sh_helpers, 'merge_extras "" ""') == "" + + def test_merge_tolerates_whitespace_in_a_user_supplied_list(self, sh_helpers: Path) -> None: + assert run_sh(sh_helpers, 'merge_extras "" " tui , aca "') == "aca,tui" + + def test_extras_become_a_pep508_direct_reference(self, sh_helpers: Path) -> None: + out = run_sh( + sh_helpers, + 'apply_extras "git+https://github.com/microsoft/conductor.git@v1.2.3" "aca,tui"', + ) + + assert out == ( + "conductor-cli[aca,tui] @ git+https://github.com/microsoft/conductor.git@v1.2.3" + ) + + def test_no_extras_leaves_the_source_untouched(self, sh_helpers: Path) -> None: + """A bare install must keep passing the plain git source, so a + first-time install is byte-for-byte what it always was.""" + out = run_sh(sh_helpers, 'apply_extras "git+https://example.com/x.git@v1" ""') + + assert out == "git+https://example.com/x.git@v1" + + def test_a_wrapped_requirements_array_is_still_parsed(self, sh_helpers: Path) -> None: + """uv may wrap the requirements array across lines, which is the whole + reason the parser flattens newlines first. Without a wrapped fixture, + deleting that flatten breaks nothing and the extras would be silently + dropped on the next upgrade.""" + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text(RECEIPT_WRAPPED, encoding="utf-8") + + assert run_sh(sh_helpers, 'merge_extras "$(receipt_extras)" ""', tools) == "aca,tui" + + def test_a_similarly_named_requirement_is_not_mistaken_for_this_one( + self, sh_helpers: Path + ) -> None: + """`conductor-cli-plugin` contains `conductor-cli`. Matching the bare + substring would fold a `--with` package's extras into our spec and + install something the user never asked for.""" + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text( + "[tool]\nrequirements = [\n" + ' { name = "conductor-cli-plugin", extras = ["evil"] },\n' + ' { name = "conductor-cli", extras = ["tui"] },\n' + "]\n", + encoding="utf-8", + ) + + assert run_sh(sh_helpers, "receipt_extras", tools) == "tui" + + def test_merge_lower_cases_so_the_comparison_converges(self, sh_helpers: Path) -> None: + """PowerShell's `Sort-Object -Unique` is case-insensitive. If `sort -u` + here is not, `--extras TUI` against a recorded `tui` yields a resolved + set that never equals the installed one, so every run reinstalls.""" + assert run_sh(sh_helpers, 'merge_extras "tui" "TUI"', sh_helpers.parent) == "tui" + + def test_an_unknown_extra_is_refused(self, sh_helpers: Path) -> None: + """uv treats an unknown extra as a warning and still exits 0, so a typo + would otherwise install nothing and report success.""" + assert run_sh(sh_helpers, "validate_extras tui,aca", sh_helpers.parent) == "" + with pytest.raises(AssertionError): + run_sh(sh_helpers, "validate_extras tuii", sh_helpers.parent) + + def test_an_unreadable_receipt_is_reported_not_treated_as_bare(self, sh_helpers: Path) -> None: + """The destructive case. An unreadable receipt reported as "no extras" + rebuilds the tool without them, which is the #441 data loss this whole + change exists to prevent — so the helper has to fail rather than + return empty.""" + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + receipt = tools / "conductor-cli" / "uv-receipt.toml" + receipt.write_text(RECEIPT_TUI, encoding="utf-8") + receipt.chmod(0o000) + try: + with pytest.raises(AssertionError): + run_sh(sh_helpers, "receipt_extras", tools) + finally: + receipt.chmod(0o644) + + def test_a_receipt_without_our_requirement_is_reported_unreadable( + self, sh_helpers: Path + ) -> None: + """Schema drift must not read as "no extras" either, for the same + reason — `read_receipt` draws the same line on the Python side.""" + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text( + '[tool]\nrequirements = [{ name = "something-else" }]\n', encoding="utf-8" + ) + + with pytest.raises(AssertionError): + run_sh(sh_helpers, "receipt_extras", tools) + + def test_a_non_canonical_distribution_name_still_matches(self, sh_helpers: Path) -> None: + """PEP 503 makes `Conductor_CLI` the same project. The Python reader + normalises; a case-sensitive shell match would find nothing here and + drop the extras on upgrade.""" + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text( + RECEIPT_NONCANONICAL, encoding="utf-8" + ) + + assert run_sh(sh_helpers, "receipt_extras", tools) == "tui" + + def test_an_extra_is_accepted_regardless_of_case(self, sh_helpers: Path) -> None: + """PowerShell's `-notcontains` is case-insensitive; rejecting `TUI` + only on POSIX would be a new cross-platform divergence.""" + assert run_sh(sh_helpers, "validate_extras TUI", sh_helpers.parent) == "" + + @pytest.mark.parametrize( + "receipt", + [RECEIPT_TUI, RECEIPT_WRAPPED, RECEIPT_BARE, RECEIPT_WITH_OTHER_PKG, RECEIPT_NONCANONICAL], + ids=["one-extra", "wrapped", "bare", "other-pkg", "non-canonical-name"], + ) + def test_the_shell_and_python_parsers_read_the_same_receipt( + self, sh_helpers: Path, receipt: str + ) -> None: + """`install.sh` parses the receipt with sed and `conductor.install_hint` + parses it with tomllib. They cannot share an implementation -- the + installer runs before any conductor exists -- so a shared oracle is the + only thing keeping them honest. This is the parity that matters: + sh-vs-ps1 can only catch divergence, never a bug both inherited.""" + tools = sh_helpers.parent / "tools" + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text(receipt, encoding="utf-8") + + from_sh = run_sh(sh_helpers, 'merge_extras "$(receipt_extras)" ""', tools) + from_python = ",".join(sorted(installed_extras(tools / "conductor-cli"))) + + assert from_sh == from_python + + +class TestInstallScriptsStayInSync: + """Both installers have to preserve extras, or upgrading on one platform + silently drops what the other kept. + + These are the checks that are awkward to execute (they live in the + scripts' argument-parsing preamble rather than in a helper). Each asserts + on the *wiring*, not on a string that also appears in the comment header + -- an earlier version of this class passed against scripts whose parsing + had been deleted, because the needle was still in the docs block. + """ + + @pytest.mark.parametrize("script", [INSTALL_SH, INSTALL_PS1], ids=["sh", "ps1"]) + def test_builds_a_direct_reference_carrying_the_extras(self, script: Path) -> None: + assert "conductor-cli[" in script.read_text(encoding="utf-8"), ( + f"{script.name} does not build a `conductor-cli[] @ ` " + "direct reference, which is the only shape that carries extras through " + "`uv tool install`." + ) + + @pytest.mark.parametrize( + ("script", "needle"), + [ + (INSTALL_SH, 'EXTRAS="${CONDUCTOR_INSTALL_EXTRAS:-}"'), + (INSTALL_PS1, "$Extras = $env:CONDUCTOR_INSTALL_EXTRAS"), + ], + ids=["sh", "ps1"], + ) + def test_accepts_an_explicit_extras_request(self, script: Path, needle: str) -> None: + assert needle in script.read_text(encoding="utf-8"), ( + f"{script.name} does not read CONDUCTOR_INSTALL_EXTRAS, so there is no way " + "to add an extra through the documented `curl | sh` / `irm | iex` install." + ) + + @pytest.mark.parametrize( + ("script", "needle"), + [ + (INSTALL_SH, 'NO_PRESERVE_EXTRAS="${CONDUCTOR_INSTALL_NO_PRESERVE_EXTRAS:-0}"'), + (INSTALL_PS1, "$NoPreserveExtras = $true"), + ], + ids=["sh", "ps1"], + ) + def test_offers_a_way_back_to_a_bare_install(self, script: Path, needle: str) -> None: + assert needle in script.read_text(encoding="utf-8"), ( + f"{script.name} does not wire up the opt-out, so a user who installed " + "[aca] once could never get back to a bare install through the installer." + ) + + @pytest.mark.parametrize( + ("script", "needle"), + [ + (INSTALL_SH, 'apply_extras "$install_source" "$resolved_extras"'), + (INSTALL_PS1, "(Format-ProcessArgument $InstallSource)"), + ], + ids=["sh", "ps1"], + ) + def test_the_spec_reaches_the_installer_intact(self, script: Path, needle: str) -> None: + """PowerShell's `-ArgumentList` does not quote its elements, so the + spec has to be routed through `Format-ProcessArgument` or uv receives + `conductor-cli[tui]`, `@` and the URL as three arguments.""" + assert needle in script.read_text(encoding="utf-8"), ( + f"{script.name} no longer passes the install source through the helper that " + "keeps it a single argument; the extras would be silently dropped." + ) + + @pytest.mark.parametrize( + ("script", "needle"), + [ + (INSTALL_SH, '[ "$resolved_extras" = "$receipt_now" ]'), + (INSTALL_PS1, "$resolvedExtras -eq $receiptNow"), + ], + ids=["sh", "ps1"], + ) + def test_the_up_to_date_shortcut_compares_against_what_is_installed( + self, script: Path, needle: str + ) -> None: + """The gate must compare against the extras *on disk*, not against the + set this run decided to carry -- `--no-preserve-extras` zeroes the + latter, which made both sides equal and turned the flag into a no-op + that reported success.""" + assert needle in script.read_text(encoding="utf-8"), ( + f"{script.name}'s already-up-to-date early return does not compare against " + "the installed extras, so --extras/--no-preserve-extras on a current " + "install is silently a no-op." + ) + + +PS_HELPERS = ( + "Get-ReceiptExtras", + "Merge-Extras", + "Add-ExtrasToSource", + "Test-ExtrasKnown", + "Format-ProcessArgument", +) + +# Extract the three helpers from install.ps1 via PowerShell's own parser and +# evaluate just those, so the real script text is exercised without running +# an install. `Get-ConductorToolDir` is then overridden to point at the +# fixture receipt. A brace-counting extraction would not work here: the +# helpers contain `}` inside string literals. +PS_HARNESS = """ +$ErrorActionPreference = 'Stop' +$tokens = $null; $errors = $null +$ast = [System.Management.Automation.Language.Parser]::ParseFile( + '{script}', [ref]$tokens, [ref]$errors) +if ($errors.Count -gt 0) {{ Write-Error ($errors | Out-String); exit 1 }} +$want = @({wanted}) +$fns = $ast.FindAll({{ param($n) + $n -is [System.Management.Automation.Language.FunctionDefinitionAst] -and + $want -contains $n.Name }}, $true) +if ($fns.Count -ne {count}) {{ + Write-Error "expected {count} helpers, found $($fns.Count)"; exit 1 +}} +foreach ($f in $fns) {{ Invoke-Expression $f.Extent.Text }} +function Get-ConductorToolDir {{ return '{tool_dir}' }} +{snippet} +""" + + +def run_pwsh(snippet: str, tool_dir: Path | str) -> str: + """Evaluate *snippet* with install.ps1's extras helpers in scope.""" + assert PWSH is not None + script = PS_HARNESS.format( + script=str(INSTALL_PS1).replace("'", "''"), + wanted=",".join(f"'{name}'" for name in PS_HELPERS), + count=len(PS_HELPERS), + tool_dir=str(tool_dir).replace("'", "''"), + snippet=snippet, + ) + proc = subprocess.run( + [PWSH, "-NoProfile", "-NonInteractive", "-Command", script], + capture_output=True, + text=True, + timeout=120, + ) + assert proc.returncode == 0, f"snippet failed:\n{proc.stdout}\n{proc.stderr}" + return proc.stdout.strip() + + +@requires_pwsh +class TestInstallPs1Extras: + """The real ``install.ps1`` helpers, executed. + + Runs on the Windows CI job (and anywhere ``pwsh`` is on PATH). Without + this, the PowerShell half of the feature is only ever grepped — and the + two installers producing *different* extras is exactly the failure this + change exists to prevent. + """ + + @pytest.fixture() + def tool_dir(self, tmp_path: Path) -> Path: + d = tmp_path / "conductor-cli" + d.mkdir() + return d + + def test_reads_the_extras_recorded_in_the_receipt(self, tool_dir: Path) -> None: + (tool_dir / "uv-receipt.toml").write_text(RECEIPT_TUI, encoding="utf-8") + + assert run_pwsh("Write-Output (Get-ReceiptExtras)", tool_dir) == "tui" + + def test_ignores_extras_belonging_to_other_requirements(self, tool_dir: Path) -> None: + (tool_dir / "uv-receipt.toml").write_text( + "[tool]\nrequirements = [\n" + ' { name = "other-pkg", extras = ["zzz"] },\n' + ' { name = "conductor-cli", extras = ["tui"] },\n' + "]\n", + encoding="utf-8", + ) + + assert run_pwsh("Write-Output (Get-ReceiptExtras)", tool_dir) == "tui" + + def test_a_bare_install_reports_no_extras(self, tool_dir: Path) -> None: + (tool_dir / "uv-receipt.toml").write_text( + '[tool]\nrequirements = [{ name = "conductor-cli" }]\n', encoding="utf-8" + ) + + assert run_pwsh('Write-Output ("[" + (Get-ReceiptExtras) + "]")', tool_dir) == "[]" + + def test_a_missing_receipt_is_not_an_error(self, tool_dir: Path) -> None: + assert run_pwsh('Write-Output ("[" + (Get-ReceiptExtras) + "]")', tool_dir) == "[]" + + def test_quotes_an_argument_containing_whitespace(self, tool_dir: Path) -> None: + """`Start-Process -ArgumentList` joins elements with spaces and does + not quote them, so an unquoted `conductor-cli[tui] @ ` reached uv + as three separate arguments and the extras were silently dropped.""" + out = run_pwsh("Write-Output (Format-ProcessArgument 'a b c')", tool_dir) + + assert out == '"a b c"' + + def test_leaves_a_whitespace_free_argument_alone(self, tool_dir: Path) -> None: + out = run_pwsh("Write-Output (Format-ProcessArgument 'git+https://x@v1')", tool_dir) + + assert out == "git+https://x@v1" + + def test_an_unreadable_receipt_is_reported_not_treated_as_bare(self, tool_dir: Path) -> None: + (tool_dir / "uv-receipt.toml").write_text( + '[tool]\nrequirements = [{ name = "something-else" }]\n', encoding="utf-8" + ) + + assert run_pwsh("Write-Output ($null -eq (Get-ReceiptExtras))", tool_dir) == "True" + + def test_a_bare_install_is_empty_not_null(self, tool_dir: Path) -> None: + """Empty means "understood, nothing recorded"; null means "could not + tell". Collapsing them is what made an unreadable receipt destructive.""" + (tool_dir / "uv-receipt.toml").write_text( + '[tool]\nrequirements = [{ name = "conductor-cli" }]\n', encoding="utf-8" + ) + + assert run_pwsh("Write-Output ('' -eq (Get-ReceiptExtras))", tool_dir) == "True" + + def test_merge_sorts_and_deduplicates(self, tool_dir: Path) -> None: + out = run_pwsh("Write-Output (Merge-Extras 'tui,aca' ' tui ')", tool_dir) + + assert out == "aca,tui" + + def test_merge_of_nothing_is_empty(self, tool_dir: Path) -> None: + assert run_pwsh("Write-Output ('[' + (Merge-Extras '' '') + ']')", tool_dir) == "[]" + + def test_extras_become_a_pep508_direct_reference(self, tool_dir: Path) -> None: + out = run_pwsh( + "Write-Output (Add-ExtrasToSource " + "'git+https://github.com/microsoft/conductor.git@v1.2.3' 'aca,tui')", + tool_dir, + ) + + assert out == ( + "conductor-cli[aca,tui] @ git+https://github.com/microsoft/conductor.git@v1.2.3" + ) + + def test_no_extras_leaves_the_source_untouched(self, tool_dir: Path) -> None: + out = run_pwsh( + "Write-Output (Add-ExtrasToSource 'git+https://example.com/x.git@v1' '')", tool_dir + ) + + assert out == "git+https://example.com/x.git@v1" + + +@requires_sh +@requires_pwsh +class TestBothInstallersAgree: + """The two implementations are separate code in separate languages; the + only thing keeping them honest is running both against the same input.""" + + @pytest.mark.parametrize( + "receipt", + [ + RECEIPT_TUI, + '[tool]\nrequirements = [{ name = "conductor-cli", extras = ["tui", "aca"] }]\n', + '[tool]\nrequirements = [{ name = "conductor-cli" }]\n', + ], + ids=["one-extra", "two-extras", "no-extras"], + ) + def test_the_same_receipt_yields_the_same_extras( + self, sh_helpers: Path, tmp_path: Path, receipt: str + ) -> None: + sh_tools = sh_helpers.parent / "tools" + (sh_tools / "conductor-cli").mkdir(parents=True) + (sh_tools / "conductor-cli" / "uv-receipt.toml").write_text(receipt, encoding="utf-8") + + ps_dir = tmp_path / "ps" / "conductor-cli" + ps_dir.mkdir(parents=True) + (ps_dir / "uv-receipt.toml").write_text(receipt, encoding="utf-8") + + from_sh = run_sh(sh_helpers, 'merge_extras "$(receipt_extras)" ""', sh_tools) + from_ps = run_pwsh("Write-Output (Merge-Extras (Get-ReceiptExtras) '')", ps_dir) + + assert from_sh == from_ps + + +@requires_sh +class TestExtrasDecisionEndToEnd: + """Drives the real ``install.sh`` and asserts on the requirement it hands + to ``uv tool install``. + + The three helpers can each be correct while the block that *uses* them is + not — which is exactly what happened: ``--no-preserve-extras`` compared the + resolved set against a variable the flag itself had zeroed, so the + up-to-date shortcut fired and the opt-out did nothing while reporting + success. Only a test that runs the script can see that, and only one that + inspects the real argv can prove the extras survived into the spec. + + ``uv`` is faked so the run is hermetic and instant: a real one would fail + to resolve the throwaway source and burn the script's retry backoff. + """ + + def run_installer( + self, + tmp_path: Path, + receipt: str | None, + *args: str, + installed_version: str | None = None, + ) -> tuple[str, str]: + """Run the installer; return (output, the spec passed to ``uv``). + + With *installed_version* set, ``curl`` and ``conductor`` are faked too + so the script takes its **release** path — the only one where the + already-up-to-date shortcut is reachable. That shortcut is gated on + ``[ -z "$SOURCE" ]``, so a ``--source`` run can never exercise it, and + the regression it guards would go unnoticed. + """ + tools = tmp_path / "tools" + tools.mkdir(parents=True) + if receipt is not None: + (tools / "conductor-cli").mkdir(parents=True) + (tools / "conductor-cli" / "uv-receipt.toml").write_text(receipt, encoding="utf-8") + + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + argv_log = tmp_path / "uv-argv.txt" + fake_uv = bin_dir / "uv" + fake_uv.write_text( + "#!/bin/sh\n" + f'if [ "$1" = "tool" ] && [ "$2" = "dir" ]; then printf "%s\\n" "{tools}"; exit 0; fi\n' + f'if [ "$1" = "tool" ] && [ "$2" = "install" ]; then\n' + f' printf "%s\\n" "$4" > "{argv_log}"\n' + "fi\n" + "exit 0\n", + encoding="utf-8", + ) + fake_uv.chmod(0o755) + + script_args = ["--force", "--skip-path-update", *args] + if installed_version is None: + script_args = ["--source", str(tmp_path / "src"), *script_args] + else: + fake_curl = bin_dir / "curl" + # The release path fetches the latest tag, then downloads + # constraints; failing the latter makes the script warn and carry + # on, which keeps this hermetic. + fake_curl.write_text( + "#!/bin/sh\n" + 'for a in "$@"; do [ "$a" = "-o" ] && exit 1; done\n' + f'printf \'{{"tag_name": "v{installed_version}"}}\'\n', + encoding="utf-8", + ) + fake_curl.chmod(0o755) + fake_conductor = bin_dir / "conductor" + fake_conductor.write_text( + f"#!/bin/sh\nprintf 'Conductor v{installed_version}\\n'\n", encoding="utf-8" + ) + fake_conductor.chmod(0o755) + + proc = subprocess.run( + [str(INSTALL_SH), *script_args], + capture_output=True, + text=True, + timeout=60, + env={ + "PATH": f"{bin_dir}:/usr/bin:/bin", + "HOME": str(tmp_path), + "UV_TOOL_DIR": str(tools), + }, + ) + spec = argv_log.read_text(encoding="utf-8").strip() if argv_log.exists() else "" + return proc.stdout + proc.stderr, spec + + def test_an_upgrade_preserves_the_recorded_extras(self, tmp_path: Path) -> None: + out, spec = self.run_installer(tmp_path, RECEIPT_TUI) + + assert "Including extras: tui" in out + assert spec.startswith("conductor-cli[tui] @ ") + + def test_requested_extras_are_added_to_the_recorded_ones(self, tmp_path: Path) -> None: + out, spec = self.run_installer(tmp_path, RECEIPT_TUI, "--extras", "aca") + + assert "Including extras: aca,tui" in out + assert spec.startswith("conductor-cli[aca,tui] @ ") + + def test_no_preserve_extras_actually_drops_them(self, tmp_path: Path) -> None: + """The regression: with the opt-out set, the script must not report + "already up to date" and leave the extras installed.""" + out, spec = self.run_installer(tmp_path, RECEIPT_TUI, "--no-preserve-extras") + + assert "Dropping extras: tui" in out + assert "Including extras" not in out + assert "already installed and up to date" not in out + assert "conductor-cli[" not in spec + + def test_an_unknown_extra_aborts_before_installing(self, tmp_path: Path) -> None: + out, spec = self.run_installer(tmp_path, RECEIPT_TUI, "--extras", "tuii") + + assert "unknown extra 'tuii'" in out + assert spec == "" + + def test_a_first_time_install_needs_no_receipt(self, tmp_path: Path) -> None: + out, spec = self.run_installer(tmp_path, None) + + assert "Including extras" not in out + assert "Dropping extras" not in out + assert "conductor-cli[" not in spec + + def test_an_up_to_date_install_with_matching_extras_is_a_no_op(self, tmp_path: Path) -> None: + out, spec = self.run_installer(tmp_path, RECEIPT_TUI, installed_version="1.2.3") + + assert "already installed and up to date" in out + assert spec == "" + + def test_no_preserve_extras_defeats_the_up_to_date_shortcut(self, tmp_path: Path) -> None: + """The regression, on the path where it actually bit: the gate used to + compare the resolved set against a variable this flag had just zeroed, + so both sides matched, the shortcut fired, and the extras stayed + installed under a green "already up to date".""" + out, spec = self.run_installer( + tmp_path, RECEIPT_TUI, "--no-preserve-extras", installed_version="1.2.3" + ) + + assert "already installed and up to date" not in out + assert "Dropping extras: tui" in out + assert "conductor-cli[" not in spec + + def test_a_newly_requested_extra_defeats_the_up_to_date_shortcut(self, tmp_path: Path) -> None: + out, spec = self.run_installer( + tmp_path, RECEIPT_TUI, "--extras", "aca", installed_version="1.2.3" + ) + + assert "already installed and up to date" not in out + assert spec.startswith("conductor-cli[aca,tui] @ ") diff --git a/tests/test_providers/test_aca.py b/tests/test_providers/test_aca.py index a83ae844..df752b07 100644 --- a/tests/test_providers/test_aca.py +++ b/tests/test_providers/test_aca.py @@ -103,7 +103,7 @@ async def test_async_default_azure_credential_constructs_without_import_error(se `azure-core[aio]`) is present.""" pytest.importorskip( "azure.identity.aio", - reason="aca extra not installed (pip install 'conductor-cli[aca]')", + reason="aca extra not installed (see docs/providers/aca.md)", ) from azure.identity.aio import DefaultAzureCredential @@ -137,6 +137,43 @@ async def test_factory_error_includes_install_suggestion(self) -> None: assert exc_info.value.suggestion is not None assert "aca" in exc_info.value.suggestion + @patch("conductor.providers.factory.CLAUDE_AGENT_SDK_AVAILABLE", False) + @patch("conductor.providers.factory.install_command", return_value="RESOLVED-COMMAND") + @pytest.mark.asyncio + async def test_factory_resolves_the_claude_agent_sdk_suggestion(self, resolver) -> None: + """The other extra reached through the factory. Both were hardcoded to + commands that cannot work on the documented install path (#441).""" + with pytest.raises(ProviderError) as exc_info: + await create_provider("claude-agent-sdk", validate=False) + assert exc_info.value.suggestion == "Install with: RESOLVED-COMMAND" + resolver.assert_called_once_with("claude-agent-sdk") + + @patch("conductor.providers.factory.AZURE_IDENTITY_AVAILABLE", False) + @patch("conductor.providers.factory.install_command", return_value="RESOLVED-COMMAND") + @pytest.mark.asyncio + async def test_factory_suggestion_is_resolved_not_hardcoded(self, _resolver) -> None: + """Issue #441: the suggestion used to hardcode + ``uv add 'conductor-cli[aca]'``, which fails outside a uv project and + cannot install into the uv tool venv the install script creates. It + now comes from the detected install context.""" + settings = ProviderSettings(name="aca", pool_endpoint="https://pool.example.com") + with pytest.raises(ProviderError) as exc_info: + await create_provider("aca", validate=False, provider_settings=settings) + assert exc_info.value.suggestion == "Install with: RESOLVED-COMMAND" + + @patch("conductor.providers.aca.AZURE_IDENTITY_AVAILABLE", False) + @patch("conductor.providers.aca.install_command", return_value="RESOLVED-COMMAND") + def test_provider_constructor_suggestion_is_resolved(self, _resolver) -> None: + """The provider guards availability a second time (it is constructible + directly, not only through the factory), so both sites have to + resolve the command rather than one of them drifting.""" + from conductor.providers.aca import AcaRuntimeProvider + + settings = ProviderSettings(name="aca", pool_endpoint="https://pool.example.com") + with pytest.raises(ProviderError) as exc_info: + AcaRuntimeProvider(provider_settings=settings) + assert exc_info.value.suggestion == "Install with: RESOLVED-COMMAND" + @patch("conductor.providers.factory.AZURE_IDENTITY_AVAILABLE", True) @pytest.mark.asyncio async def test_factory_raises_when_provider_settings_missing(self) -> None: diff --git a/tests/test_providers/test_claude_agent_sdk.py b/tests/test_providers/test_claude_agent_sdk.py index 003bb27d..0058047a 100644 --- a/tests/test_providers/test_claude_agent_sdk.py +++ b/tests/test_providers/test_claude_agent_sdk.py @@ -76,6 +76,19 @@ def test_init_raises_when_sdk_not_installed(self) -> None: with pytest.raises(ProviderError, match="Claude Agent SDK not installed"): ClaudeAgentSdkProvider() + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", False) + @patch("conductor.providers.claude_agent_sdk.install_command", return_value="RESOLVED-COMMAND") + def test_the_install_suggestion_is_resolved_not_hardcoded(self, resolver) -> None: + """Issue #441: this used to say `uv add 'claude-agent-sdk>=0.2.82'`, + which fails outside a uv project and cannot install into the uv tool + venv the install script creates. `claude-agent-sdk` is one of this + package's declared extras, so the resolver applies.""" + with pytest.raises(ProviderError) as exc_info: + ClaudeAgentSdkProvider() + + assert exc_info.value.suggestion == "Install with: RESOLVED-COMMAND" + resolver.assert_called_once_with("claude-agent-sdk") + @patch("conductor.providers.claude_agent_sdk.CLAUDE_AGENT_SDK_AVAILABLE", True) @patch("conductor.providers.claude_agent_sdk.query", lambda **kwargs: None) @patch("conductor.providers.claude_agent_sdk.ClaudeAgentOptions", Mock)