diff --git a/a2a/src/main/java/com/google/adk/a2a/executor/AgentExecutor.java b/a2a/src/main/java/com/google/adk/a2a/executor/AgentExecutor.java index 57a0d9db2..618888c4c 100644 --- a/a2a/src/main/java/com/google/adk/a2a/executor/AgentExecutor.java +++ b/a2a/src/main/java/com/google/adk/a2a/executor/AgentExecutor.java @@ -29,6 +29,7 @@ import com.google.adk.runner.Runner; import com.google.adk.sessions.BaseSessionService; import com.google.adk.sessions.Session; +import com.google.common.base.Ascii; import com.google.common.collect.ImmutableList; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.genai.types.Content; @@ -66,6 +67,18 @@ public class AgentExecutor implements io.a2a.server.agentexecution.AgentExecutor private static final Logger logger = LoggerFactory.getLogger(AgentExecutor.class); private static final String USER_ID_PREFIX = "A2A_USER_"; private static final String A2A_METADATA_KEY = "a2a_metadata"; + + /** + * Env var that puts exception text back into the failure message sent to the peer. + * + *

Off by default, and meant for local debugging only: enabling it on a network-reachable + * deployment restores the disclosure {@link #failedMessage} exists to prevent. + */ + private static final String DEBUG_ERRORS_ENV_VAR = "ADK_DEBUG_ERRORS"; + + /** Length of the correlation id, matching {@code new_error_id()} in adk-python. */ + private static final int ERROR_ID_LENGTH = 12; + private final Map activeTasks = new ConcurrentHashMap<>(); private final Runner.Builder runnerBuilder; private final AgentExecutorConfig agentExecutorConfig; @@ -232,13 +245,7 @@ public void execute(RequestContext ctx, EventQueue eventQueue) { .ignoreElements() .materialize() .flatMapCompletable( - notification -> { - Throwable error = notification.getError(); - if (error != null) { - logger.error("Runner failed to execute", error); - } - return handleExecutionEnd(ctx, error, eventQueue); - }) + notification -> handleExecutionEnd(ctx, notification.getError(), eventQueue)) .doFinally(() -> cleanupTask(ctx.getTaskId())) .subscribe( () -> {}, @@ -250,7 +257,16 @@ public void execute(RequestContext ctx, EventQueue eventQueue) { private Completable handleExecutionEnd( RequestContext ctx, Throwable error, EventQueue eventQueue) { TaskState state = error != null ? TaskState.FAILED : TaskState.COMPLETED; - Message message = error != null ? failedMessage(ctx, error) : null; + Message message = null; + if (error != null) { + // The peer is not trusted with the throwable: exception text routinely names absolute + // filesystem paths, class and module locations, configuration values and echoed request + // payloads, none of which the caller needs and all of which are useful reconnaissance. It is + // logged here in full under a short opaque id; the peer gets only that id. + String errorId = newErrorId(); + logger.error("Runner failed to execute [error_id={}]", errorId, error); + message = failedMessage(ctx, error, errorId); + } TaskStatusUpdateEvent initialEvent = new TaskStatusUpdateEvent.Builder() .taskId(ctx.getTaskId()) @@ -304,16 +320,66 @@ private Maybe prepareSession( })); } - private static Message failedMessage(RequestContext context, Throwable e) { + /** + * Builds the failure message handed back to the remote peer. + * + *

It carries {@code errorId} rather than {@code e.getMessage()}, so an operator handed the id + * can find the real stack trace in the log while the peer learns nothing about the host. Set + * {@code ADK_DEBUG_ERRORS=1} to put the exception text back into the response while debugging + * locally. + */ + private static Message failedMessage(RequestContext context, Throwable e, String errorId) { return new Message.Builder() .messageId(UUID.randomUUID().toString()) .contextId(context.getContextId()) .taskId(context.getTaskId()) .role(Message.Role.AGENT) - .parts(ImmutableList.of(new TextPart(e.getMessage()))) + .parts(ImmutableList.of(new TextPart(failureText(e, errorId, debugErrorsEnabled())))) .build(); } + /** + * Returns a short opaque id tying the peer's failure message to the logged throwable. + * + *

{@value #ERROR_ID_LENGTH} hex characters, the same shape as {@code new_error_id()} in + * adk-python, so an operator sees the same kind of id whichever runtime produced it. + */ + private static String newErrorId() { + return UUID.randomUUID().toString().replace("-", "").substring(0, ERROR_ID_LENGTH); + } + + /** + * Returns the failure text that is safe to hand to the remote peer. + * + * @param includeDetail whether to append the exception type and message; see {@link + * #DEBUG_ERRORS_ENV_VAR}. + */ + static String failureText(Throwable e, String errorId, boolean includeDetail) { + String text = "Agent execution failed. (error_id: " + errorId + ")"; + if (includeDetail) { + text = text + ": " + e.getClass().getName() + ": " + e.getMessage(); + } + return text; + } + + private static boolean debugErrorsEnabled() { + return debugErrorsEnabled(System.getenv(DEBUG_ERRORS_ENV_VAR)); + } + + /** + * Returns whether {@code value}, as read from {@link #DEBUG_ERRORS_ENV_VAR}, turns the detail + * back on. Unset or unrecognized means off, matching {@code is_env_enabled} in adk-python. + * + *

Split from the env lookup so both outcomes are testable: {@code System.getenv} cannot be set + * from a test in-process. + */ + static boolean debugErrorsEnabled(String value) { + if (value == null) { + return false; + } + return value.equals("1") || Ascii.equalsIgnoreCase(value, "true"); + } + // Processor that will process all events related to the one runner invocation. private static class EventProcessor { private final String runArtifactId; diff --git a/a2a/src/test/java/com/google/adk/a2a/executor/AgentExecutorTest.java b/a2a/src/test/java/com/google/adk/a2a/executor/AgentExecutorTest.java index 99b12286e..68cb196f1 100644 --- a/a2a/src/test/java/com/google/adk/a2a/executor/AgentExecutorTest.java +++ b/a2a/src/test/java/com/google/adk/a2a/executor/AgentExecutorTest.java @@ -63,6 +63,10 @@ @RunWith(JUnit4.class) public final class AgentExecutorTest { + /** A throwable message shaped like the ones that leak host detail. */ + private static final String SECRET_ERROR = + "Runner error: /home/victim/.config/adk/credentials.json (No such file)"; + private EventQueue eventQueue; private List enqueuedEvents; private TestAgent testAgent; @@ -137,7 +141,7 @@ public void createAgentExecutor_noAgentExecutorConfig_throwsException() { public void execute_withBeforeExecuteCallback_cancelsExecutionOnError() { // If callback returns error, execution should stop/fail. Callbacks.BeforeExecuteCallback callback = - ctx -> Single.error(new RuntimeException("Cancelled")); + ctx -> Single.error(new RuntimeException(SECRET_ERROR)); AgentExecutorConfig config = AgentExecutorConfig.builder().beforeExecuteCallback(callback).build(); @@ -162,7 +166,10 @@ public void execute_withBeforeExecuteCallback_cancelsExecutionOnError() { assertThat(statusEvent.getStatus().state().toString()).isEqualTo("FAILED"); assertThat(statusEvent.getStatus().message().getParts().get(0)).isInstanceOf(TextPart.class); TextPart textPart = (TextPart) statusEvent.getStatus().message().getParts().get(0); - assertThat(textPart.getText()).contains("Cancelled"); + // The remote peer gets a correlation id for the logged throwable, not its + // message -- see AgentExecutor#failedMessage. + assertThat(textPart.getText()).startsWith("Agent execution failed. (error_id: "); + assertThat(textPart.getText()).doesNotContain(SECRET_ERROR); } @Test @@ -289,7 +296,7 @@ public void execute_withAfterExecuteCallback_modifiesStatus() { @Test public void execute_runnerFails_registersFailedEvent() { - testAgent.setEventsToEmit(Flowable.error(new RuntimeException("Runner error"))); + testAgent.setEventsToEmit(Flowable.error(new RuntimeException(SECRET_ERROR))); AgentExecutor executor = new AgentExecutor.Builder() .agentExecutorConfig(AgentExecutorConfig.builder().build()) @@ -316,7 +323,48 @@ public void execute_runnerFails_registersFailedEvent() { assertThat(statusEvent.getStatus().state()).isEqualTo(TaskState.FAILED); assertThat(statusEvent.getStatus().message().getParts().get(0)).isInstanceOf(TextPart.class); TextPart textPart = (TextPart) statusEvent.getStatus().message().getParts().get(0); - assertThat(textPart.getText()).isEqualTo("Runner error"); + // A runner failure is reported to the peer as a correlation id only: the + // throwable's message names host paths and is for the server log. The id is + // 12 hex characters, the shape adk-python emits. + assertThat(textPart.getText()) + .matches("Agent execution failed\\. \\(error_id: [0-9a-f]{12}\\)"); + assertThat(textPart.getText()).doesNotContain(SECRET_ERROR); + assertThat(textPart.getText()).doesNotContain("/home/victim"); + } + + @Test + public void failureText_withoutDebug_carriesOnlyTheCorrelationId() { + String text = AgentExecutor.failureText(new RuntimeException(SECRET_ERROR), "abc-123", false); + + assertThat(text).isEqualTo("Agent execution failed. (error_id: abc-123)"); + } + + @Test + public void failureText_withDebug_carriesTheThrowableDetail() { + // ADK_DEBUG_ERRORS=1 is the documented opt-in for local debugging. + String text = AgentExecutor.failureText(new RuntimeException(SECRET_ERROR), "abc-123", true); + + assertThat(text).startsWith("Agent execution failed. (error_id: abc-123): "); + assertThat(text).contains("java.lang.RuntimeException"); + assertThat(text).contains(SECRET_ERROR); + } + + @Test + public void debugErrorsEnabled_recognizesTheDocumentedValues() { + assertThat(AgentExecutor.debugErrorsEnabled("1")).isTrue(); + assertThat(AgentExecutor.debugErrorsEnabled("true")).isTrue(); + assertThat(AgentExecutor.debugErrorsEnabled("TRUE")).isTrue(); + assertThat(AgentExecutor.debugErrorsEnabled("True")).isTrue(); + } + + @Test + public void debugErrorsEnabled_defaultsToOff() { + // Anything else leaves the redaction in place, including an unset variable. + assertThat(AgentExecutor.debugErrorsEnabled(null)).isFalse(); + assertThat(AgentExecutor.debugErrorsEnabled("")).isFalse(); + assertThat(AgentExecutor.debugErrorsEnabled("0")).isFalse(); + assertThat(AgentExecutor.debugErrorsEnabled("false")).isFalse(); + assertThat(AgentExecutor.debugErrorsEnabled("yes")).isFalse(); } @Test