Skip to content

chore(review): narrow automatic bundled skill context - #9022

Closed
destire-mio wants to merge 12 commits into
QwenLM:mainfrom
destire-mio:work/fix-review-context-limit
Closed

chore(review): narrow automatic bundled skill context#9022
destire-mio wants to merge 12 commits into
QwenLM:mainfrom
destire-mio:work/fix-review-context-limit

Conversation

@destire-mio

@destire-mio destire-mio commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This PR further narrows the repository review manifest's automatic core-skills context from the entire skills subtree to the top-level TypeScript implementation/tests plus every bundled skill's SKILL.md. It also strengthens the committed manifest-policy tests so each glob is exercised through the real provider, every current top-level skill source and bundled entrypoint is covered, nested bundled material is excluded, Windows path normalization is pinned, and the combined related-path set stays within the 128-file wire contract.

Why it's needed

#9028 fixed the immediate main-branch overflow by removing Web Shell E2E paths that breached the resolved-file bound. On current main, the all-rules provider probe now resolves 107 of 128 allowed related paths. This PR is a separate follow-up that reduces the same probe to 93 paths, leaving 35 slots of headroom, while retaining the shared skill loader/manager/types context and every bundled skill entrypoint. Keeping nested bundled references, scripts, and implementation details in automatic context is unnecessary because changed files still appear in the review diff and a reviewer can request additional repository context explicitly.

Reviewer Test Plan

How to verify

  1. On current main, run the committed all-rules provider probe and confirm it resolves 107 related paths after fix(review): drop the web-shell e2e related-paths that breach the resolved-file bound #9028.
  2. On this branch, run:
cd packages/cli
npx vitest run src/commands/review/lib/manifest-repository-context.committed.test.ts src/commands/review/lib/manifest-repository-context.test.ts src/commands/review/lib/review-digest-covers-only-bundled.test.ts

Expected: 3 files / 73 tests pass. The real-provider union probe resolves 93 related paths, including every top-level packages/core/src/skills/*.ts file and every packages/core/src/skills/bundled/*/SKILL.md, while excluding nested bundled references, scripts, and implementation files.

  1. Run npm run typecheck --workspace @qwen-code/qwen-code, focused ESLint and Prettier checks for the four changed files, and git diff --check. Expected: all pass.

Evidence (Before & After)

N/A — this is a non-UI repository review-policy change. Current main after #9028 resolves 107/128 related paths in the all-rules probe; this branch resolves 93/128 and leaves 35 slots. The regression deliberately checks the protocol ceiling rather than pinning an exact working-tree-sensitive count. It also verifies rule membership exhaustively: all current top-level skill TypeScript sources and all bundled SKILL.md entrypoints are included, nested bundled files are excluded, and host-native Windows paths are normalized before matching.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ path behavior covered by regression test
🐧 Linux ⚠️ not tested locally

Environment (optional)

macOS arm64, Node.js v26.5.0, npm 11.17.0. The branch is synchronized with main at abfd44369860b53aff1b0607ff983be6a0a98bfa.

Risk & Scope

  • Main risk or tradeoff: nested bundled-skill reference documents, scripts, and implementation details are no longer attached automatically as related paths. Changed files remain in the review diff, and the shared top-level skill implementation/tests plus every bundled SKILL.md remain available automatically.
  • Not validated / out of scope: changing the 128-file wire limit, silently truncating oversized manifests, redesigning repository-context selection, or manually exercising Linux. CI covers the supported Node lane.
  • Breaking changes / migration notes: none. This only narrows this repository's committed review metadata.

Linked Issues

Follow-up to #9028. Related to #8654 and the tree growth introduced by #8804.

中文说明

此 PR 做了什么

此 PR 进一步将仓库 review manifest 自动附加的 core-skills 上下文从整个 skills 子树收窄为顶层 TypeScript 实现/测试文件,以及每个 bundled skill 的 SKILL.md。同时加强已提交的 manifest 策略测试:所有 glob 都会通过真实 provider 验证,覆盖当前全部顶层 skill 源文件和 bundled 入口文件,排除嵌套 bundled 内容,固定 Windows 路径归一化行为,并确保合并后的关联路径总数始终满足 128 文件的 wire contract。

为什么需要

#9028 已经通过移除导致 resolved-file 上限溢出的 Web Shell E2E 路径,修复了主分支当时的直接失败。当前 main 的全规则 provider 探针会解析出 107/128 个关联路径。本 PR 是独立的后续收窄:同一探针会降到 93 个路径,留下 35 个位置余量,同时保留共享的 skill loader/manager/types 上下文和每个 bundled skill 的入口文件。嵌套的 bundled 参考资料、脚本和实现细节无需全部自动附加,因为发生改动的文件仍会出现在 review diff 中,Reviewer 也可以按需请求额外仓库上下文。

Reviewer 测试计划

如何验证

  1. 在当前 main 上运行已提交的全规则 provider 探针,确认 fix(review): drop the web-shell e2e related-paths that breach the resolved-file bound #9028 合入后会解析出 107 个关联路径。
  2. 在本分支运行:
cd packages/cli
npx vitest run src/commands/review/lib/manifest-repository-context.committed.test.ts src/commands/review/lib/manifest-repository-context.test.ts src/commands/review/lib/review-digest-covers-only-bundled.test.ts

预期:3 个文件 / 73 个测试全部通过。真实 provider 的规则并集探针会解析出 93 个关联路径,其中包含所有顶层 packages/core/src/skills/*.ts 文件和所有 packages/core/src/skills/bundled/*/SKILL.md,并排除嵌套的 bundled 参考资料、脚本和实现文件。

  1. 运行 npm run typecheck --workspace @qwen-code/qwen-code、针对四个改动文件的 ESLint 和 Prettier 检查,以及 git diff --check。预期:全部通过。

证据(修改前与修改后)

N/A —— 这是非 UI 的仓库 review 策略变更。#9028 合入后的当前 main 在全规则探针中解析出 107/128 个关联路径;本分支解析出 93/128 个,并留下 35 个位置。回归测试有意守住协议上限,而不固定容易受工作树影响的精确数量;同时会穷举验证规则成员:包含当前全部顶层 skill TypeScript 源文件和所有 bundled SKILL.md 入口,排除嵌套 bundled 文件,并在匹配前归一化宿主 Windows 路径。

测试平台

系统 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 路径行为由回归测试覆盖
🐧 Linux ⚠️ 未进行本地测试

环境(可选)

macOS arm64、Node.js v26.5.0、npm 11.17.0。本分支已同步到 mainabfd44369860b53aff1b0607ff983be6a0a98bfa

风险与范围

  • 主要风险或取舍:嵌套的 bundled-skill 参考文档、脚本和实现细节不再自动作为关联路径附加。发生改动的文件仍会出现在 review diff 中;共享的 skills 顶层实现/测试文件和每个 bundled SKILL.md 仍会自动提供。
  • 未验证/范围外:修改 128 文件 wire 上限、静默截断过大的 manifest、重新设计 repository-context 选择逻辑,或在本地手工验证 Linux。CI 会覆盖支持的 Node lane。
  • 破坏性变更/迁移说明:无。这里只收窄本仓库已提交的 review metadata。

关联 Issue

#9028 的后续改进。关联 #8654,以及 #8804 引入的文件树增长。

@destire-mio
destire-mio marked this pull request as ready for review August 12, 2026 18:55
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

  • Problem — observed, verified. This is not theoretical: feat(cli): add native multi-agent coordination #8804 merged today and added bundled/coordinate/SKILL.md, growing the skills subtree to 43 files. Counting the manifest's expansions on main gives 12 (config) + 43 (skills) + 74 (web-shell globs) = 129 related paths when every rule co-matches — one over the 128 wire bound. The damage is already visible in CI: unrelated PR fix(review): close the inline-quotation gap and harden the layer gate #9020's Test (ubuntu-latest, Node 22.x) failed today at 18:48 UTC with Error: repository context manifest relatedPaths exceeds limit in the co-match bound test, and most fork-PR runs since ~17:40 have failed the same way.
  • Direction — aligned. Keeping the fail-closed 128 contract and narrowing the manifest is the right call; raising the protocol limit or silently truncating would both weaken a deliberate safety bound. This repairs the repository's own dogfood manifest introduced in feat(review): add qwen-code repository context manifest #8654.
  • Size — core gate not applicable. No protected core paths are touched; the diff is repository review metadata (+4/−1) plus its pinned test in packages/cli (+6/−2).
  • Approach — minimal, one question. Every edit is needed for the stated goal; nothing to split. One thing worth thinking about before merge: packages/core/src/skills/**/*.ts would keep the nested loop/review implementation and test files in related context (union 123, still under 128) at the cost of 8 slots of headroom, versus the 13 slots your top-level-only *.ts leaves. Your tradeoff is defensible and documented in the PR body — flagging it for the maintainer's awareness rather than blocking on it.
  • Risk — no elevated signals. No high-risk-path matches.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

  • 问题——已观测、已核实。 不是理论问题:feat(cli): add native multi-agent coordination #8804 今天合入后新增了 bundled/coordinate/SKILL.md,skills 子树增至 43 个文件。在 main 上统计 manifest 各规则同时命中时的展开结果:12(config)+ 43(skills)+ 74(web-shell 各 glob)= 129 个关联路径,比 128 的线上限多 1。CI 已经在受影响:无关 PR fix(review): close the inline-quotation gap and harden the layer gate #9020Test (ubuntu-latest, Node 22.x) 今天 18:48 UTC 在共同命中上限测试中以 Error: repository context manifest relatedPaths exceeds limit 失败,17:40 之后大多数 fork PR 的 CI 同样失败。
  • 方向——对齐。 保留 fail-closed 的 128 契约、收窄 manifest 是正确选择;提高协议上限或静默截断都会削弱刻意的安全边界。此 PR 修复的是 feat(review): add qwen-code repository context manifest #8654 引入的本仓库自用 manifest。
  • 规模——核心门槛不适用。 未触及受保护的核心路径;diff 为仓库 review 元数据(+4/−1)及 packages/cli 中的固定测试(+6/−2)。
  • 方案——最小化,有一个问题。 每处改动都是必要的,无需拆分。合并前值得考虑的一点:packages/core/src/skills/**/*.ts 可以把嵌套的 loop/review 实现与测试文件保留在关联上下文中(并集 123,仍低于 128),代价是少 8 个名额余量;你选择的仅顶层 *.ts 留下 13 个余量。你的取舍合理且已在正文中说明——这里只是提请维护者知悉,不作为阻塞项。
  • 风险——无升级信号。 未命中高风险路径。

进入代码审查。🔍

Qwen Code · qwen3.8-max

Reviewed at bc77348b450b7ffa1755843ff217a33bd493c909 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

Before reading the diff, my independent proposal for "the merged manifest expansion outgrows the 128 cap after tree growth" was exactly what this PR does: narrow the core-skills relatedPaths glob to the stable entrypoints (top-level loader/manager implementation + each bundled skill's SKILL.md) and re-pin the committed test — rather than raising the wire limit or truncating. The PR matches that baseline; the only alternative I would have considered (**/*.ts to keep nested implementation files) was already flagged in Stage 1.

Findings — no blockers:

  • Manifest change is schema-valid. Both new patterns pass the provider's own validateGlob rules (segment shape, static directory prefix, no skipped-directory segments), and together they resolve to 29 files on the current tree — 19 top-level .ts plus 10 SKILL.md, verified by static count.
  • Test update keeps the pin honest. expectedManifest mirrors the JSON exactly, the sentinel map's keys still equal the flattened relatedPaths pattern set, and the new sentinel skills/skill-manager.ts exists and matches skills/*.ts. The bound test — the one currently failing across CI — now resolves to 115 ≤ 128 by static count, and it remains the early-warning tripwire for the next tree growth.
  • No correctness, security, or convention issues; nothing beyond the minimal set.

Test evidence (PR's own CI, read via API — this run executes no PR code)

The first run (on 5e78cd2) was cancelled at 18:57 UTC because the author merged origin/main into the branch — not a test failure. The replacement run on the reviewed commit is still in flight; the finalize workflow updates the table below once CI settles.

Baseline context: main is red on this exact test — unrelated PR #9020's Test (ubuntu-latest, Node 22.x) failed today at 18:48 UTC with Error: repository context manifest relatedPaths exceeds limit in stays under the resolved-file bound when every rule co-matches — so a green run here is meaningful, not vacuous. Note also that the macOS/Windows unit lanes run only in the merge queue by design; the Ubuntu lane is the PR signal.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ⏳ in progress
Classify PR ✅ success
precheck-pr / precheck ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Test (macos / windows), Integration Tests skipped (merge-queue-only lanes)

Real-scenario testing: N/A — repository review metadata with no user-visible surface, and this is an unattended CI run.

中文说明

代码审查:读 diff 之前,我对"manifest 展开超过 128 上限"的独立方案与本 PR 一致——把 core-skills 的关联 glob 收窄到稳定入口(顶层实现/测试 + 每个 bundled skill 的 SKILL.md)并同步固定已提交测试,而不是提高线上限或截断。唯一替代方案(**/*.ts 保留嵌套实现文件)已在 Stage 1 提出。无阻塞项:两个新 glob 均通过 provider 自身的 validateGlob 校验,按静态计数在当前树上共解析 29 个文件(19 个顶层 .ts + 10 个 SKILL.md);测试同步完整——期望 manifest 与 JSON 逐字一致,哨兵键集合与展开后的 pattern 集合完全相等,新哨兵 skills/skill-manager.ts 真实存在且匹配 skills/*.ts;上限测试按静态计数解析为 115 ≤ 128,并继续充当树增长时的预警。

测试证据:本次运行不执行任何 PR 代码。首次运行(5e78cd2)于 18:57 UTC 因作者合并 origin/main 被并发取消,并非测试失败;被审提交上的新运行仍在进行,finalize 工作流会在 CI 结束后更新上方表格。基线:main 上该测试正是红的——无关 PR #9020 的 Ubuntu 测试今天 18:48 UTC 以同一错误失败——因此这里的绿运行有实际意义。另注:macOS/Windows 单测 lane 按设计只在合并队列运行,PR 信号为 Ubuntu lane。

真实场景测试:N/A——仓库 review 元数据,无用户可见界面,且本次为无人值守 CI 运行。

Qwen Code · qwen3.8-max

Reviewed at bc77348b450b7ffa1755843ff217a33bd493c909 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, evidence-backed fix; the only open items are a glob-scope judgment call and the CI run still in flight on the reviewed commit.

This is a model fix. The problem is real and independently verified — the co-match union resolves to 129 related paths on main, and the bound test has been failing across fork-PR CI since #8804 landed this afternoon. The diff is the minimal correct response: narrow the manifest rather than raise or weaken the 128 wire contract, and keep the pinned test as the tripwire for the next tree growth. The 14 nested files that drop out of automatic related context (dataviz references/scripts, the review DESIGN.md, nested loop/review tests) are a documented tradeoff, and they still ride in the diff whenever they themselves change.

The one thing I'd ask the maintainer to double-check: top-level skills/*.ts leaves 13 slots of headroom, while skills/**/*.ts would keep the nested implementation/test files in context at a union of 123. Both satisfy the contract today; the choice decides when this test next trips.

Approval deferred until CI lands green on bc77348b450b7ffa1755843ff217a33bd493c909 — the Ubuntu unit lane is still running on that commit.

中文说明

置信度:4/5 —— 干净、有证据支撑的修复;唯一未决项是 glob 范围的取舍,以及被审提交上的 CI 仍在运行。

这是一个教科书式的修复:问题真实存在且已独立核实——main 上全规则共同命中时解析出 129 个关联路径,自 #8804 今天下午合入后,该上限测试在几乎所有 fork PR 的 CI 中持续失败。diff 是最小且正确的应对:收窄 manifest,而不是提高或削弱 128 线上契约,并让固定测试继续充当树增长时的预警。被移出自动关联上下文的 14 个嵌套文件(dataviz 参考文档/脚本、review 的 DESIGN.md、嵌套 loop/review 测试)是正文已说明的取舍,且它们自身变更时仍会出现在 diff 里。

请维护者复核的一点:仅顶层 skills/*.ts 留下 13 个名额余量,而 skills/**/*.ts 可以把嵌套实现/测试文件保留在上下文中(并集 123)。两者今天都满足契约,但这个选择决定该测试下次何时触发。

批准推迟到 CI 在 bc77348b450b7ffa1755843ff217a33bd493c909 上全绿之后——该提交的 Ubuntu 单测 lane 仍在运行。

Qwen Code · qwen3.8-max

Reviewed at bc77348b450b7ffa1755843ff217a33bd493c909 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): Change summary: This PR narrows the repository review man...: none — all planned checks completed (~10 of ~32 calls used)..

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):Change summary: This PR narrows the repository review man...:none — all planned checks completed (~10 of ~32 calls used).

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment thread .qwen/review-context.json
Comment on lines +24 to +27
"relatedPaths": [
"packages/core/src/skills/*.ts",
"packages/core/src/skills/bundled/*/SKILL.md"
],

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.

[Suggestion] The narrowing buys back only 14 files of the shared 128-file union (129 → 115) while the largest and fastest-growing consumers in that same union are left untouched — probe-verified against the real provider: the pre-PR manifest throws repository context manifest relatedPaths exceeds limit, the PR manifest resolves to 115 with exactly 13 files of headroom. The web-shell globs account for 74 files, of which packages/web-shell/client/hooks/** alone is 41 and grows by ~2 per new hook (impl + colocated test), and the manifest grammar has no negation syntax to exclude anything. — Failure scenario: after ~13 more files land across the config/skills/web-shell subtrees (~7 new hooks, or any mix of bundled skills, top-level skills .ts files, config files, or e2e specs), the all-rules co-match union exceeds 128 again → the committed bound test fails on every CI run of PRs unrelated to the review manifest, and any cross-area co-matching review throws fail-closed until someone edits the manifest again — exactly the breakage this PR exists to fix. Suggested fix: either rebalance within the same edit using larger levers (hooks/** = 41 files, config/** = 12 — at the cost of context breadth), or, if the recurrence is accepted as designed maintenance (the bound test is the alarm), record the headroom math (115/128, hooks grow ~2 files per hook, no negation globs) in this manifest's pinning test so the next overflow is anticipated instead of surprising its author.

中文说明

[建议] 此次收窄仅从共享的 128 文件并集中回收 14 个文件(129 → 115),而同一并集中最大、增长最快的使用者未被触及——已通过真实 provider 探针核实:PR 前的 manifest 抛出 repository context manifest relatedPaths exceeds limit,PR 的 manifest 解析为 115,仅剩恰好 13 个文件的余量。web-shell 各 glob 占 74 个文件,其中仅 packages/web-shell/client/hooks/** 就有 41 个,且每新增一个 hook 约增加 2 个文件(实现 + 同目录测试),manifest 语法也没有可排除文件的否定语法。——失败场景:当 config/skills/web-shell 各子树再新增约 13 个文件(约 7 个新 hook,或 bundled skill、顶层 skills .ts、config 文件、e2e 用例的任意组合)后,全规则共同命中的并集将再次超过 128 → 已提交的上限测试会在与 review manifest 无关的 PR 的每次 CI 运行中失败,且任何跨区域的共同命中 review 都会 fail-closed 抛错,直到有人再次编辑 manifest——这正是本 PR 要修复的故障。建议的修复:在同一次改动中用更大的杠杆重新平衡(hooks/** = 41 个文件、config/** = 12 个——代价是上下文广度),或者如果接受这种复现作为设计内的维护(上限测试就是警报),则在此 manifest 的固定测试中记录余量计算(115/128、hooks 每个 hook 增长约 2 个文件、无否定 glob),让下一次溢出在预期之内,而不是让作者感到意外。

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment thread .qwen/review-context.json
Comment on lines +24 to +27
"relatedPaths": [
"packages/core/src/skills/*.ts",
"packages/core/src/skills/bundled/*/SKILL.md"
],

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.

[Suggestion] The new globs encode an implicit "a bundled skill is exactly one SKILL.md" assumption that is already false in the tree today: 14 files under the rule's unchanged paths subtree no longer surface as related context — loop's 7 implementation/test .ts files (the largest loser — compiled package sources), dataviz's 3 references/*.md + 2 scripts/validate_palette* files, and review's DESIGN.md + SKILL.test.ts. Probe-verified: with the pre-PR manifest these siblings are all in relatedPaths for a probe changing bundled/dataviz/scripts/validate_palette.js; with the PR manifest all are excluded — and the trade-off is documented nowhere (the commit message is title-only). — Failure scenario: a PR that changes only bundled/dataviz/scripts/validate_palette.js still matches the core-skills domain, but its reviewer brief drops the palette reference docs and the sibling test that pre-drift main included → the changed file rides in the diff, its siblings do not surface; a future bundled skill with references/ or scripts/ subdirectories gets the same silent exclusion, which the next maintainer will read as an accident rather than a headroom decision. Note: the probe refuted the widening alternative — bundled/*/** matches 24 files, union 129 → the provider throws — so the exclusion is forced by the 128 bound and cannot be fixed by widening. Suggested fix: since the 128 bound forces the exclusion, document it instead — state in the PR description / commit message (and ideally next to the pinning test) which files deliberately drop out of automatic related context and why headroom won over nested bundled-skill context.

中文说明

[建议] 新的 glob 隐含了「一个 bundled skill 恰好只有一个 SKILL.md」的假设,而当前树中该假设已不成立:规则未改动的 paths 子树下有 14 个文件不再作为关联上下文出现——loop 的 7 个实现/测试 .ts 文件(损失最大者——均为已编译的包源码)、dataviz 的 3 个 references/*.md + 2 个 scripts/validate_palette* 文件,以及 review 的 DESIGN.md + SKILL.test.ts。探针核实:PR 前 manifest 下探针改动 bundled/dataviz/scripts/validate_palette.js 时这些兄弟文件均在 relatedPaths 中,PR 的 manifest 下全部被排除——且该取舍在任何地方都没有记录(commit message 仅有标题)。——失败场景:只改动 bundled/dataviz/scripts/validate_palette.js 的 PR 仍会命中 core-skills 域,但其 reviewer brief 会丢失漂移前 main 所包含的调色板参考文档和兄弟测试 → 被改动的文件随 diff 进入审查,其兄弟文件却不会出现;未来带 references/scripts/ 子目录的 bundled skill 会遭遇同样的静默排除,下一位维护者会将其误读为意外而非余量决策。注:探针已证伪放宽方案——bundled/*/** 匹配 24 个文件,并集 129 → provider 抛错——因此该排除是 128 上限强制的,无法通过放宽 glob 解决。建议的修复:既然 128 上限强制了排除,就将其文档化——在 PR 描述/commit message(最好也在固定测试旁)中说明哪些文件被有意移出自动关联上下文,以及为何选择余量而非嵌套 bundled-skill 上下文。

— qwen3.8-max via Qwen Code /review (v0.21.10)

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): This PR narrows the repository review manifest's core-ski...: run the committed vitest file itself (no node_modules in the worktree; central claims verified by filesystem emulation instead).

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):This PR narrows the repository review manifest's core-ski...:run the committed vitest file itself (no node_modules in the worktree; central claims verified by filesystem emulation instead)

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines +229 to +230
// for future rebalancing rather than permission to truncate the result.
expect(context?.relatedPaths).toHaveLength(115);

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.

[Suggestion] R2-1: The new toHaveLength(115) "deliberate alarm" pin is exact only while no relatedPaths glob covers a file the bound test's own probe set passes as a changed path — an unenforced invariant the added comment does not state. probeFor passes literal paths entries through unchanged, so the test feeds the real on-disk eslint.config.js and package.json as changedPaths, and expandRelatedPaths excludes changed paths from the resolved set; a static top-level entry is a grammatically legal relatedPaths pattern (it passes requireDirectoryPrefix). — Failure scenario: a future manifest edit adding a relatedPaths entry covering package.json or eslint.config.js makes the bound test resolve N−1 files while a real all-rule review resolves N — probe-verified end-to-end: with a manifest whose real union is 129, the bound shape stayed green at 127 while the real-review shape threw repository context manifest relatedPaths exceeds limit, so the provider fails closed at review time with the CI alarm never having fired. Suggested fix: state the invariant in the added comment.

Suggested change
// for future rebalancing rather than permission to truncate the result.
expect(context?.relatedPaths).toHaveLength(115);
// for future rebalancing rather than permission to truncate the result.
// The probe set contains two real files (eslint.config.js, package.json);
// expansion excludes changed paths, so this count stays exact only while
// no relatedPaths glob covers a literal paths entry.
expect(context?.relatedPaths).toHaveLength(115);
中文说明

[建议] 新增的 toHaveLength(115)「刻意警报」固定值只有在没有任何 relatedPaths glob 覆盖到 bound 测试自身探针集作为 changed path 传入的文件时才精确——这是一个未被强制执行的不变量,新增注释也没有说明。probeFor 会把字面量 paths 条目原样透传,因此测试会把真实存在的 eslint.config.jspackage.json 作为 changedPaths 传入,而 expandRelatedPaths 会把 changed paths 从解析结果中排除;静态顶层条目是语法上合法的 relatedPaths 模式(能通过 requireDirectoryPrefix)。——失败场景:未来某次 manifest 编辑新增了一个覆盖 package.jsoneslint.config.jsrelatedPaths 条目时,bound 测试解析出 N−1 个文件,而真实的全规则 review 解析出 N 个——已通过端到端探针核实:当 manifest 的真实并集为 129 时,bound 形态以 127 保持绿色,而真实 review 形态抛出 repository context manifest relatedPaths exceeds limit,即 provider 在 review 时 fail-closed,而 CI 警报从未触发。建议的修复:在新增注释中写明这一不变量。

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines 230 to 231
expect(context?.relatedPaths).toHaveLength(115);
expect(context?.relatedPaths.length).toBeLessThanOrEqual(MAX_ARRAY_ITEMS);

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.

[Suggestion] R2-2: The exact pin measures the working tree, not the committed tree — expandRelatedPaths scans disk via readdirSync and never consults git — so it fires on any tree change, including untracked local files, even though the 128-item wire contract is already enforced by the toBeLessThanOrEqual(MAX_ARRAY_ITEMS) line directly below. — Failure scenario: probe-verified — creating one empty untracked file matching a covered glob (e.g. a WIP file under packages/web-shell/client/hooks/) failed this committed test with "expected 115, received 116" — indistinguishable from real contract drift — although nothing was committed and the manifest was unchanged; benign committed growth under covered globs (hooks/** is named in the added comment as the fastest-growing group) likewise forces a ritual magic-number bump even though the contract line already enforces the bound. Suggested fix: either replace the exact pin with a bounded range so benign growth does not churn it, or — if the pin is kept for shrinkage detection — state in the comment that it counts working-tree files and fails on untracked files, so that failure mode is not misread as a contract violation.

Suggested change
expect(context?.relatedPaths).toHaveLength(115);
expect(context?.relatedPaths.length).toBeLessThanOrEqual(MAX_ARRAY_ITEMS);
// The pin counts working-tree files (the provider scans disk and never
// consults git), so untracked local files matching a covered glob fail it.
expect(context?.relatedPaths).toHaveLength(115);
expect(context?.relatedPaths.length).toBeLessThanOrEqual(MAX_ARRAY_ITEMS);
中文说明

[建议] 这个精确固定值度量的是工作树,而不是已提交的树——expandRelatedPaths 通过 readdirSync 扫描磁盘,从不查询 git——因此任何树变化都会触发它,包括未跟踪的本地文件;而 128 项 wire 契约已经由紧接着下方的 toBeLessThanOrEqual(MAX_ARRAY_ITEMS) 一行强制执行。——失败场景:已用探针核实——创建一个匹配已覆盖 glob 的空未跟踪文件(例如 packages/web-shell/client/hooks/ 下的 WIP 文件)就会让这个已提交测试报 "expected 115, received 116" 而失败——与真实契约漂移无法区分——尽管没有任何东西被提交、manifest 也未改动;已提交文件在已覆盖 glob 下的良性增长(新增注释点名 hooks/** 是增长最快的组)同样会强制进行一次仪式性的魔法数字升级,尽管契约行已经强制执行了该上限。建议的修复:要么把精确固定值换成有界区间,让良性增长不再反复改动它;要么——如果保留它是为了检测收缩——在注释中说明它统计的是工作树文件、未跟踪文件会使其失败,避免把这种失败误读为契约违例。

— qwen3.8-max via Qwen Code /review (v0.21.10)

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; Context: This PR narrows the repository review manifest's...: did not execute manifest-repository-context.committed.test.ts — the worktree has no node_modules and I did not install dependencies; every load-bearing asse…; Context: This PR narrows the repository review manifest's...: could not execute manifest-repository-context.committed.test.ts itself — the review worktree has no installed node_modules , so vitest fails at config load. ….

Test Plan (not a blocker): 66 tests pass — this review observed 19355 passed.

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):You are review agent reverse-audit — Reverse audit agen...:none — all checks above completed within budget.;Context: This PR narrows the repository review manifest's...:did not execute manifest-repository-context.committed.test.ts — the worktree has no node_modules and I did not install dependencies; every load-bearing asse…;Context: This PR narrows the repository review manifest's...:could not execute manifest-repository-context.committed.test.ts itself — the review worktree has no installed node_modules , so vitest fails at config load. …

Test Plan(非阻断):66 tests pass — this review observed 19355 passed

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines +199 to +201
const context = provideForRepo([
'packages/core/src/skills/bundled/dataviz/scripts/validate_palette.js',
]);

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.

[Suggestion] R3-1: The changedPaths input here is the exact target of this test's own not.toContain('.../dataviz/scripts/validate_palette.js') assertion below, and expandRelatedPaths excludes changed paths before glob matching — so that negative assertion passes for any glob, including one that matches scripts. The union test in this same file explicitly defends against this confound with synthetic nonexistent probes ("union probes must not be real changed files excluded from relatedPaths"); this assertion walks into it. Probe-verified mutation: widening the manifest with packages/core/src/skills/bundled/*/scripts/* (sentinel map updated to keep the key-equality check green) leaves all 6 committed tests green; switching this input to a file no negative assertion targets flips the test red under the same mutation — Failure scenario: a future manifest edit re-widens relatedPaths to include script material (union stays ~125 ≤ 128) → the pinned policy "nested implementations, tests, references, and scripts stay out of automatic related context" regresses with the whole suite green, because the script file is excluded as the changed input rather than by the glob.

Suggested change
const context = provideForRepo([
'packages/core/src/skills/bundled/dataviz/scripts/validate_palette.js',
]);
const context = provideForRepo([
'packages/core/src/skills/bundled/review/SKILL.md',
]);

The suggested input also keeps the positive toContain('.../dataviz/SKILL.md') genuine — it then resolves through the glob instead of being the changed file. Verified: green on the current manifest, red under the scripts-widening mutation.

中文说明

[建议] R3-1:这里的 changedPaths 输入恰好是下方测试自身 not.toContain('.../dataviz/scripts/validate_palette.js') 断言的目标文件,而 expandRelatedPaths 在 glob 匹配之前就会排除 changed paths——因此这条负向断言对任何 glob 都会通过,包括一个匹配 scripts 的 glob。同一文件中的 union 测试用「不存在的合成探针」明确防住了同样的混淆("union probes must not be real changed files excluded from relatedPaths");这条断言却踩了进去。已通过探针做变异核实:把 manifest 放宽加入 packages/core/src/skills/bundled/*/scripts/*(同时更新 sentinel map 保持键相等检查为绿)后,全部 6 个已提交测试依然为绿;把此输入换成任何负向断言都不针对的文件后,同一变异下测试变红。——失败场景:未来某次 manifest 编辑重新放宽 relatedPaths 以包含 script 文件(并集仍约 125 ≤ 128)→「嵌套的实现、测试、参考资料和脚本不进入自动关联上下文」这一被固定的策略会在整个测试套件保持绿色的情况下回归,因为该 script 文件是作为 changed input 被排除的,而不是被 glob 排除的。

建议的输入同时让正向断言 toContain('.../dataviz/SKILL.md') 保持真实——届时它通过 glob 解析,而不是作为 changed file 出现。已验证:在当前 manifest 下为绿,在 scripts 放宽变异下为红。

— qwen3.8-max via Qwen Code /review (v0.21.10)

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): Context: This PR narrows the repository review manifest's...: did not run manifest-repository-context.committed.test.ts under vitest (no node_modules in worktree or parent checkout; full monorepo npm install not atte….

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):Context: This PR narrows the repository review manifest's...:did not run manifest-repository-context.committed.test.ts under vitest (no node_modules in worktree or parent checkout; full monorepo npm install not atte…

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines +222 to +224
const wildcardPattern = rule.paths.find((pattern) =>
/[?*]/.test(pattern),
);

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.

[Suggestion] R4-1: The rewritten union test picks only each rule's first wildcard paths pattern (.find) and asserts only that it exists and that the probe is absent from disk — it never verifies the constructed probe actually matches that rule. Probe-verified against the real provider: probeFor yields <static-prefix>/probe.txt, which cannot match extension-constrained patterns, and a schema-legal future rule whose first wildcard pattern is extension-constrained and whose static prefix nests under an existing broader rule (e.g. packages/core/src/special/*.json plus packages/core/src/other/** under today's packages/core/src/** rule) is silently skipped by the .find form, while the pre-PR flatMap fired it via its second pattern. — Failure scenario: a future manifest edit adds such a rule → its probe.txt matches none of its patterns and the rule silently falls out of the co-match union → ≤ MAX_ARRAY_ITEMS passes green on an over-cap union → the overflow surfaces only at real review time as the fail-closed relatedPaths exceeds limit error this test exists to catch at manifest-change time.

Probe every wildcard pattern as before while keeping the new non-existence guard, or assert the chosen probe actually resolves its rule (e.g. the single-path provide result contains that rule's sentinel/domain contribution):

const probes = expectedManifest.rules.flatMap((rule) =>
  rule.paths
    .filter((pattern) => /[?*]/.test(pattern))
    .map(probeFor),
);
中文说明

[建议] R4-1:重写后的 union 测试只取每条规则的第一个通配符 paths pattern(.find),且仅断言该 pattern 存在、探针不在磁盘上——从未验证构造出的探针是否真的能命中该规则。已通过真实 provider 探针核实:probeFor 生成 <static-prefix>/probe.txt,无法命中限定扩展名的 pattern;一条 schema 上合法的未来规则——首个通配符 pattern 限定扩展名、且静态前缀嵌套在现有更宽规则之下(例如今天 packages/core/src/** 规则之下的 packages/core/src/special/*.jsonpackages/core/src/other/**)——会被 .find 形式静默跳过,而 PR 之前的 flatMap 会通过它的第二个 pattern 将其命中。——失败场景:未来某次 manifest 编辑加入这样一条规则 → 其 probe.txt 不匹配它的任何 pattern,该规则静默掉出共同命中并集 → ≤ MAX_ARRAY_ITEMS 在超上限的并集上仍然为绿 → 超限只在真实 review 时以 fail-closed 的 relatedPaths exceeds limit 错误浮现,而本测试本应在 manifest 变更时就将其拦住。

建议像之前一样探测所有通配符 pattern(保留新增的「探针不存在」守卫),或断言所选探针确实解析出该规则(例如单路径 provide 的结果包含该规则的 sentinel/domain 贡献),代码示例见上方英文部分。

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines +209 to +211
expect(context?.relatedPaths).not.toContain(
'packages/core/src/skills/bundled/dataviz/references/palette.md',
);

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.

[Suggestion] R4-2: This guard asserts existsSync(...)).toBe(false) for the synthetic changed path, but never asserts that the three negatively-asserted files (dataviz/references/palette.md, dataviz/scripts/validate_palette.js, loop/autonomous-loop.ts) exist. Probe-verified: expandRelatedPaths scans the real worktree and requires isContainedFile/isFile, so a file that no longer exists can never appear in relatedPaths — its not.toContain then passes for any glob, forever. Observed flip: with the target file present the guard bites on a bundled/*/references/* re-widening; after a rename the identical re-widening sails through. — Failure scenario: one of these three skill content files is renamed or deleted (routine churn) → its not.toContain passes regardless of the glob → a later partial re-widening of relatedPaths into nested bundled material (e.g. bundled/*/references/*, 3 files → union 118 ≤ 128, inside the 13-slot headroom) regresses the pinned policy with the whole suite green, because the union-bound test only backstops widenings that cross 128.

Pin each negative target's existence, mirroring the synthetic-path assertion already in this test:

expect(
  existsSync(
    join(repoRoot, 'packages/core/src/skills/bundled/dataviz/references/palette.md'),
  ),
).toBe(true);
expect(
  existsSync(
    join(repoRoot, 'packages/core/src/skills/bundled/dataviz/scripts/validate_palette.js'),
  ),
).toBe(true);
expect(
  existsSync(
    join(repoRoot, 'packages/core/src/skills/bundled/loop/autonomous-loop.ts'),
  ),
).toBe(true);
中文说明

[建议] R4-2:此测试为合成的 changed path 断言了 existsSync(...)).toBe(false),却从未断言三个被负向断言的文件(dataviz/references/palette.mddataviz/scripts/validate_palette.jsloop/autonomous-loop.ts)存在。已通过探针核实:expandRelatedPaths 扫描真实工作树并要求 isContainedFile/isFile,因此一个已不存在的文件永远不可能出现在 relatedPaths 中——它的 not.toContain 对任何 glob 都会通过,且永远如此。观测到的翻转:目标文件存在时,守卫能拦住 bundled/*/references/* 的重新放宽;一旦文件改名,同样的放宽却能畅通无阻。——失败场景:这三个 skill 内容文件中任何一个被改名或删除(内容文件的常规变动)→ 其 not.toContain 无论 glob 如何都通过 → 之后某次把 relatedPaths 部分重新放宽到嵌套 bundled 内容(例如 bundled/*/references/*,3 个文件 → 并集 118 ≤ 128,在 13 个名额余量之内)就会在整个套件保持绿色的情况下让被固定的策略回归,因为 union 上限测试只为超过 128 的放宽兜底。

建议为每个负向目标固定其存在性,与本测试中已有的合成路径断言对称,代码示例见上方英文部分。

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines 215 to 217
expect(context?.relatedPaths).not.toContain(
'packages/core/src/skills/bundled/loop/autonomous-loop.ts',
);

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.

[Suggestion] R4-3: This tripwire samples only three nested files — a references .md, a scripts .js, and one implementation .ts — and never samples the nested test-file class: 6 of the 14 nested non-entrypoint files (loop/autonomous-loop.test.ts, loop/loop-task-file.test.ts, loop/loop-tick-resolver.test.ts, loop/SKILL.test.ts, review/SKILL.test.ts, dataviz/scripts/validate_palette.test.js), nor the review/DESIGN.md reference document. Probe-verified by simulating the future edit: adding packages/core/src/skills/bundled/**/*.test.ts to the manifest, the pinned manifest, and the sentinel map leaves all 6 tests green while the provider provably auto-attaches the 5 nested .test.ts files (union 120/128, inside the bound); the static bundled/review/DESIGN.md entry behaves the same. Applying the fix below makes the exclusion test fail under that mutation. — Failure scenario: a future manifest edit adds packages/core/src/skills/bundled/**/*.test.ts (or a static entry like bundled/review/DESIGN.md) to the core-skills relatedPaths → union ≤ 121, inside the 13-slot headroom, so the bound test passes, and all three existing not.toContain checks pass because none of the sampled paths is a test file → nested skill test material is auto-attached to every bundled-skill review's related context against the policy this test's own comment pins ("deliberately leave nested implementations, tests, references, and scripts to the changed-file diff"), with CI green.

Suggested change
expect(context?.relatedPaths).not.toContain(
'packages/core/src/skills/bundled/loop/autonomous-loop.ts',
);
expect(context?.relatedPaths).not.toContain(
'packages/core/src/skills/bundled/loop/autonomous-loop.ts',
);
expect(context?.relatedPaths).not.toContain(
'packages/core/src/skills/bundled/loop/autonomous-loop.test.ts',
);
中文说明

[建议] R4-3:这个哨兵测试只抽样了三个嵌套文件——一个 references .md、一个 scripts .js、一个实现 .ts——从未抽样嵌套的测试文件类:14 个嵌套非入口文件中有 6 个(loop/autonomous-loop.test.tsloop/loop-task-file.test.tsloop/loop-tick-resolver.test.tsloop/SKILL.test.tsreview/SKILL.test.tsdataviz/scripts/validate_palette.test.js),也没有覆盖 review/DESIGN.md 参考文档。已通过模拟未来编辑做探针核实:把 packages/core/src/skills/bundled/**/*.test.ts 加入 manifest、固定 manifest 和 sentinel map 后,全部 6 个测试仍为绿,而 provider 确实会把 5 个嵌套 .test.ts 文件自动挂进关联上下文(并集 120/128,在上限之内);静态条目 bundled/review/DESIGN.md 的行为相同。应用下方的修复后,同一变异会让排除测试变红。——失败场景:未来某次 manifest 编辑把 packages/core/src/skills/bundled/**/*.test.ts(或 bundled/review/DESIGN.md 这样的静态条目)加入 core-skills 的 relatedPaths → 并集 ≤ 121,在 13 个名额余量之内,上限测试通过;三个既有 not.toContain 检查也全部通过,因为抽样的路径没有一个是测试文件 → 嵌套的 skill 测试内容会在 CI 全绿的情况下被自动挂进每个 bundled-skill review 的关联上下文,违背本测试自身注释所固定的策略(「刻意把嵌套的实现、测试、参考资料和脚本留给 changed-file diff」)。

— qwen3.8-max via Qwen Code /review (v0.21.10)

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed. Suggestions are inline.

中文说明

已审查。 建议见行内评论。

— qwen3.8-max via Qwen Code /review (v0.21.10)

const relatedPathSentinels: Readonly<Record<string, string>> = {
'packages/core/src/config/**': 'packages/core/src/config/config.ts',
'packages/core/src/skills/**':
'packages/core/src/skills/*.ts': 'packages/core/src/skills/skill-manager.ts',

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.

[Suggestion] The new packages/core/src/skills/*.ts relatedPaths glob is guarded only by the single skill-manager.ts sentinel — the other 18 top-level skills files (skill-load.ts, skill-curator.ts, skill-activation.ts, skill-paths.ts, types.ts, index.ts, ...) have no membership assertion, unlike the sibling bundled/*/SKILL.md glob, which the new nested-exclusion test covers exhaustively via a filesystem walk. — Failure scenario: a future budget-balancing edit (exactly like this PR) narrows skills/*.ts to a subset still containing skill-manager.ts (e.g. that file alone) and updates expectedManifest and the sentinel key in the same commit → the automatic core-skills related context silently loses all 18 sibling top-level sources for every skills review, while all 6 committed-manifest tests stay green. This narrowing mutation was applied and verified to survive (6/6 green), and a real-provider probe measured the loss: resolved top-level skills files drop 19 → 1.

Suggested fix — in keeps nested bundled-skill material out of automatic related context, mirror the bundled walk for the top level (flip-checked: with it the mutation fails the suite; the unmuted tree stays green):

const topLevelSources = readdirSync(
  join(repoRoot, 'packages/core/src/skills'),
  { withFileTypes: true },
).filter((entry) => entry.isFile() && entry.name.endsWith('.ts'));
expect(topLevelSources.length).toBeGreaterThan(1);
for (const entry of topLevelSources) {
  expect(context?.relatedPaths).toContain(
    `packages/core/src/skills/${entry.name}`,
  );
}
中文说明

新增的 packages/core/src/skills/*.ts relatedPaths glob 仅由单个哨兵文件 skill-manager.ts 守护——其余 18 个顶层 skills 文件(skill-load.tsskill-curator.tsskill-activation.tsskill-paths.tstypes.tsindex.ts 等)没有任何成员断言;而姊妹 glob bundled/*/SKILL.md 由新的嵌套排除测试通过文件系统遍历做了穷举覆盖。— 失败场景:未来一次预算平衡式修改(与本 PR 完全同类)把 skills/*.ts 收窄为仍包含 skill-manager.ts 的子集(例如只保留这一个文件),并在同一提交里同步更新 expectedManifest 和哨兵键 → 自动附加的 core-skills 关联上下文将在每次 skills 相关 review 中静默丢失全部 18 个兄弟顶层源文件,而全部 6 个 committed-manifest 测试仍然全绿。该收窄变异已实际注入并验证可以存活(6/6 全绿),真实 provider 探针实测损失:解析出的顶层 skills 文件从 19 个降为 1 个。

建议修复——在 keeps nested bundled-skill material out of automatic related context 中为顶层目录复制一份与 bundled 相同的遍历(已做翻转验证:加入后该变异会使测试失败;未注入变异时套件保持全绿),代码见上方英文部分。

— qwen3.8-max via Qwen Code /review (v0.21.10)

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): Change summary: This PR narrows the repository review man...: none — all planned checks completed (second mutation, dropping only the *.ts glob, was skipped as the toContain(topLevelSources) loop makes its detection se….

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):Change summary: This PR narrows the repository review man...:none — all planned checks completed (second mutation, dropping only the *.ts glob, was skipped as the toContain(topLevelSources) loop makes its detection se…

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines +163 to +165
function listFilesRecursively(relativeDirectory: string): string[] {
const files: string[] = [];
for (const entry of readdirSync(join(repoRoot, relativeDirectory), {

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.

[Suggestion] The new listFilesRecursively helper duplicates allFiles() in review-digest-covers-only-bundled.test.ts:67 (same lib/ directory): identical walk semantics (readdirSync(dir, { withFileTypes: true }), recurse on isDirectory(), collect on isFile(), implicitly skip symlinks), differing only in output shape (generator of absolute paths vs sorted repo-relative strings). A shared test-utils.ts already sits beside both tests. — Concrete cost: two copies of the enumeration contract in one directory; if the ground-truth walk ever needs a change (following a symlinked skill directory, skipping a new subtree), one site gets updated and the other does not, and the digest-coverage test and the manifest-context test silently measure different file sets for the same tree while both stay green.

Hoist one walker into the existing shared test-utils.ts and have both tests use it (e.g. keep the generator as the primitive and build the relative sorted list at this call site).

中文说明

新增的 listFilesRecursively 辅助函数与同一 lib/ 目录下 review-digest-covers-only-bundled.test.ts:67 中的 allFiles() 重复:遍历语义完全一致(readdirSync(dir, { withFileTypes: true })、遇到 isDirectory() 递归、遇到 isFile() 收集、隐式跳过符号链接),只有输出形态不同(绝对路径的生成器 vs 排序后的仓库相对路径字符串)。两个测试旁边本来就有一个共享的 test-utils.ts 模块。— 具体代价:同一目录里存在两份“遍历契约”;将来若遍历逻辑需要变更(例如跟随符号链接的 skill 目录、跳过新的子树),只更新其中一处,digest-coverage 测试与 manifest-context 测试就会对同一文件树悄悄测出不同的文件集合,而两边都还是绿的。

建议把其中一份遍历辅助函数提取到现有的共享 test-utils.ts,让两个测试都使用它(例如保留生成器形态作为原语,在本调用处构造相对路径排序列表)。

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines +207 to +210
const patterns = [
...expectedManifest.rules.flatMap((rule) => rule.paths),
'packages/core/src/synthetic/*.json',
];

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.

[Suggestion] The hardcoded 'packages/core/src/synthetic/*.json' pattern belongs to no manifest rule, points at a nonexistent directory, and carries no explanatory comment — unlike every other non-obvious choice in this file (the skills-rule rationale, the union-test alarm comment, the assertion messages). — Concrete cost: this line is the only exercise of the single-* (in-segment wildcard) branch of probeFor and of the provider's paths filter for such a pattern — every committed paths pattern is either a literal or ends in /**. A maintainer editing the manifest cannot distinguish it from stale debris; deleting it silently removes the only single-star paths-pattern coverage, and the test name ("matches every paths pattern") misleads about the extra entry's origin.

Suggested change
const patterns = [
...expectedManifest.rules.flatMap((rule) => rule.paths),
'packages/core/src/synthetic/*.json',
];
const patterns = [
...expectedManifest.rules.flatMap((rule) => rule.paths),
// No committed paths pattern uses a single-star segment; keep one synthetic
// probe so that branch of probeFor and the matcher stays covered.
'packages/core/src/synthetic/*.json',
];
中文说明

硬编码的 'packages/core/src/synthetic/*.json' 模式不属于任何 manifest 规则,指向不存在的目录,也没有解释性注释——这与本文件中其他所有非显而易见的决策都不同(skills 规则的说明、union 测试的告警注释、断言消息)。— 具体代价:这一行是 probeFor*(段内通配符)分支以及 provider paths 过滤器对此类模式的唯一覆盖——所有已提交的 paths 模式要么是字面量,要么以 /** 结尾。后续编辑 manifest 的维护者无法把它和陈旧残留区分开;删除它会悄悄移除唯一的单星 paths 模式覆盖,而测试名(“matches every paths pattern”)还会让人误解这个额外条目的来源。

— qwen3.8-max via Qwen Code /review (v0.21.10)

@yiliang114 yiliang114 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.

The test rework here is thoughtful (synthetic single-star probe, nested-exclusion pins), but note this PR and #9028 both fix the same tripwire (issue #9029: all-rules union 129 > MAX_ARRAY_ITEMS 128) by editing the same pinned manifest — only one can land cleanly.

  • #9028 (subsystem owner) drops the three web-shell e2e relatedPaths globs (22 files; test coverage already surfaced via recommendedTests), union -> ~107.
  • This one narrows skills/** to entrypoints, union -> ~102, but drops nested bundled-skill material (DESIGN.md/references/scripts) from automatic related context — the material reviewers most want when a skill changes.

Deferring to the maintainer's choice on which cut to take; if #9028 lands first, this PR would need to rebase to a test-only change (or close). Not approving yet to avoid signaling two conflicting manifest edits are both merge-ready.

@qwen-code-ci-bot qwen-code-ci-bot 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.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): This PR narrows the repository review manifest's core-ski...: none material — did not verify the "115/128" numeric claim in the comment (cosmetic comment only); did not run the full packages/cli suite (only the two touched…; This PR narrows the repository review manifest's core-ski...: none — did not verify the comment-only "115/128 resolved" figure (cosmetic) and did not run the entire packages/cli suite (only the two files this diff touches)…; You are reviewing PR #9022, which narrows the repository ...: none — all planned checks completed within budget..

中文说明

未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):This PR narrows the repository review manifest's core-ski...:none material — did not verify the "115/128" numeric claim in the comment (cosmetic comment only); did not run the full packages/cli suite (only the two touched…;This PR narrows the repository review manifest's core-ski...:none — did not verify the comment-only "115/128 resolved" figure (cosmetic) and did not run the entire packages/cli suite (only the two files this diff touches)…;You are reviewing PR #9022, which narrows the repository ...:none — all planned checks completed within budget.

— qwen3.8-max via Qwen Code /review (v0.21.10)

Comment on lines +253 to +255
const bundledFiles = [...allFiles(join(repoRoot, BUNDLED_SKILLS_ROOT))]
.map((path) => relative(repoRoot, path))
.sort();

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.

[Critical] R7-1: The new test keeps nested bundled-skill material out of automatic related context feeds platform-dependent relative() output into POSIX-only matching. On Windows, relative(repoRoot, path) yields backslash-separated paths, but the entrypoint regex /^packages\/core\/src\/skills\/bundled\/[^/]+\/SKILL\.md$/ and the POSIX-literal nestedSentinels comparisons only accept /. The provider itself is platform-safe (it builds POSIX paths and rejects \ in isSafeRepositoryRelativePath), so only this new test breaks. — Failure scenario: the required test_windows merge-queue job runs the full packages/cli vitest run; on win32 entrypoints is empty so expect(entrypoints.length).toBeGreaterThan(0) fails, expect(nestedFiles).toEqual(expect.arrayContaining(nestedSentinels)) fails (backslash strings never equal the POSIX literals), and the not.toContain(nestedFile) negatives become vacuous — the PR fails the merge queue on Windows while staying green on Linux/macOS. Probe-verified against Node's path.win32 model over the real tree: current code fails both asserts on win32; normalized code passes.

The fix spans two locations (import and call site), matching the idiom stale-bundle.ts:230 already uses:

import { dirname, join, relative, resolve, sep } from 'node:path';
    const bundledFiles = [...allFiles(join(repoRoot, BUNDLED_SKILLS_ROOT))]
      .map((path) => relative(repoRoot, path).split(sep).join('/'))
      .sort();
中文说明

[Critical] R7-1:新测试 keeps nested bundled-skill material out of automatic related context 把平台相关的 relative() 输出喂给了只接受 POSIX 路径的匹配逻辑。在 Windows 上,relative(repoRoot, path) 返回反斜杠分隔的路径,但入口文件正则 /^packages\/core\/src\/skills\/bundled\/[^/]+\/SKILL\.md$/ 和 POSIX 字面量 nestedSentinels 的比较都只接受 /。provider 自身是平台安全的(内部构造 POSIX 路径,且 isSafeRepositoryRelativePath 会拒绝含 \ 的路径),因此只有这个新测试会出问题。— 失败场景:必需的 test_windows merge-queue 任务会运行完整的 packages/cli vitest run;在 win32 上 entrypoints 为空,expect(entrypoints.length).toBeGreaterThan(0) 失败,expect(nestedFiles).toEqual(expect.arrayContaining(nestedSentinels)) 失败(反斜杠字符串永远不等于 POSIX 字面量),而且 not.toContain(nestedFile) 的负向断言变得形同虚设——PR 会在 Windows 上挂掉 merge queue,而 Linux/macOS 仍然全绿。已用 Node 的 path.win32 模型在真实文件树上做探针验证:当前代码在 win32 下两个断言都失败,归一化后的代码通过。

修复涉及两处(import 与调用处),与 stale-bundle.ts:230 已有的写法一致:

import { dirname, join, relative, resolve, sep } from 'node:path';
    const bundledFiles = [...allFiles(join(repoRoot, BUNDLED_SKILLS_ROOT))]
      .map((path) => relative(repoRoot, path).split(sep).join('/'))
      .sort();

— qwen3.8-max via Qwen Code /review (v0.21.10)

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): Context: This PR narrows the core-skills relatedPaths g...: none — all planned checks completed within budget..

Test Plan (not a blocker): 73 tests pass — this review observed 19356 passed.

中文说明

已审查。

未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):Context: This PR narrows the core-skills relatedPaths g...:none — all planned checks completed within budget.

Test Plan(非阻断):73 tests pass — this review observed 19356 passed

— qwen3.8-max via Qwen Code /review (v0.21.10)

@destire-mio destire-mio changed the title fix(review): keep repository context within file limit chore(review): narrow automatic bundled skill context Aug 13, 2026

@qwen-code-ci-bot qwen-code-ci-bot 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.

Reviewed.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Test Plan (not a blocker): src/commands/review/lib/manifest-repository-context.committed.test.tsno such file or directory; src/commands/review/lib/manifest-repository-context.test.tsno such file or directory; src/commands/review/lib/review-digest-covers-only-bundled.test.tsno such file or directory; 73 tests pass — this review observed 19353 passed.

中文说明

已审查。

未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。

Test Plan(非阻断):src/commands/review/lib/manifest-repository-context.committed.test.tsno such file or directory; src/commands/review/lib/manifest-repository-context.test.tsno such file or directory; src/commands/review/lib/review-digest-covers-only-bundled.test.tsno such file or directory; 73 tests pass — this review observed 19353 passed

— qwen3.8-max via Qwen Code /review (v0.21.10)

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.

3 participants