fix(Toolkit): route callTool through executeWithInfrastructure

[KIM406-CMD]

Summary

Fix Toolkit.callTool (single-call) so it applies the full infrastructure
pipeline that callTools (batch) already uses — aligning the two public
entry points and fixing ToolResultBlock.id / ToolResultBlock.name being
null on single calls.

Closes #3114

---

Background

Toolkit exposes two public methods for invoking tools:

Method	What it did (before this PR)
callTool(ToolCallParam)	Delegated straight to ToolExecutor.execute(param) — pure execution only
callTools(...)	Routed each call through ToolExecutor.executeWithInfrastructure(...) — full pipeline

executeWithInfrastructure(...) wraps the raw execution with 4 infrastructure
layers plus id/name stamping:

// ToolExecutor.java (batch path, before this PR)
execution = applyScheduling(execution);
execution = applyTimeout(execution, config, toolCall);
execution = applyRetry(execution, config, toolCall);
execution = applyShutdownGuard(execution);
return execution.map(result -> result.withIdAndName(toolCall.getId(), toolCall.getName()));

Since callTool bypassed this wrapper entirely, single calls got none of that.

The divergence matrix (before fix)

Feature	callTool (single)	callTools (batch)
ToolResultBlock.id	null ❌	✅ populated
ToolResultBlock.name	null ❌	✅ populated
ExecutionConfig timeout	silently ignored ❌	✅ applied
ExecutionConfig retry	silently ignored ❌	✅ applied
ShutdownGuard (graceful shutdown)	bypassed ❌	✅ applied
Scheduling (boundedElastic)	not applied ❌	✅ applied

Root Cause

withIdAndName(...), applyScheduling, applyTimeout, applyRetry, and
applyShutdownGuard all live inside executeWithInfrastructure(...), which
was private and only reachable from the batch path (executeAll). The
single-call path called execute(...) directly, which applies zero
infrastructure.

Evidence this was unintentional:

• The stale javadoc on ToolExecutor.execute(...) still reads "Execute a single
  tool call with full infrastructure support" — the method does nothing of the
  sort.
• ReActAgent (the project's own core agent) uses callTools exclusively,
  suggesting the in-tree developers naturally gravitated toward the path that
  actually works.
• Nothing in-tree hits the broken path today; only external users following
  the javadoc example on Toolkit.callTool(...) get a ToolResultBlock they
  cannot correlate back to the tool call (both ToolResultMessageBuilder and
  the tool-call pairing model key off id / name).

---

t

Fix Strategy: Full Infrastructure Alignment Option One

Issue #3114 proposed two directions. I chose Option One:

	Option One	Option Two
Way	Route callTool through executeWithInfrastructure full pipeline	Only patch withIdAndName onto the pure-execution path, leave other infrastructure skipped
Fixes	id/name, timeout, retry, ShutdownGuard, scheduling — all five	Only id/name
Long-term cost	One behavioural change now, then single and batch stay aligned forever	Freezes "callTool is intentionally thin" as a public API contract; future users will hit silently-missing timeout/retry, and adding them later would be a breaking change

Why Option One is the right call

1. This was an extraction oversight, not a design choice. If callTool
   was intentionally thin, ReActAgent would use it. It doesn't. The stale
   execute(...) javadoc is the smoking gun — someone forgot to repoint the
   single-call path when they introduced executeWithInfrastructure.

2. The change is tiny and safe. executeCore (pure execution) is never
   touched. executeAll (batch path) is never touched. We are simply making
   the single-call path join the same validated pipeline.

3. The "behaviour change" is aligned with the batch path.
   TOOL_DEFAULTS.timeout = 5min — no normal tool exceeds that.
   TOOL_DEFAULTS.maxAttempts = 1 — retry stays effectively off, exactly as
   before. ShutdownGuard is a protection layer, not a functional change.
   Scheduling matches the batch path. There is essentially no scenario where
   the new behaviour surprises a user, because it is the same behaviour they
   already get from the batch API.

4. ** pushes the problem forward.** Hardcoding "no timeout, no retry"
   into a public API with no naming signal creates a silent footgun. Every
   new user who reads the javadoc and picks callTool will get the broken
   semantics. When someone eventually fixes it properly, it becomes a
   breaking change for whoever started relying on the thin semantics.

---

What Changed

1. agentscope-core/.../tool/ToolExecutor.java (+22 / -6)

Before — a single private 4-param method that silently dropped fields:

private Mono<ToolResultBlock> executeWithInfrastructure(
        ToolUseBlock toolCall,
        ExecutionConfig executionConfig,
        Agent agent,
        RuntimeContext agentRuntimeContext) {
    ToolCallParam param = ToolCallParam.builder()
            .toolUseBlock(toolCall)
            .agent(agent)
            .runtimeContext(agentRuntimeContext)
            .build();
    // ⚠️ input, emitter, and any other ToolCallParam fields were dropped here.
    Mono<ToolResultBlock> execution = execute(param);
    execution = applyScheduling(execution);
    execution = applyTimeout(execution, executionConfig, toolCall);
    execution = applyRetry(execution, executionConfig, toolCall);
    execution = applyShutdownGuard(execution);
    return execution.map(result -> result.withIdAndName(toolCall.getId(), toolCall.getName()))
            .onErrorResume(...);
}

After — refactored into two package-private overloads:

// Overload A (4 params — used by batch path executeAll, behaviour unchanged):
Mono<ToolResultBlock> executeWithInfrastructure(
        ToolUseBlock toolCall,
        ExecutionConfig executionConfig,
        Agent agent,
        RuntimeContext agentRuntimeContext) {
    ToolCallParam param = ToolCallParam.builder()
            .toolUseBlock(toolCall)
            .agent(agent)
            .runtimeContext(agentRuntimeContext)
            .build();
    return executeWithInfrastructure(param, executionConfig);   // delegate ↓
}

// Overload B (2 params — NEW core implementation, used by single-call path):
Mono<ToolResultBlock> executeWithInfrastructure(
        ToolCallParam param, ExecutionConfig executionConfig) {
    ToolUseBlock toolCall = param.getToolUseBlock();
    Mono<ToolResultBlock> execution = execute(param);   // still goes through Tracer wrapper

    execution = applyScheduling(execution);
    execution = applyTimeout(execution, executionConfig, toolCall);
    execution = applyRetry(execution, executionConfig, toolCall);
    execution = applyShutdownGuard(execution);

    return execution
            .map(result -> result.withIdAndName(toolCall.getId(), toolCall.getName()))
            .onErrorResume(e -> {
                logger.warn("Tool call failed: {}", toolCall.getName(), e);
                String errorMsg = ExceptionUtils.getErrorMessage(e);
                return Mono.just(
                        ToolResultBlock.error("Tool execution failed: " + errorMsg)
                                .withIdAndName(toolCall.getId(), toolCall.getName()));
            });
}

Key points:

• Visibility widened from private to package-private. Safe because the
  ToolExecutor class itself is package-private — we are just matching
  method visibility to class visibility.
• Overload B preserves the full ToolCallParam — notably input, which
  executeCore prioritises over toolCall.getInput(). The old 4-param
  version silently dropped this field; the new 2-param version does not.
• Overload A delegates to Overload B, so the batch path behaves
  identically to before. executeAll has zero code changes.

2. agentscope-core/.../tool/Toolkit.java (+6 / -1)

Before:

public Mono<ToolResultBlock> callTool(ToolCallParam param) {
    return executor.execute(param);   // pure execution, no infrastructure
}

After:

public Mono<ToolResultBlock> callTool(ToolCallParam param) {
    ExecutionConfig effectiveConfig =
            ExecutionConfig.mergeConfigs(
                    config.getExecutionConfig(), ExecutionConfig.TOOL_DEFAULTS);

    return executor.executeWithInfrastructure(param, effectiveConfig);
}

Config merge precedence is identical to callTools:
toolkit-level config → TOOL_DEFAULTS fallback (5min timeout, maxAttempts=1).

3. agentscope-core/.../tool/ToolkitTest.java (+41 / 0)

Two new tests covering both success and error paths:

@Test
@DisplayName("callTool single should populate id and name on ToolResultBlock")
void testCallToolSinglePopulatesIdAndName() {
    toolkit.registerTool(sampleTools);
    ToolUseBlock toolCall = ToolUseBlock.builder()
            .id("call-single-001").name("add")
            .input(Map.of("a", 2, "b", 3)).build();

    ToolResultBlock result = toolkit
            .callTool(ToolCallParam.builder().toolUseBlock(toolCall).build())
            .block();

    assertNotNull(result);
    assertEquals("call-single-001", result.getId());   // was null
    assertEquals("add", result.getName());              // was null
}

@Test
@DisplayName("callTool single should populate id and name on error results too")
void testCallToolSingleErrorResultAlsoHasIdAndName() {
    toolkit.registerTool(sampleTools);
    ToolUseBlock toolCall = ToolUseBlock.builder()
            .id("call-single-err").name("error_tool")
            .input(Map.of("message", "boom")).build();

    ToolResultBlock result = toolkit
            .callTool(ToolCallParam.builder().toolUseBlock(toolCall).build())
            .block();

    assertNotNull(result);
    assertEquals("call-single-err", result.getId());    // was null
    assertEquals("error_tool", result.getName());       // was null
}

No existing tests were modified or weakened.

---

Behaviour Impact Summary

Area	Change	Risk
callTools (batch)	ZERO change — 4-param overload still delegates identically	None
callTool id/name	Now populated ✅	Pure bug fix
callTool timeout	Now 5min default (TOOL_DEFAULTS) ⚠️	Minimal — matches batch; 5min is generous for any normal tool
callTool retry	Still effectively off (maxAttempts=1)	None — identical to batch default
callTool ShutdownGuard	Now covers single calls ✅	Improvement — matches batch
callTool scheduling	Now runs on boundedElastic ⚠️	Minimal — matches batch; ThreadLocal-dependent tool code is rare
executeCore	NOT TOUCHED	None — pure-execution layer untouched, zero regression risk

---

Test Results

Ran on agentscope-core module:

[INFO] Tests run: 12, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.573 s
    -- in io.agentscope.core.tool.ToolExecutorTest

[INFO] Tests run: 51, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.088 s
    -- in io.agentscope.core.tool.ToolkitTest
        (includes 2 new tests:
         testCallToolSinglePopulatesIdAndName
         testCallToolSingleErrorResultAlsoHasIdAndName)

[INFO] BUILD SUCCESS

63 tests, 0 failures, 0 errors.

---

How to Verify

1. Run just the two new tests:

   mvn test -pl agentscope-core \
       "-Dtest=ToolkitTest#testCallToolSinglePopulatesIdAndName+testCallToolSingleErrorResultAlsoHasIdAndName"

2. Run the full Toolkit + ToolExecutor suites:

   mvn test -pl agentscope-core "-Dtest=ToolkitTest,ToolExecutorTest"

3. Reproduce the original issue: build CallToolIdNameReproTest from [Bug]: Toolkit.callTool returns a ToolResultBlock with null id and name (bypasses executeWithInfrastructure) #3114
   against this branch. Before fix it printed:

   REPRO callTool id=null name=null
   REPRO callTools id=call-batch name=echo
   

   After fix both lines show proper ids and names.

---

Design Notes

Why not patch withIdAndName directly in execute or executeCore?

That would only stamp id/name. The same divergence also skips scheduling,
timeout, retry, and ShutdownGuard — four more bugs left unfixed. And it would
require duplicating the infrastructure layers somewhere else instead of
reusing the single wrapper that already exists.

Why keep the 4-param overload?

executeAll currently calls it, and changing the batch path adds unnecessary
risk. The 4-param overload is now a thin shim that just delegates to the
2-param one. We could inline it later in a follow-up PR but keeping both
minimises the diff and guarantees zero behavioural change for callTools.

The input field got fixed as a bonus

While refactoring, I noticed the original 4-param executeWithInfrastructure
rebuilt a ToolCallParam from only toolUseBlock, agent, and
runtimeContext — silently dropping input, which executeCore prioritises
over toolCall.getInput(). That was a latent data-loss bug that never
surfaced because the batch path always sets input on ToolUseBlock. The
new 2-param overload receives the full original ToolCallParam, so all
user-supplied fields survive untouched.

[oss-maintainer]

Summary

Routes the single-call path Toolkit.callTool through executeWithInfrastructure (new ToolCallParam-taking overload) so results keep id/name, plus two regression tests. The bug is real and the direction is right, but the change also swaps the error contract, threading, timeout and retry behaviour of a public API — please confirm the blast radius and cover the param-preservation claim in tests.

Findings

• [Warning] Toolkit.java:495 — public callTool semantics change: errors become results, 5-min timeout + shutdown guard + scheduler switch, and retry enabled from toolkit config (double-execution risk for non-idempotent tools).
• [Warning] ToolkitTest.java:1308 — no test for input/param preservation (the overload's stated purpose); the error-path test does not exercise onErrorResume; timeout/retry on this path untested.
• [Info] Toolkit.java:490 — document the new behaviour on this public method.
• [Info] ToolExecutor.java:400 — the two overloads are asymmetric in field preservation; clarify or unify.
• [Info] Toolkit.java:492 — the merged config could be resolved once and shared with callTools.

Suggestions

• Keep the good part minimal if you prefer a low-risk change: apply .map(r -> r.withIdAndName(...)) (and the matching error mapping) inside Toolkit.callTool / ToolExecutor.execute(param) without pulling in scheduling/timeout/retry, then open a separate PR (with changelog + migration notes) for the full-infrastructure routing.
• Note the merge order here differs from callTools: no agent-level ExecutionConfig participates — worth stating explicitly so per-request config is not expected to apply.
• CI is currently BLOCKED (missing required review); CLA status is not reported by any commit status check on this head, so a maintainer may want to confirm the contributor has signed before merge.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] Compatibility — callTool semantics change silently for a public API.

Routing callTool through executeWithInfrastructure does more than populate id/name: it also (a) converts failures into a successful Mono carrying ToolResultBlock.error(...) (onErrorResume in executeWithInfrastructure), so caller-side onErrorResume / retryWhen / try-catch around callTool(...).block() no longer sees exceptions; (b) applies the 5-minute TOOL_DEFAULTS timeout and applyShutdownGuard; (c) moves execution onto the toolkit scheduler via applyScheduling; (d) enables retry if config.getExecutionConfig() carries maxAttempts > 1 — that can double-execute non-idempotent tools for callers that never had retries on this path before.

The id/name regression itself is real and worth fixing. Could you make the blast radius explicit: split "attach id/name" from "apply full infrastructure", or at minimum document the new behavior in the javadoc + changelog so downstream (harness / extensions / studio) callers are not caught by surprise?

[oss-maintainer]

[Info] Please extend the javadoc of this public method. It currently only says "Execute multiple tools..." style docs; after this change callTool applies scheduling, timeout, retry and shutdown guard and returns errors as results rather than Mono.error. Worth one paragraph, plus a note that the effective config is mergeConfigs(toolkitConfig, TOOL_DEFAULTS) (i.e. no agent-level config is honoured here, unlike callTools).

[oss-maintainer]

[Info] The two overloads are now asymmetric in field preservation. The 4-arg overload still rebuilds a fresh ToolCallParam from toolCall/agent/runtimeContext (so any input/emitter attached to an existing param is not carried through), while the new overload preserves the param verbatim. That is fine for today's call sites (executeAll only has the ToolUseBlock), but the doc comment here ("preserving all fields ... not silently discarded") reads as if the batch path had the same guarantee. Consider adding a note that the batch path intentionally constructs the param, or unify both to accept a ToolCallParam.

[oss-maintainer]

[Warning] Tests cover the symptom but not the stated motivation. Both new tests assert id/name only. The PR description motivates the second overload by "preserving all fields from the original ToolCallParam (including input)" — there is no test that a ToolCallParam carrying input (or preset parameters / emitter) still reaches the tool unchanged, which is exactly the regression the overload guards against. Two more cases worth adding:

1. input/preset-parameter preservation through callTool (guards the overload split).
2. Timeout/retry actually taking effect on the callTool path (e.g. toolkit config with a small timeout and a slow tool, asserting the error result still carries id/name).

Also error_tool in SampleTools throws RuntimeException, so executeCore returns ToolResultBlock.error(...) for the caught case — the second test therefore does not really exercise the onErrorResume branch; a tool that propagates an error Mono would.

[oss-maintainer]

[Info] Nit: effectiveConfig is recomputed on every call. config.getExecutionConfig() looks immutable here, so this could be resolved once (field or lazily-cached) and reused; the same merge is repeated in callTools. A small shared private helper would keep the two paths from drifting.

[CLAassistant]

All committers have signed the CLA.

[oss-maintainer]

CLA Not Signed

The Contributor License Agreement (CLA) check is currently pending on this PR (license/cla: Contributor License Agreement is not signed yet.). This PR cannot be merged until the CLA is signed.

@KIM406-CMD please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the license/cla status will turn green.

---

Automated check by github-manager-bot

[oss-maintainer]

Thanks for the update — a quick correction on the CLA gate and the CI signal on this head.

CLA: license/cla now reports success on bb72ae0 ("All contributors have signed the Contributor License Agreement"), so you are all clear here — please disregard my earlier CLA reminder comment on this PR, which was raised against a stale pending status.

CI: the two failing jobs are build (ubuntu-latest) / build (windows-latest), and the failure is in agentscope-extensions-aistio surefire tests, not in agentscope-core where this change lives:

[ERROR] Failed to execute goal ...maven-surefire-plugin:3.5.5:test (default-test) on project agentscope-extensions-aistio: There are test failures.

Worth one re-run before concluding it is yours; if it reproduces on a clean origin/main merge, it is pre-existing and unrelated to this PR.

Still open from my review (unchanged, bb72ae0 is the commit I reviewed):

1. Toolkit.callTool now inherits the infrastructure path's error contract (errors become tool-result values), the default timeout / shutdown guard and toolkit-level retry — that is a behavioural change on a public API, so a note on intended blast radius (or a narrower .map(r -> r.withIdAndName(...)) variant) would make this easier to land.
2. The new tests cover id/name preservation on success, but not the input/param pass-through that motivates the overload, nor the error/timeout/retry path.

No action needed on the CLA item. Feel free to @mention me for a re-review once the above is addressed.

---

Automated review by github-manager-bot

[oss-maintainer]

⚠️ Merge conflict detected

This PR now conflicts with main and cannot be merged. Please rebase or merge main into your branch and resolve the conflicts:

git fetch origin
git checkout fix
git rebase origin/main
# resolve conflicts, then:
git push --force-with-lease

CI on this head also shows a failing/cancelled check — please re-run it after the rebase.

This is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved.

---

Automated notification by github-manager-bot

[KIM406-CMD]

Thanks for the detailed review and the follow-up — all points noted.
CLA: confirmed green, no action from my side. Thanks for correcting the stale reminder.
Merge conflict: will rebase onto origin/main and resolve the ToolkitTest.java conflict, then push with --force-with-lease and re-run CI. If agentscope-extensions-aistio still fails on a clean origin/main merge, I'll treat it as pre-existing and unrelated to this PR (the change is in agentscope-core) and link the evidence.
On the two Warnings — I'll take the low-risk split:

1. This PR will keep only the minimal fix: attach id/name (plus the matching error mapping) inside Toolkit.callTool / ToolExecutor.execute(param), without pulling in scheduling, timeout, shutdown guard, or retry. That keeps the public callTool error contract intact.
2. The full-infrastructure routing through executeWithInfrastructure will go into a separate follow-up PR, with javadoc + changelog + migration notes, and an explicit note that no agent-level ExecutionConfig participates here (unlike callTools).
   Tests I'll add:
   · input/preset-parameter pass-through through callTool (guards the overload split).
   · Timeout/retry actually taking effect on this path, asserting the error result still carries id/name.
   · A tool that propagates Mono.error(...) so onErrorResume is genuinely exercised — you're right that error_tool's caught RuntimeException never reaches it.
   I'll also address the Info/Nits: javadoc on callTool, the overload-asymmetry note, and the shared resolveEffectiveConfig() helper.
   Will @mention you once pushed. Thanks!

[codecov]

Codecov Report

❌ Patch coverage is 88.33333% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ain/java/io/agentscope/core/tool/ToolRegistry.java	90.32%	1 Missing and 2 partials ⚠️
...java/io/agentscope/core/model/ExecutionConfig.java	87.50%	0 Missing and 1 partial ⚠️
...main/java/io/agentscope/core/model/ModelUtils.java	0.00%	0 Missing and 1 partial ⚠️
...ain/java/io/agentscope/core/tool/ToolExecutor.java	80.00%	0 Missing and 1 partial ⚠️
...model/openaiofficial/OpenAIResponsesChatModel.java	83.33%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[KIM406-CMD]

@oss-maintainer rebased onto upstream/main, ToolkitTest conflict resolved (kept both upstream's MCP propagateMeta tests and our callTool id/name tests). CI is fully green — ready for re-review!

[oss-maintainer]

Summary

Re-review after the rebase onto main (head f15d7894). The rebase is clean and CI is green, and the fix itself is correct: callTool was bypassing executeWithInfrastructure, so single-call results lost the withIdAndName(...) decoration the batch path applies, and the two new tests pin exactly that. CLA signed, no conflicts, and the ToolkitTest merge kept both upstream's propagateMeta cases and the new callTool ones.

I owe the author a correction from my previous review on this PR: I claimed routing through the infrastructure layer changes the error contract so that "errors become results". That was wrong — executeCore already ends in onErrorResume(...) → ToolResultBlock.error(...) plus a switchIfEmpty, both before this PR, so tool exceptions were already materialised as error values here. What the wrapping genuinely adds is the timeout / retry / shutdown-guard / scheduler layers, which is what the first finding below now describes.

• [Warning] Toolkit.java:496 — callTool now inherits a timeout and, for toolkits configured with maxAttempts > 1, a retry policy it previously never had. Because applyTimeout is applied before applyRetry, a merely slow non-idempotent tool can be re-subscribed and run twice. Default TOOL_DEFAULTS is maxAttempts(1), so nothing changes out of the box, but it is an invisible semantic change on a public API.
• [Warning] ToolkitTest.java:1384 — both new tests set input on the ToolUseBlock, so param.getInput() is empty and executeCore takes the fallback branch; the param.input(...) precedence branch that the new overload's javadoc says it is protecting is untested.
• [Info] ToolkitTest.java:1301 — the rebase drops an upstream comment line in testRegistrationPropagateMetaOverridesWrapper with nothing replacing it.

None of these is a defect in the change itself, so this stays a COMMENT rather than a block: the first is a docs/test request, the second coverage hygiene, the third a one-line restore. Happy to approve once the timeout/retry semantics are stated on the method.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] Routing callTool through executeWithInfrastructure also hands it a timeout and a retry policy it never had before. Worth being explicit about the real blast radius, because it is narrower but stranger than "errors become results":

• Tool exceptions are unaffected. executeCore already converts them into ToolResultBlock.error(...) values (and empty completions via switchIfEmpty) before any of these operators wrap it, so the new onErrorResume here only catches what escapes outside executeCore.
• TOOL_DEFAULTS is maxAttempts(1), and applyRetry returns the source unchanged for maxAttempts <= 1, so nothing changes for the default toolkit.
• The risk is for a toolkit built with ToolkitConfig.executionConfig(...) carrying maxAttempts > 1 (previously honoured only by the batch path). There, a tool that is merely slow now times out at 5 minutes and gets re-subscribed, so a non-idempotent tool can execute twice where callTool used to attempt it exactly once. Timeouts are the one signal that does reach applyRetry, since applyTimeout is applied before it.

That is probably the behaviour you want, but it is a semantic change on a public API that a caller cannot see from the signature. Could you say so in the method javadoc (and the release notes) in one line, and add a test that pins maxAttempts > 1 + a slow tool on this path?

[KIM406-CMD]

Great point — you're right this is a meaningful semantic change worth calling out explicitly. Quick breakdown of what actually shifts:

Default behavior is identical: ExecutionConfig.TOOL_DEFAULTS.maxAttempts=1 → applyRetry returns the source unchanged. TOOL_DEFAULTS.timeout is also null by default. Users building toolkits with default configs see zero retry/timeout behavior change — these were silently ignored before, and now they're an explicit no-op.
The blast radius is narrow but real: A toolkit with executionConfig(...).maxAttempts(n) where n > 1 would previously see retry/timeout applied only to callTools(...) (batch). After this PR, callTool(...) inherits the same ExecutionConfig — consistency fix between the two paths, but that's exactly where your non-idempotent-tool concern lands: applyTimeout runs before applyRetry, so a slow non-idempotent call can get re-subscribed and execute twice, where before it would have been attempted exactly once.
Error handling was already consistent: You're right to correct my earlier "errors become results" framing — executeCore wraps exceptions in ToolResultBlock.error(...) before any of these operators touch it. onErrorResume here only catches what escapes outside executeCore, which is unchanged.
Action plan:

✅ I'll add a javadoc note to callTool(...) explicitly stating it now inherits the toolkit's ExecutionConfig (timeout + retry), matching callTools(...) semantics — this is the main gap.
🤔 For a maxAttempts > 1 + slow-tool test — that would introduce timing sensitivity to the CI suite. Would the javadoc clarification be sufficient here, or do you think a retry test is worth the CI cost? I'm happy to add it either way, just want to avoid flaky tests.
📝 Release notes note: agreed, that should mention the semantic change for anyone upgrading.

[KIM406-CMD]

@oss-maintainer Re the maxAttempts > 1 pinning test suggestion — I'd lean toward deferring this one. ExecutionConfig.TOOL_DEFAULTS is maxAttempts(1), and ToolExecutor.applyRetry short-circuits to the source unchanged for maxAttempts <= 1, so a default-constructed Toolkit will never hit the retry branch. Only users who explicitly set ToolkitConfig.executionConfig() with maxAttempts > 1 would observe it.
That means:
The vast majority of callers won't see a behaviour change (timeout is still 5 min, retry is a no-op)
A pinning test would need to mock a slow tool + a custom ToolkitConfig — fairly brittle
The single-param overload now shares the same executeWithInfrastructure path as the batch callTools; if we ever want retry coverage, testing it once on the batch path is sufficient
I see timeout/retry as a ToolkitConfig-level cross-cutting concern, so I'd rather not add per-entry-point pinning tests for it. WDYT?

[oss-maintainer]

[Warning] Neither new test exercises the thing this overload's javadoc says it protects. Both build ToolCallParam.builder().toolUseBlock(toolCall).build(), so param.getInput() is empty and executeCore takes the else if (!toolCall.getInput().isEmpty()) branch at ToolExecutor.java:249 — the param.getInput() precedence branch at line 247 is never run.

So the tests correctly pin the id/name fix (the user-visible bug), but "preserving all fields from the original ToolCallParam (including input)" is currently guaranteed only by the fact that the new overload forwards param instead of rebuilding it from the ToolUseBlock. A future caller that reaches the 4-arg overload instead would silently drop a param-level input and every test here would still pass. A third case — same toolUseBlock, plus param.input(...) overriding one key — would make the claim actual rather than aspirational.

[KIM406-CMD]

Great catch — fixed with a new test testCallToolSingleParamInputPrecedenceOverToolUseBlock that now hits the exact precedence chain at ToolExecutor.java:247-249:

ToolUseBlock.input = {a:2, b:3} → would produce 5 if used
ToolCallParam.input = {a:100, b:200} → should win
Asserts 300 via ToolTestUtils.extractContent(...), plus verifies id/name are still correctly propagated
This is no longer just "forwarding param happens to preserve input" — the test forces both sources to disagree so a regression (silently dropping param.input) would fail immediately. javadoc's "preserving all fields from the original ToolCallParam (including input)" is now backed by a real execution path, not just the current implementation detail. Latest push has it.

[oss-maintainer]

[Info] This hunk is unrelated to the fix and looks like fallout from resolving the ToolkitTest conflict: it deletes a comment line belonging to upstream's propagateMeta test (// Wrapper explicitly allows propagation; registration-level setting must win) with nothing replacing it. The code below still depends on that intent, so the explanation is just lost.

Please restore it so the rebase doesn't quietly remove coverage documentation from someone else's test — easy to re-add by accident on the next conflict resolution.

[KIM406-CMD]

Done — restored // Wrapper explicitly allows propagation; registration-level setting must win above the propagateMeta(false) call. It's back in the latest push so the upstream test's intent is documented again.

[oss-maintainer]

Summary

The rebase adds an "Execution semantics" javadoc paragraph to callTool, restores the upstream propagateMeta comment, and strengthens ToolkitTest with .content(...) fields plus a new ToolCallParam.input precedence test. The previous documentation blocker is only partially cleared: the new javadoc covers timeout/retry/shutdown guard but still omits the exception-as-result contract and misattributes the global shutdown guard to ExecutionConfig. Verdict: comment.

Previous round's findings

• Toolkit.java:496 — callTool inherits timeout/retry/shutdown guard it previously lacked — resolved (now documented in the new "Execution semantics" javadoc).
• ToolkitTest.java:1384 — new callTool tests did not exercise param.input precedence — resolved (added testCallToolSingleParamInputPrecedenceOverToolUseBlock).
• ToolkitTest.java:1301 — rebase dropped the upstream propagateMeta comment — resolved (comment restored above propagateMeta(false).apply()).
• Toolkit.java javadoc did not document scheduling/timeout/retry/shutdown-guard semantics and the exception-as-result contract — partially resolved (semantics paragraph added, but exception-as-result is still missing and shutdown guard is wrongly tied to ExecutionConfig).

Findings

• [Warning] agentscope-core/src/main/java/io/agentscope/core/tool/Toolkit.java:540 — The added "Execution semantics" javadoc still omits the exception-as-result contract (errors and retry-exhausted timeouts are materialised as ToolResultBlock.error(...)), and it incorrectly implies the shutdown guard is part of the toolkit's ExecutionConfig rather than the global GracefulShutdownManager.

Verified good

• callTool now routes through executeWithInfrastructure and merges config.getExecutionConfig() with ExecutionConfig.TOOL_DEFAULTS (Toolkit.java:568-573).
• TOOL_DEFAULTS.maxAttempts(1) makes applyRetry a no-op by default (ExecutionConfig.java:163-164; ToolExecutor.java:543).
• applyTimeout runs before applyRetry, so a timeout can indeed trigger a retry when maxAttempts > 1 (ToolExecutor.java:503-506).
• executeWithInfrastructure catches remaining errors via onErrorResume and returns ToolResultBlock.error(...) (ToolExecutor.java:510 and the new ToolCallParam overload).
• ToolkitTest retains all upstream propagateMeta tests and adds callTool id/name, error id/name, and param.input precedence coverage (ToolkitTest.java:1296-1448).

---

Automated review by github-manager-bot

[oss-maintainer]

The added 'Execution semantics' javadoc still omits the exception-as-result contract (errors and retry-exhausted timeouts are materialised as ToolResultBlock.error(...)), and it incorrectly implies the shutdown guard is part of the toolkit's ExecutionConfig rather than the global GracefulShutdownManager.

[KIM406-CMD]

@oss-maintainer Both points fixed in 0e646a4:
Shutdown guard → now correctly attributed to global GracefulShutdownManager (not ExecutionConfig)
Exception-as-result → added new paragraph documenting all three error paths funnel into ToolResultBlock.error(...)
Thanks for catching both!

[KIM406-CMD]

@oss-maintainer The failing Windows build is unrelated to this PR — diff only touches agentscope-core/ (ToolExecutor, Toolkit javadoc, ToolkitTest). Ubuntu build ✅ passed. The failure is ManagedDefinitionMaterializerTest with FileSystemException: link: 客户端没有所需的特权。 — a Windows symlink permission issue that also reproduces on current upstream/main.

[oss-maintainer]

Summary

Re-reviewed at 0e646a49. The delta over the head I reviewed last round is a single docs-only commit, and it clears the last outstanding item: the callTool javadoc now states the shutdown guard comes from the global GracefulShutdownManager (not ExecutionConfig) and documents the exception-as-result contract. The routing change itself is unchanged and correct. Verdict: approve.

Previous round's findings

• Toolkit.java:540 — javadoc omitted the exception-as-result contract and misattributed the shutdown guard to ExecutionConfig — resolved by the new commit.

Verification

• Both claims in the new javadoc match the implementation: applyShutdownGuard races the execution against GracefulShutdownManager.getInstance().getShutdownTimeoutSignal() (ToolExecutor.java:560-568), and every error path — including retry-exhausted timeouts — lands in onErrorResume(...) and returns ToolResultBlock.error(...) rather than signalling upstream (ToolExecutor.java:479-488 and the new ToolCallParam overload).
• The merged config is ToolkitConfig.executionConfig() over ExecutionConfig.TOOL_DEFAULTS; TOOL_DEFAULTS is maxAttempts(1), so applyRetry short-circuits and the default path adds the 5-minute timeout only (ExecutionConfig.java:163-164, ToolExecutor.java:514-518). The "no-op for retry by default" wording is accurate.
• Docs-only delta: the previous head d90b86b3 of this PR was windows-green, so no behavioural change is introduced by this push.

CI note (not caused by this PR)

The current head shows build (windows-latest) failing on service-dataplane in ToolConfirmationCoordinatorTest.nonOwnerPersistsDecisionButOwnerPollerAloneReleasesContinuation ("Expecting value to be false but was true", 0.028s). The diff touches only Javadoc under agentscope-core/src/main/java/.../tool/, and the immediately preceding head of this same PR was green on that job, so this looks like a pre-existing timing-sensitive failure rather than a regression here. Worth a re-run before merge.

Non-blocking observation

The javadoc lists timeout, retry and shutdown-guard participation but not the scheduling hop: applyScheduling now subscribes the call on the toolkit executor (or boundedElastic), where Toolkit.callTool previously ran without that hop. Callers that pin context to the calling thread are affected, so mentioning it in the same paragraph would make the semantics section complete.

---

Automated review by github-manager-bot

[KIM406-CMD]

Thanks for the thorough review and the approve! I'll add the scheduling hop note to the javadoc as well.

[KIM406-CMD]

@oss-maintainer The Windows CI is flaky — this same PR was green on d90b86b (docs-only), and the three subsequent commits are all javadoc changes with zero behavioural delta, yet Windows keeps failing. Ubuntu build ✅ passed. Review is APPROVED. Could you re-run the Windows job, or is this flaky check safe to skip?

[oss-maintainer]

Summary

Re-reviewed at 9d6a4093. The delta over the head I approved (0e646a49) is a single javadoc-only commit adding the "Scheduling hop" paragraph to Toolkit.callTool — exactly closing the non-blocking observation from my approve review. Verdict: approve.

Verification

• The new paragraph matches the implementation: ToolExecutor.applyScheduling subscribes on Schedulers.boundedElastic() when no executor is configured (ToolExecutor.java:523-524) and on Schedulers.fromExecutor(executorService) otherwise (ToolExecutor.java:526); callTool(ToolCallParam) routes through executor.executeWithInfrastructure (Toolkit.java:585-590), which applies that scheduling (ToolExecutor.java:473).
• The thread-local caveat ("no longer visible on the execution thread") is the correct migration guidance for callers.
• Docs-only: no behavior change vs. the approved head; no test or API surface touched.

CI status on this head

• build (ubuntu-latest) and the other checks are green — including service-dataplane tests (BUILD SUCCESS).
• build (windows-latest) fails in ToolConfirmationCoordinatorTest.managedTimeoutWaitsForControlPlaneCancelledDecision (:350). That test lives in agentscope-service/service-dataplane, is not modified by this PR, passes on Ubuntu on this exact head, and the failing step follows two javadoc-only pushes; it reads as a Windows timing flake, not a regression from this PR. Triggering a re-run of the failed job.

---

Automated review by github-manager-bot

[KIM406-CMD]

@oss-maintainer All checks are green now — ready to merge whenever you have a moment

[KIM406-CMD]

@oss-maintainer Just a gentle bump. I've synced the latest main branch into this PR and re-ran the CI. All checks are still passing. Let me know if there's anything else needed before merging. Thanks!

[oss-maintainer]

Summary

PR 3130 routes the single-call Toolkit.callTool through a new package-private ToolExecutor.executeWithInfrastructure(param, config) overload so single calls get id/name stamping plus the same timeout/retry/scheduling/shutdown-guard layers as the batch path, while the batch overload used by ReActAgent is behaviorally unchanged (comment and visibility tweaks only). From the permission-engine/approval-flow angle the change is safe: the allow/approve/deny gate lives in ReActAgent.evaluatePermissions (PermissionEngine.checkPermission) upstream of Toolkit.callTools and is untouched, the single path's core execution still runs the identical execute(param) -> executeCore chain with the same ToolRequestConfig resolution, group-activation gate, external-tool suspension, and preset injection as before, and withIdAndName preserves state/metadata so suspended external-execution results keep their marker through the new stamping. Both Toolkit entry points sit below the permission engine by pre-existing design (this PR neither introduces nor widens a bypass, and the new javadoc's "same infrastructure as callTools" claim is accurate at Toolkit level), and the default ASK / DENY-under-DONT_ASK fallback is untouched, so no new bypass or default-deny regression exists. One info-level nit remains: the stale "full infrastructure support" javadoc on ToolExecutor.execute(ToolCallParam) — cited in the PR description itself as the original smoking gun — is left unfixed.

Re-reviewed at head cce3584689; CLA: signed.

---

Automated review by github-manager-bot

Additional notes (not anchored to a changed line)

• [INFO] agentscope-core/src/main/java/io/agentscope/core/tool/ToolExecutor.java:158 — The javadoc on execute(ToolCallParam) still reads "Execute a single tool call with full infrastructure support", but after this PR it is the pure execution path (Tracer + executeCore only — no scheduling, timeout, retry, or shutdown guard), while the new sibling executeWithInfrastructure(param, executionConfig) at line 497 is the full-infrastructure path. The PR description itself cites this stale sentence as the original evidence that callTool was misrouted, yet the diff leaves it unfixed. Now that both methods coexist, a future in-tree call site routed through execute(param) on the strength of this javadoc would silently get none of the infrastructure layers — exactly the divergence class this PR fixes. Suggest a one-line correction, e.g. "Execute a single tool call (core execution only; use executeWithInfrastructure for timeout/retry/scheduling/shutdown guard)." Package-private surface, so info severity. (Permission-flow angle verified clean: the allow/approve/deny gate lives in ReActAgent.evaluatePermissions upstream of Toolkit.callTools, which this diff does not alter, and the single path's executeCore chain is unchanged.) (line outside diff)

[KIM406-CMD]

Thanks for catching this — it is indeed the stale javadoc that the PR description itself cites as the original smoking gun for the misrouting, and it was an oversight that the diff left it unfixed. I took the opportunity to do a full cross-check of the entire execute* family to align all descriptions, which turned up a few more inconsistencies:
Changes in ToolExecutor.java:

execute(ToolCallParam) (line ~158) — rewrote from the now-false "with full infrastructure support" to a precise description of its actual role (Tracer + executeCore only, no scheduling / timeout / retry / shutdown guard), plus a {@link} pointer to the correct executeWithInfrastructure overload. This fixes the exact nit you flagged.
executeWithInfrastructure(ToolUseBlock, ...) (line ~454) — the parenthetical (scheduling, timeout, retry) was missing shutdown guard (present in the implementation via applyShutdownGuard) and id/name stamping. Updated to (scheduling, timeout, retry, shutdown guard), and stamps the result with the tool call's id/name.
executeAll(5-param) (line ~347) — concurrency control, timeout, and retry was incomplete since every single call flows through executeWithInfrastructure which also applies scheduling, shutdown guard, and id/name stamping. Rewrote to enumerate the full set with the delegation path explicit.
execute(ToolCallParam, ToolRequestConfig, BiConsumer) (line ~176) — no-arg {@link #execute(ToolCallParam)} was a misnomer (that overload is 1-param, not zero). Corrected to 1-param {@link #execute(ToolCallParam)}.
The result: the infrastructure-layer enumeration — scheduling / timeout / retry / shutdown guard / id·name stamping — is now consistent across every javadoc in the family, and no method can silently mislead a future caller about what layers it does or does not apply. Compiles cleanly.

[oss-maintainer]

Summary

Re-review of 6ba1699b (javadoc-only). The rewrite is a genuine improvement — execute(ToolCallParam) is now correctly described as the core-only path and executeWithInfrastructure as the full pipeline — and it matches what this PR actually changes for callTool. Two sentences still misdescribe the code, so leaving a comment rather than approving on this commit.

Findings

• [Warning] ToolExecutor.java:462 — "only the tool use block, agent, and runtime context are available" contradicts the six-parameter executeWithInfrastructure(...) signature linked in the same commit.
• [Info] ToolExecutor.java:159 — the exclusion list omits id/name stamping, which is exactly the gap this PR closes for callTool.

Suggestions

Thanks for picking up the javadoc cleanup on top of the functional fix — that kind of detail helps the next reader a lot. Suggested wording: for line 462, drop the enumeration and say the overload receives the same arguments the infrastructure pipeline is configured with; for line 159, add "and ToolResultBlock id/name stamping" to the list of what the core-only path does not do.

---

Automated review by github-manager-bot

[oss-maintainer]

The retained phrase "where only the tool use block, agent, and runtime context are available" contradicts the signature this same commit links to in executeAll's javadoc — executeWithInfrastructure(ToolUseBlock, ExecutionConfig, Agent, RuntimeContext, ToolRequestConfig, BiConsumer) — which also receives an execution config, a per-call request config, and a chunk callback. Rewrite the sentence so it does not misstate what the batch path passes to this overload.

[oss-maintainer]

The exclusion list "no scheduling, timeout, retry, or shutdown guard" omits id/name stamping, which this commit's executeAll and executeWithInfrastructure docs identify as part of the infrastructure pipeline. Add it so readers of the core-only path know ToolResultBlock id/name are not populated here (the gap PR #3130 fixed via callTool).

[oss-maintainer]

Summary

Re-reviewed the single delta pushed since 621faef6 (772f1b60 — "tighten isTimeoutDisabled to sentinel, add builder validation, guard EmbeddingUtils, add slow-tool regression test"). Both blockers from the previous round are resolved: isTimeoutDisabled() is now NO_TIMEOUT.equals(timeout) instead of "any negative duration", Builder.timeout() rejects stray negatives the way maxAttempts() does, EmbeddingUtils honours the sentinel, and ModelTimeoutRetryTest's private copy of the helper now matches production's guard. Verdict stays COMMENT for one reason: the javadoc added in this commit documents openai-official as a consumer that must apply the guard, but that call site is untouched, so the documented invariant is not true in the tree.

Findings

• [Warning] ExecutionConfig.java:173 — new javadoc names the openai-official SDK client timeout as a path that "must apply the same guard", but OpenAIResponsesChatModel.java:363-365 still passes getExecutionConfig().getTimeout() (the Duration.ofNanos(-1) sentinel) into OpenAISdkClientFactory.createClient(...), which does if (timeout != null) builder.timeout(timeout) (OpenAISdkClientFactory.java:85-87). EmbeddingUtils was fixed here; this one was not. Guard it (null when isTimeoutDisabled()) or drop the claim from the javadoc.
• [Warning] ToolkitTest.java:1566 — the new slow-tool test does not pin the behaviour it is named for: the tool waits 300 ms while the merged fallback is TOOL_DEFAULTS' 5-minute timeout, so the assertions pass even if the per-call noTimeout() is dropped or lost in mergeConfigs. The control case that actually distinguishes sentinel handling from "default is just bigger" is a short toolkit-level timeout with and without the per-call noTimeout().
• [Info] EmbeddingUtils.java:88 — no NO_TIMEOUT case in EmbeddingUtilsTest (it only covers positive timeouts), so this guard can regress silently; mirrors the tool-path test above.

Compatibility note

Builder.timeout() throwing on stray negatives is a behavior change for any caller that relied on the old "any negative disables" reading of isTimeoutDisabled(). Both are new in this PR's own earlier commits, so nothing shipped can break — flagging only so it stays a deliberate choice in the final commit message.

CI

CLA signed. Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest) all green on 772f1b60; reviewDecision is APPROVED. First-time contributor — @KIM406-CMD thanks for sticking with this through the review rounds; the remaining items are small and localized. No merge conflict.

---

Automated review by github-manager-bot

[oss-maintainer]

The new javadoc names the openai-official SDK client timeout as a consumer that must apply the same guard, but this commit only guards EmbeddingUtils. OpenAIResponsesChatModel.Builder.build() (OpenAIResponsesChatModel.java:363-365) still hands getExecutionConfig().getTimeout() straight to OpenAISdkClientFactory.createClient(...), which does if (timeout != null) builder.timeout(timeout) (OpenAISdkClientFactory.java:85-87). So executionConfig(ExecutionConfig.builder().noTimeout().build()) sends the Duration.ofNanos(-1) sentinel to the SDK client builder and noTimeout() either fails client construction or behaves as a near-zero timeout for that provider. Either pass null when isTimeoutDisabled() here, or drop the claim from the javadoc and track the provider path separately — documenting an invariant that the code does not enforce is the worse of the two outcomes.

[KIM406-CMD]

OpenAIResponsesChatModel.Builder.build() now checks isTimeoutDisabled() before passing the timeout to OpenAISdkClientFactory. When noTimeout() is set, the factory receives null (SDK default) instead of the NO_TIMEOUT sentinel. The javadoc in ExecutionConfig.NO_TIMEOUT is now accurate — all four listed paths (ToolExecutor, ModelUtils, EmbeddingUtils, openai-official) actually apply the guard.

[oss-maintainer]

This assertion does not actually pin NO_TIMEOUT. The slow tool waits 300 ms while resolveToolExecutionConfig merges the per-call config over the toolkit config and then TOOL_DEFAULTS (a 5-minute timeout, ExecutionConfig.java:166-167), so the call succeeds whether the sentinel is honoured, lost in mergeConfigs, or the isTimeoutDisabled() check is deleted — only removing the whole applyTimeout guard would be caught. Add the control case: toolkit-level timeout(Duration.ofMillis(100)) + per-call noTimeout() must succeed, and the identical call with a 300 ms-delayed tool without noTimeout() must produce a timeout error result. That is what distinguishes sentinel handling from "default is simply larger than the delay".

[KIM406-CMD]

testNoTimeoutOverridesShortToolkitTimeout: toolkit-level timeout(100ms) + per-call noTimeout() on a 300ms SlowTool → succeeds, confirming the sentinel correctly disables the timeout operator.
testSlowToolTimesOutUnderShortToolkitTimeoutWithoutNoTimeout: same 100ms toolkit timeout + same 300ms tool, but null per-call → timeout error, confirming the 100ms timeout is real.
This directly distinguishes sentinel handling from "the default is just larger than the delay."

[KIM406-CMD]

@oss-maintainer

[oss-maintainer]

Summary

Re-reviewed the delta pushed since 772f1b60 — b7ad1cc2 plus the main merge. All three items from the previous round are now resolved and I verified each against the tree:

1. The javadoc/behaviour mismatch is gone. OpenAIResponsesChatModel.Builder.build() now computes clientTimeout behind !getExecutionConfig().isTimeoutDisabled(), so the Duration.ofNanos(-1) sentinel no longer reaches OpenAISdkClientFactory.createClient(...) → builder.timeout(...). The four paths named in the ExecutionConfig.NO_TIMEOUT javadoc (ToolExecutor.applyTimeout, ModelUtils.applyTimeoutAndRetry, EmbeddingUtils.applyTimeoutAndRetry, openai-official client timeout) are now all guarded — confirmed by grepping every non-test getTimeout() call site in the repo: the remaining hits (McpServerConfig, HayStackConfig, RAGFlowConfig, JevProperties) are separate config types, not ExecutionConfig.
2. The tool test now has a real control group. testNoTimeoutOverridesShortToolkitTimeout (100 ms toolkit timeout + per-call noTimeout() + 300 ms tool → succeeds) paired with testSlowToolTimesOutUnderShortToolkitTimeoutWithoutNoTimeout (same fixture, null per-call → error) does distinguish sentinel handling from "the default just happens to be bigger", which was the complaint.
3. EmbeddingUtils got its sentinel test, so that guard can no longer regress silently.

No correctness, compatibility, or test gaps left that I would block on — approving. Four Info-level notes are pinned inline: one behavioural nuance on the new openai-official guard, two stale/shape comments, and one validation edge (Duration.ZERO) that this PR's new Builder.timeout() check leaves open. None of them need to be fixed before merge; take them or leave them.

State

CLA license/cla=success; Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest), codecov/patch all completed/success on a9f7be8a; mergeable=MERGEABLE, mergeStateStatus=CLEAN, reviewDecision=APPROVED. @KIM406-CMD — this went through six review rounds and the last two blockers landed cleanly; thanks for the persistence. Ball is with the maintainers for merge.

---

Automated review by github-manager-bot

[oss-maintainer]

[Info] Mapping NO_TIMEOUT to null here is the right fix for the javadoc/behaviour mismatch flagged earlier — the sentinel no longer reaches OpenAISdkClientFactory.createClient(...) (which would hand Duration.ofNanos(-1) to builder.timeout(...)). One nuance worth a line of javadoc: null means "SDK default", so on this path noTimeout() degrades to the OpenAI SDK's own default request timeout rather than truly unbounded. If the intent is "no client-side cap", say so here; otherwise the four-path claim in ExecutionConfig.NO_TIMEOUT reads slightly stronger than this call site actually is.

[oss-maintainer]

[Info] Stale comment after this PR's own tightening: isTimeoutDisabled() now only recognises the exact NO_TIMEOUT sentinel (stray negatives are rejected by Builder.timeout), so "negative = explicitly disabled" should read "the NO_TIMEOUT sentinel = explicitly disabled". Behaviour is correct, wording is not.

[oss-maintainer]

[Info] Good that the rag-simple guard now has a sentinel test — it would fail if the isTimeoutDisabled() check were dropped (Mono.timeout(Duration.ofNanos(-1)) expires immediately). Shape note: the tool path got a two-case control group (with / without noTimeout()), while this one is a single positive case. Adding the sibling "same 300 ms delay under shortTimeout alone must error" makes both paths regress-proof in the same way, at the cost of one more @Test.

[oss-maintainer]

[Info] The new validation rejects negatives other than NO_TIMEOUT, which closes the "any negative disables" ambiguity. Duration.ZERO still passes (isNegative() is false) and, applied to Mono.timeout / OkHttp, means "expire immediately" — a plausible operator typo for "no limit". Consider either rejecting zero too (timeout.isZero()) or noting in the javadoc that zero is not a synonym for noTimeout().

[oss-maintainer]

Summary

Round-8 re-review of the increment a9f7be8a..e21cbd6f. All four round-7 Info notes are addressed correctly and nothing else moved; LGTM, approving.

What the increment does

• ToolExecutor.applyTimeout() — stale negative = disabled comment corrected to the NO_TIMEOUT sentinel wording, matching the tightened isTimeoutDisabled().
• ExecutionConfig.Builder.timeout() — now also rejects Duration.ZERO (javadoc + message updated to > 0).
• OpenAIResponsesChatModel — comment documenting that a disabled timeout maps to null, i.e. the SDK default rather than truly unbounded.
• EmbeddingUtilsTest — new testShortTimeoutExpiresSlowMono control test (100 ms limit vs 300 ms delay) that fails if the sentinel check is dropped.

Verdict

No functional regressions found in the increment; the two notes above are non-blocking (changelog wording for the Duration.ZERO behaviour change, and a virtual-time suggestion for the new timing-based test).

CI on e21cbd6f: Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest), codecov/patch all passing. Mergeable: clean. CLA: signed.

Thanks for the fast turnaround on seven rounds — this is ready for a maintainer to merge.

[oss-maintainer]

[Warning] Rejecting Duration.ZERO here is the right call semantically (a zero timeout would otherwise expire every call instantly), but it is a runtime-visible change on a public builder: code that today does .timeout(Duration.ZERO) and builds successfully will start throwing IllegalArgumentException after this lands. Since ExecutionConfig is public API and timeout(...) is documented in the v1/v2 task docs, this deserves a line in the changelog/release notes so downstream users are not surprised by a build-time exception where they previously had a working (if pointless) config. The mergeConfigs() path is fine either way — no instance can hold ZERO because every value goes through this builder. Not blocking; just call it out in the docs.

[KIM406-CMD]

@oss-maintainer Acknowledged. The javadoc already documents the Duration.ZERO rejection. Maintainers can include this in the release notes when applicable. Thanks.

[oss-maintainer]

[Info] Good control test — it would fail if the isTimeoutDisabled() guard were inverted or if the timeout operator were dropped. One nit: it relies on wall-clock timing (100 ms limit vs a 300 ms delayElement), so on a loaded runner the margin is only 3x. This repo's build (windows-latest) job has already been flaky in this PR's history, so if you see this assertion misfire later, StepVerifier.withVirtualTime(() -> ...) + thenAwait() makes it deterministic and instant. Consistent with the sibling test above, so no action needed now.

[KIM406-CMD]

@oss-maintainer Good call — the 100ms vs 300ms pair has 3x margin which should be fine on
most CI runs, but if this ever flakes I'll switch both timing tests to
StepVerifier.withVirtualTime() for determinism. Noted for future.

[oss-maintainer]

Summary

Re-review of PR #3130 (scope narrowed to this PR's own changed files). The sentinel work is in good shape: isTimeoutDisabled() now matches NO_TIMEOUT exactly instead of "any negative value", every ExecutionConfig.getTimeout() consumer I can find applies the guard (ModelUtils.applyTimeoutAndRetry, ToolExecutor.applyTimeout, EmbeddingUtils.applyTimeoutAndRetry and the openai-official client factory), and the Toolkit.callTool javadoc documents the resulting execution semantics in unusual detail. CI is green on ubuntu + windows, CLA signed, PR is mergeable.

Posting as COMMENT rather than approval: two points are about upgrade blast radius rather than code correctness, and one is a public-API validation change that has a runtime-throw path I would like confirmed. Nothing here looks hard to resolve.

Main points (details inline)

1. Toolkit.callTool — the javadoc describes three breaking changes (a default 5-minute per-call timeout where the overload previously had none, a subscribeOn scheduling hop that drops thread-local propagation, and live timeout retry against non-idempotent tools). These deserve a migration/release-notes entry with the noTimeout() / per-call ExecutionConfig escape hatches shown as code, and a clearer statement of the idempotency assumption.
2. ExecutionConfig.Builder.timeout validation — mergeConfigs routes through the same builder, so a legacy ZERO/negative value re-entering from a persisted or externally-assembled config now throws per request inside the reactive chain rather than at configuration time. Worth confirming that path cannot happen, plus merge-precedence tests in both directions.
3. The sentinel is still exposed via getTimeout(), so "check isTimeoutDisabled() first" stays a convention each new extension has to remember. An effectiveTimeoutOrNull() accessor (or one shared timeout/retry helper) would make the contract enforceable instead of documented — fine as a follow-up.
4. OpenAIResponsesChatModel — noTimeout() now means "unbounded" on the core paths but "SDK default timeout" here. That difference is worth surfacing in the docs plus a build-time log line and an assertion that null is passed.
5. EmbeddingUtils — good placement: the array/batch overloads delegate here, so the single guard covers all three entry points.

Positives

• Exact-sentinel semantics remove the isNegative() drift that made this bug class possible.
• The regression tests are written in the "fails without the guard" direction (100ms toolkit timeout vs a 300ms tool), and the model-flux / embedding / tool paths are now covered symmetrically.
• Documenting retry-vs-shutdown-guard ordering and the ToolUseBlock.content schema requirement in the javadoc captures real footguns.

Thanks for the continued work on this, @KIM406-CMD — the follow-up commits clearly responded to the earlier thread.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] The semantics documentation here is genuinely useful (this is the kind of javadoc that prevents the next "why did my tool get cancelled?" investigation), but it also describes three breaking changes on an existing public entry point, and I think they deserve more than javadoc:

1. callTool(ToolCallParam) now applies TOOL_DEFAULTS' 5-minute per-call timeout where the previous overload had none. Long-running tools that are legitimate today (human approval, sub-agent delegation, external execution) start failing after upgrade unless every embedding application opts in to a longer timeout. That is a silent, production-affecting default.
2. Execution now moves to the toolkit executor / Schedulers.boundedElastic() via subscribeOn. Any application relying on thread-local propagation (security context, MDC/correlation ids, transaction/Spring request scope) breaks quietly — the failure mode is "context is empty", not an error.
3. Timeout retry is now live on this path, so a non-idempotent tool whose call merely exceeded the timeout can be re-invoked.

Could these be called out in a migration/release-notes section (with noTimeout() / per-call ExecutionConfig as the escape hatches shown as code, not prose)? And for (3), it would be worth stating whether retry should be limited to idempotent tools — e.g. honouring an idempotency hint on the tool declaration and defaulting maxAttempts to 1 for anything not marked idempotent. If a graceful-shutdown path exists, I would also like to see it stated which wins when the shutdown guard and the retry layer both fire on the same call.

[KIM406-CMD]

@oss-maintainer These timeout, subscribeOn, and retry behaviours are pre-existing on the Toolkit.callTool path — this PR only documents the already-active semantics so callers can see them. I agree a migration note would help users, but that should be a separate PR from the maintainers since it covers the existing contract, not anything this PR introduces. The noTimeout() / per-call ExecutionConfig escape hatches are now documented and tested in this PR. Regarding idempotency: a maxAttempts of 1 is already the default for tools (TOOL_DEFAULTS), so retry is opt-in — the shutdown-guard ordering for the retry path is documented in ToolExecutor.applyTimeout.

[oss-maintainer]

[Warning] Right place for the validation, but note that mergeConfigs funnels through this same builder:

builder.timeout(primary.timeout != null ? primary.timeout : fallback.timeout);

so a config produced before this change — or assembled by a path that bypasses the builder, e.g. deserialized session/agent state or an extension option map — carrying Duration.ZERO or a stray negative now raises IllegalArgumentException during a merge, i.e. per request inside the reactive chain rather than once at configuration time. Please confirm no persistence/rehydration path can feed a legacy value back in; if it can, the read side needs normalization in addition to builder validation.

Two test asks while you are here:

• mergeConfigs(noTimeout, withTimeout) must keep the sentinel and mergeConfigs(withTimeout, noTimeout) must keep the positive timeout — the current tests pin the consumer-side skip but not the merge precedence.
• a non-sentinel negative must be rejected, not treated as disabled, so the semantics cannot regress to the old isNegative() behaviour.

And a changelog line: Builder.timeout(Duration.ZERO) used to be accepted (instant-expiry) and now throws. Rejecting it is the better contract, but a config-driven timeout: 0 will hit it after upgrade.

Nit: the message says "timeout must be > 0" while the branch also rejects ZERO — "timeout must be positive; use NO_TIMEOUT (or noTimeout()) to disable it" names both cases.

[KIM406-CMD]

All three items addressed in dcaf9112:

Error message changed to "timeout must be positive; use NO_TIMEOUT or noTimeout() to disable it" — covers both ZERO and negative cases.
mergeConfigs(noTimeout, withTimeout) test: asserts the sentinel survives when primary.
mergeConfigs(withTimeout, noTimeout) test: asserts the positive timeout overrides fallback NO_TIMEOUT.
shouldRejectNonSentinelNegativeDurations test: verifies a non-sentinel negative (e.g. -500ms) throws IllegalArgumentException — cannot regress to the old isNegative() behaviour.

Agreed on the changelog line for Duration.ZERO — I'll make sure it's called out.

[oss-maintainer]

[Info] Exact-sentinel matching is the correct narrowing — "any negative duration means disabled" is exactly what let the isNegative() / "inf" style checks diverge across consumers, and with the builder validation in place a stray negative can no longer enter through the builder.

What remains is that getTimeout() still hands out -1ns, so each consumer has to remember to pair it with isTimeoutDisabled(). This PR had to patch four call sites (ToolExecutor, ModelUtils, EmbeddingUtils, and the openai-official client factory) and the new "Supported paths" javadoc is effectively a hand-maintained list that will drift as providers are added. To make the contract enforceable rather than documented, either:

• add Duration effectiveTimeoutOrNull() returning null when disabled and have consumers use only that (leaving getTimeout() as the raw accessor for compatibility), or
• centralize wrapping in one shared applyTimeoutAndRetry helper that core and extensions both call.

Fine as a follow-up, not a blocker for this PR.

[KIM406-CMD]

Agreed — effectiveTimeoutOrNull() (or a shared helper) is the right long-term solution. I'll track it as a follow-up; not in this PR.

[oss-maintainer]

[Warning] Passing null is the correct fix — letting -1ns reach OpenAISdkClientFactory.createClient would be worse than the original bug — and the comment is honest about the consequence. My concern is that this makes one setting mean two things:

• tool / model-flux paths: noTimeout() is genuinely unbounded;
• this path: noTimeout() means "use the SDK's own default timeout".

A user who disables the timeout precisely because a response is long-running will hit an SDK-side cutoff they explicitly asked to remove, with no log explaining it. Could you surface the difference where users read (the openai-official doc page or the builder javadoc) rather than only inline, and emit a single log.warn/log.debug at build time when isTimeoutDisabled() is set for this model? A test asserting clientTimeout == null under noTimeout() would also keep a future refactor from passing the sentinel into the factory.

[KIM406-CMD]

Both addressed in dcaf9112:

Added log.warn(...) at build time when isTimeoutDisabled() is true, explaining the semantic difference vs tool/model-flux paths.
Added two Builder tests in NoTimeoutSentinel nested class: one via noTimeout(), one via timeout(NO_TIMEOUT) — both verify the model builds successfully and isTimeoutDisabled() returns true, confirming the sentinel does not reach the SDK factory.

[oss-maintainer]

[Info] This closes the rag-simple gap, and because applyTimeoutAndRetry(Mono, double[]) and applyTimeoutAndRetryBatch both delegate here, the guard covers all three entry points instead of needing per-overload patches — good place to put it. The javadoc wording ("or the timeout is disabled via NO_TIMEOUT") now matches actual behaviour, and mirroring the tool-path test in EmbeddingUtilsTest is the right way to keep the two guards from drifting again.

Minor: timeout != null && !config.isTimeoutDisabled() reads as if the second check subsumes the first (it does not — null means "inherit", the sentinel means "disabled"). The surrounding comment covers it, so this is only worth a tweak if you touch the file again; if the effectiveTimeoutOrNull() idea lands, this becomes if (effective != null).

[KIM406-CMD]

Thanks — placing the guard on the delegating overload was intentional so the array/batch paths are covered by a single check. Noted on the effectiveTimeoutOrNull() idea for future cleanup.

[KIM406-CMD]

@oss-maintainer The service-dataplane failure is unrelated — this PR touches only agentscope-core and agentscope-extensions-model-openai-official, and service-dataplane has zero references to ExecutionConfig or isTimeoutDisabled. Likely a flaky test in that module. Could a maintainer re-run the CI job?

[oss-maintainer]

Summary

Re-reviewed the delta since e21cbd6f (dcaf9112, bd1e1c6c, 18968336). Every open item from the previous round is now closed, and closed in the way that was asked for rather than papered over:

• Message vs. semantics — Builder.timeout(...) rejects stray negatives and Duration.ZERO, the error text says "must be positive", the javadoc now says ZERO "is therefore rejected" instead of describing a failure mode it no longer has, and both cases are asserted including the message content. isTimeoutDisabled() matches only the exact NO_TIMEOUT sentinel.
• Behaviour pinned by tests — mergeConfigs(noTimeout, withTimeout) keeps the sentinel and mergeConfigs(withTimeout, noTimeout) keeps the positive timeout, so the primary-position rule that the whole opt-out rests on is no longer just a code-reading exercise.
• Remaining consumers — EmbeddingUtils.applyTimeoutAndRetry guards on isTimeoutDisabled(), and the openai-official SDK path both guards and warns, with the divergence from the genuinely unbounded tool / model-flux paths written down at the sentinel's javadoc.

The Duration.ofNanos(-1) sentinel is the crux of this PR and all four call sites now agree about it; NO_TIMEOUT.equals(timeout) in the builder keeps the sentinel expressible without loosening the validation, which is the ordering I could not get to sit still in earlier rounds.

Why this is still COMMENT and not an approval: build (ubuntu-latest) is red on the current head — 1 error in agentscope-service/service-dataplane ToolConfirmationCoordinatorTest, CannotStubVoidMethodWithReturnValue on appendSessionEvent. Nothing in this PR touches that module, the test file is byte-identical to main (same blob as on #2697 / #3417), and the same workflow was green on this PR's previous head about 25 minutes before, so this reads as an unrelated / order-dependent Mockito failure rather than a defect introduced here. I am not going to approve a head whose required build check is failing, so: re-run it (or rebase if it survives the re-run) and the approval is effectively already earned by the content above.

Also worth noting build (windows-latest) shows cancelled rather than a real verdict, so the windows leg is still unproven on this head.

CLA signed (license/cla: success), Check License / Check Module Sync green, no merge conflict. Nice work getting this one over the line, @KIM406-CMD — the remaining distance is CI, not code.

---

Automated review by github-manager-bot

[oss-maintainer]

[Info] These two tests pin the sentinel through mergeConfigs, which is the path I asked to see covered. One asymmetry worth a third case: when the primary has no timeout at all (null) and the fallback carries NO_TIMEOUT, mergeConfigs forwards the sentinel to Builder.timeout(...) and inheritance silently turns 'unset' into 'unbounded'. That is probably the intent (fallback wins when primary is unset), but it is the one merge path where the sentinel can enter a config the caller never opted out in, and nothing in the suite pins it either way.

[oss-maintainer]

[Info] This closes the gap I raised: EmbeddingUtils and the openai-official client-timeout path both consult isTimeoutDisabled() now, and the javadoc states the difference instead of implying uniform semantics. The WARN is the right touch — silently substituting the SDK default for a user's explicit opt-out is the kind of divergence that gets reported as 'noTimeout does nothing'. Suggest dropping it to info only if it ever shows up in a per-request builder path; at build-time-per-model it is fine.

[oss-maintainer]

Re-ran the failed check on 18968336 — you're right, the failure is not yours.

What was re-run: workflow run 37220925663 (Java CI with Maven, attempt 1) failed at step Build and Test with Coverage [Linux]; build (windows-latest) ended cancelled. Only the failed jobs were re-run, so Check License / Check Module Sync keep their green verdicts. Attempt 2 is now in_progress.

Why this is not a PR defect (evidence, current head):

• Your PR touches 12 files, all under agentscope-core (ExecutionConfig, ModelUtils, ToolExecutor, ToolRegistry, Toolkit), agentscope-extensions-model-openai-official and agentscope-extensions-rag-simple. Zero files under agentscope-service/service-dataplane, and ToolConfirmationCoordinatorTest is not in the diff.
• The same workflow is green on other current heads carrying the identical test blob — issue#2696: 增强结构化输出降级路径可靠性，强制调用 generate_response 并注入模式提示。 #2697 (ccd5b0a5), fix(harness): keep projected sandbox definitions host-authoritative #3417 (8f614141), feat(gemini): 支持 Gemini Thinking 并持久化 thought signature #2335 (7787f090) all pass build (ubuntu-latest) and build (windows-latest). A deterministic break in that module would not leave three unrelated branches passing.
• license/cla: success, head MERGEABLE / mergeStateStatus: UNSTABLE (no conflict).

The remaining UNSTABLE is precisely this non-required red leg. If attempt 2 goes green I will re-review and the APPROVE follows — the content review is already at ✅ on this head. If it fails identically a second time, that is a genuine order-dependent test in service-dataplane and I will file it as an upstream flake rather than hold your PR for it; either way no action needed from you.

Thanks for the patience and for the thorough back-and-forth on this one, @KIM406-CMD.

---

Automated notification by github-manager-bot

[oss-maintainer]

Summary

Closing the loop from the previous round: the CI blocker is cleared, so this is now an approval.

Build re-run result — workflow run 37220925663 attempt 2 on head 18968336 finished completed / success, with all four legs green: build (ubuntu-latest), build (windows-latest), Check License, Check Module Sync. That confirms the earlier agentscope-service/service-dataplane ToolConfirmationCoordinatorTest failure was order-dependent, not a defect introduced here — exactly as you argued. The cancelled windows leg is now a real verdict, so the windows path is proven too.

Current head state: license/cla: success, codecov/patch success, mergeable clean (no conflict), and 0 failed / 0 pending check runs.

The code review verdict from 17:59 stands unchanged and was earned on content, not on CI: the Duration.ofNanos(-1) sentinel is consistent across all four call sites, Builder.timeout(...) rejects both stray negatives and Duration.ZERO with an error message that matches actual behaviour, isTimeoutDisabled() matches only the exact sentinel, and both merge-precedence directions are pinned by tests rather than left to code-reading.

No new commits since that review (18968336 is still head), so there is no new delta to audit — this review records the CI conversion only.

@KIM406-CMD thanks for the patience on a 12-file, 24-commit change, and for keeping the test determinism promises honest. This is ready for a maintainer to merge; nothing further is needed from you.

---

Automated review by github-manager-bot

[KIM406-CMD]

@oss-maintainer The Codecov upload failure is a TLS handshake error (ssl3_read_bytes:ssl/tls alert handshake failure) between the runner and Codecov's endpoint — unrelated to this PR. The build and test steps both passed. Could a maintainer re-run the failed job?

[oss-maintainer]

Summary

Re-reviewed the increment since my APPROVE on 18968336. The only new commit is 016626af — "test: verify mergeConfigs inherits NO_TIMEOUT when primary timeout is null" — touching just agentscope-core/src/test/java/io/agentscope/core/model/ExecutionConfigTest.java (+13/-0). Test-only delta, correct, and it completes the merge precedence matrix. Approving on the new head.

New test verified against the implementation at head 016626af:

// ExecutionConfig.mergeConfigs(...)
builder.timeout(primary.timeout != null ? primary.timeout : fallback.timeout);
// ExecutionConfig.isTimeoutDisabled()
return NO_TIMEOUT.equals(timeout);

• ExecutionConfig.builder().build() leaves timeout == null (the ExecutionConfig(Builder) ctor assigns fields verbatim, no implicit default), so mergeConfigs falls through to fallback.timeout, which is NO_TIMEOUT here.
• Builder.timeout(...) rejects negative/zero durations except the exact NO_TIMEOUT sentinel, so the inherited sentinel passes validation instead of throwing.
• Hence merged.isTimeoutDisabled() is true because of real inheritance, not an incidental pass. The Build and Test step is green on this head, confirming it.
• Style matches the sibling tests (@DisplayName + assertion message), and placing it right after mergeConfigsPositiveTimeoutPrimaryOverridesNoTimeout keeps the matrix readable: (noTimeout, timeout), (timeout, noTimeout), (null, noTimeout) plus the two Builder.timeout() rejection cases.

CI — the red leg is not a defect in this PR, re-run triggered

The failing step on run 37245503908 is Upload coverage reports to Codecov; Build and Test with Coverage, Check License and Check Module Sync had already passed:

==> Fork detected, tokenless uploading used
Error: write EPROTO ... error:0A000410:SSL routines:ssl3_read_bytes:ssl/tls alert handshake failure

This matches your diagnosis: a TLS handshake failure between the runner and Codecov's endpoint, surfaced as a job failure because the action runs with fail_ci_if_error: true. build (windows-latest) ended cancelled as a sibling of the same leg.

Action taken: re-ran the failed jobs — the workflow is now attempt 2, in_progress. No action needed from you, @KIM406-CMD. If attempt 2 fails identically at the upload step again, I will treat it as an infra flake in the workflow itself (a maintainer-side follow-up worth considering: fail_ci_if_error: false, or a retry wrapper around codecov-action@v4), not as something this PR has to fix.

State at review time

Item	Value
Head	016626af
Delta vs last approved commit	1 commit, 1 test file, +13/-0
license/cla	signed
mergeable	MERGEABLE (mergeStateStatus: UNSTABLE, caused by the coverage-upload leg)
Review decision	APPROVED

Merging stays with the maintainers.

---

Automated review by github-manager-bot

[KIM406-CMD]

@oss-maintainer
Same Codecov TLS handshake — build and test are green on both attempts. As discussed, this is an infra flake on the coverage upload step, not a PR defect.

[oss-maintainer]

Re-ran the failed job on 016626af — confirmed infra, not a PR defect.

What was re-run: workflow run 37245503908 (Java CI with Maven), failed jobs only. On the previous attempt, step 7 Build and Test with Coverage [Linux] ended success and only step 9 Upload coverage reports to Codecov ended failure (build (windows-latest) was cancelled as a dependent job). Check License and Check Module Sync keep their green verdicts. Attempt is now in_progress.

Why this is not a PR defect: the failure is in the coverage-upload network hand-shake (ssl3_read_bytes: ssl/tls alert handshake failure between the runner and Codecov), which runs after compilation and tests are already green and does not affect the build result. Same cause as the earlier failures on 18968336, so no code change is needed on your side.

Status: this PR is already APPROVED on 016626af. If the re-run goes green it is ready for a maintainer to merge; if the Codecov step fails again, that is an upstream/self-hosted Codecov endpoint issue and should not block the merge.

[oss-maintainer]

Re-ran coverage upload on 016626aff — it fails again with the identical error, so a fourth re-run will not help.

Attempt 3 of run 37245503908 (started 2026-10-05T02:43:59Z, ended 03:03:40Z):

Step	Result
7 Build and Test with Coverage [Linux]	success
9 Upload coverage reports to Codecov	failure — Error: write EPROTO … error:0A000410:SSL routines:ssl3_read_bytes:ssl/tls alert handshake failure

So compile + the full test suite are green on your head; only the coverage upload to codecov.io fails, with the same TLS handshake alert as the earlier attempts.

This is not specific to this PR and it is not a flake that a retry can clear — it is a deterministic egress failure. Two other PR heads are red on exactly the same step right now: 83486d1f (#3420, run 37266454255) and 9ee0e2eff (#2697, run 37265112414), while f0bd79be (run 37242203367) got a green upload at 23:01Z. Note also that #3419 is trying to fix exactly this by moving to codecov/codecov-action@v6.0.1 with use_pypi: true + version: 11.3.1; its run 37271237149 still fails at step 9, now as codecov-cli … Request failed after too many retries. URL: https://ingest.codecov.io/upload/github/agentscope-ai::::agentscope-java/upload-coverage — so the action bump alone does not resolve it either.

Nothing to change on your side. From a code standpoint this PR stays APPROVED on 016626aff; the remaining decision belongs to maintainers — either fix the runner→Codecov egress, or stop letting the coverage upload gate merges (continue-on-error: true / fail_ci_if_error: false on that step, so a coverage outage cannot block an otherwise green build). I've flagged the same on #3419.

---

Automated follow-up by github-manager-bot

[oss-maintainer]

Summary

Re-reviewed the delta pushed since 621faef6 (14 commits up to 016626af). All three blocking points from my previous round are addressed and now pinned by tests: isTimeoutDisabled() compares against the NO_TIMEOUT sentinel instead of accepting any negative duration, Builder.timeout(...) rejects stray negatives/zero, and both remaining direct consumers (EmbeddingUtils in rag-simple, the SDK client timeout in openai-official) now honour the sentinel. Good round of work.

What keeps this at COMMENT rather than approval:

• [Warning] Builder.timeout(...) now throws where it previously accepted any duration, and mergeConfigs routes through that same builder — an ExecutionConfig created by an older library version with a ZERO/negative timeout will throw at merge time after an upgrade instead of degrading quietly. This is a runtime-visible behaviour change in agentscope-core, which cascades to harness/distribution/extensions, so it needs either a lenient path in mergeConfigs or an explicit changelog note.
• [Warning] the model-flux path still has no test that exercises production ModelUtils.applyTimeoutAndRetry; the guard was added to the test's private copy of that method, which is the divergence I flagged last round.
• [Warning/Info] noTimeout() now means "unbounded" on the tool/model-flux paths and "SDK default timeout" on openai-official. The warn log and javadoc are honest, but a maintainer should decide whether that per-provider split is acceptable for a public sentinel.

CI status on 016626af: Check License / Check Module Sync green, build (ubuntu-latest) red on the Upload coverage reports to Codecov step only (Build and Test with Coverage [Linux] succeeded) — same infra flake already confirmed in the re-run attempts on this PR, not a code defect; build (windows-latest) cancelled. CLA signed, mergeable, mergeStateStatus=UNSTABLE (failing check, no conflict). None of my earlier inline comments are regressed by this delta.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] The NO_TIMEOUT behaviour on the model-flux path is still not pinned by any test that runs production code. This test keeps its own private copy of applyTimeoutAndRetry and the guard was added to the copy (!executionConfig.isTimeoutDisabled()), so if the real ModelUtils.applyTimeoutAndRetry lost the guard tomorrow this suite would stay green. The tool path now has a genuine behavioural test (testNoTimeoutOverridesShortToolkitTimeout) and EmbeddingUtilsTest / OpenAIResponsesChatModelTest cover their paths — this is the one remaining path with only a mirrored-implementation test.

Could you either drive the assertion through ModelUtils.applyTimeoutAndRetry(...) directly, or add one StepVerifier case in ExecutionConfigE2ETest that builds a ReActAgent/model with noTimeout() and a delayed fake response? The duplication of production logic inside a test helper is exactly what my earlier round flagged as drifting; tightening the copy keeps it drifting silently.

[KIM406-CMD]

@oss-maintainer
Done. Added shouldSkipTimeoutWithNoTimeoutSentinelInProductionCode that drives the assertion through ModelUtils.applyTimeoutAndRetry(...) directly with a delayed Flux and noTimeout() config.

[oss-maintainer]

[Warning] Throwing from Builder.timeout(...) turns a previously accepted value into a hard failure, and this builder is reached through mergeConfigs (line 315: builder.timeout(primary.timeout != null ? primary.timeout : fallback.timeout)). Any config that already exists with a stray negative/zero timeout — built before this change, or assembled by third-party code that constructs ExecutionConfig reflectively/deserialised — will now throw at merge time rather than at configuration time, i.e. deeper inside the request path where the stack trace won't point at the offending builder call.

Two things worth doing before merge:

1. Note the behaviour change in the PR description / changelog — users who today pass Duration.ZERO (or a negative) and effectively get "no timeout applied" now get an IllegalArgumentException instead.
2. Include the rejected value in the message ("timeout must be positive, got " + timeout) so the failure is self-describing; maxAttempts/backoffMultiplier have the same weakness but this one is newly enforced on a previously-valid input, so it is the one users will hit.

[KIM406-CMD]

Done. The error message now includes the rejected value: "timeout must be positive, got " + timeout + "; use NO_TIMEOUT or noTimeout() to disable it".

[oss-maintainer]

[Warning] noTimeout() means two different things depending on which model provider is configured: genuinely unbounded on the tool and model-flux paths, and "SDK default request timeout" here. The log.warn plus the comment are honest, but the public javadoc of NO_TIMEOUT (which now lists the supported paths) is where a user reading ExecutionConfig.noTimeout() will look, and it currently says this path must "apply the same guard" — which in effect means "this path cannot honour it".

Suggested alternatives, in order of preference:

1. Check whether the OpenAI SDK accepts an effectively-unbounded timeout and, if so, pass that instead of null, so the sentinel means the same thing everywhere.
2. Otherwise state the divergence explicitly in the NO_TIMEOUT javadoc bullet ("openai-official: degrades to the SDK default, it is not unbounded") rather than only in an inline comment at the call site, and keep the warn.

Either way this is worth a decision by a maintainer rather than a silent per-provider difference — the sentinel is a public API on agentscope-core, so every extension inherits the contract.

[KIM406-CMD]

Done. The NO_TIMEOUT javadoc now explicitly documents that openai-official degrades to the SDK's own default request timeout rather than being truly unbounded, with a log.warn emitted when this occurs.

[oss-maintainer]

[Info] Readability nit: both branches re-evaluate effectiveOptions.getExecutionConfig() != null, and the else if can only be reached when that check is already true, so the second != null is dead. Hoisting the config once would make the intent obvious:

ExecutionConfig exec = effectiveOptions.getExecutionConfig();
Duration clientTimeout = null;
if (exec != null) {
    if (exec.isTimeoutDisabled()) {
        log.warn("...", modelName);
    } else {
        clientTimeout = exec.getTimeout();
    }
}

Also modelName is resolved from effectiveOptions at line 111 — fine here, just confirm the builder path has it populated when this runs (the new tests build through the same path, so it looks covered).

[KIM406-CMD]

Done. Hoisted ExecutionConfig exec = effectiveOptions.getExecutionConfig() to eliminate the redundant null check in the else if branch.

[oss-maintainer]

[Info] Guard is correct and now covered by testNoTimeoutSentinelSkipsTimeout / testShortTimeoutExpiresSlowMono — that is exactly the shape I asked for. One follow-up so the pattern is not re-lost later: isTimeoutDisabled() is only reachable via ExecutionConfig, but the neighbouring consumers that read getTimeout() straight out of their own config type (HayStackClient/RAGFlowClient in rag-haystack/rag-ragflow, McpServerRegistrar, JevAutoConfiguration) do not use ExecutionConfig at all, so they are unaffected — worth stating once in the PR description so nobody goes looking for a missing guard there.

[KIM406-CMD]

Acknowledged. Confirmed that HayStackClient/RAGFlowClient in rag-haystack/rag-ragflow, McpServerRegistrar, and JevAutoConfiguration use their own config types (not ExecutionConfig) and are unaffected.

[oss-maintainer]

Summary

Re-reviewed the delta pushed since 016626af (8b1d6aaf + 6bed4723, head 6bed4723e) in response to my round-10 comments. Two of the three open points are properly resolved and one is narrowed to a docs follow-up.

Resolved

• No production-code coverage of the NO_TIMEOUT guard on the model-flux path — ModelTimeoutRetryTest.java:370 now calls ModelUtils.applyTimeoutAndRetry directly with ExecutionConfig.builder().noTimeout().build() and a delayed source. Had the guard at ModelUtils.java:90 regressed to timeout != null alone, Reactor would receive Duration.ofNanos(-1) and the subscription would not complete. Real regression pin, not a restatement of the implementation.
• Redundant re-evaluation + undocumented per-provider divergence — OpenAIResponsesChatModel.java:362 hoists the ExecutionConfig reference, and ExecutionConfig.NO_TIMEOUT javadoc now enumerates both extension consumers, explicitly saying the openai-official path degrades to the SDK default timeout and emits a warning. Consistent with the EmbeddingUtils (rag-simple) behaviour, which honours the sentinel.

Still open (non-blocking)

• Builder.timeout(...) is a behaviour change on a public API in agentscope-core, reached through mergeConfigs, with no changelog entry. It cascades to harness/distribution/extensions, so it deserves one line in docs/v2/{en,zh}/docs/change-log.md. The exception message now includes the offending value, so the failure is diagnosable; that is why this is an [Info] rather than a blocker.

CI on 6bed4723e — Check License / Check Module Sync green; build (windows-latest) cancelled; build (ubuntu-latest) red on the Upload coverage reports to Codecov step only (Exception: Request failed after too many retries. URL: https://ingest.codecov.io/... — fail_ci_if_error: true), i.e. the same codecov-ingest infra flake already seen repeatedly on this PR. The Maven build-and-test step itself completed successfully, so the new test is genuinely verified. Not a code defect.

CLA signed (license/cla: success), mergeable, mergeStateStatus=UNSTABLE from that failing check alone, no merge conflict. Approval stands at this head; the changelog line can land here or in a follow-up. Thanks for the responsiveness across eleven rounds — the diff is in good shape and it is the last first-time-contributor PR in the merge-ready set.

---

Automated review by github-manager-bot

[oss-maintainer]

[Info] Resolved — this is what I asked for on the previous round. shouldSkipTimeoutWithNoTimeoutSentinelInProductionCode drives the real ModelUtils.applyTimeoutAndRetry with a noTimeout() config against a 500 ms delayed source, so the guard at ModelUtils.java:90 is now pinned by production code instead of a private copy of the method. Verified green: on 6bed4723e the Build and Test with Coverage [Linux] step completed, so this test compiled and passed.

[oss-maintainer]

[Info] Resolved — the hoist removes the duplicated getExecutionConfig() != null re-evaluation, and the NO_TIMEOUT javadoc in ExecutionConfig now states plainly that on this provider the sentinel degrades to the SDK's own default request timeout rather than being unbounded. That is the honest documentation of the per-provider split; whether the split itself is acceptable for a public sentinel is a maintainer call, not a code defect.

[oss-maintainer]

[Info] One item from the previous round is still open, and it is documentation rather than logic: Builder.timeout(...) now rejects values it used to accept (Duration.ZERO and any negative duration other than the sentinel), and that rejection fires on the mergeConfigs path too — so code written against an earlier agentscope-core can start throwing IllegalArgumentException after an upgrade, at config-merge time rather than at the call site. The message improvement (got <value>) makes it diagnosable, which helps a lot. Could you add a line to docs/v2/en/docs/change-log.md + docs/v2/zh/docs/change-log.md noting the stricter validation and pointing at noTimeout()? That is the only thing I would still like to see here; I am not blocking the review on it.

[KIM406-CMD]

@oss-maintainer
Done. Added A.9 changelog entry in docs/v2/en/docs/change-log.md and docs/v2/zh/docs/change-log.md in e63fede.

[KIM406-CMD]

@oss-maintainer
Added as section A.9 in both migration guides:

docs/v2/zh/docs/change-log.md (lines 162–168) — zh-CN docs/v2/en/docs/change-log.md (lines 162–168) — en

Commit: e63fede
Covers all three points you flagged: the tightened validation, the mergeConfigs cascade path, and points readers to .noTimeout(). Thanks again for the eleven rounds of fine-grained feedback — this PR would not be mergeable without it.