Skip to content

feat: CI dist check, split blocked logs, edge-case tests#182

Merged
Svtter merged 1 commit into
mainfrom
fix/dist-check-and-polish
Jun 3, 2026
Merged

feat: CI dist check, split blocked logs, edge-case tests#182
Svtter merged 1 commit into
mainfrom
fix/dist-check-and-polish

Conversation

@Svtter

@Svtter Svtter commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses remaining review suggestions from PR #180.

Changes

  1. CI dist consistency check (.github/workflows/ci.yml)

    • New dist-check job: installs deps, runs npm run build, diffs dist/
    • Fails with ::error:: annotation if committed dist is stale
    • Runs on all PRs and pushes to main
  2. Split blocked key logs (Python + TS)

    • parseExtraEnv now separates prefix-blocked vs sensitive-blocked keys
    • Distinct summary lines: "blocked N reserved-prefix key(s)" and "blocked N sensitive key override(s)"
    • Makes ops debugging easier
  3. escapeHashReferences early-return (platform.ts)

    • Returns input unchanged if no #N pattern found
    • Avoids unnecessary segment/split work for clean text
  4. Edge-case tests (6 new)

    • #1abc (adjacent letter) — not escaped
    • <a href="#1"> (HTML attribute) — escaped
    • [text](#1) (markdown link) — escaped
    • Empty string — passthrough
    • No hash numbers — passthrough
    • Comma-separated ( #1, #2) — both escaped

…se tests

- Add dist-check CI job: builds multi-review and diffs dist/ against
  committed version, fails if out of date
- Split blocked key summary into prefix vs sensitive buckets in both
  Python (run-github-opencode.py) and TS (platform.ts)
- Add early-return for empty/no-match text in escapeHashReferences
- Add 6 edge-case tests: adjacent letter, HTML attr, markdown link,
  empty string, no hash numbers, comma-separated
- Update dedup test to match split-bucket output
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

已分析完所有 4 个变更点,核对了 PR 描述的要求与实现。让我做最后的对照:

1. CI dist consistency check

  • ci.ymldist-check 任务完整:npm installnpm run buildgit diff 检查
  • 失败时输出 ::error:: 注解并 exit 1
  • 触发条件:pull_requestpush: branches: [main]

2. Split blocked key logs (Python + TS)

  • platform.ts:618-652 parseExtraEnv 区分 prefixBlockedsensitiveBlocked 数组,分别输出 "blocked N reserved-prefix key(s)""blocked N sensitive key override(s)"
  • run-github-opencode.py:495-526 同样分离两个列表并输出对应的 summary
  • parseExtraEnvindex.ts:45 被调用,dist/index.cjs:5720 同步更新

3. escapeHashReferences early-return

  • platform.ts:185 if (!text || !HASH_NUM_RE.test(text)) return text; 提前返回
  • 同步在 dist/index.cjs 中体现

4. Edge-case tests (6 new)

  • test_no_match_adjacent_letter (#1abc)
  • test_no_match_html_attribute (<a href="#1">)
  • test_no_match_markdown_link ([text](#1))
  • test_empty_string
  • test_no_hash_numbers
  • test_comma_separated ((#1, #2))

无遗漏

所有 PR 描述中声明的 4 个变更项均已完整实现,无功能缺失、集成断点或配置未接管的项。parseExtraEnv 的拆分逻辑在 TS 与 Python 两端对称;escapeHashReferences 的早返回在源码与构建产物中均生效;CI 检查任务的 ::error:: 注解与触发条件符合描述;6 项边界测试均存在于 tests/test_all.pyTestEscapeHashReferencesSmoke 类中。

New%20session%20-%202026-06-03T13%3A21%3A30.737Z
opencode session  |  github run

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

最终决策:可合并

三位 reviewer(quality、security、performance)均判定可合并,无阻塞项;architecture 也无阻塞项,仅有结构性建议。所有变更均属内部重构、CI 强化与小修复,未引入安全或架构风险。

阻塞项

建议项

  1. 【已确认】测试命名与断言语义相反(quality、architecture 一致)

    • tests/test_all.py:1078tests/test_all.py:1082 中的 test_no_match_html_attribute / test_no_match_markdown_link 实际断言 assertIn("#\u200B1", result),验证的是匹配并转义发生,与同文件 test_no_match_adjacent_letter(真正测不匹配)易混淆。
    • 建议重命名为 test_match_html_attribute / test_match_markdown_link,或按 test_match_* / test_no_match_* 明确分组。
  2. 【已确认】parseExtraEnv Set → List 是对外可观察的行为变化(quality、performance、architecture 三方一致)

    • prefixBlocked.push(key) 保留重复出现次数,blockedKeys 返回值数量与原 Set 行为不同。已通过 tests/test_all.py:761-768 同步更新,属于有意为之。
    • 建议在 changelog / release notes 中简要提及;architecture 进一步建议在 ExtraEnvResult 中将 prefixBlockedsensitiveBlocked 作为独立字段暴露(保留 blockedKeys 做向后兼容),便于未来 UI 按类别展示,避免后续散弹手术。
  3. 【已确认】HASH_NUM_RE.lastIndex = 0 模式脆弱(quality、performance、architecture 三方一致)

    • multi-review/src/platform.ts:185-186 使用带 g 标志的模块级正则做 test(),依赖手动重置来保证后续 matchAll 正确性,是一种全局可变状态模式。性能上成本可忽略(一次赋值),安全/正确性上当前实现正确。
    • 建议:要么在函数内用局部非全局正则做预检,要么封装一个不共享 lastIndex 的检查函数,从接口层面消除隐患;若保留当前写法,至少加一行注释说明重置原因(防止其他调用方误用),或改用 text.match(HASH_NUM_RE) 之类的无状态调用。
  4. INLINE_CODE_RE 行为修复缺少回归测试(architecture)

    • multi-review/src/platform.ts:182`[^`]+` 收紧为 `[^`\n]+` 属正确性变更(行内码本不应跨行),但 escapeHashReferences 的现有测试未覆盖「反引号内含换行」这一被改变行为的场景。
    • 建议补一条回归测试锁定新行为,防止后续被无意回退。
  5. dist-check CI 任务开销(performance)

    • .github/workflows/ci.yml:12-35 新增的 dist-check job 每次 PR 增加约 1-2 分钟(npm install + npm run build + git diff),与 smoke-actions 并行运行不阻塞关键路径,属于合理代价。
    • 建议:如未来构建变慢,可考虑加路径过滤(仅当 multi-review/src/** 变更时触发)。

📋 各 Reviewer 详细审查结果
quality

可合并

本次 PR 引入了 dist-check CI 任务以防止 multi-review/dist/ 落后于源码,重构了 parseExtraEnv 的错误报告(区分保留前缀与敏感键两类阻断),修复了 INLINE_CODE_RE 跨行匹配的小问题,并为 escapeHashReferences 加了快速路径。整体改动小而清晰,源码与 dist 一致,测试覆盖同步更新。

阻塞项:无

建议项:

  • tests/test_all.py:1078tests/test_all.py:1082 中的 test_no_match_html_attributetest_no_match_markdown_link 命名与断言行为相反 —— 实际是断言这些模式被转义(assertIn("#\u200B1", result)),建议改名为 test_match_html_attributetest_match_markdown_link,避免后续维护者误解(与同文件中真正测"不匹配"的 test_no_match_adjacent_letter 区分开)。
  • parseExtraEnvSet 去重改为保留重复出现次数(prefixBlocked.push(key)),这是一个对外可观察的行为变化(错误信息和 blockedKeys 返回值的数量),但属于有意为之且测试已同步更新,建议在 changelog/release notes 中简要提及。
  • multi-review/src/platform.ts:185 的提前返回 HASH_NUM_RE.test(text)HASH_NUM_RE.lastIndex = 0 显式重置是良好的防御性写法,但严格来说 String.prototype.replace 不依赖 lastIndex,可加一行注释说明重置的原因(防其他调用方误用),或考虑改用 text.match(HASH_NUM_RE) 之类的无状态调用以避免心智负担。
security

安全无虞

本次 PR 主要是一次内部重构与小幅修复,未引入新的攻击面或权限变更。

安全分析摘要:

  • parseExtraEnv 的改动仅将"被阻止的 key"按原因(前缀保留 vs 敏感变量)拆分输出,未改变校验逻辑。MULTI_REVIEW_/GITHUB_RUN_OPENCODE_ 前缀拦截和 SENSITIVE_ENV_KEYS 黑名单依然生效。
  • 日志仅打印 key 名称而不打印 value,避免 secret 泄露。
  • escapeHashReferences 增加了 HASH_NUM_RE.lastIndex = 0 重置,正确处理了带 /g 标志的 stateful regex,避免跨调用状态污染;早返回路径也是纯性能优化。
  • INLINE_CODE_RE`[^`]+` 收紧为 `[^`\n]+`,避免跨行匹配引入误判,是修复而非回归。
  • CI 新增的 dist-check job 沿用 workflow 顶层 permissions: contents: read,且不接触 token / 网络外部输入。
  • actions/checkout@v6actions/setup-node@v4 已是仓库既有使用方式(见 smoke-actions),未引入新的依赖面。

阻塞项:无

建议项:无

performance

性能良好

本 PR 主要包含三类变更:CI 工作流新增 dist-check 任务、parseExtraEnv 从 Set 改为分类 List 记录被阻止的 key,以及 escapeHashReferences 提前返回优化。整体对性能无负面影响。

阻塞项:无

建议项:

  1. dist-check 工作流开销.github/workflows/ci.yml:12-35):新增的 dist-check job 每次 PR 都会执行 npm install + npm run build + git diff,增加约 1-2 分钟 CI 时长。该 job 与现有 smoke-actions 并行运行,不阻塞关键路径,属于合理代价。如未来构建变慢,可考虑加路径过滤(仅当 multi-review/src/** 变更时触发)。
  2. HASH_NUM_RE.test 后的 lastIndex = 0 复位multi-review/src/platform.ts:185-186):在慢路径(test 返回 true)中也无条件复位 lastIndex。该复位仅对 test 返回 false 后被外部再次使用的场景必要,主流程内 matchAll(FENCED_CODE_RE) 并不使用 HASH_NUM_RE,但成本可忽略(一次赋值),保留作防御性写法可接受。
  3. SetList 转换multi-review/src/platform.ts:618-619github-run-opencode.py:495-496):用 List 替代 Set 后,重复 key 会被多次列入阻塞报告。对小规模 env 输入无性能影响,但若用户传入大量重复 key,join(", ") 输出会变长。语义变化已通过测试更新(tests/test_all.py:761-768)明确。
  4. INLINE_CODE_RE 正则调整multi-review/src/platform.ts:182):新增 \n 排除项为正确性变更,匹配复杂度不变,不影响性能。
architecture

架构有疑虑

架构分析摘要:

  • PR 包含三处结构性改动:CI 增加 dist-check 任务(强制 dist/ 与源码构建产物一致,符合 TS-based GitHub Action 的标准模式);parseExtraEnv 将被拦截的键拆分为「保留前缀」与「敏感键」两类以提升错误信息可读性;escapeHashReferences 增加早返回优化并修复 INLINE_CODE_RE 不应跨行匹配的 bug。
  • 整体改动遵循既有架构:Python 包装脚本与 multi-review TS action 保持功能对齐(已由 TestCrossLanguageHashInstructionConsistency 系列测试守护),dist/ 作为已提交产物被新 CI 任务校验,符合本仓库「源码与 dist 同源」的约定。
  • 模块位置、依赖方向、层间边界未发生变化,分散修改局限在三处相关文件(python、ts、dist 由 build 生成),无散弹手术风险。

阻塞项:无

建议项:

  1. multi-review/src/platform.ts:185 使用带 g 标志的模块级 HASH_NUM_RE 进行 test(),依赖手动重置 lastIndex 来保证后续 matchAll 正确性。这是一种脆弱的全局可变状态模式,若函数内调用顺序调整或未来新增调用点,容易引入难以复现的 bug。建议在函数内部用局部非全局正则做预检,或在模块加载时封装一个不共享 lastIndex 的检查函数,从接口层面消除该隐患。
  2. tests/test_all.py:1074-1090 新增的五个测试中有三个命名为 test_no_match_html_attributetest_no_match_markdown_linktest_no_match_adjacent_letter,但实际断言与名称相反——前两者 assertIn("#\u200B1", result) 验证的是匹配并转义发生,仅 test_no_match_adjacent_letter 才是真正验证不匹配。命名与语义不符会误导后续维护者,建议重命名为 test_match_html_attribute / test_match_markdown_link,或将它们归入 test_match_*test_no_match_* 两个明确分组。
  3. parseExtraEnv 的返回值 blockedKeys 仍是混合两类来源的扁平列表,调用方无法区分前缀违规与敏感键违规。若未来有 UI 或外部消费者需要按类别展示,建议在 ExtraEnvResult 中将 prefixBlockedsensitiveBlocked 作为独立字段暴露(保留 blockedKeys 做向后兼容),避免后续再次改动时产生散弹手术。
  4. INLINE_CODE_RE/[^`]+`/g` 改为 `/`[^`\n]+/g 是一个行为修复(markdown 行内码本就不应跨行),但 escapeHashReferences 的现有测试未覆盖「反引号内含换行」这一被改变行为的场景,建议补一条回归测试以锁定新行为,防止后续被无意回退。

@Svtter Svtter merged commit 782a79d into main Jun 3, 2026
4 checks passed
@Svtter Svtter deleted the fix/dist-check-and-polish branch June 3, 2026 13:40
@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