refactor(config)!: consolidate LLM credential resolution and remove the vendor endpoint - #26
Merged
Merged
Conversation
…oint conf/default.yml shipped a MITRE gateway hostname as the default api_base for both the llm and cti profiles. Both now ship empty and name MCP_LLM_API_BASE through a new api_base_env key, mirroring the existing api_key_env pattern, so no deployment's endpoint is baked into the repo. An unresolved api_base is now refused rather than passed through. dspy_lm_kwargs_from_settings drops a falsy api_base entirely, which leaves LiteLLM routing openai/* models to its built-in https://api.openai.com/v1, so a deployment that missed the env var would have quietly sent CTI prompts to a provider it never chose. resolve_llm_config now raises, matching the api_key check beside it and the identical check get_llm_provenance already performs on the cti path. Whitespace is stripped before normalization because normalize_openai_api_base treats a blank but truthy string as a real URL and would turn it into the relative path "/v1". Also removes the hostname from two tests and one code comment. BREAKING CHANGE: deployments must set MCP_LLM_API_BASE in plugins/mcp/.env or pin llm.api_base in conf/local.yml. Runs raise ValueError otherwise.
llm_defaults() was welded to the `llm` block while get_llm_provenance() took a profile argument, so the two paths each grew their own copy of the api_key_env and api_base_env lookups. LLMClient carried a third, with a comment noting it existed only because that path bypassed config.py. resolve_env_indirection() in llm_client.py is now the single implementation. It reads the *_env indirection for every field named in ENV_INDIRECT_FIELDS, consumes those keys so they never reach the settings dict handed to DSPy, strips values before normalization, and coerces provider with `or` rather than setdefault so an explicit yaml null no longer survives into the callers that branch on it. config.py gains profile_defaults(profile) and keeps llm_defaults() as a thin alias, so the cti profile resolves through exactly the same code as the llm profile. The api_key fallback in _openai_compatible_generate is removed: llm_cfg now arrives resolved, so the workaround it documented no longer applies.
The guard sat inside `if provider == 'openai_compatible'`, so a request body naming any other provider skipped it while model stayed openai/gpt-oss-120b. dspy_lm_kwargs_from_settings then dropped the empty api_base and LiteLLM routed the call, and the deployment's key, to its built-in https://api.openai.com/v1. provider reaches resolve_llm_config straight from the /execute request body with no whitelist, and conf/default.yml locks only model and api_base, so a lm_config of {"provider": "ollama"} was enough to bypass the check. Only the normalization is provider-specific; the requirement is not, and the ollama path needs a base to post to just as much. Normalizes provider before branching as well, so an explicit yaml null no longer reads as "not openai_compatible".
The override filter tested `value not in ("", None)` with no strip, so a
field containing only spaces counted as a real override. An api_base of
" " is truthy, normalize_openai_api_base turns it into the relative
path "/v1", and that satisfies the api_base guard, so the run failed
deep inside the HTTP client instead of at the resolver with an
actionable message. A pasted URL with a trailing space had the same
shape and became "https://host/v1 /v1".
Both modelSelector.vue and the chat sidebar bind these inputs with a
bare v-model, so nothing trims them client side either. The yaml and env
tiers were already stripped in resolve_env_indirection; this makes the
stated invariant true on all three.
All four dspy.LM() constructions in the plugin take their kwargs from
dspy_lm_kwargs_from_settings, which makes it the only place that can
guarantee the fallback never happens:
dspy_env.py:132 plan_execute.py:60
workflows/author.py:248 utilities/llm_client.py:172
resolve_llm_config guards the /execute path, but plan_execute and author
fall back to llm_defaults() when no lm_obj is passed, and llm_defaults()
returns an empty api_base by design. That was safe while yaml always
carried a base and is not any more. plan_execute then forwards the empty
DSPY_API_BASE to every spawned MCP subprocess, which repeats it.
Dropping the falsy api_base was what let LiteLLM apply its own default,
so this checks before assembling the kwargs rather than after. The
`and settings.get("api_base")` on custom_llm_provider goes away with it,
since a base is now guaranteed to be present.
llm_configured came from the api_key alone, which was a complete precondition until api_base moved out of the shipped yaml. Pull this branch onto a deployment whose .env carries only MCP_LLM_API_KEY and the splash page shows a green "configured" tag, omits the empty base rather than showing it blank, suppresses the warning banner, and then every run fails in the resolver. The suppressed banner also claimed that setting MCP_LLM_API_KEY was sufficient, which is no longer true. The page now reports the specific env vars that failed to resolve, so the diagnostic names the thing the operator has to fix instead of a fixed string. This is the one page whose stated job is telling the operator whether the plugin is wired up.
Nothing in the suite imported app/config.py, so both guards could be deleted with a byte-identical test result. These 26 cases pin the three merge tiers, the shared env indirection, and the two refusals. Mutation checked rather than assumed. Re-nesting the api_base guard under `if provider == "openai_compatible"` fails the three test_api_base_required_for_every_provider cases; disabling the dspy_env guard fails all four TestDspyLmKwargs refusal cases; restoring either makes them pass again. _load_defaults is stubbed via a fixture so the tests read neither conf/default.yml nor the developer's .env.
The env var is required now but appeared only in .env.example. Install step 5 still said "configure model credentials through the UI or environment/local config" without naming either variable, so a new operator had no way to reach a working install from the README alone. The local.yml advice was worse than absent. load_config returns local.yml INSTEAD of default.yml with no merge, so an operator following the old "pin llm.api_base in conf/local.yml" wrote a two-line file, lost api_key_env with it, and was then told to set MCP_LLM_API_KEY, which they had already set correctly. Both the yaml comment and the README now say to copy the whole file and spell out that local.yml replaces rather than merges. The README sample also nested the llm block under a top-level `mcp:` key that nothing reads, and omitted the *_env keys entirely. Corrected to the shape config.py actually loads.
Several comments added on this branch restated what the code already says or carried three lines of history where one would do. Trimmed to the reason a reader cannot infer from the line below it. No behaviour change; tests unchanged and still passing.
Consolidating both fields under one precedence inverted api_key. On main a named api_key_env always won; the shared resolver let a yaml literal shadow it, so a key written to conf/local.yml silently pinned itself and rotating MCP_LLM_API_KEY did nothing. Secrets now resolve env-first, endpoints stay yaml-first. Verified against main's semantics across all four cases. Also covers the yaml-side strip, which no test reached.
The Save button posts api_key with every save and set_config wrote the payload verbatim, leaving the key in plaintext on disk beside tracked config. Secrets are stripped from every section on write, so a save also clears a key an earlier build left behind. Reload now goes through reload_config: load_config is lru_cached, so the old call read back the pre-write contents.
globalConfig and every saved endpoint profile were serialized to localStorage verbatim, so the key sat in plaintext readable by anything on the origin. Stripping at the storage boundary covers both, and load purges a key an earlier build already wrote. The Save payload no longer sends api_key either, since set_config refuses to persist it.
get_llm_provenance was the last resolver still gating its api_base raise on openai_compatible, contradicting the invariant the other two now hold and test for. author.run() prechecked only api_key, so a partial lm_obj failed at dspy.LM() after the AsyncExitStack had already spawned every MCP subprocess. Both credentials are now checked together, before that. Adds provenance guard tests and the first coverage of the readiness payload, including that neither leaks the key.
It still described two credentials and claimed the UI never writes to disk. Now names MCP_LLM_API_BASE, records that set_config strips secrets from conf/local.yml, and states the per-field precedence.
author.py and plan_execute.py called set_tracking_uri, set_experiment and dspy.autolog at module scope. set_experiment is a network round trip, so importing either module blocked whenever the tracking server was down, which put both workflows out of reach of a test, a linter, or anything running before MLflow comes up. llm_client.init_mlflow already documents the rule these two broke. Both now initialise lazily on the first run() call, which makes the author credential precheck testable for the first time.
globalConfig.apiBase is restored from localStorage, and applyServerDefaults only fills it when blank, so a stored value survives forever and ships as a UI override on every /execute, where it outranks the env-resolved base. A browser that cached the old vendor endpoint kept reaching it after the URL was removed from the repo, and editing .env did nothing for that user. The stored blob now carries a schema version, and loading a pre-v2 one drops the ambient apiBase so the server default applies again. Named endpoint profiles keep theirs: those are explicit user artifacts.
The first pass walked only the top level of each section. set_config
accepts arbitrary top-level keys, and a caller posting the documented
{"config": {...}} envelope produces {"config": {"cti": {api_key: ...}}},
which the one-level scrub walked straight past. Confirmed against a real
local.yml that still held a key after a save.
Now recursive through dicts and lists.
Deferring the whole MLflow block was too broad. mlflow.dspy.autolog calls dspy.settings.configure, which pins ownership to the first asyncio task that reaches it, and mcp_svc runs each /execute in its own task. So the first workflow claimed the config and the second raised "can only be called from the same async task", permanently, since the ready flag never latched. At module scope neither call sat inside a task. set_tracking_uri is a local assignment (0.01s against a dead host) and is eager again too. Only set_experiment does I/O: it took 247s to fail with the server down, which is what blocked imports.
The experiment used to exist by the time execute() looked it up, because importing the workflow module created it. Deferring set_experiment moved creation to the first run(), which is after this lookup, so a fresh store filed runs under Default. On an author-only deployment nothing ever created caldera-mcp-client-1 and every run stayed there. create_experiment rather than set_experiment: the latter would mutate the process-wide active experiment this block deliberately avoids.
Both filters listed field names and both had fallen behind the UI. The browser blob persists capabilitySettings, so embed_api_key, plan_api_key and cti_rag_api_key survived in localStorage while only the chat and profile keys were stripped. The server list missed plan_api_key and cti_rag_api_key the same way. Both now match /api_?key/i at any depth. api_key_env is kept: it names a variable, not a value, and dropping it would disable .env resolution.
local.yml replaced default.yml wholesale, so any partial file dropped the api_key_env and api_base_env keys and silently disabled .env resolution. The UI's Save writes exactly such a file, and scrubbing api_key out of it turned that from "key stored in plaintext" into "no key at all": the resolver reported No LLM API key while MCP_LLM_API_KEY was set correctly. Merged key by key now, so local.yml only needs what it changes. Also makes a malformed local.yml harmless, since the default sections survive. Docs and the api_base error message updated: they told operators to copy the whole file, which is no longer needed.
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.
Description
Consolidates LLM credential resolution into one implementation, and removes the vendor gateway hostname from the repo.
Before:
config.py::llm_defaultswas hardcoded to the yamlllmblock whilellm_client.py::get_llm_provenancetook a profile argument. Each had its own copy of theapi_key_envlookup, andLLMClient._openai_compatible_generatecarried a third with a comment saying it existed only because that path bypassedconfig.py.After: one resolver,
llm_client.resolve_env_indirection, with three call sites.config.pyexposesprofile_defaults(profile)and keepsllm_defaults()as a thin alias, so thectiprofile resolves through exactly the same code asllm.Resolution order
conf/default.yml, overlaid byconf/local.yml.envvia the*_envkeysfields_lockedSecrets and endpoints deliberately differ at tier 2.
api_keyresolves env first, so rotating.envtakes effect without editing a tracked file.api_baseresolves yaml first, so a deployment can pin an endpoint on disk. Both are declared inENV_INDIRECT_FIELDS.Refusing to guess an endpoint
conf/default.ymlshippedapi_base: https://models.k8s.aip.mitre.org/v1for both profiles. Both now ship empty and nameMCP_LLM_API_BASE.An unresolved
api_baseis refused rather than passed through.dspy_lm_kwargs_from_settingsdropped a falsyapi_baseentirely, which left LiteLLM routingopenai/*models to its built inhttps://api.openai.com/v1, sending the prompt and the deployment's key to a provider nobody chose. The guard sits atdspy_lm_kwargs_from_settings, which is the one place all fourdspy.LM()construction sites pass through, with a friendlier check earlier inresolve_llm_config.No credential in plaintext
Three surfaces closed:
conf/local.yml:set_configpostedapi_keyverbatim. Secrets are now stripped from the whole file on every write, so a save also scrubs a key an earlier build left behind.localStorage:globalConfigand every saved endpoint profile were serialized as is. Stripped at the storage boundary, and load purges what is already stored./api_?key/iby name at any depth rather than listing fields, because the enumerated lists had already fallen behind the UI and were missingembed_api_key,plan_api_keyandcti_rag_api_key.api_key_envis kept: it names a variable, not a value.Also fixed, found while verifying the above
conf/local.ymlreplacedconf/default.ymlwholesale, so any partial file dropped the*_envwiring and silently disabled.envresolution. It is now overlaid key by key. This mattered once secrets were stripped: a UI save produced a file with neither a key nor the wiring to find one.localStorageapiBasecached before the vendor URL was removed outrankedMCP_LLM_API_BASEon every request. The stored blob is now versioned and a pre v2 blob drops the ambientapiBase. Named endpoint profiles keep theirs, since those are explicit user artifacts.configuredfromapi_keyalone, which stopped being a complete precondition. It now names the specific variables that failed to resolve.author.pyandplan_execute.pycalledmlflow.set_experimentat module scope. That is a network round trip, so importing either module blocked whenever the tracking server was down, anddiscover_workflowsswallowed the exception and dropped both workflows from the registry. Onlyset_experimentis deferred:dspy.autologstays eager because it pinsdspy.settingsto the first asyncio task that reaches it, and each/executeruns in its own task.Type of change
Upgrade note
Existing deployments must add
MCP_LLM_API_BASEtoplugins/mcp/.env..env.examplenever carried that line, so every existing.envhas onlyMCP_LLM_API_KEYand the first run after this change will fail with an actionable error naming the variable.How Has This Been Tested?
75 passedacrosstest_config.py,test_set_config_secrets.py,test_author_guards.pyandtest_llm_client.py. Three of those files are new (49 cases).Every guard was mutation tested rather than assumed. Reverting each fix fails exactly the tests that should catch it:
openai_compatibletest_api_base_required_for_every_providercasesdspy_envguardTestDspyLmKwargsrefusal casestest_strips_whitespace_from_yamltest_strips_every_key_field_the_ui_usestest_strips_nested_secrets,test_strips_secrets_inside_listsload_configTestLocalYmlMergecasesdspy.autologagaintest_both_workflows_configure_from_separate_tasksResolution was also traced end to end against a live endpoint. One
.envpair in, identical values out of all four consumers:flake8 with the repo config shows no new violations: every changed file matches its violation count on
main, and the three new test files are clean.flake8 . --select=E9,F63,F7, the CI hard gate, passes.Pre-existing test failures, not caused by this branch
Reviewers running the whole directory will hit these on
maintoo:test_relation_extractor.py:19importscti_relation_extractor, which does not exist (the module iscti_relationships.py), aborting collection for the directory.test_range_integration.pyneedsplugins/range/mcp_server.py, which is not vendored here.test_model_config.py::TestConfigStructure::test_cti_has_toggle_fieldsrequires astreamfield that thectiblock has never had.test_model_config.pyalso POSTs{"config": {...}}at a live server, andset_configstores arbitrary top level keys verbatim, so running it corruptsconf/local.yml. Harmless now thatlocal.ymlis overlaid rather than substituted, but worth its own fix.Checklist: