Skip to content

fix(multi-review): address review issues from PR #196#208

Merged
Svtter merged 1 commit into
farm/d9cd57e8/multi-review-fails-when-pr-diff-containsfrom
fix/196-review-issues
Jun 7, 2026
Merged

fix(multi-review): address review issues from PR #196#208
Svtter merged 1 commit into
farm/d9cd57e8/multi-review-fails-when-pr-diff-containsfrom
fix/196-review-issues

Conversation

@Svtter

@Svtter Svtter commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes all blocking issues and suggestions identified in the AI review of #196.

Blocking Issues Fixed

  1. globToRegex **/ prefix bug**/vendor now correctly matches top-level vendor directory, not just nested paths. Rewrote globToRegex with context-aware globstar replacement:

    • Leading **/(.+/)? (zero or more leading path segments)
    • Trailing /**(/.+)? (zero or more trailing path segments)
    • Middle /**/ → handled correctly
  2. originalBytes semantic mismatch — Renamed to filteredBytes. Log message changed from "KB original" to "KB after filtering" so users understand the value is post-filtering, not raw input.

  3. Empty diff when first section exceeds budget — Changed truncation loop to always keep at least the first section: if (truncatedKept.length > 0 && size > budget) break. Prevents sending empty content to LLM.

Suggestions Implemented

  1. action.yml description updated — Now documents gitignore convention: "Patterns without `/` match against the file basename; patterns with `/` match against the full file path."

  2. Trailing newline added to diff-filter.ts

  3. bun.lock removed — Accidentally committed in the original PR.

Tests

  • Added "**/pattern matches top-level file (gitignore semantics)" — verifies **/vendor matches both vendor and subdir/vendor
  • Added "keeps at least the first section when it exceeds maxSizeBytes" — verifies no empty diff edge case
  • All 42 tests pass

Fixes review issues from #196

- Fix globToRegex: ** at start now matches top-level paths (gitignore semantics)
  **/vendor matches 'vendor' at any depth including root
  vendor/** matches any file under vendor/
- Rename originalBytes → filteredBytes for semantic accuracy
  Log message now says 'after filtering' instead of 'original'
- Always keep at least first section when truncating to avoid empty diff
- Update action.yml description to document gitignore convention for patterns
- Add trailing newline to diff-filter.ts
- Remove bun.lock that was accidentally committed
- Add tests for **/ top-level matching and empty-first-section edge case
@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown

分析总结

根据 PR 标题和 body("address review issues from PR #196"),需求即为 PR body 中列出的 6 项修复 + 2 个测试。逐一验证如下:

# 需求项 实现位置 状态
1 globToRegex **/ 前缀 bug(顶层 vendor 应匹配) diff-filter.ts:46-73 — 上下文感知替换:前导 **/(.+/)?,后置 /**(/.+)?,中间 /**/ 正确处理
2 originalBytesfilteredBytes,日志改为 "KB after filtering" diff-filter.ts:36,147,169-171 + index.ts:88,96
3 首段超 budget 时保留至少一段,不发送空 diff diff-filter.ts:156if (truncatedKept.length > 0 && size > budget) break;
4 action.yml diff-exclude 描述补充 gitignore 约定 action.yml:129-131
5 diff-filter.ts 末尾换行 文件末尾有换行
6 删除误提交的 bun.lock 文件已删除(+0/-297)
7a 测试:**/pattern matches top-level file diff-filter.test.ts:252-263
7b 测试:keeps at least the first section when it exceeds maxSizeBytes diff-filter.test.ts:265-275

所有需求均完整实现,无遗漏。

无遗漏

New%20session%20-%202026-06-07T07%3A09%3A02.469Z
opencode session  |  github run

@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown

有条件合并 / CONDITIONAL MERGE

总结: PR 的 glob 语义改进和截断保底逻辑整体方向正确,但存在 2 个阻塞问题需修复。

阻塞项:

  1. parseExclude/** 尾缀导致递归排除失效(quality)
    vendor/** 这类 pattern 被错误设置了 matchFull = false,导致仅用 basename 与正则比对,目录递归排除完全失效。应移除 !pattern.endsWith("/**") 条件,或改为 matchFull = hasSlash && !pattern.startsWith("**/")

  2. 删除 bun.lock 但未提交替代锁文件(security)
    移除锁文件后依赖版本不再锁定,构建不具有确定性,可能引入供应链攻击风险。如已切换包管理器,应提交对应的 package-lock.jsonyarn.lock

建议项:

  1. action.yml 文档称"含 / 的 pattern 匹配完整文件路径",但代码对 **//** 做了例外处理,两者不一致,应统一文档或代码(quality)。
  2. vendor/**(目录递归排除)添加测试用例,防止回归(quality)。
  3. globToRegex 接受环境变量输入,建议增加模式长度和复杂度限制以防范 ReDoS(security)。
  4. 补充文档说明 glob pattern 的作用范围,避免用户意外排除应审查的文件(security)。
  5. filteredBytes 若后续有二次修改可考虑更精确的命名如 totalBytesBeforeTruncation(architecture)。
  6. 若未来 glob 模式集扩展(如 a/**/b),建议补充对应单元测试(architecture)。

📋 各 Reviewer 详细审查结果
quality

有条件合并 / CONDITIONAL MERGE

此 PR 改进了 ** 通配符的 gitignore 语义支持、重命名 originalBytesfilteredBytes、并确保截断时至少保留第一个 section。但存在一个回归 bug。

阻塞项:

  1. parseExclude 函数中对末尾为 /** 的 pattern 设置了 matchFull = false,导致 vendor/** 这类 pattern 退化到仅匹配 basename。例如文件 vendor/pkg/file.ts 仅用 basename file.ts 与 regex ^vendor(/.+)?$ 比对,无法匹配,因此目录递归排除完全失效。旧行为(pattern 含 / 即匹配全路径)是正确的。应移除 !pattern.endsWith("/**") 条件,或改为 matchFull = hasSlash && !pattern.startsWith("**/")

建议项:

  1. action.yml 文档说明"含 / 的 pattern 匹配完整文件路径",但代码对 **//** 做了例外处理,两者不一致。应统一文档或代码。
  2. 建议为 vendor/**(目录递归排除)添加测试用例,防止回归。
security

存在风险 / AT RISK

本次 PR 主要改进了 glob pattern 匹配逻辑以支持 gitignore 语义,并修正了 diff 截断逻辑确保至少保留首个文件段,同时删除了 bun.lock 锁文件。

阻塞项:

  • 删除了 multi-review/bun.lock 锁文件但未在 PR 中添加替代锁文件。移除锁文件后依赖项版本不再锁定,构建不再具有确定性,可能引入供应链攻击风险(依赖混淆、恶意包投毒)。如果已切换包管理器,应提交对应的 package-lock.jsonyarn.lock

建议项:

  • globToRegex 接受用户输入(来自 MULTI_REVIEW_DIFF_EXCLUDE 环境变量)并动态生成正则表达式。虽然当前实现了特殊字符转义,但建议增加模式长度和复杂度限制,以防范 ReDoS(正则表达式拒绝服务攻击)。
  • 当前 glob pattern 支持 ** 通配且允许用户配置排除规则,建议补充文档说明 pattern 的作用范围,避免意外排除应审查的文件。
performance

性能良好 / GOOD

该 PR 不涉及性能问题。主要变更包括:

  • action.yml:仅文档说明更新,不影响运行时性能
  • diff-filter.tsglobToRegex 改进 ** 匹配(遵循 gitignore 语义),算法复杂度仍为 O(n);截断逻辑优化确保首个 section 即使超预算也被保留,避免空 diff
  • index.ts 和 test 文件:仅变量重命名(originalBytesfilteredBytes)和日志文案调整
  • bun.lock 删除:仅影响构建/CI,不影响 runtime

无数据库查询、无并发问题、无内存泄漏、无冗余计算。

阻塞项:无

建议项:无

architecture

架构合理 / SOUND

此次 PR 对 multi-review 模块进行了增量改进,整体架构合理,变更范围良好地限定在模块内部。

变更摘要

  • diff-filter.ts: 增强 glob 模式匹配以支持 gitignore 风格的 **/ 语义;截断逻辑确保始终保留第一个 section(避免空 diff);originalBytes 重命名为 filteredBytes
  • diff-filter.test.ts: 新增 **/pattern 测试和截断保底测试
  • index.ts: 适配 filteredBytes 重命名及日志文案调整
  • action.yml: 文档更新,明确 basename vs full path 匹配规则
  • bun.lock: 删除(配置文件清理)

架构分析

  • 耦合: 无新增跨模块依赖,变更完全限定在 multi-review/ 目录内
  • 模块放置: 所有源文件位于 multi-review/src/,符合既有结构;bun.lock 删除是合理的仓库清理
  • 分层: diff-filter.ts 专注 diff 处理逻辑,index.ts 负责编排,职责清晰
  • 接口设计:
    • originalBytesfilteredBytes 重命名语义更准确(此时已是过滤后的字节数)
    • 截断保底策略避免了向 LLM 发送空 diff 的边缘情况
    • glob 模式增强遵循 gitignore 惯例,降低了用户认知成本
  • 霰弹式修改: 不存在 — 3 个文件的对应修改是同一逻辑变更的自然传播,且全部在单模块内
  • 一致性: 与现有编码风格和架构模式一致

阻塞项:无

建议项:

  1. filteredBytes 严格来说包含的是截断前的过滤后总字节数,若后续有二次修改可考虑更精确的命名如 totalBytesBeforeTruncation,但当前命名已明显优于原 originalBytes
  2. globToRegex 中 fallthrough 分支对中间 ** 使用 .* 是可接受的,如果未来 glob 模式集扩展(如 a/**/b),建议补充对应单元测试

@Svitter Svitter added fix Bug fix setup Setup and installation triaged Issue has been triaged review:p0 Critical review findings labels Jun 7, 2026

@Svitter Svitter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 — lgtm. Tightly scoped follow-up that fixes three real bugs from #196 without collateral changes.

  • globToRegex rewrite: **/vendor now correctly matches top-level vendor via (.+/)? — the old .*/vendor required at least one leading segment. Trailing /** and middle ** cases preserved. The bare ** fallback (no surrounding slashes) returns .* which is correct for that degenerate case.
  • filteredBytes rename: complete — zero references to originalBytes remain. The truncation notice text changed from "KB original" to "KB after filtering", matching the semantics.
  • Truncation loop: truncatedKept.length > 0 guard ensures at least the first section survives even when it alone exceeds budget. Prevents sending empty diff to LLM.
  • bun.lock removal: restores pre-#196 state; package-lock.json is the canonical lockfile.
  • Description update in action.yml documents the gitignore path/basename convention.

All 24 diff-filter tests pass. The single reviewers.test.ts failure is pre-existing — js-yaml not installed in node_modules (unrelated to this PR).

Thanks for the clean fixes.

@Svtter Svtter merged commit 3e60d5a into farm/d9cd57e8/multi-review-fails-when-pr-diff-contains Jun 7, 2026
3 of 4 checks passed
Svtter added a commit that referenced this pull request Jun 7, 2026
…ncation (#196)

* fix(multi-review): add configurable diff exclusion and size-based truncation

The existing lock-file filter strips known lock files from the PR diff,
but large diffs can still exceed the LLM context window when they contain
other auto-generated or vendored content.

Add two new action inputs:
- diff-exclude: comma-separated glob patterns for additional files to
  exclude (gitignore convention: patterns without '/' match basenames)
- diff-max-size-kb: maximum diff size in KB after filtering; if exceeded,
  only leading file sections are kept with a truncation notice

The filterDiff function now accepts FilterDiffOptions with excludePatterns
and maxSizeBytes. Truncation keeps whole file sections from the start and
appends a notice showing how many of the total sections were included.

Fixes #194

* chore: add bun.lock for multi-review

* fix(multi-review): address review issues from PR #196 (#208)

- Fix globToRegex: ** at start now matches top-level paths (gitignore semantics)
  **/vendor matches 'vendor' at any depth including root
  vendor/** matches any file under vendor/
- Rename originalBytes → filteredBytes for semantic accuracy
  Log message now says 'after filtering' instead of 'original'
- Always keep at least first section when truncating to avoid empty diff
- Update action.yml description to document gitignore convention for patterns
- Add trailing newline to diff-filter.ts
- Remove bun.lock that was accidentally committed
- Add tests for **/ top-level matching and empty-first-section edge case

* chore(multi-review): rebuild dist after diff-filter changes

---------

Co-authored-by: Svitter <Svitter@users.noreply.github.com>
Co-authored-by: 修昊 <svtter@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug fix review:p0 Critical review findings setup Setup and installation triaged Issue has been triaged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants