Repository navigation
chore(ocr): 接入自动评审(本地 pre-push + 公开仓 CI) - #2
Seller-1990 wants to merge 1 commit into
Conversation
- scripts/ocr-review.sh:标准链版(链文件外置、逐跳降级、单跳 900s/总 5400s、 用量台账、并发锁、原子发布、JSON 状态白名单校验) - .github/workflows/ocr-review.yml:PR 自动评审(pinned action、llm_timeout 600、 并发 2、sticky 总结评论) - 链首 NAS 免费模型(nas-hy4/space-bunny),云端条目按序降级 - 评审 advisory、不拦门;secrets 已在仓库级配置(OCR_LLM_URL/TOKEN/MODEL) Windows 适配:NAS 走 Tailscale 直连、GNU timeout 优先、python 优先、git-common-dir 定位
|
🔍 OpenCodeReview found 12 issue(s) in this PR.
|
| LOCK_DIR="$GIT_COMMON_DIR/ocr-review.lock" | ||
| LOCK_INFO="$LOCK_DIR/info" |
There was a problem hiding this comment.
并发锁机制存在竞态条件风险。在 LOCK_DIR 目录创建与 LOCK_INFO 写入之间存在时间窗口,如果进程在此窗口内崩溃(例如被 SIGKILL 杀死),trap 无法触发,导致锁目录永久残留。建议使用原子操作(如 flock 或 mkdir 与写入合并)确保锁的完整性。
Suggestion:
| LOCK_DIR="$GIT_COMMON_DIR/ocr-review.lock" | |
| LOCK_INFO="$LOCK_DIR/info" | |
| # 使用原子操作确保锁的完整性,例如使用 flock 或合并 mkdir 和写入操作 | |
| # 或者使用以下方式确保锁的完整性: | |
| LOCK_DIR="$GIT_COMMON_DIR/ocr-review.lock" | |
| LOCK_INFO="$LOCK_DIR/info" | |
| if mkdir "$LOCK_DIR" 2>/dev/null; then | |
| # 使用 flock 或其他原子操作确保写入成功后再释放锁 | |
| LOCK_ACQUIRED=1 | |
| trap 'rm -rf "$LOCK_DIR"' EXIT INT TERM | |
| # 确保写入操作成功后再设置 LOCK_ACQUIRED | |
| printf 'pid=%s\nstarted=%s\n' "$$" "$(date +%s)" > "$LOCK_INFO" || { | |
| rm -rf "$LOCK_DIR" | |
| exit 1 | |
| } | |
| fi |
| LOCK_ACQUIRED="" | ||
| # OCR_REVIEW_FORCE=1 可绕过(需要并行评审时显式指定) | ||
| if [ -z "${OCR_REVIEW_FORCE:-}" ]; then |
There was a problem hiding this comment.
在 cleanup() 函数中,锁删除逻辑依赖于 LOCK_ACQUIRED 变量,但该变量仅在 mkdir 成功时设置。如果进程在 mkdir 后崩溃,LOCK_ACQUIRED 未设置但实际持有锁,导致锁目录残留。此外,OCR_REVIEW_FORCE=1 时跳过锁删除,但未检查是否由当前进程持有锁。建议修改逻辑,确保锁删除仅在当前进程持有锁时执行。
Suggestion:
| LOCK_ACQUIRED="" | |
| # OCR_REVIEW_FORCE=1 可绕过(需要并行评审时显式指定) | |
| if [ -z "${OCR_REVIEW_FORCE:-}" ]; then | |
| # 使用更健壮的锁验证机制,确保仅在当前进程持有锁时删除 | |
| LOCK_ACQUIRED="" | |
| LOCK_PID_FILE="$LOCK_DIR/info" | |
| # 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_PID_FILE" || { | |
| rm -rf "$LOCK_DIR" | |
| exit 1 | |
| } | |
| trap 'cleanup_lock' EXIT INT TERM | |
| else | |
| HOLD_PID="" | |
| [ -f "$LOCK_PID_FILE" ] && HOLD_PID="$(sed -n 's/^pid=//p' "$LOCK_PID_FILE" 2>/dev/null)" | |
| if [ -n "$HOLD_PID" ] && kill -0 "$HOLD_PID" 2>/dev/null; then | |
| echo "[ocr-review] 已有评审在进行(pid ${HOLD_PID}),本次跳过以免重复消耗。" | |
| exit 0 | |
| fi | |
| # 陈旧锁:持锁进程已不存在,接管 | |
| echo "[ocr-review] 发现陈旧锁(pid ${HOLD_PID:-未知} 已退出),接管" | |
| LOCK_ACQUIRED=1 | |
| printf 'pid=%s\nstarted=%s\n' "$$" "$(date +%s)" > "$LOCK_PID_FILE" | |
| trap 'cleanup_lock' EXIT INT TERM | |
| fi | |
| fi | |
| cleanup_lock() { | |
| # 仅在当前进程持有锁时删除 | |
| if [ -n "$LOCK_ACQUIRED" ] && [ "$$" = "$(sed -n 's/^pid=//p' "$LOCK_PID_FILE" 2>/dev/null 2>&1 || true)" ]; then | |
| rm -rf "$LOCK_DIR" | |
| fi | |
| } |
| require_ref() { | ||
| local ref="$1" | ||
| case "$ref" in ''|-*) return 1 ;; esac | ||
| # rev-parse --verify 对 blob/tree 也返回 0,必须确认解析为 commit | ||
| git rev-parse --verify --quiet --end-of-options "${ref}^{commit}" >/dev/null 2>&1 |
There was a problem hiding this comment.
在 require_ref() 函数中,仅使用 git rev-parse --verify --quiet --end-of-options "${ref}^{commit}" 来验证 ref 是否为提交对象,但未排除轻量标签或注解标签。这可能导致用户传入 tag 名称(如 v1.0.0),从而导致评审范围错误。建议明确排除 tag 对象,仅接受提交对象。
Suggestion:
| require_ref() { | |
| local ref="$1" | |
| case "$ref" in ''|-*) return 1 ;; esac | |
| # rev-parse --verify 对 blob/tree 也返回 0,必须确认解析为 commit | |
| git rev-parse --verify --quiet --end-of-options "${ref}^{commit}" >/dev/null 2>&1 | |
| require_ref() { | |
| local ref="$1" | |
| case "$ref" in ''|-*) return 1 ;; esac | |
| # 确保 ref 是提交对象,且不是 tag | |
| if ! git rev-parse --verify --quiet --end-of-options "${ref}^{commit}" >/dev/null 2>&1; then | |
| return 1 | |
| fi | |
| # 进一步确认 ref 不是 tag(轻量或注解标签) | |
| if git cat-file -t "$ref" >/dev/null 2>&1; then | |
| local ref_type=$(git cat-file -t "$ref") | |
| case "$ref_type" in | |
| tag|commit) return 1 ;; # 排除 tag,仅接受 commit | |
| *) return 0 ;; # 其他类型(blob/tree)也返回 0,但已被 rev-parse 排除 | |
| esac | |
| fi | |
| return 0 |
| _pipes="${_line//[!|]/}" # 只留竖线,用来数段数 | ||
| [ "${#_pipes}" -eq 2 ] || continue # 必须正好 scope|provider|model |
There was a problem hiding this comment.
在降级链解析逻辑中,_pipes="${_line//[!|]/}" 的实现存在逻辑错误。该行意图是检查 _line 中 | 的数量,但实际操作是删除非 | 字符,导致无法正确判断 _line 是否包含两个段(scope|provider|model)。建议修正为检查 | 的数量,而不是删除非 | 字符。
Suggestion:
| _pipes="${_line//[!|]/}" # 只留竖线,用来数段数 | |
| [ "${#_pipes}" -eq 2 ] || continue # 必须正好 scope|provider|model | |
| # 修正逻辑:检查 | 的数量而不是删除非 | 字符 | |
| _pipe_count=$(echo "$_line" | tr '|' '\n' | grep -v '^$' | wc -l) | |
| [ "$_pipe_count" -eq 2 ] || continue # 必须正好 scope|provider|model |
| OCR_EXTRA_ARGS+=(--concurrency "$OCR_REVIEW_CONCURRENCY") | ||
| [ -n "$OCR_REVIEW_EXCLUDE" ] && OCR_EXTRA_ARGS+=(--exclude "$OCR_REVIEW_EXCLUDE") |
There was a problem hiding this comment.
在 OCR_EXTRA_ARGS 数组展开时,使用 ${OCR_EXTRA_ARGS[@]+"${OCR_EXTRA_ARGS[@]}"} 语法存在冗余,且未处理数组为空的情况。建议简化为 ${OCR_EXTRA_ARGS[@]},避免不必要的复杂性。
Suggestion:
| OCR_EXTRA_ARGS+=(--concurrency "$OCR_REVIEW_CONCURRENCY") | |
| [ -n "$OCR_REVIEW_EXCLUDE" ] && OCR_EXTRA_ARGS+=(--exclude "$OCR_REVIEW_EXCLUDE") | |
| OCR_EXTRA_ARGS=() | |
| OCR_EXTRA_ARGS+=(--concurrency "$OCR_REVIEW_CONCURRENCY") | |
| [ -n "$OCR_REVIEW_EXCLUDE" ] && OCR_EXTRA_ARGS+=(--exclude "$OCR_REVIEW_EXCLUDE") | |
| ... | |
| "${OCR_EXTRA_ARGS[@]}" \ |
| 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" \ |
There was a problem hiding this comment.
在 record_use() 函数中,$(basename "${REPO_ROOT:-$PWD}") 未进行敏感字符过滤,可能记录仓库路径中的私密信息(如用户主目录路径)。建议对仓库名称进行脱敏处理,避免泄漏敏感信息。
Suggestion:
| 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" \ | |
| record_use() { # $1=provider $2=model $3=ok|skip|fail | |
| local repo_name="$(basename "${REPO_ROOT:-$PWD}")" | |
| # 对仓库名称进行脱敏处理,避免泄漏敏感信息 | |
| repo_name="${repo_name%%+([^/])*/*}" # 仅保留最后一级目录或文件名 | |
| printf '%s\t%s\t%s\t%s\t%s\n' "$(date '+%F %T')" "$repo_name" "$1" "$2" "$3" \ |
| 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 | ||
| } |
There was a problem hiding this comment.
在 record_use() 函数中,$(basename "${REPO_ROOT:-$PWD}") 未进行敏感字符过滤,可能记录仓库路径中的私密信息(如用户主目录路径或绝对路径)。建议对仓库名称进行脱敏处理,例如仅记录仓库名称或哈希值,避免泄漏敏感信息。
Suggestion:
| 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() { # $1=provider $2=model $3=ok|skip|fail | |
| local repo_name="$(basename "${REPO_ROOT:-$PWD}")" | |
| # 对仓库名称进行脱敏处理,仅记录名称或哈希值 | |
| repo_name="${repo_name%%.*}" # 移除文件扩展名 | |
| printf '%s\t%s\t%s\t%s\t%s\n' "$(date '+%F %T')" "$repo_name" "$1" "$2" "$3" \ | |
| >>"$USAGE_LOG" 2>/dev/null || true | |
| } |
| set -m | ||
| "$@" & | ||
| 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 |
There was a problem hiding this comment.
在 run_with_deadline() 函数中,子进程组管理逻辑存在跨平台兼容性问题。特别是 kill -TERM -- "-$cmd_pid" 依赖于 ps -o pgid= 的输出等于 $cmd_pid 判断,但在 Windows Git Bash 或非 GNU 环境下可能行为异常。此外,pkill -TERM -P "$cmd_pid" 与 kill -TERM "$cmd_pid" 存在冗余,且 pkill 的 -P 选项在某些系统上可能不支持。建议简化逻辑,确保跨平台兼容性。
Suggestion:
| set -m | |
| "$@" & | |
| 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 | |
| # 简化子进程组管理逻辑,确保跨平台兼容性 | |
| set -m | |
| "$@" & | |
| local cmd_pid=$! | |
| set +m | |
| ( | |
| sleep "$secs" 2>/dev/null | |
| # 使用 kill -TERM -- "-$cmd_pid" 直接终止整个进程组 | |
| # 避免依赖 ps 和 pkill,提高可移植性 | |
| kill -TERM -- "-$cmd_pid" 2>/dev/null || true | |
| sleep 2 2>/dev/null | |
| # 确保进程组已终止,再发送 SIGKILL | |
| if kill -0 "$cmd_pid" 2>/dev/null; then | |
| kill -KILL -- "-$cmd_pid" 2>/dev/null || true | |
| fi | |
| ) & | |
| local watcher_pid=$! | |
| 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 |
| 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 | ||
| 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 |
There was a problem hiding this comment.
在 run_with_deadline() 函数中,虽然已经考虑了跨平台兼容性问题,但 kill -TERM -- "-$cmd_pid" 和 pkill -TERM -P "$cmd_pid" 的逻辑仍然存在冗余。特别是 pkill -TERM -P "$cmd_pid" 在某些系统(如 macOS 或 Windows Git Bash)上可能不支持 -P 选项,导致命令失败。建议简化为仅使用 kill -TERM -- "-$cmd_pid",并确保在所有平台上都能正确处理进程组终止。
Suggestion:
| 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 | |
| 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 | |
| # 新机(Windows Git Bash / 装了 coreutils 的机器)有 GNU timeout,优先用它: | |
| # | |
| # ⚠️ 必须用 `timeout --version` 探测而不是 `command -v timeout`:Windows 自带 | |
| # C:/Windows/System32/timeout.exe 是同名异物(等 N 秒的延时命令,不包装别的命令), | |
| # 命中它会把 `timeout 900 ocr review …` 当成非法参数静默跑偏。 | |
| # GNU 版有 --version,Windows 版没有(实测其退出码与输出均不同)。 | |
| if timeout --version >/dev/null 2>&1; then | |
| timeout "$secs" "$@" | |
| return $? | |
| fi | |
| set -m | |
| "$@" & | |
| 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 | |
| kill -TERM "$cmd_pid" 2>/dev/null | |
| 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 | |
| kill -KILL "$cmd_pid" 2>/dev/null | |
| fi | |
| ) & | |
| local watcher_pid=$! |
| return $rc | ||
| } | ||
|
|
||
| echo "评审范围:$FROM .. $TO" |
变更
为 Workflow 仓接入 OCR 自动评审体系:
~/.dsh/templates/ocr-review/chain-public唯一真源),内嵌快照兜底链首为 NAS 免费模型(nas-hy4/space-bunny),云端条目按序降级。评审 advisory、不拦门。
验证
bash scripts/ocr-review.sh HEAD~1 HEAD→ 首跳 nas-hy4/space-bunny 成功说明
Windows 适配:NAS 走 Tailscale 直连、GNU timeout 优先、python 优先、git-common-dir 定位。