Skip to content

#1741 - enhancement: improve agent registration failure messages and empty shell listing.#1786

Open
simeshev wants to merge 4 commits into
mainfrom
enhancement/1741-improve-the-warning-message-when-agent-does-not-meet-the-necessary-criteria-to-register-as-an-agent
Open

#1741 - enhancement: improve agent registration failure messages and empty shell listing.#1786
simeshev wants to merge 4 commits into
mainfrom
enhancement/1741-improve-the-warning-message-when-agent-does-not-meet-the-necessary-criteria-to-register-as-an-agent

Conversation

@simeshev

Copy link
Copy Markdown
Collaborator

No description provided.

… empty shell listing.

Signed-off-by: Slava Imeshev <imeshev@yahoo.com>

Copilot AI 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.

Pull request overview

This PR addresses issue #1741 by improving user-facing output when listing agents in the shell (including an empty state) and by making agent auto-registration failures more explicit and actionable through clearer warnings and stricter validation.

Changes:

  • Add an explicit empty-state response for the agents shell command and preserve detailed output + a concise summary when agents exist.
  • Improve agent registration failure warnings and enforce non-registration on GOAP validation failures, with new regression tests covering these cases.
  • Adjust logging severity when an agent process runs with no goals (where goals are required).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
embabel-agent-shell/src/test/kotlin/com/embabel/agent/shell/ShellCommandsAgentsTest.kt Adds regression tests for agents command empty-state and detailed/summary output.
embabel-agent-shell/src/main/kotlin/com/embabel/agent/shell/ShellCommands.kt Implements empty-state handling and uses a Summary section for agents/actions listings.
embabel-agent-api/src/test/kotlin/com/embabel/agent/api/annotation/support/testTypes.kt Adds new annotated test agent types to exercise registration/validation failure cases.
embabel-agent-api/src/test/kotlin/com/embabel/agent/api/annotation/support/AgentMetadataReaderMetadataTest.kt Adds OutputCapture-based assertions ensuring explicit “not registered” warnings are logged.
embabel-agent-api/src/main/kotlin/com/embabel/agent/core/support/AbstractAgentProcess.kt Raises log level to error when a goals-required process has no goals.
embabel-agent-api/src/main/kotlin/com/embabel/agent/api/annotation/support/AgentMetadataReader.kt Improves warning messages for registration failures and returns null on GOAP validation failure.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

simeshev added 2 commits July 13, 2026 16:32
…puts

Nullable domain params skip the input precondition, so path validation
failed with NO_PATH_TO_GOAL and createAgentMetadata returned null.
Keep planner=GOAP; fix RetryActionAnnotationTest and
ActionRetryPolicyPropertiesTest fixtures only.

Signed-off-by: Slava Imeshev <imeshev@yahoo.com>
…lorPalette implementation via an anonymous ColorPalette object. Using DefaultColorPalette directly avoids duplication and keeps the test aligned if the default palette changes."

Signed-off-by: Slava Imeshev <imeshev@yahoo.com>
@igordayen

igordayen commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@deleSerna - can it be closed, as you opened the issue? thanks.

@deleSerna deleSerna 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.

can it be closed, as you opened the issue?
Yes, the PR looks good to me.

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

@simeshev - looks good, few comments to consider, thanks.

// TODO: Uncomment to strengthen validation and refactor the test if needed. Because some tests might fail.
// return null
logger.warn(
"❓Agent {} is not registered due to validation failure:\n{}",

@igordayen igordayen Jul 15, 2026

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.

The prefix Agent is applied repeatedly. Please consider constant as a prefix. thanks.

@simeshev simeshev Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The prefix Agent is applied repeatedly. Please consider constant as a prefix. thanks.

This a throw-away literal. GC will take care of it. We have a lot of them, registered, planner etc. for example. If we start replacing them with constants, it will be hard(er) to read. Plus, PR blast radius will be much bigger and I'd prefer the changes to be surgical.

I'd rather keep it.

@AchievesGoal(description = "Process the input")
@Action(actionRetryPolicyExpression = "\${retry-twice}")
fun perform(input: JavaRetryTestInput?): JavaRetryTestOutput {
fun perform(input: JavaRetryTestInput): JavaRetryTestOutput {

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.

Guaranteed not null?

@azanux azanux left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @simeshev , everything looks good, just one small blocker to check.


Nothing to do with your PR but it could impact the PR, so it would be nice to fix it at the same time:

  • AgentMetadataReader.kt:84: + instead of += returns a new list and the result is discarded, so the error is never added. A blank @Agent(description = "") produces no warning at all: validationErrors() comes back empty and the agent registers silently.

agent.name,
validationResult.errors.joinToString("\n"),
)
return null

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Agents whose goal action takes no input (or only a nullable one) silently stop registering.
agents that failed validation used to register with a warning, now they don't register at all.

Not sure but the bug seems to be in GoapPathToCompletionValidator.kt:215:

if (plan == null || plan.actions.isEmpty()) {   // → NO_PATH_TO_GOAL

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.

@simeshev @azanux - good point, for pointing to GoapPathToCompletionValidator ==> so it will be at the very end, properly reported, if there are no actions, per my understanding; rather than updating half a dozen log statements with the same PREFIX, it actually gets accommodated in a single place, correct?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@igordayen It depends on how @simeshev resolve the behaviour change first: this PR uncommented the return null at line 313 : Agents whose goal action takes no input (or only a nullable one) used to register with a warning, and now don't register at all

On your point: partially right. Line 309 is the only warn fed by the validator, so that path is centralised. But lines others are early return nulls - validate() is only called at line 306, so they exit before it and never reach GoapPathToCompletionValidator.

logger.warn(
"Agent {} has @AchievesGoal actions returning distinct types [{}] - rejected. Set embabel.agent.platform.planner.restricted-goals=false to allow",
targetType.name,
"Agent {} is not registered due to @AchievesGoal actions returning distinct types [{}]. Set embabel.agent.platform.planner.restricted-goals=false to allow",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

agentName() returns the logical name (@Agent(name = "star-wars-fan")), not the
class, so the log gives no clue which file to open - and this is the path where the
agent gets removed, so we need it most.

@simeshev simeshev Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@azanux - Here is what agenName() does. If the name is blank, agenName() returns type name:

fun agentName(): String = (agentAnnotation?.name ?: "").ifBlank { targetType.simpleName }

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@azanux - maybe you meant: ?

                logger.warn(
                    "❓Agent {} is not registered due to validation failure:\n{}",
                    agent.name,
                    validationResult.errors.joinToString("\n"),

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If so, I've replaced agent.name with agenticInfo.agentName() that does provide type name in case the agent name annotation is blank.

@AchievesGoal(description = "Process the input")
@Action(actionRetryPolicyExpression = "\${retry-twice}")
fun firstAction(input: RetryTestInput?): RetryTestOutput {
fun firstAction(input: RetryTestInput): RetryTestOutput {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This test covers retry behaviour, not sure that input nullability have to do with it.
maybe this needed because the new return null in AgentMetadataReader stops the agent registering, rather than because the fixture was wrong?

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

@azanux - thanks for feedback, appears, morre attention required then.

…(name = "star-wars-fan")), not the class, so the log gives no clue which file to open - and this is the path where the agent gets removed, so we need it most."

Signed-off-by: Slava Imeshev <imeshev@yahoo.com>
@sonarqubecloud

Copy link
Copy Markdown

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve the warning message when Agent does not meet the necessary criteria to register as an Agent

5 participants