From e970408acd8c6ae08a7bcd1381b545f3bb3a2edf Mon Sep 17 00:00:00 2001 From: Turtle Date: Mon, 7 Sep 2026 19:25:05 +0800 Subject: [PATCH] chore: automate static review ownership --- ...sted-changed-file-review-routing.i18n.yaml | 6 + ...-08-trusted-changed-file-review-routing.md | 47 +++ ...-trusted-changed-file-review-routing.zh.md | 47 +++ .github/review-ownership/CODEOWNERS | 60 ++++ .github/review-ownership/README.i18n.yaml | 6 + .github/review-ownership/README.md | 53 +++ .github/review-ownership/README.zh.md | 53 +++ .github/review-ownership/request-review.mjs | 290 +++++++++++++++ .../review-ownership/request-review.test.mjs | 335 ++++++++++++++++++ .github/workflows/request-review.yml | 32 ++ package.json | 1 + scripts/ci-workflow.spec.ts | 41 +++ scripts/run-gates.spec.ts | 9 + scripts/run-gates.ts | 2 + 14 files changed, 982 insertions(+) create mode 100644 .agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.i18n.yaml create mode 100644 .agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.md create mode 100644 .agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.zh.md create mode 100644 .github/review-ownership/CODEOWNERS create mode 100644 .github/review-ownership/README.i18n.yaml create mode 100644 .github/review-ownership/README.md create mode 100644 .github/review-ownership/README.zh.md create mode 100644 .github/review-ownership/request-review.mjs create mode 100644 .github/review-ownership/request-review.test.mjs create mode 100644 .github/workflows/request-review.yml diff --git a/.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.i18n.yaml b/.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.i18n.yaml new file mode 100644 index 0000000000..6c9579402e --- /dev/null +++ b/.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.i18n.yaml @@ -0,0 +1,6 @@ +# Bilingual-pair consistency record (docs/i18n/README.md): the git blob hash of each +# side as of the last confirmed-consistent state. Both languages carry equal authority; +# after editing either side, bring the other along and re-record with: +# pnpm run verify-translation-pairing --write .agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.md +2026-09-08-trusted-changed-file-review-routing.md: 246c223d976e312e5712d7baa150bd3e84f1363e +2026-09-08-trusted-changed-file-review-routing.zh.md: 397d4174340fc9c6002419b21436b5c33542f7e4 diff --git a/.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.md b/.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.md new file mode 100644 index 0000000000..246c223d97 --- /dev/null +++ b/.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.md @@ -0,0 +1,47 @@ +# Agent Note: Route reviews from trusted changed-file policy + +Status: implemented + +English | [中文](2026-09-08-trusted-changed-file-review-routing.zh.md) + +## Problem + +GitHub's native CODEOWNERS behavior requests reviewers whenever a matching path changes. It cannot apply this repository's distinction between reviewable implementation or documentation files and test-only evidence. A native CODEOWNERS file also makes GitHub, rather than an inspected repository program, responsible for the request decision. + +Review routing needs an observable changed-file input, explicit owner rules, complete test exclusions, and a write-capable workflow that remains safe for pull requests from forks. + +## Decision + +The repository keeps a CODEOWNERS-compatible map at [`.github/review-ownership/CODEOWNERS`](../../../../.github/review-ownership/CODEOWNERS), outside GitHub's native CODEOWNERS locations. The map accepts only explicit absolute directory patterns and individual GitHub users. It rejects wildcards, hidden-directory patterns, teams, duplicate patterns, and duplicate owners. Later matching patterns replace earlier matches. + +The policy test counts non-test tracked lines in directories that match an ownership rule. It rejects a map in which `@turtle1999` owns more than one third of that eligible owned codebase. + +The [`request-review` workflow](../../../../.github/workflows/request-review.yml) runs on non-draft `pull_request_target` events for opened, synchronized, reopened, and ready-for-review pull requests. Its write-capable job checks out the default branch and executes only the default branch's scanner and ownership map. It does not check out pull-request code or read repository secrets. + +The scanner fetches every changed-file record before deciding. It fails if the pull request reports more than GitHub's 3,000-file API limit or if pagination returns an incomplete list. It normalizes repository paths, evaluates old and new paths of a rename independently, and escapes filenames before logging them. + +The scanner excludes test-only paths before owner matching. Excluded paths comprise directories named `test`, `tests`, `__tests__`, `__snapshots__`, `benches`, or `stress-tests`; the top-level `benchmarks` and `snapshots` trees; `packages/test-support`; `scripts/fixtures` and `scripts/snapshots`; filenames ending in `.bench.`, `.corpus.`, `.e2e.`, `.perf.`, `.snapshot.`, `.spec.`, `.stress.`, or `.test.`; and Python `test_*.py`, `*_test.py`, or `*_tests.py` files. Test infrastructure such as `vitest*.config.ts` and gate implementations remains reviewable because it changes how repository evidence is produced. + +The workflow prints the changed non-test paths, excluded test paths, per-file owner matches, and final reviewer list before any review-request mutation. It requests the union of matched individual owners after removing the pull-request author and users who are already requested. A test-only or wholly unmatched change requests nobody. + +## Verification + +[Scanner tests](../../../../.github/review-ownership/request-review.test.mjs) cover admitted ownership syntax, rejected syntax, each test convention, production-name negative controls, renames, last-match behavior, unmatched files, complete pagination, the 3,000-file limit, log-before-request ordering, author and existing-reviewer filtering, test-only changes, drafts, and API failures. [Workflow tests](../../../../scripts/ci-workflow.spec.ts) pin the event set, least permissions, trusted default-branch checkout, absence of pull-request-head references and secrets, and executed command. The gate graph includes both suites in static CI and `check-all`. + +## Alternatives considered + +**Use native CODEOWNERS.** Native routing cannot ignore test-only changes and offers no repository-owned decision log before requesting reviewers. + +**Run under `pull_request` and check out the pull-request head.** A fork workflow does not receive a write-capable token, while granting a write token to code from an untrusted head is unsafe. + +**Execute the pull request's scanner or owner map under `pull_request_target`.** This lets an untrusted pull request choose its own write-capable behavior or owners. + +**Infer semantic source changes from patches or language parsers.** GitHub can truncate patches, and the repository spans TypeScript, JavaScript, Python, Rust, YAML, Markdown, and generated evidence. A cross-language semantic classifier would add ambiguous rules without providing a complete input. The scanner therefore uses the complete non-test changed-file list and does not claim to distinguish formatting, comments, or documentation-only edits inside an eligible file. + +## Consequences + +Reviewer requests are reproducible from a trusted policy and the file list printed in the workflow log. Test-only changes do not request owners. Ownership changes become effective only after merge, so the pull request that changes policy cannot apply its untrusted policy to itself. + +The workflow requests every matched owner rather than choosing one owner nondeterministically. Shared ownership on large directories therefore produces multiple requests. GitHub-generated review-request events may not start other workflows that depend on recursively triggered events from `GITHUB_TOKEN`; those workflows must not rely on this request as their only trigger. + +Any non-test change under an owned directory remains eligible, including comment-only or formatting-only edits and documentation changes. Unmatched paths are logged and request nobody. Pull requests above the API file limit fail without requesting a partial owner set. diff --git a/.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.zh.md b/.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.zh.md new file mode 100644 index 0000000000..397d417434 --- /dev/null +++ b/.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.zh.md @@ -0,0 +1,47 @@ +# Agent Note: 基于受信任的变更文件策略路由评审 + +Status: implemented + +[English](2026-09-08-trusted-changed-file-review-routing.md) | 中文 + +## 问题 + +只要匹配路径发生变更，GitHub 原生 CODEOWNERS 就会请求评审者。它无法应用本仓库对需评审的实现或文档文件与纯测试证据的区分。使用原生 CODEOWNERS 文件还会让 GitHub 负责请求决策，而不是由可检查的仓库程序负责。 + +评审路由需要可观测的变更文件输入、显式 owner 规则、完整的测试排除规则，以及对 fork PR 仍然安全且具备写权限的 workflow。 + +## 决策 + +仓库在 GitHub 原生 CODEOWNERS 路径之外的 [`.github/review-ownership/CODEOWNERS`](../../../../.github/review-ownership/CODEOWNERS) 中保存兼容 CODEOWNERS 格式的映射。该映射只接受显式绝对目录模式和 GitHub 个人用户。通配符、隐藏目录模式、团队、重复模式和重复 owner 都会被拒绝。靠后的匹配模式会替换靠前的匹配结果。 + +策略测试会统计匹配所有权规则的目录中的非测试跟踪文件行数。如果 `@turtle1999` 拥有的有效代码库超过三分之一，测试就会拒绝该映射。 + +[`request-review` workflow](../../../../.github/workflows/request-review.yml) 在非草稿 PR 的 `pull_request_target` 事件上运行，订阅创建、同步、重新打开和标记为可评审操作。具备写权限的 job 检出默认分支，只执行默认分支上的扫描器和所有权映射。它不会检出 PR 代码，也不会读取仓库 secret。 + +扫描器在决策之前获取所有变更文件记录。如果 PR 报告的文件数超过 GitHub API 的 3,000 个文件上限，或者分页只返回了部分列表，扫描器就会失败。它会规范化仓库路径，分别检查重命名前后的路径，并在记录文件名之前进行转义。 + +扫描器会在匹配 owner 之前排除纯测试路径。排除范围包括名为 `test`、`tests`、`__tests__`、`__snapshots__`、`benches` 或 `stress-tests` 的目录，顶层 `benchmarks` 和 `snapshots` 目录树，`packages/test-support`、`scripts/fixtures` 和 `scripts/snapshots`，以 `.bench.`、`.corpus.`、`.e2e.`、`.perf.`、`.snapshot.`、`.spec.`、`.stress.` 或 `.test.` 结尾的文件名，以及 Python 的 `test_*.py`、`*_test.py` 或 `*_tests.py` 文件。`vitest*.config.ts` 和门禁实现等测试基础设施仍需评审，因为它们会改变仓库证据的生成方式。 + +Workflow 会在发出任何评审请求变更之前，依次打印变更的非测试路径、排除的测试路径、逐文件 owner 匹配结果和最终评审者列表。它合并匹配到的个人 owner，并排除 PR 作者和已经收到评审请求的用户。纯测试变更或全部未匹配的变更不会请求任何人。 + +## 验证 + +[扫描器测试](../../../../.github/review-ownership/request-review.test.mjs)覆盖允许的所有权语法、拒绝的语法、每种测试约定、生产文件名负向对照、重命名、最后匹配规则、未匹配文件、完整分页、3,000 个文件上限、先记录后请求的顺序、作者与现有评审者过滤、纯测试变更、草稿和 API 失败。[Workflow 测试](../../../../scripts/ci-workflow.spec.ts)固定事件集合、最小权限、受信任的默认分支检出、不引用 PR head 和 secret，以及执行的命令。门禁图在静态 CI 和 `check-all` 中包含这两组测试。 + +## 考虑过的替代方案 + +**使用原生 CODEOWNERS。** 原生路由无法忽略纯测试变更，也无法在请求评审者之前提供由仓库控制的决策日志。 + +**在 `pull_request` 下运行并检出 PR head。** Fork workflow 无法获得具备写权限的 token，而向不受信任 head 中的代码授予写权限 token 并不安全。 + +**在 `pull_request_target` 下执行 PR 中的扫描器或 owner 映射。** 这会让不受信任的 PR 选择自己的写权限行为或 owner。 + +**根据补丁或语言解析器推断语义源码变更。** GitHub 可能截断补丁，而且仓库包含 TypeScript、JavaScript、Python、Rust、YAML、Markdown 和生成的证据。跨语言语义分类器会增加含义不明确的规则，却无法提供完整输入。因此，扫描器使用完整的非测试变更文件列表，并且不会声称能够区分合格文件中的纯格式、注释或仅文档编辑。 + +## 后果 + +评审请求可以根据受信任的策略和 workflow 日志中打印的文件列表复现。纯测试变更不会请求 owner。所有权变更只有合并后才会生效，因此修改策略的 PR 无法对自身应用其中不受信任的策略。 + +Workflow 会请求所有匹配的 owner，不会随机选择一人。因此，大目录上的共享所有权会产生多个请求。GitHub 使用 `GITHUB_TOKEN` 生成的评审请求事件可能不会启动依赖递归触发事件的其他 workflow；这些 workflow 不得把此请求作为唯一触发条件。 + +所有已分配目录下的非测试变更仍符合请求条件，其中包括纯注释、纯格式调整和文档变更。未匹配的路径会被记录，但不会请求任何人。超过 API 文件上限的 PR 会失败，并且不会请求不完整的 owner 集合。 diff --git a/.github/review-ownership/CODEOWNERS b/.github/review-ownership/CODEOWNERS new file mode 100644 index 0000000000..5b41121d64 --- /dev/null +++ b/.github/review-ownership/CODEOWNERS @@ -0,0 +1,60 @@ +# Custom static-scanner input. Its nested path keeps GitHub from loading it as +# the repository's native CODEOWNERS file. +/apps/cli/ @turtle1999 +/apps/web/ @imccyu +/docs/ @turtle1999 +/native/ @mektpoy +/patches/ @mektpoy +/python/ @LegGasai +/scripts/ @turtle1999 +/vendor/ @turtle1999 +/website/ @LegGasai +/packages/acp/ @mektpoy +/packages/api/ @imccyu +/packages/attachment/ @CreatixChu +/packages/boot/ @turtle1999 +/packages/bundle/ @turtle1999 +/packages/client/ @imccyu +/packages/code-runtime/ @Chinesezjc +/packages/compaction/ @imccyu +/packages/context/ @turtle1999 +/packages/core/ @tianyicui @turtle1999 @mektpoy +/packages/credentials/ @mektpoy +/packages/e2b/ @mektpoy +/packages/experimental/ @mektpoy +/packages/extensions/ @mektpoy +/packages/feedback/ @mektpoy +/packages/fs/ @mektpoy +/packages/goal/ @mektpoy +/packages/guard/ @turtle1999 +/packages/hooks/ @mektpoy +/packages/host/ @turtle1999 +/packages/identity/ @imccyu +/packages/interaction/ @imccyu +/packages/jobs/ @imccyu +/packages/llm/ @LegGasai +/packages/lsp/ @mektpoy +/packages/mcp/ @mektpoy +/packages/plan/ @mektpoy +/packages/preset/ @LegGasai @turtle1999 +/packages/runtime-diagnostics/ @mektpoy +/packages/sandbox/ @mektpoy +/packages/schedule/ @imccyu +/packages/sdk/ @mektpoy +/packages/session/ @tianyicui @turtle1999 @mektpoy +/packages/session-query/ @mektpoy +/packages/settings/ @mektpoy +/packages/shell/ @mektpoy +/packages/skill/ @mektpoy +/packages/spill/ @mektpoy +/packages/storage/ @imccyu +/packages/subagent/ @Dudu-0223 +/packages/subprocess/ @mektpoy +/packages/terminal/ @imccyu +/packages/todo/ @mektpoy +/packages/typert/ @imccyu +/packages/util/ @mektpoy +/packages/web/ @imccyu +/packages/webhook/ @mektpoy +/packages/workflow/ @mektpoy +/packages/workspace/ @imccyu diff --git a/.github/review-ownership/README.i18n.yaml b/.github/review-ownership/README.i18n.yaml new file mode 100644 index 0000000000..e9ed0b794d --- /dev/null +++ b/.github/review-ownership/README.i18n.yaml @@ -0,0 +1,6 @@ +# Bilingual-pair consistency record (docs/i18n/README.md): the git blob hash of each +# side as of the last confirmed-consistent state. Both languages carry equal authority; +# after editing either side, bring the other along and re-record with: +# pnpm run verify-translation-pairing --write .github/review-ownership/README.md +README.md: 95dd52595bacb9b2b00bc475966f328b06098a14 +README.zh.md: a20a5f4ae41b5d0418601afa853ac2fbc141cc96 diff --git a/.github/review-ownership/README.md b/.github/review-ownership/README.md new file mode 100644 index 0000000000..95dd52595b --- /dev/null +++ b/.github/review-ownership/README.md @@ -0,0 +1,53 @@ +# Automated review requests + +English | [中文](README.zh.md) + +## Summary + +The [`request-review` workflow](../workflows/request-review.yml) reads the CODEOWNERS-compatible [ownership map](CODEOWNERS) from the trusted default branch. It prints the complete changed non-test file list, matches those files to owners, and then requests the missing reviewers. The ownership map is outside GitHub's native CODEOWNERS locations, so GitHub does not apply it directly. + +## Table of Contents + +- [Routing](#routing) +- [Test exclusion](#test-exclusion) +- [Security](#security) +- [Verification](#verification) +- [Dev Note](#dev-note) + + + +## Routing + +Non-draft pull requests run the workflow when opened, synchronized, reopened, or marked ready for review. The scanner fetches the complete pull-request file list, evaluates both paths of a rename, and fails instead of routing from a partial list. GitHub exposes at most 3,000 files for this API. + +The ownership map accepts explicit absolute directory patterns and individual GitHub users. It rejects wildcards, hidden-directory patterns, teams, and duplicate patterns or owners. Matching follows CODEOWNERS last-match semantics. The scanner prints `Changed code files`, `Excluded test files`, `Owners by changed file`, and `Reviewers to request` before it sends the review request. Unmatched files remain visible in the log. The pull-request author and users who are already requested are omitted. + +The policy test measures non-test tracked lines under matched directories and requires `@turtle1999` to own no more than one third of that eligible owned codebase. + + + +## Test exclusion + +Review routing excludes the repository's unit, end-to-end, expected-output, snapshot, benchmark, performance, stress, corpus, native, and Python test conventions. This includes `test`, `tests`, `__tests__`, `__snapshots__`, `benches`, and `stress-tests` directories; the top-level `benchmarks` and `snapshots` trees; `packages/test-support`; `scripts/fixtures` and `scripts/snapshots`; recognized test filename suffixes; and Python `test_*.py` or `*_test.py` files. + +Test infrastructure that can alter how evidence is produced remains reviewable, including `vitest*.config.ts` and gate implementations under `scripts`. A production file named `test.ts`, `spec.ts`, or `snapshot.ts` is not excluded solely by that name. + + + +## Security + +The write-capable `pull_request_target` job checks out only the repository default branch. It does not check out or execute pull-request code and does not use repository secrets. Pull-request filenames are treated as API data and escaped in logs. + +Ownership changes take effect only after they merge into the default branch. This prevents an untrusted pull request from changing the routing program or its owner assignments for its own run. + + + +## Verification + +Run `pnpm run test:request-review` for ownership parsing, test classification, pagination, logging order, reviewer filtering, and API behavior. [Workflow tests](../../scripts/ci-workflow.spec.ts) pin the trusted checkout, permissions, events, and command. The repository gate graph runs both checks in CI. + + + +## Dev Note + +The [review-routing decision](../../.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.md) records the security model, test exclusions, and alternatives. diff --git a/.github/review-ownership/README.zh.md b/.github/review-ownership/README.zh.md new file mode 100644 index 0000000000..a20a5f4ae4 --- /dev/null +++ b/.github/review-ownership/README.zh.md @@ -0,0 +1,53 @@ +# 自动请求代码评审 + +[English](README.md) | 中文 + +## 概要 + +[`request-review` workflow](../workflows/request-review.yml) 从受信任的默认分支读取兼容 CODEOWNERS 格式的[所有权映射](CODEOWNERS)。它先打印完整的非测试变更文件列表，再将这些文件与 owner 匹配，最后请求尚未加入的评审者。所有权映射不在 GitHub 原生 CODEOWNERS 路径中，因此 GitHub 不会直接应用它。 + +## 目录 + +- [路由](#routing) +- [排除测试](#test-exclusion) +- [安全性](#security) +- [验证](#verification) +- [开发说明](#dev-note) + + + +## 路由 + +非草稿 PR 在创建、同步、重新打开或标记为可评审时运行该 workflow。扫描器获取完整的 PR 文件列表，分别检查重命名前后的路径；如果只能取得部分列表，则停止执行，不发出评审请求。GitHub 对此 API 最多公开 3,000 个文件。 + +所有权映射只接受显式绝对目录模式和 GitHub 个人用户。通配符、隐藏目录模式、团队，以及重复的模式或 owner 都会被拒绝。匹配遵循 CODEOWNERS 的最后一条匹配规则。扫描器先打印 `Changed code files`、`Excluded test files`、`Owners by changed file` 和 `Reviewers to request`，再发送评审请求。未匹配的文件仍显示在日志中。PR 作者和已经收到评审请求的用户会被排除。 + +策略测试会统计已匹配目录下的非测试跟踪文件行数，并要求 `@turtle1999` 拥有的有效代码库不超过三分之一。 + + + +## 排除测试 + +评审路由会排除仓库中的单元测试、端到端测试、预期输出、快照、基准测试、性能测试、压力测试、语料测试、原生测试和 Python 测试约定。其中包括 `test`、`tests`、`__tests__`、`__snapshots__`、`benches` 和 `stress-tests` 目录，顶层 `benchmarks` 和 `snapshots` 目录树，`packages/test-support`、`scripts/fixtures` 和 `scripts/snapshots`，可识别的测试文件名后缀，以及 Python 的 `test_*.py` 或 `*_test.py` 文件。 + +能够改变证据生成方式的测试基础设施仍需评审，包括 `vitest*.config.ts` 和 `scripts` 下的门禁实现。生产文件不会仅因名称为 `test.ts`、`spec.ts` 或 `snapshot.ts` 而被排除。 + + + +## 安全性 + +具备写权限的 `pull_request_target` job 只检出仓库默认分支。它不会检出或执行 PR 代码，也不使用仓库 secret。PR 文件名仅作为 API 数据处理，并在日志中转义。 + +所有权变更只有合并到默认分支后才会生效。这可以防止不受信任的 PR 为自身的 workflow 运行修改路由程序或 owner 分配。 + + + +## 验证 + +运行 `pnpm run test:request-review` 可检查所有权解析、测试分类、分页、日志顺序、评审者过滤和 API 行为。[Workflow 测试](../../scripts/ci-workflow.spec.ts)固定受信任检出、权限、事件和命令。仓库门禁图会在 CI 中运行这两类检查。 + + + +## 开发说明 + +[评审路由决策](../../.agents/notes/implemented/process/2026-09-08-trusted-changed-file-review-routing.zh.md)记录了安全模型、测试排除规则和备选方案。 diff --git a/.github/review-ownership/request-review.mjs b/.github/review-ownership/request-review.mjs new file mode 100644 index 0000000000..5d71108abe --- /dev/null +++ b/.github/review-ownership/request-review.mjs @@ -0,0 +1,290 @@ +#!/usr/bin/env node + +import { readFileSync } from 'node:fs' +import process from 'node:process' +import { pathToFileURL } from 'node:url' + +const API_VERSION = '2026-03-10' +const MAX_PULL_REQUEST_FILES = 3_000 +const PAGE_SIZE = 100 +const TEST_DIRECTORY_NAMES = new Set(['__snapshots__', '__tests__', 'benches', 'stress-tests', 'test', 'tests']) +const TEST_FILE_MARKER = /\.(?:bench|corpus|e2e|perf|snapshot|spec|stress|test)\.[^./]+$/u +const PYTHON_TEST_FILE = /^(?:test_.+|.+_tests?)\.py$/u + +/** + * Parse the explicit directory subset accepted from the review ownership file. + * @param {string} source CODEOWNERS-compatible source text. + * @returns {Array<{pattern: string, prefix: string, owners: string[]}>} Ordered ownership rules. + */ +export function parseOwnership(source) { + const rules = [] + const patterns = new Set() + for (const [index, rawLine] of source.split('\n').entries()) { + const line = rawLine.trim() + if (!line || line.startsWith('#')) continue + const [pattern, ...owners] = line.split(/\s+/u) + const location = `ownership line ${index + 1}` + if (!/^\/[^*?[\]#!\\]+\/$/u.test(pattern)) { + throw new Error(`${location}: expected one explicit absolute directory pattern`) + } + if (pattern.startsWith('/.')) throw new Error(`${location}: hidden-directory patterns are not allowed`) + if (patterns.has(pattern)) throw new Error(`${location}: duplicate pattern ${JSON.stringify(pattern)}`) + if (owners.length === 0) throw new Error(`${location}: expected at least one owner`) + const normalizedOwners = [] + const seenOwners = new Set() + for (const owner of owners) { + if (!/^@[A-Za-z0-9-]+$/u.test(owner)) { + throw new Error(`${location}: only individual GitHub users are supported`) + } + const key = owner.toLowerCase() + if (seenOwners.has(key)) throw new Error(`${location}: duplicate owner ${owner}`) + seenOwners.add(key) + normalizedOwners.push(owner) + } + patterns.add(pattern) + rules.push({ pattern, prefix: pattern.slice(1), owners: normalizedOwners }) + } + if (rules.length === 0) throw new Error('ownership file contains no rules') + return rules +} + +/** + * Normalize a repository-relative path received from GitHub. + * @param {unknown} value GitHub file path. + * @returns {string} Slash-normalized repository path. + */ +export function normalizeRepositoryPath(value) { + if (typeof value !== 'string' || value.length === 0) throw new Error('changed file has no path') + const normalized = value.replaceAll('\\', '/').replace(/^\.\/+/, '') + if ( + normalized.startsWith('/') + || normalized.includes('\0') + || normalized.split('/').some(segment => !segment || segment === '.' || segment === '..') + ) { + throw new Error(`invalid repository path ${JSON.stringify(value)}`) + } + return normalized +} + +/** + * Decide whether a repository path belongs only to test evidence or test support. + * @param {string} value Repository-relative path. + * @returns {boolean} Whether reviewer routing must ignore the path. + */ +export function isTestPath(value) { + const file = normalizeRepositoryPath(value) + const segments = file.split('/') + if (segments[0] === 'benchmarks' || segments[0] === 'snapshots') return true + if (segments[0] === 'packages' && segments[1] === 'test-support') return true + if (segments[0] === 'scripts' && (segments[1] === 'fixtures' || segments[1] === 'snapshots')) return true + if (segments.some(segment => TEST_DIRECTORY_NAMES.has(segment))) return true + const basename = segments.at(-1) ?? '' + return TEST_FILE_MARKER.test(basename) || PYTHON_TEST_FILE.test(basename) +} + +/** + * Expand changed-file records into reviewable and excluded repository paths. + * @param {unknown[]} files Pull-request file records from GitHub. + * @returns {{changedCodeFiles: string[], excludedTestFiles: string[]}} Classified paths. + */ +export function classifyChangedFiles(files) { + const changedCodeFiles = new Set() + const excludedTestFiles = new Set() + for (const entry of files) { + if (!isRecord(entry)) throw new Error('changed-file response contains a non-object entry') + const paths = [normalizeRepositoryPath(entry.filename)] + if (entry.previous_filename !== undefined) { + paths.unshift(normalizeRepositoryPath(entry.previous_filename)) + } + for (const file of paths) { + if (isTestPath(file)) excludedTestFiles.add(file) + else changedCodeFiles.add(file) + } + } + return { + changedCodeFiles: [...changedCodeFiles].sort(), + excludedTestFiles: [...excludedTestFiles].sort(), + } +} + +/** + * Match changed paths to owners with CODEOWNERS last-match semantics. + * @param {Array<{prefix: string, owners: string[]}>} rules Ordered ownership rules. + * @param {string[]} changedCodeFiles Reviewable repository paths. + * @returns {{matches: Array<{file: string, owners: string[]}>, reviewers: string[]}} Routing plan. + */ +export function planReviewers(rules, changedCodeFiles) { + const matches = [] + const reviewers = new Map() + for (const file of changedCodeFiles) { + let owners = [] + for (const rule of rules) { + if (file.startsWith(rule.prefix)) owners = rule.owners + } + matches.push({ file, owners }) + for (const owner of owners) reviewers.set(owner.toLowerCase(), owner.slice(1)) + } + return { + matches, + reviewers: [...reviewers.values()].sort((left, right) => left.localeCompare(right, 'en')), + } +} + +/** + * Create a repository-scoped GitHub JSON API caller. + * @param {{token: string, apiUrl?: string, fetchImpl?: typeof fetch}} options API dependencies. + * @returns {(path: string, options?: {method?: string, body?: unknown}) => Promise} API caller. + */ +export function createGitHubApi({ token, apiUrl = 'https://api.github.com', fetchImpl = globalThis.fetch }) { + if (!token) throw new Error('GITHUB_TOKEN is not set') + if (typeof fetchImpl !== 'function') throw new Error('fetch is unavailable') + const root = apiUrl.replace(/\/+$/u, '') + return async (path, { method = 'GET', body } = {}) => { + const response = await fetchImpl(`${root}${path}`, { + method, + headers: { + Accept: 'application/vnd.github+json', + Authorization: `Bearer ${token}`, + 'Content-Type': 'application/json', + 'User-Agent': 'deepseek-harness-request-review', + 'X-GitHub-Api-Version': API_VERSION, + }, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + }) + if (!response.ok) { + const responseBody = await response.text() + throw new Error(`GitHub API ${method} ${path} returned ${response.status}: ${JSON.stringify(responseBody)}`) + } + if (response.status === 204) return undefined + return response.json() + } +} + +/** + * Fetch the complete pull-request file list or fail before routing a partial list. + * @param {(path: string, options?: {method?: string, body?: unknown}) => Promise} api GitHub API caller. + * @param {string} repository Owner/name repository identifier. + * @param {number} pullNumber Pull-request number. + * @param {number} expectedCount Pull-request changed-file count. + * @returns {Promise} Complete changed-file records. + */ +export async function listPullRequestFiles(api, repository, pullNumber, expectedCount) { + if (!Number.isSafeInteger(expectedCount) || expectedCount < 0) { + throw new Error('pull request changed_files must be a non-negative integer') + } + if (expectedCount > MAX_PULL_REQUEST_FILES) { + throw new Error(`pull request has ${expectedCount} files; GitHub exposes at most ${MAX_PULL_REQUEST_FILES}`) + } + const files = [] + for (let page = 1; files.length < expectedCount; page++) { + const response = await api(`/repos/${repository}/pulls/${pullNumber}/files?per_page=${PAGE_SIZE}&page=${page}`) + if (!Array.isArray(response) || response.length === 0) { + throw new Error(`GitHub returned ${files.length} of ${expectedCount} changed files`) + } + files.push(...response) + if (files.length > expectedCount) { + throw new Error(`GitHub returned ${files.length} files but the pull request reports ${expectedCount}`) + } + } + return files +} + +/** + * Print changed paths, route owners, and request every missing eligible reviewer. + * @param {{event: unknown, ownershipSource: string, api: (path: string, options?: {method?: string, body?: unknown}) => Promise, write?: (line: string) => void}} options Runtime inputs. + * @returns {Promise<{changedCodeFiles: string[], excludedTestFiles: string[], requestedReviewers: string[]}>} Applied routing result. + */ +export async function requestReviews({ event, ownershipSource, api, write = line => process.stdout.write(`${line}\n`) }) { + const pull = pullRequestFromEvent(event) + write('This is by automated Angry Turtle Cyborg, not a human') + if (pull.draft) { + write('Draft pull request; reviewer routing is deferred until ready_for_review.') + return { changedCodeFiles: [], excludedTestFiles: [], requestedReviewers: [] } + } + + const files = await listPullRequestFiles(api, pull.repository, pull.number, pull.changedFileCount) + const classified = classifyChangedFiles(files) + const plan = planReviewers(parseOwnership(ownershipSource), classified.changedCodeFiles) + writeList(write, 'Changed code files', classified.changedCodeFiles.map(file => JSON.stringify(file))) + writeList(write, 'Excluded test files', classified.excludedTestFiles.map(file => JSON.stringify(file))) + writeList( + write, + 'Owners by changed file', + plan.matches.map(({ file, owners }) => `${JSON.stringify(file)}: ${owners.length ? owners.join(' ') : '(none)'}`), + ) + + const candidates = plan.reviewers.filter(login => login.toLowerCase() !== pull.author.toLowerCase()) + if (candidates.length === 0) { + writeList(write, 'Reviewers to request', []) + return { ...classified, requestedReviewers: [] } + } + const existing = await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`) + if (!isRecord(existing) || !Array.isArray(existing.users)) { + throw new Error('requested-reviewers response has no users array') + } + const alreadyRequested = new Set(existing.users.map((user) => { + if (!isRecord(user) || typeof user.login !== 'string') { + throw new Error('requested-reviewers response contains an invalid user') + } + return user.login.toLowerCase() + })) + const reviewers = candidates.filter(login => !alreadyRequested.has(login.toLowerCase())) + writeList(write, 'Reviewers to request', reviewers.map(login => `@${login}`)) + if (reviewers.length === 0) return { ...classified, requestedReviewers: [] } + + await api(`/repos/${pull.repository}/pulls/${pull.number}/requested_reviewers`, { + method: 'POST', + body: { reviewers }, + }) + write(`Requested ${reviewers.map(login => `@${login}`).join(' ')}.`) + return { ...classified, requestedReviewers: reviewers } +} + +function pullRequestFromEvent(event) { + if (!isRecord(event) || !isRecord(event.repository) || typeof event.repository.full_name !== 'string') { + throw new Error('event has no repository.full_name') + } + if (!isRecord(event.pull_request) || !isRecord(event.pull_request.user)) { + throw new Error('event has no pull_request') + } + const { pull_request: pull } = event + if (!Number.isSafeInteger(pull.number) || pull.number <= 0) throw new Error('pull request has no valid number') + if (typeof pull.draft !== 'boolean') throw new Error('pull request has no draft flag') + if (typeof pull.user.login !== 'string' || !pull.user.login) throw new Error('pull request has no author login') + return { + repository: event.repository.full_name, + number: pull.number, + draft: pull.draft, + author: pull.user.login, + changedFileCount: pull.changed_files, + } +} + +function writeList(write, title, entries) { + write(`${title}:`) + if (entries.length === 0) write('- (none)') + else for (const entry of entries) write(`- ${entry}`) +} + +function isRecord(value) { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +async function main() { + const eventPath = process.env.GITHUB_EVENT_PATH + if (!eventPath) throw new Error('GITHUB_EVENT_PATH is not set') + const event = JSON.parse(readFileSync(eventPath, 'utf8')) + const ownershipSource = readFileSync(new URL('CODEOWNERS', import.meta.url), 'utf8') + const api = createGitHubApi({ + token: process.env.GITHUB_TOKEN ?? '', + apiUrl: process.env.GITHUB_API_URL, + }) + await requestReviews({ event, ownershipSource, api }) +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + main().catch((error) => { + process.stderr.write(`request-review failed: ${error instanceof Error ? error.message : String(error)}\n`) + process.exitCode = 1 + }) +} diff --git a/.github/review-ownership/request-review.test.mjs b/.github/review-ownership/request-review.test.mjs new file mode 100644 index 0000000000..79a44d7fd5 --- /dev/null +++ b/.github/review-ownership/request-review.test.mjs @@ -0,0 +1,335 @@ +import assert from 'node:assert/strict' +import { execFileSync } from 'node:child_process' +import { readFileSync } from 'node:fs' +import test from 'node:test' + +import { + classifyChangedFiles, + createGitHubApi, + isTestPath, + listPullRequestFiles, + normalizeRepositoryPath, + parseOwnership, + planReviewers, + requestReviews, +} from './request-review.mjs' + +const ownershipSource = readFileSync(new URL('CODEOWNERS', import.meta.url), 'utf8') + +const pullRequestEvent = ({ author = 'author', changedFiles = 1, draft = false } = {}) => ({ + repository: { full_name: 'deepseek-harness/deepseek-harness' }, + pull_request: { + number: 42, + draft, + changed_files: changedFiles, + user: { login: author }, + }, +}) + +test('loads the repository ownership policy without test-only directory rules', () => { + const rules = parseOwnership(ownershipSource) + const ownersByPattern = new Map(rules.map(rule => [rule.pattern, rule.owners])) + assert.equal(rules.length, 58) + assert.equal(rules.some(rule => rule.pattern === '/benchmarks/'), false) + assert.equal(rules.some(rule => rule.pattern === '/snapshots/'), false) + assert.equal(rules.some(rule => rule.pattern === '/packages/test-support/'), false) + assert.deepEqual(ownersByPattern.get('/apps/cli/'), ['@turtle1999']) + assert.deepEqual(ownersByPattern.get('/docs/'), ['@turtle1999']) + assert.deepEqual(ownersByPattern.get('/packages/core/'), ['@tianyicui', '@turtle1999', '@mektpoy']) + assert.deepEqual(ownersByPattern.get('/packages/llm/'), ['@LegGasai']) + assert.deepEqual(ownersByPattern.get('/packages/preset/'), ['@LegGasai', '@turtle1999']) + assert.deepEqual(ownersByPattern.get('/packages/session/'), ['@tianyicui', '@turtle1999', '@mektpoy']) + assert.deepEqual(ownersByPattern.get('/packages/subagent/'), ['@Dudu-0223']) + assert.deepEqual(ownersByPattern.get('/packages/web/'), ['@imccyu']) + assert.deepEqual(ownersByPattern.get('/python/'), ['@LegGasai']) + assert.deepEqual(ownersByPattern.get('/website/'), ['@LegGasai']) + assert.deepEqual( + rules.filter(rule => rule.owners.includes('@tianyicui')).map(rule => rule.pattern), + ['/packages/core/', '/packages/session/'], + ) + for (const excludedOwner of ['@kermeanx', '@pkh-xht']) { + assert.equal(rules.some(rule => rule.owners.some(owner => owner.toLowerCase() === excludedOwner)), false) + } +}) + +test('keeps turtle below one third of the eligible owned codebase', () => { + const rules = parseOwnership(ownershipSource) + const trackedFiles = execFileSync('git', ['ls-files', '-z'], { encoding: 'utf8' }).split('\0').filter(Boolean) + let ownedLines = 0 + let turtleLines = 0 + for (const file of trackedFiles) { + if (isTestPath(file)) continue + const owners = planReviewers(rules, [file]).matches[0]?.owners ?? [] + if (owners.length === 0) continue + const content = readFileSync(file) + const lines = content.length === 0 + ? 0 + : content.reduce((count, byte) => count + (byte === 10 ? 1 : 0), 0) + (content.at(-1) === 10 ? 0 : 1) + ownedLines += lines + if (owners.includes('@turtle1999')) turtleLines += lines + } + assert.ok( + turtleLines * 3 <= ownedLines, + `@turtle1999 owns ${turtleLines} of ${ownedLines} eligible owned lines`, + ) +}) + +test('rejects ownership forms the requester cannot apply safely', () => { + for (const [source, message] of [ + ['', /contains no rules/u], + ['* @owner\n', /explicit absolute directory/u], + ['/.github/ @owner\n', /hidden-directory/u], + ['/packages/*/ @owner\n', /explicit absolute directory/u], + ['/packages/core/\n', /at least one owner/u], + ['/packages/core/ @org/team\n', /individual GitHub users/u], + ['/packages/core/ @owner @OWNER\n', /duplicate owner/u], + ['/packages/core/ @owner\n/packages/core/ @other\n', /duplicate pattern/u], + ]) { + assert.throws(() => parseOwnership(source), message) + } +}) + +test('recognizes every repository test location and filename convention', () => { + for (const file of [ + 'apps/cli/tests/args.spec.ts', + 'apps/cli/tests/harness.ts', + 'apps/web/stress-tests/reasoning-chunks.stress.ts', + 'benchmarks/session-open/workload.ts', + 'native/landlock-run/test/entry.test.js', + 'packages/core/agent/__tests__/agent.ts', + 'packages/core/agent/benches/agent.rs', + 'packages/core/agent/src/agent.compat.spec.ts', + 'packages/core/agent/src/__snapshots__/agent.ts.snap', + 'packages/session-query/session-query/tests/test-service.ts', + 'packages/test-support/session-snapshot/src/index.ts', + 'python/sdk/src/test_client.py', + 'python/sdk/src/client_test.py', + 'scripts/fixtures/translation-prompt/response.txt', + 'scripts/session-snapshot-corpus.corpus.ts', + 'scripts/snapshots/translation-prompt-v4/request-response.expected.json', + 'snapshots/session/headless.snapshot.ts', + ]) { + assert.equal(isTestPath(file), true, file) + } +}) + +test('does not confuse production names with tests', () => { + for (const file of [ + 'apps/cli/src/testing.ts', + 'packages/core/agent/src/contest.ts', + 'packages/session/session-format/src/snapshot.ts', + 'packages/session/session-format/src/spec.ts', + 'packages/session/session-format/src/test.ts', + 'scripts/run-gates.ts', + 'vitest.config.ts', + 'vitest.bench.config.ts', + 'vitest.e2e.config.ts', + 'vitest.snapshot.config.ts', + 'vitest.web.perf.config.ts', + 'website/docs.ts', + ]) { + assert.equal(isTestPath(file), false, file) + } +}) + +test('normalizes separators and rejects paths that are not repository-relative', () => { + assert.equal(normalizeRepositoryPath('./packages\\core\\agent\\src\\index.ts'), 'packages/core/agent/src/index.ts') + for (const file of ['', '/absolute.ts', '../escape.ts', 'packages//empty.ts', 'packages/./same.ts']) { + assert.throws(() => normalizeRepositoryPath(file), /path/u, file) + } +}) + +test('classifies both sides of a rename independently', () => { + assert.deepEqual( + classifyChangedFiles([ + { + filename: 'packages/core/agent/tests/moved.spec.ts', + previous_filename: 'packages/core/agent/src/moved.ts', + }, + { + filename: 'packages/client/store/src/restored.ts', + previous_filename: 'packages/client/store/tests/restored.spec.ts', + }, + ]), + { + changedCodeFiles: [ + 'packages/client/store/src/restored.ts', + 'packages/core/agent/src/moved.ts', + ], + excludedTestFiles: [ + 'packages/client/store/tests/restored.spec.ts', + 'packages/core/agent/tests/moved.spec.ts', + ], + }, + ) +}) + +test('uses the last matching ownership rule and keeps unmatched files visible', () => { + const rules = parseOwnership('/packages/ @broad\n/packages/core/ @core @second\n') + assert.deepEqual( + planReviewers(rules, ['AGENTS.md', 'packages/core/agent/src/index.ts', 'packages/fs/fs/src/index.ts']), + { + matches: [ + { file: 'AGENTS.md', owners: [] }, + { file: 'packages/core/agent/src/index.ts', owners: ['@core', '@second'] }, + { file: 'packages/fs/fs/src/index.ts', owners: ['@broad'] }, + ], + reviewers: ['broad', 'core', 'second'], + }, + ) +}) + +test('fetches every declared changed file across pages', async () => { + const calls = [] + const pageOne = Array.from({ length: 100 }, (_, index) => ({ filename: `packages/core/file-${index}.ts` })) + const pageTwo = [{ filename: 'packages/core/file-100.ts' }] + const api = async (path) => { + calls.push(path) + return calls.length === 1 ? pageOne : pageTwo + } + const files = await listPullRequestFiles(api, 'owner/repo', 42, 101) + assert.equal(files.length, 101) + assert.deepEqual(calls, [ + '/repos/owner/repo/pulls/42/files?per_page=100&page=1', + '/repos/owner/repo/pulls/42/files?per_page=100&page=2', + ]) +}) + +test('fails closed when GitHub cannot provide the complete file list', async () => { + let calls = 0 + await assert.rejects( + listPullRequestFiles(async () => { + calls++ + return calls === 1 ? [{ filename: 'one.ts' }] : [] + }, 'owner/repo', 42, 2), + /returned 1 of 2/u, + ) + await assert.rejects( + listPullRequestFiles(async () => [], 'owner/repo', 42, 3_001), + /at most 3000/u, + ) +}) + +test('prints changed code files before requesting missing owners', async () => { + const trace = [] + const files = [ + { filename: 'packages/core/agent/src/index.ts' }, + { filename: 'packages/preset/agent-presets/src/index.ts' }, + { filename: 'packages/client/store/src/index.ts' }, + { filename: 'packages/subagent/subagent/src/index.ts' }, + { filename: 'packages/core/agent/tests/index.spec.ts' }, + { filename: 'AGENTS.md' }, + ] + const api = async (path, options = {}) => { + trace.push({ type: 'api', path, options }) + if (path.endsWith('/files?per_page=100&page=1')) return files + if (path.endsWith('/requested_reviewers') && options.method !== 'POST') { + return { users: [{ login: 'imccyu' }], teams: [] } + } + if (path.endsWith('/requested_reviewers') && options.method === 'POST') return {} + throw new Error(`unexpected API path ${path}`) + } + + const result = await requestReviews({ + event: pullRequestEvent({ author: 'turtle1999', changedFiles: files.length }), + ownershipSource, + api, + write: line => trace.push({ type: 'log', line }), + }) + + assert.deepEqual(result, { + changedCodeFiles: [ + 'AGENTS.md', + 'packages/client/store/src/index.ts', + 'packages/core/agent/src/index.ts', + 'packages/preset/agent-presets/src/index.ts', + 'packages/subagent/subagent/src/index.ts', + ], + excludedTestFiles: ['packages/core/agent/tests/index.spec.ts'], + requestedReviewers: ['Dudu-0223', 'LegGasai', 'mektpoy', 'tianyicui'], + }) + assert.equal(trace[0].type, 'log') + assert.equal(trace[0].line, 'This is by automated Angry Turtle Cyborg, not a human') + const changedHeading = trace.findIndex(item => item.type === 'log' && item.line === 'Changed code files:') + const post = trace.findIndex(item => item.type === 'api' && item.options.method === 'POST') + assert.ok(changedHeading >= 0 && changedHeading < post) + assert.deepEqual(trace[post], { + type: 'api', + path: '/repos/deepseek-harness/deepseek-harness/pulls/42/requested_reviewers', + options: { + method: 'POST', + body: { reviewers: ['Dudu-0223', 'LegGasai', 'mektpoy', 'tianyicui'] }, + }, + }) +}) + +test('does not request reviewers for a test-only change', async () => { + const calls = [] + const output = [] + const files = [ + { filename: 'apps/web/tests/chat.e2e.ts' }, + { filename: 'packages/core/agent/tests/agent.spec.ts' }, + ] + const result = await requestReviews({ + event: pullRequestEvent({ changedFiles: files.length }), + ownershipSource, + api: async (path) => { + calls.push(path) + return files + }, + write: line => output.push(line), + }) + assert.deepEqual(result, { + changedCodeFiles: [], + excludedTestFiles: files.map(file => file.filename), + requestedReviewers: [], + }) + assert.equal(calls.length, 1) + assert.deepEqual(output.slice(0, 4), [ + 'This is by automated Angry Turtle Cyborg, not a human', + 'Changed code files:', + '- (none)', + 'Excluded test files:', + ]) +}) + +test('defers draft pull requests without reading changed files', async () => { + const output = [] + const result = await requestReviews({ + event: pullRequestEvent({ draft: true }), + ownershipSource, + api: async () => assert.fail('draft routing must not call GitHub'), + write: line => output.push(line), + }) + assert.deepEqual(result, { changedCodeFiles: [], excludedTestFiles: [], requestedReviewers: [] }) + assert.deepEqual(output, [ + 'This is by automated Angry Turtle Cyborg, not a human', + 'Draft pull request; reviewer routing is deferred until ready_for_review.', + ]) +}) + +test('sends authenticated JSON and escapes an API error body', async () => { + const requests = [] + const api = createGitHubApi({ + token: 'secret', + apiUrl: 'https://github.example/api/v3/', + fetchImpl: async (url, options) => { + requests.push({ url, options }) + return new Response(JSON.stringify({ ok: true }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }) + }, + }) + assert.deepEqual(await api('/repos/owner/repo', { method: 'POST', body: { value: 1 } }), { ok: true }) + assert.equal(requests[0].url, 'https://github.example/api/v3/repos/owner/repo') + assert.equal(requests[0].options.headers.Authorization, 'Bearer secret') + assert.equal(requests[0].options.headers['X-GitHub-Api-Version'], '2026-03-10') + assert.equal(requests[0].options.body, '{"value":1}') + + const failing = createGitHubApi({ + token: 'secret', + fetchImpl: async () => new Response('::error::untrusted\nbody', { status: 422 }), + }) + await assert.rejects(failing('/failure'), /"::error::untrusted\\nbody"/u) +}) diff --git a/.github/workflows/request-review.yml b/.github/workflows/request-review.yml new file mode 100644 index 0000000000..0d202b3288 --- /dev/null +++ b/.github/workflows/request-review.yml @@ -0,0 +1,32 @@ +name: request-review + +on: + pull_request_target: + types: [opened, synchronize, reopened, ready_for_review] + +permissions: + contents: read + pull-requests: write + +concurrency: + group: request-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + request-review: + name: request-review + if: ${{ !github.event.pull_request.draft }} + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + # SECURITY: the write-capable job executes policy from the trusted default + # branch and reads pull-request filenames only as API data. + - name: Check out trusted review policy + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 + with: + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + - name: Request reviewers + env: + GITHUB_TOKEN: ${{ github.token }} + run: node .github/review-ownership/request-review.mjs diff --git a/package.json b/package.json index 349125f00b..9325f544d0 100644 --- a/package.json +++ b/package.json @@ -42,6 +42,7 @@ "test:expected": "vitest run --config vitest.expected.config.ts", "test:expected:refresh": "DSH_SNAPSHOT=refresh vitest run --config vitest.expected.config.ts", "test:issue-management": "node .github/issue-management/policy.test.mjs", + "test:request-review": "node --test .github/review-ownership/request-review.test.mjs", "test:snapshot": "vitest run --config vitest.snapshot.config.ts", "test:snapshot:record": "DSH_SNAPSHOT=record vitest run --config vitest.snapshot.config.ts --update", "test:snapshot:refresh": "DSH_SNAPSHOT=refresh vitest run --config vitest.snapshot.config.ts", diff --git a/scripts/ci-workflow.spec.ts b/scripts/ci-workflow.spec.ts index 02ffac7aba..9bb17ae6ee 100644 --- a/scripts/ci-workflow.spec.ts +++ b/scripts/ci-workflow.spec.ts @@ -735,6 +735,47 @@ describe('Python release workflows', () => { }) }) +describe('Request review workflow', () => { + it('runs trusted routing on reviewable pull request updates', () => { + const workflow = loadWorkflow('.github/workflows/request-review.yml') + const event = workflowEvent(workflow, 'pull_request_target') + const job = workflowJob(workflow, 'request-review') + if (!isRecord(workflow.on)) throw new TypeError('request-review workflow must define events') + if (!Array.isArray(job.steps)) throw new TypeError('request-review job must define steps') + const steps = job.steps.filter(isRecord) + const checkout = steps.find(step => step.name === 'Check out trusted review policy') + const request = steps.find(step => step.name === 'Request reviewers') + + expect(workflow.name).toBe('request-review') + expect(Object.keys(workflow.on)).toEqual(['pull_request_target']) + expect(event.types).toEqual(['opened', 'synchronize', 'reopened', 'ready_for_review']) + expect(workflow.permissions).toEqual({ contents: 'read', 'pull-requests': 'write' }) + expect(workflow.concurrency).toEqual({ + group: 'request-review-${{ github.event.pull_request.number }}', + 'cancel-in-progress': true, + }) + expect(job).toMatchObject({ + name: 'request-review', + if: '${{ !github.event.pull_request.draft }}', + 'runs-on': 'ubuntu-latest', + 'timeout-minutes': 5, + }) + expect(checkout).toMatchObject({ + uses: 'actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1', + with: { + ref: '${{ github.event.repository.default_branch }}', + 'persist-credentials': false, + }, + }) + expect(request).toMatchObject({ + env: { GITHUB_TOKEN: '${{ github.token }}' }, + run: 'node .github/review-ownership/request-review.mjs', + }) + expect(JSON.stringify(workflow)).not.toContain('github.event.pull_request.head') + expect(JSON.stringify(workflow)).not.toContain('secrets.') + }) +}) + describe('Issue lifecycle workflow', () => { it('runs the lifecycle job on every PR/review event but gates token and board steps', () => { const lifecycle = loadWorkflow('.github/workflows/issue-lifecycle.yml') diff --git a/scripts/run-gates.spec.ts b/scripts/run-gates.spec.ts index 120a976863..f34e4897ab 100644 --- a/scripts/run-gates.spec.ts +++ b/scripts/run-gates.spec.ts @@ -240,6 +240,15 @@ describe('gate graph validation', () => { }, ) + it.each(['ci-primary', 'ci-static', 'check-all'] as const)( + 'keeps review request policy tests in %s', + (mode) => { + const ids = withPnpmEntrypoint(() => gatesForMode(mode).map(subject => subject.id)) + + expect(ids).toContain('request-review') + }, + ) + it.each(['ci-primary', 'ci-static', 'check-all', 'hygiene'] as const)( 'keeps hard-coded Client UI copy enforcement in %s', (mode) => { diff --git a/scripts/run-gates.ts b/scripts/run-gates.ts index 53a01d604e..c5ede4c195 100644 --- a/scripts/run-gates.ts +++ b/scripts/run-gates.ts @@ -267,6 +267,7 @@ export function gatesForMode(selected: Mode): Gate[] { pnpmScript('client-domain-graph', 'verify-client-domain-graph', { label: 'client domain graph' }), pnpmScript('test', 'test'), pnpmScript('issue-management', 'test:issue-management', { label: 'Issue management policy' }), + pnpmScript('request-review', 'test:request-review', { label: 'Review request policy' }), pnpmScript('duplication', 'duplication'), snapshotGate(), expectedOutputGate(), @@ -310,6 +311,7 @@ function ciSharedStaticGates(): Gate[] { pnpmScript('client-ui-i18n', 'verify-client-ui-i18n', { label: 'client UI i18n' }), pnpmScript('no-bare-dispatcher', 'verify-no-bare-dispatcher', { label: 'proxy-aware dispatchers' }), pnpmScript('issue-management', 'test:issue-management', { label: 'Issue management policy' }), + pnpmScript('request-review', 'test:request-review', { label: 'Review request policy' }), ] }