Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

/**
Expand Down Expand Up @@ -91,15 +92,25 @@ Map<String, Object> generateParameterSchema(Method method, Set<String> excludePa
return schema;
}

// TODO: putAll silently overwrites when different parameters define the same def key
// (e.g. different classes with the same simple name under PLAIN_DEFINITION_KEYS).
// Add value-equality check and fail-fast on true conflicts in a follow-up.
@SuppressWarnings("unchecked")
private void hoistDefs(
Map<String, Object> paramSchema, String key, Map<String, Object> target) {
Object raw = paramSchema.remove(key);
if (raw instanceof Map<?, ?> defs && !defs.isEmpty()) {
target.putAll((Map<String, Object>) defs);
Map<String, Object> normalizedDefs = (Map<String, Object>) defs;
for (Map.Entry<String, Object> entry : normalizedDefs.entrySet()) {
String defKey = entry.getKey();
Object incomingDef = entry.getValue();
Object existingDef = target.get(defKey);
if (existingDef == null) {
target.put(defKey, incomingDef);
continue;
}
if (!Objects.equals(existingDef, incomingDef)) {
throw new IllegalStateException(
"Conflicting schema definition found for key: " + defKey);
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

The IllegalStateException message will be useful for debugging schema generation failures, but it could be more actionable by including whether the conflict came from "$defs" vs "definitions" and a short hint that this can be caused by simple-name collisions under PLAIN_DEFINITION_KEYS. Consider enriching the message accordingly.

Suggested change
"Conflicting schema definition found for key: " + defKey);
"Conflicting schema definition found for key '"
+ defKey
+ "' while hoisting from '"
+ key
+ "'. This can be caused by simple-name collisions when using plain definition keys.");

Copilot uses AI. Check for mistakes.
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright 2024-2026 the original author or authors.
*
* 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 io.agentscope.core.tool;

import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.util.HashMap;
import java.util.Map;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;

@Tag("unit")
@DisplayName("ToolSchemaGenerator Tests")
class ToolSchemaGeneratorTest {

private final ToolSchemaGenerator generator = new ToolSchemaGenerator();

@Test
@DisplayName("Should throw when hoisted defs contain conflicting definition for same key")
void testHoistDefsConflict() throws Exception {
Method hoistDefsMethod =
ToolSchemaGenerator.class.getDeclaredMethod(
"hoistDefs", Map.class, String.class, Map.class);
hoistDefsMethod.setAccessible(true);

Map<String, Object> existingDef =
Map.of("type", "object", "properties", Map.of("value", Map.of("type", "string")));
Map<String, Object> conflictDef =
Map.of("type", "object", "properties", Map.of("value", Map.of("type", "integer")));

Map<String, Object> target = new HashMap<>();
target.put("Material", existingDef);
Map<String, Object> paramSchema = new HashMap<>();
paramSchema.put("$defs", Map.of("Material", conflictDef));

InvocationTargetException exception =
assertThrows(
InvocationTargetException.class,
() -> hoistDefsMethod.invoke(generator, paramSchema, "$defs", target));
assertInstanceOf(IllegalStateException.class, exception.getCause());
assertTrue(exception.getCause().getMessage().contains("Material"));
}

@Test
@DisplayName("Should allow hoisted defs when same key has equivalent definition")
void testHoistDefsEquivalent() throws Exception {
Method hoistDefsMethod =
ToolSchemaGenerator.class.getDeclaredMethod(
"hoistDefs", Map.class, String.class, Map.class);
hoistDefsMethod.setAccessible(true);

Map<String, Object> definition =
Map.of("type", "object", "properties", Map.of("value", Map.of("type", "string")));

Map<String, Object> target = new HashMap<>();
target.put("Material", definition);
Map<String, Object> paramSchema = new HashMap<>();
paramSchema.put("$defs", Map.of("Material", definition));

assertDoesNotThrow(() -> hoistDefsMethod.invoke(generator, paramSchema, "$defs", target));
assertEquals(1, target.size());
assertEquals(definition, target.get("Material"));
assertFalse(paramSchema.containsKey("$defs"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
public record DeploymentProperties(String host, int port) {

private static final Logger log = LoggerFactory.getLogger(DeploymentProperties.class);
private static final String DEFAULT_HOST = "localhost";

public static class Builder {

Expand All @@ -55,7 +56,12 @@ public DeploymentProperties build() {
try {
host = NetworkUtils.getLocalIpAddress();
log.info("Local IP address: {}", host);
} catch (SocketException ignored) {
} catch (SocketException exception) {
host = DEFAULT_HOST;
log.warn(
"Failed to resolve local IP address, fallback to default host: {}",
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

Log message grammar: "fallback" is a noun; as a verb it should be "fall back" (e.g., "...; falling back to default host"). Adjusting this improves readability/searchability in logs.

Suggested change
"Failed to resolve local IP address, fallback to default host: {}",
"Failed to resolve local IP address, falling back to default host: {}",

Copilot uses AI. Check for mistakes.
DEFAULT_HOST,
exception);
}
}
if (null == port) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand Down Expand Up @@ -109,7 +108,7 @@ void testUseLocalhostWhenGetLocalIpFails() throws SocketException {
new DeploymentProperties.Builder().port(port).build();

assertNotNull(deploymentProperties);
assertNull(deploymentProperties.host()); // host will be null since we couldn't get it
assertEquals("localhost", deploymentProperties.host());
assertEquals(port, deploymentProperties.port());
}

Expand Down
Loading