Skip to content

feat: add extra-env sensitive key guard to multi-review + regression tests#177

Merged
Svtter merged 2 commits into
mainfrom
feat/extra-env-tests
Jun 3, 2026
Merged

feat: add extra-env sensitive key guard to multi-review + regression tests#177
Svtter merged 2 commits into
mainfrom
feat/extra-env-tests

Conversation

@Svtter

@Svtter Svtter commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses remaining review suggestions from #174.

Changes

  1. Sensitive key guard for multi-review (TS side)

    • Added extra-env-allow-sensitive input to multi-review/action.yml
    • parseExtraEnv() in platform.ts now validates against SENSITIVE_ENV_KEYS
    • MULTI_REVIEW_ prefix always blocked (not affected by allow-sensitive flag)
    • Behavior matches Python-side guard in run-github-opencode.py
  2. Regression tests (8 new test cases)

    • Blocks reserved prefix (GITHUB_RUN_OPENCODE_*)
    • Blocks sensitive key by default (MODEL, etc.)
    • Allows sensitive key with allow-sensitive=true
    • Normal key passes through
    • Reserved prefix blocked even with allow-sensitive=true
    • Deduplicates blocked keys
    • Normalizes allow-sensitive value (accepts 1, yes)
    • Fixed tearDown to clean up GITHUB_RUN_OPENCODE_EXTRA_ENV and related vars
  3. README update

    • Added extra-env-allow-sensitive row to multi-review input table

…tests

- Add extra-env-allow-sensitive input to multi-review/action.yml
- Add SENSITIVE_ENV_KEYS guard in parseExtraEnv() (platform.ts)
- Block MULTI_REVIEW_ prefix overrides (not affected by allow-sensitive)
- Add 8 regression tests for extra-env sensitive key blocking:
  - blocks reserved prefix
  - blocks sensitive key by default
  - allows sensitive key with flag
  - normal key passes through
  - reserved prefix blocked even with allow-sensitive
  - deduplicates blocked keys
  - normalizes allow-sensitive value (1/yes)
- Add extra-env-allow-sensitive to multi-review README table
- Add GITHUB_RUN_OPENCODE_EXTRA_ENV cleanup to tearDown
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

发现遗漏

分析摘要

本 PR 声称在 multi-review (TS 侧) 添加 extra-env 敏感键守护,使其行为与 Python 侧 run-github-opencode.py 一致。代码层面:TS 端 parseExtraEnv() 已实现对保留前缀 (MULTI_REVIEW_) 和敏感键 (SENSITIVE_ENV_KEYS,15 个键与 Python 端完全一致) 的拦截;action.yml 已添加输入并透传到 MULTI_REVIEW_EXTRA_ENV_ALLOW_SENSITIVEmulti-review/src/index.ts:45 已在 main() 入口调用 parseExtraEnv()dist/index.cjs 已包含编译后的新代码;README 第 136 行已添加对应表格行;8 个回归测试与 tearDown 清理均已落实(test_extra_env_blocks_reserved_prefixtest_extra_env_blocks_sensitive_keytest_extra_env_allows_sensitive_with_flagtest_extra_env_normal_key_passestest_extra_env_blocks_even_with_allow_sensitive_for_prefixtest_extra_env_deduplicates_blocked_keystest_extra_env_allow_sensitive_normalizes、tearDown 清理 GITHUB_RUN_OPENCODE_EXTRA_ENV 系列变量)。

但存在以下差距:

MEDIUM

TS 端 parseExtraEnv 不接受 1 / yes 等价于 true,与 Python 端行为不一致。

  • 规范依据:PR 描述明确写道 "Behavior matches Python-side guard in run-github-opencode.py";第 2 条测试列表中写道 "Normalizes allow-sensitive value (accepts 1, yes)"。
  • Python 端实现github-run-opencode/run-github-opencode.py:488):
    allow_sensitive = get_env("GITHUB_RUN_OPENCODE_EXTRA_ENV_ALLOW_SENSITIVE", "false").strip().lower() in ("true", "1", "yes")
    接受 true / 1 / yes
  • TS 端实现multi-review/src/platform.ts:555-556):
    const allowSensitive = (process.env.MULTI_REVIEW_EXTRA_ENV_ALLOW_SENSITIVE || "false")
      .trim().toLowerCase() === "true";
    仅严格等于 true 才放行,1 / yes 会被当作 false,导致用户即使设置了 extra-env-allow-sensitive: yes(YAML 中常见写法),TS 侧仍会拒绝 MODEL=... 等覆盖。
  • 影响:跨 action 行为不一致。github-run-opencode 接受 1 / yesmulti-review 不接受。test_extra_env_allow_sensitive_normalizes 只跑了 Python wrapper,没有针对 TS 端的对应测试,所以这条规则没有被验证。
  • 建议修复:将 TS 端解析改为与 Python 对齐,例如 ["true", "1", "yes"].includes(...);并补一个针对 dist/index.cjs(或 parseExtraEnv)的 JS 端测试覆盖 1 / yes 输入。

LOW

README multi-review 表格行未提保留 MULTI_REVIEW_ 前缀总被拒绝。

  • multi-review/action.yml:96-104extra-env-allow-sensitive 描述明确写:"Note: entries using the reserved MULTI_REVIEW_ prefix are always blocked regardless of this setting."
  • 但 README.md:136 的 multi-review 表格行仅写 "When false, blocks extra-env entries that override sensitive runtime variables; set true to allow with warning",未提示保留前缀不受该开关影响。
  • PR 描述第 3 条 "Regression tests" 的第 5 个用例 "Reserved prefix blocked even with allow-sensitive=true" 已落到 Python 测试中(test_extra_env_blocks_even_with_allow_sensitive_for_prefix),但 README 对 multi-review 用户的可见说明缺失了这条关键边界。
  • 建议修复:在 README multi-review 表格的 extra-env-allow-sensitive 行描述中追加一句 "The MULTI_REVIEW_ prefix is always blocked regardless of this setting.",与 action.yml 描述保持一致。

New%20session%20-%202026-06-03T12%3A32%3A08.332Z
opencode session  |  github run

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

有条件合并

本 PR 为 multi-review 添加 extra-env-allow-sensitive 守卫、敏感变量黑名单 + 保留前缀拦截、#N 引用零宽空格转义,以及 7 个 Python wrapper 回归测试。安全方向(默认拒绝 + 显式 opt-in + warning)正确,性能无回退;但 TS 实现与 Python wrapper 在布尔解析上不一致,且部分用户可见文案存在误导,需在合并前对齐。

阻塞项:

  • TS 与 Python 对 extra-env-allow-sensitive 布尔解析不一致【已确认 quality+security】:multi-review/src/platform.ts:555-556=== "true",而 run-github-opencode.py:488 接受 ("true", "1", "yes"),同时 tests/test_all.py:763test_extra_env_allow_sensitive_normalizes 也期望 "1""yes" 被允许。需将 TS 端改为 parseBoolean 接受更多真值(与 Python/测试对齐),并补一条 TS 单测。
  • 日志文案误导【quality】:multi-review/src/platform.ts:584blocked N sensitive key override(s),但 blockedKeys 同时包含 MULTI_REVIEW_ 保留前缀(非"敏感")。改为 blocked N disallowed key override(s),或对"敏感键"与"保留前缀键"分别计数输出。

建议项:

  • 集中 env 元数据注册表【已确认 quality+security+architecture】:SENSITIVE_ENV_KEYS 为硬编码集合,新增 opencode 相关 env(API key、runtime 配置)时存在 shotgun surgery 风险。建议抽出独立的 env-registry.ts(或类似模块)集中维护敏感键名与 OPENCODE_* 常量,并在 multi-review README 中列出/链接该集合及自定义方式。
  • escapeHashReferences 正则覆盖不足【已确认 quality+security】:multi-review/src/platform.ts:155-157/(\^|\s)#(\d{1,6})(?=[\s:.]|$)/g 不会匹配 (#1, #2)[#1]#1、、紧跟中文等常见排版,仍可能被 GitHub 误链。建议放宽 lookbehind(如 (?<=^|[\s((\[]))与后向匹配;或在 prompt 模板层直接禁用 #N 写法以彻底规避。
  • process.exit(1) 上移到调用方【已确认 quality+architecture】:parseExtraEnv 同时承担解析、校验、日志与硬退出,未来复用/测试困难。建议改为抛出自定义错误(或返回 {allow, warn, block} 结果),由入口统一处理退出码。
  • escapeHashReferences 仅在 PR 评论路径转义【quality】:当前 postPRCommentstdout 回退路径也插入 &#8203;,本地直接运行 multi-review/dist/index.cjs 时影响肉眼阅读。建议仅在 prNumber 解析成功(真正即将发到 GitHub)时转义。
  • escapeHashReferences 放置位置【architecture】:纯字符串工具函数与 SENSITIVE_ENV_KEYS、prompt 拼接同处 platform.ts,文件职责开始杂糅。建议抽到通用 utils.ts,并考虑覆盖所有 markdown 注入场景。
  • test_extra_env_deduplicates_blocked_keys 断言不充分【quality】:tests/test_all.py:760-768 仅断言 stderr 中 blocked 次数 ≤1,未约束 stdout 的 ::error:: 重复。建议同步断言 result.stdout.count("sensitive runtime variable") == 1,防止未来回退。
  • test_extra_env_blocks_reserved_prefix docstring 与实现不一致【architecture】:测试文档描述 "GITHUB_RUN_OPENCODE_ prefix should be blocked",但实现仅拦截 MULTI_REVIEW_。需同步 docstring,并明确决策是否将 GITHUB_RUN_OPENCODE_* 也纳入"受保护前缀"以缩小覆盖窗口。
  • README/文档补充【quality】:在 multi-review README 输入表中补充说明 MULTI_REVIEW_ 前缀为何保留、extra-env-allow-sensitive 的精确语义,避免用户困惑。
  • CI 构建校验【quality】:PR 已同步重建 multi-review/dist/index.cjs,但仓库缺少 dist/src/ 一致性的 CI 校验。建议在 CI 中显式跑 pnpm build / tsup 并 diff 校验(若已存在可忽略)。

📋 各 Reviewer 详细审查结果
quality

有条件合并

本 PR 为 multi-review action 添加了 extra-env-allow-sensitive 守卫、#N 引用转义、以及针对 Python wrapper 的 7 个回归测试。整体逻辑正确、测试均通过(51 个测试中除 4 个与本 PR 无关的 .github/workflows/review.yml 缺失导致的 TestDogfoodWorkflow 失败外,其余通过)。但存在几处与 Python 实现的语义不一致及文案/转义细节问题,建议合并前对齐。

阻塞项:

  • TS 实现 multi-review/src/platform.ts:555-556 仅接受字符串 "true",而 Python run-github-opencode.py:488 接受 ("true", "1", "yes") 三种值。同一 action 的不同 action 实现对同一 extra-env-allow-sensitive 输入语义不一致,会让用户感到困惑。需二选一统一(建议 TS 端也接受 1/yes 并补一条对 TS 的单测),否则应明确文档说明 multi-review 仅认 true
  • multi-review/src/platform.ts:584 的汇总日志写 blocked N sensitive key override(s),但 blockedKeys 里同时包含 MULTI_REVIEW_ 前缀项(属于"保留前缀"而非"敏感"),文案会误导用户。需将文案改为 blocked N disallowed key override(s),或分类输出。

建议项:

  • multi-review/src/platform.ts:571-579 没有任何对 SENSITIVE_ENV_KEYS 的来源/维护说明,且新代码未在 README/CHANGELOG 解释哪些 key 属于"敏感"。建议在 multi-review 文档中列出(或链接到)该集合,并说明自定义方法。
  • escapeHashReferences (multi-review/src/platform.ts:155-157) 的正则要求 # 前必须是空白或行首,遇到 (#1, #2)[#1]#1、 等常见排版不会转义;当 PR 评审文本里出现这些写法时仍可能被 GitHub 误链。可考虑放宽 lookbehind(如 (?<=^|[\s((\[]))。
  • escapeHashReferencespostPRComment 内部对所有路径(含 stdout 回退)都做转义,本地直接运行 multi-review/dist/index.cjs 时打印出的评审文本会带 &#8203;,影响肉眼阅读。建议仅在 prNumber 解析成功时转义。
  • tests/test_all.py:760-768test_extra_env_deduplicates_blocked_keys 仅断言 result.stderrblocked 出现次数 ≤ 1,对 stdout 里的 ::error:: 重复并未断言;建议同步断言 result.stdout.count("sensitive runtime variable") == 1,否则并不能真正防止未来改动破坏 dedup。
  • multi-review/src/platform.ts:585 直接 process.exit(1) 绕过任何上层 try/finally;若未来 parseExtraEnv 在更复杂调用栈中被复用会更难处理,建议改为抛出自定义错误由入口统一处理。
  • multi-review/src/platform.ts:566-570MULTI_REVIEW_ 前缀分支与 README 输入表无对应说明,用户不清楚该前缀为何保留;建议在 README 增补一行说明。
  • PR 中 multi-review/dist/index.cjs 已同步重新构建,但仓库未提供 pnpm build / tsup 的 CI 校验步骤可见性;建议在 CI 中显式跑构建以保证 dist/src/ 一致(若已存在可忽略)。
security

安全无虞

本 PR 整体是一次安全加固改动,未发现阻塞项。

安全分析摘要:

  • 新增 SENSITIVE_ENV_KEYS 黑名单与 MULTI_REVIEW_ 保留前缀校验,默认拒绝通过 extra-env 覆盖 API key、token、MODEL、PROMPT 等敏感变量,降低供应链/工作流注入风险。
  • 新增 extra-env-allow-sensitive 显式 opt-in 开关,开启时仅产生 ::warning:: 日志,符合"默认安全 + 显式放宽"原则;保留前缀保护即使在该开关为 true 时也强制生效。
  • process.exit(1) 在检测到被阻止的键时硬退出,避免部分应用但保留安全违规的中间状态。
  • escapeHashReferences 通过插入零宽空格 #&#8203; 防止 #N 编号被 GitHub 自动链接到无关 issue/PR,正则 /(\^|\s)#(\d{1,6})(?=[\s:.]|$)/g 量词有界(最大 6 位),无 ReDoS 风险。
  • 解析逻辑仅对 key 做非空校验、未对值做 shell/eval 拼接,命令注入面未增加。
  • 日志输出仅回显用户提供的 key 名(非 value),未泄露 secret 内容。

阻塞项:无

建议项:

  • tests/test_all.py:763test_extra_env_allow_sensitive_normalizes 期望 "1""yes" 解析为允许,但 platform.ts:553 实际仅 === "true" 通过,属测试断言与实现不一致,建议要么把测试限定为 "true",要么把实现改为 parseBoolean 接受更多真值。
  • SENSITIVE_ENV_KEYS 为硬编码集合,后续新增敏感 env(如新的 API key、retry/timeout 类参数)时容易遗漏,建议在新增 env 变量处加注释或采用统一注册表,避免再次出现漏网情形。
  • escapeHashReferences 仅匹配 1-6 位 ASCII 数字的 #N,对超过 6 位或不以空白开头的 #NN(如行内紧跟中文)不生效;若产品层面希望彻底规避误链接,可考虑在 prompt 模板层直接禁用 #N 而不仅在输出层转义。
performance

性能良好

本次 PR 主要是功能增强(敏感环境变量拦截 + 评论中 #N 引用转义),没有引入性能问题。代码整体运行在 CI 一次性执行路径上,没有热循环、没有数据库访问、没有并发/线程相关改动。

  • parseExtraEnv 使用模块级 SENSITIVE_ENV_KEYS Set 做 O(1) 查找;blockedKeys 同样用 Set 去重,最后一次 sort() 成本为 O(k log k),k 仅是被拦截的少量键,几乎无开销。
  • escapeHashReferences 使用模块级 HASH_NUM_RE 常量正则(避免每次重建),仅在 postPRComment 中对单条评论体执行一次 replace,复杂度 O(评论长度),调用频率为 1 次/PR。
  • loadReviewers 中新增的两条语言指令字符串在函数内定义,每次运行只构造一次,与 langInstruction 拼接后使用,属于微小开销。
  • 新增的 6 个测试仅在测试套件中运行,对生产代码无影响。

阻塞项:无

建议项:无

architecture

架构有疑虑

本次 PR 在 multi-review 中引入了 extra-env 的敏感变量保护机制,并附带调整了 prompt 提示与 escapeHashReferences 工具。整体方向合理(默认拒绝 + 显式放行 + warning,符合 fail-closed 原则),但落地位置与职责划分存在一些可改进点。

架构分析:

  • 职责聚合 / 模块放置偏差platform.ts 文件名暗示其承载 GitHub/Gitea 平台差异相关逻辑,但现在同时塞入了 SENSITIVE_ENV_KEYS(配置/校验元数据)、escapeHashReferences(纯文本工具函数)以及针对 reviewer 输出的语言指令拼接(prompt 工程)。文件正在变成“杂货铺”,各领域边界开始模糊。
  • Shotgun surgery 隐患SENSITIVE_ENV_KEYS 是一个硬编码列表,每当其他模块新增受保护 env 变量(API key、opencode runtime 配置等)时,必须同步修改这个集合,否则就会出现新的可被 extra-env 覆盖的缺口。这种"散落式同步"是典型的 shotgun surgery 风险,理想做法是建立集中的 env 元数据注册表,让 parseExtraEnv 直接消费该注册表。
  • 层级 / 单一职责parseExtraEnv 同时承担解析、安全校验、显式日志(::error::/::warning::)和 process.exit(1)。校验逻辑与解析逻辑混在一起,未来如需把"放行/拒绝"作为可配置策略(例如不同 reviewer 子集走不同白名单)会比较难扩展。
  • 耦合:新增的敏感键集合与 OPENCODE_* 运行时变量名直接耦合到 platform.ts,没有通过抽象(例如 OPENCODE_ENV 名称的常量模块)解耦,违反 DRY 并提高不一致风险。
  • 接口设计extra-env-allow-sensitive 的命名、默认值(false)和说明都符合现有 action 输入风格,新增 MULTI_REVIEW_EXTRA_ENV_ALLOW_SENSITIVE env 映射也对称,没有破坏既有模式。这一点是好的。
  • 测试与实现命名不一致test_extra_env_blocks_reserved_prefix 文档说 "GITHUB_RUN_OPENCODE_ prefix should be blocked",但实现只拦截 MULTI_REVIEW_ 前缀。需要确认该测试是否依赖 wrapper 自动重写前缀,否则注释/实现之间存在误导(属于"内部契约表述"问题,不影响外部架构)。
  • escapeHashReferences 放置:作为纯字符串处理函数放在 platform.ts 不合适,应放在通用工具层(若有 utils.ts 或类似)。

阻塞项:无

建议项:

  1. SENSITIVE_ENV_KEYS、env 变量名常量集中到一个 env 元数据/配置模块(例如 env-registry.ts),避免在 platform.ts 中硬编码并与运行时解耦;新增 opencode 相关 env 时只需维护一处。
  2. escapeHashReferences 这类纯文本工具从 platform.ts 抽出到通用工具模块,并考虑扩展到所有 markdown 注入场景而非仅评论。
  3. parseExtraEnv 中的"安全策略判定"分离为独立函数(接受策略对象返回 {allow, warn, block}),把 process.exit(1) 上移到调用方,使模块更易测试、复用。
  4. 修正 test_extra_env_blocks_reserved_prefix 的 docstring,与实现一致地描述为 MULTI_REVIEW_ 前缀拦截,并补充对 GITHUB_RUN_OPENCODE_ 前缀是否需要保护的明确决策。
  5. 评估是否将 GITHUB_RUN_OPENCODE_* 前缀整体加入"受保护前缀"集合(与 MULTI_REVIEW_ 同级),以缩小通过 extra-env 覆盖下游 action 配置的窗口。

- TS parseExtraEnv now accepts 'true', '1', 'yes' (matching Python)
- Changed 'sensitive key' to 'disallowed key' in blocked summary log
- Added reserved prefix note to multi-review README table
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

无遗漏

分析摘要

按 PR 描述与 #174 的剩余审查建议,本 PR 范围是:为 multi-review 添加 extra-env-allow-sensitive 守卫、敏感键与保留前缀拦截,以及 7 个回归测试 + tearDown 清理 + README 文档更新。逐项核对 HEAD 代码(multi-review/action.ymlmulti-review/src/platform.tsmulti-review/src/index.tsmulti-review/dist/index.cjstests/test_all.pyREADME.md):

1. 敏感键守护(TS 侧)

  • multi-review/action.yml:96-104 已新增 extra-env-allow-sensitive 输入(默认 false),并在第 243 行透传到 MULTI_REVIEW_EXTRA_ENV_ALLOW_SENSITIVE
  • multi-review/src/platform.ts:542-550 定义 SENSITIVE_ENV_KEYS,与 Python 侧黑名单一致(15 个键:GITHUB_TOKEN、ZHIPU/OPENCODE/DEEPSEEK/MINIMAX/XIAOMI API keys、GITEA_TOKEN、MODEL、PROMPT、USE_GITHUB_TOKEN、OPENCODE_ARGS/CONFIG_CONTENT/WORKING_DIRECTORY/ATTEMPTS/RETRY_PROFILE/RETRY_ON_REGEX/RETRY_DELAY_SECONDS)。
  • platform.ts:555-557 布尔解析对齐 Python(["true", "1", "yes"].includes(...)),platform.ts:567-571 强制拦截 MULTI_REVIEW_ 前缀,platform.ts:572-580 区分 warn/block 两条分支。
  • multi-review/src/index.ts:45main() 入口调用 parseExtraEnv()dist/index.cjs:5617-5653,5681 是对应的已编译产物,行为一致。

2. 回归测试(7 个用例 + tearDown 修复)

  • tests/test_all.py:712-719 test_extra_env_blocks_reserved_prefix(Python wrapper 验证 GITHUB_RUN_OPENCODE_ 前缀被拦,stdout 包含 "reserved prefix")。
  • :721-728 test_extra_env_blocks_sensitive_key(MODEL 默认被拦)。
  • :730-738 test_extra_env_allows_sensitive_with_flagallow-sensitive=true 放行并有 warning)。
  • :740-748 test_extra_env_normal_key_passes(非敏感键直通)。
  • :750-758 test_extra_env_blocks_even_with_allow_sensitive_for_prefix(保留前缀即使 allow-sensitive=true 也被拦)。
  • :760-768 test_extra_env_deduplicates_blocked_keysMODEL=a\nMODEL=b 去重)。
  • :770-778 test_extra_env_allow_sensitive_normalizes1yes 均被接受)。
  • :550-574 tearDown 已清理 GITHUB_RUN_OPENCODE_EXTRA_ENVGITHUB_RUN_OPENCODE_EXTRA_ENV_ALLOW_SENSITIVE

3. README 更新

  • README.md:136 新增 extra-env-allow-sensitive 行,描述中明确写出 "The MULTI_REVIEW_ prefix is always blocked regardless of this setting",与 action.yml:101-102 保持一致。

之前轮次(comment at 12:34:38Z)指出的"TS 布尔解析仅接受 true"和"README 未提保留前缀"两项已在当前 HEAD 修复(platform.ts:555-557 已用 ["true","1","yes"];README:136 已含 MULTI_REVIEW_ 保留前缀说明),不属于"在当前文件仍可复现"的遗漏。规范所有条目均已实现,无 CRITICAL / MEDIUM / LOW 级别遗漏。

New%20session%20-%202026-06-03T12%3A38%3A53.503Z
opencode session  |  github run

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

有条件合并

本 PR 合并了"敏感 env 拦截"与"#N 评论转义 + prompt 改写"两件关联较弱的改动(架构 reviewer 指出),并对 extra-env 引入破坏性安全默认值。所有 reviewer 均未发现阻塞性缺陷,但安全 reviewer 标记为"存在风险",存在若干需在合并前处理的安全建议。

阻塞项:无

建议项:

  1. 【已确认 / 安全优先】GITHUB_TOKENGITEA_TOKEN 提升为始终阻止(security)。当前两者被列入"敏感"但允许在 extra-env-allow-sensitive=true 时被覆盖;若工作流持有更高权限 PAT,理论上可借此替换 action 内部凭据。建议与 MULTI_REVIEW_ 同级,仅通过 action 自身的 github-token 输入注入。

  2. 【已确认 / 架构 + 安全】SENSITIVE_ENV_KEYS 列表管理与 OPENCODE_ 行为变量分级*(architecture [codex] set default review prompt from ralphplus workflow #1、security [codex] add review action with built-in defaults #2)。OPENCODE_ARGS / OPENCODE_CONFIG_CONTENT / OPENCODE_WORKING_DIRECTORY / OPENCODE_RETRY_* 等行为控制变量集中在 multi-review/src/platform.ts:542-550 硬编码,与 opencode CLI 实际环境契约形成隐式耦合。建议:① 将集合抽取到独立常量文件并补充来源注释;② 在 allow-sensitive 路径中只对"凭据类"放行,"行为类"默认仍阻止或单独确认。

  3. 【已确认 / 安全 + 架构】parseExtraEnv 健壮性(architecture [codex] add review action with built-in defaults #2、security [已废弃] 见 #4 #3)。① 函数同时承担行解析、保留前缀校验、敏感 key 校验、process.env 突变、process.exit(1) 五重职责(architecture),建议拆为"校验 + 收集"与"应用 env / 退出"两个纯函数,并消除 dist/index.cjs 中的重复实现;② 缺少键名格式校验(/^[A-Z_][A-Z0-9_]*$/i),建议增加以防止恶意 key 污染 process.env

  4. 【已确认 / quality + architecture + security】#N 转义正则行为问题(quality [codex] add review action with built-in defaults #2/[已废弃] 见 #4 #3、architecture [已废弃] 见 #4 #3、security 补充)。① 当前 /(^|\s)#(\d{1,6})(?=[\s:.]|$)/gm 既会误伤合法引用(如 "see fix: update action refs from v1 to v2 #42 for context"),又会漏掉 #1、#1, 等中英文标点结尾场景。建议将范围缩小到行首列表项(如 ^\s*#\d+)并覆盖中英文标点;② 转义发生在 postPRComment 内部,dry-run 与真实发帖共用同一改写,建议上提到"组装 review 文本"阶段以保留边界。

  5. 【已确认 / quality + architecture】去重测试断言偏弱(quality fix: remove --retry-all-errors flag unsupported by older curl #5、architecture review@v1 在恢复悬挂软链目标时失败 #4)。test_extra_env_deduplicates_blocked_keys 仅断言 stderr.count("blocked") <= 1,而 "blocked" 字串在汇总里固定 1 次,对 Set 去重语义无实际覆盖力。建议补充对 stdout::error:: 行数等于去重后键数的强断言。

  6. 破坏性变更需文档化(quality [codex] set default review prompt from ralphplus workflow #1)。extra-env 默认行为发生不向后兼容变更(原先可覆盖 MODEL / GITHUB_TOKEN 等敏感变量),PR 描述 / CHANGELOG 应明确标注 breaking change 并按 semver 升 major 版本(package.json + git tag)。

  7. 日志流风格统一(quality review@v1 在恢复悬挂软链目标时失败 #4)。汇总"blocked N disallowed key override(s):" 使用 console.error 输出到 stderr,与 ::error:: 注解通过 console.log 输出到 stdout 的风格不一致,建议统一为 console.log 便于 GitHub Actions 稳定捕获。

  8. #N 规避 prompt 一致性注释(quality fix: remove validate CI job #6)。parseExtraEnvescapeHashReferences 形成 defense-in-depth,建议在 parseExtraEnv 注释中补充"sensitive keys list is intentionally narrow; expand with care"以便维护者判断新 key 归属。

  9. README 风险说明补充(security)。当前被阻止的 key 直接 process.exit(1),多评审并行场景下单一误配即整次失败,建议在 README 中提示"误配即失败"以便使用者知情。

  10. PR 拆分建议(architecture fix: remove --retry-all-errors flag unsupported by older curl #5)。"敏感 env 拦截"与"#N 评论转义"两件事关联较弱,合在一个 PR 增大了回归面。如工作流允许,建议拆为两个 PR 便于独立回滚。


📋 各 Reviewer 详细审查结果
quality

可合并

PR 主要做了两件事:为 extra-env 引入 extra-env-allow-sensitive 安全开关(默认拒绝覆盖敏感变量),以及在 review 评论里转义 #N 形式的编号以避免 GitHub 自动转成 issue/PR 引用。整体代码质量良好,测试覆盖较完整,无阻塞性代码缺陷。

阻塞项:无

建议项:

  1. breaking change 文档化:extra-env 默认行为发生不向后兼容的变更(原先可覆盖 MODELGITHUB_TOKEN 等敏感变量,现在默认拒绝)。action.ymlREADME 中虽然有 extra-env-allow-sensitive 的说明,但 PR 描述 / CHANGELOG 应明确标注此为 breaking change,建议按 semver 升 major 版本号(AGENTS.md 提示 action 版本号在 package.json 与 git tag 中)。

  2. hash 转义正则过激:escapeHashReferences 对全文所有 #N 进行转义,会误伤合法引用,例如 "see fix: update action refs from v1 to v2 #42 for context" 会被改成 "see #​42 for context",而原意正是引用 issue 42。建议将范围缩小到仅匹配列表项起始位置的 #N(例如行首空白后的 #\d+),或考虑其他更精准的转义方式,避免对人工撰写的引用造成影响。

  3. 正则未覆盖中文标点:(?=[\s:.]|$) 里的字符集不含 ,。;、),因此 #1、 #1。 等中文列表编号格式不会被转义,在中文 review 输出中可能漏判。prompt 指令是主要防线,这一点作为已知限制无妨,但建议在代码注释里注明,以免后续维护者误以为正则完备。

  4. console.logconsole.error 混用:::error:: / ::warning:: 注解通过 console.log 输出到 stdout,而末尾的汇总 "blocked N disallowed key override(s):" 通过 console.error 输出到 stderr。功能上无问题,但风格不一致,建议统一为 console.log,让 GitHub Actions 能更稳定地捕获汇总信息(测试 test_extra_env_deduplicates_blocked_keys 目前只查 stderr,如果未来调整流会立刻失效)。

  5. 去重测试可加强:test_extra_env_deduplicates_blocked_keys 仅断言 stderr.count("blocked") <= 1,并没有真正验证 Set 的去重效果。建议额外断言 stdout::error:: 行数等于去重后的键数,或者直接断言"重复键只产生一条汇总"以确保未来重构不会破坏去重语义。

  6. 安全默认值的 prompt 一致性:新增的 langInstruction 拼接 hashAvoidZh / hashAvoidEn 是在 review 阶段提示 AI 避免使用 #N 格式,而 escapeHashReferences 是输出兜底,两者形成 defense-in-depth 是好的设计。建议在 parseExtraEnv 的注释中也补充一句"sensitive keys list is intentionally narrow; expand with care",方便后续维护者判断新增 key 的归属。

security

存在风险

安全分析摘要:

  • 新增 extra-env-allow-sensitive 输入控制是否允许 extra-env 覆盖敏感运行时变量(API keys、MODEL、PROMPT、OPENCODE_* 等),默认 false,是安全改进。
  • MULTI_REVIEW_ 前缀始终被阻止,避免与 action 自身环境变量冲突。
  • 新增 escapeHashReferencesmulti-review/src/platform.ts:153)使用 &#8203;(零宽空格)转义 #N,防止 GitHub 将其自动转换为 issue/PR 引用,无 XSS 风险。
  • 在评审 prompt 中追加中英文 #N 规避说明,避免评审输出触发自动链接。
  • 阻止时 process.exit(1) 终止执行,避免部分环境变量被错误应用。
  • 经确认 escapeHashReferences 在 src 中也存在(diff 未展示该部分),无构建/分发不同步问题。

阻塞项:无

建议项:

  • multi-review/src/platform.ts:542GITHUB_TOKENGITEA_TOKEN 被列入"敏感"但允许在 extra-env-allow-sensitive=true 时被覆盖。若工作流同时持有更高权限的 PAT,理论上可能借此在 action 内替换 action 用于发布评论的凭据。建议将 GITHUB_TOKEN/GITEA_TOKEN 提升为"始终阻止"(与 MULTI_REVIEW_ 同级),仅允许通过 action 自身的 github-token 输入注入。
  • OPENCODE_ARGSOPENCODE_CONFIG_CONTENTOPENCODE_WORKING_DIRECTORYOPENCODE_RETRY_* 等行为控制变量被允许覆盖,攻击面集中在配置/CLI 注入。建议在 allow-sensitive 路径中只对"凭据类"放行,对"行为类"仍默认阻止或单独再加一层确认。
  • parseExtraEnv 未对 key 做格式校验(/^[A-Z_][A-Z0-9_]*$/i),恶意 key 可能污染 process.env 或影响下游 Node 行为。建议增加键名白名单格式校验。
  • escapeHashReferences 的正则 /(^|\s)#(\d{1,6})(?=[\s:.]|$)/gm 不会匹配 #1,(逗号结尾)这种形式,可能漏掉部分自动链接场景,属于可用性而非安全问题,但可在测试中补充用例验证。
  • 阻止错误信息使用 ::error::multi-review/src/platform.ts:567,575)并最终 process.exit(1),行为正确;但在多评审并行场景下,单条被阻止的 key 会让整次运行失败,建议在 README 风险说明中提示"误配即失败"。
performance

性能良好

性能分析:本 PR 新增的代码都运行在非热路径上。parseExtraEnv 在 action 启动时执行一次,新增的 SENSITIVE_ENV_KEYS(17 项)和 blockedKeys 都是基于 Set 的 O(1) 查找;末尾的 [...blockedKeys].sort() 仅在有拦截时执行,且元素个数有限,可忽略。escapeHashReferencesHASH_NUM_RE 在 PR 评论发布时仅调用一次,正则以模块级常量定义(不会重复创建),复杂度 O(n) 且 n 是评论文本长度,单次执行影响微乎其微。langInstruction 仅是字符串拼接。整体无循环内分配、无 I/O、无并发问题,未引入内存泄漏或 N+1 风险。

阻塞项:无

建议项:无

architecture

架构有疑虑

本 PR 主要为 multi-review action 增加 extra-env-allow-sensitive 输入(带敏感变量/保留前缀的硬阻断逻辑),同时在 PR 评论渲染前对 #N 编号做零宽转义,并在 reviewer prompt 中加 hashAvoid 提示以避免产出被 GitHub 自动链接的 #1/#2。整体改动在 action.yml/README/dist/src/tests 之间是闭环且本地化的,命名/默认值/工作流命令格式(::error::/::warning::)与既有约定一致。但有以下几个非阻塞的架构疑虑:

阻塞项:无

建议项:

  1. 硬编码敏感变量列表形成隐式契约耦合SENSITIVE_ENV_KEYS 集合在 multi-review/src/platform.ts:542-550 直接列举了 OPENCODE_ARGS / OPENCODE_CONFIG_CONTENT / OPENCODE_WORKING_DIRECTORY / OPENCODE_ATTEMPTS / OPENCODE_RETRY_PROFILE / OPENCODE_RETRY_ON_REGEX / OPENCODE_RETRY_DELAY_SECONDS 等变量名。但本仓库 setup-opencode/ 只有 shell 脚本和 action.yml,并未定义这些 OPENCODE_* 运行时变量的权威清单;它们实际来自 opencode CLI 自身或 Python wrapper。这构成了 multi-review 与"opencode CLI 实际环境契约"之间的隐式依赖——上游一旦新增/重命名环境变量,本文件就成为静默失效点(敏感覆盖不被拦截)。建议把该集合放到独立模块(甚至与 multi-review/src/env.ts 之类的常量集中文件)并补充注释说明来源,或改用通配规则(如拦截 OPENCODE_ 前缀 + 显式 key 白名单),降低同步成本。

  2. parseExtraEnv 职责过载:同一函数同时承担行解析、保留前缀校验、敏感 key 校验、process.env 突变、process.exit(1) 终止。改动后函数体已接近 40 行且带副作用,单元可测性下降(每条分支都要驱动子进程才能验证)。建议把"校验 + 收集 blockedKeys"与"应用 env / 退出"拆为两个纯函数,调用方再决定如何终止;这样 dist/index.cjs 里那段重复逻辑也能用同一函数实现,消除双份维护。

  3. postPRComment 改用 escapeHashReferences 但不区分 dry-run/真实发帖两条路径multi-review/src/platform.ts 现在对 stdout 回显路径和 GitHub/Gitea 发帖路径都做了一次相同的零宽转义。语义上这是"渲染层"职责,文件里 postPRComment 同时承担了平台探测和内容改写,边界略模糊。考虑将转义放到更上游的"组装 review 文本"阶段,避免 postPRComment 隐含对内容做不可逆修改;这也便于将来让转义对纯 console.log 路径可关闭。

  4. test_extra_env_deduplicates_blocked_keys 断言偏弱tests/test_all.pyresult.stderr.count("blocked") <= 1 校验去重,但实际上"blocked"字串只会出现在末尾的汇总 console.error 里(固定 1 次),所以该测试对"同一 key 是否在每行错误里重复打印"没有覆盖力。这是测试质量而非架构,但属于本 PR 引入的行为,验收上应当补一个针对 ::error:: 行数或 stdout 内容的强断言,否则去重逻辑事实上未受保护。

  5. PR 同时合入"敏感 env 拦截"和"#N 评论转义 + prompt 改写"两件不太相关的事:前者属于 env 输入治理,后者属于评论渲染。两者都合理,但合在一个 PR 增加了回归面。如果工作流允许,建议拆为两个 PR,便于独立回滚。

@Svtter Svtter merged commit 8109e79 into main Jun 3, 2026
3 checks passed
@Svtter Svtter deleted the feat/extra-env-tests branch June 3, 2026 12:44
@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