Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import com.embabel.chat.UserMessage
import com.embabel.common.ai.model.LlmOptions
import com.embabel.common.byok.ByokFactory
import com.embabel.common.byok.InvalidApiKeyException
import com.embabel.common.byok.requireUsableApiKey
import com.embabel.common.util.ObjectProviders
import io.micrometer.observation.ObservationRegistry
import org.slf4j.LoggerFactory
Expand Down Expand Up @@ -155,10 +156,17 @@ open class AnthropicModelFactory(
* On any exception the provider-specific error is translated to [InvalidApiKeyException],
* keeping Spring AI types out of the caller.
*
* A blank key is rejected before any network call. A key is routinely blank rather than
* absent: Compose passes `ANTHROPIC_API_KEY=${'$'}{ANTHROPIC_API_KEY:-}`, so in a container the
* variable is set-but-empty, and callers reading it with a null default get `""`. Without
* this check that empty string reaches the provider and comes back as an opaque auth error,
* which is why BYOK callers otherwise re-derive "blank counts as absent" for themselves.
*
* @param model Model to use for the probe.
* @throws InvalidApiKeyException if the key is invalid.
* @throws InvalidApiKeyException if the key is blank or invalid.
*/
fun buildValidated(model: String): LlmService<*> {
requireUsableApiKey(apiKey)
val probe = build(model)
try {
probe.createMessageSender(LlmOptions()).call(listOf(UserMessage("Hi")), emptyList())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package com.embabel.agent.anthropic

import com.embabel.agent.api.models.AnthropicModels
import com.embabel.agent.spi.support.springai.SpringAiLlmService
import com.embabel.common.byok.BLANK_API_KEY_MESSAGE
import com.embabel.common.byok.InvalidApiKeyException
import com.sun.net.httpserver.HttpServer
import io.micrometer.observation.ObservationRegistry
Expand Down Expand Up @@ -154,4 +155,26 @@ class AnthropicModelFactoryBuildValidatedTest {
factory().buildValidated(AnthropicModels.CLAUDE_HAIKU_4_5)
}
}

@Test
fun `buildValidated rejects a blank key without calling the provider`() {
var requests = 0
server.createContext("/v1/messages") { exchange ->
requests++
exchange.sendResponseHeaders(500, -1)
exchange.close()
}
server.start()

val blankKeyFactory = AnthropicModelFactory(
apiKey = " ",
baseUrl = "http://localhost:$port",
observationRegistry = ObservationRegistry.NOOP,
restClientBuilder = restClientBuilder,
)

val e = assertThrows<InvalidApiKeyException> { blankKeyFactory.buildValidated() }
assertEquals(BLANK_API_KEY_MESSAGE, e.message)
assertEquals(0, requests, "a blank key must not reach the provider")
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright 2024-2026 Embabel Pty Ltd.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.embabel.agent.spi

/**
* Marks an [LlmService] that stands in for a model this deployment does not yet have a key for.
*
* A placeholder completes nothing. Its only job is to give the platform something to resolve
* before any key is supplied, so that "no key yet" surfaces as an actionable error at the call
* that needed an LLM rather than as a failure to start.
*
* The platform treats the presence of a placeholder as the deployment's own statement that keys
* arrive at runtime, and relaxes exactly one thing on the strength of it: model names in
* configuration that nothing has registered are 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.
*
* Implemented by `SetupRequiredLlm` in `embabel-agent-byok-autoconfigure`. It lives here, next to
* [LlmService], because `com.embabel.common.ai.model.ConfigurableModelProvider` has to recognise
* a placeholder without depending on the BYOK module - carrying the placeholder without dragging
* in that module is the reason the module exists.
*/
interface PlaceholderLlmService
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.embabel.common.ai.model

import com.embabel.agent.spi.LlmService
import com.embabel.agent.spi.PlaceholderLlmService
import com.embabel.common.util.indent
import com.embabel.common.util.loggerFor
import org.springframework.boot.context.properties.ConfigurationProperties
Expand Down Expand Up @@ -68,11 +69,44 @@ class ConfigurableModelProvider(
private val defaultLlm =
if (llms.isNotEmpty())
llms.firstOrNull { it.name == properties.defaultLlm }
?: placeholderLlm()
?: throw IllegalArgumentException(
"Default LLM '${properties.defaultLlm}' not found. Set the 'embabel.models.default-llm' property to one of the available models: ${llms.map { it.name }}.")
else
throw IllegalArgumentException("No models detected. Ensure that at least one Embabel Agent Starter (e.g. embabel-agent-starter-openai) is on the classpath and models are loaded into it.")

/**
* Whether this deployment is waiting for a key rather than misconfigured.
*
* True exactly when `default-llm` resolved to a [PlaceholderLlmService] - either because it
* names one, or because the model it names is not registered and a placeholder stands in. That
* is the deployment stating that keys arrive at runtime, and it is the only thing that makes an
* unresolvable model name in configuration expected rather than a typo.
*
* A deployment that has a key resolves `default-llm` to a real model and so is never in this
* mode, even with a placeholder registered alongside - which is what a BYOK starter next to a
* provider starter looks like.
*/
private val setupRequired: Boolean = defaultLlm is PlaceholderLlmService

/**
* The registered placeholder, if this deployment carries one.
*
* Deliberately structural rather than by name: `com.embabel.agent.spi` owns the marker, and
* this class must not depend on the BYOK module that implements it.
*/
private fun placeholderLlm(): LlmService<*>? =
llms.firstOrNull { it is PlaceholderLlmService }
?.also {
// 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.

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.

At some point, RJ reviewed the code and requested to change all occurrences.

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.

That settles it — done, and my count was measuring the wrong thing.

68 stacked-// against 12 block comments describes a codebase that has not caught up with what Rod asked for, not the convention it is meant to follow. Arguing from that count was the error, not the conclusion I drew from it.

Converted the multi-line explanations this PR introduces, in ConfigurableModelProvider, ConfigurableModelProviderTest, SetupRequiredLlmTest and PureByokWithRolesTest. 48 tests green.

Deliberately scoped to blocks this PR adds — I did convert the pre-existing ones in files it merely touches at first, then backed that out: it widened the diff into InstrumentedChatModel, SpringAiLlmMessageSender and converters.kt for no reviewable reason. The sweep Rod asked for is better done as a sweep than smuggled through whichever PRs happen to touch a file. Happy to do that as its own change if you want it.

"Calls will fail with an actionable 'no LLM configured' error until a key is supplied. Available: {}",
properties.defaultLlm, it.name, llms.map { it.name },
)
}

// Compute this lazily as embedding services may not be available
private fun defaultEmbeddingService() =
embeddingServices.firstOrNull { it.name == properties.defaultEmbeddingModel }
Expand All @@ -81,14 +115,45 @@ class ConfigurableModelProvider(
init {
properties.llms.forEach { (role, model) ->
if (llms.none { it.name == model }) {
error("LLM '$model' for role $role is not available: Choices are ${llms.map { it.name }}")
/*
* Fatal, unless this deployment is waiting for a key. A name that resolves to
* nothing is a typo in a deployment that has one, and letting it start would move
* the failure to whichever unrelated call first asks for that role. A deployment in
* setup-required mode has no models registered yet by definition, so the same name
* 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.

"the role will report 'no LLM configured' until one is supplied. Available: {}",
model, role, llms.map { it.name },
)
} else {
error("LLM '$model' for role $role is not available: Choices are ${llms.map { it.name }}")
}
}
}
logger.info(infoString(verbose = true))

properties.embeddingServices.forEach { (role, model) ->
if (embeddingServices.none { it.name == model }) {
error("Embedding model '$model' for role $role is not available: Choices are ${embeddingServices.map { it.name }}")
/*
* The same gate as the LLM roles above, and for the same reason: an unresolvable
* name is a typo in a deployment that holds a key, and expected in one still
* waiting for it. There is no fallback here, though, and there should not be -
* an embedding model is a schema commitment and nothing can stand in for one.
* The gate decides only whether the deployment STARTS; asking for the service
* 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.

"so that is expected; asking for that role will still fail. Available: {}",
model, role, embeddingServices.map { it.name },
)
} else {
error("Embedding model '$model' for role $role is not available: Choices are ${embeddingServices.map { it.name }}")
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
package com.embabel.common.ai.model

import com.embabel.agent.spi.LlmService
import com.embabel.agent.spi.PlaceholderLlmService
import com.embabel.agent.spi.support.springai.SpringAiLlmService
import com.embabel.common.ai.model.ModelProvider.Companion.BEST_ROLE
import com.embabel.common.ai.model.ModelProvider.Companion.CHEAPEST_ROLE
Expand All @@ -30,6 +32,111 @@ import kotlin.test.assertContains

class ConfigurableModelProviderTest {

private companion object {
/**
* Opaque identifiers, not live models: every service here wraps a mockk ChatModel, so
* nothing reaches a provider and a retired catalogue id cannot break these tests. Named
* anyway — the ids appear across a dozen assertions, and a reader should not have to
* decide whether BEST_MODEL and DEFAULT_MODEL differ meaningfully in each one.
*/
const val DEFAULT_MODEL = "gpt-4.1-mini"
const val BEST_MODEL = "gpt-4.1"
const val CHEAPEST_MODEL = "gpt-4.1-nano"
}

/**
* Stands in for `SetupRequiredLlm`, which lives in the BYOK autoconfigure module this one
* 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.

LlmService<SpringAiLlmService> by SpringAiLlmService(
"setup-required", "none", mockk<ChatModel>(),
),
PlaceholderLlmService {}

private fun providerWith(
llms: List<LlmService<*>>,
properties: ConfigurableModelProviderProperties,
embeddingServices: List<EmbeddingService> = emptyList(),
) = ConfigurableModelProvider(llms, embeddingServices, properties)

@Nested
inner class SetupRequiredMode {

private val real: LlmService<*> =
SpringAiLlmService(DEFAULT_MODEL, "openai", mockk<ChatModel>())

@Test
fun `a deployment awaiting a key boots with roles nothing can satisfy`() {
val mp = providerWith(
llms = listOf(placeholderModel),
properties = ConfigurableModelProviderProperties(
llms = mapOf(BEST_ROLE to BEST_MODEL, CHEAPEST_ROLE to CHEAPEST_MODEL),
defaultLlm = "setup-required",
),
)
assertEquals("setup-required", mp.getLlm(DefaultModelSelectionCriteria).name)
}

@Test
fun `default-llm naming an unregistered model falls back to the placeholder`() {
// The realistic pure-BYOK application.yml: default-llm still names the model the
// deployment wants once a key arrives, and the placeholder stands in until then.
val mp = providerWith(
llms = listOf(placeholderModel),
properties = ConfigurableModelProviderProperties(defaultLlm = BEST_MODEL),
)
assertEquals("setup-required", mp.getLlm(DefaultModelSelectionCriteria).name)
}

@Test
fun `a deployment holding a key still dies on a name nothing registers`() {
/*
* The other half of the gate. Making this a warning too - which an earlier revision
* did - fixes BYOK by turning every keyed deployment's typo into a late failure at
* whichever call first wants that role.
*/
val e = assertThrows<IllegalStateException> {
providerWith(
llms = listOf(real, placeholderModel),
properties = ConfigurableModelProviderProperties(
llms = mapOf(BEST_ROLE to BEST_MODEL),
defaultLlm = DEFAULT_MODEL,
),
)
}
assertContains(e.message!!, BEST_MODEL)
}

@Test
fun `an unresolvable embedding role is tolerated while awaiting a key`() {
providerWith(
llms = listOf(placeholderModel),
properties = ConfigurableModelProviderProperties(
embeddingServices = mapOf("default" to "text-embedding-3-small"),
defaultLlm = "setup-required",
),
)
}

@Test
fun `an unresolvable embedding role is still fatal for a deployment holding a key`() {
// Same gate, no fallback: there is no embedding placeholder and there should not be
// one, so this decides only whether the deployment starts.
val e = assertThrows<IllegalStateException> {
providerWith(
llms = listOf(real),
properties = ConfigurableModelProviderProperties(
embeddingServices = mapOf("default" to "text-embedding-3-small"),
defaultLlm = DEFAULT_MODEL,
),
)
}
assertContains(e.message!!, "text-embedding-3-small")
}
}

/**
* Custom EmbeddingService that does NOT extend AiModel.
* Verifies that the framework works with non-Spring AI embedding implementations.
Expand All @@ -52,7 +159,7 @@ class ConfigurableModelProviderTest {
private val mp: ModelProvider = ConfigurableModelProvider(
llms = listOf(
SpringAiLlmService("gpt40", "OpenAI", mockk<ChatModel>(), DefaultOptionsConverter),
SpringAiLlmService("gpt-4.1-mini", "OpenAI", mockk<ChatModel>(), DefaultOptionsConverter),
SpringAiLlmService(DEFAULT_MODEL, "OpenAI", mockk<ChatModel>(), DefaultOptionsConverter),
SpringAiLlmService("embedding", "OpenAI", mockk<ChatModel>(), DefaultOptionsConverter)
),
embeddingServices = listOf(
Expand Down Expand Up @@ -160,7 +267,7 @@ class ConfigurableModelProviderTest {

@Test
fun `valid name`() {
val llm = mp.getLlm(ByNameModelSelectionCriteria("gpt-4.1-mini"))
val llm = mp.getLlm(ByNameModelSelectionCriteria(DEFAULT_MODEL))
assertNotNull(llm)
}
}
Expand All @@ -185,7 +292,7 @@ class ConfigurableModelProviderTest {

private val customMp = ConfigurableModelProvider(
llms = listOf(
SpringAiLlmService("gpt-4.1-mini", "OpenAI", mockk<ChatModel>(), DefaultOptionsConverter),
SpringAiLlmService(DEFAULT_MODEL, "OpenAI", mockk<ChatModel>(), DefaultOptionsConverter),
),
embeddingServices = listOf(
CustomEmbeddingService("my-custom-embeddings", "CustomProvider"),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>com.embabel.agent</groupId>
<artifactId>embabel-agent-autoconfigure</artifactId>
<version>1.5.0-SNAPSHOT</version>
<relativePath>../../pom.xml</relativePath>
</parent>
<artifactId>embabel-agent-byok-autoconfigure</artifactId>
<packaging>jar</packaging>
<name>Embabel Agent Autoconfiguration BYOK</name>
<description>Placeholder LLM service allowing a pure BYOK deployment to start with no keys configured</description>
<url>https://github.com/embabel/embabel-agent</url>

<scm>
<url>https://github.com/embabel/embabel-agent</url>
<connection>scm:git:https://github.com/embabel/embabel-agent.git</connection>
<developerConnection>scm:git:https://github.com/embabel/embabel-agent.git</developerConnection>
<tag>HEAD</tag>
</scm>

<dependencies>
<dependency>
<groupId>com.embabel.agent</groupId>
<artifactId>embabel-agent-api</artifactId>
</dependency>

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

<dependency>
<groupId>com.embabel.agent</groupId>
<artifactId>embabel-agent-test-internal</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

</project>
Loading
Loading