Stop an API key from being a decision - #581
Merged
Merged
Conversation
Setting ANTHROPIC_API_KEY was enough to register RunDebriefer and CautionDrafter, which call an external model on every terminal Run. So a key present in the environment for some unrelated reason silently turned on experiment metadata leaving the facility and money being spent. Every one of the ~13 sibling subscribers already carried its own default-off flag; this seam was the outlier, and it was the last live hole in the observe-only story. `llm_enabled` is now the switch and defaults off; the key is demoted to what it always was, a credential. Both are required before anything is called. Setting only the key now changes nothing, which is the point. This is a BREAKING config change: a deployment running agents today on key-presence alone must add LLM_ENABLED=true. Gating `build_llm` turns out to cover all four LLM consumers, not the two I went looking for, because `llm=None` was already a supported state everywhere: the key-absent case has always produced it. The subscribers log-and-skip, `regenerate_run_debrief` answers unavailable, and a conduct command that asks for the `llm` decide substrate gets a 422 at construction, before any FSM transition. Nothing degrades into a crash. The skip warning used to name a missing ANTHROPIC_API_KEY unconditionally. With two possible causes calling for opposite remedies, that would now be a lie half the time, so it names whichever actually applies. /readyz gains `llm`, either `off` or `live`, beside `actuation`, with a `boot.llm_posture` line carrying the raw inputs. The field reports the EFFECTIVE state, so a deployment that sets the switch and forgets the key reads `off`, which is the truth. It is deliberately named `llm` and not `egress`: HttpRangeChecksumAdapter is wired unconditionally for http/https Distributions, so CORA can still make outbound requests with the LLM off, and a field called `egress` would promise a perimeter this code does not enforce. Naming it for the one path it actually knows keeps it honest, the same discipline `derive_actuation` follows on its own axis. Egress and spend are a different axis from actuation, and the docs say so rather than letting one promise imply the other. A facility told CORA cannot touch its hardware hears a claim about actuation, and would still object to discovering its data was phoned out. Two stale docstrings fixed on the way through, both describing intent rather than behaviour. build_llm claimed subscriber registration "fail-fasts" on kernel.llm is None: it does not and never did, it logs and skips so a deployment can defer Agent rollout. .env.example claimed the subscribers "register but log-and-skip"; they are not registered at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Gate review took the previous commit at its word and found the standard it set applied in exactly one place. The skip warning learned to name whichever of the two settings actually left the LLM unwired; the REST 503, its OpenAPI description, and the MCP tool error all still blamed ANTHROPIC_API_KEY unconditionally. After this change that is the wrong remedy in the DEFAULT posture, not in some corner case, and those are the surfaces an operator actually reads. The branching explanation now lives once, in `llm_unwired_reason`, and every surface calls it, so they cannot drift apart again. The MCP tool is the exception and says so: its resolver has no Settings in scope and threading one through to word a message would widen the tool-registration signature, so it names both settings instead of deriving which applies. A claim of my own turned out to be false, in four places including both files an operator reads. I wrote that a conduct command asking for the `llm` decide substrate "gets a 422 at construction". It cannot: `WireDecideSubstrate` admits only `in_memory` and `grid_walk`, so no remote caller can select `llm` at all and `build_decide_port`'s guard is an internal-caller guard. Restated as what it is. The commit body that said "two stale docstrings fixed on the way through" was itself incomplete. The exact twin of one of them survived in the `LLMFactory` Protocol, which documents the very contract the lie was about, plus five more sites encoding key-presence-alone as the condition for a live LLM. All corrected. Both operator files now carry the upgrade instruction rather than leaving it in a commit message no operator reads: a deployment running on the key alone must add LLM_ENABLED=true. And .env.example had titled itself "the EGRESS + SPEND axis" and claimed to bound what CORA "SENDS OUT", the exact overclaim deployment.md was careful to avoid, since the HTTP checksum adapter is wired unconditionally. It now carries the same narrowing. Two test consequences, both fixed rather than papered over: the switch-off assertion was too strict (the message may name the credential as part of the REMEDY, it just must not report it as the CAUSE), and the OpenAPI snapshot moved with the 503 description. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Setting
ANTHROPIC_API_KEYwas enough to register RunDebriefer and CautionDrafter, which call an external model on every terminal Run. So a key present in the environment for some unrelated reason silently turned on experiment metadata leaving the facility and money being spent. Every one of the ~13 sibling subscribers already carried its own default-off flag; this seam was the outlier, and it was the last live hole in the observe-only story.llm_enabledis now the switch and defaults off; the key is demoted to what it always was, a credential. Both are required before anything is called. Setting only the key now changes nothing, which is the point.Breaking config change
A deployment running agents today on key-presence alone must add
LLM_ENABLED=true. Nothing crashes: the two subscribers stop registering, a boot warning names the switch, and/readyzreports"llm": "off". The upgrade instruction is in both.env.exampleanddocs/stack/deployment.md, not only here.One seam covers four consumers
Gating
build_llmturns out to cover all four LLM consumers, not the two I went looking for, becausellm=Nonewas already a supported state everywhere: the key-absent case has always produced it. The subscribers log-and-skip,regenerate_run_debriefanswers unavailable, and thellmdecide substrate is not selectable over the wire at all. Verified:AnthropicLLM(is constructed at exactly one site insrcand theanthropicSDK is imported in exactly one file, so the switch is not decorative.Verifiable, and narrowly named
/readyzgainsllm, eitherofforlive, besideactuation, with aboot.llm_postureline carrying the raw inputs (the key as a BOOLEAN presence flag, never the value). It reports the EFFECTIVE state, so a deployment that sets the switch and forgets the key readsoff, which is the truth.It is deliberately named
llmand notegress:HttpRangeChecksumAdapteris wired unconditionally for http/https Distributions, so CORA can still make outbound requests with the LLM off. A field calledegresswould promise a perimeter this code does not enforce. Naming it for the one path it actually knows keeps it honest, the same disciplinederive_actuationfollows on its own axis.Gate review
Three lenses plus adversarial verification; 10 of 11 findings confirmed and fixed in the second commit.
The review's main hit was fair: the first commit set a standard ("name the cause that actually applies") and applied it in one place. The operator-facing 503, its OpenAPI description, and the MCP tool error still blamed the key unconditionally, which is the wrong remedy in the new default posture. The branching explanation now lives once in
llm_unwired_reasonand every surface calls it.It also caught a false claim of mine, repeated in four places including both operator files: that asking for the
llmdecide substrate "gets a 422". It cannot, becauseWireDecideSubstrateadmits onlyin_memoryandgrid_walk. Restated as the internal-caller guard it is.Secret safety was probed independently with a canary key and came back clean: zero occurrences across the captured run,
SecretStrredaction intact, the soleget_secret_value()unwrap unchanged.Verification: 45,619 passed, ruff and pyright clean, every pre-commit hook green.
🤖 Generated with Claude Code