feat(review): add declarative repository-context manifest - #8401
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
changed the title
|
Re-run — three commits landed since the last pass (which reviewed Template looks good ✓ Problem: feature, not bugfix — and the motivation is observed, not theoretical. The review pipeline has no way to receive repository-native guidance without hardcoding per-repository detection into shared code, and this PR's own history shows the pain: the first iteration inlined OpenJDK knowledge into the planner, and the rework extracts it into a declarative contract any repository can implement. Real workflow gap, named by the maintainer driving the workflow. Direction: internal review infrastructure; the author is a maintainer (admin) who drives that workflow. The generalization remains the right direction over the first iteration: one strict contract, one trust boundary, repository data kept out of the pipeline. Nothing in the CHANGELOG lane argues against it — repo-aware review planning is outside the generic-CLI surface. Size: author is a maintainer, and the only core-path change is bundled review skill documentation (+18/−5 in Approach: scope still feels right — versioned context in the shared plan, one strict wire validator, one provider reading strict JSON at Risk: no elevated risk signals — no changed file matches the revert-correlated path patterns. Moving on to code review. 🔍 中文说明Re-run——自上次审查（ 模板完整 ✓ 问题：功能新增而非 bugfix——动机来自实际观察：review 流水线无法在不把仓库专用识别硬编码进共享代码的前提下接收仓库原生指引；本 PR 的演进正说明了痛点：第一版把 OpenJDK 知识内置进 planner，本次重构抽成任何仓库都可实现的声明式 contract。是驱动该工作流的 maintainer 指出的真实缺口。 方向：内部 review 基础设施；作者是驱动该工作流的 maintainer（admin）。泛化方向正确：一个严格 contract、一个信任边界，仓库数据留在流水线之外。 规模：作者为 maintainer；核心路径上唯一改动是内置 review skill 文档（ 方案：范围合理，且自上次审查以来的增量全部是加固：三个 round-3 Critical（JSON.parse 前的尺寸上限、按长度计费的匹配工作量预算、规则过滤器的预算）、大小写不敏感的跳过目录检查、排序前的目录列表上限、不可读子树 fail-closed、以及 bidi/欺骗性码位拒绝。子系统外唯一改动 风险：无升级风险信号——改动文件均未命中与 revert 相关的高风险路径模式。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo blockers. I re-read the security-critical paths end to end at the new head, focused on what changed since the last pass, and verified the three round-3 Critical fixes in the code rather than in the autofix summary:
The test-isolation commit ( Test evidence — the PR's own CI at the reviewed commitThis is an unattended CI run, so this pass never builds or executes PR code; the evidence below is the PR's own CI at
The recurring Real-scenario testingN/A for this pass — unattended CI invocation; live-product testing belongs to the local path. A sandboxed verification run for this head is already in flight alongside this triage pass (see the verification status comment in this thread). 中文说明代码审查无阻塞项。在新 head 上端到端重读了安全关键路径，重点看上次审查之后的变化，并在代码中（而非 autofix 摘要中）逐一验证了 round-3 的三个 Critical 修复：
测试隔离提交（ 测试证据——被审提交上的 PR 自身 CI本次为无人值守 CI 运行，不构建、不执行 PR 代码；以上证据为 真实场景测试本次不适用——无人值守 CI 调用；真机操作属于本地调用路径。针对该 head 的沙箱验证已随本次 triage 并行运行中（见线程中的验证状态评论）。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean re-review at the new head; the round-3 Criticals are verifiably fixed in code; what remains is non-blocking Suggestions and one defense-in-depth follow-up. This PR has now been through four What keeps this at 4 rather than 5: the round-4 Suggestions are real polish items (the Housekeeping note: the standing Verdict: approve, pinned to the reviewed commit. 中文说明置信度：4/5 —— 新 head 复审干净；round-3 的三个 Critical 已在代码中验证修复；剩余为非阻塞 Suggestions 与一项纵深防御后续项。 本 PR 已经历四轮 之所以是 4 而不是 5：round-4 的 Suggestions 是真实的打磨项（最具体的是 base 模式 identity 读取加 备注：当前挂着的 结论：approve，钉在被审提交上。 — Qwen Code · qwen3.8-max Reviewed at |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
changed the title
Review —
|
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
changed the title
|
@qwen-code /takeover |
|
Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). This run already used the maximum 240 minute timeout. See workflow logs. |
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
qwen-code-dev-bot
added
the
autofix/takeover
Review —
|
Address both maintainer reviews on the repository-context PR: - repo-context: a PR plan whose merge base never resolved (mergeBaseSha: null) now degrades to a null artifact without consulting the worktree, instead of throwing a misleading "invalid plan" error or falling back to the PR head. - Identity reads return the same shape in PR and local modes (CRLF->LF, trimmed) and fail closed: absence yields null, a present-but-unreadable file throws. - Context-required roles can no longer override the roster's effort, topology, and mode gates. - The relatedPaths scan bound rises from 1024 visited entries to 16384 and is documented, so honestly scoped manifests no longer abort reviews. - A present-but-invalid repositoryContext now fails closed in every consumer; the gate no longer silently drops the disclosure. - The duplicated validators and bounds are shared between the wire format and the manifest provider; the context role allow-list is derived from a single const; manifest arrays no longer require hand-sorting (uniqueness only). - Nits: dead mkdir removed, output message names the provider, escape-message fix, unsafe changed paths skip instead of aborting, segment-glob regexes memoised, list helper hoisted. - Docs: user-facing manifest section, trust-boundary residuals and foundation status in the design doc, fail-closed exit guidance in the skill.
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review round summary — PR #8401Both maintainer reviews were triaged point by point. All actionable findings are Review 1 —
|
| Finding | Disposition |
|---|---|
🔴 agent-prompt.ts does not type-check (TS2345) |
Already fixed in 4d5ce3719 — the guard now narrows via isRepositoryContextRoleId(role) before .includes(role). Verified: tsc scoped to packages/cli exits 0. |
| 🟠 Identity file contents differ between PR and local mode; git failures indistinguishable from absence | Fixed. Both reads now return the same shape (CRLF→LF, trimmed); the contract is documented on readIdentityFile. PR mode probes git cat-file -e <sha>:<path> first: absent → null, present-but-failing show → throws (fail closed). Local mode returns null only for ENOENT/ENOTDIR; any other read error (EACCES, EISDIR, …) throws. New tests pin the CRLF/trim behaviour in both modes and the fail-closed unreadable read. |
🟡 Nothing in the pipeline runs repo-context |
Stale — this finding reviewed the first commit; the current tree's SKILL.md already adds the orchestration step for medium/high local, file, and same-repo PR reviews. This round additionally spells out that a non-zero repo-context exit is fail-closed (stop and report). |
| 🟡 Context-required roles bypass the roster's mode/effort policy | Fixed. New contextRoleRunsInThisReview gate: a manifest can require roles the policy already runs, never override it — 6a/6b/6c need high effort and Step 3A, whole-diff walkers need Step 3A, test-matrix needs the 3B fan-out, 1c needs a tree. Tests pin 6a on medium, 1c on diff-only, 2/6a in 3B (excluded) and test-matrix/1b in 3B (honoured). |
🟡 Three different behaviours for one malformed repositoryContext |
Fixed. Uniform fail-closed: repositoryContextGate and the low-signal disclosure now let validation errors propagate (only an unreadable plan yields no disclosure — the coverage gate owns plan validity). The inaccurate "coverage supplies the disclosure" comment is corrected. New test covers a malformed context through the gate. |
Nit: list() duplicated in the two block builders |
Fixed — hoisted to contextList; the block builders now take the validated context. |
Nit: dead mkdirSync(dirname(planPath)) |
Fixed — removed. |
Nit: misleading identity path escapes the worktree for the worktree root |
Fixed — a path resolving to the root is a directory and now falls out of the isFile check as absent, not "escapes"; test added. |
| Nit: backslash in a changed path aborts the whole step | Fixed — changed paths are only glob-matched, never opened, so unsafe-but-real paths are skipped instead of aborting a step that runs on every review; non-string paths (a corrupted plan) still throw. Both cases tested. |
| Nit: no test for plan-write failure after the artifact landed | Fixed — new test (permission-based, skipped as root) proves the artifact lands and the plan stays untouched, so the two never disagree. |
Nit: unverifiedDimensions could inject </details> into the posted body |
Declined with evidence. Every dimension already renders through mdField, which wraps it in an inline code span after stripping backticks and newlines; GitHub-flavoured Markdown renders HTML inside code spans as literal text, so </details> cannot close the fold. |
| Nit: validation runs on every access | Partially fixed. Context is now computed once per prompt build (was up to twice per role). Threading one validated instance through every consumer is an API churn beyond the value at these bounds — declined. |
| Test suggestions (mode/effort role, CRLF identity, malformed gate) | All three added, as part of the fixes above. |
Review 2 — feat(review): add declarative repository-context manifest
| Finding | Disposition |
|---|---|
1. relatedPaths scan bound hard-fails on realistic manifests |
Fixed. The visited-entry bound rises 1024 → 16384 (this repository's entire packages/ tree fits under it with ~2× headroom), is exported for tests, and is documented with the 128-file result bound in the design doc; the throw (fail closed) remains for pathological scans, and the SKILL now says a non-zero exit stops the review. |
2. mergeBaseSha: null produces a misleading hard error |
Fixed. trustedMergeBase now returns three states: local (no base recorded → worktree read), base (trusted sha → base read), none (PR plan whose base never resolved → null artifact, worktree never consulted — the exact trust boundary the review named). baseFetchFailed with a present sha still throws; with null it degrades like fetch-pr does. Tests cover both. |
| 3. Manifest can override the roster's effort/topology gates | Fixed — same policy gate as Review 1 above, with the same tests. |
4. readIdentityFile differs between PR and local paths |
Fixed — same change as Review 1's 🟠, including the absence-vs-failure split (cat-file -e probe; local ENOENT/ENOTDIR only). |
| Duplication between the two validators (~90 lines) | Fixed. validateBoundedString / validateBoundedStringArray and all MAX_* bounds live once in repository-context.ts, parameterised by error prefix; both validators share them, so the bounds cannot drift. |
| Type/runtime drift on the role allow-list | Fixed. One REPOSITORY_CONTEXT_ROLES const (checked satisfies readonly RoleId[]) is the single source for both the type and the guard. |
isRoleId exported but unused |
Fixed — removed entirely (the new guard is pure list membership). |
buildRoleBrief computes context unconditionally / list duplication |
Fixed — computed once per build; block builders take the validated context. |
runRepoContext message and no-op mkdirSync |
Fixed — the message now says null vs the provider name; the dead mkdir is gone. |
Performance: segmentMatches recompiles a RegExp per pair |
Fixed — memoised per pattern segment in a Map. |
Performance: isContainedFile extra syscalls on the walked branch |
Declined. The Dirent check establishes the entry's own type; the realpath of the fully resolved path is what proves containment. The scan is bounded, and splitting the check to save two syscalls per file would special-case security code for no observable gain. |
Security residuals (head-worktree expansion of relatedPaths; local reviews read the manifest from the worktree) |
Documented in the design doc's trust-boundary section, as requested. |
Tests: mergeBaseSha: null; roster/effort interaction |
Added (see above). |
| Tests: Windows coverage | Declined. The Windows-specific branches (drive letters, backslashes, sep) are covered as string inputs in repository-context.test.ts, which the review itself notes is most of the value; this environment cannot run a Windows leg. |
| Docs: no user-facing manifest documentation | Fixed — new "Repository Context" section in docs/users/features/code-review.md. |
| Docs/scope: manifest requires hand-sorted arrays | Fixed. Manifest arrays now need only be unique; the provider sorts and dedupes before the wire format's strict sorted-and-unique validator sees the result. Design doc updated; tests pin both the acceptance of unsorted input and the duplicate rejection. |
| Docs/scope: description reads as if the feature is live | Partially fixed. The design doc now carries an explicit foundation-status paragraph. The PR description itself can only be edited by a maintainer (this workflow performs no GitHub writes) — see "Open item" below. |
Open item (needs a maintainer)
The PR description should state explicitly that this is the foundation: the
contract, command, and consumers ship with tests and the skill step, but no
.qwen/review-context.json exists in-repo yet, so nothing beyond tests runs
end to end until a repository adopts a manifest. The design doc now says this;
the description text is not editable from this workflow.
Verification
Commands actually run this round (post-fix, after final formatting):
npm run build— passed (exit 0)npm run typecheck(repo-wide) — passed (exit 0)npm run lint— passed (exit 0)npx tsc --noEmit -p tsconfig.jsoninpackages/cli— passed (exit 0), confirming the TS2345 blocker stays fixednpx vitest runon the six touched suites (repo-context,roster,repository-context,manifest-repository-context,compose-review,agent-prompt) — 389 passednpx vitest run src/commands/review/check-coverage.test.ts src/commands/review/fetch-pr.test.ts— 97 passednpx vitest run src/commands/review(whole review suite, 54 files) — 1691 passed, 3 skippednpx vitest run src/skillsinpackages/core— 347 passed; 4 failures inskill-manager.test.tsare pre-existing and environmental (user-level home-dir mocks): they reproduce byte-for-byte with this round's only core change (SKILL.md) reverted to HEADnpx prettier --checkon all changed files — passed (after--writeon four of them)- Settings sources untouched →
npm run generate:settings-schemanot applicable; changed behaviour is unit-covered, not bundled-CLI-only → no integration run required
中文说明
Review 轮次总结 — PR #8401
两条 maintainer review 已逐条分诊。所有可执行的发现均已在提交 f27c77de2 中处理；两条发现附证据拒绝；一项文档诉求需要 maintainer 手工编辑 PR 描述（本工作流无权编辑）。自动 reviewer 本轮超时，没有产出需要处理的发现。
Review 1 — feat(review): add repository context foundation
| 发现 | 处理 |
|---|---|
🔴 agent-prompt.ts 类型检查失败（TS2345） |
已在此前修复（4d5ce3719）——守卫现在先经 isRepositoryContextRoleId(role) 收窄再调用 .includes(role)。已验证：packages/cli 范围内的 tsc 退出码为 0。 |
| 🟠 PR 与 local 模式下 identity 文件内容不一致；git 失败与"文件不存在"无法区分 | 已修复。 两种读取现在返回相同形状（CRLF→LF、去首尾空白），契约写入 readIdentityFile 的文档注释。PR 模式先用 git cat-file -e <sha>:<path> 探测：不存在 → null；存在但 show 失败 → 抛错（fail-closed）。local 模式仅对 ENOENT/ENOTDIR 返回 null，其他读取错误（EACCES、EISDIR 等）一律抛出。新增测试固化两种模式下的 CRLF/trim 行为，以及不可读文件 fail-closed。 |
🟡 流水线里没有任何地方执行 repo-context |
已过期 —— 该发现针对的是第一个提交；当前树的 SKILL.md 已经为 medium/high 的 local、file、同仓 PR review 添加了编排步骤。本轮另外明确写出：repo-context 非零退出是 fail-closed（停止并上报）。 |
| 🟡 context 要求的 role 绕过 roster 的 mode/effort 策略 | 已修复。 新增 contextRoleRunsInThisReview 门禁：manifest 只能要求策略本来就会运行的 role，不能覆盖策略 —— 6a/6b/6c 需要 high effort 且 Step 3A，whole-diff walker 需要 Step 3A，test-matrix 需要 3B fan-out，1c 需要 tree。测试固化：medium 下的 6a、diff-only 下的 1c、3B 下的 2/6a（被排除）与 test-matrix/1b（被接受）。 |
🟡 一个非法 repositoryContext 产生三种不同行为 |
已修复。 统一 fail-closed：repositoryContextGate 与 low-signal 披露现在让校验错误向上传播（只有不可读的 plan 才不披露 —— plan 合法性由 coverage 门禁负责）。不准确的"coverage 会提供披露"注释已更正。新增测试覆盖非法 context 经 gate 抛错。 |
细节：list() 在两个 block builder 中重复 |
已修复 —— 提升为 contextList；block builder 改为接收已校验的 context。 |
细节：死代码 mkdirSync(dirname(planPath)) |
已修复 —— 删除。 |
细节：路径解析到 worktree 根时报 identity path escapes the worktree，信息误导 |
已修复 —— 解析到根的路径是目录，现在走 isFile 检查按"不存在"处理，而不是报"逃逸"；已加测试。 |
| 细节：含反斜杠的 changed path 会中止整个步骤 | 已修复 —— changed path 只参与 glob 匹配、从不被打开，因此不安全但真实存在的路径改为跳过，而不是中止这个每次 review 都会执行的步骤；非字符串 path（plan 损坏）仍然抛错。两种情形均有测试。 |
| 细节：artifact 落盘后 plan 写入失败没有测试 | 已修复 —— 新增测试（基于权限，root 下跳过）证明 artifact 落盘、plan 原样保留，两者永不失配。 |
细节：unverifiedDimensions 可能向已发布 body 注入 </details> |
附证据拒绝。 每个 dimension 都已经过 mdField 渲染：先去掉反引号与换行，再包进行内代码 span；GFM 会把代码 span 内的 HTML 渲染为纯文本，</details> 无法闭合折叠块。 |
| 细节：每次访问都重新校验 | 部分修复。 context 现在每次 prompt 构建只计算一次（此前每个 role 最多两次）。把同一个已校验实例穿透到所有消费方属于 API 变动，在当前上界下收益不抵成本 —— 拒绝。 |
| 测试建议（mode/effort role、CRLF identity、非法 gate） | 三项均已补充，随上述修复一并落地。 |
Review 2 — feat(review): add declarative repository-context manifest
| 发现 | 处理 |
|---|---|
1. relatedPaths 扫描上限在真实 scope 的 manifest 上硬失败 |
已修复。 访问条目上限从 1024 提高到 16384（本仓库整个 packages/ 树都在其下，约 2 倍余量），导出供测试使用，并与 128 个文件的结果上限一起写入设计文档；病态扫描仍然抛错（fail-closed），SKILL 现在明确非零退出即停止 review。 |
2. mergeBaseSha: null 产生误导性的硬错误 |
已修复。 trustedMergeBase 现在返回三态：local（plan 未记录 base → 读 worktree）、base（可信 sha → 只读 base）、none（PR plan 但 base 从未解析成功 → 写 null 制品且绝不读 worktree —— 正是 review 指出的那条信任边界）。baseFetchFailed 且 sha 存在仍然抛错；sha 为 null 时像 fetch-pr 一样降级。两种情形均有测试。 |
| 3. manifest 可以覆盖 roster 的 effort/topology 门禁 | 已修复 —— 与 Review 1 同一策略门禁，同一批测试。 |
4. readIdentityFile 在 PR 与 local 路径下行为不一致 |
已修复 —— 与 Review 1 的 🟠 为同一改动，包括"不存在 vs 失败"的区分（cat-file -e 探测；local 仅认 ENOENT/ENOTDIR）。 |
| 两个校验器约 90 行重复 | 已修复。 validateBoundedString / validateBoundedStringArray 与全部 MAX_* 上界只在 repository-context.ts 中存在一份，以错误前缀参数区分；两个校验器共享，上界不再可能漂移。 |
| role 白名单的类型/运行时漂移 | 已修复。 单一的 REPOSITORY_CONTEXT_ROLES 常量（经 satisfies readonly RoleId[] 编译期检查）同时作为类型与运行时守卫的唯一来源。 |
isRoleId 导出但未使用 |
已修复 —— 整体删除（新守卫是纯列表成员判断）。 |
buildRoleBrief 无条件计算 context / list 重复 |
已修复 —— 每次构建只计算一次；block builder 接收已校验的 context。 |
runRepoContext 输出信息与 no-op mkdirSync |
已修复 —— 输出现在区分 null 与 provider 名；死代码 mkdir 已删除。 |
性能：segmentMatches 每对比较都新建 RegExp |
已修复 —— 按 pattern segment 用 Map 记忆化。 |
性能：walk 分支里 isContainedFile 的多余 syscall |
拒绝。 Dirent 检查只能证明条目本身的类型；完整解析路径的 realpath 才是包含性证明。扫描本身有上限，为每文件省两个 syscall 而给安全代码开特殊分支，收益不可观测。 |
安全残留（relatedPaths 在 head worktree 展开；local review 从 worktree 读 manifest） |
已按要求写入设计文档的信任边界小节。 |
测试：mergeBaseSha: null；roster/effort 交互 |
已补充（见上）。 |
| 测试：Windows 覆盖 | 拒绝。 Windows 特定分支（盘符、反斜杠、sep）已在 repository-context.test.ts 中以字符串输入覆盖 —— review 自己也承认这已覆盖大部分价值；本环境无法运行 Windows leg。 |
| 文档：manifest 没有用户文档 | 已修复 —— docs/users/features/code-review.md 新增 "Repository Context" 小节。 |
| 文档/scope：manifest 要求手工排序数组 | 已修复。 manifest 数组现在只要求唯一；provider 会在 wire format 的严格"排序且唯一"校验之前完成排序与去重。设计文档已更新；测试固化"接受乱序输入"与"拒绝重复"两条。 |
| 文档/scope：描述读起来像功能已上线 | 部分修复。 设计文档现在有明确的 foundation 状态段落。PR 描述本身只能由 maintainer 编辑（本工作流不做任何 GitHub 写操作）—— 见下方"待办"。 |
待办（需要 maintainer）
PR 描述应明确写出这是 foundation：contract、命令与消费方随测试和 skill 步骤一起交付，但仓库内尚无 .qwen/review-context.json，在有任何仓库采用 manifest 之前，除测试外没有端到端运行。设计文档已写明这一点；描述文本本工作流无法编辑。
验证
本轮实际执行的命令（修复后、最终格式化之后）：
npm run build— 通过（退出码 0）npm run typecheck（repo 范围）— 通过（退出码 0）npm run lint— 通过（退出码 0）packages/cli下npx tsc --noEmit -p tsconfig.json— 通过（退出码 0），确认 TS2345 阻断项保持已修复- 对 6 个被改动测试套件（
repo-context、roster、repository-context、manifest-repository-context、compose-review、agent-prompt）运行npx vitest run— 389 通过 npx vitest run src/commands/review/check-coverage.test.ts src/commands/review/fetch-pr.test.ts— 97 通过npx vitest run src/commands/review（整个 review 套件，54 个文件）— 1691 通过，3 跳过packages/core下npx vitest run src/skills— 347 通过；skill-manager.test.ts的 4 个失败为既有环境问题（user-level home 目录 mock）：把本轮唯一的 core 改动（SKILL.md）回退到 HEAD 后逐字节复现- 对全部改动文件运行
npx prettier --check— 通过（其中 4 个文件先经--write格式化） - 未触碰 settings 源 → 无需
npm run generate:settings-schema；改动行为均有单测覆盖、并非只经 bundled CLI 行使 → 无需集成测试
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。
Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。
🧠 Handled by Qwen Code · model/模型 qwen3.8-max
|
Qwen Code review timed out. Qwen review timed out after 14400 seconds (of the 240-minute budget). This run already used the maximum 240 minute timeout. See workflow logs. |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No code changes this roundOutcome: no-action — nothing was committed. What the new feedback saysThe only new feedback since the last evaluation is a bot notice that the automated
There are no code-level findings to address: no inline review comments, no Why this is not a defect in this PR
State of the actual CIAll substantive checks are green — build/test ( What was doneNo code changes were made this round, and nothing was committed. There is no safe, Suggested next step (maintainer decision)This is a review-infrastructure timing issue, not a code defect. Reasonable
These are maintainer judgement calls; none is a code fix within this PR. 中文说明本轮无需修改代码结果:no-action(无操作)—— 未提交任何内容。 新反馈的内容自上次评估以来,唯一的新反馈是一条机器人通知,说明自动 Qwen Code 代码审查
没有任何代码层面的问题需要处理:没有内联审查评论、没有包含审查发现的评审 为什么这不是本 PR 的缺陷
实际 CI 的状态所有实质性检查均为绿色——构建/测试( 本轮做了什么本轮未做任何代码修改,也未提交任何内容。没有任何安全、且在范围内的代码改动 建议的下一步(由维护者决定)这是审查基础设施的计时问题,不是代码缺陷。合理的后续操作:
这些都属于维护者的判断;其中没有一项是本 PR 内的代码修复。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /review --timeout=240 |
|
🔀 Base updated: red check(s) [review-pr] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [review-pr] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round report — no action neededFeedback triage
What this round changedNothing in the working tree. This round did run Note for maintainers (workflow, out of scope for this PR)The verify gate checks settings-schema freshness before any build, on a leg whose setup never builds Verification
中文说明Autofix 本轮报告 — 无需任何改动反馈分类
本轮改动工作树没有任何改动。本轮在任务工作区中执行了 给维护者的说明（属于工作流问题，超出本 PR 范围）验证门禁在任何构建之前检查 settings-schema 的新鲜度，而该分支任务的初始化从不构建 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): src/commands/review.test.ts — no such file or directory; src/commands/review/repo-context.test.ts — no such file or directory; src/commands/review/lib/repository-context.test.ts — no such file or directory; src/commands/review/lib/manifest-repository-context.test.ts — no such file or directory; src/commands/review/lib/roster.test.ts — no such file or directory; and 4 more.
中文说明
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：src/commands/review.test.ts — no such file or directory; src/commands/review/repo-context.test.ts — no such file or directory; src/commands/review/lib/repository-context.test.ts — no such file or directory; src/commands/review/lib/manifest-repository-context.test.ts — no such file or directory; src/commands/review/lib/roster.test.ts — no such file or directory; and 4 more。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| "paths": ["packages/*/src/**"], | ||
| "relatedPaths": ["packages/*/src/**"], |
There was a problem hiding this comment.
[Suggestion] R2-2: The design doc's canonical example manifest fails closed on the very repository the doc names as calibration base and first adopter: relatedPaths: ["packages/*/src/**"] resolves to ~3,659 files here — 28× past the documented 128-resolved-file bound. This is the residual example-scope half of round-1 Critical R1-2 (the scan-cap half is fixed; this sibling entrance is not). — Failure scenario: probe-verified at the reviewed commit: feeding the doc's example manifest verbatim through manifestRepositoryContextProvider.provide against this checkout throws repository context manifest relatedPaths exceeds limit in 9 ms. A maintainer adopting the feature by copying the canonical example commits a manifest under which every matching medium/high review exits non-zero at repo-context, and the SKILL.md contract this PR adds ("a NON-ZERO exit is fail-closed — stop the review and report it") halts the review. — Suggested fix: scope the example honestly (e.g. relatedPaths: ["packages/cli/src/commands/review/**"] resolves under 128 files on this repo), or add one sentence noting that wildcard relatedPaths are subject to the 128 resolved-file cap and this scope exceeds it on a repository this size.
中文说明
设计文档的规范示例 manifest 在文档自己指名的校准仓库兼首个采用者上 fail-closed：relatedPaths: ["packages/*/src/**"] 在本仓库解析出约 3,659 个文件——超出文档声明的 128 个解析文件上限 28 倍。这是第 1 轮 Critical R1-2 残留的"示例作用域"那一半（扫描上限那一半已修复；这个兄弟入口没有）。— 失败场景：已在被审 commit 上探测验证：把文档示例 manifest 原样喂给 manifestRepositoryContextProvider.provide，在本检出上 9 ms 内抛出 repository context manifest relatedPaths exceeds limit。维护者照抄规范示例来采用该特性，提交的 manifest 会让每一次匹配的 medium/high review 在 repo-context 非零退出，而本 PR 新增的 SKILL.md 契约（"非零退出即 fail-closed——停止 review 并报告"）会中止整个 review。— 建议修复：给示例一个诚实的作用域（例如 relatedPaths: ["packages/cli/src/commands/review/**"] 在本仓库解析出的文件数低于 128），或者加一句说明：通配 relatedPaths 受 128 解析文件上限约束，且该作用域在本仓库这种规模下会超限。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| brief.reviewsCode || | ||
| (isRepositoryContextRoleId(role) && | ||
| repositoryContext?.requiredAgents.includes(role)) |
There was a problem hiding this comment.
[Suggestion] R2-24: No negative test pins that roles outside the code-reviewing set and outside the allow-list receive no context block; the mutation brief.reviewsCode || → true || ships green (186/186). Scope corrected from the round-2 finder's claim: verify and reverse-audit have reviewsCode: true and legitimately receive the block; the affected role is '0' (Agent 0), plus 'test-matrix' when not in requiredAgents. — Failure scenario: under that mutation Agent 0 — whose brief explicitly says its scope is issue fidelity, not code review — gets the full repository guidance block whenever the plan carries context (a reachable state for every review of a repository that adopted a manifest), and the existing test would not fail. — Suggested fix: in the existing agent-prompt case, add expect(buildRoleBrief(contextPlan, '0')).not.toContain('Example project repository context').
中文说明
没有负向测试固化"代码审查集合之外、允许列表之外的 role 不应收到 context 块"；突变 brief.reviewsCode || → true || 可以绿灯上线（186/186）。对第 2 轮 finder 的说法做了范围修正：verify 与 reverse-audit 的 reviewsCode: true，本来就正当收到该块；受影响的是 '0'（Agent 0），以及不在 requiredAgents 中时的 'test-matrix'。— 失败场景：该突变下，Agent 0——其 brief 明确说自己的职责是 issue fidelity 而非代码审查——会在 plan 携带 context 时（对任何采用了 manifest 的仓库的每次 review 都可达）拿到完整的仓库指导块，而现有测试不会失败。— 建议修复：在现有 agent-prompt 用例中增加 expect(buildRoleBrief(contextPlan, '0')).not.toContain('Example project repository context')。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // 6d. Repository proof boundaries (non-capping) — dimensions the context | ||
| // planner recommends disclosing without claiming the code is defective. | ||
| clauses.push(...repositoryContextBlock); |
There was a problem hiding this comment.
[Suggestion] R2-23: Of the three render sites for the proof-boundary disclosure (REQUEST_CHANGES at ~1234, APPROVE at ~1257, COMMENT clauses here), only the APPROVE path has a composeReview-level test. — Failure scenario: mutation applied and run: deleting the block from the REQUEST_CHANGES site leaves 175/175 green; deleting the COMMENT site likewise. A review whose verdict is REQUEST_CHANGES, or an APPROVE capped to COMMENT, for a repository with unverifiedDimensions posts a body with no "Repository proof boundary" disclosure — precisely the verdicts where the reader most needs to know which dimensions the repository itself marked unverified. — Suggested fix: add two cases to the "repository context proof boundary" describe: coveredPlan plus a confirmed body Critical → REQUEST_CHANGES body contains the disclosure; and a cap-inducing variant → the resulting COMMENT body contains it.
中文说明
证明边界披露的三个渲染点（REQUEST_CHANGES 约 1234 行、APPROVE 约 1257 行、此处 COMMENT 子句）中，只有 APPROVE 路径有 composeReview 级测试。— 失败场景：已施加突变并运行：从 REQUEST_CHANGES 点删掉该块，175/175 全绿；删 COMMENT 点同样全绿。带 unverifiedDimensions 的仓库，其 REQUEST_CHANGES 或被 cap 成 COMMENT 的 APPROVE 的 review body 将没有 "Repository proof boundary" 披露——恰恰是读者最需要知道仓库自己标记了哪些未验证维度的裁决。— 建议修复：在 "repository context proof boundary" describe 中新增两个用例：coveredPlan 加一个已确认的 body Critical → REQUEST_CHANGES body 含披露；以及一个诱发 cap 的变体 → 得到的 COMMENT body 含披露。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| ])('fails closed for %s', (_name, content) => { | ||
| expect(() => provide(temp(), ['src/change.ts'], content)).toThrow(); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R2-28: The rules.length > MAX_RULES rejection (129 rules → rules is invalid) has no test; the suite pins the accept side at exactly 128 rules (the dedup test) but never the reject side. — Failure scenario: mutation applied and run: deleting the clause leaves 63/63 green. No other bound compensates — paths: [] passes validateBoundedStringArray and contributes nothing to the total-paths cap, so an unbounded rule count parses through the full per-rule loop. MAX_RULES is the parse-time/memory bound protecting the step from adversarial manifests — the design doc's explicit threat model for local reviews. — Suggested fix: add a reject-side pin: manifest({ rules: Array.from({ length: 129 }, () => ({ paths: [] })) }) → .toThrow('rules is invalid').
中文说明
rules.length > MAX_RULES 的拒绝（129 条规则 → rules is invalid）没有测试；套件只在恰好 128 条规则处固化了接受侧（去重测试），拒绝侧从未固化。— 失败场景：已施加突变并运行：删除该子句后 63/63 全绿。没有其他上界兜底——paths: [] 能通过 validateBoundedStringArray 且对总 paths 上限贡献为 0，因此无上限的规则数会完整走完逐规则校验循环。MAX_RULES 正是保护该步骤免受对抗性 manifest 影响的解析期/内存上界——设计文档为本地 review 明确的威胁模型。— 建议修复：增加拒绝侧固化：manifest({ rules: Array.from({ length: 129 }, () => ({ paths: [] })) }) → .toThrow('rules is invalid')。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const deps = join(worktree, 'src', 'node_modules'); | ||
| mkdirSync(deps, { recursive: true }); | ||
| for (let index = 0; index < MAX_GLOB_CANDIDATES; index++) { |
There was a problem hiding this comment.
[Suggestion] R2-26: 'never descends into dependency or build-output trees' claims the whole SKIPPED_DIRECTORIES contract but exercises only node_modules and dist; removing any of the other six members (.git, .next, .turbo, coverage, out, target) ships green. — Failure scenario: mutation applied and run: removing '.git' leaves 54/54 green (fixtures plant only src/node_modules and src/dist). The regression later descends into a nested skip-shape tree reached by an ordinary glob (a submodule's .git, a large coverage/target tree under packages/**), exhausts the 16,384-entry ceiling mid-scan, and a legal manifest fails the review closed with scan exceeds limit. — Suggested fix: parameterize over all eight skip-set names (it.each(['.git', '.next', '.turbo', 'coverage', 'dist', 'node_modules', 'out', 'target'])), planting MAX_GLOB_CANDIDATES files under src/<name> and expecting ['src/keep.ts'].
中文说明
'never descends into dependency or build-output trees' 声称覆盖整个 SKIPPED_DIRECTORIES 契约，但实际只演练了 node_modules 与 dist；删除其余六个成员（.git、.next、.turbo、coverage、out、target）中任何一个都能绿灯上线。— 失败场景：已施加突变并运行：删除 '.git' 后 54/54 全绿（fixture 只种了 src/node_modules 与 src/dist）。该回归之后会下钻到被普通 glob 到达的嵌套同形目录树（子模块的 .git、packages/** 下的大型 coverage/target 树），在扫描中途耗尽 16,384 条目上限，使合法 manifest 以 scan exceeds limit fail-closed。— 建议修复：对全部 8 个跳过名参数化（it.each(['.git', '.next', '.turbo', 'coverage', 'dist', 'node_modules', 'out', 'target'])），在 src/<name> 下种 MAX_GLOB_CANDIDATES 个文件并期望 ['src/keep.ts']。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| expect(option.mock.calls.map(([name]) => name)).toEqual([ | ||
| 'plan', | ||
| 'worktree', |
There was a problem hiding this comment.
[Suggestion] R2-21: The test titled 'declares all required command options' only pins option names; the demandOption: true flags — the part that makes them required — are never asserted. — Failure scenario: mutation applied and run: dropping all three demandOption: true flags leaves the whole review suite green (60 files / 2,068 tests). Runtime consequence verified directly: qwen review repo-context with a missing argument then loses yargs' clean "Missing required argument" usage error and the handler dies inside resolve(args.plan) with TypeError [ERR_INVALID_ARG_TYPE] — a raw stack replacing the exit-1 usage message. — Suggested fix: assert the config too: expect(option.mock.calls.map(([name, config]) => [name, (config as { demandOption?: boolean }).demandOption])).toEqual([['plan', true], ['worktree', true], ['out', true]]);
中文说明
标题为 'declares all required command options' 的测试只固化了选项名；demandOption: true 标志——让它们成为"必需"的那部分——从未被断言。— 失败场景：已施加突变并运行：删除全部三个 demandOption: true 后整个 review 套件保持绿色（60 个文件 / 2,068 个测试）。运行时后果已直接验证：缺参数调用 qwen review repo-context 不再得到 yargs 清晰的 "Missing required argument" 用法错误，handler 会在 resolve(args.plan) 内以 TypeError [ERR_INVALID_ARG_TYPE] 死掉——用原始堆栈替代了 exit-1 的用法信息。— 建议修复：把配置也断言上：expect(option.mock.calls.map(([name, config]) => [name, (config as { demandOption?: boolean }).demandOption])).toEqual([['plan', true], ['worktree', true], ['out', true]]);
— qwen3.8-max via Qwen Code /review (v0.21.5)
| if (segment === '' || segment === '.') continue; | ||
| if (segment === '..') { |
There was a problem hiding this comment.
[Suggestion] R2-7: resolveTreeSymlinkTarget silently drops empty segments, so a committed symlink target with a trailing slash that the filesystem cannot resolve (ENOTDIR) still resolves to content in base mode — the opposite-direction twin of R2-3, breaking the "identical in every mode" invariant. — Failure scenario: probe-verified by run: a repo commits .qwen/review-context.json → manifest.json/ (trailing slash; manifest.json a regular file — broken on disk). Base mode returns the full context artifact; local mode returns null (realpathSync fails ENOTDIR). Identity diverges by mode for the same repository; a provider exact-comparing an identity file gets content in one mode and null in the other. Content still comes from the trusted base, so this is a parity defect, not a trust violation. — Suggested fix: treat a target ending in / as requiring the resolved entry to be a directory — return null (→ absent) when the final resolved entry is a blob — and add a test pinning one chosen behavior for both branches.
中文说明
resolveTreeSymlinkTarget 会静默丢弃空段，因此文件系统无法解析（ENOTDIR）的带尾斜杠 symlink 目标，在 base 模式下仍能解析出内容——这是 R2-3 的反方向孪生体，破坏了"两种模式完全一致"的不变量。— 失败场景：已通过运行探测验证：仓库提交 .qwen/review-context.json → manifest.json/（尾斜杠；manifest.json 是普通文件——在磁盘上是坏链接）。base 模式返回完整 context 制品；local 模式返回 null（realpathSync 报 ENOTDIR）。同一仓库的 identity 因模式而分叉；做精确比对的 provider 会在一种模式拿到内容、另一种模式拿到 null。内容仍来自可信 base，所以这是对等性缺陷，不是信任违规。— 建议修复：把以 / 结尾的目标视为要求解析出的条目必须是目录——最终条目是 blob 时返回 null（按缺席处理）——并新增测试为两个分支固化同一种选定行为。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| for (let hop = 0; hop < MAX_IDENTITY_SYMLINK_HOPS; hop++) { | ||
| const entry = baseTreeEntry(worktree, mergeBase, path); | ||
| if (entry === null) return null; |
There was a problem hiding this comment.
[Suggestion] R2-3: Base mode cannot follow a symlinked intermediate path component, while the worktree branch does — the two identity readers diverge, contrary to the "mirroring the worktree branch entry by entry" comment and the "identical in every mode" contract. — Failure scenario: probe-verified in a scratch repo: a repository commits .qwen as a symlink to an in-tree directory containing review-context.json. Local review: realpathSync follows the intermediate link, containment passes, context produced. PR review: git ls-tree <base> -- .qwen/review-context.json exits 0 with empty output (git pathspecs never descend through symlink entries), so readBaseIdentity returns null — a null context for the same repository. Direction is fail-safe (base reads strictly less, never more), so this is a consistency gap, not a trust hole. — Suggested fix: walk path components in base mode (resolve each directory component with ls-tree, following 120000 entries under the same containment rule), or amend the readBaseIdentity doc comment to name the divergence so the "entry by entry" promise doesn't mislead the next provider author.
中文说明
base 模式无法跟随中间路径组件上的 symlink，而 worktree 分支可以——两个 identity 读取器因此分叉，与"逐条目对齐 worktree 分支"的注释和"两种模式下完全一致"的契约不符。— 失败场景：已在临时仓库探测验证：仓库把 .qwen 提交为指向树内某目录（内含 review-context.json）的 symlink。本地 review：realpathSync 跟随中间链接，包含性检查通过，正常产出 context。PR review：git ls-tree <base> -- .qwen/review-context.json 退出码 0 但输出为空（git pathspec 从不穿过 symlink 条目下钻），于是 readBaseIdentity 返回 null——同一个仓库得到 null context。方向是 fail-safe 的（base 只会读得更少，不会更多），所以这是一致性缺口，不是信任漏洞。— 建议修复：在 base 模式逐组件遍历路径（用 ls-tree 解析每个目录组件，按同一包含规则跟随 120000 条目），或者修改 readBaseIdentity 的文档注释，写明这一分叉，免得"逐条目对齐"的承诺误导下一个 provider 作者。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| if (entry.type !== 'blob') return null; | ||
| return normalizeIdentityContent(readBaseBlob(worktree, mergeBase, path)); |
There was a problem hiding this comment.
[Suggestion] R2-20: The base-mode guard that maps "directory or gitlink at the identity path" to null (the mirror of worktree mode's isFile() === false) has no test; deleting the clause ships green. — Failure scenario: git fact verified in a scratch repo: git show <base>:<dir> exits 0 and emits the tree listing. Mutation applied: with the guard deleted, a repository committing a directory at .qwen/review-context.json makes the listing text reach parseManifest, which throws "manifest is not valid JSON"; runRepoContext exits non-zero and per SKILL.md ("a NON-ZERO exit is fail-closed — stop the review") the orchestrator stops the entire review — where the correct behavior (and worktree mode's: statSync().isFile() false → null) is a clean degrade to a null artifact. 22/22 tests pass under the mutation. — Suggested fix: add a test that commits a directory at .qwen/review-context.json at the merge base and asserts the artifact is null (not a throw) in PR mode.
中文说明
把"identity 路径上是目录或 gitlink"映射为 null 的 base 模式守卫（worktree 模式 isFile() === false 的镜像）没有测试；删掉该子句也能绿灯上线。— 失败场景：已在临时仓库验证 git 事实：git show <base>:<dir> 退出码 0 并输出树清单。已施加突变：删除守卫后，在 .qwen/review-context.json 提交目录的仓库会让清单文本流入 parseManifest，抛出 "manifest is not valid JSON"；runRepoContext 非零退出，按 SKILL.md（"非零退出即 fail-closed——停止 review"）orchestrator 会停止整个 review——而正确行为（也是 worktree 模式的行为：statSync().isFile() 为 false → null）是干净地降级为 null 制品。突变下 22/22 测试全部通过。— 建议修复：新增测试，在 merge base 提交一个目录作为 .qwen/review-context.json，断言 PR 模式下产物为 null（而非抛错）。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| It writes the diff to `.qwen/tmp/qwen-review-<target>-diff.txt` and emits the same report `fetch-pr` does (`diffPathAbsolute`, `chunks[]`, `files[]`, the topology counts), plus two fields of its own: | ||
| It writes the diff to `.qwen/tmp/qwen-review-<target>-diff.txt` and emits the same report `fetch-pr` does (`diffPathAbsolute`, `chunks[]`, `files[]`, the topology counts), plus two fields of its own. | ||
|
|
||
| At **medium or high** effort, for local, file-path, and same-repository PR reviews, attach declarative repository context before launching agents: |
There was a problem hiding this comment.
[Suggestion] R2-9: The new repo-context step's only ordering anchor is "before launching agents", but its output is consumed at brief-generation time — and the diff does not update the normative ordering sentence ("The only orderings that matter: fetch-pr before all of them…, and agent-prompt --roster after the rules load"), which now asserts an exhaustive ordering spec missing a real constraint: repo-context must precede agent-prompt (roster AND briefs). — Failure scenario: the doc-compliant interleaving fetch-pr → batch {pr-context, comment-status, rules load} → agent-prompt --roster ("after the rules load" ✓) → repo-context ("before launching agents" ✓) → fan-out builds roster and briefs with plan.repositoryContext === undefined: roster.ts:278-279 iterates repositoryContext?.requiredAgents ?? [] → manifest-required agents never launch; agent-prompt.ts skips the context blocks → agents run without the manifest's guidance — while compose-review's repositoryContextGate still posts proof-boundary notes asserting boundaries the agents never received. One compliant ordering silently no-ops the feature this PR adds. — Suggested fix: amend the ordering sentence (e.g. "…and repo-context before agent-prompt --roster, which bakes the manifest's required agents and context blocks into the roster and every brief"), and/or fold repo-context into the one-response batch — it is mutually independent of pr-context, comment-status, and the rules load.
中文说明
新增 repo-context 步骤唯一的顺序锚点是"在启动 agent 之前"，但它的产物在生成 brief 时就被消费——而 diff 没有更新那句规范性顺序说明（"The only orderings that matter: fetch-pr before all of them…, and agent-prompt --roster after the rules load"），该句现在声称是一份穷尽的顺序规格，却缺了一条真实约束：repo-context 必须先于 agent-prompt（roster 和 brief 都是）。— 失败场景：按文档合规的交错 fetch-pr → 批处理 {pr-context、comment-status、rules load} → agent-prompt --roster（满足"在 rules load 之后"✓）→ repo-context（满足"在启动 agent 之前"✓）→ fan-out，会在 plan.repositoryContext === undefined 时构建 roster 与 brief：roster.ts:278-279 遍历 repositoryContext?.requiredAgents ?? [] → manifest 要求的 agent 不会被启动；agent-prompt.ts 跳过 context 块 → agent 在没有 manifest 指导的情况下运行——而 compose-review 的 repositoryContextGate 仍会发布证明边界说明，宣称那些 agent 从未收到的边界。一种合规的顺序就能把本 PR 新增的特性静默变成空操作。— 建议修复：修改顺序句（例如"……以及 repo-context 先于 agent-prompt --roster，后者把 manifest 要求的 agent 与 context 块烘进 roster 和每个 brief"），和/或把 repo-context 并入单响应批处理——它与 pr-context、comment-status、rules load 互相独立。
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 5/100 轮）。改动内容与我反驳保留之处如下： Round-3 summary — PR #8401 review feedback addressedCommit: The CriticalR2-1 — unbounded matching work in the
Suggestions — all 26 addressed in code, tests, or docs
Top-level review items (CHANGES_REQUESTED by the automated reviewer)
VerificationCommands actually run this round:
中文说明第 3 轮总结 — PR #8401 评审意见处理提交： Critical 项R2-1 —
建议项 — 全部 26 条已在代码、测试或文档中处理
顶层评审条目（自动评审者的 CHANGES_REQUESTED）
验证本轮实际执行的命令：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 Addressed the latest review feedback (round 6/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 6/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #8401 review feedback addressedCommit: Feedback triageThe feedback file contained exactly one actionable item:
No review bodies, inline comments, or issue-level comments were newer than the last evaluation. Investigation (evidence, not guesses)CI logs are not accessible in this mode (no GitHub credentials), so the full Test job pipeline was reproduced locally in CI-equivalent conditions:
Changes
Verification
中文说明轮次总结 — 已处理 PR #8401 的评审反馈提交： 反馈分诊反馈文件中只有一条可处理项：
没有比上次评估更新的评审正文、行内评论或议题级评论。 调查过程（依据证据，而非猜测）本模式无法访问 CI 日志（没有 GitHub 凭据），因此在与 CI 等价的条件下，于本地完整复现了 Test 作业流水线：
改动内容
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
Test Plan (not a blocker): src/commands/review.test.ts — no such file or directory; src/commands/review/repo-context.test.ts — no such file or directory; src/commands/review/lib/repository-context.test.ts — no such file or directory; src/commands/review/lib/manifest-repository-context.test.ts — no such file or directory; src/commands/review/lib/roster.test.ts — no such file or directory; and 4 more.
中文说明
未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
Test Plan（非阻断）：src/commands/review.test.ts — no such file or directory; src/commands/review/repo-context.test.ts — no such file or directory; src/commands/review/lib/repository-context.test.ts — no such file or directory; src/commands/review/lib/manifest-repository-context.test.ts — no such file or directory; src/commands/review/lib/roster.test.ts — no such file or directory; and 4 more。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| let value: unknown; | ||
| try { | ||
| value = JSON.parse(content); | ||
| } catch { |
There was a problem hiding this comment.
[Critical] No size bound anywhere before JSON.parse(content): local mode reads the worktree manifest via bare readFileSync (statSync checks isFile only), and parseManifest parses the entire content before any validation runs. — Failure scenario: in the untrusted-local-repo threat model this file is built for, an attacker-committed manifest of 93.3 MB (under GitHub's 100 MB push limit; millions of unique short strings) drove the real local-mode pipeline to RSS +640 MB / heap +379 MB before the bounded-array rejection; heap-ceiling binary search OOMs at a 320 MB cap. From non-GitHub origins the file size is unbounded. Defeats the design doc's “a few hundred kilobytes at the schema bounds” mitigation. — Suggested fix: fail-closed size ceiling on the identity read (reject statSync(resolved).size above ~1 MB) plus the same ceiling on content.length at the top of parseManifest, symmetric in both modes.
中文说明
JSON.parse(content) 之前没有任何大小上限：local 模式用裸 readFileSync 读取 worktree manifest（statSync 只检查 isFile），parseManifest 在任何校验之前就解析全部内容。— 失败场景：在本文件所针对的不可信仓库本地 review 威胁模型下，攻击者提交 93.3 MB 的 manifest（低于 GitHub 100 MB 推送上限；数百万个唯一短字符串）驱动真实 local 模式管线，在有界数组拒绝之前 RSS +640 MB / 堆 +379 MB；堆上限二分搜索显示 320 MB 上限即 OOM。非 GitHub 来源的文件大小无上界。击穿设计文档“按 schema 上界只有几百 KB”的缓解承诺。— 建议修复：在 identity 读取处设置 fail-closed 大小上限（statSync(resolved).size 超过约 1 MB 即拒绝），并在 parseManifest 顶部对 content.length 施加同样上限，两种模式保持对称。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| matchWork += patternSegments[index] * pathSegmentCount; | ||
| if (matchWork > MAX_MATCH_WORK) { |
There was a problem hiding this comment.
[Critical] R2-1: MAX_MATCH_WORK bills segment counts (patternSegments × pathSegmentCount), but segmentMatches is quadratic in segment length, and both lengths are attacker-maximizable within schema bounds (255-byte filenames, 512-char patterns) — the budget never trips for a schema-legal stall shape. Round 2's requested work budget exists, but it is billed on the wrong metric. — Failure scenario: measured against the real provider: 512 files × 128 patterns with long-segment names = 9.1 s while only 0.098% of the budget is charged; the full-cap shape charges at most 3.13% of MAX_MATCH_WORK — the limit can never trip — while actual wall time extrapolates to ~290 s of CPU. A one-file diff in a malicious repository stalls the mandatory repo-context step for minutes instead of failing closed. — Suggested fix: bill string lengths instead of segment counts (pattern.length × path.length per attempt) and recalibrate MAX_MATCH_WORK so the documented legitimate scan stays under it while the adversarial shape trips it.
中文说明
R2-1：MAX_MATCH_WORK 按段数量计费（patternSegments × pathSegmentCount），但 segmentMatches 的开销是段长度的二次方，而两个长度攻击者都能在 schema 上界内取到最大（255 字节文件名、512 字符模式）——schema 合法的拖停形状永远不会触发预算。第 2 轮要求的工作量预算已经存在，但计费指标错了。— 失败场景：对真实 provider 实测：512 个文件 × 128 个长段名模式 = 9.1 秒，而预算只消耗了 0.098%；满上限形状最多消耗 MAX_MATCH_WORK 的 3.13%——限制永远不会触发——而实际耗时外推约 290 秒 CPU。恶意仓库中一个单文件 diff 就能让强制的 repo-context 步骤拖停数分钟而不是 fail-closed。— 建议修复：按字符串长度而非段数计费（每次尝试 pattern.length × path.length），并重新校准 MAX_MATCH_WORK，使文档承诺的合法扫描仍在上限内、而对抗形状会触发。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const matched = manifest.rules.filter((rule) => | ||
| input.changedPaths.some((path) => | ||
| rule.paths.some((pattern) => globMatches(pattern, path)), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
[Critical] The rule-matching filter in provide() has no work budget — MAX_MATCH_WORK is charged only in expandRelatedPaths, and nothing caps the changedPaths count, so total filter work (changedPaths × 128 patterns × per-call globMatches cost) is unbounded; the R2-1 family survives in this sibling stage. — Failure scenario: local review of an untrusted repository (the file's own threat model): attacker-committed manifest with 128 schema-legal 512-char **-heavy paths globs plus a bulk change set. Measured against the real provider: 128 near-miss patterns × 20 changed paths = 21.9 s of synchronous CPU with nothing thrown; linear extrapolation ≈ 18 min at 1,000 paths. Violates the documented contract “a matching burst fails closed with the scan limit instead of stalling the step”. — Suggested fix: charge the same budget in the filter — accumulate per-attempt work (ideally pattern.length × path.length, see the companion finding on billing) and throw the existing scan-limit error past the ceiling; or bound **-segment count per pattern at schema validation.
中文说明
provide() 中的规则匹配过滤器没有工作量预算——MAX_MATCH_WORK 只在 expandRelatedPaths 中计费，且没有任何东西限制 changedPaths 数量，因此过滤器总开销（changedPaths × 128 个模式 × 单次 globMatches 成本）无上界；R2-1 家族在这个姊妹阶段仍然存在。— 失败场景：对不可信仓库的本地 review（本文件自身的威胁模型）：攻击者提交含 128 个 schema 合法、512 字符、** 密集 paths glob 的 manifest，加上批量变更集。对真实 provider 实测：128 个近失配模式 × 20 个变更路径 = 21.9 秒同步 CPU 且不抛任何错误；线性外推 1,000 个路径约 18 分钟。违反文档契约“匹配突发应以扫描上限 fail-closed 而不是拖停步骤”。— 建议修复：在过滤器中同样计费——累计每次尝试的开销（理想为 pattern.length × path.length，参见配套的计费发现），超过上限时抛出既有的扫描上限错误；或在 schema 校验处限制每个模式的 ** 段数量。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (segments.some((segment) => SKIPPED_DIRECTORIES.has(segment))) { | ||
| throw new Error( |
There was a problem hiding this comment.
[Suggestion] SKIPPED_DIRECTORIES enforcement is case-sensitive exact-segment comparison at both sites, and visit() never checks a scan ROOT against the set — on every platform a case-varied pattern walks into a skipped tree. — Failure scenario: probe-verified against the real provider (case-sensitive ext4 — not limited to case-insensitive filesystems; on APFS/NTFS a case-varied opener additionally resolves to the exact-case directory): node_modules/** throws “enters a skipped directory”, while NODE_MODULES/** and a static NODE_MODULES/react/index.js pass validation and the scan returns files from inside the dependency tree; recursion also descends src/NODE_MODULES. Dependency/.git path names then merge into relatedPaths → every reviewer prompt; on large trees the scan instead trips MAX_GLOB_CANDIDATES and fails the whole review. — Suggested fix: compare case-insensitively at both sites (lowercase the set and test segment.toLowerCase()) and apply the skip check to the scan root itself in visit() before descending; add a case-varied pattern test.
中文说明
SKIPPED_DIRECTORIES 在两处都是大小写敏感的精确段比较，且 visit() 从不把扫描根对照该集合检查——在任何平台上，大小写变体的模式都能走进被跳过的树。— 失败场景：对真实 provider 探测验证（大小写敏感的 ext4——不限于大小写不敏感文件系统；在 APFS/NTFS 上大小写变体还会解析到精确大小写目录）：node_modules/** 抛出“enters a skipped directory”，而 NODE_MODULES/** 与静态 NODE_MODULES/react/index.js 通过校验，扫描返回依赖树内部的文件；递归也会进入 src/NODE_MODULES。依赖/.git 路径名随后并入 relatedPaths → 每个 reviewer prompt；大树下扫描反而触发 MAX_GLOB_CANDIDATES 使整个 review 失败。— 建议修复：两处都改为大小写不敏感比较（集合小写化并测试 segment.toLowerCase()），并在 visit() 进入前对扫描根本身应用跳过检查；新增大小写变体模式测试。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (manifest['version'] !== 1) { | ||
| throw new Error('unsupported repository context manifest version'); | ||
| } |
There was a problem hiding this comment.
[Suggestion] The manifest version gate has no test and is the ONLY gate for it — the provider hardcodes version: 1 in its output, so the wire validator never sees the manifest's actual version. — Failure scenario: mutation-verified: deleting the check leaves 81/81 green across the three suites. When manifest version 2 is later introduced, a repository committing "version": 2 is silently parsed as version 1 and injects its guidance into every review brief instead of failing closed as the design doc promises. — Suggested fix: add ['unsupported manifest version', manifest({ version: 2 })] to the fail-closed it.each table.
中文说明
manifest 版本门禁没有测试，且它是唯一的门禁——provider 输出中硬编码 version: 1，wire 校验器永远看不到 manifest 的真实版本。— 失败场景：突变验证：删除该检查后三个套件 81/81 全绿。将来引入 manifest 版本 2 时，提交 "version": 2 的仓库会被静默按版本 1 解析，把其指导注入每个 review brief，而不是像设计文档承诺的那样 fail-closed。— 建议修复：向 fail-closed 的 it.each 表添加 ['unsupported manifest version', manifest({ version: 2 })]。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const outDirectory = join(root, 'out-directory'); | ||
| mkdirSync(outDirectory); |
There was a problem hiding this comment.
[Suggestion] No test passes an --out whose parent directory does not exist, so mkdirSync(dirname(outPath), { recursive: true }) is pinned by nothing. — Failure scenario: mutation-verified: deleting the mkdirSync line leaves 26/26 green; a probe with --out root/fresh-subdir/context.json fails on the mutant (ENOENT from the atomic temp-file write, observed) after the providers already ran. SKILL.md (added in this same diff) tells the orchestrator to choose an arbitrary absolute --out. — Suggested fix: add a case asserting runRepoContext writes the artifact when out is join(root, 'fresh-subdir', 'context.json').
中文说明
没有测试传入父目录不存在的 --out，因此 mkdirSync(dirname(outPath), { recursive: true }) 无任何钉扎。— 失败场景：突变验证：删除 mkdirSync 行后 26/26 全绿；用 --out root/fresh-subdir/context.json 探测时突变体在 provider 已运行后失败（观察到原子临时文件写入的 ENOENT）。SKILL.md（同一 diff 新增）告诉编排器任选绝对 --out。— 建议修复：新增用例，断言 out 为 join(root, 'fresh-subdir', 'context.json') 时 runRepoContext 成功写入制品。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| } else if ( | ||
| brief.reviewsCode || | ||
| (isRepositoryContextRoleId(role) && |
There was a problem hiding this comment.
[Suggestion] Context-block delivery to the five code-reviewing roles OUTSIDE the manifest allow-list (verify, reverse-audit, invariant-a/b/c — all reviewsCode: true, none in REPOSITORY_CONTEXT_ROLES) is pinned in only one direction: the negative pins guard widening, nothing pins that these roles RECEIVE the block. — Failure scenario: verifier-verified: a narrowing mutant that keeps every pinned role — (brief.reviewsCode && isRepositoryContextRoleId(role)) || (isRepositoryContextRoleId(role) && requiredAgents.includes(role)) — strips the block from exactly those five roles and ships 186/186 green (the only context-bearing test builds 0/1a/7/test-matrix/chunk/whole-diff). They would silently stop receiving the repository's declared domains, related paths, and unverified dimensions, with the suite certifying the change. — Suggested fix: add positive pins for at least one role reaching the block solely via brief.reviewsCode, e.g. expect(buildRoleBrief(contextPlan, 'verify')).toContain('Example project repository context'), and the same for 'reverse-audit'.
中文说明
向 manifest 允许列表之外的五个代码审查角色（verify、reverse-audit、invariant-a/b/c——都是 reviewsCode: true，都不在 REPOSITORY_CONTEXT_ROLES 中）投递 context 块只被单向钉住：负向钉扎防扩大，但没有任何钉扎保证这些角色收到该块。— 失败场景：验证者核实：一个保留所有被钉扎角色的收窄突变——(brief.reviewsCode && isRepositoryContextRoleId(role)) || (isRepositoryContextRoleId(role) && requiredAgents.includes(role))——恰好把这五个角色的块剥离且 186/186 全绿（唯一携带 context 的测试只构建 0/1a/7/test-matrix/chunk/whole-diff）。它们将静默停止接收仓库声明的领域、相关路径与未验证维度，而套件为这一变更背书。— 建议修复：为至少一个仅经 brief.reviewsCode 获得该块的角色添加正向钉扎，例如 expect(buildRoleBrief(contextPlan, 'verify')).toContain('Example project repository context')，并对 'reverse-audit' 同样处理。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const reviewerBrief = buildRoleBrief(contextPlan, '1a'); | ||
| expect(reviewerBrief).toContain('Example project repository context'); | ||
| expect(reviewerBrief).toContain('compiler, runtime'); |
There was a problem hiding this comment.
[Suggestion] Every positive assertion on the rendered repository-context block is a bare toContain against the whole brief, so no field is pinned to its section; the section labels appear only in the implementation, never in a test assertion. — Failure scenario: mutation-verified: swapping the unverifiedDimensions ↔ verificationNotes renderings ships 363/363 green — reviewer agents are then told the repository's unverified proof boundaries are its verification instructions and vice versa, with no test red. — Suggested fix: pin section adjacency, e.g. expect(reviewerBrief).toContain('Unverified dimensions:\n- Alternate runtime was not exercised') and equivalents for the other sections.
中文说明
对渲染后 repository-context 块的每个正向断言都是对整个 brief 的裸 toContain，没有字段被钉在其所属 section 上；section 标签只出现在实现中，从不出现在测试断言里。— 失败场景：突变验证：交换 unverifiedDimensions ↔ verificationNotes 的渲染后 363/363 全绿——reviewer agent 会把仓库的未验证证明边界当作验证指引（反之亦然），而没有任何测试变红。— 建议修复：钉住 section 邻接关系，例如 expect(reviewerBrief).toContain('Unverified dimensions:\n- Alternate runtime was not exercised') 及其他 section 的等价断言。
— qwen3.8-max via Qwen Code /review (v0.21.6)
|
|
||
| `repo-context` reads the fixed manifest path through `RepositoryContextProviderInput.readIdentityFile`. For pull request plans, the manifest therefore comes only from the trusted merge-base commit recorded by the fetch stage. The pull request head cannot opt in, opt out, or change the rules. For local plans, the manifest comes from the current worktree after safe-relative-path validation and realpath containment. | ||
|
|
||
| Identity reads return the same shape in both modes (CRLF normalised to LF, surrounding whitespace trimmed), and fail closed: an absent file yields `null`, but a file that is present and unreadable throws rather than masquerading as "not this repository". Both modes follow a symlinked identity under the same containment rule, and a directory yields nothing in both. A pull request plan whose merge base never resolved (`mergeBaseSha: null`) — or whose base fetch failed, leaving the recorded sha possibly stale — writes a `null` artifact without consulting the worktree at all: falling back to the worktree would read the manifest from the PR head, the exact read this boundary forbids, and a possibly stale sha is not a trusted source either. |
There was a problem hiding this comment.
[Suggestion] The design doc asserts identity-read parity (“Both modes follow a symlinked identity under the same containment rule”) and lists exactly two trust-boundary residuals, but the implementation this PR adds documents a third: ls-tree never descends through a symlinked intermediate path COMPONENT, and base mode caps symlink chains at 16 hops where the kernel resolves up to ~40 (and THROWS at the cap rather than degrading). — Failure scenario: probe through real runRepoContext: a repository committing .qwen as a symlink to an in-tree directory gets repository context in local reviews but silently never in PR reviews (ls-tree exit 0, zero lines), while this parity claim misleads the operator diagnosing “context attaches locally but never on PRs”. — Suggested fix: add this divergence to the Trust boundary residuals, or reword the parity sentence to scope it to the identity file itself and name the intermediate-component asymmetry and the 16-hop cap.
中文说明
设计文档断言 identity 读取对等（“两种模式都在同一包含规则下跟随 symlink identity”），并恰好列出两条信任边界残留，但本 PR 新增的实现记录了第三条：ls-tree 从不穿过 symlink 的中间路径组件，且 base 模式把 symlink 链上限设为 16 跳，而内核最多解析约 40 跳（且达到上限时抛错而非降级）。— 失败场景：通过真实 runRepoContext 探测：把 .qwen 提交为指向树内目录的 symlink 的仓库，在 local review 中获得 repository context，但在 PR review 中永远静默得不到（ls-tree 退出码 0、零输出），而此对等声明会误导正在诊断“context 本地能附加、PR 上从不附加”的操作者。— 建议修复：把该分歧加入 Trust boundary 残留列表，或改写对等句，把其限定到 identity 文件本身，并写明中间组件不对称与 16 跳上限。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| It writes the diff to `.qwen/tmp/qwen-review-<target>-diff.txt` and emits the same report `fetch-pr` does (`diffPathAbsolute`, `chunks[]`, `files[]`, the topology counts), plus two fields of its own: | ||
| It writes the diff to `.qwen/tmp/qwen-review-<target>-diff.txt` and emits the same report `fetch-pr` does (`diffPathAbsolute`, `chunks[]`, `files[]`, the topology counts), plus two fields of its own. | ||
|
|
||
| At **medium or high** effort, for local, file-path, and same-repository PR reviews, attach declarative repository context before `agent-prompt --roster` — the roster and every brief bake this context in, so running it later silently drops the manifest's required agents and guidance (and it is therefore also before launching agents): |
There was a problem hiding this comment.
[Suggestion] The new repository-context block is inserted between the sentence “plus two fields of its own.” and the two bullets that name those fields (untrackedFiles/skippedFiles), severing the introduction from its referent — the pre-change colon became a period and 13 lines of unrelated material now sit between sentence and bullets. — Failure scenario: the bullets carry mandatory orchestrator behavior (“Name them in the review's summary” for untrackedFiles, “List these under 'Not reviewed' in Step 6” for skippedFiles) and arrive disconnected from the sentence introducing them — the two fields can be misread as belonging to the repo-context artifact described in the intervening paragraph, and the bullets are easy to skip for an orchestrator that scans or pages the section. — Suggested fix: move the new “At medium or high effort…” block (prose + bash + manifest paragraph) to after the untrackedFiles/skippedFiles bullets, restoring adjacency (and the colon).
中文说明
新增的 repository-context 块被插在句子“plus two fields of its own.”与命名这两个字段（untrackedFiles/skippedFiles）的两个项目符号之间，切断了引言与其所指——改动前的冒号变成了句号，句子与项目符号之间现在有 13 行无关内容。— 失败场景：这两个项目符号承载编排器的强制行为（untrackedFiles 的“在 review 总结中点名”、skippedFiles 的“在 Step 6 的 Not reviewed 下列出”），却与引入它们的句子脱节——这两个字段可能被误读为属于中间段落描述的 repo-context 制品，且对扫读或分页阅读该节的编排器来说项目符号很容易被跳过。— 建议修复：把新增的“At medium or high effort…”块（散文 + bash + manifest 段落）移到 untrackedFiles/skippedFiles 项目符号之后，恢复邻接（并恢复冒号）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
|
🤖 Addressed the latest review feedback (round 7/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 7/100 轮）。改动内容与我反驳保留之处如下： Round-3 review feedback — all 32 inline findings addressedCommit: Critical findings (fixed)
Suggestions (all addressed)
Review-body items (no code action)
VerificationCommands actually run this round (all after the final code state):
中文说明第 3 轮 review 反馈——32 条 inline 发现全部处理提交： Critical 发现（已修复）
Suggestion（全部处理）
Review body 条目（无代码动作）
验证本轮实际运行的命令（均为最终代码状态之后）：
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally. Not reviewed: reverse audit — stopped before round 4 by the review time budget. Test Plan (not a blocker): src/commands/review.test.ts — no such file or directory; src/commands/review/repo-context.test.ts — no such file or directory; src/commands/review/lib/repository-context.test.ts — no such file or directory; src/commands/review/lib/manifest-repository-context.test.ts — no such file or directory; src/commands/review/lib/roster.test.ts — no such file or directory; and 4 more.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。 未审查：build-and-test — Test (macos-latest, Node 22.x) was skipped in CI and its suite did not run locally。 未审查：反向审计——评审时间预算不足，未能开始第 4 轮。 Test Plan（非阻断）：src/commands/review.test.ts — no such file or directory; src/commands/review/repo-context.test.ts — no such file or directory; src/commands/review/lib/repository-context.test.ts — no such file or directory; src/commands/review/lib/manifest-repository-context.test.ts — no such file or directory; src/commands/review/lib/roster.test.ts — no such file or directory; and 4 more。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| return gitRaw('-C', worktree, 'show', `${mergeBase}:${path}`).toString( | ||
| 'utf8', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R4-1: Base-mode identity reads materialize the entire blob (up to gitRaw's 512 MB maxBuffer) before any size check; MAX_IDENTITY_BYTES is enforced only afterwards in parseManifest, while the worktree branch probes stat.size before readFileSync — asymmetric with the MAX_IDENTITY_BYTES comment's 'symmetric in both modes' claim. Symlink-hop blobs are likewise uncapped. — Failure scenario: A merge-base commit carrying a >1 MB .qwen/review-context.json (e.g. a 300 MB committed manifest): readBaseBlob buffers the whole blob via git show, converts to UTF-8 and CRLF-normalizes before parseManifest rejects it — a ~1 GB+ transient allocation where local mode fails instantly at statSync (probe-measured +81 MB RSS for a 40 MB manifest vs +0 MB local). Fails closed, but the 1 MB ceiling exists precisely to keep that payload off the heap.
Suggested fix: Mirror the worktree stat check in base mode: probe the object size with git cat-file -s before reading and throw the same size-limit error when it exceeds MAX_IDENTITY_BYTES, at both readBaseBlob call sites.
中文说明
base 模式的 identity 读取会把整个 blob（最大到 gitRaw 的 512 MB maxBuffer）完整物化之后才做任何大小检查；MAX_IDENTITY_BYTES 只在之后由 parseManifest 执行，而 worktree 分支在 readFileSync 之前就用 stat.size 探测——与 MAX_IDENTITY_BYTES 注释声称的"两种模式对称"不符；symlink 跳转读取的 blob 同样无上限。失败场景：merge-base 提交携带 >1 MB 的 manifest（如 300 MB）时，base 模式会先经 git show 缓冲全量、转 UTF-8 并做 CRLF 归一化才被拒绝——瞬时分配可达 1 GB+，而 local 模式在 statSync 处立即失败（探针实测 40 MB manifest：base +81 MB RSS vs local +0 MB）。虽最终 fail-closed，但 1 MB 上限的存在意义正是避免该载荷上堆。建议修复：在 base 模式镜像 worktree 的 stat 检查——先用 git cat-file -s 探测对象大小，超过 MAX_IDENTITY_BYTES 即抛出同样的 size-limit 错误。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (segment === '..') { | ||
| if (resolved.length === 0) return null; | ||
| resolved.pop(); |
There was a problem hiding this comment.
[Suggestion] R3-21: resolveTreeSymlinkTarget pops an interior .. lexically without verifying the popped component is an existing directory, so base mode resolves to a blob the filesystem can never reach — a surviving instance of the previously flagged x/. / x/..-in-symlink-target class, contradicting the documented 'base mode reads strictly less, never more' invariant. — Failure scenario: Probe-verified end-to-end: commit .qwen/review-context.json as a symlink with target a/../b where .qwen/a is a regular file (or missing/../b). Local mode: realpathSync fails ENOTDIR/ENOENT → provider receives null. Base mode: the resolver pops the component and lands on .qwen/b → context attaches. Same repository, identity diverges by mode in the direction the design doc claims is impossible. Containment holds (trusted merge-base tree only) — parity defect, not a trust hole.
Suggested fix: Track directory-ness for segments introduced by the symlink target: any such component later walked through or popped by .. must be probed with baseTreeEntry and be a tree entry; otherwise return null, mirroring the kernel's ENOENT/ENOTDIR. Add a both-modes test pinning null for targets a/../b (a a file) and missing/../b.
中文说明
resolveTreeSymlinkTarget 对中间位置的 .. 只做词法 pop，不验证被弹出组件是否为存在的目录，导致 base 模式能解析到文件系统永远无法到达的 blob——这是此前已被指出的 symlink 目标含 x/. / x/.. 类别的幸存实例，与文档"base 模式只读更少、绝不多读"的不变量矛盾。失败场景（端到端探针验证）：将 .qwen/review-context.json 提交为符号链接，目标为 a/../b（.qwen/a 是普通文件）或 missing/../b：local 模式 realpathSync 以 ENOTDIR/ENOENT 失败 → provider 收到 null；base 模式解析器弹出该组件后落到 .qwen/b → context 被附加。同一仓库、同一 manifest，两种模式结果背离，且方向恰是设计文档声称不可能的方向。containment 仍成立（内容只来自可信 merge-base 树）——是 parity 缺陷而非信任漏洞。建议修复：对符号链接目标引入的组件跟踪目录性：凡被 .. 弹出或穿过的此类组件须经 baseTreeEntry 探测且为 tree，否则返回 null；并补充双模式测试。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // Names tracked source must not live under in this repository's conventions | ||
| // (a `build/` directory holds real scripts here) stay out of this set. |
There was a problem hiding this comment.
[Suggestion] R4-3: The sentence explaining why build is deliberately absent from SKIPPED_DIRECTORIES is garbled and, read literally, states the opposite of the true invariant (the set holds exactly the names tracked source must NOT live under; build stays out because tracked source CAN live under it). — Failure scenario: A maintainer extending SKIPPED_DIRECTORIES on the strength of this comment (the block explicitly invites membership reasoning) could add build; every relatedPaths glob would then silently stop matching files under build/ in repos keeping real source there (this repo has such tracked files — verified via git ls-files), degrading the scan to a complete-looking result with a hole and no error, since skipped descent is silent by design.
Suggested fix: Rewrite affirmatively, e.g.: names under which tracked source can NEVER live are skipped; build/ stays out because this repository keeps tracked scripts there, so a legitimate review target is never silently skipped.
中文说明
解释为何 build 刻意不在 SKIPPED_DIRECTORIES 中的这段注释语句不通，照字面读恰好陈述了真实不变量的反面（集合里放的恰是"tracked source 不会位于其下"的名字；build 不在其中是因为本仓库确实在其下存放 tracked 脚本）。失败场景：维护者按此注释扩充集合（该注释块明确邀请推理成员资格）时可能加入 build，届时在 build/ 下存放真实源码的仓库中，所有 relatedPaths glob 会静默不再匹配 build/ 下的文件——扫描退化为看似完整实则有空洞的结果且无任何报错（跳过 descend 本身就是静默的；已用 git ls-files 验证本仓库存在此类文件）。建议修复：改为肯定式表述，例如"tracked source 绝不会位于其下的名字被跳过；build/ 不入集合，因为本仓库在其下存放受跟踪脚本，合法 review 目标永不被静默跳过"。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // isControlFree rejects all of 0x00-0x1F, 0x7F-0x9F, U+2028/2029, the | ||
| // bidi directional formatting block, and zero-width hiding characters; | ||
| // probing range ends plus interior points pins the range, not a |
There was a problem hiding this comment.
[Suggestion] R4-4: The comment claims 'probing range ends plus interior points pins the range', but the spoofing ranges 0x202A-0x202E and 0x2066-0x2069 are probed at their ends only — no U+202B/C/D or U+2067/68 in either separator loop. — Failure scenario: Mutant-verified: reimplementing SPOOFING_FORMAT_CODE_UNITS as an endpoint list (or off-by-one like code >= 0x2067) ships the suite green while RLE, PDF, LRO, LRI, RLI pass validateRepositoryContext and render intact into the posted review body and every reviewer prompt — trojan-source-class display spoofing the tests were written to prevent.
Suggested fix: Add '\u202b', '\u202c', '\u202d', '\u2067', '\u2068' to both separator probe lists.
中文说明
注释声称"探测区间端点加内部点以钉住整个区间"，但 spoofing 区间 0x202A-0x202E 与 0x2066-0x2069 只探测了端点——两个 separator 循环均无 U+202B/C/D 与 U+2067/68。失败场景（mutant 验证）：把 SPOOFING_FORMAT_CODE_UNITS 改为端点列表实现（或 off-by-one 如 code >= 0x2067），整套测试仍绿，而 RLE、PDF、LRO、LRI、RLI 将通过 validateRepositoryContext 并原样渲染进发布的 review body 与每个 reviewer prompt——正是这些测试要防的 trojan-source 类显示欺骗。建议修复：在两个探测列表中补 '\u202b'、'\u202c'、'\u202d'、'\u2067'、'\u2068'。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const repositoryContext = repositoryContextOf(plan); | ||
| for (const role of repositoryContext?.requiredAgents ?? []) { | ||
| if (!contextRoleRunsInThisReview(role, plan, mode)) continue; |
There was a problem hiding this comment.
[Suggestion] R4-5: No check-coverage-level test pins that context-required agents enter the coverage certification roster; check-coverage.test.ts has zero repositoryContext references even though it pins the analogous effort drift class. — Failure scenario: coverageFromTranscripts passes the whole plan to requiredAgents, so today a manifest-required role (e.g. test-matrix in a 3B fan-out) is certified correctly. But a future edit reconstructing the roster input and dropping the field — the exact drift class the existing effort A/B test was written for — would certify and post APPROVE for a run that never launched the manifest-required agent. The roster half is pinned in roster.test.ts; the coverage half is not.
Suggested fix: Add a check-coverage test mirroring the effort A/B shape: a fan-out plan carrying repositoryContext.requiredAgents ['test-matrix'] whose transcripts omit the test-matrix launch → expect coverage ok:false with the role named in missingRoles (plus a companion pass case).
中文说明
没有任何 check-coverage 层面的测试钉住"context 要求的 agent 进入 coverage 认证 roster"；check-coverage.test.ts 对 repositoryContext 零引用，尽管它已为同类的 effort 漂移类别钉过测试。失败场景：coverageFromTranscripts 目前把整个 plan 传给 requiredAgents，所以 manifest 要求的角色（如 3B fan-out 中的 test-matrix）能被正确认证；但未来某次编辑若重构 roster 输入并丢掉该字段——正是现有 effort A/B 测试针对的漂移类别——一个从未启动 manifest 所要求 agent 的运行仍会通过认证并发布 APPROVE。roster 一侧已有 roster.test.ts 钉住；coverage 一侧没有。建议修复：仿照 effort A/B 形状新增 check-coverage 测试：fan-out plan 携带 requiredAgents ['test-matrix'] 而 transcripts 缺少其启动 → 期望 ok:false 且 missingRoles 点名该角色（另加一个通过的对照用例）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| worktreePath: '../somewhere-else', | ||
| }), | ||
| ).toThrow('does not match plan.worktreePath'); |
There was a problem hiding this comment.
[Suggestion] R4-24: The recorded-worktree guard's core check — realpathSync(candidate) === worktree — is not pinned: the only negative test uses a path that does not exist, which any existence check would also reject. — Failure scenario: Mutant-verified: replacing the guard with exists-only kept 33/33 green while a plan recorded for checkout A, run with --worktree=B, completed and produced an artifact containing B's manifest (observed label 'Wrong worktree manifest') — the exact cross-worktree identity confusion the guard exists to prevent.
Suggested fix: Add a rejection test where the recorded worktreePath resolves to an existing directory DIFFERENT from --worktree (two sibling dirs; record one, pass the other) and assert toThrow('does not match plan.worktreePath').
中文说明
recorded-worktree 守卫的核心检查——realpathSync(candidate) === worktree——没有钉扎：唯一的负向测试使用不存在的路径，任何存在性检查都会同样拒绝它。失败场景（mutant 验证）：把守卫替换为仅 existsSync，33/33 全绿；而把记录为 checkout A 的 plan 以 --worktree=B 运行，会完成并产出包含 B manifest 的 artifact（观测到 label 'Wrong worktree manifest'）——恰是该守卫要防止的跨 worktree 身份混淆。建议修复：补一个拒绝测试——记录的 worktreePath 解析到一个与 --worktree 不同的已存在目录（两个兄弟目录，记录其一、传入另一个），断言 toThrow('does not match plan.worktreePath')。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // Positive pins for code-reviewing roles OUTSIDE the manifest | ||
| // allow-list that reach the block solely through `brief.reviewsCode`: | ||
| // a narrowing mutant that keeps every pinned role strips exactly these | ||
| // and ships green. | ||
| for (const role of ['verify', 'reverse-audit'] as const) { |
There was a problem hiding this comment.
[Suggestion] R3-28: The loop's comment claims to pin the complete set of roles reaching the context block solely through brief.reviewsCode, but that set is five roles: invariant-a/b/c also carry reviewsCode: true and are absent from REPOSITORY_CONTEXT_ROLES — unpinned for all three. A surviving instance of the round-3 R3-28 class. — Failure scenario: Probe-verified: a declarative-list refactor of the buildRoleBrief dispatch omitting the three invariant roles ships 186/186 green while the block disappears for all three roles — the invariant agents, launched precisely on the diff's heaviest files, silently lose domains, related paths, unverified dimensions and verification notes in every context-bearing review, with no test signal.
Suggested fix: Extend the pin to the invariant roles: expect(buildRoleBrief(contextPlanWithHeavyFile, 'invariant-a', { file: 'src/big.ts' })).toContain('Example project repository context') for all three, and soften 'strips exactly these' to match what is pinned.
中文说明
该循环的注释声称钉住了"仅通过 brief.reviewsCode 到达 context 块"的完整角色集合，但该集合有五个角色：invariant-a/b/c 同样携带 reviewsCode: true 且不在 REPOSITORY_CONTEXT_ROLES 中——三者均无钉扎。这是第 3 轮 R3-28 类别的幸存实例。失败场景（探针验证）：把 buildRoleBrief 的分发改为遗漏三个 invariant 角色的声明式列表，186/186 全绿而三个角色的 context 块全部消失——恰好被派往 diff 最重文件的 invariant agent 会在每个携带 context 的 review 中静默丢失 domains、related paths、unverified dimensions 与 verification notes，且无任何测试信号。建议修复：把钉扎扩展到 invariant 角色（需以含 heavy 文件的 plan + opts.file 调用 buildRoleBrief），并把注释措辞"strips exactly these"改与实际钉扎一致。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // The merged `relatedPaths` pattern list is capped BEFORE any scan, or a | ||
| // max-cardinality manifest stalls expansion for minutes. | ||
| expect( |
There was a problem hiding this comment.
[Suggestion] R4-26: The comment claims the merged relatedPaths pattern list is 'capped BEFORE any scan', but the fixture (256 fully static patterns against an empty worktree) cannot distinguish cap-before-scan from cap-after-scan. — Failure scenario: Probe-verified: with the glob-list cap moved after the scan, the full suite stays 57/57 green while a distinguishing probe (256 wildcard patterns over a 25,600-entry tree) flips from 'relatedPaths glob list exceeds limit' in 219ms (cap at merge, no scan) to the resolved-files cap's message after a full scan — a >128-pattern manifest rooted in real trees is scanned to the expansion caps before rejection, and the operator gets the wrong diagnostic ('trim the globs' vs 'trim the glob list').
Suggested fix: Replace one static pattern in the fixture with a wildcard rooted at a populated subtree so a deferred cap provably scans before it throws.
中文说明
注释声称合并后的 relatedPaths 模式列表"在任何扫描之前"被加上限，但 fixture（256 个全静态模式对空 worktree）无法区分"扫描前加上限"与"扫描后加上限"。失败场景（探针验证）：把 glob-list 上限移到扫描之后，整套 57/57 全绿，而区分性探针（256 个通配模式对 25,600 条目树）由"219ms 抛 'relatedPaths glob list exceeds limit'（merge 处加上限、未扫描）"翻转为先完整扫描再抛 resolved-files 上限的消息——根在真实树中的 >128 模式 manifest 会被扫描到 expansion 上限才拒绝，且操作者拿到错误诊断（"裁剪 glob 列表"还是"收窄 globs"）。建议修复：把 fixture 中一个静态模式换成根在已填充子树的通配，使延后上限在抛错前必然先扫描。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| if (resolved === '') return null; | ||
| path = resolved; |
There was a problem hiding this comment.
[Suggestion] R4-27: The base-mode guard mapping a committed identity symlink resolving exactly to the tree root (resolved === '') to null is pinned by no test; the worktree-mode twin is pinned, but only via a local-mode uncommitted symlink. — Failure scenario: Mutant-verified: deleting the guard ships 33/33 green while the next hop runs git ls-tree -- '' (exit 128, probe-verified), aborting PR reviews of a repo committing .qwen/review-context.json -> .. with a raw git fatal while local reviews degrade cleanly. git show : also exits 0 with the ROOT TREE LISTING, so the guard keeps that shape away from the parser too.
Suggested fix: Add the base-mode twin of the root-link test: commit .qwen/review-context.json -> .. and assert the artifact is null in both modes.
中文说明
base 模式把"已提交 identity 符号链接恰好解析到树根（resolved === ''）"映射为 null 的守卫没有任何测试钉扎；worktree 模式的孪生用例有钉扎，但仅通过 local 模式的未提交符号链接。失败场景（mutant 验证）：删除该守卫，33/33 全绿，而下一跳会执行 git ls-tree -- ''（exit 128，探针验证），使提交了 .qwen/review-context.json -> .. 的仓库在 PR review 中以裸 git fatal 中止 repo-context 步骤，local review 却干净降级。且 git show : 会以 exit 0 打印根树列表，该守卫也把这个形状挡在解析器之外。建议修复：补 base 模式的孪生测试：提交 .qwen/review-context.json -> ..，断言两种模式 artifact 均为 null。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| "${QWEN_CODE_CLI:-qwen}" review repo-context \ | ||
| --plan <absolute-plan-path> \ | ||
| --worktree <absolute-worktree-path> \ | ||
| --out <absolute-context-path> |
There was a problem hiding this comment.
[Suggestion] R4-28: The repo-context block is the only command block in the skill leaving its --out artifact path to improvisation — every other block pins an exact .qwen/tmp/qwen-review--* path — so nothing keeps the artifact inside the cleanup prefix and nothing keeps it away from the manifest the command itself reads. — Failure scenario: Probe-verified on unmodified code: (1) cleanup sweeps only files starting with qwen-review--, so an improvised artifact name accumulates across reviews; (2) in a local review, filling with /.qwen/review-context.json gets no refusal (the command guards only --out ≠ --plan) and the computed artifact overwrites the user's tracked manifest — exit 0, destruction unnoticed, and every future repo-context run fails closed ('unknown or missing fields') until manually restored.
Suggested fix: Pin the artifact path like every sibling block (--out .qwen/tmp/qwen-review--repo-context.json), and optionally refuse an --out that resolves to the worktree's .qwen/review-context.json.
中文说明
repo-context 命令块是 skill 中唯一把 --out artifact 路径留给临场发挥的命令块——其他所有块都钉死精确的 .qwen/tmp/qwen-review--* 路径——因此没有任何机制把 artifact 保留在 cleanup 前缀内，也没有任何机制让它避开该命令自己要读的 manifest。失败场景（在未修改代码上探针验证）：(1) cleanup 只清扫以 qwen-review-- 开头的文件，临场命名的 artifact 会跨 review 累积；(2) local review 中把 填成 /.qwen/review-context.json 不会有任何拒绝（命令只守卫 --out ≠ --plan），计算出的 artifact 会覆盖用户受跟踪的 manifest——退出码 0、破坏无感知，该仓库此后每次 repo-context 都 fail-closed（'unknown or missing fields'）直到手工恢复。建议修复：像其他兄弟块一样钉死 artifact 路径（--out .qwen/tmp/qwen-review--repo-context.json），并可选地拒绝解析到 worktree manifest 的 --out。
— qwen3.8-max via Qwen Code /review (v0.21.6)
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 79 passed · 0 failed · 79 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：79 通过 · 0 失败 · 79 总计 Verification reportVerification report — PR #8401
|
| # | round-1 finding | severity | status at aa8c7cb9 |
|---|---|---|---|
| 1 | validateGlob's backslash-pattern rejection is pinned by no unit test (M3 mutant survived; shipped behavior correct) |
Suggestion | stands — re-measured: the M3 mutant again survives the manifest suite; head behavior still fails closed (cells G1/G2); the mutant silently skips (P1: exit 0 + null artifact). A measured drop-in fix is provided below (Finding 1). |
| 2 | Walker entry.isSymbolicLink() continue is redundant defense-in-depth (M4 mutant survived) |
nit | stands — mutant survives again; P3 shows the artifact stays byte-identical with the line removed. Keep-or-drop is the author's call. |
| 3 | Description details stale ("386 tests", local @octokit/rest build stoppage) |
nit | stands, count drifted further — the same 7-file plan now passes 510 tests at this head; full typecheck and the job's build are green in-container. |
No carried-over finding worsened; none was fixed either (the "pin round-2/round-3 gaps" commits pinned the new guards — budget, boundary, caps — not finding 1's backslash-pattern case).
中文摘要
- 结论：merge-ready。79 项脚本化断言全部通过，0 项意外失败。这是跟进轮（上一轮在 head
704e9f74判定 merge-ready）。 - A/B 结论（重新实测，未沿用旧报告数字）：base 上
review repo-context不存在（exit 1、无产物）；head 上同一 fixture 产出合并/排序/去重的 context 并原子更新 plan（见01-ab-cells-base-vs-head-and-new-guards.png，48 条断言）。 - 信任边界：B1–B5 全部重跑——head 侧改写被忽略、新增得 null、删除仍取 base、
mergeBaseSha: null与baseFetchFailed: true均降级为 null。 - 本轮新增守卫全部验证：① identity 读取 1 MiB 上限——worktree 侧 stat 层与 base 侧 parser 层双向 fail closed，边界值（恰好 1 MiB）被接受（I1–I3）；② 按长度计费的匹配预算——过滤器阶段超预算 1 条路径即 fail closed，恰好等于预算被接受（W1/W2），
>与>=边界由单测钉住（M7 突变被杀）。 - 突变矩阵 8 行：round-1 两个存活突变复测仍存活（均为已知非阻断项）；新守卫的 4 个突变全部被杀，含正向对照（
02-mutation-matrix-8-rows.png）。 - findings（均非阻断）：①（沿用）反斜杠 glob 守卫仍无单测 pin——已实测一条可直接落地的补测（head 绿、突变红，
06-measured-suggested-fixes.png）；②（沿用）walker 符号链接跳过为冗余纵深防御；③（沿用）描述中 "386 tests" 已过时（现 510）；④（新）identity 超限测试实际由 parser 层钉住而非其名称所指的 reader stat 层——亦附实测修正。 - 未覆盖：逐 commit 归因（depth 2，快照 20 个 commit 本地不可达）；真实 GitHub PR 流（无 token）；roster/prompt 消费侧仅套件+突变验证；Windows/macOS 平台行为；展开阶段匹配预算仅单测钉住（E2E 钉的是过滤器阶段）。
Scope
Central claim (unchanged from round 1): qwen review repo-context + the manifest provider make the review pipeline repository-aware — a strict-JSON .qwen/review-context.json yields a validated, merged/sorted/unique repositoryContext in the plan plus an independent artifact; no manifest / no match → deterministic null; malformed → fail closed.
Secondary claims: (1) PR-mode manifests come only from the trusted merge-base commit — no opt-in/opt-out/rewrite from the head; degraded base states yield null, never a head read. (2) Glob/path validation, bounded symlink-safe expansion, and inert disclosure rendering.
New this round (delta since 704e9f74, per the commit titles — per-commit diffs unreachable at depth 2): MAX_IDENTITY_BYTES identity-read cap (worktree stat layer + parser content layer) and MAX_MATCH_WORK length-billed matching budget (pattern.length × path.length per attempt) in both the rule filter and the expansion; plus the serve-streaming integration suite's hermetic-workspace change.
Central claim — A/B table (re-measured at the new head)
Witness: evidence/01-ab-cells-base-vs-head-and-new-guards.png (full 48-assertion run as printed).
| cell | environment | oracle | result |
|---|---|---|---|
| A0 base positive control | base tree (HEAD^1) via tsx, review parse-args --help |
exit 0 + help text | PASS (base arm viable) |
| A1 BASE | base tree via tsx, repo-context on manifest-bearing fixture |
exit ≠ 0, error names unknown command, no artifact | PASS — command absent on base (exit 1) |
| A1 HEAD (dist) | job-built dist/index.js, same fixture |
exit 0; artifact deep-equals merged/sorted/unique context; plan updated; stdout names provider manifest |
PASS |
| A1 HEAD (tsx) | head source via tsx, same fixture |
artifact identical to dist cell | PASS (runtime symmetry; also validates the mutant-E2E vehicle) |
| A2 HEAD | changed path matches no rule | exit 0, null artifact, pre-seeded stale plan.repositoryContext removed |
PASS |
| A3 HEAD | no manifest at all | exit 0, null artifact |
PASS |
Trust boundary (real scratch git repos with real commits, all re-run):
| cell | setup | expected | result |
|---|---|---|---|
| B1 | manifest in base ("Trusted Base"), rewritten on head ("EVIL HEAD", requiredAgents: ["6a"]) |
context from base | PASS — label Trusted Base, base domains, 6a absent |
| B2 | manifest added only on head | null (no opt-in) |
PASS |
| B3 | manifest deleted on head | context still from base (no opt-out) | PASS |
| B4 | mergeBaseSha: null, manifest present in worktree |
null, no worktree fallback |
PASS |
| B5 | baseFetchFailed: true + resolvable sha |
null |
PASS |
New guard 1 — identity-read size cap (MAX_IDENTITY_BYTES = 1 MiB)
| cell | probe | expected | result |
|---|---|---|---|
| I1 | local manifest cap + 1 bytes |
nonzero, reader-layer message, no artifact, plan untouched | PASS — identity read exceeds the size limit |
| I2 | local manifest exactly cap bytes (whitespace-padded valid JSON) |
accepted | PASS — exit 0, context attached |
| I3 | merge-base blob cap + 10 non-whitespace bytes (PR mode) |
nonzero via parser layer | PASS — repository context manifest exceeds the size limit; symmetry across modes holds |
Adjudication P2 (mutant with the stat check removed): an oversized local identity still fails closed via the parser layer, and the reader-layer message is absent — the stat check is I/O-memory defense-in-depth; the fail-closed contract is load-borne by the parser (see Finding 4).
New guard 2 — length-billed matching-work budget (MAX_MATCH_WORK = 2³⁰)
| cell | probe | expected | result |
|---|---|---|---|
| W0 | billing arithmetic sanity (512×512 per attempt; 32 paths × 128 patterns == budget) | identities hold | PASS |
| W1 | 33 distinct 512-char changed paths × 128 rules with a 512-char pattern (one path past the exact-budget point) | fail closed, filter-stage message, no artifact | PASS — paths matching work exceeds limit, exit 1 |
| W2 | 32 such paths — charged work exactly MAX_MATCH_WORK, nothing matches |
exit 0, null artifact, no review error on stderr |
PASS |
| boundary pin | unit test "accepts matching work sitting exactly at the budget" asserts the ×-identity itself |
> vs >= regressions fail |
M7 mutant (> → >=) KILLED by it |
Calibration re-measured at this head (it moved since round 1): the installed packages/ tree is 8,100 entries — 2.02× headroom under the 16,384 visited-entry cap (round 1: 7,731) — and a legitimate full-tree scan against 6 realistic globs bills ~7.4% of the matching-work budget. Both "honestly scoped manifests never trip it" claims hold.
Corrections to round 1
- Round 1's backslash-glob E2E cell used the shape
src\**. This round discovered that shape is rejected by two independentvalidateGlobclauses — the backslash clause and the "**must be a whole segment" clause (src\**contains**inside a larger segment). Exclusive evidence for the backslash clause needs a**-free shape: cell G2 (src\a.ts) fails closed on head, andP1/P1-controluse it for the mutant pair. Round 1's conclusion (behavior correct, guard unpinned) is unchanged; its evidence base is now exclusive. - Round 1's calibration number (7,731 entries) is superseded by 8,100 at this head — the claim holds with fresh numbers, as above.
Findings (all non-blocking)
- Coverage gap (carried over): the backslash-pattern guard is still pinned by no unit test (Suggestion). Re-measured: with the clause removed (M3-rerun) the manifest suite stays green (exit 0), yet the same build driven through the real CLI silently skips a backslash-pattern manifest —
P1: exit 0 with anullartifact where head fails closed (P1-control,G2). SKILL.md instructs the orchestrator to stop the review on a nonzero exit; a silentnullis precisely the failure mode that instruction exists to prevent. Measured drop-in fix (06-measured-suggested-fixes.png): add oneit.eachrow['unsafe backslash glob', manifest({ rules: [{ paths: ['src\\a.ts'] }] })]— the suite stays green and grows by exactly one test on head, and turns red on exactly that row under the M3 mutant. - Redundant guard (carried over): walker symlink skip (nit). M4-rerun survives;
P3shows the artifact is byte-identical with the line removed —visit()'slstatSync(...).isSymbolicLink()early return (dir symlinks) and the!entry.isFile()check (file symlinks report lstat type) already cover the escape. Keep or drop; the escape behavior itself remains correct and E2E-pinned (cell S2). - Stale description (carried over) (nit). "386 tests pass" → the same 7-file plan passes 510 at this head (
04-targeted-gates.png,05-raw-suite-summaries.png); the@octokit/restbuild stoppage still does not reproduce in-container (job build + full typecheck green). - New: the oversized-identity test is pinned by the parser layer, not the reader layer its name claims (nit).
fails closed when the local identity exceeds the size limitasserts the substringexceeds the size limit, which both layers emit — removing the reader's stat check (M5-new) leaves the suite green becauseparseManifestthrowsmanifest exceeds the size limitwith a matching substring. The reader layer exists to reject before reading a multi-megabyte attacker file into memory; nothing pins that. Measured fix: tighten the assertion to'identity read exceeds the size limit'— green on head (same test count), red under the M5 mutant (06-measured-suggested-fixes.png). Completeness reporting, not a merge condition — shipped behavior fails closed in both layers (I1, P2).
Mutation matrix (8 rows + positive control)
Witness: evidence/02-mutation-matrix-8-rows.png; adjudication probes evidence/03-mutant-e2e-adjudication.png. Each mutant applied to source, targeted suite run, file restored (post-run git status clean: true).
| mutant | guard removed/changed | suite | result | adjudication |
|---|---|---|---|---|
| M3-rerun | validateGlob backslash clause |
manifest | SURVIVED | coverage gap — Finding 1; P1 silent skip |
| M4-rerun | walker entry.isSymbolicLink() skip |
manifest | SURVIVED | redundant guard — Finding 2; P3 byte-identical |
| M5-new | identity reader stat-size check | repo-context | SURVIVED | defense-in-depth; parser layer load-bearing — Finding 4; P2 |
| M6-new | rule-filter matching-work budget disabled | manifest | KILLED | pinned by "fails closed when rule-filter matching work exceeds the budget" |
| M7-new | expansion budget > → >= |
manifest | KILLED | pinned by "accepts matching work sitting exactly at the budget" |
| M8-roster | context-role medium-effort gate negated | roster | KILLED | pinned (context cannot inflate a medium review) |
| M9-inert | disclosure drops mdField inert wrapping |
compose-review | KILLED | pinned — Reviewer Test Plan step 6 (inert rendering, @mention-safe) |
| PC | manifest version !== 1 → !== 99 |
manifest | KILLED | positive control — harness calibration valid |
Targeted gates (all proven live)
Witness: evidence/04-targeted-gates.png, evidence/05-raw-suite-summaries.png.
| gate | result | aliveness control |
|---|---|---|
PR's 7-file vitest plan (packages/cli) |
510/510 passed, 7/7 files | mutation kills above prove the suites bite |
npm run typecheck (8 workspaces) |
0 errors | — |
changed integration suite qwen-serve-streaming.test.ts (the only non-review delta) |
4/4 passed on a real daemon (hermetic scratch workspace) | — |
ESLint on the 17 changed .ts files |
0 errors | planted any/unused-var file → 2 errors, exit 1 |
Prettier --check on the 20 changed files |
clean | explicit file list matched |
git diff HEAD^1..HEAD --check |
clean | n/a |
| scan calibration (G9/G10) | 8,100 entries ≤ 16,384; legit scan ~7.4% of budget | recomputed from the live tree |
Not covered
- Per-commit attribution: depth-2 checkout —
git rev-list --count HEAD^1..HEAD^2returns the bogus value1at the shallow boundary while the metadata snapshot lists 20 commits; the aggregateHEAD^1..HEADdiff was verified. The round-delta commit claims ("cap identity reads", "bill match work by length", "isolate serve streaming suite") were verified by behavior (guards probed, suite run) rather than per-commit diffing. - Real GitHub PR flow: no token in this environment; the trust boundary was proven against real local git repos exercising the same code path (
git ls-tree/git showon the recorded sha), not a livefetch-prround-trip. - Roster/prompt consumption end-to-end: verified via suites + mutation kills (M8/M9) and source reading; no full multi-agent review was driven. The
agent-promptblocks render manifest text raw into agent prompts — recorded in round 1 as consistent with the pipeline's posture for repo-committed rules; unchanged this round. - Expansion-stage matching-work budget E2E: pinned by the unit's deep-chain cell; the E2E budget cells (W1/W2) exercise the filter stage. Expansion budget E2E would need a 198-deep fixture tree for marginal extra signal.
- Windows/macOS behavior:
skipIf(win32)backslash-filename tests and CRLF normalization were not exercised on those platforms (Linux container). - Base-mode symlinked-intermediate-component divergence: documented in the design doc as fail-safe (base mode reads strictly less); not exercised.
- Repo-wide test suite beyond the targeted workspaces (scope choice); no repository has adopted a manifest yet, so no end-to-end adoption run exists (foundation PR, by design).
Methodology
Environment: CI verify container (node:22-bookworm), merge-ref checkout (HEAD merge commit, HEAD^1 base tip, HEAD^2 PR head), job-prebuilt dist/ at HEAD. Harness harness/cells.mjs is mock-free: it builds scratch git repositories with real commits, writes plans/manifests, and drives the built head CLI (node packages/cli/dist/index.js) and the base CLI (npx tsx <base worktree>/packages/cli/src/cli.ts, base worktree at HEAD^1, removed after capture). Confound controls: node_modules/@qwen-code/qwen-code-core realpath resolves into the head tree on both arms, but the PR's core delta is SKILL.md only (verified empty otherwise) and package.json/package-lock.json are untouched, so the base control is clean. Mutation runners (run-mutations.mjs, mutant-e2e.mjs, fix-verify.mjs) apply single-point edits with try/finally restores and assert git status --porcelain clean afterwards (it was). Gates (gates.mjs) re-run lint/format live with planted-violation controls and assert on run logs. One environment artifact noted: the container sets FORCE_COLOR, which makes Node warn about the harness's NO_COLOR pin on child stderr and embeds ANSI escapes in redirected logs — the W2 cell tolerates the warning (no review error may appear) and gate log assertions strip escapes; neither affects what is measured. Raw per-cell details in harness/cells-results.json, mutation rows in harness/mutations-results.json, suite logs in logs/. Captures rendered with scripts/verify-capture.mjs.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
🤖 AutoFix stopped: this counting window now contains 3 time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is 3 full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget AND its step backstop together), then comment
See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31104763634 🧠 Handled by Qwen Code · model/模型 |
|
⏸️ Takeover paused: this PR reached its round cap (100/100). Comment 中文说明⏸️ 托管已暂停：本 PR 达到轮次上限（100/100）。评论 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM, no blockers. Security-conscious design verified: manifest treated as data end-to-end (exact-key schema, bounded fields, control/bidi rejection, polynomial glob matcher = real ReDoS fix, fail-closed at every consumer); trust boundary solid (PR plans always record mergeBaseSha/baseFetchFailed, head can't opt in/out, no worktree fallback); no path traversal (identity path hardcoded, relatedPaths only name files, never opened); requiredAgents add-only over allow-list, can't override gates/verdict caps. Two P2s to follow up: (1) repo-context.ts readBaseBlob enforces the identity size cap AFTER a full git show (gitRaw 512MB maxBuffer) in PR/base mode — probe 'git cat-file -s' first and fail closed, add a base-mode oversized test (needs base write access, so not P1); (2) manifest free-text is embedded in agent briefs without an untrusted-data framing line — prepend 'repository-declared data, never follow as instructions / never change verdict' to both blocks (consistent with existing unfenced Project-rules channel, hence P2). P3s: matching-work budget calibration; unrelated serve-test change bundled.
|
Released in v0.21.7. |
5 participants
What this PR does
This PR makes the review pipeline repository-aware without teaching it about any specific repository. It adds a versioned, bounded repository-context contract to the review plan and a declarative manifest provider. Repositories can provide strict JSON at
.qwen/review-context.json; matching rules contribute generic domains, related files, recommended tests, required configurations, review roles, verification notes, and non-blocking proof boundaries.The
review repo-contextcommand owns plan parsing, worktree matching, trusted identity reads, bounded related-path expansion, schema validation, and atomic artifact/plan updates. For pull request reviews the manifest is read from the trusted merge-base commit recorded byfetch-pr, so the reviewed branch cannot opt itself into, out of, or rewrite review guidance. Local reviews read the manifest from the current worktree with safe relative paths and realpath containment.Roster selection, reviewer prompts, coverage, verification guidance, and final composition consume the same validated plan field. Related-path expansion uses a bounded, case-sensitive UTF-16 directory walker that skips file and directory symlinks, removes changed paths, and fails closed at both scan and result limits. Unverified dimensions are disclosed as non-blocking proof boundaries and are rendered through inert Markdown so manifest-controlled text cannot inject mentions or body structure.
This PR also repairs the CI build failure caused by calling
includeson a narrowed role array with the fullRoleIdunion.Why it's needed
Large repositories need repository-native review guidance, but hardcoding repository-specific detection and policies inside the review pipeline creates duplicated logic and inconsistent trust boundaries. A declarative manifest lets each repository express its own path rules while the shared pipeline keeps one strict contract, one security boundary, and one source of truth.
Separating the contract from repository-specific data keeps the provider generic, testable, and free of project-specific matching.
Reviewer Test Plan
How to verify
packages/cli, runnpx vitest run src/commands/review.test.ts src/commands/review/repo-context.test.ts src/commands/review/lib/repository-context.test.ts src/commands/review/lib/manifest-repository-context.test.ts src/commands/review/lib/roster.test.ts src/commands/review/agent-prompt.test.ts src/commands/review/compose-review.test.ts. Expected: 386 tests pass.git diff --check. Expected: no errors.qwen review repo-contextagainst a tree containing.qwen/review-context.json. Expected: matched rules produce merged, sorted, unique context fields; unrelated changes producenull; malformed manifests fail closed.@mentions.Evidence (Before & After)
Before: repository-context foundation had no production provider, so
repo-contextalways wrotenull, and CI failed withTS2345when narrowing repository-context role inclusion.After: a static manifest provider makes the command usable by any repository; 386 focused tests pass locally, targeted lint/format checks pass, and the previous
TS2345build failure is repaired through an explicit type guard.Tested on
Environment (optional)
macOS local Node.js development environment. Focused Vitest: 386 passed. Targeted ESLint, Prettier, and
git diff --checkpassed.npm run buildproceeds past the previousagent-prompt.tsTypeScript error and stops only at a pre-existing unrelated@octokit/restissue in the GitHub channel package.Risk & Scope
repo-context; this is intentional because review metadata is security-sensitive, and the strict schema keeps failures deterministic.repositoryContextkeep their current behavior, and repositories without a manifest deterministically receive no repository context.Linked Issues
N/A
中文说明
此 PR 做了什么
此 PR 让 review pipeline 具备仓库感知能力，同时不内置任何特定仓库的匹配逻辑。它在 review plan 中增加带版本、有界的 repository-context contract，并提供声明式 manifest provider。仓库可通过
.qwen/review-context.json提供严格 JSON；匹配规则贡献通用 domains、related files、recommended tests、required configurations、review roles、verification notes 和 non-blocking proof boundaries。review repo-context命令负责 plan 解析、worktree 匹配、可信 identity 读取、有界 related-path 展开、schema 验证以及 artifact/plan 原子更新。PR review 从fetch-pr记录的可信 merge-base commit 读取 manifest，因此 reviewed branch 不能自行 opt in、opt out 或改写 review guidance。Local review 在当前 worktree 读取 manifest，并执行安全相对路径和 realpath containment。Roster 选择、reviewer prompt、coverage、verification guidance 和最终 composition 消费同一个经过验证的 plan 字段。Related-path 展开使用有界、case-sensitive、UTF-16 的目录遍历，跳过文件和目录 symlink，移除 changed paths，并在扫描与结果两个上限处 fail closed。Unverified dimensions 作为 non-blocking proof boundary 披露，并通过 inert Markdown 渲染，防止 manifest 控制的文本注入 mention 或正文结构。
此 PR 还修复了因完整
RoleIdunion 调用窄化 role array 的includes导致的 CI 构建失败。为什么需要
大型仓库需要 repository-native review guidance，但把仓库特定识别和策略硬编码进 review pipeline 会造成重复逻辑与不一致的 trust boundary。声明式 manifest 让每个仓库表达自己的路径规则，同时共享 pipeline 保持一个严格 contract、一个安全边界和一个事实源。
把 contract 与仓库特定数据分开，可以让 provider 保持通用、可测试，且不包含项目专用匹配。
Reviewer 测试计划
如何验证
packages/cli运行npx vitest run src/commands/review.test.ts src/commands/review/repo-context.test.ts src/commands/review/lib/repository-context.test.ts src/commands/review/lib/manifest-repository-context.test.ts src/commands/review/lib/roster.test.ts src/commands/review/agent-prompt.test.ts src/commands/review/compose-review.test.ts。预期：386 项测试通过。git diff --check。预期：无错误。.qwen/review-context.json的 tree 上运行qwen review repo-context。预期：匹配规则产生合并、排序、去重的 context 字段；不相关改动产生null；malformed manifest fail closed。@mention的值。证据（Before & After）
Before：repository-context foundation 没有生产 provider，因此
repo-context总是写入null；CI 因 repository-context role 收窄时的TS2345失败。After：静态 manifest provider 使该命令可被任何仓库立即使用；本地 386 项 focused tests 通过，targeted lint/format 通过，先前的
TS2345构建失败已通过显式 type guard 修复。测试平台
环境（可选）
macOS 本地 Node.js 开发环境。Focused Vitest：386 passed。Targeted ESLint、Prettier 和
git diff --check通过。npm run build已越过先前的agent-prompt.tsTypeScript 错误，仅在既有且无关的 GitHub channel 缺少@octokit/rest处失败。风险与范围
repo-context的 medium/high review fail closed；这是有意为之，因为 review metadata 具有安全敏感性，严格 schema 可保持失败确定。repositoryContext的现有 plan 保持当前行为；没有 manifest 的仓库确定地不获得 repository context。关联 Issue
N/A