From 732eacc1967001cf9047fb21dca3ae6a6af84f47 Mon Sep 17 00:00:00 2001 From: sammuli Date: Fri, 4 Sep 2026 13:07:42 -0700 Subject: [PATCH 1/5] fix(cli): give fdp chat/query the FDP environment again fe72baa marked chat/query needs_env=False so they would run inside fdp's own dev env, where no device contributor is installed and setup_environment raises. That fixed the crash but also meant the preferred conversational entry point ran with no PTDATA_LOC, no default_tree_path and no BEARER_TOKEN -- the agent could not reach any shot data. Add a third needs_env state, "best-effort": attempt setup, and warn and continue on the failures that mean no device is available. Strict subcommands are untouched and still exit 1. Setting the environment up here is safe because chat/query execvpe into a fresh python -m toksearch.llm.cli, so libfdpio and XRootD read the variables at load time in the new process -- the same shape as fdp run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4 --- fdp/cli.py | 38 ++++++++++++++------ tests/test_cli.py | 90 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 11 deletions(-) diff --git a/fdp/cli.py b/fdp/cli.py index f0adacb..3647236 100644 --- a/fdp/cli.py +++ b/fdp/cli.py @@ -28,6 +28,7 @@ compose_device_config, resolve_bearer_token, setup_environment, ) from .filesystem import FdpFileSystem +from . import llm_shims from .llm_shims import do_backends as _llm_do_backends from .llm_shims import do_chat as _llm_do_chat from .llm_shims import do_query as _llm_do_query @@ -390,16 +391,20 @@ def build_parser() -> argparse.ArgumentParser: action="store_false", default=True, help="When --gui is set, do not open a " "browser tab.") - # chat / query just execvpe into toksearch.llm.cli; no FDP env - # setup needed, and they tolerate no device contributor being - # installed (useful for working inside the fdp dev env). - p_chat.set_defaults(func=do_chat, needs_env=False) + # chat / query want the FDP environment -- the agent fetches shot data -- + # but must still run where no device contributor is installed (fdp's own + # dev env). "best-effort" is that middle state: try, warn, continue. + # Setting it up here is safe precisely because these subcommands execvpe + # into a fresh process, so libfdpio and XRootD read the vars at load time. + p_chat.set_defaults(func=do_chat, needs_env="best-effort", + auto_login=True) p_query = sub.add_parser("query", help="One-shot query") p_query.add_argument("query", type=str, help="Natural-language query (quote it)") _add_llm_args(p_query) - p_query.set_defaults(func=do_query, needs_env=False) + p_query.set_defaults(func=do_query, needs_env="best-effort", + auto_login=True) p_be = sub.add_parser( "backends", @@ -416,8 +421,12 @@ def main(argv=None) -> None: # Pure-metadata subcommands (devices, skills, backends) don't touch # the FDP env and shouldn't require a device contributor to be - # installed, so they opt out via `needs_env=False`. - if getattr(args, "needs_env", True): + # installed, so they opt out via `needs_env=False`. chat/query use + # `needs_env="best-effort"`: they want the env when it is available + # but must not die when it isn't. + needs_env = getattr(args, "needs_env", True) + if needs_env: + best_effort = needs_env == "best-effort" # Device resolution can fail (e.g. no default chosen among several # registered tokamaks); present it as a clean message, not a traceback. try: @@ -427,11 +436,18 @@ def main(argv=None) -> None: auto_login=getattr(args, "auto_login", False), ) except (ValueError, KeyError) as exc: - print(f"Error: {exc}", file=sys.stderr) - sys.exit(1) + if not best_effort: + print(f"Error: {exc}", file=sys.stderr) + sys.exit(1) + print(f"Warning: continuing without the FDP environment ({exc}). " + f"Data access will not work in this session.", + file=sys.stderr) except auth.AuthError as exc: - print(f"Login failed: {exc}", file=sys.stderr) - sys.exit(1) + if not best_effort: + print(f"Login failed: {exc}", file=sys.stderr) + sys.exit(1) + print(f"Warning: continuing without a bearer token ({exc}).", + file=sys.stderr) try: args.func(args) diff --git a/tests/test_cli.py b/tests/test_cli.py index 50c36c9..cc8575a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -451,5 +451,95 @@ def test_deprecated_alias_still_works(self): self.assertEqual(args.device, "d3d") +class TestChatQueryEnvironment(unittest.TestCase): + """chat/query set up the FDP environment when a device is available, + and degrade to a warning when none is (fdp's own dev env).""" + + def test_chat_sets_up_environment(self): + from fdp import cli + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "chat"])) + setup_mock = stack.enter_context( + mock.patch.object(cli, "setup_environment")) + ev = stack.enter_context( + mock.patch.object(cli.llm_shims.os, "execvpe")) + with redirect_stdout(io.StringIO()): + cli.main() + setup_mock.assert_called_once() + self.assertEqual(setup_mock.call_args.kwargs.get("auto_login"), True) + ev.assert_called_once() + + def test_query_sets_up_environment(self): + from fdp import cli + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "query", "hello"])) + setup_mock = stack.enter_context( + mock.patch.object(cli, "setup_environment")) + ev = stack.enter_context( + mock.patch.object(cli.llm_shims.os, "execvpe")) + with redirect_stdout(io.StringIO()): + cli.main() + setup_mock.assert_called_once() + ev.assert_called_once() + + def test_chat_survives_missing_device(self): + """The fe72baa case: no contributor installed. Warn, then exec.""" + from fdp import cli + buf = io.StringIO() + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "chat"])) + stack.enter_context(mock.patch.object( + cli, "setup_environment", + side_effect=ValueError("no device contributors installed"))) + ev = stack.enter_context( + mock.patch.object(cli.llm_shims.os, "execvpe")) + stack.enter_context(mock.patch.object(sys, "stderr", buf)) + with redirect_stdout(io.StringIO()): + cli.main() + ev.assert_called_once() + self.assertIn("no device contributors installed", buf.getvalue()) + + def test_chat_survives_auth_error(self): + from fdp import cli, auth + buf = io.StringIO() + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "chat"])) + stack.enter_context(mock.patch.object( + cli, "setup_environment", + side_effect=auth.AuthError("token acquisition failed"))) + ev = stack.enter_context( + mock.patch.object(cli.llm_shims.os, "execvpe")) + stack.enter_context(mock.patch.object(sys, "stderr", buf)) + with redirect_stdout(io.StringIO()): + cli.main() + ev.assert_called_once() + self.assertIn("token acquisition failed", buf.getvalue()) + + def test_strict_subcommand_still_exits_on_missing_device(self): + """Regression guard: the new branch must not soften `fdp ls`.""" + from fdp import cli + buf = io.StringIO() + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "ls", "/"])) + stack.enter_context(mock.patch.object( + cli, "setup_environment", + side_effect=ValueError("no device contributors installed"))) + stack.enter_context(mock.patch.object(sys, "stderr", buf)) + with redirect_stdout(io.StringIO()): + with self.assertRaises(SystemExit) as cm: + cli.main() + self.assertEqual(cm.exception.code, 1) + + if __name__ == "__main__": unittest.main() From 6ab2b66f424d43e003995e368c32e002a2101e85 Mon Sep 17 00:00:00 2001 From: sammuli Date: Fri, 4 Sep 2026 13:20:40 -0700 Subject: [PATCH 2/5] fix(cli): degrade only when no device is installed at all The best-effort branch caught (ValueError, KeyError) wholesale, so every ValueError from setup_environment turned into a warning for chat/query. Two of those should stay hard: * `fdp --device nosuch chat` -- devices._handle raises "Unknown device 'nosuch'". A typo in --device is exactly where failing loudly beats continuing: warn-and-continue handed the user an interactive chat whose agent silently could not fetch any data. * DeviceEnvConflict, which subclasses ValueError so cli.main renders it as a message rather than a traceback -- and thereby also inherited the degradation. Narrow the condition rather than the handler: devices.NoDevicesError (a ValueError subclass, so `except ValueError` callers are unaffected) now marks the one situation a chat can reasonably continue past, and the best-effort branch tests for that type. auth.AuthError is untouched. Also, from the same review: * Drop `from . import llm_shims`. Nothing in cli.py used it; it existed so tests could write `mock.patch.object(cli.llm_shims.os, ...)`, which reads as if scoped to the shim module but patches the one `os` module either way. Those five tests now patch `cli.os`, like their neighbours already did. * Name the magic string as BEST_EFFORT. `if needs_env:` treats any truthy value as strict, so a typo'd "best_effort" would have been silently strict; a name makes it a NameError. * Say the exclusivity in the code (`else:`) instead of leaning on sys.exit to make the following print unreachable. * Reword both warnings: no nested parenthetical, and the AuthError one now names the remedy (`fdp login`) its sibling already had. * Assert what the strict-path guard is actually for (an Error, and no Warning), and assert auto_login on the query test as its chat twin does. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4 --- fdp/cli.py | 38 ++++++++++++++++++++-------- fdp/devices.py | 15 +++++++++-- tests/test_cli.py | 64 +++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 99 insertions(+), 18 deletions(-) diff --git a/fdp/cli.py b/fdp/cli.py index 3647236..27f5989 100644 --- a/fdp/cli.py +++ b/fdp/cli.py @@ -23,17 +23,24 @@ from . import auth, config from .catalog import catalog -from .devices import CAPABILITIES, active_handles, resolve_for_capability +from .devices import ( + CAPABILITIES, NoDevicesError, active_handles, resolve_for_capability, +) from .environment import ( compose_device_config, resolve_bearer_token, setup_environment, ) from .filesystem import FdpFileSystem -from . import llm_shims from .llm_shims import do_backends as _llm_do_backends from .llm_shims import do_chat as _llm_do_chat from .llm_shims import do_query as _llm_do_query from .skills import BACKENDS, _parse_skill_md, discover_skill_dirs +# The one non-boolean `needs_env` state: attempt setup, but warn and continue +# when no device contributor is installed. Named so a typo is a NameError +# rather than a silent fall-back to strict behaviour (`if needs_env:` treats +# any truthy value as strict). +BEST_EFFORT = "best-effort" # needs_env: True | False | BEST_EFFORT + # ---------------------------------------------------------------------- # Subcommand handlers @@ -396,14 +403,14 @@ def build_parser() -> argparse.ArgumentParser: # dev env). "best-effort" is that middle state: try, warn, continue. # Setting it up here is safe precisely because these subcommands execvpe # into a fresh process, so libfdpio and XRootD read the vars at load time. - p_chat.set_defaults(func=do_chat, needs_env="best-effort", + p_chat.set_defaults(func=do_chat, needs_env=BEST_EFFORT, auto_login=True) p_query = sub.add_parser("query", help="One-shot query") p_query.add_argument("query", type=str, help="Natural-language query (quote it)") _add_llm_args(p_query) - p_query.set_defaults(func=do_query, needs_env="best-effort", + p_query.set_defaults(func=do_query, needs_env=BEST_EFFORT, auto_login=True) p_be = sub.add_parser( @@ -426,7 +433,7 @@ def main(argv=None) -> None: # but must not die when it isn't. needs_env = getattr(args, "needs_env", True) if needs_env: - best_effort = needs_env == "best-effort" + best_effort = needs_env == BEST_EFFORT # Device resolution can fail (e.g. no default chosen among several # registered tokamaks); present it as a clean message, not a traceback. try: @@ -436,18 +443,27 @@ def main(argv=None) -> None: auto_login=getattr(args, "auto_login", False), ) except (ValueError, KeyError) as exc: - if not best_effort: + # Only "nothing is installed here" is worth continuing past: it + # is the fdp-dev-env case, and the user asked for a chat, not for + # data. A mistyped --device or a real DeviceEnvConflict (also a + # ValueError) still exits -- degrading there would hand the user + # an agent that silently cannot fetch anything. + no_device_here = best_effort and isinstance(exc, NoDevicesError) + if not no_device_here: print(f"Error: {exc}", file=sys.stderr) sys.exit(1) - print(f"Warning: continuing without the FDP environment ({exc}). " - f"Data access will not work in this session.", - file=sys.stderr) + else: + print("Warning: continuing without the FDP environment. " + f"Data access will not work in this session. ({exc})", + file=sys.stderr) except auth.AuthError as exc: if not best_effort: print(f"Login failed: {exc}", file=sys.stderr) sys.exit(1) - print(f"Warning: continuing without a bearer token ({exc}).", - file=sys.stderr) + else: + print("Warning: continuing without a bearer token. Data " + "access will not work in this session; run " + f"`fdp login` to fix it. ({exc})", file=sys.stderr) try: args.func(args) diff --git a/fdp/devices.py b/fdp/devices.py index 00eb432..80a9c39 100644 --- a/fdp/devices.py +++ b/fdp/devices.py @@ -49,6 +49,16 @@ ) +class NoDevicesError(ValueError): + """No tokamak contributor packages are installed. + + A ValueError subclass so existing `except ValueError` handlers keep + working; a distinct type so callers that can reasonably continue + without a device -- `fdp chat`/`fdp query` -- can tell this apart + from a mistyped --device or a genuine env conflict. + """ + + def _listed(names) -> str: """Render device/capability names for a message: ``d3d, devb, mast``. Interpolating the list itself would leak Python repr punctuation.""" @@ -71,7 +81,8 @@ def explicit_device_name(device: str | None = None) -> str | None: def _registered_names() -> list: - """All registered device names, sorted. Raises if none are installed. + """All registered device names, sorted. Raises ``NoDevicesError`` if + none are installed. Every entry point into this module funnels through here first, so "nothing is installed" always beats "I don't recognize that name" -- @@ -79,7 +90,7 @@ def _registered_names() -> list: """ names = _catalog.names() if not names: - raise ValueError(_NO_DEVICES) + raise NoDevicesError(_NO_DEVICES) return names diff --git a/tests/test_cli.py b/tests/test_cli.py index cc8575a..bb22303 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -464,7 +464,7 @@ def test_chat_sets_up_environment(self): setup_mock = stack.enter_context( mock.patch.object(cli, "setup_environment")) ev = stack.enter_context( - mock.patch.object(cli.llm_shims.os, "execvpe")) + mock.patch.object(cli.os, "execvpe")) with redirect_stdout(io.StringIO()): cli.main() setup_mock.assert_called_once() @@ -480,15 +480,17 @@ def test_query_sets_up_environment(self): setup_mock = stack.enter_context( mock.patch.object(cli, "setup_environment")) ev = stack.enter_context( - mock.patch.object(cli.llm_shims.os, "execvpe")) + mock.patch.object(cli.os, "execvpe")) with redirect_stdout(io.StringIO()): cli.main() setup_mock.assert_called_once() + self.assertEqual(setup_mock.call_args.kwargs.get("auto_login"), True) ev.assert_called_once() def test_chat_survives_missing_device(self): """The fe72baa case: no contributor installed. Warn, then exec.""" from fdp import cli + from fdp.devices import NoDevicesError buf = io.StringIO() with ExitStack() as stack: _patch_catalog(stack) @@ -496,9 +498,10 @@ def test_chat_survives_missing_device(self): sys, "argv", ["fdp", "chat"])) stack.enter_context(mock.patch.object( cli, "setup_environment", - side_effect=ValueError("no device contributors installed"))) + side_effect=NoDevicesError( + "no device contributors installed"))) ev = stack.enter_context( - mock.patch.object(cli.llm_shims.os, "execvpe")) + mock.patch.object(cli.os, "execvpe")) stack.enter_context(mock.patch.object(sys, "stderr", buf)) with redirect_stdout(io.StringIO()): cli.main() @@ -516,7 +519,7 @@ def test_chat_survives_auth_error(self): cli, "setup_environment", side_effect=auth.AuthError("token acquisition failed"))) ev = stack.enter_context( - mock.patch.object(cli.llm_shims.os, "execvpe")) + mock.patch.object(cli.os, "execvpe")) stack.enter_context(mock.patch.object(sys, "stderr", buf)) with redirect_stdout(io.StringIO()): cli.main() @@ -539,6 +542,57 @@ def test_strict_subcommand_still_exits_on_missing_device(self): with self.assertRaises(SystemExit) as cm: cli.main() self.assertEqual(cm.exception.code, 1) + self.assertIn("Error:", buf.getvalue()) + self.assertNotIn("Warning", buf.getvalue()) + + def test_chat_exits_on_unknown_device(self): + """A mistyped --device is not a "no device available here" failure: + it is a typo, and the useful answer is a clean error rather than a + chat session that silently cannot fetch anything. Runs the real + setup_environment so the raise site in devices.py is what is under + test. (`chat` does not declare its own --device, so the flag goes + before the subcommand.)""" + from fdp import cli + buf = io.StringIO() + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "--device", "nosuch", "chat"])) + ev = stack.enter_context( + mock.patch.object(cli.os, "execvpe")) + stack.enter_context(mock.patch.object(sys, "stderr", buf)) + with redirect_stdout(io.StringIO()): + with self.assertRaises(SystemExit) as cm: + cli.main() + self.assertEqual(cm.exception.code, 1) + self.assertIn("Unknown device", buf.getvalue()) + self.assertNotIn("Warning", buf.getvalue()) + ev.assert_not_called() + + def test_chat_exits_on_device_env_conflict(self): + """DeviceEnvConflict subclasses ValueError but is a genuine + multi-device disagreement, not an absent device -- it must stay + hard for chat too.""" + from fdp import cli + from fdp.environment import DeviceEnvConflict + buf = io.StringIO() + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "chat"])) + stack.enter_context(mock.patch.object( + cli, "setup_environment", + side_effect=DeviceEnvConflict("devices disagree on FOO"))) + ev = stack.enter_context( + mock.patch.object(cli.os, "execvpe")) + stack.enter_context(mock.patch.object(sys, "stderr", buf)) + with redirect_stdout(io.StringIO()): + with self.assertRaises(SystemExit) as cm: + cli.main() + self.assertEqual(cm.exception.code, 1) + self.assertIn("devices disagree on FOO", buf.getvalue()) + self.assertNotIn("Warning", buf.getvalue()) + ev.assert_not_called() if __name__ == "__main__": From 081ea9a758e529275bc4c71ca7d0ffbc5cef493f Mon Sep 17 00:00:00 2001 From: sammuli Date: Fri, 4 Sep 2026 13:36:20 -0700 Subject: [PATCH 3/5] test(cli): pin NoDevicesError at its raise site, not at a mock Mutation says the previous commit's central behaviour was uncovered. Changing devices.py back to `raise ValueError(_NO_DEVICES)` -- exactly the bug that commit exists to fix, `fdp chat` exiting 1 in a bare dev env instead of warning -- left the suite reporting 253 passed. Two blind spots, both from the same cause: NoDevicesError's entire purpose is a *type* distinction, and nothing asserted the type. * test_chat_survives_missing_device injected NoDevicesError as a side_effect on a mocked setup_environment, so it proved the CLI handles the type it was handed and nothing about what devices.py actually raises. It now patches an empty catalog and runs the real setup_environment, so the raise site is what is under test. This stays hermetic because active_handles() raises out of _registered_names() before explicit_device_name() consults $FDP_DEFAULT_DEVICE or ~/.fdp/config.toml, and before apply_environment() writes to os.environ -- verified by a tripwire on read_default_device (never called), and the second half is now asserted in the test itself. * TestNoDevicesInstalled asserted ValueError plus a message substring, which a plain ValueError satisfies. Now NoDevicesError, which is strictly stronger since it subclasses ValueError. With the mutant in place the suite goes 4 failed, 250 passed; restored, 254 passed. Also make the conflict half real. test_chat_exits_on_device_env_conflict injects DeviceEnvConflict, which pins the CLI's dispatch but not the path that produces one, so the end-to-end version joins the two conflicting ptdata catalogs it needs, in test_composition.py: `fdp chat` over a genuine PTDATA_JSON_INDEX_DIR collision must exit 1 rather than warn. It lives there rather than in test_cli.py because two registered devices do reach ~/.fdp/config.toml, and CatalogFixture already supplies the temp $HOME that makes that safe. A second mutation (dropping the isinstance check from the best-effort condition) fails it along with both test_cli.py guards, so all three directions of the dispatch now bite. _patch_catalog's entry-point/cache dance moves to _patch_entry_points so the empty-catalog helper shares it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4 --- tests/test_capabilities.py | 20 +++++++++----- tests/test_cli.py | 56 ++++++++++++++++++++++++++++---------- tests/test_composition.py | 22 +++++++++++++++ 3 files changed, 77 insertions(+), 21 deletions(-) diff --git a/tests/test_capabilities.py b/tests/test_capabilities.py index 0635f0d..8a64b62 100644 --- a/tests/test_capabilities.py +++ b/tests/test_capabilities.py @@ -455,27 +455,33 @@ def test_empty_env_var_is_treated_as_unset(self): class TestNoDevicesInstalled(CatalogFixture): - """A bare fdp env with no contributor packages.""" + """A bare fdp env with no contributor packages. + + These assert the *type*, not just the message: `fdp chat`/`fdp query` + degrade to a warning only for NoDevicesError, so a plain ValueError here + would silently turn that degradation back into a hard exit. The message + check alone cannot see the difference. + """ YAMLS = () def test_active_handles_says_install_a_device_package(self): - from fdp.devices import active_handles - with self.assertRaises(ValueError) as ctx: + from fdp.devices import NoDevicesError, active_handles + with self.assertRaises(NoDevicesError) as ctx: active_handles() self.assertIn("No tokamak contributors", str(ctx.exception)) def test_explicit_name_still_says_install_a_device_package(self): # Not "unknown device 'd3d'" — nothing is installed at all, and that # is the actionable fact. - from fdp.devices import _resolve_device_handle - with self.assertRaises(ValueError) as ctx: + from fdp.devices import NoDevicesError, _resolve_device_handle + with self.assertRaises(NoDevicesError) as ctx: _resolve_device_handle("d3d") self.assertIn("No tokamak contributors", str(ctx.exception)) def test_capability_resolution_says_install_a_device_package(self): - from fdp.devices import resolve_for_capability - with self.assertRaises(ValueError) as ctx: + from fdp.devices import NoDevicesError, resolve_for_capability + with self.assertRaises(NoDevicesError) as ctx: resolve_for_capability("origin", "d3d") self.assertIn("No tokamak contributors", str(ctx.exception)) diff --git a/tests/test_cli.py b/tests/test_cli.py index bb22303..4908fb6 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -90,17 +90,27 @@ def _make_catalog_ep(name: str, yaml_text: str): return ep -def _patch_catalog(stack, yaml_text: str = _D3D_TEST_YAML, - name: str = "d3d"): - """Patch the catalog entry points and reset the cache.""" +def _patch_entry_points(stack, eps): + """Patch the catalog entry points to *eps* and reset the cache.""" from fdp.catalog import catalog as _cat - ep = _make_catalog_ep(name, yaml_text) stack.enter_context(mock.patch("fdp.catalog.entry_points", - return_value=[ep])) + return_value=eps)) _cat._cache = None stack.callback(lambda: setattr(_cat, "_cache", None)) +def _patch_catalog(stack, yaml_text: str = _D3D_TEST_YAML, + name: str = "d3d"): + """Patch the catalog to a single fake device.""" + _patch_entry_points(stack, [_make_catalog_ep(name, yaml_text)]) + + +def _patch_empty_catalog(stack): + """Patch the catalog to no registered devices at all -- fdp's own dev + env, where no contributor package is installed.""" + _patch_entry_points(stack, []) + + def _run_cli(argv, yaml_text: str = _D3D_TEST_YAML, name: str = "d3d"): """Invoke fdp.cli.main with mocks; return (stdout, exit_code).""" from fdp import cli @@ -488,25 +498,36 @@ def test_query_sets_up_environment(self): ev.assert_called_once() def test_chat_survives_missing_device(self): - """The fe72baa case: no contributor installed. Warn, then exec.""" + """The fe72baa case: no contributor installed. Warn, then exec. + + Runs the *real* setup_environment against an empty catalog, so the + NoDevicesError raise site in devices.py is what is under test. Faking + it with a side_effect on a mocked setup_environment would pin nothing + about the type actually raised there -- and the type is the whole + point, since the message is identical either way. + + Hermetic despite touching the real code path: active_handles() raises + out of _registered_names() before explicit_device_name() consults + $FDP_DEFAULT_DEVICE or ~/.fdp/config.toml, and before + apply_environment() writes anything. The os.environ assertion below + holds that second half in place. + """ from fdp import cli - from fdp.devices import NoDevicesError buf = io.StringIO() + before = dict(os.environ) with ExitStack() as stack: - _patch_catalog(stack) + _patch_empty_catalog(stack) stack.enter_context(mock.patch.object( sys, "argv", ["fdp", "chat"])) - stack.enter_context(mock.patch.object( - cli, "setup_environment", - side_effect=NoDevicesError( - "no device contributors installed"))) ev = stack.enter_context( mock.patch.object(cli.os, "execvpe")) stack.enter_context(mock.patch.object(sys, "stderr", buf)) with redirect_stdout(io.StringIO()): cli.main() ev.assert_called_once() - self.assertIn("no device contributors installed", buf.getvalue()) + self.assertIn("Warning", buf.getvalue()) + self.assertIn("No tokamak contributors", buf.getvalue()) + self.assertEqual(dict(os.environ), before) def test_chat_survives_auth_error(self): from fdp import cli, auth @@ -572,7 +593,14 @@ def test_chat_exits_on_unknown_device(self): def test_chat_exits_on_device_env_conflict(self): """DeviceEnvConflict subclasses ValueError but is a genuine multi-device disagreement, not an absent device -- it must stay - hard for chat too.""" + hard for chat too. + + This injects the exception, so it pins the CLI's type dispatch only. + The end-to-end version -- two really-conflicting devices composed by + the real setup_environment -- lives in test_composition.py, next to + the fixtures that produce the conflict (it needs that module's temp + $HOME, since with two devices resolution does reach + ~/.fdp/config.toml).""" from fdp import cli from fdp.environment import DeviceEnvConflict buf = io.StringIO() diff --git a/tests/test_composition.py b/tests/test_composition.py index ea7aace..4402614 100644 --- a/tests/test_composition.py +++ b/tests/test_composition.py @@ -163,6 +163,28 @@ def test_cli_renders_conflict_without_traceback(self): self.assertEqual(ctx.exception.code, 1) self.assertIn("PTDATA_JSON_INDEX_DIR", stderr.getvalue()) + def test_chat_exits_on_conflict_rather_than_warning(self): + """`fdp chat` sets up the environment best-effort: it warns and + continues when no device is installed at all. A real conflict between + two installed devices is not that case -- it is a choice the user has + to make -- so chat must still exit 1. Driven through the real + setup_environment rather than an injected exception, so the whole + path from two conflicting catalogs to the CLI's decision is covered. + """ + import contextlib + import io + from unittest import mock + from fdp import cli + stderr = io.StringIO() + with mock.patch.object(cli.os, "execvpe") as ev: + with self.assertRaises(SystemExit) as ctx, \ + contextlib.redirect_stderr(stderr): + cli.main(["chat"]) + self.assertEqual(ctx.exception.code, 1) + self.assertIn("PTDATA_JSON_INDEX_DIR", stderr.getvalue()) + self.assertNotIn("Warning", stderr.getvalue()) + ev.assert_not_called() + class TestConflictAttribution(CatalogFixture): """Three devices, two of which agree: the message must blame the device From 507c1677b3821f2d46339a8e29450104b80bc5d9 Mon Sep 17 00:00:00 2001 From: sammuli Date: Fri, 4 Sep 2026 13:54:53 -0700 Subject: [PATCH 4/5] fix(cli): let chat/query take --device after the subcommand _add_device_arg is called for run, env, login, logout, ls and the top-level parser, but was never called for chat or query. That was harmless while those two ignored devices entirely; the previous three commits made them compose the device environment and made an unknown --device a hard exit, so the flag is now load-bearing for them -- yet `fdp chat --device d3d` died with "unrecognized arguments" and only `fdp --device d3d chat` worked. README teaches the post-subcommand spelling (`fdp ls -D d3d`) for every other subcommand. Declare it on both. _add_device_arg's SUPPRESS default already exists so the flag works on either side, so nothing else changes. Ordering: `run` has to declare it before its REMAINDER positional, but `query`'s positional is a plain one, which cannot swallow a following optional -- verified in both orders -- so the placement there is cosmetic, matching `run` for consistency. Three tests, all in the four spellings a user can type: the flag after `chat`, after `query` (also asserting the query string still reaches the exec'd argv, and that --device does not leak into it), and the top-level form that worked before, as a regression guard. Dropping either _add_device_arg call fails the first two. test_chat_exits_on_unknown_device loses its now-false parenthetical about chat not declaring the flag. Also correct fdp/__main__.py's docstring, which still described the graphviz `bin/fdp` collision as unfixed ("graphviz wins", "fdp run silently invokes a graph layout tool") and pointed at docs/2026-09-02-fdp-cli-rename.md for a planned rename. That file does not exist in this repository, and recipe/recipe.yaml has declared graphviz a run dependency since 0.6.0 precisely so link order gives us the file back, with a packaged test guarding it and a rename framed as the fallback "if it ever fails". Docstring only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4 --- fdp/__main__.py | 23 +++++++++-------- fdp/cli.py | 5 ++++ tests/test_cli.py | 65 +++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 80 insertions(+), 13 deletions(-) diff --git a/fdp/__main__.py b/fdp/__main__.py index 8883f57..639594a 100644 --- a/fdp/__main__.py +++ b/fdp/__main__.py @@ -13,19 +13,20 @@ # limitations under the License. """Make the FDP CLI reachable as ``python -m fdp``. -The ``fdp`` console script is not always reachable. Graphviz installs its -force-directed layout engine at the same path -- ``bin/fdp``, one of the eight -engines it ships -- and conda has no conflict detection for that: ``conda-meta`` -records graphviz as the owner and graphviz wins. Any environment containing -``cmflib`` pulls graphviz transitively (cmflib -> dvc -> pydot -> graphviz), so -in those environments ``fdp run`` silently invokes a graph layout tool. +Graphviz ships one of its layout engines at ``bin/fdp``, the same path as our +console script, and conda has no conflict detection for that: whichever +package links last owns the file. Anything pulling graphviz in transitively +(cmflib -> dvc -> pydot -> graphviz, i.e. the whole CMF provenance stack) +could therefore replace the FDP CLI with a graph layout tool. -``python -m fdp`` resolves through the installed package rather than ``PATH``, -so it works regardless. Prefer it in scripts and documentation that must run in -environments carrying the provenance stack. +Since 0.6.0 the recipe declares graphviz as a *run dependency* precisely so +link order puts us last and bare ``fdp`` keeps working; a packaged test guards +it. Renaming the console script remains the fallback if that ever stops +holding, and so far it has not been needed. -See ``docs/2026-09-02-fdp-cli-rename.md`` in the workspace for the full -analysis; a rename of the console script is planned for a major release. +``python -m fdp`` resolves through the installed package rather than ``PATH``, +so it cannot be shadowed at all. Prefer it in scripts and documentation that +must run in environments carrying the provenance stack. """ import sys diff --git a/fdp/cli.py b/fdp/cli.py index 27f5989..3f17d74 100644 --- a/fdp/cli.py +++ b/fdp/cli.py @@ -390,6 +390,7 @@ def build_parser() -> argparse.ArgumentParser: p_chat = sub.add_parser("chat", help="Interactive conversational query") + _add_device_arg(p_chat) _add_llm_args(p_chat) p_chat.add_argument("--gui", action="store_true", help="Launch the local Gradio chat GUI " @@ -407,6 +408,10 @@ def build_parser() -> argparse.ArgumentParser: auto_login=True) p_query = sub.add_parser("query", help="One-shot query") + # Unlike `run`, the positional here is a plain one, not REMAINDER, so it + # does not swallow following optionals and the order is free; keep it + # matching `run` anyway. + _add_device_arg(p_query) p_query.add_argument("query", type=str, help="Natural-language query (quote it)") _add_llm_args(p_query) diff --git a/tests/test_cli.py b/tests/test_cli.py index 4908fb6..0b8b2eb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -571,8 +571,8 @@ def test_chat_exits_on_unknown_device(self): it is a typo, and the useful answer is a clean error rather than a chat session that silently cannot fetch anything. Runs the real setup_environment so the raise site in devices.py is what is under - test. (`chat` does not declare its own --device, so the flag goes - before the subcommand.)""" + test. Spelled with the flag before the subcommand; the sibling + test_chat_device_after_subcommand covers the other position.""" from fdp import cli buf = io.StringIO() with ExitStack() as stack: @@ -590,6 +590,67 @@ def test_chat_exits_on_unknown_device(self): self.assertNotIn("Warning", buf.getvalue()) ev.assert_not_called() + def test_chat_device_after_subcommand(self): + """`fdp chat -D d3d` must work, not just `fdp -D d3d chat`. + + chat/query consume a device now, and an unknown one is a hard exit + (test_chat_exits_on_unknown_device), so the flag is load-bearing + here. README teaches the post-subcommand spelling for every other + subcommand; _add_device_arg's SUPPRESS default is what makes both + positions work.""" + from fdp import cli + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "chat", "--device", "d3d"])) + setup_mock = stack.enter_context( + mock.patch.object(cli, "setup_environment")) + ev = stack.enter_context( + mock.patch.object(cli.os, "execvpe")) + with redirect_stdout(io.StringIO()): + cli.main() + self.assertEqual(setup_mock.call_args.kwargs.get("device"), "d3d") + ev.assert_called_once() + + def test_chat_device_before_subcommand_still_works(self): + """Regression guard: the top-level spelling is the one that worked + before chat declared its own --device, so it must keep working.""" + from fdp import cli + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "--device", "d3d", "chat"])) + setup_mock = stack.enter_context( + mock.patch.object(cli, "setup_environment")) + ev = stack.enter_context( + mock.patch.object(cli.os, "execvpe")) + with redirect_stdout(io.StringIO()): + cli.main() + self.assertEqual(setup_mock.call_args.kwargs.get("device"), "d3d") + ev.assert_called_once() + + def test_query_device_after_subcommand_keeps_the_query(self): + """`fdp query -D d3d "hi"` resolves the device *and* still delivers + the query. `query`'s positional is a plain one rather than + REMAINDER, so a preceding optional cannot swallow it -- assert that + rather than trust it, since `run` needed a specific ordering.""" + from fdp import cli + with ExitStack() as stack: + _patch_catalog(stack) + stack.enter_context(mock.patch.object( + sys, "argv", ["fdp", "query", "--device", "d3d", "hi"])) + setup_mock = stack.enter_context( + mock.patch.object(cli, "setup_environment")) + ev = stack.enter_context( + mock.patch.object(cli.os, "execvpe")) + with redirect_stdout(io.StringIO()): + cli.main() + self.assertEqual(setup_mock.call_args.kwargs.get("device"), "d3d") + ev.assert_called_once() + argv = ev.call_args.args[1] + self.assertIn("hi", argv) + self.assertNotIn("--device", argv) + def test_chat_exits_on_device_env_conflict(self): """DeviceEnvConflict subclasses ValueError but is a genuine multi-device disagreement, not an absent device -- it must stay From 0fc00bafe2f7726e523cd95a34eaf22ccccd000f Mon Sep 17 00:00:00 2001 From: sammuli Date: Fri, 4 Sep 2026 13:55:40 -0700 Subject: [PATCH 5/5] docs: note that chat/query compose device environments too The device section listed `fdp env` / `fdp run` as the commands that compose all installed devices. Since this branch, `fdp chat` and `fdp query` do the same -- and now accept `--device` on either side of the subcommand -- so they belong in that bullet. Their one difference, warning rather than exiting when nothing is installed, is worth naming there because it is the whole reason the best-effort state exists. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4 --- README.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 01d8371..840b8ed 100644 --- a/README.md +++ b/README.md @@ -16,10 +16,12 @@ contributes `d3d`, `toksearch_mast` contributes `mast`. Most commands need no device selection: -- `fdp env` / `fdp run` compose the environments of **all** installed devices. - Their variables are disjoint, so the union is well-defined. If two devices - ever set the same variable to different values, `fdp` reports the conflicting - variable and asks you to choose. +- `fdp env` / `fdp run` / `fdp chat` / `fdp query` compose the environments of + **all** installed devices. Their variables are disjoint, so the union is + well-defined. If two devices ever set the same variable to different values, + `fdp` reports the conflicting variable and asks you to choose. `chat` and + `query` differ in one way: with no device installed at all they warn and + carry on, since an LLM session that never fetches shot data is still useful. - `fdp ls` uses the device that has an origin server. - `fdp login` / `fdp logout` use the device that requires a bearer token.