Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
175 changes: 149 additions & 26 deletions scripts/ocr-review.sh
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,17 @@
# - 也**不定时跑**:只在每个 PR 合并前调用一次。
#
# 模型降级链(provider|model,与 ~/.opencodereview/config.json 的 custom_providers 对应):
# ⚠️ 本脚本含云端 provider,**只允许公开仓使用**(云端会把代码发往第三方);
# 私有仓用 ocr-review.sh.private 变体(NAS-only,见 ~/.dsh/templates/ocr-review/)。
# 前四棒全 0 倍率免费(2026-10-04 成本优先,避开 kimi-k3 等贵模型):
# 主 nas-hy4 -> deepseek-v4.1-flash (8787 免费,工具调用已实测)
# 备用① nas-octopus -> glm-5.3-flash (8088 PM-API 免费分组)
# 备用② nas-hy4 -> hy3 (8787 免费)
# 备用③ nas-hy4 -> hy4-preview-f (8787 免费)
# 备用④ x666 -> ministral-14b-latest (云端 0 倍率,工具调用已实测)
# 备用⑤ lucky-gem -> gemini-3.6-flash (云端,主人提供 key,工具调用已实测)
# 备用⑥ lucky -> stealth/space-bunny-alpha (云端,工具调用已实测)
# 备用⑦ lucky -> step-5-preview (云端,工具调用已实测)
# 备用⑧ daigua -> gpt-6-sol (云端,顾问生产验证过工具调用)
# ⚠️ 链的**唯一真源**是 ~/.dsh/templates/ocr-review/chain-public(运行时直接读它)。
# 本文件里那段 CHAIN 数组只是读不到链文件时的**兜底快照**,别在这里找当前顺序——
# 它必然滞后(改链不必改脚本,也就没人会记得同步它)。
# 改链:编辑 chain-public 再跑 bash ~/.dsh/bin/ocr-sync-reviewer.sh 刷快照。
# 这里不再逐条列举模型名与顺序:链外置的全部意义就是「枚举会过期」,再抄一份
# 只会制造第二处会撒谎的注释(2026-10-06 实测:注释写着主棒 deepseek,运行时首跳
# 其实是 chain-public 的 minimax-m3)。各模型的可用性/耗时/成本结论写在链文件里,
# 那才是它们生效的地方。
# 【2026-10-04】隐私铁律暂缓(用户拍板):私有仓也可用本链(私有副本用 ocr-review.sh.private
# 变体,链走 chain-private,同样允许 cloud 条目)。
# ⚠️ kimi-k3 不支持工具调用(三次探针无 tool_calls)→ 不能当 ocr 模型,勿加回。
# 用法:
# ./scripts/ocr-review.sh # 默认基准 origin/dev(不存在则回退 main)
# ./scripts/ocr-review.sh v1.8.8 # 对比 tag / 分支
Expand All @@ -41,9 +40,14 @@ cd "$REPO_ROOT" || exit 1
# - 持锁进程已死(崩溃/被 kill)→ 视为陈旧锁,接管
LOCK_DIR="$REPO_ROOT/.git/ocr-review.lock"
LOCK_INFO="$LOCK_DIR/info"
# 只有**自己拿到锁**才允许在退出时删锁。OCR_REVIEW_FORCE=1 整段跳过、根本不取锁,
# 若无条件删就会把另一个正在跑的评审的锁连根拔掉——而并行评审正是 FORCE 的用途,
# 于是「强制并行」变成「两个进程互相拆锁」。清理由 cleanup() 按这个标记判断。
LOCK_ACQUIRED=""
# OCR_REVIEW_FORCE=1 可绕过(需要并行评审时显式指定)
if [ -z "${OCR_REVIEW_FORCE:-}" ]; then
if mkdir "$LOCK_DIR" 2>/dev/null; then
LOCK_ACQUIRED=1
printf 'pid=%s\nstarted=%s\n' "$$" "$(date +%s)" > "$LOCK_INFO"
trap 'rm -rf "$LOCK_DIR"' EXIT INT TERM
else
Expand All @@ -57,6 +61,7 @@ if [ -z "${OCR_REVIEW_FORCE:-}" ]; then
fi
# 陈旧锁:持锁进程已不存在,接管
echo "[ocr-review] 发现陈旧锁(pid ${HOLD_PID:-未知} 已退出),接管"
LOCK_ACQUIRED=1
printf 'pid=%s\nstarted=%s\n' "$$" "$(date +%s)" > "$LOCK_INFO"
trap 'rm -rf "$LOCK_DIR"' EXIT INT TERM
fi
Expand Down Expand Up @@ -132,18 +137,76 @@ fi
AGENT_MODEL="$(tr -d '[:space:]' < "$AGENT_MODEL_FILE" 2>/dev/null || true)"
[ -n "$AGENT_MODEL" ] && echo "(编码代理当前模型:${AGENT_MODEL},评审将优先使用其他 provider)"

# 降级链:逐个尝试,第一个**产出有效 JSON** 的即采用
# === 降级链区块开始(由 ~/.dsh/bin/ocr-sync-reviewer.sh 从 chain-public 生成,勿手改)===
# 降级链唯一真源:~/.dsh/templates/ocr-review/chain-public
# 一行一条 scope|provider|model,顺序即优先级,第一个产出有效 JSON 的即采用。
# 改链只改那个文件:副本运行时也直接读它,下面这份只是读不到时的兜底快照。
CHAIN_FILE="${OCR_REVIEW_CHAIN_FILE:-$HOME/.dsh/templates/ocr-review/chain-public}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security · high
在 CHAIN_FILE 路径构建时,未对路径进行绝对路径验证或符号链接解析,可能存在目录遍历风险。建议使用 realpath 或 readlink -f 确保路径安全。

CHAIN_SCOPE="public" # public 副本:cloud 条目合法,但仍以链文件为唯一真源
Comment on lines +144 to +145

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security · high
在动态链文件加载逻辑中,虽然进行了基本的格式验证(如 scope|provider|model),但缺少对 CHAIN_FILE 的权限验证。建议在读取文件前增加权限检查,确保文件不可写且属主正确,以防止恶意篡改。例如:

if [ -r "$CHAIN_FILE" ] && [ ! -w "$CHAIN_FILE" ] && [ "$(stat -c '%u' "$CHAIN_FILE")" = "$(id -u)" ]; then
  # 继续加载链文件
fi

CHAIN=(
"nas-hy4|deepseek-v4.1-flash"
"runanytime|z-ai/glm-5.2"
"nas-octopus|glm-5.3-flash"
Comment on lines +148 to 149

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · medium
在 CHAIN 数组中添加了 runanytime-astra|gpt-6-astra 和 runanytime|z-ai/glm-5.3 两个条目,但缺少对这两个新条目的注释说明。建议在注释中明确说明这两个条目的作用、所属类别(如云端或本地)以及是否支持工具调用等信息,以保持注释与代码的一致性。

"nas-hy4|hy3"
"nas-hy4|hy4-preview-f"
"x666|ministral-14b-latest"
"lucky-gem|gemini-3.6-flash"
"runanytime-astra|gpt-6-astra"
"runanytime|z-ai/glm-5.3"
"lucky|stealth/space-bunny-alpha"
"lucky|step-5-preview"
"daigua|gpt-6-sol"
)
# 运行时优先读链文件:读不到 / 解析出 0 条 → 保留上面的快照(脚本被单独拷走也跑得起来)。
_chain_found=()
if [ -r "$CHAIN_FILE" ]; then
while IFS= read -r _line || [ -n "$_line" ]; do
_line="${_line%%#*}" # 去掉行内注释
_line="${_line//[[:space:]]/}" # 去空白,容忍手滑多打空格
_pipes="${_line//[!|]/}" # 只留竖线,用来数段数
[ "${#_pipes}" -eq 2 ] || continue # 必须正好 scope|provider|model
_scope="${_line%%|*}"; _entry="${_line#*|}"
[ -n "$_scope" ] || continue
_p="${_entry%%|*}"; _m="${_entry#*|}"
[ -n "$_p" ] && [ -n "$_m" ] || continue
Comment on lines +169 to +170

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · medium
在 CHAIN 数组的动态加载逻辑中,虽然进行了基本的格式验证(如 scope|provider|model),但缺少对 provider 或 model 的非空验证。例如,如果 provider 或 model 为空,可能会导致后续逻辑出错。建议在验证逻辑中增加对 provider 和 model 的非空检查,确保其值有效。

_chain_found[${#_chain_found[@]}]="$_entry"
done < "$CHAIN_FILE"
fi
Comment on lines +160 to +173

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · medium
在 CHAIN 数组的处理逻辑中,虽然增加了从外部文件读取链配置的功能,但未对 CHAIN_FILE 的内容进行格式验证。例如,如果 CHAIN_FILE 包含无效的行(如缺少 | 分隔符或空行),可能会导致 CHAIN 数组被错误地构建,进而影响降级链的正常工作。建议在读取 CHAIN_FILE 时添加更严格的格式验证,确保每行都符合 scope|provider|model 的格式。

Suggestion:

Suggested change
_chain_found=()
if [ -r "$CHAIN_FILE" ]; then
while IFS= read -r _line || [ -n "$_line" ]; do
_line="${_line%%#*}" # 去掉行内注释
_line="${_line//[[:space:]]/}" # 去空白,容忍手滑多打空格
_pipes="${_line//[!|]/}" # 只留竖线,用来数段数
[ "${#_pipes}" -eq 2 ] || continue # 必须正好 scope|provider|model
_scope="${_line%%|*}"; _entry="${_line#*|}"
[ -n "$_scope" ] || continue
_p="${_entry%%|*}"; _m="${_entry#*|}"
[ -n "$_p" ] && [ -n "$_m" ] || continue
_chain_found[${#_chain_found[@]}]="$_entry"
done < "$CHAIN_FILE"
fi
_chain_found=()
if [ -r "$CHAIN_FILE" ]; then
while IFS= read -r _line || [ -n "$_line" ]; do
_line="${_line%%#*}" # 去掉行内注释
_line="${_line//[[:space:]]/}" # 去空白,容忍手滑多打空格
_pipes="${_line//[!|]/}" # 只留竖线,用来数段数
[ "${#_pipes}" -eq 2 ] || {
echo "警告:跳过无效行(必须包含 scope|provider|model):$_line" >&2
continue
}
_scope="${_line%%|*}"; _entry="${_line#*|}"
[ -n "$_scope" ] || {
echo "警告:跳过无效行(scope 为空):$_line" >&2
continue
}
_p="${_entry%%|*}"; _m="${_entry#*|}"
[ -n "$_p" ] && [ -n "$_m" ] || {
echo "警告:跳过无效行(provider 或 model 为空):$_line" >&2
continue
}
_chain_found[${#_chain_found[@]}]="$_entry"
done < "$CHAIN_FILE"
fi

if [ "${#_chain_found[@]}" -gt 0 ]; then
CHAIN=("${_chain_found[@]}")
else
echo "(链文件 ${CHAIN_FILE} 不可用,用内嵌快照 ${#CHAIN[@]} 条)" >&2
fi
# === 降级链区块结束 ===

# 时间预算:防"建连后永久挂起"把整条链卡死(本机无 GNU timeout,见 run_with_deadline)。
# HOP_TIMEOUT 单跳上限。完整评审实测 5–15 分钟,900s 覆盖正常耗时,只掐挂起。
# TOTAL_BUDGET 整条链最长耗时。公开链条目多,按 6 跳封顶——再长会把
# .git/ocr-review.lock 占用到之后每次推送都被误判"已有评审在进行"。
HOP_TIMEOUT="${OCR_REVIEW_HOP_TIMEOUT:-900}"
TOTAL_BUDGET="${OCR_REVIEW_TOTAL_BUDGET:-5400}"
CHAIN_START="$(date +%s)"

# 上游限流与资产噪音的稳定参数(都是实测踩出来的教训,不是默认值拍脑袋):
# --concurrency:ocr 默认并发 8 个子任务,实测会打满 x666 的「25 请求/5 分钟」限流(429),
# 曾连续 6 次评审全灭。降并发换稳;限流充裕的 provider 用 OCR_REVIEW_CONCURRENCY=8 调回。
# --exclude:二进制图片资产进 diff 只会塞爆每个分组的上下文,对文本评审是纯噪音
# (zcode-advisor 实测两个模型 20 组全返回空 comments)。要连图片一起审就设空串关掉。
# 用数组而不是字符串展开:排除模式里的 **/*.png 是通配符,字符串展开会被
# 当前目录里的真实文件 glob 成文件名,旗标就废了。
OCR_REVIEW_CONCURRENCY="${OCR_REVIEW_CONCURRENCY:-2}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance · medium
虽然默认并发数设置为 2 以避免限流,但对于某些高延迟的 provider(如 runanytime),该值可能仍然过高。建议在文档或代码注释中明确说明如何根据 provider 的具体限流策略调整 OCR_REVIEW_CONCURRENCY 的值,以提高灵活性。例如,可以添加一条注释说明如何为不同 provider 设置不同的并发数。

Suggestion:

Suggested change
OCR_REVIEW_CONCURRENCY="${OCR_REVIEW_CONCURRENCY:-2}"
# OCR_REVIEW_CONCURRENCY:默认 2 以避免限流,但可根据 provider 的限流策略调整。
# 例如:OCR_REVIEW_CONCURRENCY=8(适用于限流充裕的 provider)

OCR_REVIEW_EXCLUDE="${OCR_REVIEW_EXCLUDE:-**/*.png,**/*.ico,**/*.icns,**/*.jpg,**/*.jpeg,**/*.gif,**/*.pdf}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · medium
在 OCR_REVIEW_EXCLUDE 的默认值中,使用了通配符排除图片和二进制文件,但缺少对 .md 文件的排除。考虑到 Markdown 文件可能包含非文本内容(如图片链接),建议明确说明是否需要排除 .md 文件,或者根据实际需求进行调整。

此外,建议在文档中明确说明如何自定义 OCR_REVIEW_EXCLUDE,以避免误排除关键文件。

Suggestion:

Suggested change
OCR_REVIEW_EXCLUDE="${OCR_REVIEW_EXCLUDE:-**/*.png,**/*.ico,**/*.icns,**/*.jpg,**/*.jpeg,**/*.gif,**/*.pdf}"
# OCR_REVIEW_EXCLUDE:默认排除图片和二进制文件,避免噪音。
# 示例:OCR_REVIEW_EXCLUDE="**/*.png,**/*.md,**/*.pdf"(排除 Markdown 文件)

OCR_EXTRA_ARGS=()
OCR_EXTRA_ARGS+=(--concurrency "$OCR_REVIEW_CONCURRENCY")
[ -n "$OCR_REVIEW_EXCLUDE" ] && OCR_EXTRA_ARGS+=(--exclude "$OCR_REVIEW_EXCLUDE")
Comment on lines +198 to +200

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · low
在 OCR_EXTRA_ARGS 数组构建逻辑中,虽然使用了数组来避免字符串展开问题,但缺少对 OCR_REVIEW_EXCLUDE 的默认值验证。建议在构建 OCR_EXTRA_ARGS 时,增加对 OCR_REVIEW_EXCLUDE 的非空验证,确保其值不会导致 --exclude 标志被错误地传递给 ocr review 命令。


# 实际使用台账:每次评审把"用了哪个 provider/model、成没成"追加到一份公共日志,
# 供 ~/.dsh/hooks/ocr-pool-status.sh 展示最近几跳(否则没人知道当前到底在用谁)。
# 只写 provider/model 名与结果,不写 diff 内容、不写 key。
USAGE_LOG="${OCR_REVIEW_USAGE_LOG:-$HOME/.dsh/hooks/ocr-pool-usage.log}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security · medium
在 record_use 函数中,虽然避免了记录敏感信息(如 diff 内容或 key),但 USAGE_LOG 的默认路径 $HOME/.dsh/hooks/ocr-pool-usage.log 可能对其他用户可见。建议检查该目录的权限设置,并确保日志文件的权限限制在当前用户。例如,可以在函数中增加权限验证或使用 umask 设置日志文件的默认权限。

Suggestion:

Suggested change
USAGE_LOG="${OCR_REVIEW_USAGE_LOG:-$HOME/.dsh/hooks/ocr-pool-usage.log}"
# 确保 USAGE_LOG 目录和文件权限仅对当前用户可写
mkdir -p "$(dirname "$USAGE_LOG")" && chmod 700 "$(dirname "$USAGE_LOG")" 2>/dev/null || true
USAGE_LOG="${OCR_REVIEW_USAGE_LOG:-$HOME/.dsh/hooks/ocr-pool-usage.log}"

record_use() { # $1=provider $2=model $3=ok|skip|fail
printf '%s\t%s\t%s\t%s\t%s\n' "$(date '+%F %T')" "$(basename "${REPO_ROOT:-$PWD}")" "$1" "$2" "$3" \
>>"$USAGE_LOG" 2>/dev/null || true
Comment on lines +207 to +208

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · medium
在 record_use 函数中,虽然避免了记录敏感信息,但日志格式中使用了 $(basename "${REPO_ROOT:-$PWD}"),这可能会导致在某些特殊情况下(如路径过长或包含特殊字符)产生意外行为。建议增加对 REPO_ROOT 或 PWD 的有效性验证,确保其值不会导致日志格式化错误。例如,可以使用 printf '%q' 来安全地处理路径中的特殊字符。

Comment on lines +205 to +208

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · low
在 record_use 函数中,虽然实现了日志记录功能,但未对 USAGE_LOG 文件的写入权限进行验证。例如,如果 USAGE_LOG 文件所在目录没有写入权限,可能会导致日志记录失败,而脚本仍然继续运行,导致日志不完整。建议在调用 record_use 前增加权限验证,并提供友好的错误提示。

Suggestion:

Suggested change
USAGE_LOG="${OCR_REVIEW_USAGE_LOG:-$HOME/.dsh/hooks/ocr-pool-usage.log}"
record_use() { # $1=provider $2=model $3=ok|skip|fail
printf '%s\t%s\t%s\t%s\t%s\n' "$(date '+%F %T')" "$(basename "${REPO_ROOT:-$PWD}")" "$1" "$2" "$3" \
>>"$USAGE_LOG" 2>/dev/null || true
record_use() {
local provider="$1"
local model="$2"
local result="$3"
# 检查 USAGE_LOG 文件的写入权限
local log_dir="$(dirname "$USAGE_LOG")"
if [ ! -w "$log_dir" ]; then
echo "警告:无法写入日志文件 $USAGE_LOG,目录 $log_dir 无写入权限,跳过记录" >&2
return 1
fi
printf '%s\t%s\t%s\t%s\t%s\n' "$(date '+%F %T')" "$(basename "${REPO_ROOT:-$PWD}")" "$provider" "$model" "$result" \
>>"$USAGE_LOG" 2>/dev/null || echo "警告:写入日志文件 $USAGE_LOG 失败" >&2

}

# 模板必须以 XXXXXX 结尾:GNU mktemp 把 -t 当 --tmpdir 并要求该后缀(BSD/macOS 两者都接受)。
TMPDIR_OCR="${TMPDIR:-/tmp}"
Expand All @@ -152,6 +215,8 @@ OUT_JSON="$(mktemp "${TMPDIR_OCR%/}/ocr-review-XXXXXX" 2>/dev/null)" || {
}
[ -n "$OUT_JSON" ] || { echo "错误:mktemp 未返回路径" >&2; exit 1; }
ATTEMPT_LOG=""
# 注意:这个 trap 会覆盖前面的锁清理 trap,所以 rm -rf "$LOCK_DIR" 必须在这里再做一次,
# 否则锁目录永不消失,之后每次运行都误报"陈旧锁"(虽然能接管,但看着像坏了)。
# OCR_REVIEW_RAW_OUT(可选,由调用方提供,例如 pre-push hook):把 ocr 的**原始 JSON**
# 另存一份。它是权威产物(下面的渲染表由它生成),而 OUT_JSON 只是 mktemp 临时文件、
# 退出时被本函数删掉 —— 此前评审成功后原始 JSON 一并消失,事后无法复核或重渲染。
Expand Down Expand Up @@ -202,6 +267,8 @@ cleanup() {
fi
fi
rm -f "$OUT_JSON" ${ATTEMPT_LOG:+"$ATTEMPT_LOG"}
# 只在**自己持锁**时回收(见 LOCK_ACQUIRED 定义处):无条件删会把并行评审的锁拆掉。
[ -n "$LOCK_ACQUIRED" ] && rm -rf "$LOCK_DIR"
}
trap cleanup EXIT INT TERM

Expand Down Expand Up @@ -242,6 +309,50 @@ if not isinstance(data.get("comments"), list):
PYCHECK
}

# hard deadline:给一条命令套"最多 secs 秒"的上限,超时整组终止(含孙进程)。
#
# 为什么不用 `timeout`:本机是 macOS + bash 3.2,没有 GNU coreutils 的 timeout,
# 也没有 gtimeout(防止某天被装进来)。
# 为什么不用 `perl -e 'alarm N; exec @ARGV'`:alarm 只杀 perl 自己变出的那个进程,
# ocr 派生的子进程会漏成孤儿。这里改用"自成进程组 + 整组 signale":
# set -m 让后台任务成为组长(PGID == PID),超时时 kill -TERM -- -PID 全组带走。
# 安全细节:只有在确认目标 PGID 等于自身 PID 时才发组信号,否则退化成
# "先杀子进程、再杀父进程",绝不可能误伤本脚本所在的进程组。
# 退出码:快命令原样透传;被杀时 143(TERM)/137(KILL),调用方据此判定超时。
run_with_deadline() {
local secs="$1"; shift
case "$secs" in ''|*[!0-9]*) "$@"; return $? ;; esac
Comment on lines +322 to +324

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · medium
在 run_with_deadline 函数中,虽然实现了超时机制,但缺少对 secs 参数的有效性验证。例如,如果 secs 为负数或非常大的值,可能会导致意外行为。建议在函数开头增加对 secs 的范围验证,确保其在合理范围内(如 1 秒到最大允许的时间限制)。

[ "$secs" -gt 0 ] || { "$@"; return $?; }
set -m

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance · high
在 run_with_deadline 函数中,虽然使用了 set -m 创建进程组,但未验证所有 shell 环境是否支持该功能。特别是在非 GNU coreutils 环境下,可能导致进程组管理失败,进而影响超时杀死逻辑的可靠性。建议添加环境检查或提供 fallback 机制。

"$@" &
local cmd_pid=$!
set +m
(
sleep "$secs" 2>/dev/null
if [ "$(ps -o pgid= -p "$cmd_pid" 2>/dev/null | tr -d ' ')" = "$cmd_pid" ]; then
kill -TERM -- "-$cmd_pid" 2>/dev/null
else
pkill -TERM -P "$cmd_pid" 2>/dev/null
kill -TERM "$cmd_pid" 2>/dev/null
fi
Comment on lines +332 to +337

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security · high
在 run_with_deadline 函数中,虽然使用了进程组(PGID)来确保信号传递的准确性,但仍需确保 ps -o pgid= 的输出不会被篡改或误判。建议增加额外的验证步骤,例如检查进程组的父进程(PPID)是否与预期一致,以进一步提高安全性。

此外,建议在 kill 命令前增加对 cmd_pid 的有效性验证,避免无效 PID 导致的意外行为。

Suggestion:

Suggested change
if [ "$(ps -o pgid= -p "$cmd_pid" 2>/dev/null | tr -d ' ')" = "$cmd_pid" ]; then
kill -TERM -- "-$cmd_pid" 2>/dev/null
else
pkill -TERM -P "$cmd_pid" 2>/dev/null
kill -TERM "$cmd_pid" 2>/dev/null
fi
if kill -0 "$cmd_pid" 2>/dev/null; then
if [ "$(ps -o pgid= -p "$cmd_pid" 2>/dev/null | tr -d ' ')" = "$cmd_pid" ]; then
kill -TERM -- "-$cmd_pid" 2>/dev/null
else
pkill -TERM -P "$cmd_pid" 2>/dev/null
kill -TERM "$cmd_pid" 2>/dev/null
fi
else
echo "警告:无效的 cmd_pid($cmd_pid),可能已终止或不存在" >&2
fi

sleep 2 2>/dev/null
if [ "$(ps -o pgid= -p "$cmd_pid" 2>/dev/null | tr -d ' ')" = "$cmd_pid" ]; then
kill -KILL -- "-$cmd_pid" 2>/dev/null
else
pkill -KILL -P "$cmd_pid" 2>/dev/null
kill -KILL "$cmd_pid" 2>/dev/null
fi
Comment on lines +338 to +344

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance · medium
在 run_with_deadline 函数中,虽然使用了进程组(PGID)来确保信号传递的准确性,但 sleep 2 的延迟可能不足以确保子进程完全终止。建议增加更健壮的进程终止机制,例如使用 wait 循环或增加更长的等待时间(如 5 秒),以确保所有子进程被完全清理。

) &
local watcher_pid=$!
# 下面三行的 || rc=$? / || true 是给 set -e 型调用方兜底:超时后这一跳返回 143,
# 裸 wait 在 set -euo pipefail 的脚本里会直接把整个评审带走,链就不再降级了(实测踩过)。
local rc=0
wait "$cmd_pid" 2>/dev/null || rc=$?
kill "$watcher_pid" 2>/dev/null || true
wait "$watcher_pid" 2>/dev/null || true
return $rc
}

echo "评审范围:$FROM .. $TO"
CHOSEN=""
SKIPPED=0
Expand All @@ -250,33 +361,41 @@ FAILURE_DIAG=""
for entry in "${CHAIN[@]}"; do
provider="${entry%%|*}"
model="${entry##*|}"
# 总预算先用完就不再试后面的:宁可本次无评审,也不无限期占着锁
REMAIN=$(( TOTAL_BUDGET - ($(date +%s) - CHAIN_START) ))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other · medium
在 REMAIN 计算中,使用了 $(date +%s) 获取当前时间戳,但未考虑时区差异或时间戳截断的问题。建议在文档中明确说明该脚本假设系统时区一致,或者使用 UTC 时间戳进行计算,以避免跨时区环境下的预算误判。

Suggestion:

Suggested change
REMAIN=$(( TOTAL_BUDGET - ($(date +%s) - CHAIN_START) ))
# 使用 UTC 时间戳以避免时区差异影响预算计算
CHAIN_START="$(date -u +%s)" # 使用 UTC 时间戳
REMAIN=$(( TOTAL_BUDGET - ($(date -u +%s) - CHAIN_START) ))

if [ "$REMAIN" -le 0 ]; then
echo " 总预算 ${TOTAL_BUDGET}s 已用尽,剩余 provider 不再尝试"
break
fi
if [ -n "$AGENT_MODEL" ] && [ "$model" = "$AGENT_MODEL" ]; then
echo " 跳过 ${provider}($model 正被编码代理占用)"
SKIPPED=$((SKIPPED + 1))
[ "$SKIPPED" -eq "${#CHAIN[@]}" ] && SKIPPED_ALL=1
continue
fi
HOP_LIMIT="$HOP_TIMEOUT"
[ "$HOP_LIMIT" -gt "$REMAIN" ] && HOP_LIMIT="$REMAIN"
Comment on lines +376 to +377

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance · low
在 HOP_LIMIT 计算逻辑中,虽然考虑了剩余预算 REMAIN,但未考虑 HOP_TIMEOUT 的最小值限制。例如,如果 HOP_TIMEOUT 设置为非常小的值(如 1 秒),可能会导致每次尝试的时间过短,影响评审结果的准确性。建议在计算 HOP_LIMIT 时,增加一个最小值限制(如 30 秒),以确保每次尝试有足够的时间完成。

# 每次尝试前清空,防止失败后残留上次结果被误判为成功
: > "$OUT_JSON"
ATTEMPT_LOG="$(mktemp "${TMPDIR_OCR%/}/ocr-attempt-XXXXXX" 2>/dev/null || true)"
echo " 尝试 provider=$provider model=$model …"
# --concurrency 2:ocr 默认并发 8 个子任务(每组还有多轮工具调用),实测会打满
# x666 的「25 请求/5 分钟」限流(429)——连续 6 次评审全灭的根因。降并发换稳。
# 可用 OCR_REVIEW_CONCURRENCY 覆盖;限流充裕的 provider 可设回 8。
# --exclude 图片:assets/icon.png 是 ~1MB 二进制,进 diff 会塞爆每个分组的上下文,
# 实测两个模型全部返回空 comments(20 组全灭)——二进制资产对文本评审只有噪音。
if ocr review --from "$FROM" --to "$TO" --format json --output "$OUT_JSON" \
--concurrency "${OCR_REVIEW_CONCURRENCY:-2}" \
--exclude '**/*.png,**/*.ico,**/*.icns,**/*.jpg,**/*.jpeg,**/*.gif,**/*.pdf' \
--provider "$provider" --model "$model" >"${ATTEMPT_LOG:-/dev/null}" 2>&1 && valid_output; then
echo " 尝试 provider=$provider model=$model (上限 ${HOP_LIMIT}s)…"
HOP_RC=0
run_with_deadline "$HOP_LIMIT" ocr review --from "$FROM" --to "$TO" --format json --output "$OUT_JSON" \
${OCR_EXTRA_ARGS[@]+"${OCR_EXTRA_ARGS[@]}"} \
--provider "$provider" --model "$model" >"${ATTEMPT_LOG:-/dev/null}" 2>&1 || HOP_RC=$?
if [ "$HOP_RC" -eq 0 ] && valid_output; then
echo " 成功:$provider / $model"
CHOSEN="$provider/$model"
record_use "$provider" "$model" ok
[ -n "$ATTEMPT_LOG" ] && rm -f "$ATTEMPT_LOG"
break
fi
case "$HOP_RC" in
137|143) echo " ⚠ 本跳到 ${HOP_LIMIT}s 上限被强制终止(退出码 ${HOP_RC}),降级到下一个" ;;
esac
Comment on lines +393 to +395

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · low
在错误信息输出中,虽然提供了基本的错误描述,但缺少具体的错误代码或上下文信息。建议在错误信息中增加更详细的说明,例如在超时终止时明确指出退出码(如 143 或 137),以帮助用户快速定位问题。

Suggestion:

Suggested change
case "$HOP_RC" in
137|143) echo " ⚠ 本跳到 ${HOP_LIMIT}s 上限被强制终止(退出码 ${HOP_RC}),降级到下一个" ;;
esac
case "$HOP_RC" in
143) echo " ⚠ 本跳超时终止(退出码 143,上限 ${HOP_LIMIT}s),降级到下一个" ;;
137) echo " ⚠ 本跳被强制终止(退出码 137,上限 ${HOP_LIMIT}s),降级到下一个" ;;
*) echo " ⚠ 本跳终止(退出码 ${HOP_RC},上限 ${HOP_LIMIT}s),降级到下一个" ;;
esac

# 保留该次失败输出尾部,供全失败时排查(此前一律丢进 /dev/null)
if [ -n "$ATTEMPT_LOG" ]; then
FAILURE_DIAG="${FAILURE_DIAG}--- ${provider}/${model} ---
FAILURE_DIAG="${FAILURE_DIAG}--- ${provider}/${model} (exit=${HOP_RC}, limit=${HOP_LIMIT}s) ---
$(tail -5 "$ATTEMPT_LOG" 2>/dev/null)
"
Comment on lines +398 to 400

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maintainability · medium
在 FAILURE_DIAG 的构建中,仅保留了最后 5 行日志,对于复杂失败场景可能不足以定位问题。建议增加更多上下文信息,例如完整的错误日志或关键变量状态,以便更好地排查问题。

Suggestion:

Suggested change
FAILURE_DIAG="${FAILURE_DIAG}--- ${provider}/${model} (exit=${HOP_RC}, limit=${HOP_LIMIT}s) ---
$(tail -5 "$ATTEMPT_LOG" 2>/dev/null)
"
FAILURE_DIAG="${FAILURE_DIAG}--- ${provider}/${model} (exit=${HOP_RC}, limit=${HOP_LIMIT}s) ---
$(tail -20 "$ATTEMPT_LOG" 2>/dev/null || echo "无日志可用")
关键变量:provider=${provider}, model=${model}, HOP_TIMEOUT=${HOP_TIMEOUT}
"

rm -f "$ATTEMPT_LOG"
Expand All @@ -286,10 +405,14 @@ done

if [ -z "$CHOSEN" ]; then
if [ "$SKIPPED_ALL" = "1" ]; then
# 与「链坏了」区分开:这不是 provider 失败,是编码代理占用了链上每一个模型。
# 记 skip 而不是 fail,否则台账会把「今天没评审」统计成「provider 全挂」。
record_use "-" "-" skip
echo "错误:所有 provider 都因与编码代理同模型被跳过(agent-model=${AGENT_MODEL})。" >&2
echo " 可临时指定:ocr review --provider <p> --model <m>" >&2
else
echo "错误:三级 provider 全部失败,未产生有效评审结果。" >&2
record_use "-" "-" fail
echo "错误:${TOTAL_BUDGET}s 预算内没有一跳产出有效评审结果。" >&2
fi
if [ -n "$FAILURE_DIAG" ]; then
echo "各 provider 输出尾部:" >&2
Expand Down
Loading
Loading