diff --git a/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/AgentPlatform.kt b/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/AgentPlatform.kt index 4264f96ce..9e8f2e373 100644 --- a/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/AgentPlatform.kt +++ b/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/AgentPlatform.kt @@ -16,6 +16,7 @@ package com.embabel.agent.core import com.embabel.agent.api.common.PlatformServices +import com.embabel.agent.core.support.distinctByNameReportingCollisions import com.embabel.agent.spi.ToolGroupResolver import com.embabel.agent.spi.common.Constants import java.util.concurrent.CompletableFuture @@ -133,15 +134,19 @@ interface AgentPlatform : AgentScope { ): AgentProcess override val domainTypes: Collection - get() = agents().flatMap { it.domainTypes }.distinctBy { it.name } + get() = agents().flatMap { it.domainTypes } + .distinctByNameReportingCollisions(kind = "domain type") { it.name } override val actions: List - get() = agents().filterNot { it.opaque }.flatMap { it.actions }.distinctBy { it.name } + get() = agents().filterNot { it.opaque }.flatMap { it.actions } + .distinctByNameReportingCollisions(kind = "action") { it.name } override val goals: Set - get() = agents().flatMap { it.goals }.distinctBy { it.name }.toSet() + get() = agents().flatMap { it.goals } + .distinctByNameReportingCollisions(kind = "goal") { it.name }.toSet() override val conditions: Set - get() = agents().filterNot { it.opaque }.flatMap { it.conditions }.distinctBy { it.name }.toSet() + get() = agents().filterNot { it.opaque }.flatMap { it.conditions } + .distinctByNameReportingCollisions(kind = "condition") { it.name }.toSet() } diff --git a/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/support/nameCollisions.kt b/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/support/nameCollisions.kt new file mode 100644 index 000000000..1db9ac36d --- /dev/null +++ b/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/support/nameCollisions.kt @@ -0,0 +1,72 @@ +/* + * 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.core.support + +import org.slf4j.LoggerFactory +import java.util.concurrent.ConcurrentHashMap + +private val logger = LoggerFactory.getLogger("com.embabel.agent.core.support.NameCollisions") + +/** + * Collisions already reported, so that a collision on a hot path — tool resolution runs + * per LLM operation, and the platform's aggregated views are recomputed on every read — + * is reported once rather than on every pass. Bounded so a pathological caller cannot + * grow it without limit. + */ +private const val MAX_REPORTED = 1_000 +private val reported = ConcurrentHashMap.newKeySet() + +/** + * De-duplicate by name, reporting the elements that are lost. + * + * These views are keyed by name and downstream consumers identify their elements by name + * alone, so a name carried by two different elements cannot be honoured twice and one of + * them is dropped. Dropping is not itself the problem — two declarations of the very same + * element collapse harmlessly. Dropping *silently* is, because the capability simply stops + * existing with nothing in the log to say so. + * + * @param kind what is being de-duplicated, for the log message + * @param sameValue whether two elements sharing a name are in fact the same thing, and so + * safe to collapse. Defaults to equality, which is meaningful for the value types the + * platform aggregates; callers holding types without value semantics should pass identity. + * @param name the name to de-duplicate on + */ +internal fun Iterable.distinctByNameReportingCollisions( + kind: String, + sameValue: (T, T) -> Boolean = { a, b -> a == b }, + name: (T) -> String, +): List { + val kept = LinkedHashMap() + for (element in this) { + val elementName = name(element) + val incumbent = kept.putIfAbsent(elementName, element) + if (incumbent != null && !sameValue(incumbent, element)) { + report(kind, elementName) + } + } + return kept.values.toList() +} + +private fun report(kind: String, name: String) { + if (reported.size < MAX_REPORTED && reported.add("$kind/$name")) { + logger.error( + "🛑 Two different {}s are named '{}'. Only one of them is visible; the other has been dropped. " + + "Names must be unique because downstream consumers — published tools, the goal ranker — " + + "identify {}s by name alone. Rename one of them.", + kind, name, kind, + ) + } +} diff --git a/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/support/toolUtils.kt b/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/support/toolUtils.kt index 3c9a2b94c..f2b1449e6 100644 --- a/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/support/toolUtils.kt +++ b/embabel-agent-api/src/main/kotlin/com/embabel/agent/core/support/toolUtils.kt @@ -40,7 +40,12 @@ private val externalToolExtractor: ExternalToolExtractor? = try { */ fun safelyGetTools(instances: Collection): List = instances.flatMap { safelyGetToolsFrom(it) } - .distinctBy { it.definition.name } + // Tool is an interface with no value semantics, so two tools sharing a name can only + // be told apart by identity: the same tool reaching us through two tool objects is + // harmless, two distinct tools under one name means one of them is not callable. + .distinctByNameReportingCollisions(kind = "tool", sameValue = { a, b -> a === b }) { + it.definition.name + } .sortedBy { it.definition.name } /** @@ -69,7 +74,12 @@ fun safelyGetToolsFrom(toolObject: ToolObject): List { it } } - .distinctBy { it.definition.name } + // Renaming happens just above, so a naming strategy that maps two distinct tools onto + // one name has its collision created and then discarded here. Report it: the caller + // wrote the strategy and is the only one who can fix it. + .distinctByNameReportingCollisions(kind = "renamed tool", sameValue = { a, b -> a === b }) { + it.definition.name + } .sortedBy { it.definition.name } } diff --git a/embabel-agent-api/src/test/kotlin/com/embabel/agent/core/AgentPlatformNameCollisionReportingTest.kt b/embabel-agent-api/src/test/kotlin/com/embabel/agent/core/AgentPlatformNameCollisionReportingTest.kt new file mode 100644 index 000000000..d0406ed1b --- /dev/null +++ b/embabel-agent-api/src/test/kotlin/com/embabel/agent/core/AgentPlatformNameCollisionReportingTest.kt @@ -0,0 +1,123 @@ +/* + * 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.core + +import ch.qos.logback.classic.Level +import ch.qos.logback.classic.Logger +import ch.qos.logback.classic.spi.ILoggingEvent +import ch.qos.logback.core.read.ListAppender +import com.embabel.agent.api.dsl.Frog +import com.embabel.agent.api.dsl.agent +import com.embabel.agent.domain.io.UserInput +import com.embabel.agent.test.integration.IntegrationTestUtils +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import org.slf4j.LoggerFactory +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +private fun agentWithGoalNamed(agentName: String, goalName: String, meaning: String) = + agent(agentName, description = "Turn a person into a frog") { + transformation(name = "$agentName-action") { Frog(agentName) } + goal(name = goalName, description = meaning, satisfiedBy = Frog::class) + } + +/** + * [AgentPlatform] exposes goals, actions and conditions as flat views keyed by name, so two + * agents contributing different elements under one name cannot both be honoured and one is + * dropped. That is longstanding behaviour and this change does not alter it. + * + * What it alters is that the drop is no longer silent. The original report on this was that + * both agents deployed, `agents()` returned both, nothing was logged, and the capability had + * simply stopped existing. + */ +class AgentPlatformNameCollisionReportingTest { + + private lateinit var appender: ListAppender + private lateinit var logger: Logger + + @BeforeEach + fun attachAppender() { + logger = LoggerFactory.getLogger("com.embabel.agent.core.support.NameCollisions") as Logger + appender = ListAppender().apply { start() } + logger.addAppender(appender) + logger.level = Level.ERROR + } + + @AfterEach + fun detachAppender() { + logger.detachAppender(appender) + appender.stop() + } + + private fun errorsMentioning(name: String): List = + appender.list + .filter { it.level == Level.ERROR } + .map { it.formattedMessage } + .filter { it.contains(name) } + + private fun goalName(suffix: String) = "reporting-test-$suffix-${System.nanoTime()}" + + @Test + fun `a goal lost to another agent's goal of the same name is reported`() { + val shared = goalName("lost") + val platform = IntegrationTestUtils.dummyAgentPlatform() + platform.deploy(agentWithGoalNamed("AardvarkWizard", shared, "what Aardvark means")) + platform.deploy(agentWithGoalNamed("ZebraWizard", shared, "what Zebra means")) + + assertEquals(2, platform.agents().size, "Both agents still deploy: deployment does not validate") + assertEquals( + 1, + platform.goals.count { it.name == shared }, + "One goal is still dropped: the view is keyed by name", + ) + assertEquals( + 1, + errorsMentioning(shared).size, + "The drop must be reported: ${appender.list.map { it.formattedMessage }}", + ) + } + + @Test + fun `two agents declaring the very same goal are not reported`() { + val shared = goalName("identical") + val platform = IntegrationTestUtils.dummyAgentPlatform() + platform.deploy(agentWithGoalNamed("AardvarkWizard", shared, "one meaning, agreed by both")) + platform.deploy(agentWithGoalNamed("ZebraWizard", shared, "one meaning, agreed by both")) + + assertEquals(1, platform.goals.count { it.name == shared }, "Collapsing them loses nothing") + assertTrue( + errorsMentioning(shared).isEmpty(), + "Nothing was lost, so there is nothing to report: ${errorsMentioning(shared)}", + ) + } + + @Test + fun `goals with distinct names are all kept and nothing is reported`() { + val aardvark = goalName("aardvark") + val zebra = goalName("zebra") + val platform = IntegrationTestUtils.dummyAgentPlatform() + platform.deploy(agentWithGoalNamed("AardvarkWizard", aardvark, "what Aardvark means")) + platform.deploy(agentWithGoalNamed("ZebraWizard", zebra, "what Zebra means")) + + assertEquals( + setOf(aardvark, zebra), + platform.goals.map { it.name }.filter { it.startsWith("reporting-test-") }.toSet(), + ) + assertTrue(errorsMentioning(aardvark).isEmpty() && errorsMentioning(zebra).isEmpty()) + } +} diff --git a/embabel-agent-api/src/test/kotlin/com/embabel/agent/core/support/NameCollisionsTest.kt b/embabel-agent-api/src/test/kotlin/com/embabel/agent/core/support/NameCollisionsTest.kt new file mode 100644 index 000000000..1ce7046c0 --- /dev/null +++ b/embabel-agent-api/src/test/kotlin/com/embabel/agent/core/support/NameCollisionsTest.kt @@ -0,0 +1,151 @@ +/* + * 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.core.support + +import ch.qos.logback.classic.Level +import ch.qos.logback.classic.Logger +import ch.qos.logback.classic.spi.ILoggingEvent +import ch.qos.logback.core.read.ListAppender +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Nested +import org.junit.jupiter.api.Test +import org.slf4j.LoggerFactory +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +private data class Named(val name: String, val meaning: String) + +/** + * The de-duplication itself is not new — the platform has always collapsed name-keyed + * views. What is new is that a collapse which loses a distinct element says so. + */ +class NameCollisionsTest { + + private lateinit var appender: ListAppender + private lateinit var logger: Logger + + @BeforeEach + fun attachAppender() { + logger = LoggerFactory.getLogger("com.embabel.agent.core.support.NameCollisions") as Logger + appender = ListAppender().apply { start() } + logger.addAppender(appender) + logger.level = Level.ERROR + } + + @AfterEach + fun detachAppender() { + logger.detachAppender(appender) + appender.stop() + } + + private fun errors(): List = + appender.list.filter { it.level == Level.ERROR }.map { it.formattedMessage } + + private fun unique(suffix: String) = "collision-test-$suffix-${System.nanoTime()}" + + @Nested + inner class Reports { + + @Test + fun `two different elements sharing a name are reported`() { + val name = unique("different") + val kept = listOf( + Named(name, "what A means"), + Named(name, "what B means"), + ).distinctByNameReportingCollisions(kind = "goal") { it.name } + + assertEquals(1, kept.size, "One is still dropped: the view is keyed by name") + assertEquals(1, errors().size, "The loss must be reported") + assertTrue( + errors().single().contains(name), + "The report must name the offending element: ${errors()}", + ) + } + + @Test + fun `a collision is reported once, not on every pass`() { + val name = unique("hot-path") + val elements = listOf(Named(name, "A"), Named(name, "B")) + + repeat(5) { elements.distinctByNameReportingCollisions(kind = "goal") { it.name } } + + assertEquals( + 1, + errors().size, + "Aggregated views are recomputed on every read; reporting must not repeat", + ) + } + + @Test + fun `elements without value semantics are compared as the caller asks`() { + val name = unique("identity") + val one = Named(name, "same") + val two = Named(name, "same") + + listOf(one, two).distinctByNameReportingCollisions( + kind = "tool", + sameValue = { a, b -> a === b }, + ) { it.name } + + assertEquals( + 1, + errors().size, + "Equal by value but distinct instances: identity comparison must still report", + ) + } + } + + @Nested + inner class StaysQuiet { + + @Test + fun `the same element declared twice is not a collision`() { + val name = unique("identical") + val kept = listOf( + Named(name, "one meaning, agreed by both"), + Named(name, "one meaning, agreed by both"), + ).distinctByNameReportingCollisions(kind = "goal") { it.name } + + assertEquals(1, kept.size) + assertTrue(errors().isEmpty(), "Nothing is lost, so nothing to report: ${errors()}") + } + + @Test + fun `distinct names are all kept and nothing is reported`() { + val a = unique("a") + val b = unique("b") + val kept = listOf(Named(a, "A"), Named(b, "B")) + .distinctByNameReportingCollisions(kind = "goal") { it.name } + + assertEquals(listOf(a, b), kept.map { it.name }) + assertTrue(errors().isEmpty()) + } + + @Test + fun `first declaration wins, as before`() { + val name = unique("order") + val kept = listOf(Named(name, "first"), Named(name, "second")) + .distinctByNameReportingCollisions(kind = "goal") { it.name } + + assertEquals( + "first", + kept.single().meaning, + "distinctBy kept the first; that behaviour is unchanged", + ) + } + } +}