From 956d836bc7a7b4e3afa73d37c3c1f5fb44da9e6a Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 12 Nov 2026 13:23:56 +0100 Subject: [PATCH 1/3] fix(mission): resolve the mission LLM as an agent-loop run `fluid mission run` always drives the tool-use agent loop, but it resolved its LLM without saying so. `_agents_allowed` therefore stayed true, so a run without a TTY auto-picked a coding agent from PATH. Coding agents have no tool use, so the loop then failed mid-run with NotImplementedError after the agent had been invoked. Resolve with a copy of the args carrying `agent_loop=True`. No coding agent is chosen, the caller's args are untouched, and a run with nothing else configured stops with `copilot_no_llm_provider` before any agent runs. --- fluid_build/cli/mission.py | 8 +++++++- tests/test_llm_keyless_autopick.py | 12 ++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/fluid_build/cli/mission.py b/fluid_build/cli/mission.py index 03a5817a..a543520f 100644 --- a/fluid_build/cli/mission.py +++ b/fluid_build/cli/mission.py @@ -259,10 +259,16 @@ def _resolve_workspace_root(args: argparse.Namespace) -> Path: def _resolve_mission_llm_config(args: argparse.Namespace): """Resolve the LLM config, or ``(None, message)`` when unavailable.""" + import copy + from fluid_build.llm.providers import CopilotGenerationError, resolve_llm_config + # A mission always runs the tool-use agent loop, which no coding agent can + # serve: say so, so neither a saved nor an auto-picked agent is chosen. + loop_args = copy.copy(args) + loop_args.agent_loop = True try: - return resolve_llm_config(args, environ=None), None + return resolve_llm_config(loop_args, environ=None), None except CopilotGenerationError as exc: return None, str(exc) diff --git a/tests/test_llm_keyless_autopick.py b/tests/test_llm_keyless_autopick.py index ef9c3b26..1bbb00af 100644 --- a/tests/test_llm_keyless_autopick.py +++ b/tests/test_llm_keyless_autopick.py @@ -363,6 +363,18 @@ def test_the_loop_check_is_one_rule(self): assert agent_loop_requested(False, {"FLUID_COPILOT_AGENT_LOOP": "on"}) is True assert agent_loop_requested(False, {"FLUID_COPILOT_AGENT_LOOP": "0"}) is False + def test_a_mission_never_auto_picks_an_agent(self, world): + # ``fluid mission run`` always runs the loop, without the flag. + import argparse + + from fluid_build.cli.mission import _resolve_mission_llm_config + + world.install("claude-code") + args = argparse.Namespace(llm_provider=None, llm_model=None, llm_endpoint=None) + config, message = _resolve_mission_llm_config(args) + assert config is None + assert "copilot_no_llm_provider" in message # stopped before any agent ran + class TestAgentReadinessNeedsTheBinary: """``fluid doctor`` says "Ready" for a coding agent only when its CLI is on PATH. From b3cea1d46896ceae9a7944a167c5cdd883024141 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 12 Nov 2026 13:24:13 +0100 Subject: [PATCH 2/3] fix(ai): use the saved `fluid ai setup` choice in runs without a TTY Only the interactive inline setup applied the provider, model and key that `fluid ai setup` saves. `check_llm_readiness` (`fluid doctor`) read the saved choice too, but the real selection (`_provider_name_from`) never did. So non-interactive runs (CI, scripts, `fluid forge --agent` with an AI flag) ignored it, and doctor and forge disagreed about which provider would run. Selection now walks one ladder, and readiness calls it (`_provider_name_from` with no args) instead of keeping its own copy: 1. A named provider: `--llm-provider`, `FLUID_LLM_PROVIDER` or `FLUID_FORGE_AGENT`. 2. Exactly one key environment variable. 3. The saved `fluid ai setup` choice. 4. Exactly one keyring key. 5. A local Ollama, then (non-interactive, nothing configured) the first coding-agent CLI on PATH. - The saved model and Ollama host fill in what the run does not state, for the saved provider only. The environment's `OLLAMA_HOST` wins over the saved host. - A run that names its provider takes nothing from the saved choice. Without `--llm-model`, a named provider gets its catalog model, as before. Command Center's `fluid forge data-model from-intent --llm-provider

` call relies on that, so its provider and model are unchanged even where a saved config exists. - The saved `endpoint` field is not applied. It has had no writer since the multi-provider config (#363). Applied next to an environment key, it would send that key to a host it was not saved with. - A saved agent (coding-agent CLI or MCP sampling) is passed over wherever the auto-pick is. That covers a stated endpoint or model, given as a flag or as a `FLUID_LLM_*` variable, and the agent loop. A stated destination is never swapped for an agent. When nothing else selects a provider, `no_provider_message` names the saved agent. - Saved names are read through the provider registry, so a hand-written `claude` or `claude_code` means `anthropic` or `claude-code`. - Keys still resolve through `_resolve_api_key`. The saved choice is never written into `--llm-provider` or `FLUID_LLM_PROVIDER`. - The lean (no litellm) preflight follows the same order. A saved coding agent runs without litellm. - Three re-exports that only the old readiness ladder used are removed. tests/conftest.py gains an autouse fixture that hides the developer's saved choice (`ai_config.json` and the unified `config.yaml`) from every test, as `_hermetic_keyring` does for the OS keyring. Without it, the Command Center pin test failed on a machine with a saved gemini config. The precedence follows layered-config practice (flags, then environment, then the config file, then defaults), as in ConfigArgParse and pydantic-settings. --- .secrets.baseline | 14 +- fluid_build/llm/provider_resolve.py | 119 +++++++--- fluid_build/llm/providers.py | 135 +++++------ tests/conftest.py | 35 +++ tests/test_ai_setup.py | 28 +-- tests/test_litellm_absent.py | 27 ++- tests/test_llm_keyless_autopick.py | 356 +++++++++++++++++++++++++++- tests/test_llm_no_provider.py | 10 +- 8 files changed, 574 insertions(+), 150 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index dc6b206c..3a8bd8d2 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -1516,42 +1516,42 @@ "filename": "tests/test_ai_setup.py", "hashed_secret": "c54f9e7c993c2432fcb7b992d4a4663ce4eae83d", "is_verified": false, - "line_number": 306 + "line_number": 299 }, { "type": "Basic Auth Credentials", "filename": "tests/test_ai_setup.py", "hashed_secret": "e5e9fa1ba31ecd1ae84f75caaa474f3a663f05f4", "is_verified": false, - "line_number": 443 + "line_number": 436 }, { "type": "Secret Keyword", "filename": "tests/test_ai_setup.py", "hashed_secret": "63d185ab613a8f4ba8a3f2669f49e8df82bff53f", "is_verified": false, - "line_number": 476 + "line_number": 469 }, { "type": "Secret Keyword", "filename": "tests/test_ai_setup.py", "hashed_secret": "06c3796a342a1f1172aa7a907d3c0a5da02a053a", "is_verified": false, - "line_number": 1101 + "line_number": 1091 }, { "type": "Secret Keyword", "filename": "tests/test_ai_setup.py", "hashed_secret": "6b1196369d5cd92be55dbdd72ef619957b6e6624", "is_verified": false, - "line_number": 1105 + "line_number": 1095 }, { "type": "Secret Keyword", "filename": "tests/test_ai_setup.py", "hashed_secret": "9ea9d5acda505555f09a5d2203f7078f1a3b6363", "is_verified": false, - "line_number": 1110 + "line_number": 1100 } ], "tests/test_ai_setup_import_keys.py": [ @@ -2317,5 +2317,5 @@ } ] }, - "generated_at": "2026-11-12T07:03:08Z" + "generated_at": "2026-11-12T10:50:58Z" } diff --git a/fluid_build/llm/provider_resolve.py b/fluid_build/llm/provider_resolve.py index d7c43e65..a7ce42de 100644 --- a/fluid_build/llm/provider_resolve.py +++ b/fluid_build/llm/provider_resolve.py @@ -30,7 +30,7 @@ import logging import shutil import sys -from typing import Dict, List, Mapping, Optional, Tuple +from typing import Any, Dict, List, Mapping, Optional, Tuple LOG = logging.getLogger("fluid.cli.llm_providers.resolve") @@ -86,26 +86,77 @@ def _explicit_key_signals(env: Mapping[str, str]) -> Dict[str, List[str]]: return signals +def _canonical_provider_name(name: Any) -> Optional[str]: + """*name* as the provider registry spells it, ``None`` when it is not a name. + + A hand-written ``~/.fluid/config.yaml`` may say ``claude`` or ``claude_code``; + the registry answers ``anthropic`` and ``claude-code``. A name it does not + know (a plugin provider) comes back as written. + """ + if not isinstance(name, str) or not name.strip(): + return None + registry = getattr(_host(), "BUILTIN_LLM_PROVIDERS", {}) + key = name.strip().lower() + provider = registry.get(key) or registry.get(key.replace("_", "-")) + return provider.name if provider is not None else name.strip() + + def _saved_provider_name() -> Optional[str]: - """The provider ``fluid ai setup`` saved, if any (``~/.fluid``).""" + """The provider ``fluid ai setup`` saved, if any (``~/.fluid``), by its canonical name.""" try: from fluid_build._ai_config_shared import load_ai_config name = (load_ai_config() or {}).get("provider") except Exception: # noqa: BLE001 — an unreadable config names nothing return None - return name if isinstance(name, str) and name else None + return _canonical_provider_name(name) + + +def _saved_choice_for(args: Any, env: Dict[str, str], provider: str) -> dict: + """The active ``fluid ai setup`` entry when it is *provider*'s, else ``{}``. + + It fills in what a run does not state, for that provider only: its model, + and its Ollama host, set in *env* here when ``OLLAMA_HOST`` is not. A run + that names its provider (``--llm-provider``, ``FLUID_LLM_PROVIDER``, + ``FLUID_FORGE_AGENT``) takes nothing from it: a named provider without a + stated model gets its catalog model, which Command Center's drafting call + relies on. Shared by ``check_llm_readiness`` and ``resolve_llm_config``. + """ + if _host()._explicit_provider_name(args, env): + return {} + try: + from fluid_build._ai_config_shared import load_ai_config + + saved = load_ai_config() + except Exception: # noqa: BLE001 — an unreadable config supplies nothing + return {} + if not saved or _canonical_provider_name(saved.get("provider")) != provider: + return {} + if provider == "ollama" and saved.get("ollama_host") and not env.get("OLLAMA_HOST"): + env["OLLAMA_HOST"] = saved["ollama_host"] + return saved + + +def _passed_over(saved: str, allow_agents: bool) -> bool: + """Whether a run passes over the saved provider *saved*. + + Only an agent (a coding-agent CLI or MCP sampling), and only where the keyless + auto-pick is off too (``allow_agents=False``): an agent cannot call a stated + endpoint, may not get a stated model meant for another LLM, and has no tool use + for the agent loop. + """ + return not allow_agents and saved in _host()._NON_LITELLM_PROVIDERS def no_provider_message(env: Mapping[str, str]) -> str: """Why no LLM provider is selected. ``fluid doctor`` and forge both say this.""" saved = _saved_provider_name() - if saved: - # Only a command that does not read the saved choice gets here: the - # readiness ladder (``fluid doctor``) uses it. + if saved in _host()._NON_LITELLM_PROVIDERS: + # The one saved choice the ladder passes over (:func:`_passed_over`). return ( - f"No LLM provider is selected for this run: 'fluid ai setup' saved {saved}, " - f"but this command does not read the saved choice. Pass --llm-provider {saved}." + f"No LLM provider is selected: 'fluid ai setup' saved {saved}, and an agent " + "is not used when the run states an endpoint or a model, or runs the agent " + "loop. Name a provider with --llm-provider." ) signals = _explicit_key_signals(env) if len(signals) > 1: @@ -216,15 +267,24 @@ def _first_coding_agent_on_path(env: Mapping[str, str]) -> Optional[str]: def _lean_keyless_autopick( env: Mapping[str, str], *, non_interactive: bool = False, allow_agents: bool = True ) -> Optional[str]: - """The coding agent a run without litellm auto-picks, else ``None``. - - The ambient rung of the ladder without its keyring read, so the litellm - preflight stays cheap: a non-interactive run, no configuring variable, no - saved provider, an agent CLI on PATH, and no local Ollama (it needs litellm, - and a running one is never silently swapped for a cloud agent). - ``allow_agents=False`` (the agent loop, or a stated endpoint or model) - disables it. + """The agent a run without litellm selects, else ``None``. + + The selection ladder without its keyring read, so the litellm preflight + stays cheap. A key variable, a keyring key and Ollama all need litellm, so + two rungs can answer: an agent ``fluid ai setup`` saved, and the keyless + auto-pick (a non-interactive run, no configuring variable, no saved + provider, an agent CLI on PATH, and no local Ollama: a running one is never + silently swapped for a cloud agent). ``allow_agents=False`` (the agent loop, + or a stated endpoint or model) disables both. """ + if _infer_provider_from_explicit_keys(env): + return None + saved = _saved_provider_name() + if saved: + # Below the saved rung only providers that need litellm remain. + if saved in _host()._NON_LITELLM_PROVIDERS and not _passed_over(saved, allow_agents): + return saved + return None if not allow_agents or not _is_non_interactive(non_interactive): return None if _configured_by_env(env): @@ -232,14 +292,6 @@ def _lean_keyless_autopick( agent = _first_coding_agent_on_path(env) if agent is None: return None - try: - from fluid_build._ai_config_shared import load_ai_config - - saved = load_ai_config() - except Exception: # noqa: BLE001 — detection must never hard-fail - saved = None - if saved and saved.get("provider"): - return None detect_fn = getattr(_host(), "detect_ollama_available", None) if detect_fn is not None and detect_fn(env): return None @@ -260,7 +312,7 @@ def _infer_provider_from_ambient( Only call this AFTER the saved config and the keyring have been consulted — ambient services must not override an explicit - operator choice. See :func:`check_llm_readiness` for the full + operator choice. See :func:`_infer_provider_from_env` for the full resolution ladder. """ @@ -280,16 +332,25 @@ def _infer_provider_from_ambient( return None -# Backwards-compatible alias for callers outside this module that -# imported ``_infer_provider_from_env``. The new code paths inside -# :func:`check_llm_readiness` use the split functions above so the -# saved config gets a chance to win over ambient Ollama. def _infer_provider_from_env( env: Mapping[str, str], *, non_interactive: bool = False, allow_agents: bool = True ) -> Optional[str]: + """The provider the environment selects when no provider is named. + + 1. Exactly one explicit key variable: a deliberate per-run choice. + 2. The provider ``fluid ai setup`` saved, unless :func:`_passed_over`. + 3. Exactly one keyring key. + 4. Ambient discovery (:func:`_infer_provider_from_ambient`). + + The rest of the ladder after a named provider, shared by the real selection + and :func:`check_llm_readiness`, so ``fluid doctor`` names what a run uses. + """ explicit = _infer_provider_from_explicit_keys(env) if explicit: return explicit + saved = _saved_provider_name() + if saved and not _passed_over(saved, allow_agents): + return saved keyring_match = _infer_provider_from_keyring() if keyring_match: return keyring_match diff --git a/fluid_build/llm/providers.py b/fluid_build/llm/providers.py index fccf1617..13f982bb 100644 --- a/fluid_build/llm/providers.py +++ b/fluid_build/llm/providers.py @@ -797,7 +797,12 @@ def has_llm_api_key( def _provider_name_from(args: Any, env: Mapping[str, str]) -> Optional[str]: - """The provider the flags and environment select (flag, env, key sniff, ambient). + """The provider the flags and environment select. + + The named provider (``--llm-provider``, ``FLUID_LLM_PROVIDER``, + ``FLUID_FORGE_AGENT``), else :func:`_infer_provider_from_env`: one key + variable, the saved ``fluid ai setup`` choice, one keyring key, then ambient + discovery. :func:`check_llm_readiness` calls this too (``args=None``). ``None`` when nothing selects one. There is no default provider: guessing one would send ``FLUID_LLM_API_KEY`` to a vendor it may not belong to, and @@ -825,11 +830,12 @@ def agent_loop_requested(flag: Any, environ: Optional[Mapping[str, str]] = None) def _agents_allowed(args: Any, env: Mapping[str, str]) -> bool: - """Whether the keyless auto-pick may choose a coding agent for this run. + """Whether an agent this run did not name may be used: auto-picked or saved. Not for the agent loop (it needs tool use, which the agents do not offer), - and not when ``--llm-endpoint`` / ``--llm-model`` (or their routing twins) - state where or what to call: that destination is never swapped for an agent. + and not when ``--llm-endpoint`` / ``--llm-model`` (or their routing twins, + as flags or ``FLUID_LLM_*`` variables) state where or what to call: that + destination is never swapped for an agent. """ return not ( agent_loop_requested(getattr(args, "agent_loop", False), env) @@ -837,6 +843,15 @@ def _agents_allowed(args: Any, env: Mapping[str, str]) -> bool: getattr(args, name, None) for name in ("llm_endpoint", "llm_model", "llm_routing_endpoint", "llm_routing_model") ) + or any( + env.get(var) + for var in ( + "FLUID_LLM_ENDPOINT", + "FLUID_LLM_MODEL", + "FLUID_LLM_ROUTING_ENDPOINT", + "FLUID_LLM_ROUTING_MODEL", + ) + ) ) @@ -918,14 +933,21 @@ def resolve_llm_config(args: Any, environ: Optional[Mapping[str, str]] = None) - raise CopilotGenerationError("copilot_no_llm_provider", no_provider_message(env)) provider = get_llm_provider(provider_name) + # The saved ``fluid ai setup`` choice fills in the model and the Ollama host + # this run does not state, for its own provider and only when the run names + # none: what ``check_llm_readiness`` reports. + saved = _saved_choice_for(args, env, provider.name) + # A stated endpoint is where the call goes, so the Ollama model pick and the # model preflight below ask that daemon, not OLLAMA_HOST or localhost. stated_endpoint = getattr(args, "llm_endpoint", None) or env.get("FLUID_LLM_ENDPOINT") models_endpoint = stated_endpoint if isinstance(stated_endpoint, str) else None - # Resolve model: explicit flag → env var → catalog default → class default. + # Resolve model: explicit flag → env var → saved choice → catalog default → class default. catalog_default = get_catalog_default(provider.name) - explicit_model = getattr(args, "llm_model", None) or env.get("FLUID_LLM_MODEL") + explicit_model = ( + getattr(args, "llm_model", None) or env.get("FLUID_LLM_MODEL") or saved.get("model") + ) if explicit_model: model = resolve_model_name(provider.name, explicit_model) model_source = "explicit" @@ -969,9 +991,13 @@ def resolve_llm_config(args: Any, environ: Optional[Mapping[str, str]] = None) - ) model_notes: List[str] = [] - if not _explicit_provider_name(args, env) and provider.name in _NON_LITELLM_PROVIDERS: - # Nothing named a provider and it is not a key-based one, so the keyless - # auto-pick chose it: say which, and how to override. + if ( + not _explicit_provider_name(args, env) + and not saved + and provider.name in _NON_LITELLM_PROVIDERS + ): + # Nothing named or saved a provider and it is not a key-based one, so the + # keyless auto-pick chose it: say which, and how to override. note = ( f"No LLM provider is configured; using {provider.name}, the first coding agent " f"found on PATH. Override with --llm-provider." @@ -1496,13 +1522,11 @@ def call_llm_streaming( from fluid_build.llm.provider_resolve import ( # noqa: E402,F401 _LLM_KEYRING_PREFIX, _get_api_key_from_keyring, - _infer_provider_from_ambient, _infer_provider_from_env, - _infer_provider_from_explicit_keys, - _infer_provider_from_keyring, _keyring_key, _lean_keyless_autopick, _resolve_api_key, + _saved_choice_for, clear_api_key_from_keyring, no_provider_message, save_api_key_to_keyring, @@ -1575,80 +1599,31 @@ class LlmReadinessCheck: def check_llm_readiness(environ: Optional[Mapping[str, str]] = None) -> LlmReadinessCheck: """Non-throwing check of whether an LLM provider is accessible. - Resolution ladder (highest precedence first): + The provider comes from the ladder the real selection walks + (:func:`_provider_name_from`), so ``fluid doctor`` names what a run uses: - 1. ``FLUID_LLM_PROVIDER`` env var — explicit selector wins everything. - 2. Explicit API-key env vars (``OPENAI_API_KEY`` / - ``ANTHROPIC_API_KEY`` / ``GEMINI_API_KEY`` / ``GOOGLE_API_KEY`` - / ``OLLAMA_HOST``) when exactly one is set — deliberate - per-run override. + 1. A named provider: ``FLUID_LLM_PROVIDER`` or ``FLUID_FORGE_AGENT``. + 2. Exactly one explicit key variable (``OPENAI_API_KEY`` / + ``ANTHROPIC_API_KEY`` / ``GEMINI_API_KEY`` / ``GOOGLE_API_KEY`` / + ``OLLAMA_HOST``) — a deliberate per-run choice. 3. ``~/.fluid/ai_config.json`` — the user's persisted choice from ``fluid ai setup``. This MUST beat ambient detection (e.g. Ollama running on ``localhost:11434``); operators report confusion when ``fluid doctor`` shows ``ollama`` despite having configured Gemini / Anthropic / OpenAI explicitly. - 4. Keyring — a saved provider key in the OS keyring. - 5. Ambient Ollama on localhost — last-resort discovery. + 4. Keyring — exactly one saved provider key in the OS keyring. + 5. Ambient discovery — a local Ollama, then (non-interactive, nothing + configured) the first coding-agent CLI on PATH. + + Unless the provider was named (rung 1), the saved choice's model, key and + Ollama host apply when it is that provider's: ``GOOGLE_API_KEY=…`` plus a + saved ``{provider: gemini, model: gemini-2.5-pro}`` reports the saved model. Used by ``fluid doctor`` and forge. """ env = dict(environ or os.environ) - saved = None - saved_model = None - saved_key = None - - # Step 1 — explicit env-var selector. - provider_name = env.get("FLUID_LLM_PROVIDER") - - # Step 2 — explicit API-key env vars (deliberate per-run override). - if not provider_name: - provider_name = _infer_provider_from_explicit_keys(env) - - # Step 3 — saved ``~/.fluid/ai_config.json`` choice. - # - # Read the saved config through the tier-0 ``_ai_config_shared`` leaf, NOT - # ``cli.ai_setup``. Importing ``ai_setup`` here (from inside the LLM-provider - # module) is the edge that closed the ``ai_setup <-> llm_providers`` import - # cycle; the leaf owns the shared config-loading surface so neither module - # imports the other for it. - if not provider_name: - try: - from fluid_build._ai_config_shared import load_ai_config - - saved = load_ai_config() - if saved: - provider_name = saved.get("provider") - saved_model = saved.get("model") - saved_key = saved.get("api_key") - LOG.debug("LLM readiness: found provider=%s in config file", provider_name) - except ImportError: - pass - - # Step 4 — keyring fallback. - if not provider_name: - provider_name = _infer_provider_from_keyring() - - # Step 5 — last-resort ambient discovery (Ollama on localhost). - if not provider_name: - provider_name = _infer_provider_from_ambient(env) - - # Even when the provider was selected via env-var inference, we - # still want the saved config's model + key for the matching - # provider — that way ``GOOGLE_API_KEY=…`` plus a saved - # ``ai_config.json`` of ``{provider: gemini, model: gemini-2.5-pro}`` - # honours the saved model. - if provider_name and saved is None: - try: - from fluid_build._ai_config_shared import load_ai_config - - saved = load_ai_config() - if saved and saved.get("provider") == provider_name: - saved_model = saved.get("model") - saved_key = saved.get("api_key") - except ImportError: - pass - + provider_name = _provider_name_from(None, env) if not provider_name: LOG.debug("LLM readiness: no provider detected from env or config") return LlmReadinessCheck( @@ -1664,13 +1639,11 @@ def check_llm_readiness(environ: Optional[Mapping[str, str]] = None) -> LlmReadi error=f"Unknown LLM provider '{provider_name}'.", ) - # For ollama from config, ensure OLLAMA_HOST is set - if provider.name == "ollama" and saved and saved.get("ollama_host"): - if not env.get("OLLAMA_HOST"): - env["OLLAMA_HOST"] = saved["ollama_host"] + # The saved choice's model, key and Ollama host, when it is this provider's. + saved = _saved_choice_for(None, env, provider.name) # Resolve API key: env vars → config file - api_key = _resolve_api_key(provider.name, env) or saved_key + api_key = _resolve_api_key(provider.name, env) or saved.get("api_key") from fluid_build.llm.coding_agent import _AGENT_SPECS agent = _AGENT_SPECS.get(provider.name) @@ -1702,7 +1675,7 @@ def check_llm_readiness(environ: Optional[Mapping[str, str]] = None) -> LlmReadi error=f"No API key found for {provider.name}. Run 'fluid ai setup'.", ) - model = env.get("FLUID_LLM_MODEL") or saved_model or provider.default_model + model = env.get("FLUID_LLM_MODEL") or saved.get("model") or provider.default_model endpoint = env.get("FLUID_LLM_ENDPOINT") or provider.default_endpoint(model, env) # Every built-in provider except the keyless agent/MCP ones runs through diff --git a/tests/conftest.py b/tests/conftest.py index 5d50f190..afa2a5dc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -309,6 +309,41 @@ def delete_password(self, service, username): keyring.set_keyring(previous) +@pytest.fixture(scope="session") +def _empty_fluid_home(tmp_path_factory): + return tmp_path_factory.mktemp("no-fluid-home") + + +@pytest.fixture(autouse=True) +def _hermetic_saved_ai_config(monkeypatch, _empty_fluid_home): + """No test reads the developer's saved ``fluid ai setup`` choice. + + Provider selection and ``fluid doctor`` read it through + ``_ai_config_shared.load_ai_config``: the ``llm:`` section of the unified + ``~/.fluid/config.yaml``, then ``~/.fluid/ai_config.json``. A contributor's + own saved provider would change what a test resolves, as the OS keyring + would without ``_hermetic_keyring``. Both default paths point into a + per-test directory that does not exist, so nothing one test saves reaches + another. A test that wants a saved choice patches ``load_ai_config`` or + ``_CONFIG_FILE`` itself, or sets ``FLUID_CONFIG`` / ``FLUID_HOME``, which + ``unified_config_path`` reads before the home default. + """ + import os + import sys + import uuid + + home = _empty_fluid_home / uuid.uuid4().hex + config_file = home / "ai_config.json" + monkeypatch.setattr("fluid_build._ai_config_shared._CONFIG_FILE", config_file) + ai_setup = sys.modules.get("fluid_build.cli.ai_setup") + if ai_setup is not None: + # The same object: ``ai_setup`` re-exports the constant, and its write + # path reads it there. + monkeypatch.setattr(ai_setup, "_CONFIG_FILE", config_file) + if not (os.environ.get("FLUID_CONFIG") or os.environ.get("FLUID_HOME")): + monkeypatch.setenv("FLUID_CONFIG", str(home / "config.yaml")) + + def _noop_assert_safe_url(url, *, allow_private=False): # noqa: ARG001 """Replacement for safe_http.assert_safe_url used in unit tests. diff --git a/tests/test_ai_setup.py b/tests/test_ai_setup.py index cfa14f1e..80911d1a 100644 --- a/tests/test_ai_setup.py +++ b/tests/test_ai_setup.py @@ -262,13 +262,10 @@ def test_ready_with_openai_env(self): def test_not_ready_without_keys(self): from fluid_build.cli.forge_copilot_llm_providers import check_llm_readiness - # Patch every step in the resolution ladder so the real config - # file isn't read, no ambient Ollama is picked up, and no - # keyring entry leaks in. The provider-precedence fix split - # ``_infer_provider_from_env`` into explicit / keyring / - # ambient helpers — ``check_llm_readiness`` calls them - # individually so every test-time Ollama install must be - # mocked at the ambient step. + # Readiness walks the selection ladder (``_provider_name_from``), so + # every rung's input is emptied: no key variable in the environment + # handed in, no saved config (never the real ~/.fluid), the hermetic + # keyring from conftest, no ambient Ollama and no agent CLI on PATH. with ( patch("fluid_build.cli._ai_config_shared.load_ai_config", return_value=None), patch( @@ -276,19 +273,15 @@ def test_not_ready_without_keys(self): Path("/nonexistent/ai_config.json"), ), patch( - "fluid_build.cli.forge_copilot_llm_providers._infer_provider_from_explicit_keys", - return_value=None, - ), - patch( - "fluid_build.cli.forge_copilot_llm_providers._infer_provider_from_keyring", - return_value=None, + "fluid_build.cli.forge_copilot_llm_providers.detect_ollama_available", + return_value=False, ), patch( - "fluid_build.cli.forge_copilot_llm_providers._infer_provider_from_ambient", + "fluid_build.llm.provider_resolve._first_coding_agent_on_path", return_value=None, ), ): - result = check_llm_readiness({}) + result = check_llm_readiness({"PATH": "/nonexistent"}) assert not result.ready assert result.error is not None @@ -1048,11 +1041,8 @@ def test_resolve_llm_config_stops_when_no_provider_is_selected(self, monkeypatch args = argparse.Namespace(llm_provider=None, llm_model=None, llm_endpoint=None) + # The keyring is conftest's empty in-memory one. with ( - patch( - "fluid_build.cli.forge_copilot_llm_providers._infer_provider_from_keyring", - return_value=None, - ), patch( "fluid_build.cli.forge_copilot_llm_providers.detect_ollama_available", return_value=False, diff --git a/tests/test_litellm_absent.py b/tests/test_litellm_absent.py index 4f75f9e8..dd233c68 100644 --- a/tests/test_litellm_absent.py +++ b/tests/test_litellm_absent.py @@ -487,6 +487,24 @@ def test_a_saved_provider_still_fails_fast(self, block, monkeypatch, tmp_path): with pytest.raises(MissingOptionalDependency): preflight_llm_provider(self._args(), env) + def test_a_saved_agent_passes_the_preflight(self, block, monkeypatch, tmp_path): # noqa: F811 + # ``fluid ai setup`` saved a coding agent, which needs no litellm: the run + # uses it, as the selection ladder does. + from fluid_build.llm.providers import _provider_name_from, preflight_llm_provider + + env = self._lean(block, monkeypatch, tmp_path, saved={"provider": "claude-code"}) + preflight_llm_provider(self._args(), env) + assert _provider_name_from(self._args(), env) == "claude-code" + + def test_a_saved_agent_with_a_stated_endpoint_still_fails_fast( + self, block, monkeypatch, tmp_path + ): # noqa: F811 + from fluid_build.llm.providers import preflight_llm_provider + + env = self._lean(block, monkeypatch, tmp_path, saved={"provider": "claude-code"}) + with pytest.raises(MissingOptionalDependency): + preflight_llm_provider(self._args(llm_endpoint="http://onprem.internal/v1"), env) + def test_a_tty_still_fails_fast(self, block, monkeypatch, tmp_path): # noqa: F811 from fluid_build.llm.providers import preflight_llm_provider @@ -552,8 +570,13 @@ def test_nothing_configured_says_not_configured(self, block, monkeypatch): # no ): monkeypatch.delenv(var, raising=False) monkeypatch.setattr("fluid_build._ai_config_shared.load_ai_config", lambda *a, **k: None) - for name in ("_infer_provider_from_keyring", "_infer_provider_from_ambient"): - monkeypatch.setattr(f"fluid_build.llm.providers.{name}", lambda *a, **k: None) + # Readiness walks the selection ladder: empty its keyring, Ollama and + # agent rungs at their inputs. + monkeypatch.setattr("fluid_build.llm.providers._get_api_key_from_keyring", lambda n: None) + monkeypatch.setattr("fluid_build.llm.providers.detect_ollama_available", lambda env: False) + monkeypatch.setattr( + "fluid_build.llm.provider_resolve._first_coding_agent_on_path", lambda env: None + ) row = self._row(monkeypatch) assert "Not configured" in row["status"] assert "fluid ai setup" in row["details"] diff --git a/tests/test_llm_keyless_autopick.py b/tests/test_llm_keyless_autopick.py index 1bbb00af..16364490 100644 --- a/tests/test_llm_keyless_autopick.py +++ b/tests/test_llm_keyless_autopick.py @@ -62,8 +62,11 @@ def _args(**kw): @pytest.fixture def world(monkeypatch, tmp_path): """An empty world: no keys, no saved config, no keyring, no Ollama, empty PATH.""" - for var in _SELECTOR_ENV + _KEY_ENV: + for var in _SELECTOR_ENV + _KEY_ENV + ("FLUID_COPILOT_AGENT_LOOP",): monkeypatch.delenv(var, raising=False) + # Anything else that reads ``~/.fluid`` (the model catalog's user override) + # finds an empty home. + monkeypatch.setenv("HOME", str(tmp_path / "home")) bindir = tmp_path / "bin" bindir.mkdir() monkeypatch.setenv("PATH", str(bindir)) @@ -89,6 +92,18 @@ def _tty(world, value): world.monkeypatch.setattr(sys, "stdin", SimpleNamespace(isatty=lambda: value)) +def _saved(world, **entry): + """``fluid ai setup`` saved *entry* as the active provider.""" + world.monkeypatch.setattr( + "fluid_build._ai_config_shared.load_ai_config", lambda *a, **k: dict(entry) + ) + + +def _keyring(world, **keys): + """The OS keyring holds *keys* (provider name to key).""" + world.monkeypatch.setattr(providers, "_get_api_key_from_keyring", lambda name: keys.get(name)) + + class TestPick: def test_claude_only(self, world): world.install("claude-code") @@ -198,13 +213,10 @@ def test_ambiguous_keyring_is_still_configured(self, world): world.monkeypatch.setattr(providers, "_get_api_key_from_keyring", lambda n: "k") assert _provider_name_from(_args(), dict(world.env)) is None - def test_saved_config_is_still_configured(self, world): + def test_saved_config_beats_an_agent_on_path(self, world): world.install("claude-code") - world.monkeypatch.setattr( - "fluid_build._ai_config_shared.load_ai_config", - lambda *a, **k: {"provider": "openai", "model": "gpt-4.1-mini"}, - ) - assert _provider_name_from(_args(), dict(world.env)) is None + _saved(world, provider="openai", model="gpt-4.1-mini") + assert _provider_name_from(_args(), dict(world.env)) == "openai" class TestSaysWhichWasPicked: @@ -225,7 +237,288 @@ def test_explicit_agent_choice_is_not_announced(self, world, caplog): assert not [r for r in caplog.records if "--llm-provider" in r.getMessage()] +class TestSavedChoiceSelects: + """The provider ``fluid ai setup`` saved is one rung of the selection ladder. + + It comes after a named provider (``--llm-provider``, ``FLUID_LLM_PROVIDER``, + ``FLUID_FORGE_AGENT``) and exactly one key variable, and before a keyring + key, a running Ollama and the keyless auto-pick: the order + ``check_llm_readiness`` uses, so a run without a TTY uses the choice + ``fluid doctor`` shows. + """ + + def test_the_saved_provider_is_selected_without_a_tty(self, world): + _saved(world, provider="anthropic") + assert _provider_name_from(_args(non_interactive=True), dict(world.env)) == "anthropic" + + def test_one_key_variable_beats_the_saved_provider(self, world): + _saved(world, provider="anthropic") + env = {**world.env, "OPENAI_API_KEY": "k"} # pragma: allowlist secret + assert _provider_name_from(_args(), env) == "openai" + + def test_the_saved_provider_breaks_a_tie_between_two_keys(self, world): + _saved(world, provider="anthropic") + env = {**world.env, "OPENAI_API_KEY": "k", "GEMINI_API_KEY": "k"} + assert _provider_name_from(_args(), env) == "anthropic" + + def test_the_saved_provider_beats_a_keyring_key(self, world): + _saved(world, provider="anthropic") + _keyring(world, openai="k") + assert _provider_name_from(_args(), dict(world.env)) == "anthropic" + + def test_the_saved_provider_beats_a_running_ollama(self, world): + _saved(world, provider="anthropic") + world.monkeypatch.setattr(providers, "detect_ollama_available", lambda env: True) + assert _provider_name_from(_args(), dict(world.env)) == "anthropic" + + @pytest.mark.parametrize( + "flags, env, named", + [ + ({"llm_provider": "gemini"}, {}, "gemini"), + ({}, {"FLUID_LLM_PROVIDER": "gemini"}, "gemini"), + ({}, {"FLUID_FORGE_AGENT": "codex"}, "codex"), + ], + ) + def test_a_named_provider_beats_the_saved_one(self, world, flags, env, named): + _saved(world, provider="anthropic") + assert _provider_name_from(_args(**flags), {**world.env, **env}) == named + + +class TestSavedChoiceResolves: + """The saved model and Ollama host go with the saved provider, and only with it.""" + + def test_the_saved_model_and_the_keyring_key(self, world): + _saved(world, provider="anthropic", model="claude-saved-test") + _keyring(world, anthropic="kr-test") # pragma: allowlist secret + config = resolve_llm_config(_args(non_interactive=True), dict(world.env)) + assert (config.provider, config.model) == ("anthropic", "claude-saved-test") + assert config.api_key == "kr-test" # pragma: allowlist secret + + @pytest.mark.parametrize( + "flags, env", + [({"llm_model": "claude-stated"}, {}), ({}, {"FLUID_LLM_MODEL": "claude-stated"})], + ) + def test_a_stated_model_beats_the_saved_model(self, world, flags, env): + _saved(world, provider="anthropic", model="claude-saved-test") + _keyring(world, anthropic="kr-test") # pragma: allowlist secret + config = resolve_llm_config(_args(**flags), {**world.env, **env}) + assert config.model == "claude-stated" + + def test_the_saved_model_goes_only_with_the_saved_provider(self, world): + _saved(world, provider="anthropic", model="claude-saved-test") + env = {**world.env, "OPENAI_API_KEY": "k"} # pragma: allowlist secret + config = resolve_llm_config(_args(), env) + assert config.provider == "openai" + assert config.model_source == "catalog" + + @pytest.mark.parametrize( + "flags, env", + [({"llm_provider": "anthropic"}, {}), ({}, {"FLUID_LLM_PROVIDER": "anthropic"})], + ) + def test_a_named_provider_takes_nothing_from_the_saved_choice(self, world, flags, env): + # Named means stated for this run: the catalog model, not the saved one. + _saved(world, provider="anthropic", model="claude-saved-test") + env = {**world.env, "ANTHROPIC_API_KEY": "k", **env} # pragma: allowlist secret + config = resolve_llm_config(_args(**flags), env) + assert (config.provider, config.model_source) == ("anthropic", "catalog") + assert config.model != "claude-saved-test" + + @pytest.mark.parametrize("spelling", ["claude", "Anthropic"]) + def test_a_saved_alias_is_its_provider(self, world, spelling): + # A hand-written ~/.fluid/config.yaml may spell the provider another way. + _saved(world, provider=spelling, model="claude-saved-test") + _keyring(world, anthropic="kr-test") # pragma: allowlist secret + config = resolve_llm_config(_args(), dict(world.env)) + assert (config.provider, config.model) == ("anthropic", "claude-saved-test") + assert check_llm_readiness(dict(world.env)).model == "claude-saved-test" + + def test_the_saved_ollama_host_is_where_the_call_goes(self, world): + _saved( + world, + provider="ollama", + model="llama3.2:saved", + ollama_host="http://gpu-box.internal:11434", + ) + config = resolve_llm_config(_args(), dict(world.env)) + assert (config.provider, config.model) == ("ollama", "llama3.2:saved") + assert config.endpoint == "http://gpu-box.internal:11434" + + def test_an_ollama_host_in_the_environment_beats_the_saved_one(self, world): + _saved( + world, + provider="ollama", + model="llama3.2:saved", + ollama_host="http://gpu-box.internal:11434", + ) + config = resolve_llm_config(_args(), {**world.env, "OLLAMA_HOST": "http://127.0.0.1:11500"}) + assert config.endpoint == "http://127.0.0.1:11500" + assert config.model == "llama3.2:saved" + + def test_a_saved_agent_is_not_announced_as_auto_picked(self, world, caplog): + world.install("claude-code") + _saved(world, provider="claude-code") + with caplog.at_level(logging.WARNING, logger=providers.LOG.name): + config = resolve_llm_config(_args(), dict(world.env)) + assert config.provider == "claude-code" + assert not [r for r in caplog.records if "first coding agent" in r.getMessage()] + assert not [n for n in config.model_resolution_notes if "first coding agent" in n] + + def test_a_saved_provider_without_a_key_asks_for_the_key(self, world): + _saved(world, provider="anthropic") + with pytest.raises(providers.CopilotGenerationError) as info: + resolve_llm_config(_args(), dict(world.env)) + assert info.value.event == "copilot_missing_llm_api_key" + + def test_a_saved_key_provider_calls_a_stated_endpoint(self, world): + # As with a keyring key or a key variable: the operator stated where to call. + _saved(world, provider="openai") + _keyring(world, openai="kr-test") # pragma: allowlist secret + config = resolve_llm_config(_args(llm_endpoint="http://proxy.internal/v1"), dict(world.env)) + assert (config.provider, config.endpoint) == ("openai", "http://proxy.internal/v1") + + +class TestSavedAgentNeverTakesAStatedDestination: + """A saved coding agent is passed over wherever the auto-picked one is. + + It cannot call a stated endpoint, cannot be trusted with a stated (perhaps + on-prem) model and has no tool use for the agent loop. The run goes on + down the ladder, and stops with ``copilot_no_llm_provider`` naming the + saved agent when nothing else selects. + """ + + _ONPREM = "http://onprem-llm.internal:8000/v1" + + @pytest.mark.parametrize( + "flags, env", + [ + ({"llm_endpoint": _ONPREM}, {}), + ({"llm_model": "my-model"}, {}), + ({"agent_loop": True}, {}), + ({}, {"FLUID_LLM_ENDPOINT": _ONPREM}), + ({}, {"FLUID_LLM_MODEL": "my-model"}), + ({}, {"FLUID_COPILOT_AGENT_LOOP": "1"}), + ], + ) + def test_the_run_stops_and_names_the_saved_agent(self, world, flags, env): + world.install("claude-code") + _saved(world, provider="claude-code") + with pytest.raises(providers.CopilotGenerationError) as info: + resolve_llm_config(_args(**flags), {**world.env, **env}) + assert info.value.event == "copilot_no_llm_provider" + assert "claude-code" in info.value.message + assert "--llm-provider" in info.value.message + + def test_the_run_goes_on_down_the_ladder(self, world): + world.install("claude-code") + _saved(world, provider="claude-code") + world.monkeypatch.setattr(providers, "detect_ollama_available", lambda env: True) + assert _provider_name_from(_args(llm_model="llama3.2"), dict(world.env)) == "ollama" + + def test_readiness_says_the_same(self, world): + world.install("claude-code") + _saved(world, provider="claude-code") + ready = check_llm_readiness({**world.env, "FLUID_COPILOT_AGENT_LOOP": "1"}) + assert ready.ready is False + assert "claude-code" in ready.error + + def test_an_underscore_spelling_is_the_same_agent(self, world, caplog): + world.install("claude-code") + _saved(world, provider="claude_code") + assert _provider_name_from(_args(llm_model="my-model"), dict(world.env)) is None + with caplog.at_level(logging.WARNING, logger=providers.LOG.name): + config = resolve_llm_config(_args(), dict(world.env)) + assert config.provider == "claude-code" + assert not [r for r in caplog.records if "first coding agent" in r.getMessage()] + + def test_a_mission_never_gets_a_saved_agent(self, world): + # ``fluid mission run`` always runs the tool-use agent loop. + import argparse + + from fluid_build.cli.mission import _resolve_mission_llm_config + + world.install("claude-code") + _saved(world, provider="claude-code") + args = argparse.Namespace(llm_provider=None, llm_model=None, llm_endpoint=None) + config, message = _resolve_mission_llm_config(args) + assert config is None + assert "copilot_no_llm_provider" in message # stopped before any agent ran + assert not hasattr(args, "agent_loop") # the caller's args are left alone + + +def _ladder_world(world, case): + """Set up one ladder case; returns the environment to resolve against.""" + env = dict(world.env) + if case == "saved": + _saved(world, provider="anthropic", model="claude-saved-test") + _keyring(world, anthropic="k") + elif case == "saved_vs_one_key": + _saved(world, provider="anthropic", model="claude-saved-test") + env["OPENAI_API_KEY"] = "k" # pragma: allowlist secret + elif case == "saved_vs_two_keys": + _saved(world, provider="anthropic", model="claude-saved-test") + _keyring(world, anthropic="k") + env.update(OPENAI_API_KEY="k", GEMINI_API_KEY="k") # pragma: allowlist secret + elif case == "saved_vs_keyring": + _saved(world, provider="anthropic", model="claude-saved-test") + _keyring(world, openai="k", anthropic="k") + elif case == "saved_vs_ollama": + _saved(world, provider="anthropic", model="claude-saved-test") + _keyring(world, anthropic="k") + world.monkeypatch.setattr(providers, "detect_ollama_available", lambda env: True) + elif case == "saved_vs_agent": + world.install("claude-code") + _saved(world, provider="openai", model="gpt-saved-test") + _keyring(world, openai="k") + elif case == "saved_agent": + world.install("claude-code") + _saved(world, provider="claude-code") + elif case == "forge_agent_var_vs_key": + world.install("codex") + env.update(FLUID_FORGE_AGENT="codex", OPENAI_API_KEY="k") # pragma: allowlist secret + elif case == "agent_loop_var": + world.install("claude-code") + env["FLUID_COPILOT_AGENT_LOOP"] = "1" + elif case == "named_var_vs_saved": + _saved(world, provider="anthropic", model="claude-saved-test") + _keyring(world, anthropic="k") + env["FLUID_LLM_PROVIDER"] = "anthropic" + return env + + +_LADDER_CASES = ( + "saved", + "saved_vs_one_key", + "saved_vs_two_keys", + "saved_vs_keyring", + "saved_vs_ollama", + "saved_vs_agent", + "saved_agent", + "forge_agent_var_vs_key", + "agent_loop_var", + "named_var_vs_saved", +) + + class TestReadinessAgreesWithSelection: + @pytest.mark.parametrize("case", _LADDER_CASES) + def test_one_ladder(self, world, case): + # ``fluid doctor`` names the provider a run without a TTY then uses. + env = _ladder_world(world, case) + assert check_llm_readiness(env).provider == _provider_name_from(_args(), env) + + @pytest.mark.parametrize("case", ["saved", "saved_vs_two_keys", "saved_vs_ollama"]) + def test_the_saved_model_is_the_model_both_report(self, world, case): + env = _ladder_world(world, case) + ready = check_llm_readiness(env) + assert ready.ready is True + assert ready.model == resolve_llm_config(_args(), env).model == "claude-saved-test" + + def test_a_named_provider_reports_its_catalog_model_in_both(self, world): + env = _ladder_world(world, "named_var_vs_saved") + ready = check_llm_readiness(env) + config = resolve_llm_config(_args(), env) + assert ready.model == config.model != "claude-saved-test" + def test_readiness_reports_the_same_provider_non_interactive(self, world): world.install("codex") ready = check_llm_readiness({}) @@ -437,3 +730,52 @@ def default_endpoint(self, model, env): {"FLUID_LLM_PROVIDER": "myplugin", "PATH": str(world.bindir)} ) assert ready.ready is True + + +class TestCommandCenterDraftingIsUnchanged: + """Command Center's drafting call keeps its provider and its catalog model. + + ``fluid_cc_backend/app/services/fluid_cli_service.py::forge_from_intent`` runs + ``fluid forge data-model from-intent --output --technique + dimensional --quiet --llm-provider

`` with the key in the environment and + no ``--llm-model``. Its container has no saved ``fluid ai setup`` choice, and + a saved one would change nothing: the provider is named. + """ + + @staticmethod + def _cc_args(provider): + import argparse + + from fluid_build.cli import forge as forge_mod + + parser = argparse.ArgumentParser() + forge_mod.register(parser.add_subparsers()) + argv = ["forge", "data-model", "from-intent", "intent.fluid.yaml"] + argv += ["--output", "contract.fluid.yaml", "--technique", "dimensional", "--quiet"] + return parser.parse_args([*argv, "--llm-provider", provider]) + + @pytest.mark.parametrize("saved", [None, "another provider", "the same provider"]) + @pytest.mark.parametrize( + "provider, key_vars, canonical", + [ + ("openai", ("OPENAI_API_KEY",), "openai"), + ("anthropic", ("ANTHROPIC_API_KEY",), "anthropic"), + ("claude", ("ANTHROPIC_API_KEY",), "anthropic"), + ("gemini", ("GOOGLE_API_KEY", "GEMINI_API_KEY"), "gemini"), + ], + ) + def test_provider_and_model(self, world, saved, provider, key_vars, canonical): + from fluid_build.llm.model_catalog import get_catalog_default + + if saved == "another provider": + _saved(world, provider="ollama", model="llama3.2:saved") + elif saved == "the same provider": + # A named provider takes nothing from the saved choice. + _saved(world, provider=canonical, model="saved-model-test") + env = {**world.env, **{var: "k" for var in key_vars}} + config = resolve_llm_config(self._cc_args(provider), env) + assert config.provider == canonical + expected = ( + get_catalog_default(canonical) or providers.get_llm_provider(canonical).default_model + ) + assert (config.model, config.model_source) == (expected, "catalog") diff --git a/tests/test_llm_no_provider.py b/tests/test_llm_no_provider.py index c9779ee0..beaf3e4f 100644 --- a/tests/test_llm_no_provider.py +++ b/tests/test_llm_no_provider.py @@ -127,13 +127,13 @@ def test_the_error_has_catalog_suggestions(): assert suggestions_for("copilot_no_llm_provider") -def test_a_saved_setup_choice_is_named_instead_of_asking_to_run_setup(monkeypatch): - # Commands that do not read the saved choice (non-interactive forge) say so. +def test_a_saved_setup_choice_selects_its_provider(monkeypatch): + # A run without a TTY reads the saved choice too, so what is missing is + # that provider's key, not a provider. monkeypatch.setattr( "fluid_build._ai_config_shared.load_ai_config", lambda *a, **k: {"provider": "anthropic"} ) with pytest.raises(CopilotGenerationError) as info: resolve_llm_config(_args(), _NOTHING) - assert info.value.event == "copilot_no_llm_provider" - assert "'fluid ai setup' saved anthropic" in info.value.message - assert "--llm-provider anthropic" in info.value.message + assert info.value.event == "copilot_missing_llm_api_key" + assert "anthropic" in info.value.message From dfb8f64b27dd929cf14e2d4aaa750a82ab1f82c8 Mon Sep 17 00:00:00 2001 From: Speculator55005 <50082482+fas89@users.noreply.github.com> Date: Thu, 12 Nov 2026 13:24:13 +0100 Subject: [PATCH 3/3] fix(forge): keep `fluid forge --agent` stdout to JSON Lines AGENT_IDE.md promises one JSON object per line on stdout. But Rich panels ("Forge Complete", "Next steps"), prints and error text landed between the `{"event": ...}` lines. In blank mode, 28 of 32 stdout lines were not JSON. Under `--agent`, `forge.run` keeps the stdout the run started with for the events (`_AGENT_EVENTS`) and runs the rest under `contextlib.redirect_stdout(sys.stderr)`. Every human-facing writer moves to stderr without being touched: - Rich consoles built without `file=` (Rich looks `sys.stdout` up at each write, rich/console.py); - `print`; - a provider SDK's banner. A missing extra still propagates from `forge.run`, so an in-process caller (the MCP `forge_run`) still gets the exception. The CLI entry point prints the remedy on stderr under `--agent` (`_render_missing_optional`). Outside `--agent`, nothing changes. The redirect is the stdlib pattern `cli/mcp/dispatch.py` already uses to capture the same events in-process. --- fluid_build/cli/__init__.py | 10 ++- fluid_build/cli/forge.py | 66 +++++++++------ tests/cli/test_forge_agent_mode.py | 128 ++++++++++++++++++++++++++++- 3 files changed, 176 insertions(+), 28 deletions(-) diff --git a/fluid_build/cli/__init__.py b/fluid_build/cli/__init__.py index 839bd7ab..c35939a5 100644 --- a/fluid_build/cli/__init__.py +++ b/fluid_build/cli/__init__.py @@ -301,8 +301,14 @@ def _render_missing_optional(e: Any, args: argparse.Namespace, context: Any, cli "exit_code": e.exit_code, }, ) - context.console.print(literal_text(_redact_error_str(f"❌ {e}"), "red")) - context.console.print(literal_text(f"[{slug}]", "dim")) + import contextlib + + # ``fluid forge --agent``: stdout carries the JSON-Lines events only, so the + # remedy goes to stderr with the rest of the run's human output. + agent = getattr(args, "agent", False) + with contextlib.redirect_stdout(sys.stderr) if agent else contextlib.nullcontext(): + context.console.print(literal_text(_redact_error_str(f"❌ {e}"), "red")) + context.console.print(literal_text(f"[{slug}]", "dim")) return int(e.exit_code) diff --git a/fluid_build/cli/forge.py b/fluid_build/cli/forge.py index fd30309b..92d6cbad 100644 --- a/fluid_build/cli/forge.py +++ b/fluid_build/cli/forge.py @@ -14,6 +14,7 @@ from __future__ import annotations import argparse +import contextlib import functools import json import logging @@ -1251,23 +1252,31 @@ def _print_next_steps(console: Any, target_dir: Path, contract_path: Path) -> No # event stream over stdout lets the agent parse progress (start / done / # contract_written / cost) without scraping Rich console output. # -# Design choice: events go to stdout AS WHOLE JSON OBJECTS PER LINE so the -# agent's parser is ``for line in stdout: try: obj = json.loads(line); -# except: pass``. Rich's banners on stdout don't parse as JSON, so they're -# harmlessly ignored. No need to mute Rich (which would break user-facing -# UX outside ``--agent``). +# Design choice: stdout carries the events and nothing else, one whole JSON +# object per line (NDJSON, as AGENT_IDE.md promises), so the agent's parser +# is ``for line in stdout: json.loads(line)``. ``run`` keeps the real stdout +# for the events and points ``sys.stdout`` at stderr for the rest of the run +# (``contextlib.redirect_stdout``, as the MCP ``forge_run`` dispatch does), so +# every human-facing writer moves without being touched: Rich consoles built +# without ``file=`` (Rich looks ``sys.stdout`` up at each write), ``print``, +# and a provider SDK's banner. Outside ``--agent`` nothing changes. + +#: Where ``--agent`` events go while ``run`` points ``sys.stdout`` at stderr: +#: the stdout the run started with. ``None`` outside an ``--agent`` run. +_AGENT_EVENTS: Optional[Any] = None def _emit_agent_jsonl(event: str, **kvs: Any) -> None: - """Emit one canonical JSON-Lines event to stdout for agent consumption. + """Emit one canonical JSON-Lines event on the run's real stdout for the agent. Only used when ``--agent`` is active; callers gate via ``args.agent``. Failures are swallowed so a stdout pipe-closed never poisons the run. """ try: + stream = _AGENT_EVENTS or sys.stdout payload = {"event": event, "ts": time.time(), **kvs} - sys.stdout.write(json.dumps(payload, default=str) + "\n") - sys.stdout.flush() + stream.write(json.dumps(payload, default=str) + "\n") + stream.flush() except Exception: # noqa: BLE001 pass @@ -1492,26 +1501,35 @@ def run(args, logger: logging.Logger) -> int: ) ) ) - _emit_agent_jsonl( - "forge.start", - run_id=run_id, - mode=mode, - data_product_type=getattr(args, "data_product_type", None), - target_dir=str(getattr(args, "target_dir", None) or ""), - emit_plan=bool(getattr(args, "emit_plan", False)), - ) + global _AGENT_EVENTS # noqa: PLW0603 — one run at a time, reset below + _AGENT_EVENTS = sys.stdout rc = 1 try: - rc = _run_main(args, logger) - # On success, optionally emit the structured plan checklist BEFORE - # forge.done so consumers parsing the stream see the plan in order. - if rc == 0 and getattr(args, "emit_plan", False): - target_dir = getattr(args, "target_dir", None) - contract_path = Path(target_dir) / "contract.fluid.yaml" if target_dir else None - _emit_forge_plan(args, contract_path) - return rc + # Everything a human reads goes to stderr; the events keep stdout. + with contextlib.redirect_stdout(sys.stderr): + _emit_agent_jsonl( + "forge.start", + run_id=run_id, + mode=mode, + data_product_type=getattr(args, "data_product_type", None), + target_dir=str(getattr(args, "target_dir", None) or ""), + emit_plan=bool(getattr(args, "emit_plan", False)), + ) + # A missing extra still propagates, as it always has: the CLI entry + # point says the remedy on stderr under --agent + # (``cli._render_missing_optional``), and an in-process caller + # (the MCP ``forge_run``) gets the exception itself. + rc = _run_main(args, logger) + # On success, optionally emit the structured plan checklist BEFORE + # forge.done so consumers parsing the stream see the plan in order. + if rc == 0 and getattr(args, "emit_plan", False): + target_dir = getattr(args, "target_dir", None) + contract_path = Path(target_dir) / "contract.fluid.yaml" if target_dir else None + _emit_forge_plan(args, contract_path) + return rc finally: _emit_agent_jsonl("forge.done", run_id=run_id, exit_code=int(rc or 0)) + _AGENT_EVENTS = None #: Per-process flag for the personal-memory sanitiser. See diff --git a/tests/cli/test_forge_agent_mode.py b/tests/cli/test_forge_agent_mode.py index 0a5c8c58..2e3df781 100644 --- a/tests/cli/test_forge_agent_mode.py +++ b/tests/cli/test_forge_agent_mode.py @@ -38,8 +38,8 @@ def _fluid_invocation() -> list[str]: def _parse_jsonl_events(stdout: str) -> list[dict]: """Filter stdout for lines that parse as JSON and look like our events. - Rich banners and other non-JSON noise on stdout are silently ignored — - matches what the agent's parser will do. + Lenient on purpose, as a defensive agent parser is; that stdout holds + nothing else is asserted by ``_json_lines_only`` (section 7). """ events: list[dict] = [] for line in stdout.splitlines(): @@ -535,3 +535,127 @@ def test_agent_ai_mode_expands_a_tilde_target_dir(tmp_path, capsys, monkeypatch) contract = (home / "prod" / "contract.fluid.yaml").resolve() assert [(e["path"], e["action"]) for e in written] == [(str(contract), "created")] + + +# --------------------------------------------------------------------------- +# 7. stdout carries the JSON-Lines events and nothing else (AGENT_IDE.md) +# --------------------------------------------------------------------------- + + +def _json_lines_only(stdout: str) -> list[dict]: + """Every stdout line is one JSON event object, or the test fails on that line.""" + events: list[dict] = [] + for line in stdout.splitlines(): + try: + obj = json.loads(line) + except json.JSONDecodeError: + pytest.fail(f"non-JSON line on --agent stdout: {line!r}\nstdout:\n{stdout}") + assert isinstance(obj, dict) and isinstance(obj.get("event"), str), line + events.append(obj) + assert events, "no events on stdout" + return events + + +def _hermetic_env(tmp_path: Path) -> dict: + """The process env with no LLM keys, an empty home and no OS keyring.""" + env = { + k: v + for k, v in os.environ.items() + if not k.endswith("_API_KEY") and not k.startswith(("FLUID_LLM_", "FLUID_FORGE_")) + } + home = tmp_path / "home" + home.mkdir(exist_ok=True) + env.update(HOME=str(home), PYTHON_KEYRING_BACKEND="keyring.backends.null.Keyring") + return env + + +def test_agent_blank_stdout_is_json_lines_only(tmp_path: Path): + """The real wire format: the human panels move to stderr, not off the run.""" + out = tmp_path / "product" + proc = subprocess.run( + [*_fluid_invocation(), "--agent", "--blank", "--data-product-type", "SDP", "-d", str(out)], + stdin=subprocess.DEVNULL, + capture_output=True, + text=True, + timeout=60, + env=_hermetic_env(tmp_path), + ) + assert proc.returncode == 0, proc.stderr + names = [e["event"] for e in _json_lines_only(proc.stdout)] + assert names[0] == "forge.start" and names[-1] == "forge.done" + assert "forge.contract_written" in names + assert "Forge Complete" in proc.stderr + + +def test_agent_ai_stdout_is_json_lines_only(tmp_path, capsys, monkeypatch): + """AI mode with the generation seam mocked: what it and forge print goes to stderr. + + The fake prints the way the AI path does: a Rich ``Console()`` built in its + own module, a plain ``print`` (a provider SDK's banner), and forge's own + success output afterwards. + """ + from rich.console import Console + from rich.panel import Panel + + from fluid_build.cli import forge as forge_mod + + product = tmp_path / "product" + + def _fake_ai(args, logger): + Console().print(Panel("Contract drafted by the copilot", title="Forge Complete")) + print("Give Feedback / Get Help: provider SDK banner") + product.mkdir(parents=True, exist_ok=True) + (product / "contract.fluid.yaml").write_text("fluidVersion: 0.7.6\n") + return 0 + + monkeypatch.chdir(tmp_path) + monkeypatch.setattr(forge_mod, "run_ai_copilot_mode", _fake_ai) + monkeypatch.setattr( + "builtins.input", lambda *a, **k: pytest.fail("input() called under --agent") + ) + args = _agent_args(["--llm-provider", "claude-code"], tmp_path) + assert forge_mod.run(args, logging.getLogger("test")) == 0 + + captured = capsys.readouterr() + events = _json_lines_only(captured.out) + names = [e["event"] for e in events] + assert names[0] == "forge.start" and names[-1] == "forge.done" + assert "forge.contract_written" in names + assert "Contract drafted by the copilot" in captured.err + assert "provider SDK banner" in captured.err + + +def _raise_missing_litellm(args, logger): + from fluid_build._optional import MissingOptionalDependency + + raise MissingOptionalDependency("litellm", "litellm", "AI-assisted authoring") + + +def test_agent_missing_extra_still_propagates_in_process(tmp_path, capsys, monkeypatch): + """An in-process caller (the MCP ``forge_run``) gets the exception, after forge.done.""" + from fluid_build._optional import MissingOptionalDependency + from fluid_build.cli import forge as forge_mod + + monkeypatch.setattr(forge_mod, "run_ai_copilot_mode", _raise_missing_litellm) + args = _agent_args(["--llm-provider", "openai"], tmp_path) + with pytest.raises(MissingOptionalDependency): + forge_mod.run(args, logging.getLogger("test")) + + events = _json_lines_only(capsys.readouterr().out) + assert events[-1]["event"] == "forge.done" + assert events[-1]["exit_code"] == 1 + + +def test_agent_missing_extra_remedy_goes_to_stderr(tmp_path, capsys, monkeypatch): + """Through the CLI entry point: the install line on stderr, stdout events only, exit 1.""" + from fluid_build.cli import forge as forge_mod + from fluid_build.cli import main as cli_main + + monkeypatch.setattr(forge_mod, "run_ai_copilot_mode", _raise_missing_litellm) + argv = ["forge", "--agent", "--llm-provider", "openai", "-d", str(tmp_path / "product")] + assert cli_main(argv) == 1 + + captured = capsys.readouterr() + events = _json_lines_only(captured.out) + assert events[-1]["event"] == "forge.done" + assert "pip install 'data-product-forge[litellm]'" in captured.err