Skip to content

fix: address review feedback for hash-reference escaping#176

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

fix: address review feedback for hash-reference escaping#176
Svtter merged 1 commit into
mainfrom
fix/hash-reference-review-feedback

Conversation

@Svtter

@Svtter Svtter commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses review suggestions from PR #175 (multi-review quality, architecture, security reviewers).

Changes

Regex coverage improvement (platform.ts)

  • Widened escapeHashReferences regex to cover (#1), [#1], >#1, :#1, ,#1, 、#1, :#1 etc.
  • Added code block (triple backtick) exclusion — content inside ``` fences is not escaped
  • Fixed zero-width space insertion position: between # and digits (not after the whole token)

Code quality

  • Extracted buildLangInstruction() function in reviewers.ts for readability
  • Added architectural comment clarifying: platform layer is authoritative safeguard, prompt layer is merely a hint
  • Used neutral notation in source comments (avoids literal #N in code comments)

Cross-language consistency

  • Unified Chinese/English instruction text between reviewers.ts and run-github-opencode.py

Testing

  • Added 14 unit tests for escapeHashReferences covering:
    • Line start, after space, after (, [, >, :
    • After Chinese punctuation , ,
    • Code blocks are not escaped
    • Multiple code blocks handled correctly
    • Markdown headings (##) are not escaped
    • Regular # heading not escaped
    • Trailing punctuation #1, handled

Testing

  • 53 tests pass (39 existing + 14 new)
  • TypeScript build passes (npx tsc --noEmit)

- Widen escapeHashReferences regex to cover more contexts: parentheses,
  brackets, angle brackets, colons, and Chinese punctuation (:,、)
- Skip code blocks (triple backtick fences) during escaping
- Insert zero-width space between # and digits (not after) for correct
  GitHub reference breaking
- Extract buildLangInstruction() in reviewers.ts for readability
- Add architectural comment clarifying platform layer is authoritative
- Use neutral notation in source comments (#<num> -> hash-number)
- Unify Chinese/English instruction text between Python and TS
- Add 14 unit tests for escapeHashReferences covering line start, after
  punctuation, Chinese punctuation, code block exclusion, and headings

Addresses suggestions from PR #175 multi-review.
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

无遗漏

经过对比 PR body 中描述的改动(针对 PR #175 review feedback 的修复)和实际实现:

1. 正则覆盖增强(platform.ts:158

  • 扩大后的正则 /(?:^|(?<=[\s(\[{>:,、:]))(#)(\d{1,6})(?=[\s)\]},:.!?;,。!?、:]|$)/gm 完整覆盖了 (#1), [#1], >#1, :#1, ,#1, 、#1, :#1 等前后标点情况 ✓

2. 代码块排除(platform.ts:160-174

  • CODE_BLOCK_RE = /````[\s\S]*?````/g 配合 escapeHashReferences 函数对文本进行分段处理,确保三反引号代码块内的内容不被转义 ✓
  • 替换字符串 "$1\u200B$2" 确保零宽空格插入在 # 和数字之间 ✓

3. buildLangInstruction() 提取(reviewers.ts:44-58

  • 函数被正确提取,根据 language 返回中英文指令 ✓

4. 架构注释(platform.ts:152-156

  • 已添加说明 platform 层是权威防护、prompt 层只是提示 ✓

5. 中性命名

  • 测试函数名(test_after_open_parentest_after_chinese_colon 等)和代码注释("hash-number references")均使用中性命名 ✓

6. 跨语言一致性

  • run-github-opencode.pyzh_instruction / en_hash_instruction 文本与 reviewers.tsHASH_AVOID_ZH / HASH_AVOID_EN 实际指令内容一致 ✓

7. 14 个新增测试(tests/test_all.py:920-1001

  • TestEscapeHashReferences 包含全部 14 个测试用例,对应 PR body 列表的每一项 ✓

8. 集成完整性

  • escapeHashReferencespostPRCommentplatform.ts:179)中被调用,postPRCommentindex.ts:145 被实际使用,转义逻辑已正确接入主流程 ✓

PR body 描述的所有审查反馈点都已完整实现,未发现遗漏项。

New%20session%20-%202026-06-03T12%3A24%3A11.724Z
opencode session  |  github run

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

可合并

本 PR 核心变更为强化 escapeHashReferences#N 模式(含中英文标点后)和代码块跳过的处理,并补充测试覆盖。所有成功输出的 reviewer(quality、security、performance)均判定为可合并/安全无虞/性能良好,architecture reviewer 因网络原因未产出。跨 reviewer 去重后无重复问题,无任一问题被 ≥2 名 reviewer 提及,因此无可"已确认"条目;无阻塞项。

阻塞项:无

建议项:

  • 测试代码与被测实现存在双源真相风险:tests/test_all.py 新增的 TestEscapeHashReferences 用 Python 重新实现了一遍 escape_proper 逻辑,并非直接调用 platform.ts 中的函数。若 HASH_NUM_RE 或代码块切片规则未来在 TS 端被改动,Python 测试无法感知。建议在 TS 端为 escapeHashReferences 添加 vitest/node:test 单元测试直接断言被测函数;或将正则与切片逻辑抽到独立纯函数以便跨语言复用。(quality)
  • HASH_NUM_RE 启用了 g 标志并依赖 lastIndex 状态,当前用法(replace、重新构造正则)虽安全,但建议在文件顶部加注释说明该正则带 g 标志、不得跨调用共享 exec/matchAll 之外的状态,以免后续维护者误用。(quality)
  • CODE_BLOCK_RE 仅匹配三反引号围栏代码块,不处理单反引号内联代码(`#1`)及未闭合围栏。建议在 JSDoc 注释中明确写清当前覆盖范围,避免读者误以为已处理所有代码块。(quality)
  • escapeHashReferences 语义已从"转义 #N"扩展为"转义 + 跳过代码块",函数名略显单薄。考虑重命名为 escapeHashReferencesOutsideCode,或在 JSDoc 中补充"跳过 围栏代码块"的说明。(quality)
  • multi-review/src/reviewers.tsHASH_AVOID_ZH/HASH_AVOID_ENrun-github-opencode/run-github-opencode.py 中对应文案通过 Keep in sync with … 注释维护同步,建议在 CI 或单测中加入最小校验(检查两段字符串完全一致),防止一方改动遗漏另一方。(quality)
  • run-github-opencode/run-github-opencode.py 中仅将一行长字符串拆成两行拼接,行为完全等价,建议在 PR 描述里注明"纯排版改动、无功能差异",便于 reviewer 一眼判断。(quality)
  • multi-review/src/platform.ts:165-178 escapeHashReferences 对每个非代码块片段分别调用 replace,会创建多份中间字符串。对典型 PR 评论长度无影响;若输入极大(数十万字符 + 大量代码块)可改用单次 replace + 自定义回调,或 exec 循环配合 lastIndex 手工拼接以减少 slice 次数。(performance)
  • tests/test_all.py:920-928 import re as re_mod 位于 _run_escape 内部,每次测试都执行。Python 已缓存模块,仅一次 dict 查找,可忽略;如求洁癖可提升到模块级。(performance)

📋 各 Reviewer 详细审查结果
quality

可合并

本次 PR 主要目的是强化 escapeHashReferences#N 模式(包括中英文标点后)以及代码块的跳过处理,同时对提示词的拼装做了轻量重构,并补充了较完整的测试覆盖。整体逻辑正确,回归风险较低。

阻塞项:无

建议项:

  • tests/test_all.py 新增的 TestEscapeHashReferences 是在 Python 里重新实现了一遍 platform.ts 中的逻辑(escape_proper),并非直接调用被测代码。若 HASH_NUM_RE 或代码块切片逻辑未来在 TS 端被改动,Python 测试无法感知。建议在 multi-review/ 中加入 TS 单元测试(如 vitest/node:test),直接断言被测函数;或至少将正则与切片逻辑抽到独立纯函数以便跨语言复用。
  • HASH_NUM_RE 启用了 g 标志并依赖 lastIndex 状态。当前用法(replace、重新构造正则)虽不会踩坑,但建议在文件顶部加一行注释说明"该正则带 g 标志,不得跨调用共享 exec/matchAll 上下文之外的状态",以免后续维护者误用。
  • CODE_BLOCK_RE 只能匹配三反引号围栏代码块,不处理单反引号的内联代码(`#1`)以及未闭合的围栏。是否需要扩展取决于真实使用场景,建议在注释中明确写清当前覆盖范围,避免读者误以为已处理所有代码块。
  • escapeHashReferences 现在的语义已经从"转义 #N"扩展为"转义 + 跳过代码块",函数名略显单薄。考虑改为 escapeHashReferencesOutsideCode 或在 JSDoc 中补充"跳过 围栏代码块"的说明。
  • multi-review/src/reviewers.tsHASH_AVOID_ZH / HASH_AVOID_ENgithub-run-opencode/run-github-opencode.py 中对应文案通过注释 Keep in sync with … 维护同步。建议在 CI 或单测中加一个最小校验(如检查两段字符串完全一致),防止后续某一方被改而另一方遗漏。
  • run-github-opencode/run-github-opencode.py 中仅把一行长字符串拆成两行拼接,行为完全等价,可考虑在 PR 描述里注明"纯排版改动、无功能差异",便于 reviewer 一眼判断。
security

安全无虞

本次 PR 的核心变更集中在 escapeHashReferences 函数的加固与提示词重构,不涉及认证、密钥、网络、文件 I/O 等敏感路径。主要审查点:

  1. 正则表达式安全性:新 HASH_NUM_RE 使用 (?:^|(?<=...)) 锚定配合 (\d{1,6}) 有界量词和固定字符集的前后断言,无灾难性回溯风险。CODE_BLOCK_RE 使用 [\s\S]*? 懒惰量词且有明确字面边界 ```,同样安全。JavaScript 引擎的回溯不会因输入长度产生指数级时间复杂度。
  2. 转义逻辑正确性:将代码块内容原样保留(segments.push(m[0])),仅对代码块外的 #N 模式插入 \u200B(零宽空格),可有效阻断 GitHub/Gitea 的自动 issue/PR 引用识别,且不会破坏代码示例的展示。
  3. 输入处理边界matchAll 在空字符串上安全返回空迭代器;m.index 已做 undefined 防御检查;分段处理无越界。
  4. 无新增外部依赖、无 secrets/PII、无 SQL/命令注入面:纯文本字符替换,不调用 evalFunctionchild_process,不执行 shell 命令。
  5. 纵深防御设计:prompt 提示与平台层转义双保险(PR 注释中已明确说明),符合 OWASP A04(Insecure Design)最佳实践。
  6. 测试代码:测试通过 re-implement Python 版逻辑进行断言,未引入 pickleos.system 等危险调用,re_mod 仅做字面模式匹配。

阻塞项:无

建议项:无

performance

性能良好

PR 主要是文本处理逻辑的增强(扩展 #N 转义规则、跳过代码块)和提示词文案调整,未涉及热点路径或重计算。

阻塞项:无

建议项:

  • multi-review/src/platform.ts:165-178 escapeHashReferences 对每个非代码块片段分别调用 replace,会创建多份中间字符串。对典型的 PR 评论长度无影响,但若输入极大(数十万字符 + 大量代码块)可改用单次 replace + 自定义回调,或 exec 循环配合 lastIndex 手工拼接以减少 slice 次数。
  • tests/test_all.py:920-928 import re as re_mod 位于 _run_escape 内部,每次测试都执行。Python 已缓存模块,仅一次 dict 查找,可忽略;如需洁癖可提升到模块级。
architecture

(审查失败: fetch failed)

@Svtter Svtter merged commit ab9413f into main Jun 3, 2026
3 checks passed
@Svtter Svtter deleted the fix/hash-reference-review-feedback branch June 3, 2026 12:30
Svtter added a commit that referenced this pull request Jun 3, 2026
- 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 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