fix(core): log an error when a duplicate name silently drops a goal, action or tool

[tuannx]

Updated:
AgentPlatform exposes goals, actions and conditions as flat views keyed by name, and
de-duplicates them with distinctBy { it.name }. Two agents declaring different elements
under one name therefore lose one of them: both agents deploy, agents() reports both,
and the capability simply stops existing with nothing in the log. safelyGetTools and
safelyGetToolsFrom do the same for tool names.

This PR does not change any of that. It makes the loss visible.

A shared helper, distinctByNameReportingCollisions, keeps the existing distinctBy
semantics — first declaration wins — and logs an ERROR when the element being dropped
differs from the one being kept. Two agents declaring the very same goal still collapse
silently, because nothing is lost.

Applied to:

• AgentPlatform.goals / actions / conditions / domainTypes — compared by value,
  which is meaningful for those data classes
• toolUtils.safelyGetTools and safelyGetToolsFrom — compared by identity, since Tool
  is an interface with no value semantics

DefaultToolLoop.addTools is deliberately left alone: re-injecting a tool group during
the loop is idempotent by design, and the decorator immediately above it produces new
instances, so identity comparison would report on every iteration.

Reporting is once per collision. safelyGetTools runs per LLM operation and the
platform's aggregated views are recomputed on every read, so an unguarded log would repeat
indefinitely. The trade-off is that a collision which disappears and returns is not
reported a second time until restart.

What this does not do

• It does not stop the element being dropped — that behaviour is unchanged
• It does not reject a deployment, and adds no exception
• It does not change any published name, so MCP tool names, MCP prompt names and A2A skill
  ids are untouched

No breaking change.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[igordayen]

@tuannx - thanks for reporting.
Per the process, could you please create an issue for this PR?
Also, could you please help interpret the write-up on the issue, preferably with simpler prose? A bit harder to understand the root cause hidden by the write-up potentially generated by AI.

Also - appears something got broken due to migration, or its existing issue; could you please try to dig into the history of the problem?
Thank you for contributing!

[tuannx]

Thanks for the review. Issue opened: #1834

On the history question — this is not from the migration. It goes back to
a977cfd5b,
which changed how the platform de-duplicates:

- get() = agents().flatMap { it.actions }.distinct()
+ get() = agents().flatMap { it.actions }.distinctBy { it.name }

- get() = agents().flatMap { it.goals }.toSet()
+ get() = agents().flatMap { it.goals }.distinctBy { it.name }.toSet()

Before: de-duplicate by value. Two identical goals collapse into one, which is
right.
After: de-duplicate by name. Two different goals sharing a name also collapse,
and one meaning is lost.

That is the only commit that ever touched those lines. conditions was already name-based before it.

The fix restores that distinction rather than removing the de-duplication:
elements equal by value are still collapsed, elements that merely share a name
are rejected at deploy time.

Sorry about the dense write-up. Issue and PR description are rewritten in
plainer terms.

One decision I would rather you made than inherited from a diff: deploy()
currently throws. I picked that because deploy() is an explicit call, so
silently not deploying seemed worse. The lighter option is to log an ERROR and
skip the conflicting agent, matching the direction in #1786 — existing apps keep
booting, but still lose the capability, just loudly. Happy to switch.

[igordayen]

Thank you, @tuannx - will follow up. Best regards!

[igordayen]

One decision I would rather you made than inherited from a diff: deploy()
currently throws. I picked that because deploy() is an explicit call, so
silently not deploying seemed worse. The lighter option is to log an ERROR and
skip the conflicting agent, matching the direction in #1786 — existing apps keep
booting, but still lose the capability, just loudly. Happy to switch.

@alexheifetz - could you please advise - conflicting goals:

• flag ERROR, continue booting
• throw an exception and stop booting?

Should probably align with logic:

val achievableGoalValidationResult = AchievableGoalValidator().validate(agenticInfo.agentName(), targetType, instance, requireInterfaceDeserializationAnnotations)
        if(!achievableGoalValidationResult.isValid) {
            val errorMsg = achievableGoalValidationResult.errors.map { it.message }.joinToString { it }
            logger.error(errorMsg)
            return null
        }

from PR #1801
@deleSerna - FYI

[igordayen]

Before: de-duplicate by value. Two identical goals collapse into one, which is
right.

@tuannx - could you please elaborate on "goal value"? Thank you.

[igordayen]

@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.
and have documented the property
embabel.agent.platform.exit-on-error
Would it work 4all?
Thanks

[deleSerna]

from PR #1801

AchievesGoal without Action should definitely stop the Agent as it's spec violation and it can be easily fix while developing the Agent itself.
But the issue mentioned here seems a bit more tricky as goals/conditions can randomly be selected/dropped and that seems bad to me . We should throw an error if the developer can fix that duplicated goal/condition by renaming them but could the developer always do that?

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.

[igordayen]

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. B

==> That's the reason for suggesting having an error policy configurable. thanks

[tuannx]

@igordayen @deleSerna There's a third option, and the annotation path already does it. Annotated goals
are named after their agent:

name = "${stateClass.simpleName}.${method.name}"     // "WeatherAgent.myGoal"

and the tool naming strategy expects that shape:

/** "com.myco.MyAgent.myGoal" becomes "MyAgent_myGoal". */

So two annotated agents can both have a same goal and never collide. Only the
DSL takes the name literally — and AgentBuilder already holds the agent name:

    Goal(
(-)      name = name,
(+)       name = "${this@AgentBuilder.name}.$name",

That makes the collision impossible instead of reporting it better, and nobody
has to rename anything.

If you agree with the direction I'll rework this PR — the deploy-time check goes
away, and the test asserts both goals survive instead of asserting a rejection.

[deleSerna]

That's the reason for suggesting having an error policy configurable.

@igordayen But that would not also fix the real issue when the conflicting actions/goal are coming from multiple agents right?

name = "${stateClass.simpleName}.${method.name}"

This could also still result in duplicate name as it's still simpleName not Name .
Even if we use 'Name, 'Name+ method.name still cause duplicate names unless we use signature. Therefore, we should make sure that Name+ method.name, still not esult in duplicate names within the agent itself.

@igordayen @alexheifetz IMO, we should go in the direction suggested by @tuannx but should use 'Name+ method.name` every where. But that looks like a much bigger change. Therefore, IMO, need a bit more thought before implementing it.

[igordayen]

Three points raised:

1. Error policy doesn't fully address the problem — Even with a configurable
   error policy, it wouldn't solve the
   collision when conflicting actions/goals come from multiple agents rather than
   within a single agent.
2. simpleName still risks collisions — The proposed naming scheme
   ${stateClass.simpleName}.${method.name} uses simpleName, which can still
   duplicate. Even upgrading to Name (fully qualified), Name + method. name can
   still collide unless you include the full method signature.
3. Recommendation: use Name + method.name everywhere, but with caution —
   @deleSerna agrees with @tuannx tuannx's direction and suggests using Name + method. name
   universally, but flags it as a bigger change that needs more thought before
   implementation.

@tuannx - concrete naming examples, please, to substantiate the idea and impact assessment.
Thanks

[igordayen]

Consider renaming "fix(core): reject deployments that would silently drop goals or actions- #1833
" to "fix(core): reject deployments that would silently drop goals or actions DUE TO DUPLICATES" #1833

[tuannx]

Before we choose between throwing, an error policy, or qualification, could we do a
small test-only PR first?

@igordayen asked for concrete naming examples and an impact assessment. We can generate
those instead of writing them by hand: a test that records every name Embabel publishes
externally into a checked-in file.

mcp-tool   StarNewsFinder_findNewsStories
a2a-skill  embabel_goal_com.embabel.examples.StarNewsFinder.findNewsStories

Any naming change then shows up as a diff, so we can see what breaks on the wire before
deciding anything.

We have changed these names unnoticed before: #599 (Claude Desktop rejected a tool name)
and #306 (the $embabel_agent_api suffix). Neither had a test.

It would also show that two agents in different packages with the same state class name
still produce the same MCP tool name, because the naming strategy keeps only the last
two segments.

Happy to open it. This PR would then rebase on top.

[igordayen]

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?

[deleSerna]

Was actually inquiring about all patterns on validation logic in the agent validation package
Please compile full documentation on known agent validators behavior for consistency

DefaultAgentStructureValidator currently report errors ( agent booting won’t stop) for the following cases:

• no actions, conditions, or goals defined
• Missing goals
• Duplicate action names
• Actions has preconditions with multiple parameters ( not sure why this is an issue)

AgentMetadataReader stop the agent for following cases:

• Missing EmbabelComponent or Agent annotation
• Both @agentic and @agent annotations present
• No description provided on the Agent
• No actions, conditions, or goals defined
  • Duplicate as it already there on DefaultAgentStructureValidator
• SuperVisor planner has more than one @AchievesGoal
• If embabel.agent.platform.planner.restricted-goals is true then all goals should return same type.
• @AchievesGoal cannot be applied to void-returning @action method

AgentMetadataReader reports errors for following case

• No goal defined

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?

[igordayen]

@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

[igordayen]

@tuannx - Is this PR valid then? As the analysis attached to the issues clearly states that deployments should not be rejected.

[tuannx]

@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!

[deleSerna]

Please add a doc for return too.

[deleSerna]

May be a configurable property?

[deleSerna]

I am wondering whether printing the name enough here to identify which one is dropped.
Instead of just printing the name , may we print some kind of full name so that, it's easily identifiable which item is it. I have not done a thorough check but looks like all of them may be implementing HasInfoString, if that is the case then we can print infoString instead of just name.

[igordayen]

Please cross-check with Junits, thanks