Add managed skill archive storage, prewarming, and runtime mounting - #160
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change replaces managed-agent skill prewarming and E2B skill volumes with atomic Filestore archive projections exposed through a read-only ChangesFilestore skills projection
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![duckpr[bot]](https://avatars.githubusercontent.com/in/4166808?s=60&v=4){kind=small}
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
Important
整体方向（用 filestore 虚拟 /skills 只读命名空间取代 E2B 卷预挂载）清晰，设计文档同步完整，删除链路对称无残留。建议合并前确认两点运行期行为：单个损坏归档的爆炸半径，以及 migration 00032 的回滚安全性。
Reviewed changes — 本 PR 将 managed-agent skill 的运行时投递从「E2B 卷预挂载 + prewarm worker」改为「filestore 虚拟 /skills 只读命名空间投影」：解析后的 skill 版本以不可变行写入新表 filestore_skill_archives（migration 00032/00033），由现有 filestore List/Read API 惰性加载、校验、LRU 缓存后投递，并在 E2B/rclone sandbox 内以只读 rclone 挂载到 /root/.claude/skills；同时删除了旧的 skillprewarm 包、SkillMount/PrepareSkillMount 卷挂载链路、BuildMountManifest/MountArchiveFilename、RuntimeSkill.LoadArchive 等约 2300 行代码。
- 新增
filestore_skill_archives存储与投影 — 新表、ReplaceFilestoreSkillArchives事务（按 session 删后插、for update锁 + advisory lock 串行化）、ListFilestoreSkillArchives、validateFilestoreSkillArchiveInput格式校验；/skills纳入filestoreFixedRootPaths。 - filestore 惰性加载与 LRU 缓存 —
skill_archives.go负责 sha256/zip/SKILL.md/zip-slip 校验，skillArchiveCache（默认 64 MiB）缓存已解压归档，rejectSkillMutation守卫所有写端点；ReadFile/ReadMetadata/ListDirectory按/skills前缀分流到 skill 路径。 - rclone 第五个只读挂载 —
rcloneMountPreparationCommand清理 legacy 符号链接后mkdir，/skills以0555/0444、uid=0/gid=0只读挂载到/root/.claude/skills。 - resolver 瘦身与清理路径 —
RuntimeResolver不再加载字节，只返回S3Bucket/S3Key/SHA256/VersionUUID；skill/version 删除与 builtin seed 剪枝不再删除 S3 对象（测试显式断言保留，design doc 明确为 reference-aware GC 的后续工作）。 - migrations 00032/00033 — 建表、为每个存量 filesystem 回填
/skills目录行、清空skill_prewarmjobs、收紧sha256CHECK 为小写 hex。 - 设计文档 —
managed-agent-skills-runtime.md、filestore.md、e2b-sandbox-image-contract.md、skills-builtin-seed-and-storage.md均同步更新，覆盖启动流程、虚拟目录合同、archive 校验、生命周期与对象保留、移除的旧链路。
⚠️ 单个损坏归档会拖垮整个 session 的 /skills 视图
listSkillDirectory 与 resolveSkillNode 遍历投影时，对每个匹配的归档调用 loadSkillArchive，只要其中一个返回 apiErr（checksum 不匹配、非法 zip、缺 SKILL.md、zip-slip 等）就立即中止并返回 HTTP 500，即便其余归档完全健康。设计文档把损坏归档的行为表述为「fail closed」，这是合理的默认姿态；但当前实现的爆炸半径是「整个 /skills 命名空间不可用」而非「该归档自身子树不可用」。配合下一条 finding（内容校验推迟到首次读取），session 内任意一个坏归档会让 ListDirectory("/skills") 这种顶层列举持续返回 500，而不是只影响 /skills/<该归档目录>/...。建议对单个归档的加载失败做隔离——跳过该归档继续处理其余，或仅对该归档子树返回降级结果。
Technical details
# 单个损坏归档会拖垮整个 session 的 `/skills` 视图
## Affected sites
- `internal/filestore/skill_archives.go:131-134` — `listSkillDirectory` 循环内 `loadSkillArchive` 出错即 `return listDirectoryResponse{}, apiErr`，即便是 `ListDirectory("/skills")` 这种需要聚合所有归档的顶层列举也会整体失败。
- `internal/filestore/skill_archives.go:256-258` — `resolveSkillNode` 在首个匹配前缀的归档加载失败时立即返回错误。
## Required outcome
- 单个归档的内容损坏只影响其自身子树（`/skills/<该归档目录>/...`），不影响同级其他归档或 `/skills` 顶层列举；损坏归档的列举返回 notFound 或带降级标记的部分结果，而非整体 HTTP 500。
## Suggested approach
- 在 `listSkillDirectory` 的循环中捕获 `loadSkillArchive` 的错误，记录后将该归档跳过，继续处理其余归档；仅当请求路径正好落在损坏归档子树内时才返回错误。
- `resolveSkillNode` 同理：仅当 `entryPath` 命中损坏归档时才返回 500，否则跳过。⚠️ 归档内容校验推迟到首次读取，故障从启动期漂移到运行期
旧的 prepareRuntimeSkillMount 路径在 session 启动时即对每个归档执行 size + sha256 + zip 目录完整性校验（validateRuntimeSkillArchive），坏归档会让 launch 直接失败。新流程在 replaceRuntimeSkillArchives（runner.go:404）只写入投影行，validateFilestoreSkillArchiveInput 仅做格式校验（hex 长度、source 枚举、目录非空、size>0），真正的字节级校验推迟到 loadSkillArchive 首次读取时才执行。设计文档（managed-agent-skills-runtime.md 第 128、131 行）把这一行为明确为「resolver 不在启动路径下载 archive」「损坏 archive fail closed」，因此惰性校验本身是记录在案的设计决策。需要确认的是其操作面代价：一个 sha256/size/zip 与对象实际不符的投影行（例如对象被覆盖写、catalog 被手工改过）会在 launch 成功、sandbox 已起来之后，于客户端首次读取 /skills 时以 HTTP 500 暴露，并因上一条 finding 的爆炸半径扩散到整个命名空间。若希望保留惰性校验的启动延迟收益、同时避免运行期 500，可在 replaceRuntimeSkillArchives 写入投影后对 builtin 归档做一次 eager 预热校验（catalog 可信度高、数量少），custom 归档保持惰性。
Technical details
# 归档内容校验推迟到首次读取
## Affected sites
- `internal/environments/runner.go:404` — `replaceRuntimeSkillArchives` 仅调用 `ReplaceFilestoreSkillArchives`，后者只做 `validateFilestoreSkillArchiveInput` 格式校验。
- `internal/db/filestore_skill_archives.go:243-264` — 格式校验（hex 长度、source 枚举、size>0），不读取对象字节、不校验 sha256 与实际内容是否一致。
- `internal/filestore/skill_archives.go:269-311` — `loadSkillArchive` 在首次 list/read 时才读取对象、比对 sha256、解析 zip、校验 SKILL.md，任一失败返回 `internalError`（HTTP 500）。
- `docs/design/be/managed-agent-skills-runtime.md:128,131` — 明确记录 resolver 不下载 archive、损坏归档 fail closed；惰性校验是设计意图。
## Required outcome
- 若惰性校验是最终决策，建议在 design doc 中补充说明「运行期 500 是预期故障模式」并配套监控告警；若希望 fail-fast，在 launch 路径对 catalog 归档做一次 eager 校验。
## Suggested approach
- 在 `replaceRuntimeSkillArchives` 写入投影后、sandbox 启动前，对 builtin 归档并发触发一次 `loadSkillArchive`（预热 + 校验），失败则中止 launch；custom 归档保持惰性。🚨 migration 00032 回滚后会破坏 /skills 只读命名空间契约
migration 00032 为每个存量 filesystem 回填 /skills 目录行并清空 skill_prewarm jobs，新代码把 /skills 纳入 filestoreFixedRootPaths 并用 rejectSkillMutation 守卫所有写端点。但如果部署后回滚到旧二进制：旧代码不认识 /skills 固定根（其列表是 /outputs,/uploads,/transcripts,/tool_results），也没有 rejectSkillMutation 守卫，客户端即可通过 createFile/makeDirectory/moveFile 向 /skills 写入真实的 filestore_entries 行。重新前滚部署后，这些用户行会与虚拟投影并存，listSkillDirectory 会把两者合并展示；而 migration 的 do $$ ... raise exception 守卫只在 up 时执行，不会发现这种污染。设计文档未提及 00032 的回滚安全性。这是一次单向门迁移：前向安全，但回滚路径会破坏命名空间契约且无法自愈。
Technical details
# migration 00032 回滚后会破坏 `/skills` 只读命名空间契约
## Affected sites
- `internal/db/migrations/00032_add_filestore_skill_archives.sql:107-108` — `delete from jobs where type = 'skill_prewarm'` + 为所有 `fs.deleted_at is null` 的 filesystem 插入 `/skills` 目录行。
- `internal/db/filestore_fixed_roots.go:5` — `/skills` 加入固定根列表（新代码独有；旧代码列表只有四个根）。
- `internal/filestore/service.go:500-507` — `rejectSkillMutation` 守卫（新代码独有）。
- `internal/db/migrations/00032_add_filestore_skill_archives.sql:46-69` — `do $$` 守卫仅在 up 迁移时执行，回滚→前滚后不会重新检测污染。
## Required outcome
- 回滚到旧代码后，`/skills` 命名空间要么不可写，要么前滚时能检测并拒绝被污染的状态；避免用户行与虚拟投影持久并存。
## Suggested approach
- 在 design doc / runbook 中显式标注 00032 为生产环境不可回滚迁移，或附带回滚前的数据清理脚本（删除 `/skills` 下的真实 `filestore_entries` 行）。
- 或在应用启动时增加自检：若 `/skills` 下存在非目录或带 `managed_by`/`source_file_uuid` 的真实 entry，则拒绝启动并提示需要人工清理。
## Open questions for the human
- 生产环境是否允许 00032 回滚？若允许，回滚后的 `/skills` 清理由谁负责？ℹ️ Nitpicks
internal/skills/resolver.go:40—NewRuntimeResolver仍接受store storage.ObjectStore参数但完全忽略（对象加载已移至filestore.Service.loadSkillArchive）。为减少误导，建议移除该参数或加一行注释说明为签名兼容保留。internal/filestore/skill_archives.go:383—validateSkillZipPath现在拒绝任何含\的路径，旧代码（inspectSkillZipFiles）会先ReplaceAll(name, "\\", "/")归一化。这是更严格的收紧，但来自 Windows 打包工具、使用反斜杠分隔符的历史归档会在首次读取时以 500 失败。若需兼容可保留归一化。- S3 对象保留：deleted custom skill version 与 pruned builtin 的 S3 对象不再被任何路径删除（
tests/skills_api_test.go/tests/skills_seed_test.go显式断言保留）。managed-agent-skills-runtime.md:103-107已明确这是「独立的 reference-aware catalog GC」的后续工作，当前选择保留对象以保活动 Session 快照稳定性。建议跟进一个 issue 跟踪该 GC，避免无界存储泄漏长期悬而未决。
anthropic/glm-5.2 ｜ 𝕏
| func (s *Service) loadSkillArchive( | ||
| ctx context.Context, | ||
| projection db.FilestoreSkillArchive, | ||
| ) (*loadedSkillArchive, *apiError) { | ||
| if s.skillArchives == nil { | ||
| return nil, internalError("load skill archive", errors.New("skill archive cache is unavailable")) | ||
| } | ||
| cacheKey := strings.Join([]string{projection.S3Bucket, projection.S3Key, projection.SHA256}, "\x00") | ||
| if archive, ok := s.skillArchives.get(cacheKey); ok { | ||
| return archive, nil | ||
| } | ||
| if s.store == nil { | ||
| return nil, internalError("load skill archive", errors.New("object store is unavailable")) | ||
| } | ||
| if projection.S3Bucket != s.store.Name() { | ||
| return nil, internalError("load skill archive", errors.New("skill archive bucket is unavailable")) | ||
| } | ||
| if projection.SizeBytes <= 0 || projection.SizeBytes > maxSkillArchiveBytes { | ||
| return nil, internalError("load skill archive", errors.New("skill archive size is invalid")) | ||
| } | ||
| object, err := s.store.Open(ctx, projection.S3Key, nil) | ||
| if err != nil { | ||
| return nil, mapBlobstoreError("load skill archive", err) | ||
| } | ||
| defer object.Body.Close() | ||
| data, err := io.ReadAll(io.LimitReader(object.Body, maxSkillArchiveBytes+1)) | ||
| if err != nil { | ||
| return nil, mapBlobstoreError("read skill archive", err) | ||
| } | ||
| if int64(len(data)) != projection.SizeBytes { | ||
| return nil, internalError("validate skill archive", errors.New("skill archive size mismatch")) | ||
| } | ||
| sum := sha256.Sum256(data) | ||
| if !strings.EqualFold(hex.EncodeToString(sum[:]), projection.SHA256) { | ||
| return nil, internalError("validate skill archive", errors.New("skill archive checksum mismatch")) | ||
| } | ||
| archive, err := indexSkillArchive(projection, data) | ||
| if err != nil { | ||
| return nil, internalError("validate skill archive", err) | ||
| } | ||
| s.skillArchives.put(cacheKey, archive) | ||
| return archive, nil | ||
| } |
There was a problem hiding this comment.
SKILL.md 存在、zip-slip）现在只在这里——loadSkillArchive 首次读取时——才执行，而旧的 prepareRuntimeSkillMount 路径在 launch 时即通过 validateRuntimeSkillArchive eager 校验。设计文档（managed-agent-skills-runtime.md:128,131）把这一行为明确为「resolver 不在启动路径下载 archive」「损坏 archive fail closed」，因此是记录在案的设计决策。需要确认的是操作面代价：一个 sha256/size/zip 与对象实际不符的投影行会在 launch 成功、sandbox 已启动之后，于客户端首次读取 /skills 时以 HTTP 500 暴露。若希望保留惰性校验的启动延迟收益、同时避免运行期 500，可在 replaceRuntimeSkillArchives 写入投影后对 builtin 归档做一次 eager 预热校验。
Technical details
# 归档内容校验推迟到首次读取
## Affected sites
- `internal/environments/runner.go:404` — `replaceRuntimeSkillArchives` 只调用 `ReplaceFilestoreSkillArchives`，后者仅 `validateFilestoreSkillArchiveInput` 格式校验。
- `internal/db/filestore_skill_archives.go:243-264` — 格式校验（hex 长度、source 枚举、size>0），不读取对象字节。
- `internal/filestore/skill_archives.go:269-311` — 首次读取才比对 sha256、解析 zip、校验 SKILL.md。
- `docs/design/be/managed-agent-skills-runtime.md:128,131` — 明确记录为设计意图。
## Required outcome
- 若惰性校验是最终决策，在 design doc 补充「运行期 500 是预期故障模式」并配套告警；若希望 fail-fast，在 launch 路径对 catalog 归档做一次 eager 校验。| archive, apiErr := s.loadSkillArchive(ctx, projection) | ||
| if apiErr != nil { | ||
| return listDirectoryResponse{}, apiErr | ||
| } |
There was a problem hiding this comment.
loadSkillArchive 失败（checksum 不匹配、非法 zip、缺 SKILL.md 等）会让整个 /skills 列举返回 HTTP 500，即便其余归档完全健康。设计文档把损坏归档表述为「fail closed」是合理的，但当前爆炸半径是「整个 /skills 命名空间」而非「该归档自身子树」。配合上一条 finding（惰性校验），session 内任意一个坏归档会使 ListDirectory("/skills") 这种顶层列举持续不可用。建议跳过该归档继续处理其余，或仅对该归档子树返回降级结果。resolveSkillNode（:256-258）有同样问题。
| db: database, | ||
| store: store, | ||
| } | ||
| func NewRuntimeResolver(_ config.Config, database *db.DB, _ storage.ObjectStore) *RuntimeResolver { |
There was a problem hiding this comment.
ℹ️ store storage.ObjectStore 参数已被忽略（对象加载移至 filestore.Service.loadSkillArchive），仅为签名兼容保留。建议移除该参数或加一行注释说明，避免读者误以为 resolver 仍做对象 I/O。
There was a problem hiding this comment.
已在 5bee93c 中处理：NewRuntimeResolver 签名精简为只接受 database，RuntimeResolver.store 字段也一并移除。
anthropic/glm-5.2 ｜ 𝕏
| } | ||
|
|
||
| func validateSkillZipPath(name string) (string, []string, error) { | ||
| if name == "" || strings.Contains(name, "\\") || strings.HasPrefix(name, "/") || strings.Contains(name, "\x00") { |
There was a problem hiding this comment.
ℹ️ validateSkillZipPath 现在直接拒绝任何含 \ 的路径。旧代码（inspectSkillZipFiles）会先 strings.ReplaceAll(name, "\\", "/") 归一化后再校验。这是更严格的收紧，但来自 Windows 打包工具、用反斜杠分隔符的历史归档会在首次读取时以 500 失败。若需兼容这类归档，可保留归一化逻辑。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/environments/runner.go (1)
400-406: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFreeze skill version selection before launching a managed-agent Session.
prepareManagedAgentLaunchreadssession.AgentSnapshotand resolveslatestreferences fresh every time, thenreplaceRuntimeSkillArchivesoverwritesfilestore_skill_archiveswith the resolvedVersionUUID/archive fields. If an existing cloud Session work is re-run with the same"latest"refs after the catalog active version has changed, the sandbox projection can be remounted to a different version despite the design doc sayinglatestis frozen after Session launch. Persist or otherwise lock the resolved skill versions per filesystem launch, and avoid re-resolving/lateral updating an already-launched Session.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/environments/runner.go` around lines 400 - 406, Update prepareManagedAgentLaunch and its resolveRuntimeSkills/replaceRuntimeSkillArchives flow to freeze resolved skill versions per filesystem launch. Reuse the persisted resolution for an already-launched Session instead of resolving latest references again or overwriting filestore_skill_archives with newer catalog versions; only resolve and persist versions for the initial launch.
🧹 Nitpick comments (1)
internal/skills/handler.go (1)
382-397: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftMake projection-aware archive cleanup explicit and testable.
The skill handlers no longer delete or enqueue archive cleanup, so correctness depends on the new Filestore cleanup owner. Current tests retain archives but never create projections, leaving both retention and reclamation paths unverified.
internal/skills/handler.go#L382-L397: verify skill deletion is followed by retryable projection-aware cleanup.internal/skills/handler.go#L653-L664: apply the same guarantee to version deletion.tests/skills_api_test.go#L399-L437: create an active projection and add an eventual-cleanup case without one.tests/skills_seed_test.go#L165-L166: make prune retention depend on an actual projection or verify cleanup after projection removal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/skills/handler.go` around lines 382 - 397, Make archive cleanup after skill deletion explicit and retryable through the projection-aware Filestore cleanup owner in the skill deletion handler, and apply the same guarantee to the version deletion handler. In tests/skills_api_test.go lines 399-437, create an active projection and add an eventual-cleanup case without one; in tests/skills_seed_test.go lines 165-166, base prune retention on an actual projection or verify cleanup after projection removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/db/filestore_skill_archives.go`:
- Around line 194-196: Update the archive version handling in the relevant
filestore method to return ErrDuplicate when seenVersions detects an existing
(source, skill_version_uuid) key, rather than continuing and silently omitting
the later directory; preserve normal processing for unique version keys.
In `@internal/filestore/skill_archives.go`:
- Around line 185-196: Update resolveSkillNode to return the caller-scoped
projection resolved for the current workspace alongside the node and archive,
rather than relying on the cached archive.projection. Change readSkillMetadata
to pass that projection to skillNodePayload, and update readSkillFile’s
resolveSkillNode call site for the new return arity while preserving its
existing behavior.
- Around line 111-183: Update listSkillDirectory to special-case non-recursive
requests for skillNamespacePath: derive the top-level directory entries directly
from each projection.VirtualPath, deduplicate them as needed, and skip
s.loadSkillArchive entirely. Preserve the existing archive-loading traversal for
recursive listings and all non-top-level paths, including sorting, pagination,
and not-found behavior.
---
Outside diff comments:
In `@internal/environments/runner.go`:
- Around line 400-406: Update prepareManagedAgentLaunch and its
resolveRuntimeSkills/replaceRuntimeSkillArchives flow to freeze resolved skill
versions per filesystem launch. Reuse the persisted resolution for an
already-launched Session instead of resolving latest references again or
overwriting filestore_skill_archives with newer catalog versions; only resolve
and persist versions for the initial launch.
---
Nitpick comments:
In `@internal/skills/handler.go`:
- Around line 382-397: Make archive cleanup after skill deletion explicit and
retryable through the projection-aware Filestore cleanup owner in the skill
deletion handler, and apply the same guarantee to the version deletion handler.
In tests/skills_api_test.go lines 399-437, create an active projection and add
an eventual-cleanup case without one; in tests/skills_seed_test.go lines
165-166, base prune retention on an actual projection or verify cleanup after
projection removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 892b9c8c-3ce6-4be5-bc9c-4a2c9d4e2f41
📒 Files selected for processing (45)
docs/design/be/e2b-sandbox-image-contract.mddocs/design/be/filestore.mddocs/design/be/managed-agent-skills-runtime.mddocs/design/be/runtime-configuration.mddocs/design/be/skills-builtin-seed-and-storage.mdinternal/agents/handler.gointernal/api/server.gointernal/db/filestore_cleanup.gointernal/db/filestore_cleanup_sqlx_test.gointernal/db/filestore_filesystems.gointernal/db/filestore_fixed_roots.gointernal/db/filestore_skill_archives.gointernal/db/filestore_skill_archives_test.gointernal/db/migrations/00032_add_filestore_skill_archives.sqlinternal/db/migrations/00033_validate_filestore_skill_archive_checksum.sqlinternal/db/skill_prewarm.gointernal/deployments/handler.gointernal/environments/rclone_filestore.gointernal/environments/rclone_filestore_test.gointernal/environments/runner.gointernal/filestore/service.gointernal/filestore/service_test_support_test.gointernal/filestore/skill_archives.gointernal/filestore/skill_archives_test.gointernal/networkpolicy/metadata_test.gointernal/runtime/e2bruntime/runtime.gointernal/runtime/e2bruntime/runtime_test.gointernal/skillprewarm/enqueuer.gointernal/skillprewarm/enqueuer_test.gointernal/skillprewarm/worker.gointernal/skillprewarm/worker_test.gointernal/skills/archive_limits.gointernal/skills/handler.gointernal/skills/mount_manifest.gointernal/skills/mount_manifest_test.gointernal/skills/resolver.gointernal/skills/seed.gomain.gotests/environments_runner_cloud_test.gotests/filestore_db_test.gotests/filestore_fixed_roots_test.gotests/filestore_provision_roots_test.gotests/skill_prewarm_api_test.gotests/skills_api_test.gotests/skills_seed_test.go
💤 Files with no reviewable changes (12)
- tests/skill_prewarm_api_test.go
- internal/skills/seed.go
- internal/skills/archive_limits.go
- internal/skillprewarm/enqueuer_test.go
- main.go
- internal/skills/mount_manifest_test.go
- internal/skillprewarm/enqueuer.go
- internal/skills/mount_manifest.go
- internal/skillprewarm/worker.go
- internal/skillprewarm/worker_test.go
- internal/db/skill_prewarm.go
- internal/runtime/e2bruntime/runtime_test.go
| if _, exists := seenVersions[versionKey]; exists { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject duplicate archive versions rather than silently dropping one.
A later input with the same (source, skill_version_uuid) but a different directory returns success while omitting that requested /skills path. The stored projection then depends on input order. Return ErrDuplicate instead; the schema already declares this combination unique.
Proposed fix
if _, exists := seenVersions[versionKey]; exists {
- continue
+ return fmt.Errorf(
+ "duplicate filestore skill version %q from source %q: %w",
+ archive.SkillVersionUUID,
+ archive.Source,
+ ErrDuplicate,
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, exists := seenVersions[versionKey]; exists { | |
| continue | |
| } | |
| if _, exists := seenVersions[versionKey]; exists { | |
| return fmt.Errorf( | |
| "duplicate filestore skill version %q from source %q: %w", | |
| archive.SkillVersionUUID, | |
| archive.Source, | |
| ErrDuplicate, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/db/filestore_skill_archives.go` around lines 194 - 196, Update the
archive version handling in the relevant filestore method to return ErrDuplicate
when seenVersions detects an existing (source, skill_version_uuid) key, rather
than continuing and silently omitting the later directory; preserve normal
processing for unique version keys.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/environments_runner_cloud_test.go (1)
624-645: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the test to match its new behavior.
TestEnvironmentRunnerFailsWhenSkillResolverUnavailablenow expectsRunOnceto succeed and create a projection, so the name still describes the removed behavior. Rename it to reflect the fallback/projection path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/environments_runner_cloud_test.go` around lines 624 - 645, Rename the test function TestEnvironmentRunnerFailsWhenSkillResolverUnavailable to reflect that RunOnce succeeds through the missing-resolver fallback and creates a skill projection. Update only the test name and any references to it, preserving the existing assertions and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/environments_runner_cloud_test.go`:
- Around line 624-645: Rename the test function
TestEnvironmentRunnerFailsWhenSkillResolverUnavailable to reflect that RunOnce
succeeds through the missing-resolver fallback and creates a skill projection.
Update only the test name and any references to it, preserving the existing
assertions and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60394053-a021-48f8-86cc-14165eee1084
📒 Files selected for processing (8)
docs/design/be/e2b-sandbox-image-contract.mddocs/design/be/filestore.mddocs/design/be/managed-agent-skills-runtime.mddocs/design/be/runtime-configuration.mdinternal/environments/rclone_filestore.gointernal/environments/rclone_filestore_test.gointernal/environments/runner.gotests/environments_runner_cloud_test.go
💤 Files with no reviewable changes (2)
- internal/environments/rclone_filestore.go
- internal/environments/runner.go
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/design/be/runtime-configuration.md
- docs/design/be/e2b-sandbox-image-contract.md
- docs/design/be/filestore.md
- docs/design/be/managed-agent-skills-runtime.md
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
ℹ️ 无新增严重问题——仅一处文档与实现不一致的小建议。上一轮 Pullfrog review 提出的 4 个线程（损坏归档爆炸半径、惰性校验漂移、migration 00032 回滚、resolver 未用
store参数）均未被这两个新提交触及，相关代码未变，线程保持开放。
Reviewed changes — 这两个新提交（c6f4eaa「Delegate mount destination creation to multimount」+ 9edc20b「Unify skills mount permissions with read-only mounts」）把 /skills 第五个挂载的权限从专用的 uid=0/gid=0/0555/0444 统一为与其他三个只读 mount 一致的 uid=999/gid=1000/0755/0644，并把挂载点目录创建下放给 rclone-filestore multimount 内部 MkdirAll，Runner 不再单独准备挂载目录。
- 统一
/skills挂载权限 —buildRcloneMultimountConfig的mounthelper 现在对全部五个挂载统一使用UID=999、GID=1000、目录0755、文件0644，/skills不再走独立的 root-only 配置；rclone_filestore_test.go同步断言 skills mount 的四项权限。 - 挂载点创建委托给 multimount — 新增常量
rcloneSkillsDestination = "/root/.claude/skills"，/skills通过统一的mount(...)加入五个挂载；Runner 不再有独立的rcloneMountPreparationCommand/ symlink 清理步骤，由 multimount 内部MkdirAll创建 destination。 - 设计文档同步 —
e2b-sandbox-image-contract.md、managed-agent-skills-runtime.md、filestore.md、runtime-configuration.md更新挂载计数为五、说明/skills与其他只读 mount 统一权限、Runner 不再单独准备挂载目录。
权限统一方向正确：Claude Code 在 sandbox 内以 uid 999 运行，旧的 uid=0/0555 反而是与其他三个只读 mount 不一致的特例，统一后与 discovery 路径的所有权模型一致。挂载点创建下沉到 multimount 也消除了 Runner 的重复准备逻辑。
anthropic/glm-5.2 ｜ 𝕏
| A --> W["Write config and chmod 0600"] | ||
| W --> B["Start fixed rclone-filestore binary"] | ||
| B --> C{"All four mounts ready?"} | ||
| W --> S["Remove legacy skill symlink and create mountpoint"] |
There was a problem hiding this comment.
ℹ️ 这一步「Remove legacy skill symlink and create mountpoint」与本次提交「Delegate mount destination creation to multimount」的意图不一致。代码侧 Runner 已无任何 symlink 清理或挂载点准备步骤（internal/environments/rclone_filestore.go 只有 rcloneConfigPermissionsCommand / rcloneConfigCleanupCommand，无 rcloneMountPreparationCommand），同文件正文也写明「Runner 不再单独准备挂载目录」「rclone-filestore multimount 在内部对 destination 执行 MkdirAll」，验收第 6 条同样要求镜像不得预置软链、由 multimount 创建 destination。
建议把 S 节点删除或改写为 multimount 内部行为（例如 S["multimount MkdirAll destinations"] 由 B 触发），让 mermaid 与正文、代码一致；managed-agent-skills-runtime.md 第 146 行序列图中的 R->>E: Remove legacy skill symlink and create mountpoint 同步处理。
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
ℹ️ 无新增严重问题——架构统一方向正确，一处 DB 层隔离不对称值得后续收紧。上一轮 Pullfrog review 的 4 个线程（损坏归档爆炸半径、惰性校验、migration 回滚、resolver 未用参数）均未被这两个新提交触及，相关代码未变，线程保持开放。注意：非递归
ListDirectory("/skills")的顶层路径已跳过loadSkillArchive，损坏归档爆炸半径 thread 的顶层列举子项已被间接修复。
Reviewed changes — 这两个新提交（20dbdc8「Document runtime skill archive replacement behavior」+ 2f830f3「Unify skill archives with Filestore entries」）把 skill archive 从独立表 filestore_skill_archives 统一为 filestore_entries 中 kind='archive'、managed_by='skill_archive' 的受管 entry，并同步更新设计文档。
- archive 统一到
filestore_entries— migration 00034 删除旧表、把archive加入 entry kind 枚举、增加blob_shape_check/archive_shape_check（NOT VALID）和filestore_entries_skill_archive_active_v1_key唯一索引（按managed_resource_uuid）；migration 00035 单独验证三个约束。FilestoreSkillArchiveEntryInput/ReplaceFilestoreSkillArchiveEntries/ListFilestoreSkillArchiveEntries取代旧投影 API。 - 容量与清理隔离 —
OwnedBytes()对kind != 'file'返回 0，archive 不计入 workspace 存储；buildFilestoreEntriesPageQuery显式过滤kind <> 'archive'；retireFilestoreCleanupNamespaceEntriesQuery软删除kind in ('directory','archive')且不产生 object cleanup job。 - 重复版本从静默跳过改为 fail-closed —
normalizeFilestoreSkillArchiveEntries对重复skill_version_uuid返回ErrDuplicate（旧代码continue跳过），与设计文档「不能让后一个静默覆盖前一个」一致。 - 顶层
/skills列举免下载 —listSkillDirectory对非递归ListDirectory("/skills")直接从 archive entry 的path返回一级目录，跳过loadSkillArchive。 - 设计文档同步 —
filestore.md、managed-agent-skills-runtime.md更新为 archive entry 模型，记录旧表删除、约束拆分和生命周期。
migration 设计稳健：NOT VALID/validate 拆分与 00027/00028、00029/00030 一脉相承；既有 filestore_entries_filesystem_path_active_v2_key 已覆盖路径唯一性；filestore_entries_archive_shape_check 与 00032 回填的 /skills directory row 不冲突（kind <> 'archive' 分支 managed_by is distinct from 'skill_archive' 对 NULL 成立）。
ℹ️ Nitpicks
internal/db/filestore_sqlx.go:112-124（未在 diff 中，pre-existing）—getActiveFilestoreEntrySQLX（GetFilestoreEntry的底层查询）没有kind <> 'archive'过滤，而同表的buildFilestoreEntriesPageQuery已显式排除。当前 service 层（ReadFile/ReadMetadata/rejectSkillMutation）拦截了所有/skills/...路径，但 DB 层契约现在是隐式的：未来任意内部调用方直接GetFilestoreEntry(ctx, ws, fs, "/skills/demo")会拿到原始 archive 行而非虚拟视图。详见 inline comment。internal/db/migrations/00034_unify_filestore_skill_archives.sql:97-98— Down 路径delete from filestore_entries where kind = 'archive'会销毁新代码写入的 archive 行，但未像 Up 方向那样加注释说明。若回滚时 session 状态需要保留，值得在注释中标注 intentional data loss。
anthropic/glm-5.2 ｜ 𝕏
| query := filestoreEntrySelectSQL() + ` | ||
| where workspace_uuid = :workspace_uuid | ||
| and filesystem_uuid = :filesystem_uuid | ||
| and kind <> 'archive' |
There was a problem hiding this comment.
ℹ️ 这里显式加了 kind <> 'archive'，但同表的 getActiveFilestoreEntrySQLX（internal/db/filestore_sqlx.go:112-124，即 GetFilestoreEntry 的底层查询）没有对应的 kind 过滤。当前所有 service 层调用方在到达 GetFilestoreEntry 之前已被 rejectSkillMutation 或 /skills 前缀分流拦截，所以不是当前 bug。但 archive 行现在是 filestore_entries 中的真实行（path='/skills/<dir>'，size_bytes 非空），未来任何内部调用方直接 GetFilestoreEntry(ctx, ws, fs, "/skills/demo") 会拿到原始 archive 行而非 NotFound 或虚拟视图——悄然绕过 skill 虚拟化契约。
Technical details
# GetFilestoreEntry 缺少 archive kind 过滤
## Affected sites
- `internal/db/filestore_sqlx.go:112-124` — `getActiveFilestoreEntrySQLX` 查询无 `kind` 过滤，`GetFilestoreEntry` 的公开契约隐式依赖调用方不查询 `/skills/<dir>` 路径。
- `internal/db/filestore_entries.go:64` — `buildFilestoreEntriesPageQuery` 已显式 `kind <> 'archive'`，两个读路径的隔离策略不对称。
## Required outcome
- `GetFilestoreEntry` 对 `kind='archive'` 行返回 `ErrNotFound`（或在 `getActiveFilestoreEntrySQLX` 加 `and kind <> 'archive'`），使 DB 层隔离契约不依赖 service 层的前缀检查。
## Suggested approach
- 在 `getActiveFilestoreEntrySQLX` 的 WHERE 子句中加 `and kind <> 'archive'`，或在 `GetFilestoreEntry` 返回后检查 `entry.Kind == archive` 并映射为 `ErrNotFound`。
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/db/migrations/00034_unify_filestore_skill_archives.sql (1)
85-93: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNon-concurrent index build takes an
ACCESS EXCLUSIVE-adjacent write lock onfilestore_entries.Static analysis flags this;
filestore_entriesis a hot table, so a plainCREATE UNIQUE INDEXblocks writes for the duration of the build. If the table is already large in any deployed environment, preferCONCURRENTLY— note this requires-- +goose NO TRANSACTIONat the top of the file, and the same applies to theDROP INDEXon Line 100. If the table is still small, this is fine as-is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/db/migrations/00034_unify_filestore_skill_archives.sql` around lines 85 - 93, Update the migration to use concurrent index creation and add the required Goose NO TRANSACTION directive at the top of the file; also make the corresponding filestore_entries skill-archive index drop concurrent so both operations can run without blocking writes.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/db/filestore_archive_entries.go`:
- Around line 245-248: Canonicalize the skill version UUID in the normalization
flow before deduplication and storage. In the code around parsed UUID
validation, retain the result of uuid.Parse and replace the raw trimmed
versionUUID with parsedVersion.String(), ensuring seenVersions and
managed_resource_uuid use the canonical representation.
---
Nitpick comments:
In `@internal/db/migrations/00034_unify_filestore_skill_archives.sql`:
- Around line 85-93: Update the migration to use concurrent index creation and
add the required Goose NO TRANSACTION directive at the top of the file; also
make the corresponding filestore_entries skill-archive index drop concurrent so
both operations can run without blocking writes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e09c2ec-1bf7-448d-a159-d85b58e78853
📒 Files selected for processing (21)
docs/design/be/e2b-sandbox-image-contract.mddocs/design/be/filestore.mddocs/design/be/managed-agent-skills-runtime.mdinternal/db/filestore.gointernal/db/filestore_archive_entries.gointernal/db/filestore_archive_entries_test.gointernal/db/filestore_cleanup.gointernal/db/filestore_cleanup_sqlx_test.gointernal/db/filestore_entries.gointernal/db/filestore_entry_methods.gointernal/db/migrations/00034_unify_filestore_skill_archives.sqlinternal/db/migrations/00035_validate_filestore_archive_entries.sqlinternal/environments/rclone_filestore.gointernal/environments/rclone_filestore_test.gointernal/environments/runner.gointernal/filestore/service.gointernal/filestore/service_test_support_test.gointernal/filestore/skill_archives.gointernal/filestore/skill_archives_test.gotests/environments_runner_cloud_test.gotests/filestore_db_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/environments/rclone_filestore_test.go
- internal/environments/runner.go
- internal/filestore/skill_archives_test.go
- docs/design/be/filestore.md
- docs/design/be/e2b-sandbox-image-contract.md
- internal/filestore/skill_archives.go
- tests/environments_runner_cloud_test.go
| versionUUID := strings.TrimSpace(input.SkillVersionUUID) | ||
| if _, err := uuid.Parse(versionUUID); err != nil { | ||
| return normalizedFilestoreSkillArchiveEntry{}, fmt.Errorf("invalid skill version UUID: %w", err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)filestore_archive_entries\.go$|manage|filestore' || true
echo
echo "== file outline =="
ast-grep outline internal/db/filestore_archive_entries.go --view compact || true
echo
echo "== relevant normalization section =="
sed -n '190,260p' internal/db/filestore_archive_entries.go | cat -n
echo
echo "== duplicate/insert-related searches in file =="
rg -n "seenVersions|SkillVersionUUID|managed_resource_uuid|Insert|sqlx|CAST|versionUUID" internal/db/filestore_archive_entries.go -C 4
echo
echo "== schema/migration references for filestore archive entries managed_resource unique =="
rg -n "filestore.*archive|archive_entries|managed_resource_uuid|CREATE TABLE|ALTER TABLE .*archive" internal migration.go migrations -S || trueRepository: superduck-ai/open-managed-agents
Length of output: 22402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== full normalizeFilestoreSkillArchiveEntry with line numbers =="
sed -n '234,272p' internal/db/filestore_archive_entries.go | nl -ba -v234
echo
echo "== migration unique constraints around skill archive entries =="
sed -n '80,95p' internal/db/migrations/00034_unify_filestore_skill_archives.sql | nl -ba -v80
sed -n '180,192p' internal/db/migrations/00032_add_filestore_skill_archives.sql | nl -ba -v180
echo
echo "== go uuid package availability/version =="
if [ -f go.mod ]; then rg -n 'github.com/google/uuid' go.mod go.sum || true; fi
echo
echo "== standalone go uuid Parse behavior if tooling is available =="
tmp="$(mktemp -d)"
cat > "$tmp/main.go" <<'GO'
package main
import (
"fmt"
"github.com/google/uuid"
)
func main() {
inputs := []string{
"12345678-1234-1234-1234-123456789abc",
"12345678-1234-1234-1234-123456789ABC",
"abc12345-1234-1234-1234-123456789abc",
"ABC12345-1234-1234-1234-123456789abc",
}
seen := map[string]struct{}{}
for _, s := range inputs {
parsed, err := uuid.Parse(s)
fmt.Println(s, "parsedErr", err == nil, "parsedString", parsed.String(), "seen", func() bool {
_, ok := seen[s]
return ok
}())
seen[s] = struct{}{}
}
}
GO
go run "$tmp/main.go" 2>/dev/null && true || { echo "go run unavailable"; }Repository: superduck-ai/open-managed-agents
Length of output: 275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== normalizedFilestoreSkillArchiveEntry and normalizeFilestoreSkillArchiveEntry relevant lines =="
awk 'NR>=14 && NR<=21 && NR>=234 && NR<=266 {printf "%5d %s\n", NR, $0}' internal/db/filestore_archive_entries.go
echo
echo "== migration unique indexes relevant lines =="
awk 'NR>=80 && NR<=92' internal/db/migrations/00034_unify_filestore_skill_archives.sql
awk 'NR>=181 && NR<=192' internal/db/migrations/00032_add_filestore_skill_archives.sql
echo
echo "== go uuid package version references =="
awk '/github.com\/google\/uuid/' go.mod go.sum 2>/dev/null || true
echo
echo "== standalone go uuid Parse behavior if available =="
tmp="$(mktemp -d)"
cat > "$tmp/main.go" <<'GO'
package main
import (
"fmt"
"github.com/google/uuid"
)
func main() {
inputs := []string{
"12345678-1234-1234-1234-123456789abc",
"12345678-1234-1234-1234-123456789ABC",
"abc12345-1234-1234-1234-123456789abc",
"ABC12345-1234-1234-1234-123456789abc",
}
seen := map[string]struct{}{}
for _, s := range inputs {
parsed, err := uuid.Parse(s)
fmt.Println(s, "parsedErr", err == nil, "parsedString", parsed.String(), "seen", func() bool {
_, ok := seen[s]
return ok
}())
seen[s] = struct{}{}
}
}
GO
if command -v go >/dev/null 2>&1; then
go run "$tmp"
else
echo "go unavailable"
fiRepository: superduck-ai/open-managed-agents
Length of output: 978
🌐 Web query:
``
💡 Result:
No web search query provided.
🌐 Web query:
github.com google uuid Parse String canonical lowercase source
💡 Result:
In the google/uuid Go package, the Parse function is designed to be lenient and does not require a specific case (upper or lower) for hexadecimal characters [1][2]. It handles various string formats, including the canonical 36-character format, URNs, and other non-standard representations [1][2]. Key details regarding parsing and canonicalization: 1. Case Insensitivity: The Parse function processes hexadecimal digits using a custom helper (typically named xtob), which handles both uppercase and lowercase characters without requiring them to be in a specific case [1][2]. 2. Canonical Output: While Parse accepts various inputs, the String method, which produces the canonical representation of a UUID, always outputs the string in the standard 36-character format (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx) using lowercase hexadecimal characters [1][2]. 3. Lax vs. Strict Parsing: Historically, the package's Parse function has been intentionally lenient, accepting formats beyond what is strictly defined by RFC standards (e.g., Microsoft-style braces or raw hex) [1][2][3]. Recent developments in the project have introduced "Strict" variants of these functions to enforce RFC-9562 compliance for users who require stricter validation [3]. In summary, you can parse any valid hex-encoded UUID string regardless of case, and the library will normalize it to its canonical lowercase form when you call the String method on the resulting UUID object [1][2].
Citations:
- 1: https://github.com/google/uuid/blob/master/uuid.go
- 2: https://github.com/google/uuid/blob/c2e93f3ae59f2904160ceaab466009f965df46d6/uuid.go
- 3: feat: Add strict (RFC-9562-compliant) parsing and validation functions google/uuid#192
Canonicalize skill version UUIDs before deduplication/storage.
uuid.Parse is case-insensitive, but the raw trimmed value is used for seenVersions and managed_resource_uuid; use parsedVersion.String() so case-different duplicates and mixed-case insertions are handled consistently.
🔧 Proposed fix
versionUUID := strings.TrimSpace(input.SkillVersionUUID)
- if _, err := uuid.Parse(versionUUID); err != nil {
+ parsedVersion, err := uuid.Parse(versionUUID)
+ if err != nil {
return normalizedFilestoreSkillArchiveEntry{}, fmt.Errorf("invalid skill version UUID: %w", err)
}
+ versionUUID = parsedVersion.String()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| versionUUID := strings.TrimSpace(input.SkillVersionUUID) | |
| if _, err := uuid.Parse(versionUUID); err != nil { | |
| return normalizedFilestoreSkillArchiveEntry{}, fmt.Errorf("invalid skill version UUID: %w", err) | |
| } | |
| versionUUID := strings.TrimSpace(input.SkillVersionUUID) | |
| parsedVersion, err := uuid.Parse(versionUUID) | |
| if err != nil { | |
| return normalizedFilestoreSkillArchiveEntry{}, fmt.Errorf("invalid skill version UUID: %w", err) | |
| } | |
| versionUUID = parsedVersion.String() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/db/filestore_archive_entries.go` around lines 245 - 248,
Canonicalize the skill version UUID in the normalization flow before
deduplication and storage. In the code around parsed UUID validation, retain the
result of uuid.Parse and replace the raw trimmed versionUUID with
parsedVersion.String(), ensuring seenVersions and managed_resource_uuid use the
canonical representation.
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 无新增问题——这是一次干净的行为保持重构，测试覆盖到位。上一轮 Pullfrog 的 5 个线程（损坏归档爆炸半径、惰性校验、migration 00032 回滚、
NewRuntimeResolver未用store参数、GetFilestoreEntry缺kind <> 'archive'）均未被dcd2122触及，相关代码未变，线程保持开放。
Reviewed changes — 新提交 dcd2122「Refactor filestore reads through pluggable path backends」把 filestore 的读取从 *Service 上的方法 + rejectSkillMutation/isSkillNamespacePath 前缀分派，重构为 pathRouter + 可插拔 pathBackend/readOnlyPathBackend 抽象：普通读取由 persistentPathBackend 处理，/skills 虚拟读取由 skillArchivePathBackend 处理，写守卫统一走 pathRouter.authorizeMutation。
- 抽取
pathRouter与 backend 接口 — 新增path_backend.go：pathBackend（list/readFile/readMetadata）、readOnlyPathBackend（namespaceRoot/matchesRead/containsPath）、pathRouter（backendFor按操作+路径选 backend，authorizeMutation拒绝只读 namespace 的 mutation）。新增只读虚拟 namespace 只需注册新 backend，无需在每个 API 入口加特例。 persistentPathBackend承接普通读取 — 新增persistent_backend.go，把原先内联在Service.ListDirectory/ReadFile/ReadMetadata中的 DB + ObjectStore 读取逻辑原样搬入；空区间短路、ErrNotFound→ blob 缺失映射、Size由 DB 元数据决定等行为均保留。skillArchivePathBackend取代*Service的 skill 方法 —listSkillDirectory/readSkillFile/readSkillMetadata/resolveSkillNode/loadSkillArchive从*Service方法变为skillArchivePathBackend方法，缓存引用从s.skillArchives改为b.cache。matchesRead精确复现旧前缀分派：/skills/...全部接管，/skills根只接管 list，file/metadata 仍回落 persistent。- 写守卫统一 — 所有 mutation 端点（
MakeDirectory/RemoveDirectory/CreateFile/CopyFile/MoveFile/MoveDirectory/RemoveFile）改用s.paths.authorizeMutation(...)，与旧rejectSkillMutation等价但泛化为遍历所有只读 backend。 - 测试扩展 —
path_backend_test.go覆盖路由选择与 mutation 拒绝（含/skills-old前缀边界、混合/outputs+/skills路径）；service_test_support_test.go的fakeServiceDatabase增加ListFilestoreSkillArchiveEntries桩，mutation 拒绝表测试新增make directory/remove file/copy from/copy into/move file from/move file into/move directory from/move directory into用例，并断言ReadMetadata("/skills")返回 persistent directory entry。
重构忠实保持行为不变：persistentPathBackend.readFile 与旧内联逻辑逐行对应，matchesRead 对 /skills 根的 file/metadata 回落与旧 strings.HasPrefix(path, "/skills/") 守卫一致。internal/filestore 全部测试通过。
｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using anthropic/glm-5.2 ｜ 𝕏
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/design/be/filestore.md (1)
372-372: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the acceptance checklist to cover five fixed roots.
This still says “四根目录回滚,” contradicting the five-root contract defined above, which now includes
/skills. Update the wording so acceptance criteria cannot omit the new root.As per coding guidelines, design documentation must stay synchronized with behavior and contract changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/be/filestore.md` at line 372, Update the acceptance checklist wording around “四根目录回滚” to state that rollback coverage includes five fixed roots, explicitly including /skills. Keep the surrounding acceptance scenarios unchanged.Source: Coding guidelines
internal/filestore/service.go (1)
95-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssign a stable cursor ID for skill archive pagination.
skillArchivePathBackend.listDirectoryencodes the next cursor withLastID = 0, butdecodeDirectoryCursorrejects empty path cursors unlessLastID > 0. Repeated/skipped pages can be rejected by the API even though the archive listing is sorted by path. Derive a stable ID from the archive entry/node identity when encoding and checking the skills-backend cursor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/filestore/service.go` around lines 95 - 104, The skill archive pagination flow must use a stable nonzero cursor ID derived from the archive entry or node identity. Update skillArchivePathBackend.listDirectory and its cursor encoding/validation alongside decodeDirectoryCursor so path-sorted cursors no longer use LastID = 0, while preserving consistent IDs across subsequent pages and existing pagination behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/design/be/filestore.md`:
- Line 372: Update the acceptance checklist wording around “四根目录回滚” to state
that rollback coverage includes five fixed roots, explicitly including /skills.
Keep the surrounding acceptance scenarios unchanged.
In `@internal/filestore/service.go`:
- Around line 95-104: The skill archive pagination flow must use a stable
nonzero cursor ID derived from the archive entry or node identity. Update
skillArchivePathBackend.listDirectory and its cursor encoding/validation
alongside decodeDirectoryCursor so path-sorted cursors no longer use LastID = 0,
while preserving consistent IDs across subsequent pages and existing pagination
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dba35f36-2fdb-4262-9425-725115f655af
📒 Files selected for processing (9)
docs/design/be/filestore.mdinternal/filestore/path_backend.gointernal/filestore/path_backend_test.gointernal/filestore/persistent_backend.gointernal/filestore/service.gointernal/filestore/skill_archives.gointernal/filestore/skill_archives_test.gointernal/skills/handler.gointernal/skills/seed.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/skills/seed.go
- internal/filestore/skill_archives_test.go
- internal/filestore/skill_archives.go
- internal/skills/handler.go
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 无新增问题——缓存与并发重构方向正确，测试覆盖到位。上一轮 Pullfrog 的
NewRuntimeResolver未用参数 nitpick 已在本提交中处理（签名精简为NewRuntimeResolver(database *db.DB)，store字段移除），已回复并关闭该线程。其余 3 个开放线程（损坏归档爆炸半径、惰性校验、反斜杠路径）未被本提交触及，相关代码未变，保持开放。
Reviewed changes — 新提交 5bee93c「Use bounded LRU caching and singleflight for skill archives」把手写的 byte-counted LRU 缓存（skillArchiveCache，64 MiB）替换为 hashicorp/golang-lru/v2 的 entry-counted LRU（20 个 archive），并用 golang.org/x/sync/singleflight 合并同一 archive 的并发 cold load；同时精简 NewRuntimeResolver 签名并同步全部调用方与测试。
- 缓存改用
golang-lru/v2—skillArchiveCache手写 LRU（sync.Mutex+container/list+ byte 计数）整体替换为lru.Cache[string, *loadedSkillArchive]（defaultSkillArchiveCacheEntries = 20）；loadSkillArchive的get/put改为Get/Add。设计文档同步记录「单个压缩 archive 最大 8 MiB，理论上限 160 MiB + 索引开销」。 - singleflight 合并并发 cold load — 新增
skillArchivePathBackend.archiveLoads singleflight.Group，loadSkillArchive快路径 miss 后进入Do(cacheKey, ...)；closure 内二次检查缓存（处理 singleflight 入队前刚完成的加载），加载逻辑抽到fetchSkillArchive；失败不写缓存，后续请求可重试。*apiError通过类型断言还原，非*apiError包裹为internalError。 NewRuntimeResolver签名精简 — 移除未使用的config.Config和storage.ObjectStore参数及store字段，所有调用方（main.go、runner_dependencies_test.go、environments_*_test.go、sdk_go_quickstart_demo_test.go、sessions_api_test.go、skills_runtime_resolver_test.go）同步更新。- 测试扩展 — 新增三个测试：
TestSkillArchiveLoadRetriesAfterFailure（失败后重试可成功、object 打开 2 次）、TestSkillArchiveCacheKeepsTwentyMostRecentArchives（LRU 逐出语义、Get刷新 recency、Peek不刷新）、TestSkillArchiveConcurrentColdLoadUsesSingleObjectRead（16 个并发 caller 共享同一结果指针、object 只打开 1 次）。
singleflight 的 context 传播是标准 tradeoff：Do(key, fn) 不接受 context，closure 捕获首个 caller 的 ctx，若其取消则所有 waiter 收到错误——但因错误不缓存，后续请求可重新加载。TestSkillArchiveLoadRetriesAfterFailure 明确验证了这一重试路径。internal/filestore 全部测试通过，go build ./... 无报错。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/filestore/skill_archives.go (2)
30-33: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEntry-count LRU replaces the previous byte-budgeted cache; worst case is ~160 MiB of retained archive bytes.
defaultSkillArchiveCacheEntries = 20bounds entries, not bytes, and eachloadedSkillArchiveretains its fulldataslice (up tomaxSkillArchiveBytes= 8 MiB) plus the zip index. That is a hard ceiling of roughly 160 MiB per backend instance, noticeably above the earlier 64 MiB byte cap. If the byte budget was intentional, considerlru.NewWithEvictplus a running byte accounting, or document the new ceiling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/filestore/skill_archives.go` around lines 30 - 33, The skill archive cache now has an entry-count ceiling that can retain roughly 160 MiB, exceeding the former byte budget. In the cache implementation surrounding defaultSkillArchiveCacheEntries and loadedSkillArchive, restore byte-based accounting with lru.NewWithEvict and evict entries until retained archive data stays within the intended budget, including updates when entries are added or removed.
314-321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
errors.Asover a bare type assertion when unwrapping the singleflight error.
loadErr.(*apiError)fails if the error is ever wrapped (e.g. by a futurefmt.Errorf("%w")in the load path), silently downgrading a mapped error to a generic 500.♻️ Proposed refactor
if loadErr != nil { - apiErr, ok := loadErr.(*apiError) - if !ok { + var apiErr *apiError + if !errors.As(loadErr, &apiErr) { return nil, internalError("load skill archive", loadErr) } return nil, apiErr }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/filestore/skill_archives.go` around lines 314 - 321, Update decodeSkillArchiveLoadResult to unwrap loadErr with errors.As into an *apiError instead of using a direct type assertion, preserving the existing apiErr return for wrapped or unwrapped mapped errors and falling back to internalError only when no apiError is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/filestore/skill_archives.go`:
- Around line 332-339: Update skillArchiveRequestCanceled and its callers so
request_canceled is translated through the existing filestore error mapping
before WriteProtocolError emits it; if the endpoint requires
Anthropic-compatible /v1/* errors, route the failure through
internal/httpapi.WriteError instead. Ensure the new 408 response does not expose
an unmapped request_canceled code outside the established error vocabulary.
---
Nitpick comments:
In `@internal/filestore/skill_archives.go`:
- Around line 30-33: The skill archive cache now has an entry-count ceiling that
can retain roughly 160 MiB, exceeding the former byte budget. In the cache
implementation surrounding defaultSkillArchiveCacheEntries and
loadedSkillArchive, restore byte-based accounting with lru.NewWithEvict and
evict entries until retained archive data stays within the intended budget,
including updates when entries are added or removed.
- Around line 314-321: Update decodeSkillArchiveLoadResult to unwrap loadErr
with errors.As into an *apiError instead of using a direct type assertion,
preserving the existing apiErr return for wrapped or unwrapped mapped errors and
falling back to internalError only when no apiError is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5d3bd2c-af98-4dd5-aa29-fd58a04e30eb
📒 Files selected for processing (17)
docs/design/be/managed-agent-skills-runtime.mdinternal/db/filestore_archive_entries.gointernal/db/filestore_archive_entries_test.gointernal/environments/runner.gointernal/environments/runner_dependencies_test.gointernal/filestore/service.gointernal/filestore/skill_archives.gointernal/filestore/skill_archives_test.gointernal/skills/resolver.gomain.gotests/environments_e2b_integration_test.gotests/environments_full_e2b_bridge_integration_test.gotests/environments_runner_cloud_test.gotests/filestore_db_test.gotests/sdk_go_quickstart_demo_test.gotests/sessions_api_test.gotests/skills_runtime_resolver_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/db/filestore_archive_entries_test.go
- internal/db/filestore_archive_entries.go
- internal/environments/runner.go
- main.go
- tests/filestore_db_test.go
- internal/filestore/service.go
- docs/design/be/managed-agent-skills-runtime.md
- tests/environments_runner_cloud_test.go
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
✅ 无新增问题——历史保留与共享加载隔离两处改动方向正确，与既有唯一索引和缓存语义一致。上一轮的
NewRuntimeResolver未用store参数线程已在5bee93c处理（pullfrog 已回复，尝试 resolve 时权限不足，GitHub 上仍为 open）。其余 5 个开放线程（损坏归档爆炸半径、惰性校验、反斜杠路径、GetFilestoreEntry缺kind过滤、e2b-sandbox-image-contract.mdmermaid 与正文不一致）均未被e4395b0触及，保持开放。
Reviewed changes — 新提交 e4395b0「Preserve skill archive history and isolate shared loads」把 /skills archive entry 的全量替换从硬删除改为软删除（deleted_at）以保留审计历史，并把 singleflight.Do（共享加载绑定首个调用者的 context）升级为 DoChan + context.WithoutCancel(ctx) + 独立 30 秒超时，使单个调用者的取消不再终止其他等待者共享的下载。
- archive entry 替换改为软删除 —
filestoreSkillArchiveEntryDeleteQuery（delete from filestore_entries）改为filestoreSkillArchiveEntryRetireQuery（update ... set deleted_at = :now, updated_at = :now where ... and deleted_at is null）；ListFilestoreSkillArchiveEntries和唯一索引filestore_entries_skill_archive_active_v1_key都已带deleted_at is null谓词，软删除后用相同 path/version 重新插入不会冲突。retire→insert 顺序保持，事务内for update+ namespace lock 串行化不变。 - 共享加载隔离 —
loadSkillArchive从Do改为DoChan，共享任务通过context.WithoutCancel(ctx)+WithTimeout(30s)独立运行，不继承首个调用者的取消或 deadline；每个调用者通过select { ctx.Done / result }独立等待，取消只结束自身返回request_canceled（HTTP 408）。进入 singleflight 前预检ctx.Err()避免为已取消请求凭空创建无等待者的后台加载；取消时不调用Forget，避免后续请求启动第二次同 key 冷下载。失败不写缓存，后续可重试。 - 测试覆盖 — 新增四个并发测试：共享加载超时不缓存失败（重试成功、object 打开 2 次）、已取消请求不启动加载（object 打开 0 次）、leader 取消不中止共享加载（waiter 仍拿到结果、object 打开 1 次）、已加入 flight 的 waiter 独立取消提前返回（leader 仍完成）；DB 测试断言替换后旧行
deleted_at非空。 - 设计文档同步 —
managed-agent-skills-runtime.md更新启动流程第 4 步与「生命周期与对象保留」段落，记录软删除保留历史投影、活动读取与唯一索引只考虑deleted_at is null、singleflight 共享加载的独立超时与取消隔离语义。
软删除与两个 partial unique index（filestore_entries_filesystem_path_active_v2_key、filestore_entries_skill_archive_active_v1_key 均 where deleted_at is null）完全一致；retired 行在 filesystem cleanup 时随 directory/archive 一起软删除，不产生对象回收。DoChan + WithoutCancel + 有界超时的组合正确解决了上一轮记录的 singleflight context 传播 tradeoff——leader 断开不再让等待者集体失败，且所有调用者离开后共享任务最多占用 30 秒。
anthropic/glm-5.2 ｜ 𝕏
There was a problem hiding this comment.
DuckPR reviewer: opencode
Model: anthropic/glm-5.2
ℹ️ 无新增严重问题——迁移合并干净，仅一处测试守卫在合并后变为残留。
Reviewed changes — 新提交 944bfdd「Consolidate filestore archive migrations」把原先四步迁移（旧 00032 建独立 filestore_skill_archives 表 → 00033 收紧 checksum → 00034 删表并统一到 filestore_entries → 00035 验证约束）合并为两步（00032 直接把 archive 加入 filestore_entries.kind + 形状/唯一约束 + /skills 回填 + prewarm 清理；00033 单独 validate 三个约束），消除了中间表及其 drop/重建步骤，设计文档同步更新。
- 消除中间投影表 — 合并后
filestore_skill_archives表不再出现在迁移历史中，filestore_entries从一开始就以kind in ('file','directory','archive')建立约束，避免「先建表再删表」的无谓 DDL 噪音。 - 强化
/skillsnamespace 守卫 — 00032 的do $$现在同时拒绝/skills/%下的既有行和形态不合法的/skills根目录行（错误 kind、错误 parent、带 managed/source 字段），比原 00032 只检查/skills/%更严格。 - Down 路径自洽 — 00032 Down 删除所有 archive 行（含软删除历史）、drop 索引和三个约束、恢复 file/directory-only 约束，并保留
/skills目录行（附注释说明无法区分迁移创建与用户既有）；00033 Down 因 PostgreSQL 不能把已验证约束改回 NOT VALID，正确地 drop 后重建为 NOT VALID，镜像 00032 Up 状态。
迁移历史中约束命名链（00018 建立 → 00024 重命名 → 00029 替换 → 00030 验证 → 00032 替换 → 00033 验证）无冲突。go build ./... 通过，设计文档 (filestore.md、managed-agent-skills-runtime.md) 已更新为引用 00032_add_filestore_archive_entries.sql / 00033_validate_filestore_archive_entries.sql，不再提及 00034/00035 或独立投影表。
上一轮 Pullfrog 的 NewRuntimeResolver 未用 store 参数线程已在 5bee93c 处理（代码已精简，pullfrog 已回复，GitHub 线程因权限不足仍为 open）。其余开放线程（损坏归档爆炸半径、惰性校验、反斜杠路径、GetFilestoreEntry 缺 kind 过滤）均未被 944bfdd 触及，相关代码未变，保持开放。
anthropic/glm-5.2 ｜ 𝕏
| var legacyTable *string | ||
| if err := app.db.Pool.QueryRow( | ||
| context.Background(), | ||
| `select cast(to_regclass('filestore_skill_archives') as text)`, | ||
| ).Scan(&legacyTable); err != nil { | ||
| t.Fatalf("check legacy archive table: %v", err) | ||
| } | ||
| if legacyTable != nil { | ||
| t.Fatalf("legacy archive table still exists: %s", *legacyTable) | ||
| } |
There was a problem hiding this comment.
ℹ️ 这段 legacy filestore_skill_archives 表守卫在 944bfdd 合并迁移后已成为残留——合并后的迁移历史不再包含创建该表的步骤，to_regclass('filestore_skill_archives') 恒为 NULL。测试其余部分（ReplaceFilestoreSkillArchiveEntries → ListFilestoreSkillArchiveEntries → 读取校验）仍有价值，但这 10 行守卫可以移除。
1 participant
Summary
Testing
Not run (not requested).
Summary by CodeRabbit
/root/.claude/skillsdirectory backed by/skillsin the sandbox./skillsmutations are denied.