Skip to content

fix: address review feedback from PR #176#178

Merged
Svtter merged 1 commit into
mainfrom
fix/hash-ref-review-v3
Jun 3, 2026
Merged

fix: address review feedback from PR #176#178
Svtter merged 1 commit into
mainfrom
fix/hash-ref-review-v3

Conversation

@Svtter

@Svtter Svtter commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses all suggestions from PR #176 multi-review (quality, performance reviewers).

Changes

Inline code handling (platform.ts)

  • Added INLINE_CODE_RE to skip `#1` inline code during escaping
  • Extracted escapeSegment() helper to handle nested fenced + inline code exclusion

JSDoc and documentation

  • Added comprehensive JSDoc to escapeHashReferences documenting coverage scope
  • Added comments about g flag behavior on HASH_NUM_RE
  • Renamed CODE_BLOCK_RE to FENCED_CODE_RE for clarity

TS unit tests (eliminates dual-source-of-truth risk)

  • Added platform.test.ts using node:test — 14 tests directly testing the actual function
  • No longer relies on Python re-implementation of the regex logic

Cross-language consistency enforcement

  • Added TestCrossLanguageHashInstructionConsistency in Python test suite
  • Validates ZH and EN hash-avoidance strings match between reviewers.ts and run-github-opencode.py
  • Extracted hash_avoid_zh/hash_avoid_en variables in Python to mirror TS structure

Dev dependencies

  • Added tsx for running TS tests
  • Added npm test script

Testing

  • 14 TS unit tests pass (npm test)
  • 56 Python tests pass (including 2 new consistency tests + inline code test)
  • TypeScript build passes (npx tsc --noEmit)

- Handle inline code (single backtick) in escapeHashReferences
- Add comprehensive JSDoc documenting coverage scope
- Add comment about global flag on HASH_NUM_RE
- Export escapeHashReferences for direct testing
- Add 14 TS unit tests (node:test) directly testing the function
- Add cross-language consistency test (ZH/EN hash-avoidance strings
  must match between reviewers.ts and run-github-opencode.py)
- Extract hash_avoid_zh/hash_avoid_en variables in Python to match
  TS structure and ensure one-to-one correspondence
- Add npm test script and tsx devDependency for TS testing

Addresses all confirmed suggestions from PR #176 multi-review.
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

无遗漏

PR 描述中列出的所有改动均已在代码中落实,未发现规格遗漏。

验证结果:

  • Inline code handlingINLINE_CODE_RE 已添加 (multi-review/src/platform.ts:169),escapeSegment() helper 已抽取 (multi-review/src/platform.ts:197-209),并在 escapeHashReferences 中被调用 (lines 187, 193)。
  • JSDoc 与文档escapeHashReferences 的完整 JSDoc 已添加 (platform.ts:171-181),HASH_NUM_REg flag 注释已添加 (lines 158-159),CODE_BLOCK_REFENCED_CODE_RE 重命名完成 (line 166),无残留旧名引用。
  • TS 单元测试multi-review/src/platform.test.ts 已新增,使用 node:test,正好 14 个 it() 测试(lines 6-81),直接 import 真实的 escapeHashReferences 函数。
  • 跨语言一致性TestCrossLanguageHashInstructionConsistency 类已添加 (tests/test_all.py:1023-1077),包含 test_zh_instruction_matchestest_en_instruction_matches 两个测试;hash_avoid_zh/hash_avoid_en 变量已在 github-run-opencode/run-github-opencode.py:459-468 抽取,与 TS 端 HASH_AVOID_ZH/HASH_AVOID_EN 结构对齐,字符串内容已对照一致。
  • Dev 依赖tsx 已加入 multi-review/package.json:19npm test 脚本已添加 (package.json:9),package-lock.json 同步更新。

无 closingIssuesReferences,PR 自身描述即为 spec,所有具体可验证的代码变更项目均已实现,没有发现集成断点、未接线的配置或缺失分支。

New%20session%20-%202026-06-03T12%3A36%3A14.386Z
opencode session  |  github run

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

可合并

四位 reviewer 均无阻塞项。escapeHashReferences 增加 inline code 保护、hash-avoid 提示词抽取共享变量、tsx 接入测试运行器以及新增 TestCrossLanguageHashInstructionConsistency 的整体方向一致认可;本地 14 个 TS 测试 + 17 个 Python 测试 + tsc --noEmit + tsup 构建 + smoke-actions CI 均通过。

阻塞项:无

建议项(按已确认度排序):

已确认(≥ 2 位 reviewer 提及)

  1. tsx 依赖引入方式需收敛(quality / performance / architecture)

    • multi-review/package.json 缺少 engines.node 声明(tsx@4 需 Node ≥ 20)
    • tsx 作为 devDependency 会拉取 esbuild 全平台 native binary(~30MB),npm install 体积与耗时上升
    • 建议二选一:① 在 engines 声明 Node 版本并保持现状;② 直接复用项目已有的 tsupesbuild-register 跑测试,避免新增工具链分支;若保留 tsx,建议在 multi-review/READMEAGENTS.md 注明测试方式,以免与根目录 tests/test_all.py 入口混淆
  2. 跨语言一致性测试与源码字面量形态强耦合(quality / architecture)

    • tests/test_all.py:1031-1059 用正则从源码抽取字符串字面量,断言中硬编码了"ZH 2 段、EN 3 段 ... + ..."的拼接结构
    • 任何提示词定义形式的变更(模板字符串、三引号、Array.join)都会让测试无声失效
    • 建议:① 至少在测试顶部加注释提示"如修改 HASH_AVOID_ZH/EN 或 hash_avoid_zh/en 的定义形式,需同步更新本测试";② 更彻底的做法是将两段文案抽到共享数据文件(如 shared/prompts/hash-avoid.{zh,en}.txt),由 TS 与 Python 各自加载,从根源消除双份字面量
  3. inline code 正则边界可更严谨(quality / security)

    • multi-review/src/platform.ts:169INLINE_CODE_RE = /[^]+/g[^] 跨行匹配,理论上会把 `code\n#1` 整段当 inline code;未闭合反引号场景亦未覆盖
    • 建议改为 /`[^`\n]+`/g,与 Python 版 escape_hash_references 保持完全一致

单一 reviewer 建议(领域特定)

  1. export escapeHashReferences 标注测试入口(architecture)

    • multi-review/src/platform.ts:182 仅因测试需要改为 export,建议在 JSDoc 中加 @internal 标记或旁注,避免被外部模块当作公开 API 误用
  2. Python 镜像测试边界可缩减(architecture)

    • tests/test_all.py::TestEscapeHashReferencesmulti-review/src/platform.test.ts 测的是"同一份逻辑的两个语言实现"。建议明确两套测试边界——Python 侧镜像实现可降级为冒烟级,避免任何 escape 行为变更都需要在 3 个地方(TS 实现、Python 镜像、两套用例)同步修改
  3. npm 镜像供应链可固定(security)

    • package-lock.json 中包源指向 registry.npmmirror.com,虽 integrity sha512 哈希防篡改,建议 CI 固定使用 registry.npmjs.org 或在文档中明确接受该镜像的可信度,以降低供应链单点风险(不影响本次合并)

📋 各 Reviewer 详细审查结果
quality

I'll review this PR carefully. Let me first check the current state of the files to verify the claims.
可合并

PR 干净且完整地解决了 PR #176 的 review 反馈,核心改动包括:把 hash-avoidance 提示词抽取为共享变量、给 escapeHashReferences 增加 inline code 保护、补全两边的测试,并通过新的 TestCrossLanguageHashInstructionConsistency 防止 TS/Python 后续漂移。

我已在本地验证:14 个 TS 测试全部通过、17 个 Python 测试全部通过(含 2 个跨语言一致性用例)、tsc --noEmit 通过、tsup 构建成功且不打包测试文件、smoke-actions CI 已 SUCCESS。

阻塞项:无

建议项:

  • multi-review/src/platform.ts:169INLINE_CODE_RE = /[^]+/g[^] 会跨行匹配,理论上会把 `code\n#1` 整段当成 inline code(虽然 markdown 规范不允许多行 inline code)。如果想更严谨,可改为 /`[^`\n]+`/g,与 Python 版保持完全一致。
  • tests/test_all.py:1030-1059 的跨语言一致性测试用正则从源码里抽取字符串字面量,结构变化(改用模板字符串、三引号、Array.join 等)会立即失败。建议在测试顶部加一行注释说明"如修改 HASH_AVOID_ZH/EN 或 hash_avoid_zh/en 的定义形式,需同步更新本测试",以免后续维护者困惑。
  • multi-review/package.json 未声明 engines.node,而 tsx@4node --import 需要 Node ≥ 20。建议补上 "engines": {"node": ">=20"}tsuptarget: node20 保持一致。
security

安全无虞

安全分析:

  • 改动主要为 escapeHashReferences 函数的内部重构(增加内联代码识别),添加单元测试,以及将 tsx 作为 devDependency 引入用于运行测试
  • 涉及文本变换(插入零宽空格 \u200B)和正则匹配,字符类边界严格,无 ReDoS 风险
  • tsx 是 dev 依赖,不进入 action 生产运行时(run-github-opencode.py 为 Python),不会扩大攻击面
  • 新增的 TestCrossLanguageHashInstructionConsistency 仅以只读方式解析本地源码文件做字符串一致性断言,无文件系统副作用
  • 函数签名仅在测试需要时改为 export,无新增外部 API 面
  • 注释中明确提醒 HASH_NUM_RE 全局标志(/g)的 matchAll 状态共享陷阱,避免了跨调用 lastIndex 污染导致的安全/正确性问题
  • 未引入网络调用、shell 执行、SQL/模板拼接,无注入面
  • 无新增 secret、token、PII 处理逻辑

阻塞项:无

建议项:

  • package-lock.json 中包源指向 registry.npmmirror.com(淘宝/阿里 npm 镜像),虽 integrity sha512 哈希可防篡改,建议团队在 CI 中固定使用官方 registry.npmjs.org 或在文档中明确接受该镜像的可信度,以降低供应链单点风险(不影响本次合并)
  • 内联代码正则 /`[^`]+`/g 在未闭合反引号场景下行为未覆盖,注释中已说明,但若未来 agent 输出包含格式异常的 markdown 段落,可能出现误处理;当前实现对 PR 评论输出已加多语言提示降低发生概率,后续可考虑增加稳健性测试
performance

性能良好

PR 主要是补全 escapeHashReferences 对 inline code 的处理能力、添加跨语言一致性测试、以及把 tsx 接入测试运行器。生产路径 platform.ts:escapeHashReferences 仍是线性 O(N) 复杂度,每个字符最多被常数次正则扫描,分配模式合理(text.slice 在 V8 中通常为指针+长度,无全量复制;segments.join("") 是单次最终分配),没有泄漏或并发问题。run-github-opencode.py 改动是纯重构,零运行时差异。Python 测试中的文件 I/O + 正则匹配仅在 CI 跑一次,可忽略。

阻塞项:无

建议项:

  • tsx 作为 devDependency 会拉取 esbuild 全平台 native binary(~30MB),会显著增加 npm install 时间与磁盘占用(仅 dev 环境,不影响运行时)。若 CI 已用 tsup 编译产物跑测试,可考虑在 test 脚本中改为 node --test dist/platform.test.js,避免引入 tsx;若坚持 tsx,可在 .npmrcoptional=false 并配合 npm ci --omit=optional 控制 CI 安装体积。
  • multi-review/src/platform.ts:escapeHashReferences 每次调用会对同一段文本做 2–3 次正则扫描(外层 fenced、内层 inline、最后 HASH_NUM_RE.replace)。对典型 PR 评论(几 KB 至几十 KB)完全无感;若未来要处理超长评论(例如一次性贴多文件 diff),可考虑合并为单次状态机扫描,但目前无需优化。
architecture

架构有疑虑

整体变更遵循既有的"平台层为权威转义、prompt 层仅做提示"分层模式,并将该约定明确写入注释。escapeHashReferences 拆出 escapeSegment 子函数以处理 inline code,分层和命名都合理;Python 端把 hash_avoid_zh/en 提为单独变量也降低了行内重复。新增 platform.test.ts 直接测试真实 TS 实现(而非旧 test_all.py 中的 Python 镜像),是测试架构的正向改进。但跨语言的 prompt 文案仍分散在 5 处(reviewers.tsrun-github-opencode.pyplatform.ts、两个测试文件),并通过新增的 TestCrossLanguageHashInstructionConsistency 用正则解析源码字面量来保证一致,这种"靠测试解析源码字符串拼接结构"的耦合方式较脆弱,是本次主要的架构隐忧。

阻塞项:无

建议项:

  1. tests/test_all.py:1031-1035 的跨语言一致性校验把测试与源码的字符串拼接形态硬编码(ZH 必须是 2 段、EN 必须是 3 段 "..." + "...")。未来若有人把 EN 文案合并/拆分一段,正则就会抓不到常量,断言信息也会失真。建议改为:把这两段提示文案放到一个共享数据文件(例如 shared/prompts/hash-avoid.{zh,en}.txt 或 YAML),由 TS 和 Python 各自读取,从根源消除"双份字面量 + 正则比对"。
  2. 现在 multi-review/src/platform.test.tstests/test_all.py::TestEscapeHashReferences 测的是"同一份逻辑的两个语言实现",TS 测真实实现、Python 测镜像实现。建议明确两套测试的边界——例如把 Python 侧的镜像实现及其用例移除或缩减为冒烟级,避免任何 escape 行为变更都需要在 3 个地方(TS 实现、Python 镜像、两套用例)同步修改的霰弹式手术。
  3. tsx 仅为运行 node --import tsx --test 而引入。multi-review/ 已有 tsup(依赖 esbuild),可考虑直接用 tsupesbuild-register 跑测试以减少一条新的工具链分支;若坚持用 tsx,建议在 multi-review/READMEAGENTS.md 注明本目录的测试方式,避免与项目根 tests/test_all.py 的 Python 测试入口造成混淆。
  4. platform.ts:182 现已 export escapeHashReferences 仅为给测试使用,可在 JSDoc 中标注 @internal 或在导出名旁加注释说明这是测试入口,避免被外部模块当作公开 API 误用。

@Svtter Svtter merged commit 44f1655 into main Jun 3, 2026
3 checks passed
@Svtter Svtter deleted the fix/hash-ref-review-v3 branch June 3, 2026 12:42
Svtter added a commit that referenced this pull request Jun 3, 2026
- INLINE_CODE_RE: add newline boundary to prevent cross-line matching
- escapeHashReferences: add @internal tag (exported for testing only)
- package.json: add engines.node >= 20 (required by tsx/node --import)
- Python TestEscapeHashReferences: slim down to 4 smoke tests;
  comprehensive coverage lives in platform.test.ts (14 tests)
- Cross-language consistency tests: add CAVEAT comment warning that
  regex extraction is coupled to string literal form
@github-actions github-actions Bot mentioned this pull request Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant