fix: give fdp chat/query the FDP environment again - #19
Merged
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4
_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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
fdp chatandfdp queryexecvpeintopython -m toksearch.llm.cli, and the LLM agent in that session fetches shot data — so it needs the FDP environment.It hasn't had one.
fe72baamarked both subcommandsneeds_env=Falsebecausesetup_environment()raises when no device contributor is installed, which broke evenfdp chat --helpinside fdp's own dev env. That fixed the crash, but it also meant the preferred conversational entry point ran with noPTDATA_LOC, nodefault_tree_pathand noBEARER_TOKEN— the agent could not reach any shot data at all.This surfaced while correcting the TokSearch docs, which asserted the environment was configured. Rather than document the broken behaviour, fix it.
What
A third
needs_envstate,BEST_EFFORT.chat/queryattempt setup and warn-and-continue only when no device contributor is installed at all — an LLM session that never fetches shot data is still useful. Everything else stays hard.Setting the environment up here is safe precisely because these subcommands
execvpeinto a fresh process, so libfdpio and XRootD read the variables at load time — the same shape asfdp run, the known-good path. The hazard to avoid issetup_environment()followed by MDSplus use in the same process; that is not what happens here.Degradation is gated on type, not on the
exceptclause. A first cut caught(ValueError, KeyError), which also swallowed a mistyped--deviceand a genuineDeviceEnvConflict— a session would warn and then silently fail to fetch anything.NoDevicesError(ValueError)is now raised at the single_registered_namessite that means "nothing installed", and only that degrades:--deviceworks after the subcommand._add_device_argwas never called forp_chat/p_query, sofdp chat --device d3dfailed with "unrecognized arguments" — harmless when those subcommands ignored devices, a papercut now that the flag is load-bearing for them.Cleanup in code being touched: dropped the vestigial
handleparameter that_build_llm_cmddocumented as unused, and the resolver that existed only to produce it. Rewrote__main__.py's docstring, which still described the graphvizbin/fdpcollision as unfixed and pointed at a design doc that does not exist in this repo —recipe/recipe.yamlhas declared graphviz a run dependency since 0.6.0, with a guard test.Verification
NoDevicesErrorto a plainValueError— reintroducing exactly the bug this PR fixes — must fail the suite. It does: 4 failed, 253 passed. An earlier revision of these tests could not tell the fixed code from the broken code, because the degradation test injected the exception into a mockedsetup_environment; it now runs the real path against an empty catalog.--devicespellings and the unknown-device exit code exercised against the real CLI.Coordination
Lands with
GA-FDP/toksearch#docs/refresh-2026-09andGA-FDP/toksearch_cmf#docs/readme-refresh, which document this behaviour. The docs describe the fixed behaviour, so this needs to ship — merge, tag, and bless throughfdp-core— for those docs to be true.Known, not addressed
tests/test_cli.pydoesn't neutralize$FDP_DEFAULT_DEVICEorPath.homethe waytest_composition.py'sCatalogFixturedoes; 7 of its existing tests fail if a developer exports that variable. Pre-existing.NoDevicesError/DeviceEnvConflictaren't re-exported fromfdp/__init__.py, so downstream callers wanting the same distinction must reach intofdp.devices. This PR is the first to make the distinction load-bearing.🤖 Generated with Claude Code
https://claude.ai/code/session_014c9v9tjK6XWLKxNrvXuzx4