Skip to content

Adopt proposal 0122: the extras surface is a container - #294

Merged
chris-colinsky merged 7 commits into
mainfrom
fix/0122-extras-container-reachability
Sep 7, 2026
Merged

Adopt proposal 0122: the extras surface is a container#294
chris-colinsky merged 7 commits into
mainfrom
fix/0122-extras-container-reachability

Conversation

@chris-colinsky

@chris-colinsky chris-colinsky commented Sep 6, 2026

Copy link
Copy Markdown
Member

Implements proposal 0122, accepted at spec v0.117.0. Two halves: a shape change to the runtime configs, and a one-line fix to the Cohere embedding_types gate.

Both were open items in the v0.17.0 batched spec review. We raised them as questions rather than changing anything unilaterally, and the ruling went our way on one and against our reading on the other.

The extras surface is a container

§6 never stated what the extras surface is. Read one way it is a named container on the config record alongside the declared fields; read the other it is undeclared fields on the record itself. We had the second reading: extra="allow", undeclared names landing as attributes.

That reading has a consequence we reported to spec as a defect in the fixtures. A key whose name matches a declared field binds the field rather than landing in extras, so a caller cannot set both at once, and one arm of 0108's managed-field collision rule becomes unreachable. We concluded shipped fixtures pinned an unreachable case. 0122 rules the other way: the arm is real, and the flat reading was the defect.

So RuntimeConfig, EmbeddingRuntimeConfig and RerankRuntimeConfig (and SamplingConfig, which derives from the first) gain an extras mapping field and move to extra="forbid".

Breaking, in the pre-1.0 sense. An undeclared name passed flat now raises:

RuntimeConfig(temperature=0.2, guided_decoding={...})              # was fine, now raises
RuntimeConfig(temperature=0.2, extras={"guided_decoding": {...}})  # the spelling now

Declared fields are unchanged. from_partial still only drops None-valued entries and does not route undeclared names, so there is one spelling rather than two.

The fixtures already agreed with 0122. Their config.extras: sub-block has always been nested; five of our harnesses were flattening it to match our model. They now pass it through, which is what makes the fixture and the code agree about what the fixture always said.

Two fixtures come off the deferred list. llm-provider 075 and retrieval-provider 052 were held because the same-name reject looked unreachable through the real caller path. Both now run, and are mutation-verified against the reject rather than accepted on a green run.

embedding_types is not a vocabulary check

The Cohere gate carried an and t truthiness clause that treated an empty-string element as malformed and dropped the whole list to ["float"]. 0122 tightens §8.4's wording from "not a precision string" to "not a string", because the former reads as a vocabulary check and the general rule it inherits explicitly is not one.

The gate's own comment already said malformation was structural only, so the code and its comment disagreed. Fixture 053 gains a case pinning ["banana", ""]["float", "banana", ""]. The structural arm is unchanged: a non-string element still drops the whole list with no partial salvage.

What the reviews changed

Two adversarial passes and CoPilot ran against this branch. The baseline pass found 8 findings, the delta pass over its fixes found 12 more, and CoPilot found 3. The two worth calling out are regressions this branch introduced and would otherwise have shipped:

A retry silently dropped the caller's vendor knobs. extras is a declared field defaulting to {}, which exclude_none keeps, so _config_for_attempt's generic dump carried an empty container into the merge and replaced the base container on every attempt. Nothing surfaced it: the extras projection runs once against the base config before the retry loop, so the emitted event reported extras the attempt never sent.

extras now follows the same rule the declared fields follow, with one wrinkle. Its default is an empty container rather than None, so an override declaring no extras counts as unspecified and inherits, while one declaring any replaces wholesale. Any base key the override does not carry is logged as not sent on that attempt, since replacing discards it. Clearing extras for a single attempt is not expressible, because empty is how inheriting is spelled. All three doc sites that state the override contract now say so.

A prompt sidecar became a fetch-time crash. With the config rejecting undeclared names, splatting an operator-authored sidecar verbatim raised a bare pydantic error out of fetch(). That is neither PromptNotFound nor PromptStoreUnavailable, so PromptManager's multi-backend fallback never ran and one stray key took down every fetch for that prompt with a second backend sitting idle. Unrecognized keys are now filtered and logged, matching what the token_budget path and the Langfuse backend already did; a malformed value on a recognized key converts to PromptStoreUnavailable so it stays fallback-eligible.

Also from the reviews: a Langfuse prompt.config now lifts an extras sub-object, so the container is honored on both documented sources rather than one; the render-time copy rebuilds stop_sequences as well as extras, since model_copy shares every mutable field; conformance.toml no longer publishes the unreachability claim at the same commit that un-defers the fixtures it named; and the docs for all three config types describe the container, where retrieval.md previously documented extras nowhere.

Testing

13 mutants, 13 killed. Both directions on the Cohere gate; extra="forbid" on all four config classes; the collision reject behind 075 and 052; all four arms of the retry semantics; the three sidecar arms; the Langfuse lift; and both halves of the render-time isolation.

Four of those survived on first run and drove new tests rather than being explained away: the extra="forbid" pair (the container worked, but nothing pinned the single-spelling half), and the render-time isolation pair (fixed across two review rounds, tested in neither).

Ahead of the pin

Spec v0.117.0 is beyond the current v0.112.0 pin, so this ships unit-tested and the conformance.toml entry plus fixtures 054 / 055 / 056 ride the pin bump.

Tracked, not fixed here

Two findings are recorded as follow-ups rather than folded in: the Langfuse conformance harness's extras path has no fixture behind it, and a retry attempt's event reports the base config rather than the attempt's. The second predates this work and is why the extras regression went unseen.

Proposal 0122 tightens retrieval section 8.4: the malformed test on a
merge-extra is structural, never a vocabulary check. A well-typed string
the provider does not recognize merges, including the empty string, and
the provider rejects it if unsupported.

The Cohere gate carried an `and t` truthiness clause that dropped the
whole list to ["float"] on an empty element. Its own comment already
said malformation was structural only, so the code and the comment
disagreed. Fixture 053 gains a case pinning ["banana", ""].

The structural arm is unchanged: a non-string element still drops the
whole list with no partial salvage.

This was raised as a spec question rather than changed unilaterally, and
the ruling went the way the comment described.
Proposal 0122 settles the shape of the extras surface: undeclared fields
live in a container on the config record that is separately addressable
from the declared ones, and its name is normative.

We had the flat reading. RuntimeConfig, EmbeddingRuntimeConfig and
RerankRuntimeConfig accepted undeclared names as attributes on the
record, which meant a key whose name matched a declared field bound the
field instead of landing in extras. That made one arm of 0108's
managed-field collision rule unreachable, and we reported it as such.
0122 rules the other way, so the arm is real and the reading was the
defect.

Breaking in the pre-1.0 sense: an undeclared name passed flat now raises,
and the same call is written with extras={...}. Declared fields are
unchanged. from_partial still only drops None-valued entries and does not
route undeclared names, so there is one spelling rather than two.

The conformance fixtures already nested their config.extras sub-block and
four harnesses were flattening it to match our model. They now pass it
through, which is the change that makes the fixture and the code agree
about what the fixture always said.
Copilot AI lite review requested due to automatic review settings September 6, 2026 21:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

A new unit test does not close its provider (missing await provider.aclose()), and there are small cleanup issues (a leftover type-ignore and an incomplete comment) that should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR adopts accepted spec proposal 0122 by changing runtime configs to treat provider-specific extras as a dedicated extras container (with extra="forbid"), and aligns provider/test/documentation surfaces accordingly. It also fixes the Cohere /v2/embed embedding_types gate to be structural-only (no truthiness or vocabulary checks), matching the tightened spec wording.

Changes:

  • Introduce an extras: dict[str, Any] container on RuntimeConfig, EmbeddingRuntimeConfig, and RerankRuntimeConfig, and update harnesses/tests to pass nested extras through instead of flattening.
  • Update LLM and retrieval provider mappings to read request extras from config.extras (not model_extra) and adjust managed-extra collision tests for same-name collisions.
  • Fix Cohere embedding_types validation to accept any strings (including "") and add unit coverage for the empty-string merge case.
File summaries
File Description
tests/unit/test_structured_output.py Updates structured-output test to pass response_format via RuntimeConfig.extras.
tests/unit/test_retrieval_provider.py Migrates retrieval config construction to nested extras and adds a unit test for Cohere embedding_types empty-string merge behavior.
tests/unit/test_prompts.py Updates prompt sidecar assertions to read vendor knobs from SamplingConfig.extras.
tests/unit/test_llm_provider.py Updates LLM unit tests to use the extras container and adds coverage ensuring undeclared fields must be nested under extras.
tests/conformance/test_retrieval_provider.py Stops flattening config.extras into top-level config kwargs for conformance embedding and rerank configs.
tests/conformance/test_prompt_management.py Updates prompt-management conformance adapter to treat sampling.extras as a nested container and compare dumps without flattening.
tests/conformance/test_observability.py Builds RuntimeConfig by passing provider-specific request params through the extras container.
tests/conformance/test_llm_provider.py Stops flattening fixture config.extras into RuntimeConfig declared fields.
src/openarmature/retrieval/response.py Changes embedding/rerank runtime configs to extra="forbid" and adds an explicit extras container.
src/openarmature/retrieval/providers/tei.py Reads provider request extras from config.extras.
src/openarmature/retrieval/providers/openai.py Reads provider request extras from config.extras.
src/openarmature/retrieval/providers/jina.py Reads provider request extras from config.extras.
src/openarmature/retrieval/providers/cohere.py Reads provider request extras from config.extras and fixes embedding_types structural gating to allow empty strings.
src/openarmature/prompts/backends/filesystem.py Parses sampling sidecar dict into SamplingConfig with a nested extras container.
src/openarmature/llm/response.py Changes RuntimeConfig to extra="forbid" and adds an explicit extras container plus updated from_partial semantics documentation.
src/openarmature/llm/providers/openai.py Reads request extras from config.extras when building OpenAI request bodies.
docs/concepts/llms.md Updates documentation examples to use RuntimeConfig(..., extras={...}).
CHANGELOG.md Documents the breaking pre-1.0 surface change to nested extras and the Cohere embedding_types clarification.
Review details
  • Files reviewed: 18/18 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/unit/test_retrieval_provider.py Outdated
Comment thread tests/conformance/test_prompt_management.py Outdated
Comment thread tests/unit/test_structured_output.py Outdated
llm-provider 075 and retrieval-provider 052 were held because the coded
reject looked unreachable: a declared-name key bound the declared field
instead of landing in extras, so nothing could construct the collision.
The container makes it constructible, and both fixtures pass.

Mutation-verified rather than taken on a green run: making the reject
arm never fire turns both red.

Also corrects the comments that asserted the unreachability, and two
module comments still describing the configs as extra="allow".
Two were real defects this branch introduced.

A retry with a per-attempt override silently dropped the base config's
extras. `extras` is a declared field defaulting to an empty dict, which
exclude_none keeps, so the generic dump carried an empty container into
the merge and replaced the caller's vendor knobs on every attempt. It was
invisible in the trace too: request_params is projected once from the
base config before the retry loop, so the emitted event reported extras
the attempt never sent. The merge is now per key, with the override
winning on a collision.

A filesystem sidecar carrying an unrecognized top-level key raised a
pydantic error out of fetch(). That is neither of the two documented
error types, so PromptManager's multi-backend fallback never ran and one
stray key in one operator-authored file took down every fetch for that
prompt. Unrecognized keys are now filtered, matching what the
token_budget path and the Langfuse backend already did.

A Langfuse prompt.config now lifts an extras sub-object as well, so the
container is honored on both documented sources rather than one.

Also: a fifth conformance harness was still flattening the fixture's
extras block, PromptManager's defensive copy shared the container by
reference, and an assertion presented as covering the None-dropping was
a tautology under the new strictness.

Comment and docs corrections, including two module comments still
describing the configs as extra="allow", a comment mangled into a
half-sentence, and one I wrote narrating a mutation result.
The new cohere test left its provider open, so a failing assertion
mid-test leaked the transport. Wrapped in try/finally, verified by
forcing the assertion red and confirming no unclosed-transport warning.

The type ignore on a RuntimeConfig construction predated the container:
extras is a typed declared field now, so pyright accepts it. The sibling
ignore on RuntimeConfig(top_k=None) stays, since that name is
deliberately undeclared.

Also drops a comment clause describing what the gate used to do.
The per-key merge contradicted the contract stated in three places: the
override's set fields replace, unspecified ones inherit. `extras` now
follows that rule, with the wrinkle that its default is an empty
container rather than None, so empty means unspecified and inherits
while non-empty replaces wholesale. All three sites now say so.

Replacing discards whatever the base carried, so the dropped keys are
logged rather than lost in silence. Clearing extras for one attempt
stays inexpressible, since empty is how inheriting is spelled.

The sidecar fix covered only the unrecognized-key arm. A malformed value
on a recognized key still raised out of fetch() as an undocumented type,
bypassing the manager's fallback; it now converts to
PromptStoreUnavailable like the token_budget path. An ignored key is
logged too: filtering it changes model behavior, and the symptom is a
sampling config that appears to do nothing.

Also: the render-time copy now rebuilds stop_sequences as well as
extras, since model_copy shares every mutable field; conformance.toml no
longer publishes the unreachability claim at the commit that un-defers
the two fixtures it named; and a retrieval doc paragraph said a
same-name override was how you override a modelled wire field when that
call always raises.

Two findings are tracked rather than folded in: the Langfuse harness
extras path has no fixture behind it, and a retry attempt's event
reports the base config rather than the attempt's, which predates this
work and is why the extras regression went unseen.
The copy that rebuilds extras and stop_sequences had no test behind it.
Mutation found both: dropping the dict() around extras and the list()
around stop_sequences each left the whole suite green, so a regression
letting a rendered result reach back into its Prompt would have gone
unnoticed.

The test renders two results from one Prompt, mutates one, and asserts
neither the Prompt nor the sibling sees it.
@chris-colinsky
chris-colinsky merged commit 6b3cce9 into main Sep 7, 2026
6 checks passed
@chris-colinsky
chris-colinsky deleted the fix/0122-extras-container-reachability branch September 7, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants