fix(deepseek): fail-loud on missing API key + on-disk key fallback (closes deployment gap from #37)#38
Merged
Conversation
…erge gap) Bug discovered immediately after merging PR #37: the daemon was launching fine with no TRIUMVIRATE_DEEPSEEK_API_KEY in its environment, then sending `Authorization: Bearer ` (empty) to api.deepseek.com on every consult. The API returned HTTP 401, which our runner classified as HardProvider(401) and dropped to fallback — a misleading error that looked like a stale-key problem instead of a missing-key problem. The pre-fix doc on from_env() acknowledged this: 'An ABSENT or empty TRIUMVIRATE_DEEPSEEK_API_KEY is NOT an error here — the runner is responsible for noticing an empty key and failing loud at request time.' The runner never did. Production-impact: every consult through a non-env-configured daemon failed with no diagnostic naming the actual problem. ALSO: the env-only design required the parent shell of every MCP-shim launch chain to have the variable exported. Since Claude Code launches MCP shims from its own shell context (not the operator's interactive shell), `export TRIUMVIRATE_DEEPSEEK_API_KEY=...` in ~/.zshrc didn't propagate to running daemons — operator had to restart Claude Code itself to pick up the variable. Two-layer fix: LAYER 1 — fail loud at config load. ConfigError::MissingApiKey { searched: Vec<String> } new variant. from_env() returns this when no key is found, with the list of sources it checked so the operator sees EXACTLY where to put it. The runtime OnceLock in agent_exec caches this Err, so the misleading 401 path is gone — operator sees the typed error on first DeepSeek consult. LAYER 2 — on-disk key fallback at $TRIUMVIRATE_HOME/deepseek.key. load_api_key() checks env first, then the file. Same pattern as ~/.triumvirate/daemon.token. Operator sets the key ONCE on disk (mode 0600 recommended; loose perms warn but still read), and EVERY future daemon process picks it up regardless of who launched it. No more 'export then restart Claude Code' dance. Resolution order: env > file > error. Env wins when both are set so tests and CI can override. PERMISSIONS check (Unix only): loose mode (group/other can read) emits a tracing::warn! suggesting `chmod 600` but the daemon still reads the file. Fail-soft — operators have reasons; logging the risk is the right balance. TEST ISOLATION: `with_clean_env` in the test module now ALSO points TRIUMVIRATE_HOME at a fresh tempdir for the duration of `f`. Without this, a developer with a real ~/.triumvirate/deepseek.key on their machine would see tests silently pass via file fallback when they thought they were testing the 'no key' path. NEW TESTS (5): - from_env_without_key_returns_missing_api_key_error Bug regression. No env, no file → typed MissingApiKey with both sources in the searched list. - from_env_reads_api_key_from_file_when_env_var_absent File-fallback works. Locks the file to 0600 to also exercise the happy path of the permissions check. - from_env_prefers_env_var_over_file_when_both_present Convention: env > file. Tests override file values when set. - from_env_empty_file_falls_through_to_missing_api_key A file with only whitespace doesn't satisfy. Avoids 'I have the file, why doesn't it work?' confusion. - missing_api_key_error_lists_both_searched_sources Display message asserts both 'TRIUMVIRATE_DEEPSEEK_API_KEY' and 'deepseek.key' appear. A stub that just said 'missing key' fails. Runbook §1.3 updated with the recommended setup procedure (file fallback as the primary path, env var for tests/CI overrides only). Verification: cargo test -p mcp-bridge --lib deepseek_config: 16/16 (was 11; +5) cargo test -p mcp-bridge --lib: 105/105 cargo clippy --workspace --exclude pantheon -- -D warnings: clean No new crate deps. tempfile was already a dev-dep on mcp-bridge from T-009. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (live MCP test caught it)
Second wire-shape bug in the same family as B.9, surfaced immediately after
fixing the API-key gap. The first ask_agent({agent:'deepseek'}) through the
live MCP bridge with deepseek_thinking='disabled' returned HTTP 400 with:
"thinking options type cannot be disabled when reasoning_effort is set"
build_request_body always included reasoning_effort in the payload. When
thinking is enabled the API accepts it; when thinking is disabled the API
rejects the combination as mutually exclusive.
Why no test caught it before:
- Probe-08 sent thinking=disabled with NO reasoning_effort field at all
(which is why it passed); the contract probes never exercised the
disabled+effort combination.
- Every OTHER probe sent thinking=enabled, so the combo was incidentally
legal.
- Mock-server unit tests don't validate request shape against API rules.
- PROBE-09 (end-to-end runner integration probe added with B.9 fix) used
thinking=enabled by default — never exercised disabled+effort.
Fix in build_request_body: conditionally include reasoning_effort ONLY
when cfg.thinking == ThinkingMode::Enabled. Both v4-pro and v4-flash
accept this shape; both reject disabled+effort.
NEW TEST (1):
- b9b_reasoning_effort_omitted_when_thinking_disabled
Asserts body["reasoning_effort"] is present when thinking=enabled,
and the body's serialized form contains NO "reasoning_effort" key
when thinking=disabled (string-grep guard against partial reverts).
Live confirmation against api.deepseek.com (with the corrected request shapes):
Flash + thinking=enabled + reasoning_effort=high: OK, content='ok', 27 completion tokens
Flash + thinking=disabled (no reasoning_effort): OK, content='ok', 0 reasoning chars
Verification:
cargo test -p mcp-bridge --lib: 106/106 (was 105; +1)
cargo clippy --workspace --exclude pantheon -- -D warnings: clean
Binary rebuilt:
pre-B.9b: 01e072107b7e25291cbf326a31f4093b7dbb8475
post: ed4445a2522452056e038ac4ad729d4e6c5b16b1
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
The bug: PR #37 merged with a configuration gap — the daemon was launching successfully with no
TRIUMVIRATE_DEEPSEEK_API_KEYin its environment, then sendingAuthorization: Bearer(empty Bearer) to api.deepseek.com on every consult. The API correctly returned HTTP 401, which our runner classified asHardProvider(401)and dropped to fallback — surfacing as a misleading "stale-key" error instead of the actual "no key configured" problem.How we discovered it: Immediately after merging #37, the user tried
ask_agent({agent:"deepseek", message:"..."})through the live MCP bridge. Got 502 → fallback → dead-drop ticket saying "deepseek HTTP 401 (hard)". The daemon's env didn't have the key; no shell init had the export; the runner sent an empty Bearer.Why the env-only path was fragile: Claude Code launches MCP shims from its own shell context, not the operator's interactive shell.
export TRIUMVIRATE_DEEPSEEK_API_KEY=...in~/.zshrcdoesn't propagate to running daemons — operator has to restart Claude Code itself. Friction.Two-layer fix
Layer 1 —
ConfigError::MissingApiKey { searched }(fail loud at config load)from_env()no longer accepts an empty/absent key. Returns a typed error naming both searched sources so the operator knows exactly where to put the key.Layer 2 — on-disk key fallback at
$TRIUMVIRATE_HOME/deepseek.keySame pattern as
daemon.token. Operator sets the key ONCE on disk (mode 0600 recommended; loose perms emit a warn but the daemon still reads), and every future daemon picks it up regardless of who launches it.Resolution order: env > file > error. Env wins when both are set so tests and CI can override.
Operator setup (now the recommended path)
That's it. Any future daemon process picks up the key.
Tests (5 new regressions)
from_env_without_key_returns_missing_api_key_error— the original bugfrom_env_reads_api_key_from_file_when_env_var_absent— file fallback worksfrom_env_prefers_env_var_over_file_when_both_present— env wins (CI override)from_env_empty_file_falls_through_to_missing_api_key— whitespace-only file doesn't satisfymissing_api_key_error_lists_both_searched_sources— error message names both pathsPlus a test-isolation fix:
with_clean_envnow pointsTRIUMVIRATE_HOMEat a fresh tempdir so a developer with a real key file on their machine doesn't silently satisfy the "no key" test path.Verification
cargo test -p mcp-bridge --lib deepseek_config: 16/16 (was 11; +5)cargo test -p mcp-bridge --lib: 105/105cargo clippy --workspace --exclude pantheon -- -D warnings: cleanNo new crate deps.
🤖 Generated with Claude Code