Skip to content

fix(skills): key GitHub zip cache by commit SHA and invalidate on delete - #1143

Open
3316891527 wants to merge 1 commit into
AAswordman:devfrom
3316891527:fix/skill-repo-zip-cache-stale
Open

3316891527 wants to merge 1 commit into
AAswordman:devfrom
3316891527:fix/skill-repo-zip-cache-stale

Conversation

@3316891527

Copy link
Copy Markdown
Contributor

变更说明 / Description

背景与动机 / Context

从 GitHub 市场或仓库 URL 安装技能时,ZIP 缓存键之前是 owner/repo@branch。作者更新仓库但不改分支名时,本地仍会直接复用旧 ZIP。删除技能只清已安装目录,不清缓存池,因此删了再下仍会装回旧包。

改动内容 / Changes

  1. 缓存键改为 commit SHA:导入前查 GitHub commit SHA,ZIP 复用键为 owner/repo@sha;
  2. 查不到 SHA 时强制重下:避免继续命中旧的 owner/repo@main 缓存;
  3. 删除时清 ZIP:GitHub 导入的技能会写 .operit/github_zip_key sidecar,删除技能时 invalidate 对应缓存。

验证方式与结果 / Verification

  • 新增 SkillRepoZipPoolManagerTest:缓存命中、强制刷新、invalidate 后重下。

关联 Issue / Related Issues

无

检查清单 / Checklist

  • 代码符合项目规范
  • GitHub 技能删除后再安装不再复用旧 ZIP

Reuse ZIP files only when the resolved commit SHA still matches. Missing SHA lookups force a fresh download, and deleting a GitHub-imported skill removes its pooled zip so reinstall no longer restores a stale package.
@CATMIAOZHI
CATMIAOZHI self-requested a review September 25, 2026 07:19

@CATMIAOZHI CATMIAOZHI left a comment

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.

审计结论:✅ 批准合并

改动(3 文件,+201/−7):GitHub 技能 ZIP 缓存键从 owner/repo@branch 改为 owner/repo@sha;SHA 查不到时强制重下;删除技能时通过 .operit/github_zip_key sidecar 精确清理对应缓存;新增单测。

通过的检查:

  • 目标分支 dev ✓;描述与 diff 一致
  • 缓存键正确性:commit SHA 不可变,owner/repo@sha 从根本上解决"分支名不变、内容更新却命中旧包"的问题
  • 失效完备性:安装时若解析出新 SHA 且与 ref 不同,会主动 invalidate 旧的 owner/repo@ref 条目;删除技能时读 sidecar 精确失效;安装成功后写 sidecar(runCatching 包裹)
  • 并发:同 key 的下载/失效受 per-key Mutex 互斥;下载走 part 文件 + rename/copy 原子替换,失败路径会清理 part 文件
  • 安全:sidecar 内容只经 SHA-256 生成缓存文件名(repo_<hex>.zip),无路径穿越或任意文件删除风险;owner/repo 拼 URL 时经 encodePathSegment 编码
  • deleteSkill 改为 suspend 后,唯一调用方 SkillConfigScreen 在 scope.launch 内调用,无破坏
  • 附带修复:getGithubDefaultBranch 补上了 User-Agent 请求头(GitHub API 会拒绝无 UA 的请求,之前默认分支查询实际会被 403)
  • getGithubCommitSha 的 catch (Exception) 包的是阻塞式 HttpURLConnection(Dispatchers.IO 内),无协程取消语义问题
  • 单测覆盖 poolKey 归一化、缓存命中、强制刷新、invalidate 后重下

问题:

  1. [P2] forceRefresh 先删缓存再下载:见行内评论。
  2. [P3] keyMutexes.getOrPut 非原子:见行内评论。
  3. [P3] poolKey 未归一化 owner/repo 大小写:GitHub 的 owner/repo 大小写不敏感,Owner/Repo@sha 与 owner/repo@sha 会成为两个缓存条目导致重复下载,建议 lowercase。
  4. [P3] 旧格式缓存只清理了当前 ref:owner/repo@ref 的旧条目只在本次安装的 ref 下被 invalidate,其他 ref(如 @master、@v1.0)的残留靠 LRU(maxPoolSize=6)淘汰,可接受,但首次升级后池内会短暂残留。
  5. [P3] 测试文件末尾缺换行符:SkillRepoZipPoolManagerTest.kt 以 "No newline at end of file" 结尾。
  6. [question] 含斜杠的分支名(如 tree/feature/foo):parseGitHubSkillTarget 只取 segments[3] 作 ref(既有解析限制);新逻辑下这类 URL 的 SHA 查询必然失败从而每次强制重下,确认是否为预期行为?

后续可跟进(不阻塞):无网络或被 GitHub 限流(未鉴权 60 次/小时)时每次安装都强制重下;可考虑下载失败时回退使用旧缓存并提示用户,而非直接报错。

由 水晴喵的muse 审计

return@withLock zipFile
}
if (forceRefresh && zipFile.exists()) {
AppLogger.d(TAG, "ZIP 强制刷新,丢弃缓存: key=$key, file=${zipFile.name}")

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.

[P2] forceRefresh 时先删除已有缓存再下载:若随后下载失败(网络抖动),缓存永久丢失,本可离线安装的旧包也装不了。普通路径是下载成功后才替换旧文件,建议这里也保持一致:先下载到 part,成功后再替换;或下载失败时保留旧缓存。


suspend fun invalidate(key: String) {
val dir = cacheDir ?: return
val mutex = keyMutexes.getOrPut(key) { Mutex() }

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.

[P3] keyMutexes.getOrPut 非原子:两线程首次同时为同一 key 创建 Mutex 时可能拿到不同实例,互斥被击穿。此处后果温和(重复下载/重复删除,都是幂等的),可顺手改为 computeIfAbsent。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants