test(agent): regression coverage for #2169 skill load tool on fresh sessions

[gxgeek-n]

Summary

Adds end-to-end regression tests pinning the fix for #2169 (Unauthorized tool call: 'load_skill_through_path' is not available on fresh sessions).

The bug: on the first call of a fresh session, activateSlotForContext applied the state's (empty) activated group list via setActiveGroups, deactivating the build-time active skill-build-in-tools group. SkillHook kept advertising skills in the system prompt, so the model inevitably called load_skill_through_path and got the Unauthorized error. This was fixed on main by #2319 (fresh slots inherit build-time active groups), but the end-to-end path through a real SkillBox + ReActAgent call had no coverage — existing tests only assert state seeding with manually-created groups, and SkillBoxTest/SkillBoxToolsTest never drive a full agent call.

What the tests do

SkillLoadToolFreshSessionTest drives a scripted model through a real ReAct loop with a registered skill, asserting on the tool result exactly as the model observes it:

1. freshSession_skillLoadToolExecutes — the load tool executes and returns the skill content (not Unauthorized)
2. freshSession_stateSeedsSkillGroup — ToolContextState keeps skill-build-in-tools activated after the call
3. secondCall_skillLoadToolStillAuthorized — activation survives repeated calls in one session (the wipe happened per call, not just on the first)

Verified both directions

• v2.0.0 tag: all 3 tests fail with the exact Error: Unauthorized tool call: 'load_skill_through_path' is not available from the issue report
• main (with fix: preserve skill isolation and tool result history #2319): all 3 tests pass

# v2.0.0
Tests run: 3, Failures: 3 — Unauthorized tool call: 'load_skill_through_path' is not available
# main
Tests run: 3, Failures: 0, Errors: 0 — BUILD SUCCESS

Notes

• Test-only change, no production code touched
• Related: fix(tool): use call-scoped group activation #2333 (call-scoped group authorization) is complementary — it addresses cross-session state isolation, while these tests pin the fresh-session build-defaults path
• We hit this in production on an unattended (no-HITL) agent deployment on v2.0.0 and currently carry an application-side hook that re-activates the group on PreCallEvent; happy to drop it once a release ships with fix: preserve skill isolation and tool result history #2319

[CLAassistant]

All committers have signed the CLA.

[Copilot]

Pull request overview

Adds end-to-end regression coverage for #2169 by exercising a full ReAct loop with a real SkillBox + ReActAgent, asserting that load_skill_through_path remains authorized on fresh sessions and across repeated calls.

Changes:

• Introduces SkillLoadToolFreshSessionTest with scripted-model e2e tests to ensure the skill load tool executes (not Unauthorized) on the first call of a fresh session.
• Verifies ToolContextState retains the build-time active skill-build-in-tools group after the call.
• Verifies authorization persists across a second call within the same session.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[oss-maintainer]

LGTM. Looks good.

---

Automated review by "github-manager-bot"

[gxgeek-n]

Friendly ping — this one is test-only and looks ready to merge:

• All 6 status checks are green, and codecov/patch reports all modified and coverable lines are covered.
• oss-maintainer has already left an APPROVED review.
• Note on the CLA comment above: the "CLA Not Signed" message from oss-maintainer is stale. The actual commit status is license/cla: success — Contributor License Agreement is signed. — the CLA is not blocking.

Scope reminder: this PR adds no production code, only end-to-end regression coverage for #2169 — it drives a real SkillBox + ReActAgent through a full ReAct loop and asserts load_skill_through_path stays authorized on fresh sessions and across repeated calls.

Why it's worth landing alongside the fix: #2169 was a deadlock where the unlock tool itself got gated on a fresh session — the fix in a7008d05 (initialActiveToolGroups) is easy to regress on any future change to tool-group activation, and unit tests alone don't catch it because the failure only shows up through the full loop.

I checked the 15 commits merged into main since this was opened — none touch skill-related files, so no conflict. Currently 15 commits behind main; happy to rebase if that helps.

[gxgeek-n]

Thanks @copilot — I checked this against the source, and the _custom suffix is correct here. Detail below in case it is useful for others reading the test.

AgentSkill.getSkillId() composes the id from name and source:

// AgentSkill.java:267
public String getSkillId() {
    return getName() + "_" + source;
}

and the 4-arg constructor the test uses defaults source to "custom":

// AgentSkill.java:94
public AgentSkill(String name, String description, String skillContent, Map<String, String> resources) {
    this(name, description, skillContent, resources, "custom");   // ← source
}

So new AgentSkill("demo_skill", ...) registers under skill id demo_skill_custom, and the skillId enum in SkillToolFactory is populated from getSkillId() — not the raw name. The scripted call therefore targets a skill that does exist.

Worth noting the suffix is not a hard-coded "_custom" string anywhere (grepping for it across agentscope-core and agentscope-harness returns nothing) — it is assembled from the default source, which is easy to miss.

The test also asserts positively rather than just "no error":

assertTrue(model.observedToolResult.contains(SKILL_BODY), ...)

If the load had returned "Skill not found", observedToolResult would carry the error text and not SKILL_BODY, so the assertion would fail. The suite is green (3/3), which confirms the intended authorization path is exercised.

That said, your comment points at a real readability problem: the test comment says the suffix comes from RegisteredSkill, which is the wrong reference. I will correct it to point at AgentSkill.getSkillId() / the default source so the next reader does not have to derive it. Let me know if you would rather the test construct the id via skill.getSkillId() instead of writing the literal — that would make it self-documenting and immune to a future change in the composition rule.

[gxgeek-n]

Pushed the readability fix I mentioned — went with the self-documenting option:

private static final AgentSkill DEMO_SKILL =
        new AgentSkill(SKILL_ID, "Demo Skill", SKILL_BODY, new HashMap<>());

/** AgentSkill.getSkillId() composes name + "_" + source; the 4-arg ctor defaults source to "custom". */
private static final String REGISTERED_SKILL_ID = DEMO_SKILL.getSkillId();

The literal "_custom" is gone, the wrong RegisteredSkill reference is corrected, and the same instance is now used for both registration and the scripted call — so if the id composition rule ever changes, the test follows it instead of silently targeting a stale id.

3/3 still green.