Conversation
…n-name rules AgentTaskOutcomeTool registers the tool as task.submit_result, but OpenAI-compatible APIs (OpenAI, DeepSeek) require function names to match ^[a-zA-Z0-9_-]+$. The dotted name makes every AgentTask conversation fail with HTTP 400: Invalid 'tools[i].function.name': string does not match pattern Rename to task_submit_result in the tool registration and in the five prompt/checks inside HarnessAgentTaskStarter that reference it, keeping the model instructions and toolkit membership check consistent.
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Codecov flagged the changed lines in HarnessAgentTaskStarter as uncovered. Add coverage for the rename: - AgentTaskCollaborationToolTest asserts the worker and leader role instructions reference task_submit_result. - HarnessAgentTaskOutcomeTest dispatches a second task on the same toolkit so the already-registered branch of registerCollaborationTools is exercised.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Correct, targeted fix: the dotted task.submit_result really does violate the OpenAI-compatible ^[a-zA-Z0-9_-]+$ function-name rule, the rename is applied consistently to the registration, the toolkit guard and all five prompt references, and the module test suite is green on CI. Verdict: the change is right, the concerns below are about keeping it right.
Two things I would like addressed before this is merged:
- The magic string is still a magic string, now in five places. Extract a
static final String TOOL_NAMEonAgentTaskOutcomeTooland reference it from the annotation, thegetToolNames().contains(...)guard and the prompt builders, so the next rename cannot leave the registration and the prompt out of sync (that drift is exactly what produced this bug). - Dotted names remain in the same prompt block (
task.complete,task.fail,run.node.complete,run.node.fail,run.replan,issue.child.create). They are harmless while they are only prohibitions, but the block also states thatavailableActionstools are registered "with the exact names shown", i.e. control-plane supplied — please confirm none of those names is dotted, otherwise leader turns that delegate will hit the same HTTP 400.
Also flagged: the compatibility question for in-flight AgentTask sessions that recorded the old name, and a cheap guard (validate tool names at registration in core) that would make this bug class impossible for every model provider. Thanks for the thorough description and for pointing out the Go control-plane mismatch — that is real cross-module drift, worth an issue.
Findings
- [Warning]
AgentTaskOutcomeTool.java:32— tool name duplicated as a literal in 5 places; hoist to astatic finalconstant. - [Warning]
HarnessAgentTaskStarter.java:135— dotted names still present in the same prompt block; confirm noavailableActionsname is dotted, and consider validating names at registration time. - [Warning]
HarnessAgentTaskStarter.java:436— renaming a registered tool affects persisted session state; confirm replay behavior for sessions recorded with the old name. - [Info]
HarnessAgentTaskStarter.java:469— prompt literals are not asserted anywhere; a small contains-check would lock the text.
Cross-repo Note
agentscope-service/aistio/internal/runtimebinding/resolver.go still emits dotted tool names in task prompts while the Java side registers underscore names (the author flagged this). That drift belongs in a follow-up issue so the control plane and the adapter agree on one naming rule.
Automated review by github-manager-bot
| public final class AgentTaskOutcomeTool { | ||
| @Tool( | ||
| name = "task.submit_result", | ||
| name = "task_submit_result", |
There was a problem hiding this comment.
[Warning] The registered name and the prompt/guard references are now the same string literal repeated in 5 places across two files (HarnessAgentTaskStarter role instructions, the "call task_submit_result" nudge, the retry message and the getToolNames().contains("task_submit_result") guard). Any future rename has to touch all of them, and a partial update fails silently: the registration would succeed while the guard re-registers a duplicate tool, or the model would be told to call a name that is not registered.
Could you hoist it into a constant and reference it everywhere?
public final class AgentTaskOutcomeTool {
/** Registered tool name; must match `^[a-zA-Z0-9_-]+$` (OpenAI-compatible function-name rule). */
public static final String TOOL_NAME = "task_submit_result";
@Tool(name = TOOL_NAME, description = "...")and in the adapter: if (!runtimeAgent.getToolkit().getToolNames().contains(AgentTaskOutcomeTool.TOOL_NAME)). Annotation values must be compile-time constants, which a static final String literal satisfies.
| + " the exact names shown in availableActions. The adapter owns" | ||
| + " task.complete and task.fail; do not call them. Before returning," | ||
| + " call task.submit_result with an explicit business outcome and the" | ||
| + " call task_submit_result with an explicit business outcome and the" |
There was a problem hiding this comment.
[Warning] This is exactly the line where the dotted-name problem survives the rename: task.complete, task.fail, run.node.complete, run.node.fail, run.replan, issue.child.create are all still spelled with dots here and in roleInstructions (below), and a name containing . is rejected by OpenAI-compatible /chat/completions with the same HTTP 400 you are fixing here (^[a-zA-Z0-9_-]+$) as soon as such a tool is actually registered.
Today the first two are prohibitions ("do not call them") so no request payload is generated for them, which is why the PR is enough to unblock the reported failure — but the text still teaches the model a name shape that cannot be called, and the same block says the collaboration tools are registered "with the exact names shown in availableActions", i.e. control-plane supplied. Two questions:
- Are any
availableActionsnames (e.g.issue.child.create) registered as tools verbatim? If yes, the 400 will come back for leader turns that delegate. - Is there a plan to align the Go control plane (you flagged
agentscope-service/aistio/internal/runtimebinding/resolver.go)? A one-line follow-up issue would be helpful so the prompt strings and the registered names do not diverge per-repo.
Optional hardening that would make the class of bug impossible: validate every name at registration time in the core toolkit, e.g. throw/warn when a tool name does not match ^[a-zA-Z0-9_-]{1,64}$, instead of discovering it at the first model request.
| Object toolkit = runtimeAgent.getToolkit(); | ||
| synchronized (toolkit) { | ||
| if (!runtimeAgent.getToolkit().getToolNames().contains("task.submit_result")) { | ||
| if (!runtimeAgent.getToolkit().getToolNames().contains("task_submit_result")) { |
There was a problem hiding this comment.
[Warning] Compatibility check for the rename. The tool name is part of what gets persisted in agent session/memory state and replayed on recovery: a session recorded before the upgrade contains a task.submit_result tool-use block, while after the upgrade the toolkit only exposes task_submit_result. Could you confirm how state recovery / history replay handles a tool-use block whose name is no longer registered (ignored, error, or fail-fast)? If it is a hard error, in-flight AgentTasks resumed after an upgrade would break.
If a transition is needed, the usual cheap pattern is to keep a hidden alias for one release:
runtimeAgent.getToolkit().registerTool(new AgentTaskOutcomeTool()); // task_submit_result
runtimeAgent.getToolkit().registerAlias("task.submit_result", AgentTaskOutcomeTool.TOOL_NAME); // deprecated, one release(if the toolkit has no alias support, documenting "restart in-flight AgentTasks after upgrading" in the PR description is enough — the repo has no versioned-upgrade note for extension tool names that I can find.)
| + " Issues and do not call run.node.complete, run.node.fail, or run.replan." | ||
| + " Complete only the assigned work and submit its result using" | ||
| + " task.submit_result; the adapter will complete this AgentTask."; | ||
| + " task_submit_result; the adapter will complete this AgentTask."; |
There was a problem hiding this comment.
[Info] Nit: these two prompt rewrites now break mid-word (using + task_submit_result; and calling + task_submit_result with waiting). Spotless keeps the line split, but since the new name is longer than the old one it is worth a manual pass over the rendered string to make sure no doubled/missing space slipped in — the concatenated literal is what the model actually sees, and it is not covered by any assertion. A tiny test like assertTrue(roleInstructions(...).contains(" task_submit_result ")) (or a snapshot of the worker/leader instructions) would lock the prompt text cheaply.
|
Added a follow-up commit addressing the Codecov patch-coverage report: the changed lines in
Local verification: |
|
Follow-up to my review above — I found the answer to my own open question, and the dotted-name problem is not fully closed.
if ("task.complete".equals(name) || "task.fail".equals(name)) { // :444
continue;
}
...
runtimeAgent.getToolkit().registerAgentTool(
new AgentTaskCollaborationTool(collaboration, definition)); // :453 — `name` registered verbatimLine 444 only proves the two lifecycle names are dotted, but anything else that survives the Two small options:
Not requesting changes on this diff for it: your change is correct, targeted and unblocks the reported failure. I would only like the remaining dotted names tracked in a linked issue instead of staying as prose in the description, so the leader path does not re-introduce the same 400 after this merges. Follow-up note by github-manager-bot |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of the delta since my previous pass (03867177 -> c972c1ef). The new commit is test-only and it does land what it claims: AgentTaskCollaborationToolTest now pins the renamed literal in both the worker and leader role instructions, which closes the "prompt text is asserted nowhere" item from my last review, and the second dispatch in HarnessAgentTaskOutcomeTest exercises the already-registered branch of registerCollaborationTools that the rename touched. CI is green on this head (build on ubuntu and windows, Check License, Check Module Sync, codecov/patch) and the CLA is signed.
Not approving yet, for two reasons that are unchanged by this commit:
- The name is still a magic string in five places.
AgentTaskOutcomeTool@Tool(name = "task_submit_result"), thegetToolNames().contains("task_submit_result")guard, and the three prompt sites still repeat the literal. Astatic final String TOOL_NAMEonAgentTaskOutcomeToolreferenced from the annotation, the guard and the prompt builders would make the registration/prompt drift that produced this bug impossible. This is the only substantive thing I asked for last time and it is still open. - The dotted-name class is not closed, only moved. As you confirmed in your own follow-up,
registerCollaborationToolsregisters control-plane action names verbatim after thetask.complete/task.failskip, androleInstructionsstill spellsissue.child.create,run.node.complete,run.node.fail,run.replanwith dots. So a leader turn that delegates can still build atools[]entry that OpenAI-compatible endpoints reject with the same HTTP 400. Please track that as a linked issue before merge (either normalize withname.replace('.', '_')at registration in this adapter, or validate registered names against^[a-zA-Z0-9_-]{1,64}$once inagentscope-core, which would protect every provider).
The two inline notes below are non-blocking test-strength nits on the new commit.
Automated review by github-manager-bot
| new byte[0], | ||
| 1); | ||
| starter.start(second).block(); | ||
| verify(agent, times(2)).call(any(Msg.class), any(RuntimeContext.class)); |
There was a problem hiding this comment.
[Info] The test name says "OnlyOnce", but nothing here asserts "only once" — verify(agent, times(2)).call(...) plus verify(client, times(2)).finish(...) only prove both dispatches completed. starter() already stubs agent.getToolkit() to return one shared Toolkit instance, so the stronger assertion is cheap, e.g. wrap it in a spy and verify(spyToolkit, times(1)).registerTool(any(AgentTaskOutcomeTool.class)), or snapshot getToolNames() before/after the second start(...). As written this closes the Codecov branch-coverage gap (which is what the commit set out to do), but a future regression that re-registers the tool and silently replaces the first instance would still pass.
| starter.start(assignment).block(); | ||
| // A second dispatch on the same toolkit must hit the "already registered" branch of | ||
| // registerCollaborationTools and still complete normally. | ||
| AgentTaskAssignment second = |
There was a problem hiding this comment.
[Info] Nit: the 12-argument AgentTaskAssignment is now duplicated verbatim except for attemptId. Since assignment is already a field, something like assignment.withAttempt("attempt-2") (or a one-line local helper) keeps a future record-shape change from breaking this test in two places instead of one.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Renames the AgentTaskOutcomeTool registration and its prompt/guard references from task.submit_result to task_submit_result so the tool list passes the ^[a-zA-Z0-9_-]+$ name pattern enforced by OpenAI-compatible endpoints (OpenAI, DeepSeek). Good catch and a correctly scoped fix — I verified that this is the only dotted @Tool(name=...) value in the repository and that no other module, doc or permission rule references the old string, so nothing outside agentscope-extensions-aistio needs a matching change. CLA signed, all checks green (build ubuntu/windows, codecov/patch, license), mergeable with mergeStateStatus=BLOCKED only because the repo still requires a maintainer approval.
Findings
- [Warning]
AgentTaskOutcomeTool.java:32— the rename is not retro-active for persisted sessions:ToolUseBlockstores the tool name in message history andJsonFileAgentStateStorepersists it, so a resumed pre-rename session can still hit the same request-level name-pattern rejection. Needs confirmation, or a legacy-name rewrite / release note. - [Warning]
HarnessAgentTaskStarter.java:135— the prompt still names dotted tools (task.complete,task.fail,run.node.complete,run.replan) that are not registered under those names; this is exactly the class of confusion the PR removes. - [Info]
HarnessAgentTaskStarter.java:436— the name literal now lives in 6 places with only 2 string assertions pinning it; suggest a sharedpublic static finalconstant. - [Info]
HarnessAgentTaskOutcomeTest.java:81—sameToolkitRegistersOutcomeToolOnlyOncedoes not actually assert the "only once" invariant; the 12-argAgentTaskAssignmentis duplicated verbatim.
No blocking issues: the two warnings are about the blast radius of the rename and prompt wording, not about the code path being wrong. Re-reviewing the new commits confirms they are a merge of main plus the follow-up test commits that closed the earlier Codecov patch-coverage gap; the retry loop in runToOutcome is bounded by the corrections++ > 0 guards, so the "call the outcome tool again" nudge cannot loop forever.
Automated review by github-manager-bot
| public final class AgentTaskOutcomeTool { | ||
| @Tool( | ||
| name = "task.submit_result", | ||
| name = "task_submit_result", |
There was a problem hiding this comment.
Compatibility of the rename for already-persisted sessions. ToolUseBlock keeps the tool name inside the message payload, and JsonAgentStateStore/JsonFileAgentStateStore persist those messages, so a session created before this rename still replays history containing task.submit_result. For the same OpenAI-compatible endpoint that rejects the definition with HTTP 400, the tools[i].function.name pattern check is a request-level validation, so resuming such a session can keep failing even after this fix lands.
Could you confirm the behaviour on a resumed session with pre-rename history? If it does fail, the options are a read-side rewrite when loading legacy sessions (map the old name to the new one), or stating explicitly in the changelog that in-flight AgentTasks must be re-dispatched rather than resumed.
| Object toolkit = runtimeAgent.getToolkit(); | ||
| synchronized (toolkit) { | ||
| if (!runtimeAgent.getToolkit().getToolNames().contains("task.submit_result")) { | ||
| if (!runtimeAgent.getToolkit().getToolNames().contains("task_submit_result")) { |
There was a problem hiding this comment.
The registered name is now the same literal repeated in 6 places across two files (this guard, the @Tool(name=...), and 4 prompt strings); only two of them are pinned by string-equality assertions in AgentTaskCollaborationToolTest, so a future rename can still drift silently.
Suggested follow-up (no need to block this PR on it):
public static final String SUBMIT_RESULT_TOOL = "task_submit_result";...and build the prompt sentences with that constant (or a text...formatted(...) call) so registration, the guard and the instructions cannot disagree.
| + " the exact names shown in availableActions. The adapter owns" | ||
| + " task.complete and task.fail; do not call them. Before returning," | ||
| + " call task.submit_result with an explicit business outcome and the" | ||
| + " call task_submit_result with an explicit business outcome and the" |
There was a problem hiding this comment.
This is the line where the dotted-name problem survives the rename: the model is still told about task.complete, task.fail, run.node.complete, run.node.fail and run.replan, none of which are registered under those names by this module. The intent here is "do not call these", which is fine in prose, but a model that tries one of them gets an unknown-tool error instead of a clean validation message.
task.submit_result is the only dotted @Tool name in the whole repository (I grepped), so this PR does fix the only real blocker. For consistency I would phrase the negative list as capabilities the adapter owns ("completing and failing the task is owned by the adapter; never attempt to call a task-completion tool yourself") rather than by name, or state the [a-zA-Z0-9_-]+ convention once so future tool authors do not reintroduce a dotted name.
| return new HarnessAgentTaskStarter(() -> agent, client); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Nice addition — the second dispatch does exercise the "already registered" branch of registerCollaborationTools, which the previous patch-coverage gap pointed at. Strictly speaking the assertions (times(2) on call/finish) prove "a second dispatch still works", not "registered only once".
To assert the actual invariant, something like this is cheap:
verify(agent.getToolkit(), org.mockito.Mockito.never())
.registerTool(org.mockito.ArgumentMatchers.any(AgentTaskOutcomeTool.class));
// or: assertEquals(1, countOf(agent, AgentTaskOutcomeTool.class))Also consider a small factory for the 12-argument AgentTaskAssignment since it is now duplicated verbatim apart from attemptId.
|
This PR currently conflicts with git fetch origin
git checkout fix/aistio-task-outcome-tool-name
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review once the conflicts are resolved. Automated notification by github-manager-bot |
This branch has not been deployed
3 participants
AgentScope-Java Version
Current
main(97696a3, 2026-09-15).Description
AgentTaskOutcomeToolregisters its tool astask.submit_result, but OpenAI-compatible APIs (OpenAI, DeepSeek) requirefunction.nameto match^[a-zA-Z0-9_-]+$. The dotted name makes every AgentTask conversation fail on the first model request with:This renames the tool to
task_submit_resultin the registration and in the five prompt/toolkit references insideHarnessAgentTaskStarter, keeping the model instructions and thegetToolNames().contains(...)check consistent with the registered name.How to test
Validated locally end-to-end with DeepSeek (
deepseek:deepseek-chat): after the rename the tool list passes server-side validation and task turns complete normally.Related observation (not changed here)
The Go control plane's task prompts still reference dotted names (
task.get,task.complete,math.evaluate, seeagentscope-service/aistio/internal/runtimebinding/resolver.go) while the registered tools use underscores (task_get,task_complete,math_evaluate). Out of scope for this PR; flagging in case maintainers want a follow-up.Checklist
mvn spotless:applymvn test):agentscope-extensions-aistio73 tests, upstream dependency modules (core/harness) 1040 tests, 0 failures