fix(Toolkit): route callTool through executeWithInfrastructure

agentscope-core/src/main/java/io/agentscope/core/model/ExecutionConfig.java

@@ -155,6 +155,33 @@ private static boolean isRetryableError(Throwable error) {
                     .retryOn(RETRYABLE_ERRORS)
                     .build();
 
+    /**
+     * Sentinel value for {@link #timeout} meaning "no timeout". A negative duration is never
+     * produced by normal usage and is recognised by {@code ToolExecutor.applyTimeout} and
+     * {@link ModelUtils#applyTimeoutAndRetry ModelUtils.applyTimeoutAndRetry}
+     * as "skip the timeout operator entirely".
+     *
+     * <p>This is the only way to opt out of the timeout that {@link #TOOL_DEFAULTS} and {@link
+     * #MODEL_DEFAULTS} always carry, because {@link #mergeConfigs} treats {@code null} as
+     * "inherit from fallback".
+     *
+     * <p><b>Supported paths</b>: The sentinel is honoured on the <em>tool</em> path
+     * ({@code ToolExecutor.applyTimeout}) and the <em>model-flux</em> path
+     * ({@code ModelUtils.applyTimeoutAndRetry}), where it means genuinely unbounded
+     * (no timeout operator is applied). Extension consumers that read
+     * {@link #getTimeout()} directly and pass the value to a framework timeout operator
+     * must apply the same guard via {@link #isTimeoutDisabled()}:
+     * <ul>
+     *   <li>{@code EmbeddingUtils.applyTimeoutAndRetry} in {@code rag-simple} — honours the
+     *       sentinel by skipping the timeout operator, consistent with tool/model-flux.</li>
+     *   <li>{@code openai-official} provider — the sentinel is recognised but degrades to
+     *       the OpenAI SDK's own default request timeout rather than being truly unbounded,
+     *       because the SDK client does not accept an unbounded timeout value. A warning is
+     *       logged when this occurs.</li>
+     * </ul>
+     */
+    public static final Duration NO_TIMEOUT = Duration.ofNanos(-1);
+
     /**
      * Standard defaults for tool executions.
      *
@@ -184,6 +211,20 @@ public Duration getTimeout() {
         return timeout;
     }
 
+    /**
+     * Returns true when the configured timeout is the {@link #NO_TIMEOUT} sentinel,
+     * meaning consumers should skip applying any timeout operator.
+     *
+     * <p>Only the exact {@link #NO_TIMEOUT} sentinel is recognised; stray negative
+     * durations (which should be rejected by {@link Builder#timeout(Duration)}) are
+     * not treated as "no timeout".
+     *
+     * @return true if timeout is disabled via {@link #NO_TIMEOUT}
+     */
+    public boolean isTimeoutDisabled() {
+        return NO_TIMEOUT.equals(timeout);
+    }
+
     /**
      * Gets the maximum number of attempts.
      *
@@ -305,14 +346,36 @@ public static class Builder {
         /**
          * Sets the timeout duration for a single execution.
          *
-         * @param timeout the timeout duration, or null for no timeout
+         * @param timeout the timeout duration (must be &gt; 0, or {@link #NO_TIMEOUT}),
+         *        or null to inherit from fallback; {@code Duration.ZERO} is not a synonym
+         *        for {@link #noTimeout()} and is therefore rejected
          * @return this builder instance
+         * @throws IllegalArgumentException if timeout is a negative duration other than
+         *        {@link #NO_TIMEOUT}, or if timeout is {@code Duration.ZERO}
          */
         public Builder timeout(Duration timeout) {
+            if (timeout != null
+                    && (timeout.isNegative() || timeout.isZero())
+                    && !NO_TIMEOUT.equals(timeout)) {
+                throw new IllegalArgumentException(
+                        "timeout must be positive, got "
+                                + timeout
+                                + "; use NO_TIMEOUT or noTimeout() to disable it");
+            }
             this.timeout = timeout;
             return this;
         }
 
+        /**
+         * Opt out of timeout entirely for this call. Equivalent to {@code timeout(NO_TIMEOUT)}.
+         * This is the only way to prevent the timeout inherited from {@link #TOOL_DEFAULTS} /
+         * {@link #MODEL_DEFAULTS}, because {@link #mergeConfigs} treats {@code null} as inherit.
+         */
+        public Builder noTimeout() {
+            this.timeout = NO_TIMEOUT;
+            return this;
+        }
+
         /**
          * Sets the maximum number of attempts (including the initial attempt).
          *

agentscope-core/src/main/java/io/agentscope/core/model/ModelUtils.java

@@ -87,7 +87,7 @@ public static Flux<ChatResponse> applyTimeoutAndRetry(
         if (execConfig != null) {
             // Apply timeout if configured
             Duration timeout = execConfig.getTimeout();
-            if (timeout != null) {
+            if (timeout != null && !execConfig.isTimeoutDisabled()) {
                 responseFlux =
                         responseFlux.timeout(
                                 timeout,

agentscope-core/src/main/java/io/agentscope/core/tool/ToolExecutor.java

@@ -155,7 +155,9 @@ private void invokeChunkCallback(
     // ==================== Single Tool Execution ====================
 
     /**
-     * Execute a single tool call with full infrastructure support.
+     * Execute a single tool call (core execution only: Tracer + {@link #executeCore};
+     * no scheduling, timeout, retry, shutdown guard, or id/name stamping). Use
+     * {@link #executeWithInfrastructure(ToolCallParam, ExecutionConfig)} for the full-infrastructure path.
      *
      * @param param Tool call parameters
      * @return Mono containing execution result
@@ -171,8 +173,9 @@ Mono<ToolResultBlock> execute(ToolCallParam param) {
 
     /**
      * Execute a single tool call with a per-call tool request config and a per-call internal chunk
-     * callback. This is the single core entry point; the no-arg {@link #execute(ToolCallParam)}
-     * resolves the request config from its explicit runtime context and uses no internal callback.
+     * callback. This is the single core entry point; the 1-param {@link #execute(ToolCallParam)}
+     * overload resolves the request config from its explicit runtime context and uses no internal
+     * callback.
      */
     Mono<ToolResultBlock> execute(
             ToolCallParam param,
@@ -342,7 +345,10 @@ private Collection<String> resolveActiveGroups(ToolCallParam param) {
     // ==================== Batch Tool Execution ====================
 
     /**
-     * Execute multiple tool calls with concurrency control, timeout, and retry.
+     * Execute multiple tool calls with concurrency control plus full per-call infrastructure
+     * (scheduling, timeout, retry, shutdown guard, id/name stamping). Each single call is routed
+     * through {@link #executeWithInfrastructure(ToolUseBlock, ExecutionConfig, Agent,
+     * RuntimeContext, ToolRequestConfig, BiConsumer)}.
      *
      * @param toolCalls List of tool calls to execute
      * @param parallel Whether to execute in parallel
@@ -449,33 +455,70 @@ private boolean isConcurrencySafe(ToolUseBlock toolCall, ToolRequestConfig reque
     }
 
     /**
-     * Execute a single tool call with infrastructure (scheduling, timeout, retry).
+     * Execute a single tool call with infrastructure (scheduling, timeout, retry, shutdown
+     * guard), and stamps the result with the tool call's id/name.
+     *
+     * <p>This overload is used by the batch path ({@link #executeAll(List, boolean,
+     * ExecutionConfig, Agent, RuntimeContext)}), which routes each {@link ToolUseBlock} with
+     * the infrastructure config, per-call request config, and chunk callback.
      */
-    private Mono<ToolResultBlock> executeWithInfrastructure(
+    Mono<ToolResultBlock> executeWithInfrastructure(
             ToolUseBlock toolCall,
             ExecutionConfig executionConfig,
             Agent agent,
             RuntimeContext agentRuntimeContext,
             ToolRequestConfig requestConfig,
             BiConsumer<ToolUseBlock, ToolResultBlock> internalChunkCallback) {
-        // Build tool call parameter
         ToolCallParam param =
                 ToolCallParam.builder()
                         .toolUseBlock(toolCall)
                         .agent(agent)
                         .runtimeContext(agentRuntimeContext)
                         .build();
 
-        // Get core execution
         Mono<ToolResultBlock> execution = execute(param, requestConfig, internalChunkCallback);
 
-        // Apply infrastructure layers
+        return applyInfrastructure(execution, executionConfig, toolCall);
+    }
+
+    /**
+     * Execute a single tool call with full infrastructure, preserving all fields from the
+     * original {@link ToolCallParam} (including input).
+     *
+     * <p>This overload is used by {@code Toolkit.callTool} so that user-supplied fields on the
+     * param object are not silently discarded before reaching {@link #executeCore}.
+     */
+    Mono<ToolResultBlock> executeWithInfrastructure(
+            ToolCallParam param, ExecutionConfig executionConfig) {
+        ToolUseBlock toolCall = param.getToolUseBlock();
+
+        Mono<ToolResultBlock> execution = execute(param);
+
+        return applyInfrastructure(execution, executionConfig, toolCall);
+    }
+
+    /**
+     * Applies the shared infrastructure pipeline (scheduling, timeout, retry, shutdown guard)
+     * and stamps the result with the tool call's id/name. The four infrastructure layers and
+     * the error-to-result conversion live here so that both entry points (batch and single)
+     * stay in sync when a new layer is added.
+     *
+     * <p><b>Retry semantics</b>: {@link #applyRetry} only fires for the timeout
+     * {@code RuntimeException} emitted by {@link #applyTimeout}. Tool failures are converted
+     * to normal {@link ToolResultBlock#error} completions inside {@link #executeCore} before
+     * this pipeline runs, and {@link #applyShutdownGuard} runs <em>after</em> retry so
+     * shutdown signals are never seen by {@code retryWhen} either. "Retry" here means
+     * "retry on timeout", nothing else.
+     */
+    private Mono<ToolResultBlock> applyInfrastructure(
+            Mono<ToolResultBlock> execution,
+            ExecutionConfig executionConfig,
+            ToolUseBlock toolCall) {
         execution = applyScheduling(execution);
         execution = applyTimeout(execution, executionConfig, toolCall);
         execution = applyRetry(execution, executionConfig, toolCall);
         execution = applyShutdownGuard(execution);
 
-        // Add tool metadata and error handling
         return execution
                 .map(result -> result.withIdAndName(toolCall.getId(), toolCall.getName()))
                 .onErrorResume(
@@ -499,7 +542,8 @@ private Mono<ToolResultBlock> applyScheduling(Mono<ToolResultBlock> execution) {
 
     private Mono<ToolResultBlock> applyTimeout(
             Mono<ToolResultBlock> execution, ExecutionConfig config, ToolUseBlock toolCall) {
-        if (config == null || config.getTimeout() == null) {
+        // null = inherit from fallback, NO_TIMEOUT sentinel = explicitly disabled
+        if (config == null || config.getTimeout() == null || config.isTimeoutDisabled()) {
             return execution;
         }
 

agentscope-core/src/main/java/io/agentscope/core/tool/ToolRegistry.java

@@ -28,7 +28,9 @@
  * and retrieve tools.
  *
  * <p><b>Thread Safety:</b> This class is thread-safe, using {@link ConcurrentHashMap} for internal
- * storage to support concurrent tool registration and lookup operations.
+ * storage to support concurrent tool registration and lookup operations. Tool instance and
+ * registration metadata are stored together in a single compound map entry, so put/remove of the
+ * two are a single atomic operation.
  *
  * <p><b>Key Responsibilities:</b>
  * <ul>
@@ -39,8 +41,9 @@
  */
 class ToolRegistry {
 
-    private final Map<String, AgentTool> tools = new ConcurrentHashMap<>();
-    private final Map<String, RegisteredToolFunction> registeredTools = new ConcurrentHashMap<>();
+    private record Entry(AgentTool tool, RegisteredToolFunction registered) {}
+
+    private final Map<String, Entry> entries = new ConcurrentHashMap<>();
 
     /**
      * Register a tool with its metadata.
@@ -53,8 +56,7 @@ void registerTool(String toolName, AgentTool tool, RegisteredToolFunction regist
         if (toolName == null || toolName.isBlank()) {
             throw new IllegalArgumentException("Tool name cannot be null or blank");
         }
-        tools.put(toolName, tool);
-        registeredTools.put(toolName, registered);
+        entries.put(toolName, new Entry(tool, registered));
     }
 
     /**
@@ -67,7 +69,8 @@ AgentTool getTool(String name) {
         if (name == null || name.isBlank()) {
             return null;
         }
-        return tools.get(name);
+        Entry e = entries.get(name);
+        return e != null ? e.tool() : null;
     }
 
     /**
@@ -80,7 +83,8 @@ RegisteredToolFunction getRegisteredTool(String name) {
         if (name == null || name.isBlank()) {
             return null;
         }
-        return registeredTools.get(name);
+        Entry e = entries.get(name);
+        return e != null ? e.registered() : null;
     }
 
     /**
@@ -89,7 +93,7 @@ RegisteredToolFunction getRegisteredTool(String name) {
      * @return Set of tool names
      */
     Set<String> getToolNames() {
-        return new HashSet<>(tools.keySet());
+        return new HashSet<>(entries.keySet());
     }
 
     /**
@@ -98,7 +102,13 @@ Set<String> getToolNames() {
      * @return Map of tool name to RegisteredToolFunction
      */
     Map<String, RegisteredToolFunction> getAllRegisteredTools() {
-        return new ConcurrentHashMap<>(registeredTools);
+        Map<String, RegisteredToolFunction> result = new ConcurrentHashMap<>();
+        for (Map.Entry<String, Entry> e : entries.entrySet()) {
+            if (e.getValue().registered() != null) {
+                result.put(e.getKey(), e.getValue().registered());
+            }
+        }
+        return result;
     }
 
     /**
@@ -110,24 +120,35 @@ void removeTool(String toolName) {
         if (toolName == null || toolName.isBlank()) {
             throw new IllegalArgumentException("Tool name cannot be null or blank");
         }
-        tools.remove(toolName);
-        registeredTools.remove(toolName);
+        entries.remove(toolName);
     }
 
     /**
      * Atomically remove a tool only if the current instance matches the expected one.
      * Uses {@link ConcurrentHashMap#remove(Object, Object)} to avoid TOCTOU races.
      *
+     * <p><b>Identity semantics</b>: The guard check ({@code existing.tool() == expected})
+     * compares the expected tool by reference ({@code ==}), not via {@link Object#equals}.
+     * Two {@code AgentTool} instances that are {@link Object#equals equal} but not the same
+     * reference will not match — this guards against accidental removal of a tool that was
+     * re-registered under the same name by another caller. The CAS at
+     * {@link ConcurrentHashMap#remove(Object, Object)} additionally depends on the
+     * {@code Entry} record's {@link Object#equals}, which compares both the
+     * {@code AgentTool} and {@code RegisteredToolFunction} fields; callers that
+     * rebuild {@code Entry} objects (e.g. via {@code copyTo}) must ensure
+     * {@code RegisteredToolFunction} equality remains stable across rebuilds.
+     *
      * @param toolName Tool name to remove
-     * @param expected The expected AgentTool instance (identity comparison)
+     * @param expected The expected {@link AgentTool} instance, compared by reference
+     *        ({@code ==}), not by {@link Object#equals}
      * @return true if the tool was removed, false if it was already replaced or absent
      */
     boolean removeToolIfSame(String toolName, AgentTool expected) {
-        boolean removed = tools.remove(toolName, expected);
-        if (removed) {
-            registeredTools.remove(toolName);
+        Entry existing = entries.get(toolName);
+        if (existing != null && existing.tool() == expected) {
+            return entries.remove(toolName, existing);
         }
-        return removed;
+        return false;
     }
 
     /**
@@ -149,20 +170,20 @@ void removeTools(Set<String> toolNames) {
      * @param target The target registry to copy tools to
      */
     void copyTo(ToolRegistry target) {
-        for (Map.Entry<String, AgentTool> entry : tools.entrySet()) {
-            String toolName = entry.getKey();
-            AgentTool tool = entry.getValue();
-            RegisteredToolFunction registered = registeredTools.get(toolName);
-            target.registerTool(
+        for (Map.Entry<String, Entry> e : entries.entrySet()) {
+            String toolName = e.getKey();
+            Entry entry = e.getValue();
+            target.entries.put(
                     toolName,
-                    tool,
-                    registered == null
-                            ? null
-                            : new RegisteredToolFunction(
-                                    tool,
-                                    registered.getExtendedModel(),
-                                    registered.getMcpClientName(),
-                                    registered.getPresetParameters()));
+                    new Entry(
+                            entry.tool(),
+                            entry.registered() == null
+                                    ? null
+                                    : new RegisteredToolFunction(
+                                            entry.tool(),
+                                            entry.registered().getExtendedModel(),
+                                            entry.registered().getMcpClientName(),
+                                            entry.registered().getPresetParameters())));
         }
     }
 }