Conversation
|
@tuannx - thanks for reporting. Also - appears something got broken due to migration, or its existing issue; could you please try to dig into the history of the problem? |
|
Thanks for the review. Issue opened: #1834 On the history question — this is not from the migration. It goes back to Before: de-duplicate by value. Two identical goals collapse into one, which is That is the only commit that ever touched those lines. The fix restores that distinction rather than removing the de-duplication: Sorry about the dense write-up. Issue and PR description are rewritten in One decision I would rather you made than inherited from a diff: |
|
Thank you, @tuannx - will follow up. Best regards! |
@alexheifetz - could you please advise - conflicting goals:
Should probably align with logic: from PR #1801 |
@tuannx - could you please elaborate on "goal value"? Thank you. |
|
@tuannx @deleSerna @alexheifetz - since we are not sure whether it is an exception or a flagging error, it is a better option - may I suggest considering something like ErroneousAgentExitPolicy with default behavior ERROR. |
AchievesGoal without Action should definitely stop the Agent as it's spec violation and it can be easily fix while developing the Agent itself. Could these conflicting goals/action/conditions belongs to agents from third party libraries? If yes then I do not think there is a straight forward solution to this. But my knowledge here is limited. I always write Agent for a stand alone spring boot application. But, if Agent could also be in 3rd party library then just throwing an exception or just flagging an error also won't help as it's not actionable for the consumer of those conflicting libraries. |
==> That's the reason for suggesting having an error policy configurable. thanks |
|
@igordayen @deleSerna There's a third option, and the annotation path already does it. Annotated goals and the tool naming strategy expects that shape: So two annotated agents can both have a same goal and never collide. Only the That makes the collision impossible instead of reporting it better, and nobody If you agree with the direction I'll rework this PR — the deploy-time check goes |
@igordayen But that would not also fix the real issue when the conflicting actions/goal are coming from multiple agents right?
This could also still result in duplicate name as it's still @igordayen @alexheifetz IMO, we should go in the direction suggested by @tuannx but should use 'Name |
|
Three points raised:
@tuannx - concrete naming examples, please, to substantiate the idea and impact assessment. |
|
Before we choose between throwing, an error policy, or qualification, could we do a @igordayen asked for concrete naming examples and an impact assessment. We can generate Any naming change then shows up as a diff, so we can see what breaks on the wire before We have changed these names unnoticed before: #599 (Claude Desktop rejected a tool name) It would also show that two agents in different packages with the same state class name Happy to open it. This PR would then rebase on top. |
|
thanks @tuannx Was actually inquiring about all patterns on validation logic in the agent validation package - what behavior do they expose by default? Is it consistent? |
DefaultAgentStructureValidator currently report errors ( agent booting won’t stop) for the following cases:
AgentMetadataReader stop the agent for following cases:
AgentMetadataReader reports errors for following case
GoapPathToCompletionValidator reports error for cases where it can not to the goal I have not checked in other places @igordayen do you mean this report? |
|
@deleSerna - thanks for the analysis. So, Duplicate action names ==> already in place, but the algorithm requires refinements. What is the flow? metadata reader ==> validator ==> deployer. Maybe propagate errors up to the deployer, and at the deployer level apply a proper exit policy? Looking for an architecturally sound flow. thanks |
|
@tuannx - Is this PR valid then? As the analysis attached to the issues clearly states that deployments should not be rejected. |
Hi @igordayen: Updated PR to log error only, please review whenever you're available. Thank you! |
tuannx
changed the title
|
@tuannx Ran by Claude: findings (no blockers, all medium/low):
===================================== |
| val namingStrategy = processContext.platformServices.toolNamingStrategy() | ||
| val actionSignatures = tools.filterIsInstance<CurriedActionTool>().joinToString("\n") { tool -> | ||
| val signature = TypeSchemaExtractor.buildActionSignature(tool.action) | ||
| val owner = processContext.agentProcess.agent.name |
There was a problem hiding this comment.
no NPE danger?
explain logic, please add comments, add example in comment
There was a problem hiding this comment.
Outdated: SupervisorAgentFactory is no longer changed by this PR.
|
From Codex:
Open question
@azanux - could you please provide feedback, thanks |
@igordayen Both Codex findings are the two disabled tests in this PR. That is what those tests are for — they record what qualification by owner cannot fix, so it is visible instead of hidden. This is the point I have been trying to get across for a while.
Also: keeping the action in the owner would not have fixed either one. All tools in one interaction go to one call for one action, so the owner is the same either way. Dropping it lost nothing. Your question: keep export.name as an opt-out, but stop being silent. Two agents picking the same export name should be reported at deploy time, using the nameCollisions code we already have. Same for distinctBy — warn instead of dropping quietly. |
azanux
left a comment
azanux
left a comment
There was a problem hiding this comment.
Hi @tuannx, thanks for the work. The naming rule is fine.
My concern is how much code it needs.
Why the code is heavy: the rename happens at the end, right before the LLM call. After that point, the rest of the code no longer knows the tool. So we add helpers to find the old name again.
1. sanitize() encodes too much. It hex-encodes every char outside [a-zA-Z0-9] - even _ and -, which providers accept. So my.tool becomes my_2e_tool, and my_tool becomes my_5f_tool. A simple replace(Regex("[^a-zA-Z0-9_-]"), "_") is enough. We still need bound(): even simple names can pass 64 chars.
2. ToolNamingContext.kt:95 - runCatching { ... }.getOrDefault(LEGACY) is there only for the mocks. Fix the tests instead. This code hides real errors.
3. The rename runs in 4 places: AbstractLlmOperations, ToolResolutionHelper, ToolLoop, Streaming. That is why name() needs the is QualifiedTool check, and why DefaultToolDecorator has a names() helper. One rename point, earlier, removes both.
4. SupervisorAgentFactory:330 - signature.removePrefix(...) fixes the string after building it. Build the signature with the published name instead. Then CurriedActionTool.action can stop being internal.
5. PerGoalToolFactory - does 3 things at once (owner qualification, input-type discrimination, dedup changes). Also, firstOrNull { it.goals.any { g === goal } } scans every agent by reference - the platform should give the owner directly.
Suggestion: split into 4 PRs or comcar je ne vois aps l'intreret mits
To be clear: these are suggestions only, nothing mandatory. The code was just not easy to follow as a reader - that is the main reason for this comment.
(1) strategy + config + one rename point, (2) MCP goal tools, (3) input-type discrimination, (4) collision reporting for #1834.
On @igordayen's question: I agree on export.name. It is a public name the user chose - do not rewrite it. But if two agents pick the same name, report it at deploy time. Do not stay silent.
| ?.takeIf { it.isNotBlank() } | ||
| ?.let { listOf(it, toolName) } | ||
| ?: listOf(toolName) | ||
| bound(parts.joinToString("-") { sanitize(it) }, parts.joinToString("\u0000")) |
There was a problem hiding this comment.
Only the owner needs escaping
sanitize is applied to both the owner (Agent) and the tool name. The join point is the first -, so escaping the owner alone is enough to keep the split unambiguous. Escaping the tool name buys no extra uniqueness.
It does cost length. Every _ or . grows from 1 to 4 characters, and real MCP tool names use underscores heavily (brave_web_search, read_file).
Example:
ToolNamingStrategy.FULLY_QUALIFIED
.nameFor("com.acme.research.DeepResearchAgent", "brave_web_search")
// actual: com_2e_acme_2e_research_2e_DeepResearchAgent-brave__38a4886d2533
// expected: com_2e_acme_2e_research_2e_DeepResearchAgent-brave_web_searchThe encoded name is 67 characters, over MAX_NAME_LENGTH = 64, so bound() truncates it and appends a hash. The model can no longer tell which tool it is calling. Dropping the tool-side escaping brings the same name to 61 characters and it stays readable.
Suggestion: escape the owner as it is now, and on the tool side escape only characters a provider would reject (anything outside [a-zA-Z0-9_-]). That keeps _ and - at one character while every published name still matches ^[a-zA-Z0-9_-]{1,64}$.
There was a problem hiding this comment.
Agreed that escaping common characters makes names longer and less readable. However, escaping only the owner protects the owner/tool separator, not the encoding within the tool name.
For example, preserving _ while keeping the current encoding for . would produce:
read.file → AgentA-read_2e_file
read_2e_file → AgentA-read_2e_file
The current underscore escaping keeps these distinct. I suggest retaining it for now, or replacing it with a shared encoding rule that improves readability while preserving this distinction across all publication paths.
There was a problem hiding this comment.
Fair point on read.file vs read_2e_file. But that collision needs a . in a tool name, and that case is very rare today:
@LlmTool/@Toolnames come from method names, which cannot contain..- Spring AI's default MCP naming (
McpToolUtils.format) already strips invalid characters, so an MCPread.filereaches us asreadfile. - OpenAI and Anthropic reject
.in tool names anyway.
So we would pay a real cost on every tool (brave_web_search → 67 chars → truncated + hash) to protect a name that almost never exists.
Proposal that keeps your guarantee:
- Owner: keep the current escaping (it protects the
-separator). - Tool name: if it already matches
[a-zA-Z0-9_-], keep it as-is. Otherwise replace invalid characters and append a short hash of the original name (read.file→read_file_a1b2c3). Two different originals can no longer collide, and normal names stay readable.
Or simply reject invalid tool names with a clear error, which matches what you suggested for over-long export names.
| ToolNamingStrategy.LEGACY_NAME_ONLY -> goalToolNamingStrategy.nameForGoal(goal) | ||
| ToolNamingStrategy.FULLY_QUALIFIED -> toolNamingStrategy.nameFor(ownerName, goal.name) | ||
| } | ||
| exportName != null -> toolNamingStrategy.nameFor(exportName, discriminator) |
There was a problem hiding this comment.
Agreed with @tuannx on the principle: export.name is a public name the user chose for outside clients, so it should be published as-is, and a deploy-time warning on duplicates is the right answer rather than qualifying it.
One place does not follow that principle. With a single starting input type the name goes out untouched. With two or more it is passed through sanitize , so the pinned name is rewritten:
1 input type -> my.export
2 input types -> my_2e_export-UserInput
The -UserInput suffix is expected - one goal becomes two tools and they have to differ. Turning my.export into my_2e_export is not. Adding an input type silently renames a tool an outside client may already be calling, which is the exact thing publishing it as-is was meant to protect.
If export names are trusted, they should be trusted on both branches: my.export and my.export-UserInput.
Related, same value: line 179 skips bound() as well, so an export name over 64 characters is never truncated
There was a problem hiding this comment.
You’re right: preserving an explicit name for one input but encoding it for multiple inputs is inconsistent.
Current: my.export → my_2e_export-UserInput
Proposed: my.export → my.export-UserInput
Preserving the explicit base consistently makes sense. However, adding the suffix still changes the public name, so this does not preserve existing calls to my.export when another input type is added. That behavior needs to be explicit in the contract.
If an explicit name, including its suffix, exceeds the publication target’s limits, I would prefer a clear validation error over silently shortening it.
There was a problem hiding this comment.
Agreed on both: my.export-UserInput, and a clear error instead of truncation.
@azanux Thanks for these points. |
igordayen
changed the title
|
Thanks. @tuannx The goal is fine, no disagreement there. Point 1, export names and duplicates are covered in the inline threads. remaining
|
|
Hi @azanux @igordayen, Thank you both for the thoughtful feedback and discussion. Reflecting on the conversation, I completely agree that trying to guard against rare edge cases (like dots in tool names or collisions between To keep the PR focused, clean, and easy to maintain, I'd like to align on this simplified, fail-fast direction:
If this simplified approach looks good to you, I will proceed with updating the code, pruning the tests accordingly, and pushing the commit. |
Sounds good !! Thanks @tuannx |
6f18bcd to
335b4ef
Compare
igordayen
left a comment
igordayen
left a comment
There was a problem hiding this comment.
@tuannx - thanks for the next revision, i did not realize that it touches super critical artifacts in embabel-agent-api/src/main/kotlin/com/embabel/agent/spi/support package, which forces me to think more about how to make less intrusive and more manageable.
| resolvedTools += resolution.resolvedToolGroup.tools | ||
| } | ||
| } | ||
| val publishedTools = resolvedTools.map(name) |
There was a problem hiding this comment.
comment on how the outcome looks like
There was a problem hiding this comment.
Outdated: this PR no longer touches ToolConsumer or the LLM tool path. Only MCP goal tools are renamed.
…l tools - Introduce ToolNamingStrategy (LEGACY_NAME_ONLY, FULLY_QUALIFIED) with fail-fast validation against [a-zA-Z0-9_-] and max 64 characters. - In PerGoalToolFactory, qualify exported MCP goal tools with agent name under FULLY_QUALIFIED while preserving explicit export.name. - Propagate tool naming strategy lookup errors in sync and async MCP callback publishers. - Add configuration properties and documentation. Signed-off-by: TuanNX <tuannx87@gmail.com>
5949921 to
e8ffb4f
Compare
|
@tuannx -getting challenging to follow code without at least a brief explanation of the recent commits. |
|
@igordayen @azanux — latest revision, as simple as I could make it:
The PR description has a short before/after table. Duplicate detection remains in #1834. |
…l names Signed-off-by: TuanNX <tuannx87@gmail.com>
2432a5d to
f94a177
Compare
@tuannx - where is before / after table? Could you please elaborate on: same goal, input types UserInput + Order AgentA-search-UserInput, AgentA-search-Order Thanks |
| require(toolName.matches(TOOL_NAME_REGEX)) { | ||
| "Invalid tool name '$toolName': must match [a-zA-Z0-9_-]" | ||
| } | ||
| val prefix = ownerName?.takeIf { it.isNotBlank() }?.let { sanitizeOwner(it) } |
There was a problem hiding this comment.
please explain role of prefix by supplying examples
| return published | ||
| } | ||
|
|
||
| /** Hex-escape owner chars outside ASCII letters/digits (e.g. `.` becomes `_2e_`) to keep owners distinct. */ |
| With `FULLY_QUALIFIED`: | ||
|
|
||
| * Goal `com.acme.AgentA.search` publishes as `AgentA-search`. | ||
| * A goal with several starting input types publishes one tool per type: `AgentA-search-UserInput`, `AgentA-search-Order`. |
There was a problem hiding this comment.
example of goal, please, java or kotlin
| startingInputTypes: Set<Class<*>> = setOf(UserInput::class.java, MagicVictim::class.java), | ||
| ): List<GoalTool<*>> { | ||
| val agentPlatform = IntegrationTestUtils.dummyAgentPlatform() | ||
| agentPlatform.deploy( |
There was a problem hiding this comment.
Is it a single or multiple goals?
| } | ||
|
|
||
| @Test | ||
| fun `fully qualified naming keeps same goal names from different agents`() { |
There was a problem hiding this comment.
KDOC please. why FQN would affect original goal names
| ) | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
KDOC with example, name before / after
|
Closing this PR. Thanks @igordayen @azanux @deleSerna for the detailed reviews. The change has grown hard to follow in one PR. I'll split it into smaller, self-contained PRs. Each one will go through self-review and a PR on my fork first, before I raise it here. Duplicate-name detection stays tracked in #1834. |
|
FROM CODEX:
|
Summary
Opt-in naming for goal tools exported through MCP, so goals from different agents get distinct names.
Before (default
LEGACY_NAME_ONLY) vs after (FULLY_QUALIFIED), application namemyapp:com.acme.AgentA.searchmyapp_AgentA_searchAgentA-searchcom.acme.AgentB.searchmyapp_AgentB_searchAgentB-searchcom.acme.AgentA.searchacceptingUserInputorOrdermyapp_AgentA_searchtwiceAgentA-search-UserInput,AgentA-search-Orderexport.name = "my.export"my.exportmy.export(unchanged)Names over 64 characters fail at startup instead of being truncated. Default behaviour is unchanged.
Scope
ToolNamingStrategy(new,api.tool) +embabel.agent.platform.tools.naming-strategypropertyPerGoalToolFactoryapplies it; sync/async MCP publishers inject it fromAgentPlatformPropertiesspi/supportNot in this PR
Duplicate-name detection across the full tool list (incl.
_confirm) — #1834.Related: #1990.
🤖 Generated with Claude Code