Repository navigation
fix(harness): stage lazy skill resources from origin directories - #3376
xihongshichaojidan8 wants to merge 2 commits into
Conversation
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
Stages lazy-repository skill support files from originDir instead of only reporting an unusable files-root. I ran the new suite against this PR head: MarketplaceStagerLazyTest passes at head and fails at HEAD~1 with Cached, so the regression it claims to fix is genuinely fixed here. Copy safety holds — relativize preserves the tree (:357), readAllBytes/Files.write keep bytes exact, the walk starts at toRealPath() (:319) and never follows links, !attrs.isRegularFile() (:351) treats symlinked files and symlinked directories identically, and validateOriginTarget (:368-379) rejects poisoned symlink ancestry. Failure handling is right too: Cached is set only after materialize returns (:252-253), exceptions fall through to NONE (:254-257), and retained.add precedes the copy (:243) so earlier good copies escape GC.
Posting as COMMENT rather than APPROVE for one reason only: #3374 fixes the same issue #3373 in the same method with the opposite contract (return StageResult.NONE rather than stage from originDir). This looks like the better fix to me — it restores script access instead of hiding the broken path — but two PRs should not both be approved when only one contract is intended.
Non-blocking items (inline)
visitFileFailedis not overridden, so one unreadable entry insideoriginDiraborts the whole staging pass and degrades the skill toNONEinstead of losing a single file.- Staging is not memoised across calls:
prestageMarketplaceSkills(HarnessSkillMiddleware.java:265) andonSystemPrompt(:292) each re-walk the tree, re-read and re-hash every byte twice (:359,:381-389), so cost is O(total tree bytes) per call. Fine for small skill bundles; a skill carrying a large asset directory would inflate every reasoning step. - Test gaps:
emptySkillWithoutOriginKeepsExistingStagingBehavior(:137) asserts onlyCachedwith no content check; a partial-failure case and a directremoveUnexpected(:364) self-heal assertion would be worth adding. The strongest cases arelazyRepositoryStagesScriptsAndBinaryResourcesWithoutLoadingTheMap(:79-84, exact text, binary bytes,SKILL.md/hidden skip rules, empty map) andrestagingRefreshesChangedFilesAndRemovesDeletedFiles(:96-109, mtime preservation and deletion propagation).
Also worth noting
docs/v2/{en,zh}/docs/harness/skill.md each add 2 lines describing the new staging path; the same file still promises every marketplace skill a files-root (:344, :377-380), which is not true when nothing was staged. Minor doc drift, same nit carried on #3374.
Review scope
Static review of the two changed sources plus the new test, and a local test run in this PR-head checkout. No builds of the full multi-module reactor were attempted and no CI was triggered from this review.
Automated review by github-manager-bot
| } | ||
| Files.createDirectories(destination); | ||
| Set<Path> expected = new HashSet<>(); | ||
| Files.walkFileTree( |
There was a problem hiding this comment.
[Warning] The tree walk is correct on safety (default walkFileTree does not follow links, !attrs.isRegularFile() at :351 skips symlinked files and symlinked directories identically, and validateOriginTarget at :368-379 rejects poisoned ancestry), but FileVisitResult.TERMINATE on failure is the wrong granularity: visitFileFailed is not overridden, so a single unreadable entry inside originDir aborts the whole staging pass and the skill degrades to NONE (:254-257) instead of losing just that one file. Consider overriding visitFileFailed to skip-and-continue (with a debug/warn log). Not blocking.
| || Files.isHidden(file)) { | ||
| return FileVisitResult.CONTINUE; | ||
| } | ||
| Path target = destination.resolve(source.relativize(file)).normalize(); |
There was a problem hiding this comment.
[Warning] Cost model: staging from originDir is not memoised across calls. prestageMarketplaceSkills (HarnessSkillMiddleware.java:265) and onSystemPrompt (:292) both run it, and each pass re-walks the tree, re-reads every file and hashes it twice (:359 and :381-389). Total work is O(bytes in the origin tree) per call, so a skill bundling a large asset directory inflates the latency of every reasoning step, not just the first. A cheap improvement is to key off the origin directory mtime (or file count + mtime) and skip the walk when unchanged. Ship-able as is, but worth a follow-up.
| void emptySkillWithoutOriginKeepsExistingStagingBehavior() throws Exception { | ||
| source(); | ||
| AgentSkill skill = new AgentSkill("demo", "demo", "body", null); | ||
| assertInstanceOf(StageResult.Cached.class, stage(repository(true), skill)); |
There was a problem hiding this comment.
[Info] Weakest case in the suite: emptySkillWithoutOriginKeepsExistingStagingBehavior only asserts Cached, no content. Strongest is lazyRepositoryStagesScriptsAndBinaryResourcesWithoutLoadingTheMap (:79-84) — exact text, binary bytes, SKILL.md/hidden skip rules, and an empty resource map. Two gaps worth one more test each: (a) a partial-failure case proving a mid-copy IO error does not leave a directory reported Cached, and (b) the removeUnexpected self-heal path (:364) asserted directly rather than via restagingRefreshesChangedFilesAndRemovesDeletedFiles.
|
感谢 review。目前保留读取失败时返回 NONE 的行为，避免将不完整的技能资源报告为可用。性能优化暂不纳入本次修复。关于与 #3374 的方案重叠，请维护者协调选择，我们愿意配合保持改动范围最小。 |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Follow-up: your three replies are accepted — the #3374 choice belongs to maintainers
Thanks for the detail. Nothing new has been pushed since my review at a49928d9, so there was no code to re-verify; closing the loop on the three non-blocking items instead:
visitFileFailed/ partial-failure degradation — accepted as-is. Failing the whole staging pass toNONErather than publishing a partially populatedfiles-rootis the safer contract, and it is the same signal #3374 emits when nothing stages. Not asking for this again.- Memoisation across
prestageMarketplaceSkills/onSystemPrompt— agreed, out of scope here. Worth a follow-up issue: both call sites re-walk and re-hash the tree, so a skill carrying a large asset directory pays for it on every reasoning step. That cost is a property of the existing stager, not something this PR introduces. - Test gaps (
emptySkillWithoutOriginKeepsExistingStagingBehaviorassertsCachedwith no content check; no partial-failure case; no directremoveUnexpectedself-heal assertion) — still welcome, definitely not a merge condition. The strong cases are already there.
The #3374 overlap is a maintainer decision, so I'm deliberately not flipping either review. Current state of the two:
#3374 (598eff20) |
this PR (a49928d9) |
|
|---|---|---|
| Contract | StageResult.NONE when nothing stages → no files-root advertised |
stage from AgentSkill.getOriginDir() → Cached with a working files-root |
| Diff | 2 files, +234/-5 (1 source, 1 new test) |
4 files, +372/-1 (1 source, 1 new test, 2 docs) |
| Effect on #3373 | makes the failure non-silent; lazy-repo scripts stay unreachable | restores script access, which is the reported symptom |
| Bot review | APPROVED | COMMENT (this thread) |
| Merge state | MERGEABLE / CLEAN — can be merged as-is |
MERGEABLE / BLOCKED — needs an approving review first |
| CI | green, license/cla signed |
green, license/cla signed |
They cannot both land: they rewrite the same two methods (materializeIfChanged, stageAll) with opposite return contracts, so the loser is a rework, not a rebase. My technical read is unchanged from round 1 — this PR fixes the symptom the issue reports, while #3374 only stops advertising a path that does not work, and #3374's own description says so. That said, #3374 is the smaller, already-approved stop-gap and is mergeable right now, so taking it first and keeping this PR as the follow-up is a perfectly reasonable call. Either way, if #3374 lands first this PR will need rebase work against its version of MarketplaceStager.
One nit that survives both outcomes: docs/v2/{en,zh}/docs/harness/skill.md line ~289 already says files-root is rendered "when present", but the sandbox-mode table further down still maps every Marketplace skill to /workspace/.skills-cache/<source>/<name> with no exception for the nothing-staged case. Worth one line of clarification in whichever PR lands.
Automated review by github-manager-bot
This branch has not been deployed
2 participants
AgentScope-Java Version
2.0.4-SNAPSHOT；基于官方 main 的 addf82b。
Description
关联 #3373。lazy FileSystemSkillRepository 的资源映射为空，但提供 originDir。原实现只读取资源映射，缓存目录中没有脚本，却返回 Cached，导致本地 shell 和 sandbox 中的技能脚本不可用。
本 PR 在资源映射为空且 originDir 存在时，从源目录逐文件缓存支持文件，恢复脚本访问，而不仅仅隐藏不可用的 files-root。
改动范围
与现有 PR 的协调
创建前发现 #3374 已提交另一种方案：资源未缓存时返回 NONE，但明确不恢复 originDir 脚本访问。本 PR 提供恢复访问的替代方案，并非否定对方的止损方案。此前已在 #3373 留言说明复现和修复意向；请维护者协调决定采用哪一种方案，或是否适合后续组合处理。
#3314 修改同一缓存类及文档，存在合并重叠；本 PR 不包含其技能名称/来源命名空间策略，也不替代全面路径安全加固。若其先合并，可再协调最小范围的适配。
load_skill_through_path 的 originDir 适配涉及另一条资源读取链，本 PR 不修改该接口，留待维护者单独确定。
验证
本地 macOS / JDK 21：
mvn -T1 -pl agentscope-harness -am verify -q mvn -T1 -pl agentscope-harness -am test -Dtest=MarketplaceStagerLazyTest,MarketplaceStagerExecBitTest,MarketplaceStagerOrphanGcTest -Dsurefire.failIfNoSpecifiedTests=false -q mvn -T1 -pl agentscope-harness -am spotless:check -q均通过。缓存相关 29 个测试无失败、无跳过，其中 lazy staging 15 个，覆盖脚本/二进制、幂等更新和删除、失败回退、eager 资源优先、源/目标链接、真实路径重叠、过期缓存保护及技能移除后的回收。新增别名重叠测试在修复前有 4 个失败，修复后全部通过。
完整模块验证报告：core 2500 个测试，harness 1097 个测试，无失败或错误，合计 26 个跳过。未执行全仓库验证；未本地运行 Windows，符号链接/POSIX 测试在 Windows 按条件跳过，跨平台结果以 CI 为准。
docs 目录的 npm test、npm run validate、npm run broken-links 已通过。
Checklist