fix(ocr-review): 采纳评审意见（版本探针 / 空格路径 / 注释回归） by Seller-1990 · Pull Request #15 · Seller-1990/zcode-advisor

Seller-1990 · 2026-10-08T08:30:08Z

背景

#14 合并后，用新脚本对自身跑了一次端到端评审（bash scripts/ocr-review.sh HEAD~1 HEAD，nas-hy4/space-bunny，7 条意见）。逐条核实后本 PR 处理其中 4 条。

已处理

意见	处理
版本探针只判主版本 `version_info[0] == 3`，而内嵌脚本用 f-string（需 3.6+），3.0–3.5 会被判可用、随后渲染阶段 `SyntaxError`	改判 `version_info >= (3, 6)`
候选目录用无引号 `$(ls …)` 分词，`$HOME` 在 Windows 上常含空格（`C:\Users\First Last`），一个路径被拆成两个不存在的目录	改为逐行读（`while IFS= read -r` + here-string），顺带免掉 glob 展开
头部链注释被重写时弄丢了「改链：编辑 chain-public 再跑 `ocr-sync-reviewer.sh`」的操作指引	补回；private 模板同步补 chain-private 版
`cleanup()` 上方被压成 3 行，丢了 main 原有的「两道守卫」不变量说明与 `OCR_REVIEW_RAW_OUT` 存在理由	还原 main 原文

未采纳（误报）

两条称「只把 LOCK_ACQUIRED 写进注释、没改代码」：224f506 起 cleanup() 尾部就有 [ -n "$LOCK_ACQUIRED" ] && rm -rf "$LOCK_DIR"（当前 scripts/ocr-review.sh:296），本次只是在它上方加解释，函数体未动。
一条称「for candidate 此前因字面 \n 失效、现在才生效故有分词风险」：分词风险判断正确（已按上表第二条修掉），但「此前失效」是误读——旧写法虽产生伪候选词 n，目录本身仍被扫描。

验证

bash -n scripts/ocr-review.sh 通过
node --test test/ocr-review.test.js → 28/28 pass
npm test → 594 tests / 593 pass / 1 skipped / 0 fail
候选循环空格路径实测：HOME=/tmp/First Last 下逐行候选为 /tmp/First Last/AppData/Roaming/npm，不再被拆
模板与仓库副本 sha256 一致（c5b5f3862709942b…），同步器幂等

落点

改动同时写入 ~/.dsh/templates/ocr-review/ocr-review.sh 与 .private——standard 模式下仓库副本由模板整份再生，只改仓库会在下次同步时丢失。

新脚本端到端跑出的评审提了 7 条，逐条核实后处理 4 条： - 版本探针只判主版本 `version_info[0] == 3`，而内嵌脚本用 f-string（3.6+）： 3.0–3.5 会被判可用、随后渲染阶段 SyntaxError。→ 改判 `>= (3, 6)`。 - 候选目录用无引号 `$(ls …)` 分词，`$HOME` 在 Windows 上常含空格（C:\Users\First Last），一个路径会被拆成两个不存在的目录。 → 改为逐行读（`while IFS= read -r` + here-string），顺带免掉 glob 展开。 - 头部链注释被重写时弄丢了「改链：编辑 chain-public 再跑 ocr-sync-reviewer.sh」这句操作指引。→ 补回（private 模板同步补 chain-private 版）。 - cleanup() 上方被压成 3 行，丢了「两道守卫」不变量说明与 OCR_REVIEW_RAW_OUT 存在理由（main 原有）。→ 还原 main 原文。未采纳 3 条（均为误报）： - 两条称「只把 LOCK_ACQUIRED 写进注释、没改代码」：224f506 起 cleanup() 尾部就有 `[ -n "$LOCK_ACQUIRED" ] && rm -rf "$LOCK_DIR"`（当前 :296），改动只是在它上方加解释，函数体未动。 - 一条称「for candidate 之前因字面 \n 失效、现在才生效故有分词风险」：风险判断正确，已按上面第二条修掉；但「此前失效」是误读——旧写法虽产生伪候选词 n，目录本身仍被扫描，并非完全失效。模板（~/.dsh/templates/ocr-review/ocr-review.sh 与 .private）同步更新，保证下次 ocr-sync-reviewer.sh 不会把这几处改回去。

github-actions · 2026-10-08T08:31:14Z

🔍 OpenCodeReview found 8 issue(s) in this PR.

✅ Successfully posted inline: 5 comment(s)
⏭️ Skipped (overlap with history): 3 comment(s)

github-actions · 2026-10-08T08:31:15Z

+    _ocr_cands="$(ls -1d "$HOME"/.nvm/versions/node/*/bin 2>/dev/null | sort -Vr)
+/usr/local/bin
+/opt/homebrew/bin
+${APPDATA:-$HOME/AppData/Roaming}/npm
+$HOME/AppData/Roaming/npm"


在处理 OCR 二进制文件路径时，新增的 _ocr_cands 变量和 while 循环逻辑虽然改进了 Windows 路径处理，但缺少对 _ocr_cands 变量的初始化和清理逻辑的说明。建议在代码中添加注释，说明 _ocr_cands 的作用以及为什么需要逐行读取候选目录，以提高代码的可读性和可维护性。

Suggestion:

Suggested change

_ocr_cands="$(ls -1d "$HOME"/.nvm/versions/node/*/bin 2>/dev/null | sort -Vr)

/usr/local/bin

/opt/homebrew/bin

${APPDATA:-$HOME/AppData/Roaming}/npm

$HOME/AppData/Roaming/npm"

# _ocr_cands: 存储所有可能包含 ocr 二进制文件的候选目录路径，逐行读取以避免 Windows 路径分词问题

_ocr_cands="$(ls -1d "$HOME"/.nvm/versions/node/*/bin 2>/dev/null | sort -Vr)

+/usr/local/bin

+/opt/homebrew/bin

${APPDATA:-$HOME/AppData/Roaming}/npm

$HOME/AppData/Roaming/npm"

github-actions · 2026-10-08T08:31:15Z

  _p="$(command -v "$_cand" 2>/dev/null || true)"
  [ -n "$_p" ] || continue
-  if "$_p" -c 'import sys; sys.exit(0 if sys.version_info[0] == 3 else 1)' 2>/dev/null; then
+  if "$_p" -c 'import sys; sys.exit(0 if sys.version_info >= (3, 6) else 1)' 2>/dev/null; then


在处理 Python 版本检查时，新增的版本条件 sys.version_info >= (3, 6) 可能过于宽泛。考虑到代码的长期维护性，建议明确说明为什么选择 Python 3.6 而不是更高版本（如 3.7+），或者根据项目需求调整为更具体的版本要求（例如 sys.version_info >= (3, 8)）。这有助于避免未来兼容性问题。

Suggestion:

Suggested change

if "$_p" -c 'import sys; sys.exit(0 if sys.version_info >= (3, 6) else 1)' 2>/dev/null; then

# 仅支持 Python 3.8+，确保兼容性与稳定性

if "$_p" -c 'import sys; sys.exit(0 if sys.version_info >= (3, 8) else 1)' 2>/dev/null; then

github-actions · 2026-10-08T08:31:15Z

 [ -n "$OUT_JSON" ] || { echo "错误：mktemp 未返回路径" >&2; exit 1; }
 ATTEMPT_LOG=""
-# 注意：这个 trap 会覆盖前面的锁清理 trap，所以锁回收必须在这里再做一次。
+# 注意：这个 trap 会覆盖前面的锁清理 trap，所以 rm -rf "$LOCK_DIR" 必须在这里再做一次，


在 cleanup 函数的注释中，提到了 rm -rf "$LOCK_DIR" 的重要性，但缺少对 $LOCK_DIR 变量定义和作用的说明。建议在代码中添加注释，解释 $LOCK_DIR 的含义以及它在何处被定义，以便于维护者理解其作用和逻辑。

Suggestion:

Suggested change

# 注意：这个 trap 会覆盖前面的锁清理 trap，所以 rm -rf "$LOCK_DIR" 必须在这里再做一次，

# $LOCK_DIR: 用于存储锁文件的目录，确保在脚本退出时清理锁文件，避免影响后续运行

# 注意：这个 trap 会覆盖前面的锁清理 trap，所以 rm -rf "$LOCK_DIR" 必须在这里再做一次，

github-actions · 2026-10-08T08:31:15Z

+#   - `$CHOSEN` 非空 = 确有一跳产出完整评审。**不能**只看 OUT_JSON 非空：链全失败时
+#     OUT_JSON 里留着最后一跳的 partial/无效输出，照拷会把上一份好结论覆盖成半成品。


在 cleanup 函数的注释中，提到了 $CHOSEN 变量的重要性，但缺少对其赋值逻辑的具体说明。建议在代码中添加注释，解释 $CHOSEN 变量在何处被赋值以及其具体含义（例如，哪一跳模型被成功选择并产出了完整评审）。这有助于维护者理解其作用和逻辑。

Suggestion:

Suggested change

# - `$CHOSEN` 非空 = 确有一跳产出完整评审。**不能**只看 OUT_JSON 非空：链全失败时

# OUT_JSON 里留着最后一跳的 partial/无效输出，照拷会把上一份好结论覆盖成半成品。

# - `$CHOSEN` 变量在 `valid_output` 函数中被赋值，表示成功选择的模型名称（例如：chain-public 中的某个模型）。

# 该变量非空表示至少有一跳模型产出了完整且有效的评审结果，确保 OUT_JSON 的内容可信。

github-actions · 2026-10-08T08:31:15Z

+#     OUT_JSON 里留着最后一跳的 partial/无效输出，照拷会把上一份好结论覆盖成半成品。
+#   - 命中锁而提前退出的进程根本走不到这里（锁检查在 OUT_JSON 创建之前）。
 cleanup() {
  if [ -n "${OCR_REVIEW_RAW_OUT:-}" ] && [ -n "${CHOSEN:-}" ]; then


在 cleanup 函数的注释中，提到了 OCR_REVIEW_RAW_OUT 和 $CHOSEN 的重要性，但缺少对 $CHOSEN 变量赋值逻辑的说明。建议在代码中添加注释或文档，解释 $CHOSEN 何时被赋值以及其具体含义，以便于维护者理解其作用和逻辑。

Suggestion:

Suggested change

if [ -n "${OCR_REVIEW_RAW_OUT:-}" ] && [ -n "${CHOSEN:-}" ]; then

# $CHOSEN 仅在 valid_output 函数通过后赋值，表示成功选择了一个有效的模型输出。

if [ -n "${OCR_REVIEW_RAW_OUT:-}" ] && [ -n "${CHOSEN:-}" ]; then

* fix(test): 超时退出码断言补 124，修 main 上的单元测试红灯 PR #14 给 run_with_deadline 加了「有 GNU timeout 就优先用它」的分支，脚本这边 case 已经改成 124|137|143，但测试的断言只写了 137|143： - macOS（本机开发环境）没有 timeout，走进程组回退，返回 143 → 本地全绿； - ubuntu-latest 有 GNU timeout，到点自杀返回 124 → main 上这个用例红。实测复现：把 GNU timeout 的行为（自成进程组 + 整组 TERM→KILL + 返回 124）做成 PATH 上的替身，再用旧断言跑，报的错与 CI 一字不差： actual: ' ⚠ 本跳到 2s 上限被强制终止（退出码 124），降级到下一个\n…' expected: /上限被强制终止（退出码 (137|143)）/ error: '超时必须明说并带上退出码' 脚本的分类没错（124 确实是 GNU timeout 到点自杀的码），错的是断言把平台的具体退出码写死了。改成只要求「是脚本认定为被强制终止的那组码」，并补注释说明两条实现路径各返回什么。两条路径本机都实测通过（原 143 路径 28/28；注入 GNU timeout 替身后 28/28，含孙进程回收断言）。 * ci: PR 也跑单元测试（Linux 差异此前只在合并后才暴露） build-installers 原先只在 push main / tag / 手动触发时跑，PR 上只有 ocr-review 的 advisory 评审，不跑 npm test —— 所以「本机全绿、Linux 红」这类差异会一路合进 main 才亮灯。PR #14 和 #15 就是这么各红了一次（两次都是同一个用例）。构建类 job 都带 `startsWith(github.ref,'refs/tags/v') || event_name == 'workflow_dispatch'` 守卫，加 pull_request 后 PR 上只剩 test 这一个 job。顺带修掉文件头一句不实的注释（原先写「此前只有 tag 与 PR 触发测试」，实际从来没有过 pull_request 触发，git log 三个版本都没有）。

github-actions Bot reviewed Oct 8, 2026

View reviewed changes

Seller-1990 merged commit ff9c3c6 into main Oct 8, 2026
1 check passed

Seller-1990 deleted the fix/ocr-review-followups branch October 8, 2026 08:32

Seller-1990 mentioned this pull request Oct 10, 2026

fix(ci): 超时退出码断言补 124 + PR 阶段也跑单元测试（修 main 上的红灯） #16

Merged

Navigation Menu

Repository navigation

fix(ocr-review): 采纳评审意见（版本探针 / 空格路径 / 注释回归） - #15

fix(ocr-review): 采纳评审意见（版本探针 / 空格路径 / 注释回归）#15
Seller-1990 merged 1 commit into
mainfrom
fix/ocr-review-followups

Seller-1990 commented Oct 8, 2026

Uh oh!

github-actions Bot commented Oct 8, 2026 •

edited

Loading

Uh oh!

github-actions Bot Oct 8, 2026

Uh oh!

github-actions Bot Oct 8, 2026

Uh oh!

github-actions Bot Oct 8, 2026

Uh oh!

github-actions Bot Oct 8, 2026

Uh oh!

github-actions Bot Oct 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	if "$_p" -c 'import sys; sys.exit(0 if sys.version_info >= (3, 6) else 1)' 2>/dev/null; then
	# 仅支持 Python 3.8+，确保兼容性与稳定性
	if "$_p" -c 'import sys; sys.exit(0 if sys.version_info >= (3, 8) else 1)' 2>/dev/null; then

	# 注意：这个 trap 会覆盖前面的锁清理 trap，所以 rm -rf "$LOCK_DIR" 必须在这里再做一次，
	# $LOCK_DIR: 用于存储锁文件的目录，确保在脚本退出时清理锁文件，避免影响后续运行
	# 注意：这个 trap 会覆盖前面的锁清理 trap，所以 rm -rf "$LOCK_DIR" 必须在这里再做一次，

		# - `$CHOSEN` 非空 = 确有一跳产出完整评审。不能只看 OUT_JSON 非空：链全失败时
		# OUT_JSON 里留着最后一跳的 partial/无效输出，照拷会把上一份好结论覆盖成半成品。

	if [ -n "${OCR_REVIEW_RAW_OUT:-}" ] && [ -n "${CHOSEN:-}" ]; then
	# $CHOSEN 仅在 valid_output 函数通过后赋值，表示成功选择了一个有效的模型输出。
	if [ -n "${OCR_REVIEW_RAW_OUT:-}" ] && [ -n "${CHOSEN:-}" ]; then

Conversation

Seller-1990 commented Oct 8, 2026

背景

已处理

未采纳（误报）

验证

落点

Uh oh!

github-actions Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

github-actions Bot commented Oct 8, 2026 •

edited

Loading