fix(skills): run orphan GC on empty merged skills to clear stale cache by weixsun · Pull Request #2665 · agentscope-ai/agentscope-java

weixsun · 2026-08-11T10:23:21Z

Description

Closing #2664

Checklist

Please check the following items before code is ready to be reviewed.

Code has been formatted with mvn spotless:apply
All tests are passing (mvn test)
Javadoc comments are complete and follow project conventions
Related documentation has been updated (e.g. links, examples, etc.)
Code is ready for review

CLAassistant · 2026-08-11T10:28:57Z

All committers have signed the CLA.

codecov · 2026-08-11T10:45:46Z

Codecov Report

❌ Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...rness/agent/middleware/HarnessSkillMiddleware.java	84.21%	2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

AgentScopeJavaBot

🤖 AI Review

This PR fixes a real and impactful bug: when all skills are removed from repositories or filtered out by visibility/skill filters, stale content in .skills-cache/ was never cleaned up, causing outdated skill resources to leak into the sandbox. The fix introduces a MergeResult record that carries both the merged skills map and a loadFailed flag, enabling a new stageEmptyRetainedIfSafe() helper that triggers orphan GC with an empty retained set — but only when the empty result is intentional (not caused by a transient repository failure). The outage-protection design is thoughtful, the code reuse of stager.stage(List.of(), sourceNamespaces) is clean (no API surface expansion needed), and the 5-test suite covers the key scenarios well including the critical load-failure preservation case.

AgentScopeJavaBot · 2026-08-12T01:42:06Z

-    private Map<String, RepoBound> skillsForCall(RuntimeContext ctx) {
-        return frozenSkills != null ? frozenSkills : mergeRepositories(ctx);
+    private MergeResult skillsForCall(RuntimeContext ctx) {
+        return frozenSkills != null ? new MergeResult(frozenSkills, false) : mergeRepositories(ctx);


[nit] When frozenSkills != null, loadFailed is hardcoded to false. This means if all repositories failed during construction (with freezeRepositories=true), the loadFailed signal is silently discarded and a subsequent empty-merge will trigger cache GC. In practice this is harmless (empty frozen skills + empty cache = no-op GC), but a brief inline comment explaining why frozen mode always reports loadFailed=false would help future maintainers understand the intent.

AgentScopeJavaBot · 2026-08-12T01:42:06Z

+    }
+
+    @Test
+    void onSystemPromptClearsSkillsCacheWhenVisibilityFilterHidesAll() throws IOException {


[nit] There's a test for onSystemPrompt + visibility-filter-hides-all (this test), but no symmetric test for prestageMarketplaceSkills + visibility-filter-hides-all. The skill-filter variant is covered (prestageMarketplaceSkillsClearsCacheWhenSkillFilterHidesAll), but visibility filter takes a different code path (applyVisibility returns empty before applySkillFilter is even reached). Consider adding a prestageMarketplaceSkillsClearsCacheWhenVisibilityFilterHidesAll test for symmetry.

AgentScopeJavaBot · 2026-08-12T01:42:06Z

+     * is removed when no skill survives merging/filtering. Skipped when a repository failed to
+     * load — an empty result caused by an error must not wipe the cache (outage protection).
+     */
+    private void stageEmptyRetainedIfSafe(boolean loadFailed) {


[praise] Clean design: reusing stager.stage(List.of(), sourceNamespaces) to trigger orphan GC through the existing public API avoids expanding MarketplaceStager's API surface (no need to widen garbageCollectOrphans visibility). The loadFailed guard is a well-thought-out outage protection — an empty result caused by a transient error must never wipe the cache.

AgentScopeJavaBot

🤖 AI Review

This PR fixes a real and impactful bug: when all skills are removed from repositories or filtered out by visibility/skill filters, stale content in .skills-cache/ was never cleaned up, causing outdated skill resources to leak into the sandbox. The fix introduces a MergeResult record that carries both the merged skills map and a loadFailed flag, enabling a new stageEmptyRetainedIfSafe() helper that triggers orphan GC with an empty retained set — but only when the empty result is intentional (not caused by a transient repository failure). The outage-protection design is thoughtful, the code reuse of stager.stage(List.of(), sourceNamespaces) is clean (no API surface expansion needed), and the 5-test suite covers the key scenarios well including the critical load-failure preservation case.

AgentScopeJavaBot · 2026-08-12T01:43:05Z

-    private Map<String, RepoBound> skillsForCall(RuntimeContext ctx) {
-        return frozenSkills != null ? frozenSkills : mergeRepositories(ctx);
+    private MergeResult skillsForCall(RuntimeContext ctx) {
+        return frozenSkills != null ? new MergeResult(frozenSkills, false) : mergeRepositories(ctx);


[nit] When frozenSkills != null, loadFailed is hardcoded to false. This means if all repositories failed during construction (with freezeRepositories=true), the loadFailed signal is silently discarded and a subsequent empty-merge will trigger cache GC. In practice this is harmless (empty frozen skills + empty cache = no-op GC), but a brief inline comment explaining why frozen mode always reports loadFailed=false would help future maintainers understand the intent.

AgentScopeJavaBot · 2026-08-12T01:43:05Z

+    }
+
+    @Test
+    void onSystemPromptClearsSkillsCacheWhenVisibilityFilterHidesAll() throws IOException {


[nit] There's a test for onSystemPrompt + visibility-filter-hides-all (this test), but no symmetric test for prestageMarketplaceSkills + visibility-filter-hides-all. The skill-filter variant is covered (prestageMarketplaceSkillsClearsCacheWhenSkillFilterHidesAll), but visibility filter takes a different code path (applyVisibility returns empty before applySkillFilter is even reached). Consider adding a prestageMarketplaceSkillsClearsCacheWhenVisibilityFilterHidesAll test for symmetry.

AgentScopeJavaBot · 2026-08-12T01:43:05Z

+     * is removed when no skill survives merging/filtering. Skipped when a repository failed to
+     * load — an empty result caused by an error must not wipe the cache (outage protection).
+     */
+    private void stageEmptyRetainedIfSafe(boolean loadFailed) {


[praise] Clean design: reusing stager.stage(List.of(), sourceNamespaces) to trigger orphan GC through the existing public API avoids expanding MarketplaceStager's API surface (no need to widen garbageCollectOrphans visibility). The loadFailed guard is a well-thought-out outage protection — an empty result caused by a transient error must never wipe the cache.

- Change mergeRepositories() to return both merged skills and a load failure flag - Use new MergeResult wrapper for skills and load failure tracking - Trigger orphan GC with empty retained white-list when merging yields no skills - Skip orphan GC if repository load failure occurred to protect cache during outages - Call stageEmptyRetainedIfSafe() in all code paths where merged skills are empty or filtered empty - Add tests verifying orphan GC clears .skills-cache on empty skills and preserves cache on load failure - Ensure safe behavior when MarketplaceStager is null during empty cache staging

weixsun force-pushed the main branch 2 times, most recently from 85a7d19 to e3f69a0 Compare August 11, 2026 10:58

AgentScopeJavaBot added bug Something isn't working area/harness agentscope-harness (test/runtime support) labels Aug 12, 2026

AgentScopeJavaBot approved these changes Aug 12, 2026

View reviewed changes

weixsun force-pushed the main branch from e3f69a0 to 104ee53 Compare August 12, 2026 09:09

dangzitou mentioned this pull request Aug 12, 2026

[Bug] .skills-cache is never cleaned up when skill repositories return an empty collection; HarnessAgent still reports installed skills #2664

Open

Navigation Menu

Uh oh!

fix(skills): run orphan GC on empty merged skills to clear stale cache - #2665

fix(skills): run orphan GC on empty merged skills to clear stale cache#2665
weixsun wants to merge 1 commit into
agentscope-ai:mainfrom
weixsun:main

weixsun commented Aug 11, 2026

Uh oh!

CLAassistant commented Aug 11, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Aug 11, 2026

Uh oh!

AgentScopeJavaBot left a comment

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Uh oh!

AgentScopeJavaBot left a comment

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

weixsun commented Aug 11, 2026

Description

Checklist

Uh oh!

CLAassistant commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Aug 11, 2026

Codecov Report

Uh oh!

AgentScopeJavaBot left a comment

Choose a reason for hiding this comment

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

AgentScopeJavaBot left a comment

Choose a reason for hiding this comment

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

AgentScopeJavaBot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CLAassistant commented Aug 11, 2026 •

edited

Loading