diff --git a/apps/api/src/cora/agent/build_llm.py b/apps/api/src/cora/agent/build_llm.py index 07c93c1618b..3d7aeac20fb 100644 --- a/apps/api/src/cora/agent/build_llm.py +++ b/apps/api/src/cora/agent/build_llm.py @@ -37,6 +37,8 @@ so a deployment may defer Agent rollout without refusing to boot.) """ +from pydantic import SecretStr + from cora.agent._gpu_metrics import make_gpu_usage_sink from cora.agent.adapters.anthropic_llm import AnthropicLLM from cora.agent.adapters.argo_llm import ArgoLLM @@ -47,6 +49,50 @@ from cora.infrastructure.ports.clock import SystemMonotonicClock +def _anthropic_credential(settings: Settings) -> SecretStr | None: + """The Anthropic API key, or None when it is not configured.""" + return settings.anthropic_api_key + + +def _argo_identity(settings: Settings) -> SecretStr | None: + """The ANL domain identity the Argo gateway authenticates, or None.""" + return settings.argo_username + + +def _local_endpoint(settings: Settings) -> tuple[str, str] | None: + """The (base_url, model) a served local endpoint needs, or None when + either half is missing.""" + if settings.local_llm_base_url is None or settings.local_llm_model is None: + return None + return settings.local_llm_base_url, settings.local_llm_model + + +def llm_provider_configured(settings: Settings) -> bool: + """Whether the SELECTED provider has its own configuration present. + + Ignores `llm_enabled` on purpose; the switch is a separate, + provider-independent gate that both callers below check for + themselves. This is the one place that matches + `settings.llm_provider` against the credential each provider + actually reads, via the same `_anthropic_credential` / + `_argo_identity` / `_local_endpoint` extractors `build_llm` uses to + construct the adapter. `build_llm` and `derive_llm` (in + `cora.api._readiness`) both consult this instead of restating the + match, so they cannot drift on what "configured" means for a given + provider. + + They used to drift: `derive_llm` checked `anthropic_api_key` alone, + so a deployment running the argo or local arm booted its own log + line reporting the LLM as `off`, seconds before serving hundreds of + calls through it. + """ + if settings.llm_provider == "argo": + return _argo_identity(settings) is not None + if settings.llm_provider == "local": + return _local_endpoint(settings) is not None + return _anthropic_credential(settings) is not None + + def build_llm(settings: Settings) -> LLM | None: """Construct the production LLM, or `None` when off or unconfigured. @@ -84,9 +130,10 @@ def build_llm(settings: Settings) -> LLM | None: return _build_argo_llm(settings) if settings.llm_provider == "local": return _build_local_llm(settings) - if settings.anthropic_api_key is None: + credential = _anthropic_credential(settings) + if credential is None: return None - return AnthropicLLM(api_key=settings.anthropic_api_key.get_secret_value()) + return AnthropicLLM(api_key=credential.get_secret_value()) def _build_argo_llm(settings: Settings) -> LLM | None: @@ -96,12 +143,10 @@ def _build_argo_llm(settings: Settings) -> LLM | None: API key, so the absent-identity case looks the same as the absent-key case and returns None the same way. """ - if settings.argo_username is None: + identity = _argo_identity(settings) + if identity is None: return None - return ArgoLLM( - username=settings.argo_username.get_secret_value(), - base_url=settings.argo_base_url, - ) + return ArgoLLM(username=identity.get_secret_value(), base_url=settings.argo_base_url) def _build_local_llm(settings: Settings) -> LLM | None: @@ -114,13 +159,12 @@ def _build_local_llm(settings: Settings) -> LLM | None: serving engine is stood up out of band; this only needs its base URL and served model name. """ - if settings.local_llm_base_url is None or settings.local_llm_model is None: + endpoint = _local_endpoint(settings) + if endpoint is None: return None + base_url, model = endpoint return LocalLLM( - backend=OpenAICompatibleBackend( - base_url=settings.local_llm_base_url, - model=settings.local_llm_model, - ), + backend=OpenAICompatibleBackend(base_url=base_url, model=model), monotonic_clock=SystemMonotonicClock(), on_measure=make_gpu_usage_sink(settings.local_llm_gpu_usd_per_hour), device_id=settings.local_llm_device_id, @@ -168,4 +212,4 @@ def llm_unwired_reason(settings: Settings) -> str: ) -__all__ = ["build_llm", "llm_unwired_reason"] +__all__ = ["build_llm", "llm_provider_configured", "llm_unwired_reason"] diff --git a/apps/api/src/cora/api/_readiness.py b/apps/api/src/cora/api/_readiness.py index 74f274588cb..7be79e959d3 100644 --- a/apps/api/src/cora/api/_readiness.py +++ b/apps/api/src/cora/api/_readiness.py @@ -49,6 +49,8 @@ import asyncpg +from cora.agent.build_llm import llm_provider_configured + if TYPE_CHECKING: from cora.infrastructure.config import Settings from cora.infrastructure.schema_version import SchemaPosture @@ -59,15 +61,18 @@ nothing to report rather than nothing wrong.""" LlmReach = Literal["off", "live"] -"""Whether this deployment calls an external language model. +"""Whether this deployment calls a language model on any serving route. -`live` means both the switch (`llm_enabled`) and the credential -(`anthropic_api_key`) are present, so the LLM-backed subscribers are -registered and will call out on every terminal Run. `off` means no -external model is called through this seam. +`live` means both the switch (`llm_enabled`) and the SELECTED provider's +own configuration (`anthropic_api_key`, `argo_username`, or the local +base URL and model, matched by `llm_provider_configured`) are present, +so the LLM-backed subscribers are registered and will call out on every +terminal Run. `off` means no model is called through this seam by any +provider. Named `llm`, NOT `egress`, and the distinction is the honest part. This -reports ONE outbound path. It is not a claim that nothing leaves the +reports ONE outbound path, and for the `local` provider it is not even +outbound past the facility. It is not a claim that nothing leaves the deployment: `HttpRangeChecksumAdapter` is wired unconditionally for http/https Distributions (`cora.data.wire`), so CORA can make outbound requests with the LLM entirely off. Calling this field `egress` would @@ -213,19 +218,29 @@ def derive_actuation(settings: Settings) -> ActuationReach: def derive_llm(settings: Settings) -> LlmReach: - """Report whether an external language model gets called. - - `live` requires BOTH the switch and the credential, mirroring - `build_llm`'s two guards, so this answers the question an operator - actually has ("is CORA phoning out and spending?") rather than - restating one flag. A deployment that sets `llm_enabled` and forgets - the key reads `off`, which is the truth: nothing is called. + """Report whether a language model gets called, on whichever + provider is selected. + + `live` requires BOTH the switch and `llm_provider_configured`, + mirroring `build_llm`'s two guards by calling the SAME predicate + rather than restating the per-provider match, so this answers the + question an operator actually has ("is CORA running a model and + spending?") for whichever of the three providers is selected, not + only `anthropic`. A deployment that sets `llm_enabled` and forgets + the credential its selected provider needs reads `off`, which is the + truth: nothing is called. Before this shared predicate existed, this + function checked `anthropic_api_key` alone, so an `argo` or `local` + deployment reported `off` at boot while serving every call. Like `derive_actuation` this is a REPORT, not a gate. It decides - nothing; `build_llm` is the thing that refuses. And it is scoped to - the LLM seam alone, not to egress in general (see `LlmReach`). + nothing; `build_llm` is the thing that refuses. It calls + `llm_provider_configured` rather than `build_llm` itself: `build_llm` + constructs a live adapter with real credentials and, for `local`, an + HTTP client, and a probe endpoint must not do that on every read of + `/readyz`. And it is scoped to the LLM seam alone, not to egress in + general (see `LlmReach`). """ - return "live" if settings.llm_enabled and settings.anthropic_api_key is not None else "off" + return "live" if settings.llm_enabled and llm_provider_configured(settings) else "off" def readiness_body( diff --git a/apps/api/tests/unit/api/test_readiness.py b/apps/api/tests/unit/api/test_readiness.py index 39c724f13d2..fa74c6181ac 100644 --- a/apps/api/tests/unit/api/test_readiness.py +++ b/apps/api/tests/unit/api/test_readiness.py @@ -19,6 +19,7 @@ import pytest from pydantic import SecretStr +from cora.agent.build_llm import build_llm from cora.api._readiness import ( derive_actuation, derive_llm, @@ -305,3 +306,115 @@ def test_readiness_body_never_carries_the_api_key() -> None: """The body is unauthenticated; a credential must not ride in it.""" body = readiness_body("ok", _llm_settings(enabled=True, key="sk-ant-secret-VALUE")) assert "sk-ant-secret-VALUE" not in str(body) + + +@pytest.mark.unit +def test_derive_llm_reports_live_for_argo_when_configured() -> None: + settings = Settings( # type: ignore[call-arg] + app_env="test", + llm_enabled=True, + llm_provider="argo", + argo_username=SecretStr("svcbeamline"), + ) + assert derive_llm(settings) == "live" + + +@pytest.mark.unit +def test_derive_llm_reports_off_for_argo_without_a_username() -> None: + settings = Settings( # type: ignore[call-arg] + app_env="test", llm_enabled=True, llm_provider="argo", argo_username=None + ) + assert derive_llm(settings) == "off" + + +@pytest.mark.unit +def test_derive_llm_reports_live_for_local_when_configured() -> None: + settings = Settings( # type: ignore[call-arg] + app_env="test", + llm_enabled=True, + llm_provider="local", + local_llm_base_url="http://gpu-host:8000", + local_llm_model="llama-3.3-70b", + ) + assert derive_llm(settings) == "live" + + +@pytest.mark.unit +def test_derive_llm_reports_off_for_local_without_an_endpoint() -> None: + settings = Settings( # type: ignore[call-arg] + app_env="test", + llm_enabled=True, + llm_provider="local", + local_llm_base_url=None, + local_llm_model=None, + ) + assert derive_llm(settings) == "off" + + +@pytest.mark.unit +def test_derive_llm_ignores_an_absent_anthropic_key_when_argo_is_selected() -> None: + """The bug this whole change fixes: derive_llm used to check only + `anthropic_api_key`, so a deployment running the argo arm with no + Anthropic key configured (the normal case) read `off` at boot while + serving every call through the gateway.""" + settings = Settings( # type: ignore[call-arg] + app_env="test", + llm_enabled=True, + llm_provider="argo", + anthropic_api_key=None, + argo_username=SecretStr("svcbeamline"), + ) + assert derive_llm(settings) == "live" + + +def _provider_matrix() -> list[tuple[str, dict[str, object]]]: + """One entry per (provider, credential-shape) combination that + `llm_provider_configured` must classify; `enabled` is applied on top + of each by the differential test below.""" + return [ + ( + "anthropic-with-key", + {"llm_provider": "anthropic", "anthropic_api_key": SecretStr("sk-test-fake")}, + ), + ("anthropic-without-key", {"llm_provider": "anthropic", "anthropic_api_key": None}), + ("argo-with-username", {"llm_provider": "argo", "argo_username": SecretStr("svcbeamline")}), + ("argo-without-username", {"llm_provider": "argo", "argo_username": None}), + ( + "local-fully-configured", + { + "llm_provider": "local", + "local_llm_base_url": "http://gpu-host:8000", + "local_llm_model": "llama-3.3-70b", + }, + ), + ( + "local-missing-model", + { + "llm_provider": "local", + "local_llm_base_url": "http://gpu-host:8000", + "local_llm_model": None, + }, + ), + ( + "local-unconfigured", + {"llm_provider": "local", "local_llm_base_url": None, "local_llm_model": None}, + ), + ] + + +@pytest.mark.unit +@pytest.mark.parametrize( + "label,provider_kwargs", _provider_matrix(), ids=[m[0] for m in _provider_matrix()] +) +@pytest.mark.parametrize("enabled", [True, False], ids=["enabled", "disabled"]) +def test_build_llm_and_derive_llm_agree_on_every_configuration( + label: str, provider_kwargs: dict[str, object], enabled: bool +) -> None: + """The differential invariant this fix exists to hold: whatever + `build_llm` actually constructs, `derive_llm` must report `live`, and + nothing else. A predicate that merely looks correct can still drift + from `build_llm`; comparing the two outcomes directly is the only + thing that would catch that drift.""" + del label + settings = Settings(app_env="test", llm_enabled=enabled, **provider_kwargs) # type: ignore[arg-type] + assert (build_llm(settings) is not None) == (derive_llm(settings) == "live")