-
Notifications
You must be signed in to change notification settings - Fork 3k
refactor(core): move review skill incident narratives to DESIGN.md #8499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
685c4c4
29aa168
33db4f1
4c95b57
6927f84
bf0d949
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import * as fs from 'node:fs'; | ||
| import * as path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| const skillDir = path.dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| // Titles may end in one parenthesized qualifier, e.g. "The two-dot phantom | ||
| // regressions (PR #6626)", so the match allows a single nested group. | ||
| const POINTER_RE = /\(measured; DESIGN\.md — ([^()\n]+(?:\([^()\n]*\))?)\)/g; | ||
| const POINTER_OPEN = '(measured; DESIGN.md — '; | ||
|
|
||
| function skillBody(): string { | ||
| return fs.readFileSync(path.join(skillDir, 'SKILL.md'), 'utf8'); | ||
| } | ||
|
|
||
| function incidentPointers(body: string): string[] { | ||
| return [...body.matchAll(POINTER_RE)].map(([, title]) => title.trim()); | ||
| } | ||
|
|
||
| function incidentHeadings(): string[] { | ||
| const design = fs.readFileSync(path.join(skillDir, 'DESIGN.md'), 'utf8'); | ||
| const start = design.indexOf('## Measured incidents'); | ||
| const end = design.indexOf('\n## ', start + 1); | ||
| const section = end === -1 ? design.slice(start) : design.slice(start, end); | ||
| return [...section.matchAll(/^### (.+)$/gm)].map(([, title]) => title.trim()); | ||
| } | ||
|
|
||
| describe('bundled review skill', () => { | ||
| it('anchors every SKILL.md incident pointer at a DESIGN.md heading', () => { | ||
| const body = skillBody(); | ||
| const pointers = incidentPointers(body); | ||
| expect(pointers.length).toBeGreaterThan(0); | ||
|
|
||
| // A pointer the regex cannot parse must fail loudly, not drop silently: | ||
| // every literal opener owes exactly one match. | ||
| let opens = 0; | ||
| for ( | ||
| let i = body.indexOf(POINTER_OPEN); | ||
| i !== -1; | ||
| i = body.indexOf(POINTER_OPEN, i + POINTER_OPEN.length) | ||
| ) { | ||
| opens++; | ||
| } | ||
| expect(pointers).toHaveLength(opens); | ||
|
|
||
| const headings = new Set(incidentHeadings()); | ||
| for (const title of pointers) { | ||
| expect( | ||
| headings.has(title), | ||
| `SKILL.md points at a missing DESIGN.md heading: "### ${title}"`, | ||
| ).toBe(true); | ||
| } | ||
| }); | ||
|
|
||
| it('leaves no DESIGN.md incident heading without a SKILL.md pointer', () => { | ||
| const referenced = new Set(incidentPointers(skillBody())); | ||
| for (const title of incidentHeadings()) { | ||
| expect( | ||
| referenced.has(title), | ||
| `DESIGN.md incident heading has no SKILL.md pointer: "### ${title}"`, | ||
| ).toBe(true); | ||
| } | ||
| }); | ||
|
|
||
| it('keeps the runtime guard against reading DESIGN.md mid-review', () => { | ||
| expect(skillBody()).toContain( | ||
| 'Never `read_file` DESIGN.md during a review.', | ||
| ); | ||
| }); | ||
|
|
||
| it('pins the setup-batch ordering constraints', () => { | ||
| const body = skillBody(); | ||
| expect(body).toContain('`fetch-pr` before all of them'); | ||
| expect(body).toContain('`agent-prompt --roster` after the rules load'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,7 +70,11 @@ export function copyBundleAssets({ root = defaultRoot } = {}) { | |
| const destBundledDir = join(distDir, 'bundled'); | ||
| fs.rmSync(destBundledDir, { recursive: true, force: true }); | ||
| copyRecursiveSync(bundledSkillsDir, destBundledDir, { | ||
| skipEntry: isBundledSkillTestFile, | ||
| // DESIGN.md files are maintainer design narratives, not runtime inputs; | ||
| // shipping one would hand a review a ~125 KB read_file target that | ||
| // outweighs the context the slimmed skill saves. | ||
| skipEntry: (entry) => | ||
| isBundledSkillTestFile(entry) || entry === 'DESIGN.md', | ||
|
Comment on lines
+76
to
+77
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The DESIGN.md exclusion only covers the esbuild-bundle path ( Suggested fix: mirror the exclusion in 中文说明[Suggestion] 对 DESIGN.md 的排除只覆盖了 esbuild 打包路径( 建议修复:在 — qwen3.8-max via Qwen Code /review (v0.21.5) |
||
| }); | ||
| console.log('Copied bundled skills to dist/bundled/'); | ||
| } else { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
POINTER_REsilently drops any pointer it cannot parse instead of failing loudly, and the reverse-direction test only backstops headings with a single pointer — so a malformed pointer to a multiply-referenced heading escapes both tests. Two titles are referenced twice today ("The roles nobody launched", "The self-filed COMMENT review (PR #6771)"). Verified by execution: a pointer with two parenthesized qualifiers yields 2 literal openers, 1 regex match, and both tests green. — Failure scenario: a future edit gives one of two sibling pointers a shape the regex cannot parse → it is silently excluded, the sibling still satisfies heading coverage, and a stale/broken anchor ships with nothing red.Suggested fix — count literal openers and assert the matched list has exactly that length:
中文说明
[Suggestion]
POINTER_RE遇到解析不了的指针会静默丢弃而不是大声失败,而反向测试只为“只有一个指针”的标题兜底——因此指向“被多次引用标题”的畸形指针能同时逃过两个测试。当前已有两个标题被引用两次(“The roles nobody launched”“The self-filed COMMENT review (PR #6771)”)。已实际执行验证:带两个括号限定符的指针会产生 2 个字面起始符、1 个正则匹配,两个测试全绿。— 失败场景:未来某次编辑把两个兄弟指针之一改成该正则无法解析的形状 → 它被静默排除,兄弟指针仍满足标题覆盖,陈旧/损坏的锚点在无任何测试变红的情况下被放行。建议修复——统计字面起始符数量并断言匹配列表长度一致:
— qwen3.8-max via Qwen Code /review (v0.21.5)