Skip to content

Add embabel-agent-starter-byok so a BYOK app can boot without keys - #1890

Open
jasperblues wants to merge 11 commits into
mainfrom
feat/1888-starter-byok
Open

Add embabel-agent-starter-byok so a BYOK app can boot without keys#1890
jasperblues wants to merge 11 commits into
mainfrom
feat/1888-starter-byok

Conversation

@jasperblues

@jasperblues jasperblues commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What

Adds embabel-agent-starter-byok: model factories without provider autoconfiguration, plus a setup-required placeholder LlmService available under a public constant.

A pure BYOK application can now depend on a single artifact, start with no provider keys configured, and avoid having to provide its own placeholder implementation.

The implementation is split into two modules, following the existing starter/autoconfigure structure:

Module Contents
embabel-agent-starters/embabel-agent-starter-byok platform-autoconfigure, byok-autoconfigure, embabel-agent-anthropic, embabel-agent-openai. No provider autoconfiguration.
embabel-agent-autoconfigure/models/embabel-agent-byok-autoconfigure SetupRequiredLlm (name/provider/message constants + factory), SetupRequiredLlmConfig, AgentByokAutoConfiguration

Opt in with:

embabel:
  models:
    default-llm: setup-required

What Guide can delete

Guide is the BYOK reference implementation, and it already solved both halves in application code. This starter replaces all of it.

Packaging — today: Guide skips the provider starters and depends on the factory libraries directly, because the starters would bring autoconfiguration that requires a key at startup.

<dependency>
    <groupId>com.embabel.agent</groupId>
    <artifactId>embabel-agent-openai</artifactId>
</dependency>
<dependency>
    <groupId>com.embabel.agent</groupId>
    <artifactId>embabel-agent-anthropic</artifactId>
</dependency>

Packaging — now: one artifact, with the exclusion enforced rather than merely intended.

<dependency>
    <groupId>com.embabel.agent</groupId>
    <artifactId>embabel-agent-starter-byok</artifactId>
</dependency>

The placeholder — today: SetupRequiredChatModel.kt is a whole file of framework-shaped code sitting in the application, plus a registration in UserLlmResolver.kt.

class SetupRequiredChatModel : ChatModel {

    override fun call(prompt: Prompt): ChatResponse {
        val message = AssistantMessage("")
        return ChatResponse(listOf(Generation(message)))
    }

    override fun getDefaultOptions(): ChatOptions = ChatOptions.builder().build()

    companion object {
        const val MODEL_NAME = "setup-required"
        const val SETUP_MESSAGE = """..."""
    }
}

@Configuration(proxyBeanMethods = false)
class SetupRequiredLlmConfig {
    @Primary
    @Bean(SetupRequiredChatModel.MODEL_NAME)
    fun setupRequiredLlmService(): LlmService<*> = SpringAiLlmService(
        name = SetupRequiredChatModel.MODEL_NAME,
        provider = "none",
        chatModel = SetupRequiredChatModel(),
    )
}

The placeholder — now: both delete entirely. The starter registers the bean; application.yml already says default-llm: setup-required, and that value is now SetupRequiredLlm.NAME.

UserLlmResolver keeps its injected placeholder, but qualifies it by name instead of relying on @Primary:

@Service
class UserLlmResolver(
    // ...
    @Qualifier(SetupRequiredLlm.NAME) private val setupRequiredService: LlmService<*>,
)

The starter deliberately does not mark the placeholder @Primary — that would hijack LlmService injection in any application that also has real models registered.

Guide's resolve() is unchanged and still works:

return ctx.ai().withLlmService(setupRequiredService)

The difference is what happens if that call is ever reached. Today it returns "" and the user sees an empty answer; with this PR it throws NoLlmConfiguredException, which Guide can catch to show SETUP_MESSAGE — the message it already has, now attached to an error rather than to silence. Guide's hasLlm() short-circuit remains the happy path either way.

What stays in Guide, correctly: UserKeyStore, UserModelFactory, LlmProvider, LlmRole, and how a key reaches a request.

Design notes

Provider autoconfiguration is explicitly excluded. This is important because adding a provider starter transitively would bring back the startup API-key requirement. A bannedDependencies enforcer rule makes this a build failure rather than allowing it to regress unnoticed.

The rule uses combine.self="override" because the parent merges DependencyConvergence into every enforcer execution, and fail=true would otherwise turn the repo-wide convergence warnings into failures.

Verified both ways: adding a provider autoconfigure dependency fails the build; removing it passes.

The placeholder carries a marker, and that marker is what relaxes startup validation. PlaceholderLlmService is an empty interface in com.embabel.agent.spi, so ConfigurableModelProvider can recognise a placeholder without depending on this module - carrying the placeholder without dragging in that dependency is why the module exists. When default-llm resolves to one, the deployment is stating that keys arrive at runtime, and unresolvable model names in embabel.models become expected rather than fatal. Every other deployment keeps failing fast on a name it cannot resolve, because there a name that resolves to nothing is a typo. The embedding-services check is gated the same way, with no fallback: an embedding model is a schema commitment and nothing can stand in for one, so the gate decides only whether the deployment starts.

SetupRequiredLlmService carries the marker by delegating to a SpringAiLlmService, which is a data class and so cannot be extended. Its self-typed methods return a wrapper rather than the delegate, since delegating them would hand back a bare SpringAiLlmService and silently drop the marker.

#1889 changes these same lines for its own reasons and the hunks are byte-identical, verified with git merge-tree - the two merge cleanly in either order.

The placeholder is registered unconditionally, rather than using @ConditionalOnMissingBean(LlmService). The model autoconfigurations register their LlmService instances imperatively with registerSingleton inside a @Bean method, so the condition would be evaluated before those singletons exist and would always see no LlmService.

Instead, the placeholder is only used when explicitly selected through default-llm. If another model is configured as the default, setup-required is never resolved. There is a test covering this.

The placeholder throws rather than returning an empty completion. This differs from Guide's current SetupRequiredChatModel, which returns AssistantMessage("").

A missing API key should produce an actionable error rather than an empty response. Applications can catch NoLlmConfiguredException and show their own "add an API key" UI. Guide can use this and remove its local placeholder implementation.

Happy to change this to an empty completion if compatibility with Guide's current behaviour is preferred.

Non-goal: key storage. The starter provides the factories and placeholder only. Key lifecycle remains the responsibility of the application, as described in the existing BYOK documentation.

Docs

  • Added a Pure BYOK deployments section to reference/customizing/page.adoc, covering:

    • which artifact to depend on
    • why default-llm should point to the placeholder
    • how to start an application without a provider key
  • Added the BYOK starter to the table in modules/page.adoc.

Both were verified in the rendered HTML with:

mvn -P embabel-agent-docs

Testing

All passing:

  • SetupRequiredLlmTest (7)

    • the service reports the name and provider the constants declare, and carries the PlaceholderLlmService marker
    • ConfigurableModelProvider resolves the placeholder as default and by name
    • the placeholder is not selected when another model is the default
    • calling the placeholder throws an actionable error, and streaming fails the same way rather than with an unrelated streaming error
    • the platform still reports it as not supporting streaming
    • a message sender can still be created, so failure occurs at call time rather than setup time
  • PureByokWithRolesTest (4) — boots with best and cheapest configured and nothing to satisfy them; still resolves the placeholder as default; an unsatisfiable role throws when asked for; and a deployment holding a key still dies at startup on a name nothing registers

  • AgentByokAutoConfigurationTest (2)

    • registers with no provider key configured
    • contributes no other LlmService
  • ByokStarterBootTest (3)

    • runs against the starter's own dependency set
    • context starts with no provider key
    • a model provider constructed the same way as the platform resolves the default LLM
  • BlankApiKeyTest (5) — empty, whitespace-only and null keys rejected; a real key passes; surrounding whitespace does not make a real key blank

  • AnthropicModelFactoryBuildValidatedTest (+1) and OpenAiCompatibleModelFactoryBuildValidatedTest (+2) — a blank key throws with BLANK_API_KEY_MESSAGE and reaches the provider zero times; the four ByokSpec entry points (openAi/deepSeek/mistral/gemini) reject one too

Built with -am so embabel-agent-api was built from source rather than using a stale ~/.m2 artifact.

Blank keys count as absent — on the BYOK path only

A key is blank rather than absent more often than it looks. Compose passes ANTHROPIC_API_KEY=${ANTHROPIC_API_KEY:-}, so in a container the variable is routinely set-but-empty, and a caller reading it with a null default (@Value("${VAR:#{null}}"), System.getenv(...)) gets "" rather than null. That empty string passes a null check, reaches the provider, and returns an opaque authentication error a long way from its cause — which is why BYOK callers have each been re-deriving "blank counts as absent" for themselves.

requireUsableApiKey now lives in the embabel-agent-byok module, which both factory libraries already depend on, and throws the InvalidApiKeyException callers already handle for a rejected key. Both buildValidated paths check before the probe.

That a blank key never reaches the wire is asserted by counting requests against a local HttpServer, not merely by the exception type:

val e = assertThrows<InvalidApiKeyException> { blankKeyFactory.buildValidated() }
assertEquals(BLANK_API_KEY_MESSAGE, e.message)
assertEquals(0, requests, "a blank key must not reach the provider")

Scoped deliberately to BYOK. The provider autoconfigurations are untouched and still error() when their key is missing — git diff over both autoconfigure modules is empty. Their key is documented as * Required: for every provider, so autoconfiguration on the classpath is a statement that the deployment has that key, and failing fast without one is that contract working. Relaxing it would weaken a documented requirement for everyone in order to serve a deployment shape that should not have those modules on the classpath at all — which is what this starter is for.

Why not just relax the provider configs

The docs make the contract explicit. getting-started/installing/page.adoc lists the key under * Required: for every provider starter — OPENAI_API_KEY, ANTHROPIC_API_KEY, DEEPSEEK_API_KEY, GEMINI_API_KEY, MISTRAL_API_KEY, OPENAI_CUSTOM_API_KEY — and the BYOK section opens by describing autoconfiguration as "you set one or more API keys… the right approach for a platform-level key shared across all users".

Provider autoconfiguration on the classpath means "this deployment has this provider's key". Failing fast without one is that contract working, not a defect to be smoothed over.

So the answer for a BYOK deployment is not to put provider autoconfiguration on the classpath and teach it to tolerate a missing key — that makes a documented Required setting quietly optional for everyone. The answer is to not have that autoconfiguration at all, which is what this starter packages and what the bannedDependencies rule keeps true.

An earlier revision of this description discussed fix/719-boot-without-model-keys as complementary work. Correcting two things about that branch: it does not touch ConfigurableModelProvider and does not make defaultLlm resolution lazy (No models detected. is unchanged on main, and this PR leaves it alone). What it does change is AnthropicModelsConfig and OpenAiModelsConfig, so a missing key registers no models instead of failing — which is the relaxation this section argues against.

The one part of that work that stands on its own — treating a blank key as absent — is now folded into this PR, scoped to BYOK, as described above. The rest is not needed.

What this PR does and does not add

Being precise about the value, since part of it is already reachable today:

  • Packaging — convenience and enforcement, not new capability. An application can already depend on embabel-agent-openai and embabel-agent-anthropic directly and get the factories without autoconfiguration; Guide does exactly that. This PR makes it one dependency, documents it, and makes the exclusion a build failure rather than a convention someone re-derives.
  • The placeholder — genuinely new. There is no shipped LlmService for the "no key yet" state, so every BYOK application writes its own no-op ChatModel. Nothing in the docs or the embabel-agent-byok module mentions needing one; you find it by reading Guide's source.

A deployment whose provider key arrives per user or per request had no starter it
could depend on. The only starters carrying the BYOK factories also carry the
provider autoconfiguration, which requires a key at construction time — exactly
what such a deployment does not have. Applications worked around this by depending
on the factory libraries directly and hand-rolling a placeholder model, twice
independently, which is the usual sign a starter is missing rather than that
applications are doing something unusual.

Two modules, split along the repo's existing seam: starters aggregate dependencies,
autoconfigure modules carry code.

embabel-agent-starter-byok pulls in the factory artifacts (embabel-agent-anthropic,
embabel-agent-openai) and no provider autoconfiguration. That absence is the whole
point of the artifact and would regress silently — a transitive provider starter
would reintroduce the fail-fast, and would do so only for people without a key in
their environment. A bannedDependencies rule makes it a build failure instead.
It needs combine.self=override because the parent merges DependencyConvergence into
every enforcer execution, and fail=true would make that tolerated warning fatal.

embabel-agent-byok-autoconfigure contributes the setup-required placeholder, so the
platform has something to resolve for embabel.models.default-llm before any key
exists. It is registered unconditionally rather than behind
@ConditionalOnMissingBean(LlmService): the model autoconfigurations register their
models with registerSingleton from inside a @bean method, so the condition would be
evaluated before those singletons exist and would always see none. Opting in through
default-llm avoids the ordering question. An unused placeholder is inert — models are
only ever selected by name or role, so a deployment that does not name it never
resolves it.

The placeholder fails loudly. Calling it throws NoLlmConfiguredException rather than
returning an empty completion, so a missing key surfaces as an actionable error
instead of a silently empty answer; applications catch it to render their own
"add an API key" experience.

Key storage and lifecycle stay with the caller, as the BYOK documentation already
states. The starter stops at the factories and the placeholder.

Closes #1888

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A key is blank rather than absent more often than it looks. Compose passes
ANTHROPIC_API_KEY=${ANTHROPIC_API_KEY:-}, so in a container the variable is
routinely set-but-empty, and a caller reading it with a null default —
@value("${VAR:#{null}}"), System.getenv(...) — gets "" rather than null. That
empty string passes a null check, reaches the provider, and comes back as an
opaque authentication error a long way from its cause. BYOK callers have been
re-deriving "blank counts as absent" for themselves as a result.

requireUsableApiKey lives in the byok module, which both factory libraries
already depend on, and throws the InvalidApiKeyException callers handle for a
rejected key. Both buildValidated paths check before the probe, so a blank key
never reaches the wire — asserted by counting requests against a local server,
not merely by the exception type.

Scoped deliberately to BYOK. The provider autoconfigurations are untouched and
still call error() when their key is missing. Their API key is documented as
Required for every provider (getting-started/installing/page.adoc), so
autoconfiguration on the classpath is a statement that the deployment has that
key, and failing fast without one is that contract working. Making it tolerate
a missing key would relax a documented requirement for everyone in order to
serve a deployment shape that should not have those modules on the classpath at
all — which is what embabel-agent-starter-byok is for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jasperblues added a commit that referenced this pull request Aug 8, 2026
…he Java constructor

Three problems with role resolution as it stood.

ModelSelectionContext is a ThreadLocal, and the platform moves work off the calling
thread itself — AgentPlatform.start, parallel actions, OperationContext.parallelMap.
Losing the context there does not fail: ConfigurableRoleResolver falls back to the
deployment's own provider and serves a model the deployment is billed for, on a call
the user brought their own key for. That is exactly the invariant this feature claims.
ExecutorAsyncer now propagates it alongside AgentProcess, restoring the previous value
so nothing leaks between tasks on a pooled thread.

ConfigurableModelProvider gained two Kotlin default parameters, which Java callers
cannot skip. @jvmoverloads keeps the three-argument constructor — without it the BYOK
starter's boot test (#1890) fails to compile the moment these two branches meet.

The startup warning promised a fallback to the default LLM that no longer happens: an
unsatisfiable role throws at the point of use, deliberately. Say that instead.

Tests: context propagation through async and parallelMap including pooled-thread
isolation; role resolution end to end through ChatClientLlmOperations, so the wiring
that makes a role carry hyperparameters is pinned, not just the provider that computes
them; LlmOptions.withDefaultsFrom and modelName in their own module; the credential
paths that fail rather than fall back.

Signed-off-by: jasper blues <jasper@liberation-data.com>
ChatModel defaults stream() to UnsupportedOperationException("streaming is not supported").
For the placeholder that is both untrue and unactionable — streaming is not the problem, the
missing key is — and an application catching NoLlmConfiguredException to show its "add an API
key" page would not catch it at all. Chat applications are the ones that stream and the ones
that most need that path, so the default hid the message exactly where it was needed.

The platform still reports the placeholder as not supporting streaming: the capability
verifier probes by calling stream() and treats any exception as no. This only changes what a
caller is told when something streams anyway.

Signed-off-by: jasper blues <jasper@liberation-data.com>
@alexheifetz alexheifetz added this to the 2.0.0-Release milestone Aug 8, 2026
@igordayen

Copy link
Copy Markdown
Contributor

nstead, the placeholder is only used when explicitly selected through default-llm. If another model is configured as the default, setup-required is never resolved. There is a test covering this.

What about the "best" and "cheapest" if there is no default-LLM setup?

@igordayen

Copy link
Copy Markdown
Contributor

A missing API key should produce an actionable error rather than an empty response. Applications can catch NoLlmConfiguredException and show their own "add an API key" UI. Guide can use this and remove its local placeholder implementation.

At some point, we were considering behavior to have a fallback to a local LLM. Looping @alexheifetz

@igordayen igordayen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jasperblues - good development, few comments to consider. thank you

…onventions

Review found the starter could not do the thing it exists for. A deployment with no provider
key has no model registered for any role, and roles are ordinary configuration such a
deployment may already have — so naming any role at all failed context refresh with
"LLM 'gpt-4.1' for role best is not available", regardless of default-llm pointing at the
placeholder. Reproduced, then fixed: an unavailable role warns at startup and fails when
asked for, rather than taking the whole context down.

Deliberately not a fallback to the placeholder, or to a local model. A role that silently
resolves to something that cannot answer is the failure nobody notices; "configure me" has
to be loud to be worth anything.

The same hunk lands byte-identically on #1889 so the two merge without conflict.

Also, conventions this repo already follows and these files did not:
- multi-line strings instead of concatenation, for the three message constants
- blankApiKey.kt, lowerCamelCase, since it holds only top-level functions
  (messagePromptBuilders.kt, toolUtils.kt and eleven others set the precedent)
- dropped an assertion that only restated a constant's literal value
- TestableModelProviderConfiguration, since it is a test fixture rather than production wiring

Signed-off-by: jasper blues <jasper@liberation-data.com>
It has to be a name rather than a type: this module deliberately does not depend on
platform-autoconfigure, which is the point of the starter. But a rename would then drop the
ordering silently instead of failing the build, and the symptom would be the startup failure
this starter exists to prevent.

The starter module does have platform-autoconfigure on its classpath, so the name can be
pinned there.

Signed-off-by: jasper blues <jasper@liberation-data.com>
@jasperblues

jasperblues commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

What about the "best" and "cheapest" if there is no default-LLM setup?

Good catch — this was a real hole, and the starter could not do the thing it exists for.

A pure BYOK deployment has no model registered for any role, and roles are ordinary configuration such a deployment may already have. Naming any role at all took the context down, regardless of default-llm pointing at the placeholder. Reproduced against this branch before changing anything:

PROBE: construction FAILED -> IllegalStateException:
LLM 'gpt-4.1' for role best is not available: Choices are [setup-required]

Fixed: default-llm falls back to the placeholder when the model it names is not registered, and that fallback is what puts the deployment into setup-required mode. In that mode an unresolvable model name in embabel.models.llms is expected rather than fatal, so the context refreshes. A deployment that holds a key resolves default-llm to a real model, never enters the mode, and still dies at startup on a name nothing registers — which is what you want when the name really is a typo.

On this branch an unsatisfiable role still throws NoSuitableModelException when asked for. Once #1889 lands it resolves to the placeholder instead and throws NoLlmConfiguredException — the same actionable error the default LLM already gives. Role resolution lives in that PR, so the fallback belongs there; adding it here too would edit the one region the two branches would then diverge on.

PureByokWithRolesTest covers all three halves — it boots with best and cheapest configured and nothing to satisfy them, it still resolves the placeholder as the default, and asking for an unsatisfiable role throws. 15 tests green in embabel-agent-byok-autoconfigure, 8 in SetupRequiredModeTest.

One packaging note: #1889 changes the same lines for its own reasons, so I made the hunk byte-identical on both branches. Whichever lands first, the other merges clean.

@jasperblues

Copy link
Copy Markdown
Contributor Author

At some point, we were considering behavior to have a fallback to a local LLM. Looping @alexheifetz

Deliberately not a fallback here, and I would argue against adding one to this path.

The placeholder's whole job is to say "configure me", loudly. A fallback answers the question instead — plausibly, and worse — and the user attributes the drop in quality to the product rather than to a missing key. That is the failure nobody notices, which is the expensive kind. Same reasoning as an unsatisfiable role throwing rather than resolving to the default LLM: a wrong-but-working answer costs more than a stop.

A local model is also not free. It has to be present, pulled, and resourced. A fallback that itself requires setup is not much of a fallback, and it fails at the same moment for a second, less obvious reason.

The important part: the capability already exists without coupling it to the placeholder. A deployment that wants local-model behaviour registers a local model and points default-llm at it — no placeholder, no BYOK starter needed. That is available today, and having it as a stated deployment choice is strictly better than having it as a silent consequence of a missing key. Whoever is on call can tell the two apart.

So I would frame the choice as: BYOK-with-no-key should fail loudly, and local-model-as-default should be explicit configuration. Both are supported; they should just not be the same switch.

Happy to be talked out of it if there is a concrete deployment that needs the implicit version — but I would want to hear how an operator is expected to notice they are running on the fallback.

"Models are only ever selected by name or by role" omitted the default path — which is the
one the placeholder is actually selected by. The conclusion held, but the reasoning skipped
the case a reader would probe first, and this claim is what justifies registering the bean
unconditionally.

Signed-off-by: jasper blues <jasper@liberation-data.com>
@jasperblues

jasperblues commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Correcting my earlier reply about best and cheapest with no default-llm set.

The bug and the reproduction stand. The fix I described does not, and I would rather flag that now than let it merge.

I made the fatal check in ConfigurableModelProvider a warning unconditionally. That does fix the BYOK case, but it also means a deployment that holds a real key and has a typo in embabel.models.llms no longer fails to start — it boots and fails later, at whatever call first asks for that role. For every deployment. I bought the case you asked about by making the common case worse, and I did not notice the trade until it was pointed out.

The better shape, which is what will land: relax the check only when default-llm resolved to a placeholder. That is the deployment stating that keys arrive at runtime, so unresolvable model names are expected rather than a typo. Everyone else keeps failing fast, which is what you want when a name that resolves to nothing really is a mistake.

Landed. The check is fatal again except when default-llm resolved to a PlaceholderLlmService, and the embedding-services check is gated the same way.

Worth noting for whoever merges: #1889 touches the same lines, and I have been keeping the hunk byte-identical across both branches so they merge cleanly. That still needs to hold for the replacement.

@jasperblues

Copy link
Copy Markdown
Contributor Author

Some earlier comments on this PR have been edited in place rather than corrected by follow-ups, so the thread reads as one current account rather than a trail of revisions. GitHub keeps the original text under the "edited" control on each comment.

The substantive change: unresolvable model names in embabel.models configuration are fatal at startup again, as on main. An earlier revision made that check an unconditional warning so a pure BYOK deployment could boot — which fixed BYOK by making every keyed deployment worse, since a typo then became a late failure at whatever call first wanted that role. The check is now relaxed only when default-llm resolved to a placeholder, which is the deployment stating that keys arrive at runtime.

@igordayen — flagging directly, since one of the edited comments is a reply to you and reverses a claim in it.

@jasperblues
jasperblues requested a review from igordayen August 9, 2026 01:17
jasperblues and others added 4 commits August 9, 2026 13:32
An earlier revision made it an unconditional warning so a pure BYOK deployment
could boot. That fixed BYOK by making every keyed deployment worse: a typo in
embabel.models.llms became a late failure at whichever call first wanted that
role, instead of a failed start.

The check is now relaxed only when default-llm resolved to a PlaceholderLlmService,
which is the deployment stating that keys arrive at runtime. The embedding-services
check is gated the same way, with no fallback - an embedding model is a schema
commitment and nothing can stand in for one.

SetupRequiredLlmService carries the marker by delegation, because SpringAiLlmService
is a data class and cannot be extended. The self-typed methods return a wrapper
rather than the delegate, so the marker survives withPromptContributor.

The shared hunks are byte-identical with #1889 so the two still merge in either order.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ying

Sonar's new-code coverage gate caught this: the hand-written delegation was
largely untested, and the untested part included the two self-typed methods whose
entire justification is that they must return a wrapper rather than the delegate.
Losing the marker there is silent - the deployment stops being in setup-required
mode and goes back to failing at startup on names it cannot resolve.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both are new behaviour on this branch and neither was exercised: default-llm
naming a model nothing has registered yet (the realistic pure-BYOK
application.yml, where the name stays put and the placeholder stands in until a
key arrives), and the embedding-services check on both sides of the gate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate lives in embabel-agent-api but was only exercised from the BYOK
autoconfigure module's tests, so per-module coverage saw the new branches as
dead. Testing platform behaviour in the platform module is the right place for
it anyway - the BYOK module's tests should be about the placeholder, not about
ConfigurableModelProvider's startup rules.

Uses the same delegating stand-in for the placeholder that #1889 does, since
SpringAiLlmService is a data class and cannot carry the marker by extension.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@igordayen

Copy link
Copy Markdown
Contributor
image

@igordayen igordayen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jasperblues - looks good, a few comments to consider, thanks

// Named, because degrading a real model to the placeholder would otherwise hide the
// case where the key IS set and the model simply failed to register.
logger.warn(
"Default LLM '{}' is not registered; falling back to the '{}' placeholder. " +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

multiline comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Happy to change these, but the repo currently goes the other way, so flagging before I do.

Counting indented multi-line comments in embabel-agent-api/src/main/kotlin:

  • stacked // runs of 3+ lines: 68
  • indented /* ... */ non-KDoc blocks: 12

So stacked // is roughly 6:1 the house style — AgentProcessChatbot.kt, TokenBudgetConversationFormatter.kt and HybridUtilityPlanner.kt are three at random. KDoc /** */ is of course used everywhere for declarations; this is only about explanatory comments inside function bodies, which is what these are.

I have no strong preference and will switch all three if you want them as block comments — just say and it is one edit. If the intent is that the repo should move to /* */ generally, that is worth doing as its own sweep rather than only on the lines this PR happens to touch, otherwise the file ends up mixed.

// is expected there and only worth reporting.
if (setupRequired) {
logger.warn(
"LLM '{}' for role '{}' is not registered. This deployment is awaiting a key, so that is expected; " +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as above — flagged the 68:12 count in favour of stacked // on the first of these rather than repeating it. Happy to change all three together.

// still throws.
if (setupRequired) {
logger.warn(
"Embedding model '{}' for role '{}' is not registered. This deployment is awaiting a key, " +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

miltiline comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

* cannot depend on. [SpringAiLlmService] is a data class and so final; the real placeholder
* carries the marker by delegation in the same way.
*/
private val placeholderModel: LlmService<*> = object :

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

test will fail upon gpt-mini deprecation. use const or local config for better source code mgmt

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Extracted to constants — DEFAULT_MODEL, BEST_MODEL, CHEAPEST_MODEL.

Correcting the premise though, because it changes what the fix is for: these cannot fail on deprecation. Every service in this test is SpringAiLlmService(name, provider, mockk<ChatModel>()), so nothing reaches a provider and the id is an opaque identifier — the test would pass just as well with "model-a".

The real cost was readability: with the id repeated a dozen times a reader has to decide in each assertion whether gpt-4.1 and gpt-4.1-mini differ meaningfully. The constants say they do, and the comment says the ids are arbitrary so nobody has to work that out from the mock.

@Test
fun `starts with roles configured and no model to satisfy them`() {
assertThatCode {
pureByok(mapOf(BEST_ROLE to "gpt-4.1", CHEAPEST_ROLE to "gpt-4.1-nano"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same treatment, though that particular test has since been replaced — the placeholder gate is now covered by ConfigurableModelProviderTest.SetupRequiredMode and SetupRequiredModeTest, which supersede PureByokWithRolesTest. Constants applied where the ids now live.

Review point. The ids appeared across a dozen assertions, so a reader had to decide in each
one whether "gpt-4.1" and "gpt-4.1-mini" differed meaningfully.

Correcting the premise slightly: these cannot break on deprecation. Every service in this
test wraps a mockk ChatModel, so nothing reaches a provider and the id is an opaque
identifier. Named anyway, and the comment says which it is, so nobody has to work that out
from the mock.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: jasper blues <jasper@liberation-data.com>
@sonarqubecloud

sonarqubecloud Bot commented Aug 9, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants