Skip to content

feat(): 完成 skills 领域模型与本地注册表基础能力#269

Merged
pionxe merged 4 commits into
1024XEngineer:mainfrom
Cai-Tang-www:main
Apr 14, 2026
Merged

feat(): 完成 skills 领域模型与本地注册表基础能力#269
pionxe merged 4 commits into
1024XEngineer:mainfrom
Cai-Tang-www:main

Conversation

@Cai-Tang-www

@Cai-Tang-www Cai-Tang-www commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

背景

本 PR 对应 Skills 子任务落地,目标是在不改动主链路(TUI -> Runtime -> Provider -> Tools)的前提下,先完成 internal/skills 领域层的可用骨架:

  • 技能模型定义
  • 本地 SKILL.md 加载
  • 内存注册表查询
  • 可观测的问题聚合(issues)

关联父任务:#255
关联子任务:#257

本次改动

1) 新增 skills 领域模型与契约

  • 新增 SourceKind / ActivationScope / Descriptor / Content / Skill 等核心类型
  • 新增 SnapshotLoadIssueLoadIssueCode,用于“部分成功 + 问题聚合”模式
  • 新增 Loader / Registry 接口,明确后续扩展边界

2) 新增本地加载器 LocalLoader

  • 扫描本地技能目录,支持根目录和一级子目录 SKILL.md
  • 支持 Frontmatter 解析(YAML)与正文分段抽取(instruction/references/examples/toolhints)
  • 统一错误/问题输出:
    • 缺文件、读文件失败
    • metadata 非法
    • source/scope 非法
    • 内容为空

3) 新增内存注册表 MemoryRegistry

  • 支持 Refresh/List/Get
  • 支持 source/scope/workspace 过滤
  • 处理重复 ID、无效 ID,避免单点坏数据中断整体可用性
  • 增加 Issues() 暴露非致命问题,便于上层观测

4) 覆盖率与测试补齐

  • 新增 internal/skills/coverage_test.go,补齐 helper/边界/异常分支测试
  • internal/skills 覆盖率提升至 100%(statement)
  • 保持全量回归通过

验证结果

  • go test ./...
  • go test ./internal/skills -coverprofile skills.out -count=1
  • go tool cover -func skills.out 显示 total: 100.0%

风险与说明

  • 本次主要是 skills 领域与加载/注册表能力,不触达 runtime 注入逻辑,属于低侵入改动
  • LocalLoader 新增内部可测性钩子仅用于覆盖异常路径测试,不改变默认运行行为

Closes #257

@codecov

codecov Bot commented Apr 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.75655% with 28 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/skills/loader.go 92.49% 24 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

fennoai[bot]

This comment was marked as outdated.

@Cai-Tang-www

Copy link
Copy Markdown
Collaborator Author

/code 帮我检查现在代码还有没有你提到的风险,有就修复

@fennoai

fennoai Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

已按你提到的历史风险点完成复查并修复,改动已推送到当前 PR 头分支 Cai-Tang-www:main(commit: a23eb69)。

本次修复

  1. 修复 MemoryRegistry 首次加载失败后不再重试的问题
  • 文件:internal/skills/registry.go
  • 调整:Refreshloader.Load 失败时不再把 loaded 置为 true,避免 ensureLoaded() 被短路,后续 List/Get 可继续触发重试。
  1. 修复 frontmatter 结束标记必须 \n---\n 的兼容性问题
  • 文件:internal/skills/loader.go
  • 调整:splitFrontMatter 改为按行查找结束分隔符,支持 --- 作为 EOF 结束标记(末尾无换行也可)。
  1. 增加 SKILL.md 读取前的文件约束,降低可用性风险
  • 文件:internal/skills/loader.go
  • 调整:新增 Lstat 校验、仅允许 regular file、并增加默认大小上限(1MB),超限或类型异常统一以 IssueReadFailed 聚合。

测试补充

  • internal/skills/loader_test.go
    • TestLocalLoaderLoadFrontmatterEOF
    • TestLocalLoaderLoadFileConstraints
  • internal/skills/registry_test.go
    • TestMemoryRegistryEnsureLoadedRetriesAfterFailure

验证结果

  • go test ./internal/skills -count=1
  • go test ./... -count=1

未做内容:未扩展到 internal/skills 之外的架构改动(仅针对本次风险点做最小修复)。

View job run

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
@pionxe

pionxe commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

/review

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Focused review complete. I found 1 noteworthy issue in the new skills loader around file-size enforcement under concurrent filesystem changes.

Comment thread internal/skills/loader.go Outdated
continue
}

data, readErr := readFile(skillPath)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxFileBytes is enforced using Lstat().Size() before ReadFile(), but this is a TOCTOU window: the file can be swapped/grown between those two operations, and ReadFile will still load the full contents into memory. In adversarial or concurrent environments this bypasses the intended memory cap and can cause large allocations/DoS. Consider opening the file once and reading via io.LimitedReader (or equivalent bounded read) so the size limit is enforced at read time, not only at pre-check time.

@Cai-Tang-www

Copy link
Copy Markdown
Collaborator Author

/simplify

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the PR changes in internal/skills (code quality, performance, security, and docs consistency).

No noteworthy issues found in this diff.

Comment thread internal/skills/loader.go

var headingPattern = regexp.MustCompile(`^\s{0,3}#{1,6}\s+(.+?)\s*$`)

// LocalLoader scans one root directory and loads local skills.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed this change set end-to-end (including tests and edge paths). No actionable issues found.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已完成多维度审查并去重,保留 1 条需要关注的问题。

Comment thread internal/skills/loader.go
}
name := strings.TrimSpace(meta.Name)
if name == "" {
name = firstHeading(body)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name 的回退逻辑直接取 firstHeading(body),会把结构化分段标题(例如仅有 ## References)当作技能名。当前测试也固定了该行为,导致无显式 name 且无标题时会生成语义错误的名称(如 references)。建议仅将一级标题(如 # Title)用于名称回退,或在找不到合适标题时直接回退到 id

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one correctness/observability issue in the loader path handling. See inline note for details.

Comment thread internal/skills/loader.go Outdated

candidates := make([]string, 0, len(entries)+1)
rootSkillFile := filepath.Join(absRoot, skillFileName)
if _, err := statPath(rootSkillFile); err == nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

statPath(rootSkillFile) errors other than nil are silently ignored, so a root-level SKILL.md that exists but is unreadable (e.g. permission error / transient IO error) disappears without any LoadIssue. This breaks the "partial success + issue aggregation" contract and makes debugging difficult.

Please handle non-NotExist errors explicitly (append IssueReadFailed for rootSkillFile) instead of treating all errors as "no root skill".

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Focused review completed on internal/skills changes. I found 2 actionable issues (deduplicated across quality/perf/security/docs perspectives).

Comment thread internal/skills/loader.go Outdated

candidates := make([]string, 0, len(entries)+1)
rootSkillFile := filepath.Join(absRoot, skillFileName)
if _, err := statPath(rootSkillFile); err == nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

statPath(rootSkillFile) errors are silently ignored unless nil. That drops permission/IO failures for root SKILL.md and makes loader issues non-observable. Consider handling non-NotExist errors by appending a LoadIssue (IssueReadFailed) instead of skipping root candidate detection entirely.

Comment thread internal/skills/loader.go Outdated
continue
}

data, readErr := readFile(skillPath)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Size is checked via Lstat before ReadFile, but the file can change between those operations (TOCTOU). A replaced/grown file can bypass maxFileBytes and trigger unbounded memory read. Consider opening the file once, validating size from the opened descriptor (or reading through io.LimitReader) before/while reading.

@fennoai

fennoai Bot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

结果

已按触发评论完成处理:先检查历史评论并修复可执行问题,再做 simplify,最后提交并推送到当前 PR 头分支。

已完成内容

  • 修复 root/SKILL.md 的可观测性问题:statNotExist 错误不再静默,统一聚合为 IssueReadFailed
  • 修复文件大小限制 TOCTOU 风险:读取改为受限读(io.LimitReader),在读取阶段强制 maxFileBytes
  • 修正名称回退语义:name 回退仅使用一级标题 #,否则回退到 id,避免把 ## References 之类结构标题当技能名。
  • 使用子 agent 执行 simplify 审查,并采纳简化:提取测试辅助断言 requireLoadIssue,减少重复循环断言。

变更文件

  • internal/skills/loader.go
  • internal/skills/loader_test.go
  • internal/skills/coverage_test.go

验证

  • go test ./internal/skills -count=1
  • go test ./... -count=1

提交与推送

View job run

- aggregate root SKILL.md stat errors as non-fatal issues
- enforce read-time size limit with bounded file reader
- restrict name fallback to level-1 heading or id
- add regression tests and simplify issue assertions

Generated with [codeagent](https://github.com/qbox/codeagent)
Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
@pionxe pionxe merged commit c14c5a0 into 1024XEngineer:main Apr 14, 2026
2 checks passed
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.

feat(skills): 建立 Skills Registry / Loader / Source Model

3 participants