Skip to content

feat(multi-review): support custom reviewer personas from target repo#214

Merged
Svtter merged 1 commit into
mainfrom
feature/custom-reviewer-personas
Jun 7, 2026
Merged

feat(multi-review): support custom reviewer personas from target repo#214
Svtter merged 1 commit into
mainfrom
feature/custom-reviewer-personas

Conversation

@Svtter
Copy link
Copy Markdown
Collaborator

@Svtter Svtter commented Jun 7, 2026

Summary

Allows target repositories to define custom reviewer personas by placing YAML files in .github/reviewers/. This enables project-specific review configurations without modifying the action itself.

How it works

  1. Place .yaml or .yml files in your repo's .github/reviewers/ directory
  2. Each file must have name and prompt fields
  3. Reference custom personas in default-team just like built-in ones
  4. Custom personas with the same name as a built-in override the built-in

Example:

# .github/reviewers/accessibility.yaml
name: accessibility
prompt: |
  Review this PR for accessibility issues...
# workflow
default-team: "accessibility:1,quality:1,security:1"

Changes

  • multi-review/src/reviewers.ts: Add loadCustomReviewers() to scan .github/reviewers/, merge into loadReviewers()
  • multi-review/src/index.ts: Pass GITHUB_WORKSPACE as repoDir for custom persona resolution
  • multi-review/src/custom-reviewers.test.ts: 7 new tests covering custom loading, override, skip-missing-dir, invalid-YAML handling, .yml extension, and built-in+custom mixing
  • multi-review/README.md: Document custom personas feature
  • multi-review/package.json: Add custom-reviewers.test.ts to test script

Safety

  • permission: { edit: "deny", bash: "deny" } already prevents code execution from any custom prompt
  • Missing .github/reviewers/ produces no error (fully opt-in)
  • Invalid YAML / missing fields are skipped with warnings

Closes #213

Load custom reviewer YAML files from the target repository's
.github/reviewers/ directory. Custom personas can extend or override
built-in ones (quality, security, performance, architecture,
regression-test).

Key changes:
- Add loadCustomReviewers() in reviewers.ts to scan .github/reviewers/*.yaml|*.yml
- Merge custom personas into loadReviewers() with override-on-collision
- Pass GITHUB_WORKSPACE as repoDir in index.ts
- Add 7 unit tests in custom-reviewers.test.ts
- Update README with custom personas documentation

Closes #213
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 7, 2026

无需回归测试

检测到的 PR 类型:新功能

该 PR 新增了自定义 reviewer 角色(persona)支持,属于纯新增功能,未修改现有行为。内置 reviewer 的加载逻辑未发生变化,仅在此基础上增加了从 .github/reviewers/ 加载自定义配置的能力。PR 已包含完整测试(custom-reviewers.test.ts,7 个测试用例),覆盖自定义加载、覆盖内置、目录缺失、YAML 格式错误、.yml 扩展名以及混合使用等场景。

New%20session%20-%202026-06-07T10%3A03%3A16.098Z
opencode session  |  github run

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 7, 2026

最终决策:有条件合并 / CONDITIONAL MERGE

Quality reviewer 发现一个阻塞性测试隔离问题:测试 "silently skips when .github/reviewers/ does not exist" 因前面的测试创建了目标目录且未清理,实际并未验证目录缺失时的行为。其余 reviewer 均无阻塞项。

阻塞项

  1. 测试隔离失效:test "silently skips when .github/reviewers/ does not exist" 未在独立环境下运行,前面 test 留下的 .github/reviewers/ 目录使之无法真正测试"目录不存在"的路径。建议使用 beforeEach/afterEach 清理临时目录。

建议项

  1. 环境变量污染:测试直接修改 process.env 但未在每个 test 间恢复(quality)
  2. 符号链接遍历风险loadCustomReviewers 读取 YAML 前未检查文件是否为常规文件,仓库内符号链接可读取外部文件(security)
  3. YAML 安全 schema:使用 yaml.load(完整 schema)而非 DEFAULT_SAFE_SCHEMA,可能意外读取 !!binary 等类型(security)
  4. 名称字段校验缺失name 字段未做格式限制,若在模板插值场景中存在提示注入风险(security)
  5. 类型安全yaml.load(raw) as PersonaYAMLnull 返回时外层 try/catch 虽能兜底,但类型安全性可改善(quality)
  6. 清理时机after() 中只运行一次的清理若遇 test 提前退出,残留文件可能影响后续 test(quality)

📋 各 Reviewer 详细审查结果
quality

有条件合并 / CONDITIONAL MERGE

此 PR 总体设计合理,实现清晰,向后兼容性好。但存在一个测试隔离问题需要修复。

阻塞项

  1. 测试 "silently skips when .github/reviewers/ does not exist" 并未真正测试"目录不存在"的场景——因为前面的测试用例(test 1、test 2)创建了 .github/reviewers/ 目录及其文件,各 test 之间没有清理,导致该目录始终存在。测试虽然能通过(因为 team 设为 quality:1 恰好匹配到历史遗留的自定义 quality.yaml),但测试的是目录存在且有文件时的行为,失去了对"目录缺失时静默跳过"这一逻辑的覆盖验证。建议使用 beforeEach/afterEach 清理临时目录,或至少在该 test 开始时显式删除并重建 tmpDir

建议项

  1. 测试中直接修改 process.env 但没有在 test/test 之间恢复(仅在 after() 全局恢复一次),虽然由于每个 test 都会重新赋值 MULTI_REVIEW_DEFAULT_TEAM 不会产生实际错误,但属于脆弱模式。建议改用 beforeEach 保存/恢复环境变量,或使用类似 setupEnv 的帮助函数来隔离。

  2. src/reviewers.ts:67yaml.load(raw) as PersonaYAML 的类型断言若 yaml.load 返回 null(空文件)或非对象类型时,parsed.name 访问会触发 TypeError。当前已被外围 try/catch 捕获转为 console.warn,不会崩溃,但类型安全性可进一步改善。

  3. 测试文件第 26 行 rmSync(tmpDir, ...) 放在 after() 中只运行一次,若某个 test 执行失败提前退出,残留文件可能影响后续 test。建议改为在每个 test 开始前(beforeEach)清理 tmpDir

  4. 可选:custom-reviewers.test.ts 开头没有 import { loadCustomReviewers } from "./reviewers.js",它直接通过 loadReviewers 间接测试 loadCustomReviewers。功能上是够的,但如果未来需要单独测试 loadCustomReviewers 的边界条件,建议添加直接针对该函数的单元测试。

security

存在风险 / AT RISK

安全分析摘要
该 PR 通过从仓库的 .github/reviewers/ 目录加载 YAML 文件,增加了自定义审阅人角色的功能。代码具有良好的错误处理结构,使用了标准的 js-yaml 库,并默认跳过 fork 的 PR。但仍存在一些安全注意事项。

阻塞项:无

建议项

  1. 符号链接遍历风险
    loadCustomReviewersreaddirSync 之后直接对每个 .yaml/.yml 文件调用 readFileSync,但未检查文件是否为常规文件(非符号链接)。恶意角色可以利用仓库中的符号链接(例如 .github/reviewers/mal.yaml → /etc/passwd)读取工作空间之外的文件。虽然 fork PR 默认被跳过,但具有写权限的协作者仍可能利用此漏洞。
    修复建议:在 readFileSync 之前添加 lstatSync(dir_entry).isFile() 检查,跳过符号链接。

  2. YAML 解析器选择
    代码使用 yaml.load(raw)(完整默认 schema),而用户提供的 YAML 文件建议使用 yaml.load(raw, { schema: yaml.DEFAULT_SAFE_SCHEMA })。当前 schema 下代码执行风险较低,但如果 YAML 文件包含 !!binary 等类型,可能读取预期之外的二进制内容。
    修复建议:使用受限 schema 进行解析。

  3. 名称字段缺乏校验
    name 字段从 YAML 中提取后直接用于审阅人匹配,如果在系统提示模板中有插值场景,可能被用于提示注入。
    修复建议:添加正则校验,限制 name 仅允许字母、数字、连字符和下划线。

performance

性能良好 / GOOD

本次 PR 添加了从仓库 .github/reviewers/ 目录加载自定义 reviewer 配置的功能,性能分析如下:

  • loadCustomReviewers 使用同步 readdirSync + readFileSync 读取少量 YAML 文件,与已有 loadBuiltInReviewers 模式一致。该函数仅在 action 启动时调用一次,文件数量预期极少(1-5 个),对启动时间影响可忽略。
  • 没有数据库查询、循环引用、递归调用或高复杂度算法。
  • 每个自定义 persona 仅包含 nameprompt 两个字符串字段,内存占用极低。
  • 没有引入并发或线程问题。
  • 没有网络 I/O 或资源泄漏风险。

阻塞项:无

建议项:无

architecture

架构合理 / SOUND

架构分析摘要

本次 PR 为 multi-review 引入了自定义 reviewer persona 的支持,整体架构设计与现有代码风格一致,改动集中且合理。

耦合度: loadReviewers 新增 repoDir 参数,引入对仓库目录结构的依赖,但这是功能本身的内在需求,耦合程度合理。

模块放置: loadCustomReviewers 函数放置在 reviewers.ts 中,与 loadBuiltInReviewers 并列,保持了 reviewer 加载逻辑的内聚性。

分层: 自定义 reviewer 的 YAML 解析逻辑直接嵌入在 loadCustomReviewers 中,与 loadBuiltInReviewers 的处理方式一致,没有引入额外的抽象层,符合现有模式。

接口设计: loadReviewers opts 接口新增可选 repoDir 字段,是对现有接口的合理扩展,向后兼容。

改动范围: 改动集中在 reviewers.ts(核心逻辑)、index.ts(调用入口)、新增测试文件、README 文档和 build artifact。没有 shotgun surgery 问题。

一致性: 自定义 reviewer 的加载、解析、warning 输出、Map 管理方式均与内置 reviewer 加载逻辑一致。覆盖(override)行为的日志清晰。

阻塞项:无

建议项:无

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 7, 2026

分析总结

PR 实现了标题和描述中声明的所有功能,没有发现遗漏。

对照验证:

  1. loadCustomReviewers() 函数已添加至 multi-review/src/reviewers.ts:41-65,扫描 .github/reviewers/ 并合并到 loadReviewers()
  2. multi-review/src/index.ts:104-105 已将 GITHUB_WORKSPACE 作为 repoDir 传入
  3. ✅ 7 个测试覆盖了:自定义加载、同名覆盖、目录不存在时静默跳过、无效YAML带警告、缺失字段带警告、.yml 扩展名、内置+自定义混合 — 全部在 custom-reviewers.test.ts:27-114
  4. ✅ README.md 新增了 "Custom reviewer personas" 章节(第28-42行)
  5. package.json 测试脚本已包含 custom-reviewers.test.ts
  6. dist/index.cjs 正确包含 loadCustomReviewers 调用链(第4674-4727行)
  7. ✅ 所有规范中描述的行为均已实现:目录不存在无报错、同名覆盖、字段/文件格式校验、警告输出

无遗漏

New%20session%20-%202026-06-07T10%3A03%3A20.406Z
opencode session  |  github run

@Svitter Svitter added agent OpenCode agent SDK integration feat New feature or enhancement review:p1 Major review findings triaged Issue has been triaged labels Jun 7, 2026
@Svtter Svtter merged commit aa13370 into main Jun 7, 2026
5 checks passed
@Svitter
Copy link
Copy Markdown
Contributor

Svitter commented Jun 7, 2026

Rank: P1 — mergeable after minor fixes. Clean feature with good test coverage.

Adds custom reviewer personas from .github/reviewers/ in the target repo — solid opt-in extension. Production code in reviewers.ts and index.ts looks correct: directory-read failure silently returns empty map, invalid/missing-field YAML skipped with warnings, custom personas override built-ins by name.

Findings

Test pollution (custom-reviewers.test.ts:58) — should-fix: The "silently skips when .github/reviewers/ does not exist" test runs after prior tests have already created that directory, so it never actually tests the absent-directory case. The assertion only checks reviewers[0].name === "quality" (no prompt content check), so the test passes either way. Add per-test cleanup (e.g. rmSync(tmpDir, { recursive: true, force: true }) in a per-test before) or add a prompt content assertion like the other tests.

Missing CHANGELOG — should-fix: This feature addition needs an entry under [Unreleased]### Added in the repo root CHANGELOG.md.

@Svtter — thanks for the PR. Both fixes are quick; the feature itself is well-structured and follows existing patterns.

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

Labels

agent OpenCode agent SDK integration feat New feature or enhancement review:p1 Major review findings triaged Issue has been triaged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(multi-review): support custom reviewer personas from target repo

2 participants