-
Notifications
You must be signed in to change notification settings - Fork 399
#1741 - enhancement: improve agent registration failure messages and empty shell listing. #1786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
90183b6
6ea03f1
a8fe741
4dbb8ba
594d139
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,8 +216,9 @@ class AgentMetadataReader( | |
| } | ||
|
|
||
| if (actionMethods.isEmpty() && goals.isEmpty() && conditionMethods.isEmpty()) { | ||
| logger.warn( | ||
| "❓No methods annotated with @{} or @{} and no goals defined on {}", | ||
| logger.warn("❓{} {} is not registered due to no methods annotated with @{} or @{} and no goals defined on {}", | ||
| if (agenticInfo.isAgent()) "Agent" else "Agentic component", | ||
| agenticInfo.agentName(), | ||
| Action::class.simpleName, | ||
| Condition::class.simpleName, | ||
| targetType.name, | ||
|
|
@@ -230,15 +231,16 @@ class AgentMetadataReader( | |
| if (plannerType == PlannerType.SUPERVISOR) { | ||
| // Find the goal action (the action with @AchievesGoal) | ||
| if (goalActions.isEmpty()) { | ||
| logger.warn( | ||
| "SUPERVISOR planner requires at least one @AchievesGoal action on {}", | ||
| logger.warn("❓Agent {} is not registered: SUPERVISOR planner requires at least one @AchievesGoal action on {}", | ||
| agenticInfo.agentName(), | ||
| targetType.name, | ||
| ) | ||
| return null | ||
| } | ||
| if (goalActions.size > 1) { | ||
| logger.warn( | ||
| "SUPERVISOR planner currently supports only one @AchievesGoal action, found {} on {}", | ||
| "❓Agent {} is not registered: SUPERVISOR planner currently supports only one @AchievesGoal action, found {} on {}", | ||
| agenticInfo.agentName(), | ||
| goalActions.size, | ||
| targetType.name, | ||
| ) | ||
|
|
@@ -264,8 +266,8 @@ class AgentMetadataReader( | |
| val typeNames = distinctGoalTypes.joinToString { it.simpleName.ifEmpty { it.name } } | ||
| if (restrictedGoals) { | ||
| 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", | ||
| agenticInfo.agentName(), | ||
| typeNames, | ||
| ) | ||
| return null | ||
|
|
@@ -303,9 +305,12 @@ class AgentMetadataReader( | |
| if (plannerType == PlannerType.GOAP && agenticInfo.isAgent()) { | ||
| val validationResult = agentValidationManager.validate(agent) | ||
| if (!validationResult.isValid) { | ||
| logger.warn("Agent validation failed:\n${validationResult.errors.joinToString("\n")}") | ||
| // 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{}", | ||
|
simeshev marked this conversation as resolved.
|
||
| agent.name, | ||
| validationResult.errors.joinToString("\n"), | ||
| ) | ||
| return null | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Not sure but the bug seems to be in if (plan == null || plan.actions.isEmpty()) { // → NO_PATH_TO_GOAL
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I can reduce the scope of thange and comment
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@azanux - per my understanding, the only open item as of now.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes current behavior...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @igordayen Is it Ok if I create a separate ticket for
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @deleSerna - should be fine, post-release 1.0.0. The current PR then needs to change logging statements accordingly and also elaborate on changes in Retry. thanks |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,201 @@ | ||
| /* | ||
| * 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.shell | ||
|
|
||
| import com.embabel.agent.api.common.ToolsStats | ||
| import com.embabel.agent.api.common.autonomy.Autonomy | ||
| import com.embabel.agent.api.common.autonomy.AutonomyProperties | ||
| import com.embabel.agent.core.Agent | ||
| import com.embabel.agent.core.AgentPlatform | ||
| import com.embabel.agent.shell.config.ShellProperties | ||
| import com.embabel.agent.spi.logging.ColorPalette | ||
| import com.embabel.agent.spi.logging.LoggingPersonality | ||
| import com.embabel.common.ai.model.ModelProvider | ||
| import com.fasterxml.jackson.databind.ObjectMapper | ||
| import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper | ||
| import io.mockk.every | ||
| import io.mockk.mockk | ||
| import org.junit.jupiter.api.Assertions.assertEquals | ||
| import org.junit.jupiter.api.Assertions.assertFalse | ||
| import org.junit.jupiter.api.Assertions.assertTrue | ||
| import org.junit.jupiter.api.BeforeEach | ||
| import org.junit.jupiter.api.Nested | ||
| import org.junit.jupiter.api.Test | ||
| import org.springframework.context.ConfigurableApplicationContext | ||
| import org.springframework.core.env.ConfigurableEnvironment | ||
|
|
||
| /** | ||
| * Regression tests for the agents shell command (issue #1741): | ||
| * empty-state messaging and preserved good output when agents exist. | ||
| */ | ||
| class ShellCommandsAgentsTest { | ||
|
|
||
| private val autonomy: Autonomy = mockk(relaxed = true) | ||
| private val modelProvider: ModelProvider = mockk(relaxed = true) | ||
| private val terminalServices: TerminalServices = mockk(relaxed = true) | ||
| private val environment: ConfigurableEnvironment = mockk(relaxed = true) | ||
| private val objectMapper: ObjectMapper = jacksonObjectMapper() | ||
| private val colorPalette: ColorPalette = object : ColorPalette { | ||
| override val highlight: Int = 0xbeb780 | ||
| override val color2: Int = 0x7da17e | ||
| } | ||
|
simeshev marked this conversation as resolved.
Outdated
|
||
| private val loggingPersonality: LoggingPersonality = mockk(relaxed = true) { | ||
| every { logger } returns mockk(relaxed = true) | ||
| every { colorPalette } returns this@ShellCommandsAgentsTest.colorPalette | ||
| } | ||
| private val toolsStats: ToolsStats = mockk(relaxed = true) | ||
| private val context: ConfigurableApplicationContext = mockk(relaxed = true) | ||
| private val agentPlatform: AgentPlatform = mockk(relaxed = true) | ||
| private val autonomyProperties: AutonomyProperties = mockk(relaxed = true) | ||
| private val shellProperties = ShellProperties() | ||
|
|
||
| private lateinit var shellCommands: ShellCommands | ||
|
|
||
| @BeforeEach | ||
| fun setUp() { | ||
| every { autonomy.agentPlatform } returns agentPlatform | ||
| every { autonomy.properties } returns autonomyProperties | ||
| shellCommands = ShellCommands( | ||
| autonomy = autonomy, | ||
| modelProvider = modelProvider, | ||
| terminalServices = terminalServices, | ||
| environment = environment, | ||
| objectMapper = objectMapper, | ||
| colorPalette = colorPalette, | ||
| loggingPersonality = loggingPersonality, | ||
| toolsStats = toolsStats, | ||
| context = context, | ||
| shellProperties = shellProperties, | ||
| asyncer = mockk(relaxed = true), | ||
| ) | ||
| } | ||
|
|
||
| private fun agent( | ||
| name: String, | ||
| description: String, | ||
| provider: String = "test-provider", | ||
| ) = Agent( | ||
| name = name, | ||
| provider = provider, | ||
| description = description, | ||
| actions = emptyList(), | ||
| goals = emptySet(), | ||
| ) | ||
|
|
||
| /** Strip ANSI escape codes so assertions ignore bold/color styling. */ | ||
| private fun String.stripAnsi(): String = | ||
| replace(Regex("\u001B\\[[;\\d]*m"), "") | ||
|
|
||
| @Nested | ||
| inner class EmptyAgents { | ||
|
|
||
| @Test | ||
| fun `shows no agents registered when platform has no agents`() { | ||
| every { agentPlatform.agents() } returns emptyList() | ||
|
|
||
| val result = shellCommands.agents() | ||
|
|
||
| assertEquals("No agents registered", result) | ||
| } | ||
| } | ||
|
|
||
| @Nested | ||
| inner class RegisteredAgents { | ||
|
|
||
| @Test | ||
| fun `single agent has detailed listing and summary`() { | ||
| val demo = agent(name = "demo-agent", description = "A demo agent") | ||
| every { agentPlatform.agents() } returns listOf(demo) | ||
|
|
||
| val result = shellCommands.agents().stripAnsi() | ||
|
|
||
| assertTrue(result.contains("Agents:"), "Expected Agents header, got: $result") | ||
| assertTrue(result.contains("description: A demo agent"), "Expected detailed description, got: $result") | ||
| assertTrue(result.contains("provider: test-provider"), "Expected provider in detail, got: $result") | ||
| assertTrue(result.contains("name: demo-agent"), "Expected name in detail, got: $result") | ||
| assertTrue(result.contains("Summary"), "Expected Summary section, got: $result") | ||
| assertTrue( | ||
| result.contains("demo-agent: A demo agent"), | ||
| "Expected name:description summary line, got: $result", | ||
| ) | ||
| assertTrue( | ||
| result.indexOf("Agents:") < result.indexOf("Summary"), | ||
| "Detail listing should come before Summary, got: $result", | ||
| ) | ||
| assertTrue( | ||
| result.indexOf("description: A demo agent") < result.indexOf("Summary"), | ||
| "Verbose detail should appear before Summary, got: $result", | ||
| ) | ||
| assertFalse( | ||
| result.contains("No agents registered"), | ||
| "Should not claim no agents when agents exist: $result", | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `multiple agents are separated and all appear in summary`() { | ||
| val poet = agent(name = "Poet", description = "Write poems") | ||
| val coder = agent(name = "Coder", description = "Answer coding questions") | ||
| every { agentPlatform.agents() } returns listOf(poet, coder) | ||
|
|
||
| val result = shellCommands.agents().stripAnsi() | ||
| val separator = "-".repeat(shellProperties.lineLength) | ||
|
|
||
| assertTrue(result.contains("description: Write poems"), "Expected Poet detail, got: $result") | ||
| assertTrue( | ||
| result.contains("description: Answer coding questions"), | ||
| "Expected Coder detail, got: $result", | ||
| ) | ||
| assertTrue( | ||
| result.contains(separator), | ||
| "Expected separator between agents of length ${shellProperties.lineLength}, got: $result", | ||
| ) | ||
| assertTrue( | ||
| result.indexOf("description: Write poems") < result.indexOf(separator) && | ||
| result.indexOf(separator) < result.indexOf("description: Answer coding questions"), | ||
| "Separator should sit between agent detail blocks, got: $result", | ||
| ) | ||
|
|
||
| val summaryStart = result.indexOf("Summary") | ||
| assertTrue(summaryStart >= 0, "Expected Summary section, got: $result") | ||
| val summary = result.substring(summaryStart) | ||
| assertTrue(summary.contains("Poet: Write poems"), "Expected Poet in summary: $summary") | ||
| assertTrue( | ||
| summary.contains("Coder: Answer coding questions"), | ||
| "Expected Coder in summary: $summary", | ||
| ) | ||
| assertTrue( | ||
| summary.indexOf("Poet: Write poems") < summary.indexOf("Coder: Answer coding questions"), | ||
| "Summary should preserve agent order, got: $summary", | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `summary is only name and description lines after the Summary header`() { | ||
| val agent = agent(name = "demo-agent", description = "A demo agent") | ||
| every { agentPlatform.agents() } returns listOf(agent) | ||
|
|
||
| val result = shellCommands.agents().stripAnsi() | ||
| val summaryLines = result.substringAfter("Summary").trim().lines() | ||
|
|
||
| assertEquals( | ||
| listOf("demo-agent: A demo agent"), | ||
| summaryLines, | ||
| "Summary should be only concise name:description lines", | ||
| ) | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.