fix(aistio): dual-name collaboration tools for OpenAI function.name - #3205
tengjiaozhai wants to merge 1 commit into
Conversation
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
The dual-name design (dotted wire names for MCP dispatch, . → _ model names for OpenAI-compatible function.name) is a sound fix for #3153, and the registration / availableActions / terminal-check paths are consistently reworked. However, this cannot be approved in its current state:
Critical: in AgentTaskCollaborationToolTest.discoveresAndCallsTaskScopedMcpTool the escaped quote was dropped from a JSON string literal (+ " a comment","), which leaves the literal unbalanced and makes the test source non-compiling — CI should fail before any of the new assertions run.
Please restore \" on that line and re-run mvn -pl agentscope-extensions/agentscope-extensions-aistio test-compile (the PR reports 76 tests passing, so this looks like it slipped in on the last rebase).
Non-blocking suggestions: an explicit hint about toModelName() collisions (foo.bar_baz vs foo_bar.baz, or a pre-existing task_submit_result shadowing the outcome tool) plus a test pinning that behavior, and a note on documenting the task.submit_result rename in release notes for resumed sessions / external references.
Good first contribution — the fix rationale, shared wire-name constants, and the added dual-name tests are all well done. Once the test file compiles again, re-request review and we can move quickly.
Automated review by github-manager-bot
|
Soft review items addressed (no further code churn):
Version remains 2.0.3-SNAPSHOT. Code fixes (escaped quote + collision warn) already in |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at 57093bc. Both threads from the previous request are resolved:
- The dropped
\"in thediscoversAndCallsTaskScopedMcpToolfixture is restored — the literal is balanced again and CI confirmsbuild (ubuntu/windows-latest)pass on this commit. - The model-name collision suggestion was taken further than asked:
registerCollaborationToolsnow logs a warning identifying the skippedwireName/modelNamepair, andcollidingModelNameKeepsFirstRegisteredToolpins the keep-first semantics (including that the pre-existing local tool is not replaced).
No new issues found in the delta. Nice turnaround — approving.
Automated review by github-manager-bot
|
This PR currently conflicts with git fetch origin
git checkout fix/aistio-collaboration-tool-model-names
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 |
Rebased onto upstream main (c0d03cc). Keep wireName/modelName dual naming, collision warn, escaped quote in tests, and main's ConfirmResult denyMessage HITL path.
57093bc to
f838e87
Compare
|
Rebased onto current upstream
Ready for re-review, cc @oss-maintainer |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after the rebase onto c0d03ccea. The dual-name design is intact and the conflict resolution looks correct: wireName (dotted) drives MCP dispatch, isReadOnly(), and the terminal-action checks, while getName() now returns the OpenAI-safe modelName; the wire-name constants are used for the task.complete / task.fail exclusion, and ConfirmResult(..., decision.denyMessage()) matches main's current 4-arg HITL signature. The added tests (modelNameUsesUnderscoresWhileWireNameKeepsDots, terminalWireNamesStillTriggerMarkTerminalCommitted, collidingModelNameKeepsFirstRegisteredTool, sameToolkitRegistersOutcomeToolOnlyOnce) cover the right seams, including asserting that params.name on the wire stays dotted.
Not re-approving this round: once normalization is in play, name presence in the toolkit is no longer proof of tool identity, and the outcome tool relies on exactly that proof — plus two compatibility questions created by the rename.
Findings
- [Critical]
adapter/HarnessAgentTaskStarter.java:440— thecontains(submitResultModelName)guard treats any tool namedtask_submit_resultas the adapter's outcome tool. If a foreign tool holds that name,AgentTaskOutcomeToolis never registered, no outcome can be submitted, andrunToOutcomespins on its "turn ended without a business outcome" nudge until the budget is exhausted — silently, unlike the collaboration-loop collision which does log. Check identity (getTool(name) instanceof AgentTaskOutcomeTool) instead. - [Warning]
adapter/AgentTaskCollaborationTool.java:97—toModelNamenormalizes only., so the^[a-zA-Z0-9_-]+$property claimed in the class comment holds just for dotted names; core already has a stricter sanitizer inSubAgentTool.resolveToolName(^[a-zA-Z0-9_-]{1,64}$with a hash fallback) that would also shrink the collision surface. - [Warning]
adapter/AgentTaskOutcomeTool.java:32—task.submit_result→task_submit_resultbreaks persisted permission rules, becausePermissionEngine.rulesFor(...)is an exact key lookup ontool.getName()over theallow_rules/deny_rules/ask_rulesmaps stored inPermissionContextState. Previously-allowed work can re-prompt, or DENY underDONT_ASK. Needs confirmation plus a migration or dual-spelling lookup. - [Info]
adapter/HarnessAgentTaskStarter.java:464— a shadowed action is still advertised inavailableActions; if a terminal action is the one dropped, the leader stalls. Consider naming dropped pairs in the prompt or refusing the dispatch. - [Info]
test/HarnessAgentTaskOutcomeTest.java:379— two more cases worth pinning: outcome-tool shadowing, and a wire name outside[A-Za-z0-9_-].
Notes
- Static review only; I did not compile or run the module in this sweep (the local mirror could not be synced this cycle), so the CI run remains the authority on build/test status.
- The
roleInstructionsstring concatenation reads a little awkwardly after interpolation (e.g." also" + " completes ..."), but the output is correct and the newassertFalse(...contains("run.node.complete"))assertions are a nice guard against the dotted spelling leaking back into prompts.
Automated review by github-manager-bot
| String submitResultModelName = | ||
| AgentTaskCollaborationTool.toModelName( | ||
| AgentTaskCollaborationTool.WIRE_TASK_SUBMIT_RESULT); | ||
| if (!runtimeAgent.getToolkit().getToolNames().contains(submitResultModelName)) { |
There was a problem hiding this comment.
After the '.' -> '_' normalization, this guard assumes "a tool named task_submit_result already in the toolkit must be our outcome tool". That assumption is not safe — a foreign tool can occupy the same model name, which is exactly the situation the new collidingModelNameKeepsFirstRegisteredTool test demonstrates for task_get.
If it happens here, AgentTaskOutcomeTool is never registered, no business outcome can ever be submitted, and runToOutcome keeps emitting its "The turn ended without a business outcome" nudge until the turn budget is exhausted. The collaboration-tool loop below at least logs a warning on collision; this path is silent.
Suggest matching on identity rather than name, e.g. getTool(submitResultModelName) instanceof AgentTaskOutcomeTool, and either replacing the shadowed tool or failing the dispatch loudly — this is the one model-facing name the completion protocol cannot do without.
| if (wireName == null) { | ||
| return null; | ||
| } | ||
| return wireName.replace('.', '_'); |
There was a problem hiding this comment.
Only . is normalized, while the class comment states the constraint as function.name =~ ^[a-zA-Z0-9_-]+$. Wire names containing any other character outside that class (:, /, whitespace, non-ASCII) pass through unchanged, so the "OpenAI-safe" property holds only for the dotted-names case the control plane happens to publish today; length is also unbounded.
Core already has a sanitizer for the same rule (SubAgentTool.resolveToolName, documented as ^[a-zA-Z0-9_-]{1,64}$ with a deterministic-hash fallback). Reusing it here — or asserting the regex and falling back to a stable suffixed name — would make the mapping total instead of case-specific, and would also remove the collision surface this PR had to add a warning for.
| public final class AgentTaskOutcomeTool { | ||
| @Tool( | ||
| name = "task.submit_result", | ||
| name = "task_submit_result", |
There was a problem hiding this comment.
Renaming the model-facing tool from task.submit_result to task_submit_result also invalidates persisted permission rules: PermissionEngine.rulesFor(...) does an exact key lookup on tool.getName() against the allow/deny/ask tables, and those tables are persisted in PermissionContextState (allow_rules / deny_rules / ask_rules). A session or stored config that granted ALLOW for the dotted spelling stops matching, so previously-approved work re-prompts — or, under DONT_ASK, falls through to DENY.
Could you confirm nothing deployed references the dotted name (session stores, seeded rules, prompt templates), and either migrate those entries or accept both spellings during rule lookup? Worth one line in the docs/release note either way.
| new AgentTaskCollaborationTool(collaboration, definition)); | ||
| } else { | ||
| LOG.warning( | ||
| "Skipping collaboration tool registration due to model-name collision:" |
There was a problem hiding this comment.
Logging collisions is the right call. One gap remains: the dropped action is still advertised by availableActions, so the only signal the model gets is the Actual tool names available to this agent: line at the end of the prompt. If a terminal action (run.node.complete / run.node.fail) is the one shadowed, a leader can never converge and the run just stalls.
Consider naming the dropped wire/model pairs explicitly in the prompt, or refusing the dispatch when a terminal action cannot be exposed.
| } | ||
|
|
||
| @Test | ||
| void collidingModelNameKeepsFirstRegisteredTool() throws Exception { |
There was a problem hiding this comment.
Good coverage of the rebase follow-ups — the local-collision case and the idempotent re-dispatch case are both pinned here. Two more tests would lock down the risky paths flagged inline above:
- outcome-tool shadowing: pre-register a foreign tool under
task_submit_resultand assert the dispatch either still captures a business outcome or fails loudly; - a wire name outside
[A-Za-z0-9_-](e.g.mcp:server.tool), so the normalization contract is specified rather than implied by the dotted-name fixture.
2 participants
AgentScope-Java Version
2.0.3-SNAPSHOT (based on upstream/main at
d70910700793329ecd45c927f2a79c282d8bc47e)Description
OpenAI-compatible APIs require
function.name =~ ^[a-zA-Z0-9_-]+$. Collaboration tools were registered with dotted control-plane wire names (task.get,issue.comment.add, …), so every AgentTask model request against OpenAI/DeepSeek-compatible gateways failed with HTTP 400.original：https://raw.githubusercontent.com/openai/openai-openapi/refs/heads/main/openapi.yaml
Option 1 (adapter dual naming) — this PR:
AgentTaskCollaborationToolkeeps two names:tools/call,READ_ONLY, and terminalrun.node.complete/run.node.failchecks'.' → '_'via sharedtoModelName()): returned fromgetName()for the LLM / OpenAI payloadHarnessAgentTaskStarter.registerCollaborationToolsregisters and dedupes by model name; lifecycle skip fortask.complete/task.failuses shared wire-name constants (WIRE_TASK_COMPLETE/WIRE_TASK_FAIL).roleInstructions(and kickoff / correction prompts) use model-facing underscore names derived from the sametoModelName()mapping so prose cannot drift from registration.AgentTaskOutcomeTool:task.submit_result→task_submit_result(same mapping). This generalizes / may supersede the manual rename in fix(aistio): rename task.submit_result tool to satisfy OpenAI function-name rules #3151.Wire dispatch to the control plane is unchanged (still dotted). Option 2 (core
Toolkitpattern validation) is left as a follow-up only.Closes #3153
Relationship to #3151
#3151 only renames the adapter-owned
task.submit_resulttool. This PR applies the same underscore rule to all collaboration tools plus that outcome tool, so naming stays consistent. If #3151 merges first, this branch should rebase cleanly over the overlapping lines inHarnessAgentTaskStarter/ tests; if this lands first, #3151 becomes redundant and can be closed.Compatibility / release notes
Breaking for callers that hardcode the old outcome tool name: the model-facing name is now
task_submit_result(wastask.submit_result). Wire/dispatch for dotted collaboration tools is unchanged. Callers, prompts, and resumed sessions that hardcode the old dotted outcome name should use the underscore form.Verification
mvn -pl agentscope-extensions/agentscope-extensions-aistio -am spotless:apply mvn -pl agentscope-extensions/agentscope-extensions-aistio spotless:check mvn -pl agentscope-extensions/agentscope-extensions-aistio -am testagentscope-extensions-aistio: 77 tests, 0 failures (includes new dual-name + terminal-wire coverage, outcome-tool idempotent registration, and collision keep-first pin)-am testChecklist
mvn spotless:applymvn test): aistio module 77/0