Skip to content

fix(harness): activate skill-bound tool groups on skill load #2662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
StevenZhang2002 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(harness): activate skill-bound tool groups on skill load #2662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7531768
fix(harness): activate skill-bound tool groups on skill load
StevenZhang2002 Aug 11, 2026
e21f4f4
fix(harness): correct license header in skill load tool test
StevenZhang2002 Aug 11, 2026
ad5de2e
test(harness): cover skill load tool toolkit fallback paths
StevenZhang2002 Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 82 additions & 4 deletions ...tscope-harness/src/main/java/io/agentscope/harness/agent/skill/runtime/SkillLoadTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import io.agentscope.core.skill.util.MarkdownSkillParser;
import io.agentscope.core.tool.AgentTool;
import io.agentscope.core.tool.ToolCallParam;
import io.agentscope.core.tool.ToolGroup;
import io.agentscope.core.tool.Toolkit;
import io.agentscope.harness.agent.skill.SkillResources;
import java.util.ArrayList;
import java.util.LinkedHashSet;
Expand Down Expand Up @@ -50,6 +52,11 @@
* ToolCallParam#getRuntimeContext() tool call's RuntimeContext}, keeping concurrent calls and
* sessions isolated without re-registering the tool. A legacy catalog reference is consulted only
* for deprecated callers that invoke the tool without any RuntimeContext.
*
* <p>On-demand tool disclosure: once a resource is found, every {@link
* io.agentscope.core.tool.SkillToolGroup} bound to the skill via {@code activateOnSkill} is
* activated on the call's toolkit, so the skill's tools become visible to the model for the
* following turns (see issue #2653).
*/
@SuppressWarnings("deprecation")
public final class SkillLoadTool implements AgentTool {
Expand All @@ -61,10 +68,11 @@ public final class SkillLoadTool implements AgentTool {

private final AtomicReference<SkillCatalog> legacyCatalogRef;
private final boolean exposeLegacyCatalogInSchema;
private final AtomicReference<Toolkit> toolkitRef;

/** Creates a context-scoped loader with an empty context-free compatibility catalog. */
public SkillLoadTool() {
this(new AtomicReference<>(SkillCatalog.empty()), false);
this(new AtomicReference<>(SkillCatalog.empty()), false, new AtomicReference<>());
}

/**
Expand All @@ -80,16 +88,24 @@ public SkillLoadTool() {
*/
@Deprecated(since = "2.2.0")
public SkillLoadTool(AtomicReference<SkillCatalog> catalogRef) {
this(catalogRef, true);
this(catalogRef, true, new AtomicReference<>());
}

SkillLoadTool(
AtomicReference<SkillCatalog> legacyCatalogRef, boolean exposeLegacyCatalogInSchema) {
this(legacyCatalogRef, exposeLegacyCatalogInSchema, new AtomicReference<>());
}

SkillLoadTool(
AtomicReference<SkillCatalog> legacyCatalogRef,
boolean exposeLegacyCatalogInSchema,
AtomicReference<Toolkit> toolkitRef) {
if (legacyCatalogRef == null) {
throw new IllegalArgumentException("catalogRef must not be null");
}
this.legacyCatalogRef = legacyCatalogRef;
this.exposeLegacyCatalogInSchema = exposeLegacyCatalogInSchema;
this.toolkitRef = toolkitRef != null ? toolkitRef : new AtomicReference<>();
}

@Override
Expand Down Expand Up @@ -177,7 +193,8 @@ public Mono<ToolResultBlock> callAsync(ToolCallParam param) {
+ "'. Check the available skills list."));
}

return Mono.just(loadOne(entry, path));
Toolkit toolkit = toolkitFor(param);
return Mono.just(loadOne(entry, path, toolkit));
} catch (Exception e) {
log.error("load_skill_through_path failed", e);
return Mono.just(ToolResultBlock.error(e.getMessage()));
Expand All @@ -192,24 +209,43 @@ private SkillCatalog catalogFor(ToolCallParam param) {
return legacy != null ? legacy : SkillCatalog.empty();
}

private ToolResultBlock loadOne(HarnessSkillEntry entry, String path) {
/**
* Resolves the toolkit used to activate skill-bound tool groups. Prefers the call-scoped
* instance (installed by {@link SkillRuntime#install(SkillCatalog,
* io.agentscope.core.agent.RuntimeContext, Toolkit)}), falling back to the shared reference
* kept current by {@link SkillRuntime#prepareToolkit(Toolkit)} for context-free callers.
*/
private Toolkit toolkitFor(ToolCallParam param) {
if (param.getRuntimeContext() != null) {
Toolkit toolkit = param.getRuntimeContext().get(Toolkit.class);

@oss-maintainer oss-maintainer Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toolkitFor falls back to the shared toolkitRef whenever the call context has no Toolkit — which includes the case where install was called with a null toolkit (agent == null in HarnessSkillMiddleware.onSystemPrompt, line 266 there). So a context-scoped call can silently activate groups on the last toolkit prepareToolkit saw (constructor-time toolkit, i.e. the pre-copy() instance at ReActAgent.Builder:5535) rather than the agent's live one. Since the deep copy keeps the tool instances but not the group state, that fallback can activate a group that this agent's schema rendering never reads. Worth an explicit "no reachable toolkit -> log at debug and skip" branch instead of mutating whatever reference is cached.

if (toolkit != null) {
return toolkit;
}
}
return toolkitRef.get();
}

private ToolResultBlock loadOne(HarnessSkillEntry entry, String path, Toolkit toolkit) {
AgentSkill skill = entry.skill();

// 1. Special case: SKILL.md returns the parsed body text.
if (SKILL_FILE.equals(path)) {
activateSkillTools(toolkit, skill);
return ToolResultBlock.text(formatSkillMarkdown(entry));
}

// 2. In-memory map (Layer 1/3 host repos + marketplace fully-loaded resources).
Map<String, String> mem = skill.getResources();
if (mem != null && mem.containsKey(path)) {
activateSkillTools(toolkit, skill);
return ToolResultBlock.text(formatResource(entry, path, mem.get(path)));
}

// 3. Lazy fallback (WorkspaceSkillRepository and other LazyResourceCapable sources).
if (entry.lazyResources() != null) {
Optional<String> lazy = entry.lazyResources().read(path);
if (lazy.isPresent()) {
activateSkillTools(toolkit, skill);
return ToolResultBlock.text(formatResource(entry, path, lazy.get()));
}
}
Expand All @@ -218,6 +254,48 @@ private ToolResultBlock loadOne(HarnessSkillEntry entry, String path) {
return ToolResultBlock.error(formatNotFound(entry, path));
}

/**
* Activates every {@link io.agentscope.core.tool.SkillToolGroup} bound to the loaded skill via
* {@code activateOnSkill} so its tools become visible to the model (on-demand tool disclosure).

@oss-maintainer oss-maintainer Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SkillLoadTool is registered as a single shared instance (SkillRuntime.loadTool is final, built once in the constructor, and returned by loadTool()), while the thing this new code hands it — the live toolkit — is per agent build. So the shared-reference fallback is not just for "deprecated callers without a RuntimeContext": after a second HarnessAgent is built in the same JVM/process from a skill repository that reuses the middleware (or whenever install ran with a null agent), toolkitRef.get() points at whichever agent most recently passed through prepareToolkit, and activation mutates that agent's groups.

Concretely: prepareToolkit is called from both HarnessSkillMiddleware construction (line 213) and every onSystemPrompt, and context.put(Toolkit.class, toolkit) is unconditional — so a call whose context has no toolkit (legacy call(msgs) path, AgentBase:214 skips the write when context == null) silently activates groups on the last-prepared agent. Suggest either making the absence explicit (Optional<Toolkit> semantics: no context toolkit and legacy constructor ⇒ log at debug and skip) or binding the toolkit per call rather than per runtime.

*
* <p>Matches the activation semantics of the core skill access tool ({@code
* SkillToolFactory.activateSkill}): a group is only activated after a resource was actually
* found, and only when it is not already active. The toolkit's shared activation state is
* picked up by {@code ReActAgent}'s post-acting {@code syncToolkitToState}, which persists it
* into the session's {@code ToolContextState} for subsequent reasoning turns.
*/
private void activateSkillTools(Toolkit toolkit, AgentSkill skill) {

@oss-maintainer oss-maintainer Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, for discoverability: matching is findSkillToolGroupsByActivateOnSkill(skill.getName()) — i.e. exact String.equals against the frontmatter name — while the parameter the model passes to this tool is the skillId (<name>_<source>). @duola8789 hit exactly this in the thread above. A one-line javadoc note here ("bound by skill name, not skill id") plus the same note in the activateOnSkill builder javadoc would save the next person a debugging session.

if (toolkit == null || skill == null) {
return;
}
try {
List<String> boundGroups =
toolkit.findSkillToolGroupsByActivateOnSkill(skill.getName());

@oss-maintainer oss-maintainer Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Activation is per-resource-load and mutates the toolkit, so on a session whose persisted ToolContextState.activatedGroups was previously empty the first load of a skill permanently flips the group on for the rest of that session's life — and, given point 1, potentially for the shared toolkit. That matches core's SkillToolFactory.activateSkill semantics, so this is a question rather than a defect: is there a deactivation story for the harness path? Core at least has skillRegistry.setSkillActive(skillId, false) alongside it; here there is no equivalent, so a model that loads a skill and then moves on keeps the tools disclosed. If that's intended ("once disclosed, stays disclosed"), say so in the class javadoc, because the on-demand-disclosure feature is usually sold as the opposite.

if (boundGroups.isEmpty()) {
return;
}
List<String> toActivate = new ArrayList<>(boundGroups.size());
for (String group : boundGroups) {
ToolGroup existing = toolkit.getToolGroup(group);
if (existing != null && !existing.isActive()) {
toActivate.add(group);
}
}
if (!toActivate.isEmpty()) {
toolkit.updateToolGroups(toActivate, true);

@oss-maintainer oss-maintainer Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Activation is written to the shared toolkit (toolkit.updateToolGroups(...)), and reaches session state only indirectly via ReActAgent.syncToolkitToState, i.e. state.getToolContext().setActivatedGroups(toolkit.getActiveGroups()). Two consequences worth checking:

  1. Persistence is conditional on the acting path taken. syncToolkitToState(state) is called after the acting loop completes (ReActAgent.java ~2935) — but the branches just above it return early for isStopRequested(), returnDirect and non-empty pendingPairs (suspension / awaiting permission approval). If load_skill_through_path is followed by a stop, a direct return, or a suspended tool call, the group is activated in memory but never copied into the persisted ToolContextState, so the next call — or a session recovered from an AgentStateStore, which is the whole point of agentscope-distribution — starts with the toolkit active and the state not, and getActiveGroups()/state reconciliation can flip it back off. Would it be more robust to activate through the call's own state (state.getToolContext().addActivatedGroup(...)) and let the toolkit mirror follow, the way reset_equipped_tools does?
  2. Shared instance, multiple sessions. A Toolkit is per agent instance, while activatedGroups is per session. When one HarnessAgent instance serves several sessions, a skill loaded in session A activates the group for everyone whose state is synced from that toolkit afterwards. The SkillCatalog half of this PR is correctly call-scoped; the toolkit half is not.

log.info(
"Activated skill-bound tool groups {} for skill '{}'",
toActivate,
skill.getName());
}
} catch (Exception e) {
log.warn(

@oss-maintainer oss-maintainer Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log.info for the happy path fires on every successful resource load, not only on the first one for a group (the isActive() filter at line 281 covers repeat loads, so in practice it is once per group — fine). The catch (Exception e) only logs e.getMessage(), which drops the stack for the most likely failure (updateToolGroups throwing IllegalArgumentException on a vanished group). Consider log.warn("...", e) or at least including the exception class.

"Failed to activate skill-bound tool groups for skill '{}': {}",
skill.getName(),
e.getMessage());
}
}

// ---------------------------------------------------------------------
// Formatting helpers
// ---------------------------------------------------------------------
Expand Down
15 changes: 12 additions & 3 deletions agentscope-harness/src/main/java/io/agentscope/harness/agent/skill/runtime/SkillRuntime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public final class SkillRuntime {
private final RuntimeContext legacyContext = RuntimeContext.empty();
private final AtomicReference<SkillCatalog> legacyCatalogRef =
new AtomicReference<>(SkillCatalog.empty());
private final AtomicReference<Toolkit> toolkitRef = new AtomicReference<>();
private final SkillLoadTool loadTool;
private final SkillPromptBuilder promptBuilder;

Expand All @@ -49,7 +50,7 @@ public SkillRuntime() {

public SkillRuntime(SkillPromptBuilder promptBuilder) {
this.promptBuilder = promptBuilder != null ? promptBuilder : new SkillPromptBuilder();
this.loadTool = new SkillLoadTool(legacyCatalogRef, false);
this.loadTool = new SkillLoadTool(legacyCatalogRef, false, toolkitRef);
}

/**
Expand Down Expand Up @@ -84,17 +85,21 @@ public AgentTool loadTool() {
}

/**
* Ensures the skill loading tool is present as an ungrouped tool.
* Ensures the skill loading tool is present as an ungrouped tool and remembers the toolkit
* used to activate skill-bound tool groups on load.
*
* <p>Harness calls this while the agent toolkit is still being assembled. The toolkit copy
* made by {@code ReActAgent.Builder} therefore contains the tool before its first model call.
* Keeping the loader ungrouped is intentional: ungrouped tools remain visible regardless of a
* session's persisted active-group list, including sessions created before the loader existed.
* Each system-prompt pass re-binds the reference so it always points at the agent's live
* toolkit (the one that was deep-copied during agent construction).
*/
public void prepareToolkit(Toolkit toolkit) {
if (toolkit == null) {
return;
}
this.toolkitRef.set(toolkit);
try {
AgentTool existing = toolkit.getTool(SkillLoadTool.TOOL_NAME);
if (existing == loadTool) {
Expand All @@ -115,7 +120,10 @@ public void prepareToolkit(Toolkit toolkit) {
*
* <p>The catalog is deliberately stored on {@code context}, not on this shared runtime. This is
* the authorization boundary used by {@link SkillLoadTool}; a skill omitted from the call's
* filtered catalog cannot be loaded even while another session exposes it.
* filtered catalog cannot be loaded even while another session exposes it. The live toolkit is
* bound to the same context so that loading a skill can activate the {@link
* io.agentscope.core.tool.SkillToolGroup SkillToolGroups} bound to it via {@code
* activateOnSkill} (on-demand tool disclosure).
*
* @param catalog the call snapshot; pass {@link SkillCatalog#empty()} to clear visibility
* @param context the current call context; may be {@code null}, in which case no catalog is
Expand All @@ -125,6 +133,7 @@ public void prepareToolkit(Toolkit toolkit) {
public void install(SkillCatalog catalog, RuntimeContext context, Toolkit toolkit) {
if (context != null) {
context.put(SkillCatalog.class, catalog != null ? catalog : SkillCatalog.empty());
context.put(Toolkit.class, toolkit);

@oss-maintainer oss-maintainer Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

install now publishes the agent's live Toolkit into the call-scoped RuntimeContext under Toolkit.class. Before this change the context only carried SkillCatalog, which the class javadoc explicitly describes as "the authorization boundary used by SkillLoadTool". With a raw Toolkit in the context, any tool or middleware that receives the same RuntimeContext can now mutate registration state (register/remove tools, activate/deactivate groups), not just read the catalog.

Could this be narrowed to the one capability the load tool needs, e.g. put a small functional handle instead of the whole toolkit:

public interface SkillGroupActivator { void activate(String skillName); }
context.put(SkillGroupActivator.class, (SkillGroupActivator) name -> activateBoundGroups(name, toolkit));

If keeping Toolkit.class is intentional, it is worth stating the trust assumption in the javadoc ("the toolkit in context is writable and must only be installed by harness-owned middleware"), because sub-agents and extensions also share RuntimeContext.

@oss-maintainer oss-maintainer Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prepareToolkit(toolkit) unconditionally overwrites toolkitRef on every system-prompt pass, but the deep-copied toolkit from ReActAgent.Builder (line 5535) is what the model schemas are read from, and the middleware holds its own reference (agentToolkit = agent.getToolkit()). The two only agree because activation happens after the copy — the javadoc you added here says "always points at the agent's live toolkit", which is true only in the steady state and makes the constructor-time prepareToolkit call at HarnessSkillMiddleware:213 misleading (it pre-binds a toolkit that will be replaced). Worth stating that the reference is best-effort and the context binding is authoritative, so a reader doesn't rely on the constructor bind.

}
prepareToolkit(toolkit);
}
Expand Down
Loading
Loading