fix(aistio): dual-name collaboration tools for OpenAI function.name - #3205
tengjiaozhai wants to merge 3 commits 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.
There was a problem hiding this comment.
Fixed in 095675e.
ensureOutcomeToolRegistered now identity-checks getTool(task_submit_result) instanceof AgentTaskOutcomeTool (outcome tool converted to a ToolBase so the check is real, not ReflectiveFunctionTool-wrapped). A foreign occupant is removed with a warning and replaced; idempotent re-dispatch stays quiet. Covered by foreignOutcomeToolIsReplacedSoBusinessOutcomeStillWorks.
| 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.
There was a problem hiding this comment.
Fixed in 095675e.
toModelName is now a total sanitizer matching ^[a-zA-Z0-9_-]{1,64}$: dotted control-plane names keep the fast '.'→'_' path; anything else (e.g. mcp:server.tool) is sanitized with a deterministic 8-hex hash suffix (same intent as core SubAgentTool.sanitizeName, without the call_ prefix). Class javadoc updated. Covered by toModelNameSanitizesWireNamesOutsideOpenAiCharset.
| 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.
There was a problem hiding this comment.
Fixed in 095675e.
PermissionEngine.rulesFor now also accepts legacy dotted spellings whose '.'→'_' form equals the live tool name (and the reverse when the live name is still dotted). So persisted task.submit_result / issue.comment.add allow/deny/ask keys keep matching task_submit_result / issue_comment_add. Brief note on AgentTaskOutcomeTool + Compatibility section in the PR body. Covered by PermissionEngineTest$DualSpellingLookup.
| 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.
There was a problem hiding this comment.
Fixed in 095675e.
Dropped colliding wire→model pairs are now listed in the kickoff prompt (Dropped collaboration tools due to model-name collision (not callable): …). If a terminal action (run.node.complete / run.node.fail) cannot be exposed, dispatch fails loudly with IllegalStateException. Covered by terminalActionCollisionRefusesDispatch.
| } | ||
|
|
||
| @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.
There was a problem hiding this comment.
Fixed in 095675e.
Added:
foreignOutcomeToolIsReplacedSoBusinessOutcomeStillWorks— foreigntask_submit_resultis replaced; business outcome still finishes.toModelNameSanitizesWireNamesOutsideOpenAiCharset—mcp:server.toolnormalization contract.terminalActionCollisionRefusesDispatch— terminal shadow refuses dispatch.
aistio module: 81 tests, 0 failures.
Replace outcome-tool name guard with AgentTaskOutcomeTool identity check (replace foreign occupants), make toModelName a total OpenAI-safe sanitizer with hash fallback, accept dotted/underscore permission rule aliases, refuse terminal model-name collisions and list dropped tools in the kickoff prompt, and pin the risky paths with tests.
oss-maintainer review follow-up (
|
| Finding | Fix |
|---|---|
Critical — silent skip when foreign tool occupies task_submit_result |
Identity check instanceof AgentTaskOutcomeTool; replace foreign occupant with warning |
Warning — toModelName only mapped . |
Total sanitizer ^[a-zA-Z0-9_-]{1,64}$ + deterministic hash fallback |
| Warning — permission rules break on rename | PermissionEngine.rulesFor dual-spelling (dotted ↔ underscore via '.'→'_' alias) |
| Info — dropped actions still advertised | Dropped pairs named in kickoff prompt; terminal collision refuses dispatch |
| Info — missing tests | Outcome shadowing, mcp:server.tool normalization, terminal collision |
Verify: mvn -pl agentscope-extensions/agentscope-extensions-aistio -am spotless:apply && spotless:check && test → aistio 81 tests, 0 failures.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review after 095675ea ("address oss-maintainer dual-name review findings"). All five findings from the previous round are genuinely resolved, and each fix is pinned by a test rather than asserted in prose:
- outcome tool registration is now identity-checked (
ensureOutcomeToolRegistered→getTool(...) instanceof AgentTaskOutcomeTool, foreign occupant removed and replaced) —foreignOutcomeToolIsReplacedSoBusinessOutcomeStillWorks toModelNameis now a total sanitizer matching coreSubAgentToolsemantics (charset + 64-char cap + deterministic hash suffix) —toModelNameSanitizesWireNamesOutsideOpenAiCharset- the terminal-action collision no longer stalls the leader silently: it is reported into the kickoff prompt and
terminalActionCollisionRefusesDispatchrefuses dispatch task.complete/task.failexclusion moved onto the sharedWIRE_*constants- the
task.submit_resultrename keeps persisted permission rules matching viaPermissionEngine#rulesFor
CI is green on the new head on both runners, CLA signed, MERGEABLE/CLEAN. Not approving this round for one reason only: the compatibility fix for an agentscope-extensions-aistio tool name was implemented in agentscope-core's permission engine, as an unconditional bidirectional name-alias merge for every tool. That is a much wider blast radius than the bug it fixes, and it is the one part of the PR where I would want either a narrower mechanism or an explicit maintainer sign-off. Everything else here I would take as-is.
Findings
- [Warning]
core/.../PermissionEngine.java:329— the alias merge is bidirectional and unconditional, so a rule authored for toola.bcan now match a different tool registered asa_b(allow and deny directions). - [Warning]
core/.../PermissionEngine.java:310—rulesFordegrades from an O(1) map hit to a full table scan withreplace()allocations on every permission check for any tool without an exact-key rule set; it runs three times per tool invocation. - [Info]
aistio/.../AgentTaskOutcomeTool.java:45—LEGACY_DOTTED_NAMEis unused, so it cannot keep the two spellings in sync with the engine. - [Info]
aistio/.../AgentTaskOutcomeTool.java:130— a[null]inpending_task_idsescapes as an NPE fromStream.toList()instead of a tool error. - [Info]
aistio/.../HarnessAgentTaskStarter.java:574— interpolation-driven string splitting; output is correct, readability is not.
Notes
- Static review only this cycle; CI on head
095675ea(ubuntu + windows build, license, module sync, codecov/patch) is all green and is the authority on build/test status. - If the alias behaviour is deliberate for 2.0 adoption, consider a core-level follow-up issue so the trade-off is documented rather than implicit in a helper's comment.
Automated review by github-manager-bot
| toolName.equals(key.replace('.', '_')) | ||
| || (toolName.contains(".") && key.equals(toolName.replace('.', '_'))); | ||
| if (!alias) { | ||
| continue; |
There was a problem hiding this comment.
[Warning] The alias merge is bidirectional and unconditional, so it is no longer true that a rule only ever constrains the tool it was authored for.
toolName.equals(key.replace('.', '_')) matches any table key whose dots were turned into underscores. A DENY stored for a real dotted tool a.b now also denies a completely unrelated tool that happens to be registered as a_b (and the reverse branch does the symmetric thing for ALLOW). Both directions are privilege-relevant in a permission engine, and this is agentscope-core — it applies to every tool in every deployment, not just the aistio adapter.
Could the match be narrowed to names the tool itself declares as legacy spellings?
// ToolBase: default no-alias, AgentTaskOutcomeTool overrides to return LEGACY_DOTTED_NAME
List<String> aliases = tool.nameAliases();
for (String alias : aliases) {
merged.addAll(table.getOrDefault(alias, List.of()));
}At minimum, requiring toolName to contain no dot for the forward branch (i.e. only an underscore-normalised name may pull in a dotted key) would stop the reverse direction from granting an ALLOW authored for a first-class underscore tool to a dotted twin. Either way, the javadoc above ("the legacy dotted spelling") should describe what the code actually does today.
There was a problem hiding this comment.
Fixed in 2ab8a85.
Replaced the unconditional bidirectional string-shape merge with opt-in ToolBase.nameAliases() (default empty). PermissionEngine.rulesFor now only merges keys the tool itself declares — AgentTaskOutcomeTool returns LEGACY_DOTTED_NAME (task.submit_result) only. Unrelated a.b / a_b pairs no longer cross-match in either direction (underscore ALLOW no longer grants a dotted twin). Javadoc updated to describe the opt-in behavior.
| * task.submit_result} still matches live {@code task_submit_result}). | ||
| */ | ||
| private static List<PermissionRule> rulesFor( | ||
| Map<String, List<PermissionRule>> table, String toolName) { |
There was a problem hiding this comment.
[Warning] rulesFor was an O(1) map hit; the scan runs whenever the exact-key lookup did not already return the whole rule set — which includes the common case of a tool that simply has no rules. Each call is now O(alias keys x rules) with two replace() allocations per entry, and checkAllowRules / checkDenyRules / checkAskRules (lines 266/279/292) all go through it, so every single tool invocation in the ReAct loop pays it three times against a rule table that grows with every learned suggestion.
Cheap fixes: skip the loop when the table has no dotted keys (tables.keySet().stream().noneMatch(k -> k.contains(".")) precomputed at construction), or cache the expanded alias list per toolName in the engine.
There was a problem hiding this comment.
Fixed in 2ab8a85 (same change as the alias narrowing).
With opt-in aliases there is no table scan: tools with empty nameAliases() take a single Map.get (O(1)). Aliased tools do O(|aliases|) keyed lookups — no replace() / full entry walk on every checkAllow/Deny/Ask.
| public static final String MODEL_NAME = "task_submit_result"; | ||
|
|
||
| /** Legacy dotted spelling previously used as {@code @Tool(name=...)} / permission keys. */ | ||
| public static final String LEGACY_DOTTED_NAME = "task.submit_result"; |
There was a problem hiding this comment.
[Info] LEGACY_DOTTED_NAME is declared (and advertised in the class javadoc as the reason persisted keys keep matching), but nothing references it — the PermissionEngine lookup is purely string-shape-based, so the constant cannot actually keep the two spellings in sync and will drift silently.
Either feed it into the lookup (the per-tool alias hook suggested in the PermissionEngine comment), assert on it in PermissionEngineTest, or drop it and let the javadoc refer to AgentTaskCollaborationTool.toModelName(...) as the single source of truth.
There was a problem hiding this comment.
Fixed in 2ab8a85.
AgentTaskOutcomeTool.nameAliases() returns List.of(LEGACY_DOTTED_NAME), so the constant is the source of truth for permission dual-lookup. Asserted in outcomeToolDeclaresLegacyDottedNameAsPermissionAlias.
| return null; | ||
| } | ||
| if (value instanceof List<?> list) { | ||
| return list.stream().map(v -> v == null ? null : String.valueOf(v)).toList(); |
There was a problem hiding this comment.
[Info] The v == null branch here makes the method throw instead of degrading: Stream.toList() rejects null elements, so a model emitting "pending_task_ids": [null] produces an NPE out of callAsync rather than an IllegalArgumentException/ToolResultBlock.error like every other malformed input on this path.
return list.stream().filter(Objects::nonNull).map(String::valueOf).toList();(List.of(String.valueOf(value)) at line 132 is fine — that overload prints the literal "null".)
There was a problem hiding this comment.
Fixed in 2ab8a85.
stringListArg now does list.stream().filter(Objects::nonNull).map(String::valueOf).toList(), so [null] no longer NPEs out of Stream.toList().
| + " call only when every child Issue and worker node has" | ||
| + " converged. Never send those mutations in parallel. " | ||
| + runNodeComplete | ||
| + " also" |
There was a problem hiding this comment.
[Info] The token now reaches the model as "... also" + " completes ...", i.e. the string was split at every interpolation rather than at the existing sentence boundaries. Output is correct, but this block is going to be painful to reword — worth one pass to reflow the concatenation points (or switch to a text block plus formatted(...)) before this grows another instruction.
There was a problem hiding this comment.
Fixed in 2ab8a85.
Reflowed roleInstructions at sentence boundaries and switched the team-role branches to .formatted(...) so interpolations sit at readable phrase breaks (no more " also" + " completes" mid-token splits).
Replace unconditional bidirectional dotted/underscore alias merge in PermissionEngine with ToolBase.nameAliases(); only AgentTaskOutcomeTool declares LEGACY_DOTTED_NAME. Restores O(1) lookup for tools without aliases, filters null pending_task_ids, and reflows roleInstructions.
|
@oss-maintainer polished the five findings from #3205 (review) in PermissionEngine: opt-in aliases (blocking)Unconditional bidirectional Other findings
Tests
mvn -pl agentscope-core,agentscope-extensions/agentscope-extensions-aistio -am spotless:apply
mvn -pl agentscope-core,agentscope-extensions/agentscope-extensions-aistio spotless:check
mvn -pl agentscope-core,agentscope-extensions/agentscope-extensions-aistio -am testThread replies: alias merge · O(1) · LEGACY wiring · null filter · roleInstructions |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review of 2ab8a85f (follow-up to the findings raised on 095675ea). The coupling heuristic in PermissionEngine.rulesFor — which previously auto-matched any dotted/underscore string reshaping — has been replaced with an explicit opt-in ToolBase.nameAliases()" hook: rules now merge only names a tool itself declares (AgentTaskOutcomeTool→ legacytask.submit_result), the no-alias path stays a single O(1) map hit, and the new negative tests (a.bvsa_b` twins not cross-matching in either direction) pin the security property exactly. This fully resolves the dual-spelling concern; the aistio-side rename and the rest of the PR are unchanged from the earlier review. Approving — merge state CLEAN, thanks for the quick turnaround.
Automated review by github-manager-bot
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review response to @tengjiaozhai's two re-review requests (2026-09-22). I re-read 2ab8a85 against the five findings from the 2026-09-21 review, and all five are genuinely addressed — no new issues found in the changed surface, so this is an approval.
Verified in the diff:
- Permission aliases are opt-in —
ToolBase.nameAliases()defaults to an empty list, andPermissionEngine.rulesFor(...)short-circuits to the exact-key lookup when there are no aliases, so the previous unconditional'.'↔'_'cross-matching (and its per-lookup list building) is gone. LEGACY_DOTTED_NAMEis declared only byAgentTaskOutcomeTool, which is the single tool that actually needs persisted-key compatibility.- Total model-name sanitizer —
^[a-zA-Z0-9_-]{1,64}$with a deterministic 8-hex suffix whenever characters were dropped or the length budget was exceeded, so two distinct wire names cannot silently collide onto the same model name. pending_task_idsnull-filtering and theroleInstructionsreflow are in place.- Tests cover outcome shadowing,
mcp:server.toolnormalization and terminal collision.
Note for maintainers
The PermissionEngine change is in agentscope-core, so it affects every tool name lookup, not just the aistio adapter; the empty-alias fast path keeps that cost at one Map.get, which is the right shape. CLA (license/cla) is green and the branch is mergeable.
Automated review by github-manager-bot
This branch has not been deployed
2 participants
AgentScope-Java Version
2.0.4-SNAPSHOT (branch revision; no invented bump in this PR)
Description
OpenAI-compatible APIs require
function.name =~ ^[a-zA-Z0-9_-]{1,64}$. 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.failcheckstoModelName()(total OpenAI-safe sanitizer:'.'→'_'fast path; other illegal chars + length>64 get deterministic hash suffix): 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). Outcome tool registration identity-checksAgentTaskOutcomeTooland replaces foreign occupants. Dropped collisions are listed in the kickoff prompt; terminal collisions refuse dispatch.roleInstructions(and kickoff / correction prompts) use model-facing names derived from the sametoModelName()mapping so prose cannot drift from registration.AgentTaskOutcomeTool:task.submit_result→task_submit_result(same mapping); now aToolBasefor identity + permission participation.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.Permission rules:
PermissionEngineaccepts both dotted and underscore spellings during rule lookup when one is the'.'→'_'form of the other (e.g. persistedtask.submit_result/issue.comment.addstill match livetask_submit_result/issue_comment_add). No manual migration required for exact-key allow/deny/ask tables stored under the legacy dotted names.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: 81 tests, 0 failures (dual-name + terminal-wire coverage, outcome-tool identity replace, collision keep-first / terminal refuse,mcp:server.toolsanitizer contract)-am test(includesPermissionEngineTest$DualSpellingLookup)Checklist
mvn spotless:applymvn test): aistio module 81/0