# code-review Adopt this skill: https://proskills.md/skills/other/kyy-logs-code-review-skill-skills-code-review Page: https://proskills.md/skills/other/kyy-logs-code-review-skill-skills-code-review This document is the public skill bundle for that page. It contains SKILL.md and every helper file the catalog carries. No login is required. ## File: skills/code-review/SKILL.md size: 16307 sha: b31ca329a757873858f341b4ec76063a7b87af4e upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/SKILL.md ````text --- name: code-review description: Use when code has been written, before committing, or when merging a PR; also when the user says "review my code", "check before commit", "take a look at this change", "PR review", or asks for a strict review/merge gate. --- # Code Review > Check for code defects, test quality, and change impact, and confirm that the issues found are closed. ## Resources and Execution Location - `references/`, `scripts/`, and `assets/` are relative to the directory containing the `SKILL.md` read for this run. - Run shell commands from the **reviewed project root**; `docs/work/`, code paths, and project rules all belong to the reviewed project. - `CODE_REVIEW_SKILL_DIR` below is the absolute path of the skill installation directory. Set it on every shell invocation, or replace it directly with the actual absolute path; do not assume variables persist across different shell invocations. - Scripts require basic commands such as Bash 3.2+, Git, awk, grep, and sed. Invoke them via `bash`; no executable bit is needed. - Report parsing requires Python 3.9+ and PyYAML; `python` is invoked by default, and `CODE_REVIEW_PYTHON` can specify the interpreter's absolute path. For installation, see the release repository README; the in-repo entry point is `scripts/install.py`. ## Scope - Review functional, bug-fix, refactoring, or configuration changes that affect code behavior. - Pure documentation, formatting, or typo changes that do not affect code behavior may be skipped. - Static review is still possible when tests fail or have not been run; the report must state the test status and must not claim that merge conditions are met. ## Inputs | Input | Required | Description | |---|---|---| | git repository + diffable changes | Yes | Specify whether to review uncommitted changes or a committed base/head range | | Acceptance criteria | No | When provided, check them off one by one under "Optional Wrap-up" | | Coverage data / command | No | When provided, used for the optional measurement in `test-coverage`; without it, static quality checks on the relevant test code still run | | Rule pack | No | Defaults to the skill's built-in `references/rules/`; can be overridden with `CODE_REVIEW_RULES_DIR` | | Review identifier | No | `{slug}`, defaults to `-` | The artifact root `{output_root}` defaults to `docs/work`; override it with project `.code-review/config.json` `output_dir` or `CODE_REVIEW_OUTPUT_DIR`. This run's artifacts are written to `{output_root}/active/{slug}/`. ## Review Process > Check items are not hardcoded in this file; they are driven by the `references/probes.yaml` registry. > Probe implementation details are in `references/probes/.md`; this file covers **main workflow orchestration and shared probe execution boundaries**. > > **Execution boundary: static review by default.** For coverage checks, prefer reusing an existing report; > when measurement is needed, coverage commands may be run within what this round's task allows. Other checks remain read-only evidence. > Do not modify the reviewed source code during review. Test and CI output are review evidence; > **all tests green does not mean the behavior is correct**. 1. **Determine the review scope**: **Choose a mode**; when unspecified, review uncommitted changes by default. The base/head below are recorded as concrete commit IDs. - **Uncommitted changes (worktree)**: base is the current HEAD. Use `git diff --` to collect staged and unstaged changes; use `git ls-files --others --exclude-standard -z` to collect untracked files, and review their full contents as newly added code. - **Committed range (commits)**: use the user-specified base/head, and collect the diff with `git diff --`. When head is omitted, use the current HEAD and review only that commit range. **Read the corresponding source**: - In worktree mode, read the working tree files. - In commits mode, use `git show :` to read the target version, and read base when an older version is needed. File scans use only on-disk files consistent with the target version. **Record and hand off**: save the mode, base, target (working tree or head), changed-file list, and diff, for use by all probes. Use NUL separators when processing the file list programmatically, and pass each path as a quoted argument. After collecting scope, initialize a new run before dispatching: ```bash bash "$CODE_REVIEW_SKILL_DIR/scripts/collect-metrics" init {slug} ``` The returned JSON gives `review_dir`, `output_dir`, `line_target`, and `branch_target`. Existing active directories, archived runs, or event history with the same slug are rejected. Use a unique slug for a new review; explicitly continuing the same run reuses its directory without init. Never dispatch two runs writing the same directory. Pass the actual directory and effective targets to all probes, overriding the registry's coverage defaults. Init creates the report template. Source inspection is read-only; the workflow writes reports, probe JSON, policy.json, events, and archive metrics. 2. **Read the registry and determine the probe list**: Read `references/probes.yaml` and decide by `requires`: - Specified but missing / unreadable → `error` - Optional input not provided → `skipped`, stating what is missing - Available → execute 3. **Run probes in parallel**: **Dispatch**: the main executor dispatches an independent agent per probe according to the registry, providing the review scope, diff, project root, skill path, and that probe's inputs and `params`, and including in the dispatch prompt "Execution boundary" above and "Shared probe constraints" in this step. Also pass any provided requirements, acceptance criteria, and change context as text or file paths, and ask each probe to read relevant material. Note missing inputs without inventing them. If dependency source or contract material is already available, pass its path, version, and origin for on-demand reuse; do not pre-download all dependencies. The agent reads the instructions pointed to by the registry entry's `file` and `references/output-schema.md`, and executes according to the parameters. Run in batches when concurrent slots are insufficient; if independent agents are unavailable, run serially and record the limitation. **Shared probe constraints (include in dispatch)**: an agent checks only its own probe, and does not re-run the whole process or dispatch further agents; the source version follows step 1. A finding must explain how this change introduced or triggered the issue; unrelated pre-existing issues are described separately and are not included in this round's finding list or counts. Reuse the review scope and material confirmed and handed off by the main executor, together with context already read by this agent. Read more as needed; merge independent read-only queries where possible, run dependent queries step by step, and preserve accurate line numbers and necessary context on first reads of source. Limit local searches to the reviewed repository, explicitly provided related directories, and cache paths corresponding to dependency coordinates. Prefer `rg`, starting with changed files and known relevant directories and narrowing by file type; return only paths when locating files. Expand the scope only for a specific evidence gap; avoid searching the whole repository with `grep -r` and then filtering the output. Record missing or unreadable evidence without expanding to the disk root or user directory, or repeating searches without a new lead. Write only `{output_root}/active/{slug}/probes/.json`, not shared reports or metrics. Save identified but unresolved specific concerns in `unverified_items` under the output contract, rather than only in the return message. Once checks are complete, first wait for any background tasks you started to finish, or stop tasks that are no longer needed and confirm they have exited. Then validate against the output contract, save the final JSON, and immediately return the handoff message. After saving, do not search, gather more evidence, or repeat explanations of findings. **Handoff**: every probe must have JSON for this run. Record skip or error reasons according to the output contract; on execution errors, preserve findings already made and let the other probes continue. If an agent fails to start or returns no valid result, the main executor records `error` and the reason without reusing old results. The return message only briefly states the JSON path, the actual checked scope, and necessary limitations, including specific evidence gaps; the main executor incorporates the relevant limitations into the final report. Record `duration` when reliable timing is available; the sum of per-probe durations does not represent this round's wall-clock time. `test-coverage` hands off the report path, target version, numerator/denominator, and percentage for valid coverage via its return message, and the main executor reports them; `--line` accepts only diff line coverage: ```bash bash "$CODE_REVIEW_SKILL_DIR/scripts/collect-metrics" coverage {slug} --line {diff-line-coverage} ``` Add `--branch {branch-coverage}` when a branch value exists, or report only that item. Omit unmeasured or inapplicable metrics without filling in 0; do not call it when neither value is valid. A single-item report updates only that item, and the script does not verify the statistical basis. **Aggregation**: after all agents finish, the main executor runs the synchronization command. The script reads `required` from the registry and computes counts at each level from the probe JSON; do not rewrite raw results or hand-fill counts. Do not deduplicate across probes yet; count by original finding ID, which may include duplicate records of the same root cause: ```bash bash "$CODE_REVIEW_SKILL_DIR/scripts/collect-metrics" sync {slug} --probes ``` This step invalidates the previous batch's reconciliation state, and reconciliation is not recorded yet. 4. **Reconciliation completeness check**: Derive the final counts automatically from the probe JSON: ```bash bash "$CODE_REVIEW_SKILL_DIR/scripts/collect-metrics" sync {slug} ``` `required` only controls whether that probe's `failed/error` execution status directly blocks; it does not change the finding closure requirement. High findings from any probe enter reconciliation and still block merge until closed; medium/low are recorded in the report and excluded from the reconciliation total: ```text RECONCILE_TOTAL = total number of high findings RECONCILE_DONE = number of those whose status is reconciled or false-positive RECONCILE_PENDING = TOTAL - DONE ``` Run it even when there are no high findings, explicitly recording the zero result; do not substitute old counts or hand-filled numbers. Fix format errors first; if records are missing, IDs are duplicated, or statuses conflict, synchronization fails, no counts are written, and the old result must not be used to pass. When there are unresolved issues (`RECONCILE_PENDING > 0`), still deliver the report this round and list the blocking items; do not proceed toward merge. After follow-up fix tasks complete, re-review the affected parts; see `execution.max_retry` in `references/probes.yaml` for the re-dispatch limit. 5. **Compile the report**: Fill in the review scope, explanation, fix suggestions, and limitations per the template; do not embed raw JSON in the body; when acceptance criteria are provided, complete the item-by-item check-off first. The script displays `unverified_items` as "Pending confirmation" in Findings without including them in severity counts; original records retain their IDs, evidence, and counts; when showing the number of distinct issues, group them by the same root cause and consequence and list the corresponding IDs, without raising severity due to duplication or substituting the displayed count for probe or reconciliation counts. Run the following commands to generate the report's facts section from the JSON, validate the display, and obtain the final answer summary: ```bash bash "$CODE_REVIEW_SKILL_DIR/scripts/collect-metrics" render-report {slug} bash "$CODE_REVIEW_SKILL_DIR/scripts/collect-metrics" verify-report {slug} bash "$CODE_REVIEW_SKILL_DIR/scripts/collect-metrics" summary {slug} ``` The scripts update only the `code-review:probes` / `gate` marker regions under Findings and Review Conclusion, preserving other body text and raw JSON. Rendering validates before an atomic update and can be repeated; it stops when markers are missing or duplicated. Validation covers IDs, locations, evidence, severity, status, and counts. `summary` validates the report first and outputs the machine summary only when it is consistent. The high/medium/low counts, finding IDs, and blocking conclusion in the final answer must follow the summary; do not add unrecorded issues or omit recorded high findings. Deliver the report, blocking items, and unverified content to the user; you may add explanations and suggestions, but must not rewrite the automatic regions or overturn known blocking facts. "No recorded blocking items" does not mean acceptance, coverage, tests, or CI have passed. Both `gate-pass {slug} code-review` and `archive` with review records validate the report first, then synchronize counts, and do not automatically fix a rewritten display. ## **Any of the following conditions blocks merge**. This round's report lists the blocking items; the table below gives the follow-up handling requirements: | Blocking condition | Follow-up action | |---|---| | A required probe is `failed` / `error` (`PROBE_REQUIRED_FAILED > 0`) | Handle the issues found or the execution error, then re-run the corresponding probe | | A high finding has not completed reconciliation (`RECONCILE_PENDING > 0`) | Handle per step 4, and re-dispatch the affected probes after fixing | | Reconciliation was never reported (`RECONCILE_REPORTED=0`) | Complete step 4; run the final synchronization even when there are no high findings | | Coverage data was provided and is below baseline (below project line_target; default 85%) | List the coverage gaps, add tests, and re-review | | Acceptance criteria were provided and not all pass, or the criteria themselves are ambiguous | List the failing items; clarify ambiguity first, and do not self-check them as passing | | A high finding remains in `wontfix` | Complete handling per the output contract; "won't fix" cannot offset a pending item | See section 4 of `references/output-schema.md` for how execution status and input errors are determined. Script validation verifies consistency between report records and counts; it cannot prove the checks were sufficient. ## Output Contract - **Main report**: `{output_root}/active/{slug}/code-review.md`, generated per the sections and filling requirements in `assets/code-review.md`. - **Machine records**: `{output_root}/active/{slug}/probes/.json`, format per `references/output-schema.md`; the human report's facts section is derived from them. Historical records must be explicitly migrated to standalone JSON; do not infer from free text or auto-fill zeros, and do not read old embedded blocks at the same time. ## Optional Wrap-up The following items are done conditionally, not as mandatory steps each round: - **Acceptance**: when acceptance criteria are provided, check them off one by one before the final conclusion; otherwise skip and say so. - **lessons**: when a `lessons/` directory exists and something that actually happened this round is worth reusing, record it with the specific problem, cause, and handling: - Pitfalls hit during implementation → `lessons/pitfalls.md` - Cases where AI output was corrected → `lessons/bad-cases.md` - New architectural decisions → `lessons/arch-decisions.md` - **Archive**: when archiving is needed, move `{output_root}/active/{slug}/` to `{output_root}/archive/{YYYY}/`. ```` ## File: skills/code-review/scripts/collect-metrics size: 15036 sha: 89d6a20922bc4299318f162ef937d8adb7c1c629 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/scripts/collect-metrics ```text #!/usr/bin/env bash # code-review skill 质量指标采集（随 skill 安装） # 由 code-review 的探针与主会话调用。 # # code-review 的计数由 sync-results.py 从报告派生；其他指标仍由调用方提供。 # 本脚本记录事件日志，并在 archive 时渲染 scorecard 与趋势 CSV。 set -euo pipefail [ "${BASH_VERSINFO[0]}" -gt 3 ] || { [ "${BASH_VERSINFO[0]}" -eq 3 ] && [ "${BASH_VERSINFO[1]}" -ge 2 ]; } || { echo "Bash 3.2+ required" >&2; exit 1; } SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # shellcheck source=lib-metrics . "$SCRIPT_DIR/lib-metrics" usage() { cat <<'EOF' 用法: scripts/collect-metrics <子命令> [选项] 子命令: init Exclusively create a new run; return its directory and effective policy deviation --count N --reasonable M 偏离总数与其中合理的条数 security --high N --medium M --low L 安全与规范检查的三级违规计数 lessons --count N --files "a.md,b.md" 写入的 lessons 条数与文件列表 coverage [--line N] [--branch N] 两项独立选填，只记录提供的数值；单项上报仅更新该项 gate-pass [--kv k=v ...] 记录一次门禁通过。node 取 prd-review / tech-design / tdd-implementation / code-review 之一 sync [--probes] 校验报告并自动记录探针计数及 reconciliation；--probes 仅记录探针，不进行对账 render-report 从独立 JSON 直接更新 Markdown 报告的自动区域 verify-report 只读校验报告自动区域与 JSON 事实完全一致 summary 先校验报告，再输出最终答复可引用的机器摘要（计数、ID、阻塞项） archive [--profile P] 渲染 quality-scorecard.txt 并更新 quality-trend.csv rollback 记录一次回滚（无自动埋点，供人工补录） 说明: 输出根: 项目 output_dir / CODE_REVIEW_OUTPUT_DIR（默认 docs/work） 事件日志: /workflow-events.log scorecard: /active//quality-scorecard.txt 趋势: /quality-trend.csv EOF } # 先完整校验，再一次性追加派生事件；校验失败不写入计数。 sync_review_results() { local slug="$1" phase="${2:-final}" directory skill_dir rows batch="" line local node event key value ts iso details="" status required duration high low validate_slug "$slug" directory="$(find_slug_dir "$slug")" || die "找不到审查目录: $slug" skill_dir="${CODE_REVIEW_SKILL_DIR:-$(dirname "$SCRIPT_DIR")}" local args=(--project-root "$WF_ROOT" --review-dir "$directory" --registry "$skill_dir/references/probes.yaml" --events) [ "$phase" != probes ] || args+=(--probes) rows="$("${CODE_REVIEW_PYTHON:-python}" -X utf8 "$SCRIPT_DIR/sync-results.py" "${args[@]}")" || return 1 event_time ts="$EVENT_TS"; iso="$EVENT_ISO" printf -v batch '%s\t%s\t%s\t-\t-\treview-sync\tphase\t%s\n' "$ts" "$iso" "$slug" "$phase" SYNC_TOTAL=0; SYNC_DONE=0 while IFS=$'\t' read -r node event key value; do [ -n "$node" ] || continue printf -v line '%s\t%s\t%s\t-\t%s\t%s\t%s\t%s\n' "$ts" "$iso" "$slug" "$node" "$event" "$key" "$value" batch+="$line" if [ "$event" = metric:reconcile ]; then case "$key" in total) SYNC_TOTAL="$value" ;; done) SYNC_DONE="$value" ;; esac elif [ "$event" = metric:probe ]; then case "$key" in status) status="$value" ;; required) required="$value" ;; duration) duration="$value" ;; high) high="$value" ;; low) low="$value" ;; medium) printf -v line '%s status=%s required=%s duration=%s high=%s medium=%s low=%s\n' \ "$node" "$status" "$required" "$duration" "$high" "$value" "$low" details+="$line" ;; esac fi done <<< "$rows" mkdir -p "$WF_WORK" if [ ! -f "$WF_EVENTS" ]; then printf '%s\n%s' "$EV_SCHEMA" "$batch" > "$WF_EVENTS" else printf '%s' "$batch" >> "$WF_EVENTS" fi printf '%s' "$details" if [ "$phase" = probes ]; then echo "已同步探针结果（尚未对账）" else echo "已同步报告: total=$SYNC_TOTAL done=$SYNC_DONE pending=$(( SYNC_TOTAL - SYNC_DONE ))" fi } review_report() { local slug="$1" action="$2" directory skill_dir validate_slug "$slug" directory="$(find_slug_dir "$slug")" || die "找不到审查目录: $slug" skill_dir="${CODE_REVIEW_SKILL_DIR:-$(dirname "$SCRIPT_DIR")}" "${CODE_REVIEW_PYTHON:-python}" -X utf8 "$SCRIPT_DIR/sync-results.py" \ --project-root "$WF_ROOT" --review-dir "$directory" --registry "$skill_dir/references/probes.yaml" "$action" } summary_report() { local slug="$1" directory skill_dir validate_slug "$slug" directory="$(find_slug_dir "$slug")" || die "找不到审查目录: $slug" skill_dir="${CODE_REVIEW_SKILL_DIR:-$(dirname "$SCRIPT_DIR")}" "${CODE_REVIEW_PYTHON:-python}" -X utf8 "$SCRIPT_DIR/sync-results.py" \ --project-root "$WF_ROOT" --review-dir "$directory" --registry "$skill_dir/references/probes.yaml" --summary } # 校验非负整数。空串在这里一律拒绝 —— 「没传」与「传了 0」必须区分开， # 否则一个漏传的参数会静默变成 0 分指标。 require_uint() { # <名称> <值> case "${2:-}" in ''|*[!0-9]*) die "$1 必须是非负整数，收到: '${2:-}'" ;; esac } # 通用选项解析：把 "标志 值" 对收进 PARAM_* 全局变量。 # 不用 getopt(1)：长选项在部分平台的 getopt 上不可用。 # # 两次 shift 而不是 shift 2：--flag 恰为最后一个参数时 shift 2 会失败并 # 触发 set -e 直接杀掉脚本；拆成两次则第二次失败可以用 || true 吞掉。 # 取参一律 "${2:-}"：同样是为了躲开 set -u 在未绑定参数上的中止。 parse_flags() { # <...args> PARAM_count=""; PARAM_reasonable=""; PARAM_low=""; PARAM_high="" PARAM_medium=""; PARAM_files=""; PARAM_line=""; PARAM_branch="" PARAM_line_set=false; PARAM_branch_set=false PARAM_profile=""; PARAM_kv="" while [ $# -gt 0 ]; do case "$1" in --count) PARAM_count="${2:-}"; shift; shift 2>/dev/null || true ;; --reasonable) PARAM_reasonable="${2:-}"; shift; shift 2>/dev/null || true ;; --low) PARAM_low="${2:-}"; shift; shift 2>/dev/null || true ;; --high) PARAM_high="${2:-}"; shift; shift 2>/dev/null || true ;; --medium) PARAM_medium="${2:-}"; shift; shift 2>/dev/null || true ;; --files) PARAM_files="${2:-}"; shift; shift 2>/dev/null || true ;; --line) PARAM_line_set=true; PARAM_line="${2:-}"; shift; shift 2>/dev/null || true ;; --branch) PARAM_branch_set=true; PARAM_branch="${2:-}"; shift; shift 2>/dev/null || true ;; --profile) PARAM_profile="${2:-}"; shift; shift 2>/dev/null || true ;; --kv) # 可重复。用换行分隔累积 —— --kv 的值本身不含换行（append_event 会消毒）。 if [ -n "$PARAM_kv" ]; then PARAM_kv="$PARAM_kv ${2:-}" else PARAM_kv="${2:-}" fi shift; shift 2>/dev/null || true ;; *) return 1 ;; esac done return 0 } [ $# -ge 1 ] || { usage >&2; exit 1; } CMD="$1"; shift || true case "$CMD" in init) [ "$#" -eq 1 ] || die "init requires one new slug" "${CODE_REVIEW_PYTHON:-python}" -X utf8 "$SCRIPT_DIR/sync-results.py" \ --registry "${CODE_REVIEW_SKILL_DIR:-$(dirname "$SCRIPT_DIR")}/references/probes.yaml" \ --project-root "$WF_ROOT" --init-review "$1" ;; -h|--help|help) usage; exit 0 ;; deviation) slug="${1:-}"; shift || true [ -n "$slug" ] || { echo "错误: 缺少 slug" >&2; usage >&2; exit 1; } parse_flags "$@" || die "deviation: 未知选项" [ -n "$PARAM_count" ] || die "deviation: 缺少 --count" [ -n "$PARAM_reasonable" ] || die "deviation: 缺少 --reasonable" require_uint "--count" "$PARAM_count" require_uint "--reasonable" "$PARAM_reasonable" append_event "$slug" "-" "-" "metric:deviation" count "$PARAM_count" append_event "$slug" "-" "-" "metric:deviation" reasonable "$PARAM_reasonable" echo "已记录 deviation: count=$PARAM_count reasonable=$PARAM_reasonable" ;; security) slug="${1:-}"; shift || true [ -n "$slug" ] || { echo "错误: 缺少 slug" >&2; usage >&2; exit 1; } parse_flags "$@" || die "security: 未知选项" require_uint "--low" "${PARAM_low:-0}" require_uint "--high" "${PARAM_high:-0}" require_uint "--medium" "${PARAM_medium:-0}" append_event "$slug" "-" "-" "metric:security" low "${PARAM_low:-0}" append_event "$slug" "-" "-" "metric:security" high "${PARAM_high:-0}" append_event "$slug" "-" "-" "metric:security" medium "${PARAM_medium:-0}" echo "已记录 security: high=${PARAM_high:-0} medium=${PARAM_medium:-0} low=${PARAM_low:-0}" ;; lessons) slug="${1:-}"; shift || true [ -n "$slug" ] || { echo "错误: 缺少 slug" >&2; usage >&2; exit 1; } parse_flags "$@" || die "lessons: 未知选项" [ -n "$PARAM_count" ] || die "lessons: 缺少 --count" require_uint "--count" "$PARAM_count" append_event "$slug" "-" "-" "metric:lessons" count "$PARAM_count" append_event "$slug" "-" "-" "metric:lessons" files "${PARAM_files:-}" echo "已记录 lessons: count=$PARAM_count files='${PARAM_files:-}'" ;; coverage) slug="${1:-}"; shift || true [ -n "$slug" ] || { echo "错误: 缺少 slug" >&2; usage >&2; exit 1; } parse_flags "$@" || die "coverage: 未知选项" # 显式提供的选项必须有有效值；先全部校验，避免只写入一半数据。 if [ "$PARAM_line_set" = true ]; then require_uint "--line" "$PARAM_line"; fi if [ "$PARAM_branch_set" = true ]; then require_uint "--branch" "$PARAM_branch"; fi if [ "$PARAM_line_set" = false ] && [ "$PARAM_branch_set" = false ]; then echo "覆盖率未提供（视作未测量），不记录 metric:coverage" >&2 exit 0 fi # 每项独立写入；省略的指标不写事件，也不覆盖该项已有记录。 if [ "$PARAM_line_set" = true ]; then append_event "$slug" "-" "-" "metric:coverage" line "$PARAM_line" fi if [ "$PARAM_branch_set" = true ]; then append_event "$slug" "-" "-" "metric:coverage" branch "$PARAM_branch" fi echo "已记录 coverage: line=${PARAM_line:-本次未上报} branch=${PARAM_branch:-本次未上报}" ;; gate-pass) slug="${1:-}"; shift || true node="${1:-}"; shift || true [ -n "$slug" ] || { echo "错误: 缺少 slug" >&2; usage >&2; exit 1; } [ -n "$node" ] || { echo "错误: 缺少 node" >&2; usage >&2; exit 1; } parse_flags "$@" || die "gate-pass: 未知选项" if [ "$node" = code-review ]; then review_report "$slug" --check-report sync_review_results "$slug" collect_metrics "$slug" [ "$CM_RECONCILE_PENDING" -eq 0 ] || die "code-review 仍有未处置的 high" [ "$CM_PROBE_REQUIRED_FAILED" -eq 0 ] || die "required 探针为 failed/error" [ "$CM_COVERAGE_PASS" != false ] || die "增量行覆盖率低于基线" fi # 总是额外写一行 key="-"：门禁状态的判定依据是「这个 (slug,node) 发生过 # gate-pass」。像 prd-review 这种永远带 --kv 的埋点，若不写这行就检测不到。 append_event "$slug" "-" "$node" "gate-pass" "-" "-" if [ -n "$PARAM_kv" ]; then # PARAM_kv 以换行分隔累积（见 parse_flags）。用 IFS=$'\n' + for 拆分： # 不能用 `read </dev/null summary_report "$slug" ;; sync) slug="${1:-}"; shift || true [ -n "$slug" ] || die "sync: 缺少 slug" phase=final if [ "${1:-}" = --probes ]; then phase=probes; shift; fi [ "$#" -eq 0 ] || die "sync: 只接受 [--probes]" sync_review_results "$slug" "$phase" ;; probe|reconcile) die "手填探针计数和对账计数已停用，请写报告后调用 sync [--probes]" ;; archive) slug="${1:-}"; shift || true [ -n "$slug" ] || { echo "错误: 缺少 slug" >&2; usage >&2; exit 1; } parse_flags "$@" || die "archive: 未知选项" _review_dir="$(find_slug_dir "$slug" || true)" _apply_snapshot "$slug" if [ -d "$_review_dir/probes" ] || [ -f "$_review_dir/code-review.md" ] \ || [ "$CM_PROBE_TOTAL" -gt 0 ] || [ "$CM_RECONCILE_REPORTED" = 1 ]; then review_report "$slug" --check-report sync_review_results "$slug" fi # 先校验再写：否则校验失败时已经往日志里留下了一行孤立的 archive 事件。 require_events "$slug" append_event "$slug" "-" "-" "archive" profile "${PARAM_profile:--}" run_scorecard "$slug" "$PARAM_profile" echo "已生成 scorecard: $(scorecard_of "$(find_slug_dir "$slug")")" echo "已更新趋势: $WF_TREND" # 「没走完流程就归档」的唯一可检测信号。只统计本 profile 的节点， # 否则 hotfix（2 节点）会为 3 个不相干的节点发告警。 pending="$(_pending_count)" if [ "$pending" -gt 0 ]; then warn "本 profile 内有 $pending 个节点无门禁记录（PENDING）。注意本版埋点只覆盖 4 个节点，execution-plan 恒为 PENDING" fi ;; rollback) slug="${1:-}"; shift || true [ -n "$slug" ] || { echo "错误: 缺少 slug" >&2; usage >&2; exit 1; } parse_flags "$@" || die "rollback: 未知选项" append_event "$slug" "-" "-" "rollback" "-" "1" echo "已记录 rollback: $slug" ;; *) echo "错误: 未知子命令: $CMD" >&2 usage >&2 exit 1 ;; esac ``` ## File: skills/code-review/scripts/lib-metrics size: 37776 sha: b218e4c3c2288e1dd8b7aeeffeda82425a0b5179 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/scripts/lib-metrics Content is not inlined in the catalog. Read the upstream file: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/scripts/lib-metrics ## File: skills/code-review/scripts/rule-grep size: 10380 sha: fdd8c6eda1cde833b4546986df815ef3c3e8a31f upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/scripts/rule-grep ```text #!/usr/bin/env bash # code-review skill —— 规则检测模式扫描（repo-guard 探针的第一步） # rule-grep —— 把规则包里**已经写好的「检测模式」**跑一遍，先筛出候选行。 # # 【为什么要它】 # 规则包里带「检测模式」（正则）的条目，不该让 AI **用眼睛执行正则**。 # 先执行已有模式筛选候选，再由 AI 结合规则和上下文复核。 # # 【单一事实源】正则**从规则文件里抽**，不在这里重抄一遍。 # 两处各写一份必然漂；抽出来用，规则改了这里自动跟上。 # # ⚠️ 它只**筛**，不**判**： # - 命中的行**仍然要读对应规则文件**（正例/反例/为什么）才判得准 —— 正则只说「这行像」 # - **未命中也不等于合规**：一部分「检测模式」是从散文里摘的，本来就不完整 # （实测那个误判就是这么来的）。所以未命中只作「已经看过」的记录，不是结论。 # # 用法: # scripts/rule-grep <文件...> # 通常是 git diff --name-only 出来的源码文件 # scripts/rule-grep --list # 只列模式（看能机判多少），不扫 # # 退出码: 0 = 跑完（**不表示有/无命中**）；1 = 用法错误。命中与否看输出。 set -uo pipefail HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" # 规则包位置，优先级： # 1. $CODE_REVIEW_RULES_DIR # 2. 项目根 .code-review/rules/ # 3. skill 安装目录内的 references/rules/ DEFAULT_STD="$HERE/../references/rules" STD="${CODE_REVIEW_RULES_DIR:-}" if [ -z "$STD" ]; then # 规则属于被审项目；不能从 skill 安装位置推导项目根。 PROJECT_ROOT="$(git rev-parse --show-toplevel 2>/dev/null || true)" if [ -z "$PROJECT_ROOT" ]; then # 无 Git 的样例目录也可使用项目规则；支持从项目子目录调用。 PROJECT_ROOT="$PWD" search_dir="$PWD" while [ -n "$search_dir" ] && [ "$search_dir" != "/" ]; do if [ -d "${search_dir%/}/.code-review/rules" ] || [ -d "${search_dir%/}/docs/work" ]; then PROJECT_ROOT="$search_dir" break fi parent_dir="$(dirname "$search_dir")" [ "$parent_dir" = "$search_dir" ] && break search_dir="$parent_dir" done fi PROJECT_STD="$PROJECT_ROOT/.code-review/rules" if [ -d "$PROJECT_STD" ]; then STD="$PROJECT_STD"; else STD="$DEFAULT_STD"; fi fi [ -d "$STD" ] || { echo "错误: 找不到规则包: $STD" >&2; exit 1; } LIST_ONLY=0 if [ "${1:-}" = "--list" ]; then LIST_ONLY=1; shift; fi if [ "$LIST_ONLY" -eq 0 ] && [ "$#" -eq 0 ]; then echo "用法: scripts/rule-grep <文件...> | scripts/rule-grep --list" >&2 exit 1 fi # --- 抽取：`### [严重级] <标题>` 之后的 `**检测模式**` 块，取**反引号里的片段** --- # # ⚠️ 不能在 `|` 或 `/` 上切 —— 那些是**正则自己的或**（`Executors.(newFixedThreadPool|...)`）， # 切了就把它切碎了。反引号才是模式的分界。 _extract() { local f for f in "$STD"/*.md; do [ -f "$f" ] || continue # index.md 是规则包说明，不是规则本身；里面的示例会污染抽取 [ "$(basename "$f")" = "index.md" ] && continue awk -v src="$(basename "$f")" ' /^### [A-Za-z]/ { id = ""; sev = ""; title = "" n = split($0, a, " ") id = a[2] if (match($0, /\[[A-Z]+\]/)) sev = substr($0, RSTART + 1, RLENGTH - 2) if (sev != "" && sev != "HIGH" && sev != "MEDIUM" && sev != "LOW") exit 2 # 标题 = [严重级] 之后的全部 if (match($0, /\] /)) title = substr($0, RSTART + 2) inpat = 0 semantic = 0 next } /^\*\*(检测方式|Detection mode)\*\*/ { semantic = 1 inpat = 0 next } /^\*\*(检测模式|Detection pattern)\*\*/ { if (semantic) next inpat = 1 # ⚠️ **这一行本身也可能带模式** —— 规则库有两种写法，而这里原先直接 `next`： # 块式： **检测模式**（任一命中即违规）: # - `pattern` ← 在**下面**几行 # 内联式：**检测模式**: `pattern` ← 就在**这一行上** # 于是内联式的模式**全部被丢掉**，而且不报错、不计入「不可机判」桶、 # 连计数都没有 —— 输出看起来完全正常。 line = $0 while (match(line, /`[^`]+`/)) { printf "%s\t%s\t%s\t%s\n", src, id, sev, substr(line, RSTART + 1, RLENGTH - 2) line = substr(line, RSTART + RLENGTH) } next } inpat && /^\*\*/ { inpat = 0 } # 遇到 **正例** / **反例** 就停 inpat && /^- / { line = $0 while (match(line, /`[^`]+`/)) { printf "%s\t%s\t%s\t%s\n", src, id, sev, substr(line, RSTART + 1, RLENGTH - 2) line = substr(line, RSTART + RLENGTH) } } ' "$f" || return 1 done } # 标题另取一次（放 awk 里要跟着 id 传递，单独取更简单） _title_of() { awk -v want="$1" ' /^### [A-Za-z]/ { n = split($0, a, " "); cur = a[2] if (match($0, /\] /)) t = substr($0, RSTART + 2) if (cur == want) { print t; exit } }' "$STD"/*.md 2>/dev/null } PATTERNS="$(_extract)" || { echo "错误: 规则严重级别仅支持 HIGH / MEDIUM / LOW" >&2; exit 1; } if [ "$LIST_ONLY" -eq 1 ]; then printf '%s\n' "$PATTERNS" | while IFS=$'\t' read -r src id sev pat; do [ -n "$id" ] || continue printf '%-9s %-13s %s\n' "$id" "[$sev]" "$pat" done exit 0 fi # --- 跑：可机判的先编译试一遍，编译不过的**必须报出来**，不能静默跳过 --- # # ⚠️ 还有一类**比编译不过更坏的**：**看着像正则、其实是描述**。 # `方法签名中含用户输入参数`、`String.format("%s", ...)` # —— 它们在 ERE 里**全都合法**，于是永远"未命中"，给出**虚假的安心**。 # 所以两条件单独归一类：**含中文**（那是规则描述）· **含 `...`**（省略号占位）。 _is_prose() { # ⚠️ 用 here-string，**不要用管道**：`grep -q` 一命中就退出，上游 `printf` 收到 # SIGPIPE（退出 141），而本脚本开了 `pipefail` —— 于是整条管道的状态变成非零， # `_is_prose` **永远返回假**，散文一条都拦不住。实测踩过。 LC_ALL=C grep -q '[^ -~]' <<< "$1" # 含非 ASCII → 中文描述 } # `$` 出现在模式的**中段**（非末尾）→ ERE 把它读成**行尾锚点**，于是该模式 # **永远不可能命中**。编译得过、跑得动、结果恒为「未命中」—— 读起来就是 # 「这条看过了、干净」。**虚假的安心**，和上面那类散文同型，但更隐蔽： # 散文至少看得出是中文。 # # 例如 `a$b` 想匹配字面 `$`，但未转义时会被当作行尾锚点： # `printf 'a$b\n' | grep -E 'a$b'` 不会命中。 # # **只报不改**：规范库是内部标准，改它属于另一件事 —— 这里只负责让它**出声**。 # 判据保守：只看未转义的 `$`，且排除字符类（`[$]` 里的 `$` 是字面量）。 _has_inline_anchor() { local p="$1" i=0 c inbr=0 # ⚠️ `n` **必须单独一行赋值**。写成一条 `local p="$1" … n="${#p}"` 时， # `${#p}` 求值的时刻 `p` 还没绑上 → `set -u` 下 **unbound variable**， # 而且它是在 while 循环里被调的 → **整个扫描当场死**，不是跳过一条模式。 # （同型错误本会话已踩过四次：变量必须在**引用处之前**就已经赋值。） local n="${#p}" while [ "$i" -lt "$n" ]; do c="${p:$i:1}" if [ "$c" = '\' ]; then i=$((i + 2)); continue; fi if [ "$c" = '[' ]; then inbr=1 elif [ "$c" = ']' ]; then inbr=0 elif [ "$c" = '$' ] && [ "$inbr" -eq 0 ] && [ $((i + 1)) -lt "$n" ]; then return 0 fi i=$((i + 1)) done return 1 } _hit=0; _ok=0; _bad=0; _prose=0; _miss="" while IFS=$'\t' read -r src id sev pat; do [ -n "$id" ] || continue if _is_prose "$pat" || case "$pat" in *...*) true ;; *) false ;; esac; then _prose=$((_prose + 1)) printf ' 不可机判 %-9s %-13s %s\n （是**描述**不是模式 —— 仍需人工过）\n' "$id" "[$sev]" "$pat" continue fi grep -qE -- "$pat" /dev/null 2>/dev/null rc=$? if [ "$rc" -eq 2 ]; then # 2 = 正则本身非法 _bad=$((_bad + 1)) # ⚠️ 措辞要准：这一桶里**多数不是"写坏了的正则"，而是"片段"** —— # 规则把说明和几段模式写在同一行（`\`for(X : collection)\` 且同 diff 中存在 # \`.add(\` | …`），按反引号切出来的片段单独看就是括号不配对的。 # 这类**必须人工过**，而它和「跑过且没命中」是两回事 —— 说成「编译不过」 # 会让人以为规则写错了正则，跑去改一个其实没坏的东西。 printf ' 不可机判 %-9s %-13s %s\n （**单独不成模式** —— 常见于规则把片段与说明写在同一行。需人工过，不是「跑过且没命中」）\n' \ "$id" "[$sev]" "$pat" continue fi # 编译得过 ≠ 能命中。见 `_has_inline_anchor` 的注释：中段 `$` = 行尾锚点 = 恒不命中。 if _has_inline_anchor "$pat"; then _bad=$((_bad + 1)) printf ' 不可机判 %-9s %-13s %s\n （`$` 在模式**中段** —— ERE 里那是行尾锚点，该模式**永远不可能命中**。要字面量得写 \\$）\n' \ "$id" "[$sev]" "$pat" continue fi _ok=$((_ok + 1)) out="$(grep -nE -- "$pat" "$@" 2>/dev/null || true)" if [ -n "$out" ]; then _hit=$((_hit + 1)) printf '%s\n' "$out" | while IFS= read -r l; do printf ' %s\n %-9s %-13s %s\n' "$l" "$id" "[$sev]" "$(_title_of "$id")" done else _miss="$_miss$id " fi done <<< "$PATTERNS" echo echo "== 覆盖 ==" echo "可机判 $_ok 条（命中 $_hit 条）· 不可机判 $((_bad + _prose)) 条（散文 $_prose · 编译不过 $_bad）" [ -n "$_miss" ] && echo "未命中: $_miss" echo echo "⚠️ **未命中不等于合规** —— 散文类模式（上面那 $_prose 条）本来就不会命中；" echo " 命中也**不等于违规** —— 读对应规则文件（正例/反例）才判得准。" ``` ## File: skills/code-review/scripts/sync-results.py size: 20542 sha: 9d8ba87db7d3686f24d8214b101615a84b378c91 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/scripts/sync-results.py ````text #!/usr/bin/env python3 """Validate review JSON records, derive metrics, and render a human Markdown report.""" import argparse from collections import Counter from copy import deepcopy import json import html import os from pathlib import Path import re import sys import tempfile import yaml SEVERITIES = ('high', 'medium', 'low') STATUSES = ('open', 'reconciled', 'wontfix', 'false-positive') NAME = re.compile(r'[A-Za-z0-9][A-Za-z0-9._-]*\Z') LABELS_PATH = Path(__file__).resolve().parents[1] / 'assets' / 'report-labels.json' LABEL_KEYS = frozenset({ 'coverage_policy', 'language', 'probe_table_header', 'probe_table_separator', 'required_yes', 'required_no', 'status_passed', 'status_failed', 'status_error', 'status_skipped', 'findings_heading', 'finding_id', 'finding_location', 'finding_status', 'finding_status_open', 'finding_status_reconciled', 'finding_status_wontfix', 'finding_status_false_positive', 'finding_status_suffix', 'finding_evidence', 'finding_reconciled_by', 'no_findings', 'reconcile_line', 'gate_prefix', 'gate_blocked', 'gate_clear', 'gate_detail', 'gate_footer', 'summary_heading', 'summary_findings', 'summary_high_ids', 'summary_medium_ids', 'summary_low_ids', 'summary_reconcile', 'summary_required_failed', 'summary_status', 'summary_blocked', 'summary_clear', 'summary_none', 'unverified_status', 'unverified_probe', 'unverified_reason', 'unverified_needed_evidence', 'summary_unverified', }) def load_labels(): data = json.loads(LABELS_PATH.read_text(encoding='utf-8-sig'), object_pairs_hook=unique_object) require(isinstance(data, dict), 'report labels must be an object') require(data.get('language') in ('en', 'zh-CN'), 'unsupported report language') missing = LABEL_KEYS - data.keys() require(not missing, 'report labels missing keys: ' + ', '.join(sorted(missing))) for key in LABEL_KEYS: require(isinstance(data[key], str) and data[key].strip(), 'empty report label: ' + key) return data def require(condition, message): if not condition: raise ValueError(message) def unique_object(pairs): result = {} for key, value in pairs: require(key not in result, f'重复字段: {key}') result[key] = value return result def runtime_config(project, configuration): """Resolve project policy once for shell commands and probe handoff.""" project = Path(project).resolve() path = project / '.code-review/config.json' data = load_record(path) if path.exists() else {} fields(data, (), ('output_dir', 'line_target', 'branch_target'), 'project policy') registry_data = yaml.load(Path(configuration).read_text(encoding='utf-8-sig'), Loader=UniqueLoader) params = next((p.get('params', {}) for p in registry_data['probes'] if p['name'] == 'test-coverage'), {}) result = {} for key, default in (('line_target', 85), ('branch_target', 80)): value = data.get(key, params.get(key, default)) require(type(value) is int and 0 <= value <= 100, key + ' must be an integer from 0 to 100') result[key] = value output = os.environ.get('CODE_REVIEW_OUTPUT_DIR', data.get('output_dir', 'docs/work')) text(output, 'output_dir') require(not any(c in output for c in '\t\r\n'), 'invalid output_dir') result['output_dir'] = (project / output).resolve().as_posix() require(Path(result['output_dir']) != project, 'output_dir must not be the project root') return result def init_review(project, configuration, slug): require(NAME.fullmatch(slug) is not None and slug not in ('.', '..'), 'invalid slug') policy = runtime_config(project, configuration) root = Path(policy['output_dir']) require(not list((root / 'archive').glob('*/' + slug)), 'slug already archived: ' + slug) events = root / 'workflow-events.log' if events.exists(): require(not any(len(row.split('\t')) > 2 and row.split('\t')[2] == slug for row in events.read_text(encoding='utf-8').splitlines()), 'slug already has events: ' + slug) directory = root / 'active' / slug directory.parent.mkdir(parents=True, exist_ok=True) directory.mkdir() # Exclusive creation: a new run must never reuse existing output. (directory / 'probes').mkdir() template = LABELS_PATH.parent / 'code-review.md' content = template.read_text(encoding='utf-8').replace('{line_target}', str(policy['line_target'])) content = content.replace('{branch_target}', str(policy['branch_target'])) (directory / 'code-review.md').write_text(content, encoding='utf-8') (directory / 'policy.json').write_text(json.dumps(policy, indent=2), encoding='utf-8') print(json.dumps(dict(review_dir=directory.as_posix(), **policy))) class UniqueLoader(yaml.SafeLoader): pass def yaml_mapping(loader, node): return unique_object(loader.construct_pairs(node, deep=True)) UniqueLoader.add_constructor(yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG, yaml_mapping) def text(value, label): require(isinstance(value, str) and bool(value.strip()), f'{label} 必须为非空字符串') return value def fields(value, required, optional, label): require(isinstance(value, dict), f'{label} 必须为对象') require(set(required) <= value.keys(), f'{label} 缺少字段: {set(required) - value.keys()}') require(value.keys() <= set(required) | set(optional), f'{label} 含未知字段: {value.keys() - set(required) - set(optional)}') def load_record(path): return json.loads(path.read_text(encoding='utf-8-sig'), object_pairs_hook=unique_object, parse_constant=lambda value: (_ for _ in ()).throw(ValueError(f'非法 JSON 数值: {value}'))) def registry(path): data = yaml.load(path.read_text(encoding='utf-8-sig'), Loader=UniqueLoader) require(isinstance(data, dict) and isinstance(data.get('probes'), list) and data['probes'], '注册表缺少 probes 清单') probes = {} for item in data['probes']: require(isinstance(item, dict), '探针配置必须为对象') name = text(item.get('name'), '探针 name') require(NAME.fullmatch(name) is not None and name not in probes, f'非法或重复探针名: {name}') require(type(item.get('required')) is bool, f'{name}: required 必须为布尔值') probes[name] = item['required'] return probes def finding(item, probe): required = ('id', 'probe', 'severity', 'status', 'location', 'summary', 'evidence') optional = ('repro', 'reconciled_by', 'reconcile_commit') fields(item, required, optional, 'finding') for key in item: text(item[key], f'finding.{key}') require(item['probe'] == probe, f"{item['id']}: probe 不匹配") pattern = re.escape(probe) + r'-[1-9][0-9]*' require(re.fullmatch(pattern, item['id']) is not None, f"非法 finding ID: {item['id']}") require(item['severity'] in SEVERITIES, f"{item['id']}: 非法 severity") require(item['status'] in STATUSES, f"{item['id']}: 非法 status") require(item['status'] != 'wontfix' or item['severity'] in ('medium', 'low'), f"{item['id']}: 严重问题不得 wontfix") if item['status'] == 'reconciled': text(item.get('reconciled_by'), f"{item['id']}.reconciled_by") return deepcopy(item) def read_probes(directory, configuration): probe_dir = directory / 'probes' expected = {name + '.json' for name in configuration} actual = {p.name for p in probe_dir.glob('*.json')} require(actual == expected, f'探针报告清单不匹配: 缺少 {expected - actual}; 未注册 {actual - expected}') summaries, findings, unverified_items = [], {}, [] for name, required in configuration.items(): block = load_record(probe_dir / (name + '.json')) fields(block, ('probe', 'status', 'findings'), ('duration', 'reason', 'unverified_items'), name) require(block['probe'] == name, f'{name}: 报告 probe 不匹配') require(block['status'] in ('passed', 'failed', 'skipped', 'error'), f'{name}: 非法执行状态') duration = block.get('duration', 0) require(type(duration) is int and duration >= 0, f'{name}: duration 必须为非负整数秒') require(isinstance(block['findings'], list), f'{name}: findings 必须为数组') if block['status'] in ('skipped', 'error'): text(block.get('reason'), f'{name}.reason') elif 'reason' in block: text(block['reason'], f'{name}.reason') pending_items = block.get('unverified_items', []) require(isinstance(pending_items, list), f'{name}: unverified_items 必须为数组') for item in pending_items: keys = ('location', 'summary', 'reason', 'needed_evidence') fields(item, keys, (), f'{name}.unverified_items') for key in keys: text(item[key], f'{name}.unverified_items.{key}') unverified_items.append(dict(probe=name, **deepcopy(item))) counts = Counter() open_high = 0 for raw in block['findings']: item = finding(raw, name) require(item['id'] not in findings, f"重复 finding ID: {item['id']}") findings[item['id']] = item counts[item['severity']] += 1 open_high += item['severity'] == 'high' and item['status'] == 'open' require(block['status'] != 'skipped' or not block['findings'], f'{name}: skipped 不能带 finding') require(block['status'] != 'passed' or not open_high, f'{name}: passed 与 open high 矛盾') require(block['status'] != 'failed' or open_high, f'{name}: failed 缺少 open high') summaries.append(dict(name=name, required=required, status=block['status'], duration=duration, reason=block.get('reason', ''), **{severity: counts[severity] for severity in SEVERITIES})) return summaries, findings, unverified_items def summarize(directory, configuration, probes_only=False): probes, findings, unverified_items = read_probes(directory, registry(configuration)) reconciliation = None if not probes_only: severe = [item for item in findings.values() if item['severity'] == 'high'] done = sum(item['status'] in ('reconciled', 'false-positive') for item in severe) reconciliation = dict(total=len(severe), done=done, pending=len(severe) - done) return dict(probes=probes, reconcile=reconciliation, findings=list(findings.values()), unverified_items=unverified_items) def markdown(value): """Render record text literally, including pipes, newlines, and code fragments.""" value = html.escape(str(value), quote=False) value = re.sub(r'([\\`*_\[\]|#!])', r'\\\1', value) return value.replace('\r\n', '\n').replace('\r', '\n').replace('\n', '
') def render_sections(summary, labels, policy=None): rows = [labels['probe_table_header'], labels['probe_table_separator']] for p in summary['probes']: status = labels['status_' + p['status']] rows.append('| ' + ' | '.join(markdown(value) for value in (p['name'], labels['required_yes'] if p['required'] else labels['required_no'], status, p['duration'], p['high'], p['medium'], p['low'], p['reason'])) + ' |') rows += ['', labels['findings_heading'], ''] for item in summary['findings']: status = labels['finding_status_' + item['status'].replace('-', '_')] title, _, description = item['summary'].replace('\r\n', '\n').replace('\r', '\n').partition('\n') rows += [f"#### [{item['severity']}] {markdown(title)}", ''] if description.strip(): rows += [markdown(description), ''] rows += [ labels['finding_id'] + markdown(item['id']), labels['finding_location'] + markdown(item['location']), labels['finding_status'] + status + labels['finding_status_suffix'].format(status=item['status'])] if item.get('reconciled_by'): rows.append(labels['finding_reconciled_by'] + markdown(item['reconciled_by'])) rows += ['', '
', '' + html.escape(labels['finding_evidence']) + '', '', markdown(item['evidence']), '', '
', ''] for item in summary['unverified_items']: title, _, description = item['summary'].replace('\r\n', '\n').replace('\r', '\n').partition('\n') rows += [f"#### [{labels['unverified_status']}] {markdown(title)}", ''] if description.strip(): rows += [markdown(description), ''] rows += [ labels['unverified_probe'] + markdown(item['probe']), labels['finding_location'] + markdown(item['location']), labels['unverified_reason'] + markdown(item['reason']), labels['unverified_needed_evidence'] + markdown(item['needed_evidence']), '', ] if not summary['findings'] and not summary['unverified_items']: rows.append(labels['no_findings']) r = summary['reconcile'] rows += ['', labels['reconcile_line'].format(**r)] failed = [p['name'] for p in summary['probes'] if p['required'] and p['status'] in ('failed', 'error')] gate = labels['gate_prefix'] + (labels['gate_blocked'] if r['pending'] or failed else labels['gate_clear']) gate += labels['gate_detail'].format(pending=r['pending'], failed=len(failed)) gate += '\n\n' + labels['gate_footer'] if policy is not None: gate += '\n\n' + labels['coverage_policy'].format(**policy) return dict(probes='\n'.join(rows).strip(), gate=gate) def summary_text(summary, labels): """Canonical delivery summary derived only from validated JSON records.""" findings = summary['findings'] by_severity = { severity: [item['id'] for item in findings if item['severity'] == severity] for severity in SEVERITIES } required_failed = [ probe['name'] for probe in summary['probes'] if probe['required'] and probe['status'] in ('failed', 'error') ] reconcile = summary['reconcile'] blocked = bool(reconcile['pending'] or required_failed) lines = [ labels['summary_heading'], labels['summary_findings'].format( **{severity: len(by_severity[severity]) for severity in SEVERITIES}), labels['summary_high_ids'] + (', '.join(by_severity['high']) or '-'), labels['summary_medium_ids'] + (', '.join(by_severity['medium']) or '-'), labels['summary_low_ids'] + (', '.join(by_severity['low']) or '-'), labels['summary_unverified'].format(count=len(summary['unverified_items'])), labels['summary_reconcile'].format(**reconcile), labels['summary_required_failed'] + (', '.join(required_failed) or labels['summary_none']), labels['summary_status'] + (labels['summary_blocked'] if blocked else labels['summary_clear']), ] return '\n'.join(lines) def update_report(directory, sections, check=False): path = directory / 'code-review.md' with path.open(encoding='utf-8-sig', newline='') as stream: raw = stream.read() newline = '\r\n' if '\r\n' in raw else '\n' source = raw.replace('\r\n', '\n') require(not re.search(r'^```json probe-result\b', source, re.M), '原始记录须保存为独立 JSON，最终报告不得嵌入机器记录区块') require('' not in source and '' not in source and re.search(r'^```json reviewer-result', source, re.M) is None, '报告仍包含旧版 Reviewer 区域；reviewer 功能已移除，请迁移或删除该区域后重新渲染') updated = source for name, content in sections.items(): begin, end = f'', f'' require(source.count(begin) == source.count(end) == 1, f'{name}: 自动区域标记缺失或重复') pattern = re.compile(re.escape(begin) + r'\n.*?\n' + re.escape(end), re.S) require(pattern.search(updated) is not None, f'{name}: 自动区域标记顺序错误') updated = pattern.sub(lambda _: begin + '\n' + content + '\n' + end, updated, count=1) if check: require(source == updated, '报告自动区域与原始记录不一致，请重新渲染') return # Validate all input and all markers before touching the report; replace atomically. descriptor, temporary = tempfile.mkstemp(prefix='.code-review-', suffix='.tmp', dir=directory) try: with os.fdopen(descriptor, 'w', encoding='utf-8', newline='') as stream: stream.write(updated.replace('\n', newline)) os.replace(temporary, path) finally: if os.path.exists(temporary): os.unlink(temporary) def events(summary): rows = [] for probe in summary['probes']: for key in ('status', 'required', 'duration', 'high', 'low', 'medium'): value = str(probe[key]).lower() if type(probe[key]) is bool else str(probe[key]) rows.append('\t'.join((probe['name'], 'metric:probe', key, value))) if summary['reconcile'] is not None: for key in ('total', 'done'): rows.append(f"-\tmetric:reconcile\t{key}\t{summary['reconcile'][key]}") return '\n'.join(rows) def main(): # Native Windows Python otherwise emits CRLF, which would enter Bash metric values. sys.stdout.reconfigure(newline='\n') parser = argparse.ArgumentParser(description=__doc__) parser.add_argument('--review-dir', type=Path) parser.add_argument('--registry', type=Path, required=True) parser.add_argument('--project-root', type=Path) parser.add_argument('--runtime-config', action='store_true') parser.add_argument('--init-review') parser.add_argument('--probes', action='store_true', help='Validate probe reports without claiming reconciliation') output = parser.add_mutually_exclusive_group() output.add_argument('--events', action='store_true') output.add_argument('--has-high', action='store_true') output.add_argument('--render', action='store_true', help='Preview generated report sections') output.add_argument('--write-report', action='store_true', help='Replace generated report regions only') output.add_argument('--check-report', action='store_true', help='Verify every generated field without writing') output.add_argument('--summary', action='store_true', help='Print the canonical delivery summary from validated JSON') args = parser.parse_args() try: if args.runtime_config or args.init_review: require(args.project_root is not None, 'project root required') if args.init_review: init_review(args.project_root, args.registry, args.init_review) else: policy = runtime_config(args.project_root, args.registry) print('{output_dir}\t{line_target}\t{branch_target}'.format(**policy)) return 0 require(args.review_dir is not None, 'review directory required') labels = load_labels() summary = summarize(args.review_dir, args.registry, args.probes) if args.render or args.write_report or args.check_report: require(not args.probes, '渲染和展示校验需要最终同步结果') policy = runtime_config(args.project_root, args.registry) if args.project_root else None sections = render_sections(summary, labels, policy) if args.write_report or args.check_report: update_report(args.review_dir, sections, check=args.check_report) if args.summary: require(not args.probes, '摘要需要最终同步结果') except (ValueError, TypeError, OSError, yaml.YAMLError) as error: print(f'错误: {error}', file=sys.stderr) return 1 if args.write_report or args.check_report: print('报告展示校验通过' if args.check_report else '已更新报告自动区域') elif args.render: print('\n\n'.join(sections.values())) elif args.events: print(events(summary)) elif args.has_high: print(str(any(p['high'] for p in summary['probes'])).lower()) elif args.summary: print(summary_text(summary, labels)) else: print(json.dumps(summary, ensure_ascii=False)) return 0 if __name__ == '__main__': sys.exit(main()) ```` ## File: skills/code-review/references/concurrency-examples.md size: 3599 sha: a8d1703308d00cb3c38526368306fd7a01bef5bc upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/concurrency-examples.md ````text # Concurrency interleaving examples These are constructed execution orders, not runtime records. Select relevant examples and verify the interleaving against concrete entry points, reads, writes, and contracts; similar structure alone does not establish a defect. The examples do not cover every matrix type or require every trace to be output. ## 1. Check-then-act: separately locked operations can oversell Contract: stock must not become negative. The check and decrement each hold the same lock, but the decrement does not recheck. ```text Initially: stock = 1. A: acquire the lock, read stock > 0, release the lock. B: acquire the lock, check stock > 0, decrement to 0, release the lock. A: reacquire the lock, act on the earlier result, decrement to -1, release the lock. ``` Failure: A's check becomes stale before the action; locking individual reads and writes does not protect the compound operation. Verify: check and decrement share one critical section, or an effective recheck, conditional update, or equivalent atomic protocol protects the action. ## 2. Cancellation and task termination: an old task writes after its guard is released Contract: only one task may write to a resource at a time. Here, cancellation notification does not wait for termination, and old and new tasks share their result state. ```text Initially: A is running, busy = true. User: request cancellation of A. Cancellation callback: busy = false; A has not exited. User: start again; B passes the busy check and begins execution. A: continue writing the shared result; B also starts writing it. ``` Failure: the guard admits a new task while the old task can still affect the same resource. Verify: the old task and relevant subtasks have terminated before releasing the guard, or actual task isolation prevents the effect. Preventing the old task from publishing results only rules out the corresponding result-overwrite risk; whether busy/loading may be cleared still depends on that state's contract. Separately verify state accuracy and the conditions for admitting a new task. ## 3. Stale callbacks: an old request overwrites a new result Contract: the page displays data for the current selection. Requests can complete out of order, and callbacks do not check request identity. ```text User: select X and start request A. User: switch to Y and start request B. B: return first; the page displays the result for Y. A: return later; its callback unconditionally updates the page with the result for X. ``` Failure: a callback from an old request changes the current page state. Verify: request IDs or versions are checked before publishing results, or another actual guarantee prevents stale callbacks from taking effect; requesting cancellation alone does not establish that callbacks cannot run. ## 4. Correct control: rechecking under the lock prevents a stale decision Contract: reuse cached results when available. Cache publication and the decision to start loading use the same lock. ```text Initially: cache is empty. A: read an empty cache under the lock, then release it. B: publish cache = S under the lock, then release it. A: enter the loading-decision critical section, recheck cache, find S, and decide to reuse it without starting a load. ``` Protection: A does not act on the stale empty-cache check; the decision uses the current cache under the lock. The exclusion rests on the actual recheck and shared-lock protocol, not merely the presence of a lock. If another flag is checked instead, verify why it guarantees that the original action's precondition still holds. ```` ## File: skills/code-review/references/output-schema.md size: 9741 sha: d7898b43b38cc26aa29f347fe7f16b34ea056a80 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/output-schema.md ````text # Probe output contract (findings) This file defines the finding fields, severity levels, handling statuses, and probe execution statuses, for use by probes when they write standalone JSON. A finding is one specific problem; `severity` indicates how serious it is, a finding's `status` indicates handling progress, and the probe status indicates the execution result of one round of checks. ## 1. How to write a single issue | Field | Requirement | Value | Description | |---|---|---|---| | `id` | Required | `-` | Globally unique, number starts at 1, **must be stable** | | `probe` | Required | Probe name | Probe name taken from `references/probes.yaml` | | `severity` | Required | `high` / `medium` / `low` | See the criteria table below | | `status` | Required | `open` / `reconciled` / `wontfix` / `false-positive` | See the state machine below | | `location` | Required | `path:line` | When no line can be located, use the file level and explain why | | `summary` | Required | Title and brief explanation | First line: short title. Subsequent lines: trigger, actual versus expected behavior, supported impact, and fix suggestion; do not repeat detailed evidence | | `evidence` | Required | Concrete basis | Cite a rule ID / interface contract / code path / test output. **Anything without evidence is not a finding** | | `repro` | Optional | Verification command | E.g. `grep -n 'formatCents' src/…/Calculator.java`, `git show :`. Read-only evidence commands only; must not build or run tests | | `reconciled_by` | Conditionally required | Handling note | Fill in when `status=reconciled`: where it was changed and why | | `reconcile_commit` | Optional | commit id | Fill in when a corresponding handling commit exists | Optional fields not provided and conditional fields that do not apply may be omitted; empty fields need not be filled. `summary` remains a string; use JSON `\n` for line breaks. Legacy single-line records still render. Evidence is preserved in full at the end of each finding, collapsed by default. Keep static-reasoning status, unverified assumptions, and contract disputes visible in the explanation, not only in hidden evidence. Each probe saves this round's records as a standalone `probes/.json`. For example: ```json { "probe": "test-coverage", "status": "failed", "duration": 12, "findings": [{ "id": "test-coverage-1", "probe": "test-coverage", "severity": "high", "status": "open", "location": "src/main/java/com/example/Calculator.java:137", "summary": "Incremental line coverage is 60%, below the 85% baseline", "evidence": "Report matches the target version; 3 of 5 executable changed lines in production code are covered, 3/5 = 60%" }] } ``` `probe`, `status`, and `findings` are required; `duration` is a non-negative integer in seconds and may be omitted, with omission recorded as 0. `skipped` / `error` must also include a non-empty `reason`; when there are no issues, explicitly write `findings: []` rather than inventing empty records. Checked scope, general execution limitations, and additional suggestions go in the human report; each finding's explanation and fix suggestion go in `summary`. The machine JSON is the single source of truth; do not copy full findings or hand-fill counts elsewhere. **Pending confirmation**: when a specific concern has been identified but a contract, reachability, or evidence gap prevents confirming a defect, save it in the probe's optional top-level `unverified_items` array rather than only in the return message. Each item contains only these four non-empty strings: | Field | Content | |---|---| | `location` | Source location, preferably `path:line` | | `summary` | Title, observed behavior, and possible consequences; distinguish facts from inference | | `reason` | The specific reason confirmation is pending; for contract conflicts, cite both sides | | `needed_evidence` | Evidence or a product decision needed to resolve the concern | Omit the field or use `[]` when there are no such concerns; legacy JSON remains valid. Do not turn general limitations such as tests not run into individual pending items, or duplicate a concern already explained in a finding. Proven defects still belong in `findings`; this field must not bypass grading. The script displays these items beside high / medium / low in the report's Findings section, labeled "Pending confirmation", with their source probe. They have no severity or handling status, do not enter defect counts or reconciliation, and do not directly change probe status or gates; malformed records still fail validation. ## 2. Issue severity | Level | Criterion | Enters reconciliation | |---|---|---| | `high` | Serious security rule violation / money or data correctness issue / HIGH-severity standards rule violation / contract break / measured diff line coverage below the effective project target (default 85%) / acceptance item failing | **Yes** | | `medium` | Confirmed behavioral defects that deviate from expected behavior, have localized impact, and do not meet the high criteria; maintainability issues, missing tests | No (record only) | | `low` | Style, naming | No | Only the three levels above are accepted; other levels are an error and are not auto-converted or used to rewrite historical records. ### Behavioral issues **1. Verify the basis** To report `high` on the grounds of violating a method contract, `evidence` must include the full signature of the actual method, the file and line number of the corresponding interface declaration (or the method declaration if there is no interface), and a short excerpt of the contract text. Same-name overloads or similar interfaces cannot substitute. When there is confirmed to be no textual contract, you may cite specific code invariants or tests and their locations; not having read something cannot be treated as it not existing. When the basis has not been verified or listed, the maximum is `medium`, noting "contract not verified". **2. Grade by proven errors** - `high`: at public contract boundaries such as API / SPI / cache / state machine, an incorrect return, exception, or missed invalidation occurs under reachable inputs. Source-code reasoning may serve as evidence; end-to-end execution is not required. - Maximum `medium`: the boundary error is not yet proven and downstream impact is only speculated; or the performance issue is unmeasured. Do not omit a proven behavioral defect solely because performance is unmeasured or downstream effects are unverified. When a boundary error is proven, unverified downstream UI / templates / integration layers are recorded only in `evidence` and do not lower the severity. **3. Handling contract conflicts** When contracts, docs, or tests conflict, list both sides' bases in `evidence`. Only evidence that applies to the target version and explains the conflict can resolve it; "probably a typo / copy-paste / old comment" does not count as evidence. If the judgment depends on an unresolved conflict, save it in `unverified_items`; if kept as a finding instead, do not duplicate it, the maximum is `medium`, and the summary notes "contract to be confirmed". Other issues with independent, sufficient evidence are graded as usual. `unverified` is not a finding severity or status. ### Combined risk and test gaps - **Combined risk**: when multiple distinct issues interact and evidence proves they jointly cause a more serious consequence, grade by the combined impact and explain the interaction path. - **Contract changes and test gaps**: when a corresponding test is confirmed missing, record the test gap; do not automatically escalate it to `high`. When evidence already proves that a contract change causes a breaking impact, grade per the severity criteria above. When test context is not provided or is incomplete, note "relevant tests unverified", and do not assert that no tests exist on that basis. ## 3. Issue handling statuses ``` open ──┬─→ reconciled Handled (code changed / tests added) ├─→ wontfix Explicitly not fixing (**must give a reason**, and severity must be medium/low) └─→ false-positive Probe false positive (**must give evidence** explaining why it is not a problem) ``` ### Re-running after handling After a high finding is handled, re-run the corresponding probe and report the result; do not directly hand-edit it to `passed`. The re-run must state what changed in the code or the basis for judgment; execution status follows the latest result, and historical findings and handling records are retained. ## 4. Probe execution results `requires` lists the inputs a probe needs, checked against this round's provided materials and default configuration. Probe execution status is reported separately from the handling status of individual findings: | Status | Meaning | |---|---| | `passed` | The relevant checks completed with no execution errors and no open high findings | | `failed` | The relevant checks completed with no execution errors but open high findings | | `skipped` | **Normally skipped per the probe's convention**, e.g. this round did not provide the optional input required by `requires`; the reason must be stated. **Not counted as a failure** | | `error` | A specified input path does not exist, cannot be read or parsed, or a command / tool failed. Only when `required=true` does it count toward `PROBE_REQUIRED_FAILED`, same as `failed` | When a specified coverage report path cannot be read, record it as `error`. Missing or corrupted resources pointed to by the default configuration likewise count as `error` and must not be disguised as a normal skip. ```` ## File: skills/code-review/references/probes.yaml size: 1318 sha: d4f4af3786d319b2ff337d495b1386e4370270c1 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/probes.yaml ```text # code-review probe registry: maintains the probe list, dependencies, and parameters. # See section 4 of references/output-schema.md for the meaning of required / requires and execution status. version: 1 execution: mode: parallel max_retry: 1 # Maximum re-dispatches when reconciliation finds a gap # requires lists input dependencies; rules uses built-in rules by default, and coverage is optionally handled internally by test-coverage. probes: - name: behavior-review title: Behavioral correctness file: references/probes/behavior-review.md required: true requires: [] - name: test-coverage title: Test quality and incremental coverage file: references/probes/test-coverage.md required: true requires: [] params: line_target: 85 branch_target: 80 - name: repo-guard title: Repository standards file: references/probes/repo-guard.md required: true requires: [rules] params: read_only: true # Rule pack selection priority: see probes/repo-guard.md. rules_dir: references/rules - name: impact-analysis title: Impact analysis file: references/probes/impact-analysis.md required: false requires: [] params: depth: 1 # Caller trace depth (number of fan-in levels) ``` ## File: skills/code-review/references/probes/behavior-review.md size: 10002 sha: fce0a851aa7fd7e3a6bd390d4f92bcde8a60212b upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/probes/behavior-review.md ```text # probe: behavior-review (behavioral correctness) **required: true** | **requires: []** Check behavioral defects introduced or triggered by this change. The review scope and source version follow "Determine the review scope" in `SKILL.md`; it does not depend on rule packs and is not limited by the language extensions scanned for rules. Trace actual call chains as needed, unconstrained by the single-level fan-in of `impact-analysis`; not finding a caller does not by itself prove the internal logic correct or incorrect. ## 1. Evidence-gathering method Identify changed behavior entry points or branches from the diff, group them into chains by related state or objects, and gather evidence and judge each chain. Reuse evidence across related branches, but assess different triggers and consequences separately; after finding an issue, continue checking the remaining changed paths. Cite concrete source locations without presuming defects or inventing business rules. 1. **Confirm the change**: read relevant entry points and implementations to verify the actual method signatures, existing contracts, and behavior before and after the change. Expected behavior may be based on interface contracts, design, tests, or explicit code invariants. For changes to validation ranges, defaults, or branches, separately verify whether newly accepted or rejected inputs match the change's intent. Do not treat existing tests as outdated merely because the implementation changed; record specific conflicts when the supporting evidence remains unresolved. 2. **Trace state chains**: first establish the contracts and read uses of key states involved in this change. Then trace each write entry point through "entry point → state reads and writes → guard → subsequent action", comparing trigger timing and separately judging whether state writes, subsequent admission, and side effects respect those contracts. Assess completion, exception, and cancellation entry points sharing a wrapper separately; their judgments are not interchangeable. For concurrency or async behavior, verify actual executing threads, scheduling boundaries, synchronous versus queued callbacks, and task termination ordering. If a state contract cannot be established, save the specific dispute in `unverified_items` under the output contract. 3. **Fill specific gaps**: read more only for a concrete gap that could change the judgment, first identifying the question and likely evidence source. For decisive library or framework calls, verify the target version's declaration, contract, or implementation. For model or cache reads, follow object acquisition and wrapping to confirm the actual implementation and relevant overrides; do not infer behavior from method names, same-name overloads, or default implementations. Search Git history only when a concrete historical lead could change the judgment. When a contract conflict cannot be resolved and there is no new lead, stop gathering more evidence, record it as unverified under the output contract, and state both sides' evidence and the behavioral consequences under each interpretation; do not rule out the issue by accepting only one side. 4. **Complete the judgment**: record each path as "confirmed issue", "ruled out with evidence", or "unverified with a specific gap". Exclusion conclusions cover only the consequences verified by their evidence. Assess other contracts, state meanings, and admission conditions on the same path separately; ruling out one risk does not rule out the entire path. For unverified paths, identify which step in the trigger, ordering, or consequence the gap affects. Distinguish "the supplied implementation supports the path" from "the target build version is confirmed"; retain unverified status when a version gap determines reachability, without dismissing existing evidence wholesale. Move to the next chain when complete, revisiting only when new evidence or a concrete counterexample could change the judgment. ## 2. Behavioral checks For the main paths affected by this change, check with the entry points, callers, and context in mind: - Do conditional branches behave as expected, and are there inverted or missing branches? When routing changes, compare the actual paths for the same input before and after the change, and include pre-existing defects triggered by the new path in this round's checks. - Are state transitions allowed, and do side effects run under the correct conditions? - Are nulls, empty collections, and numeric boundaries handled correctly? - After an exception or partial failure, are state and resources handled correctly? For sequences involving databases, caches, messages, or external calls, check whether partial success, timeouts, or retries lead to inconsistent state or repeated side effects that violate the contract. Choose applicable check directions and concrete counterexamples for each behavior chain, and reason through them: - **Parsing / validation**: first confirm the input language and dependency versions, and proactively read the corresponding syntax or format contract; the current sample alone cannot stand for valid input. Choose valid equivalent forms per the contract, and check consistency along "parse → validate → use" and whether validation can be bypassed. - **Serialization / template rendering**: follow the call chain to read the actual wrapping, rendering, and parsing implementations, and compare with the related old entry points. Trace variable values along the actual conversion order, choose inputs such as quotes, backslashes, or newlines, and check whether escaping or decoding changes the original value or breaks the structure. **Concurrency and async check matrix**: when shared state, retries, or async tasks are involved, select applicable rows from the code signals in this change; a behavior chain may match multiple rows. Read the applicable [interleaving examples](../concurrency-examples.md), map the actors, state, and interleaving points to actual source, and verify reachability and protection mechanisms; do not copy the example conclusions. | Type and code signal | Core risk | Basis for exclusion | | --- | --- | --- | | Check-then-act: inspect a cache, state, or existence before acting | State changes before the action, including between separate critical sections | An atomic protocol preserves the action's precondition; rechecking another state must guarantee that precondition; individually locked accesses are insufficient | | Read-modify-write: read, compute, write back | Concurrent updates overwrite each other | A lock, atomic operation, or suitably isolated transaction protects the whole operation | | Visibility and publication: cross-thread initialization or readiness flags | Readiness is visible but data is incomplete or stale | Effective publication and visibility guarantees, or thread confinement | | Cancellation and task termination: callbacks release busy/loading | A new task enters while the old task or subtasks still run | Verify state-query contracts, cleanup ordering, and new-task admission separately; result isolation rules out result overwrites only and does not by itself justify releasing busy/loading; cancellation notification is not task termination | | Stale callbacks: old and new tasks share state | A late callback overwrites or clears new task state | Task identity/version checks, or effective ordering and isolation | | Repeated execution: retries, duplicate messages or triggers | An operation required once runs repeatedly | Atomic claims, deduplication, or idempotency cover state and observable side effects; equal results alone are insufficient; contractually permitted repetition is not a defect | | Deadlock: multiple locks or calls while holding a lock | Reverse lock ordering or circular waits | Lock ordering, avoiding waits while holding locks, or another guarantee prevents the cycle | For operation races, give a concrete interleaving and state changes; for visibility/publication, explain the synchronization and publication guarantees. Apply section 1's evidence and judgment method. Do not enumerate every thread combination, force findings, or output the matrix; reuse the existing "actual check scope". For other behavioral changes, choose counterexamples per the main paths. ## 3. Recording and reporting Check records uniformly state: **handling point location → trigger conditions and specific input → expected behavior and basis → relevant code path → actual behavior and consequence**. Once the main paths have these judgments, reuse the records to assemble and write the probe JSON. Do not routinely expand dependencies or recheck judged paths while finishing. If new evidence, a concrete counterexample, or contradictory evidence could change a conclusion, return to the relevant chain for evidence gathering. Save specific concerns that cannot yet be confirmed as defects in `unverified_items` under the output contract. In the return message, use the existing "actual check scope" to briefly state checked paths and other scope limitations; the return message does not replace JSON records. Confirmed issues go into the finding's `evidence`; grading and contract-conflict handling follow the [output contract](../output-schema.md). Record different root causes or consequences separately; do not merge them to evade grading. Save issues that depend on unresolved contract conflicts in `unverified_items` when no finding is created. When the syntax basis, the actual implementation path, or the reachability of a valid triggering input has not been verified, state the specific gap and do not treat it as a proven defect. Checks not completed or with insufficient evidence are listed as unverified items and limitations, not lumped into "checked"; when not run, mark "static reasoning" and do not call it reproduced. Behavioral findings do not require an existing rule ID and are not subject to `repo-guard`'s HIGH-only reporting limit. ``` ## File: skills/code-review/references/probes/impact-analysis.md size: 4518 sha: 01a3a7832fede208d9afa34d375acd21e0781888 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/probes/impact-analysis.md ```text # probe: impact-analysis (impact analysis) **required: false** | **requires: []** Check this change's impact on callers, contracts, performance, and architecture. ## 1. Inbound callers of changed methods (fan-in) For methods in this diff whose **signature or caller-visible behavior changed**, trace callers one by one: Use the review scope and target version determined by the main flow. Method declaration changes are only used to help locate; also check changes in return-value meaning, exceptions, and side effects caused by method body changes. Call forms differ by language and must be confirmed against the actual code. For methods involving the above changes, answer: | Question | Why ask | |---|---| | Who calls it? (how many) | The larger the fan-in, the larger the blast radius of the change | | Are callers **outside this repository**? | Cross-repository callers cannot change along with it, so the contract must stay compatible | | Do callers also need changes? | When callers need adaptation but were not changed, describe the specific call path, trigger conditions, and consequences | | Is an overload with the same name affected by mistake? | Overload resolution changes are a classic silent break | `depth=1` (registry parameter): by default, trace only **one level** of callers. ## 2. Contract changes - Were fields added, removed, or changed in external interfaces (HTTP / RPC / MQ message bodies)? - Is the database schema change **backward compatible**? (nullable column added vs type change vs column dropped) - Did config file / environment variable key names change? (multi-environment matrix, see the design doc) - Did the serialization format change? (JSON field names, time format) First confirm whether the change breaks an existing contract, and describe the affected call forms, data, or deployment conditions and the specific consequences, then grade per the severity criteria in `references/output-schema.md`. A compatible change is not graded high merely because no separate compatibility strategy was written. ## 3. Performance Perform a static performance check on the paths involved in this change; it does not depend on whether a design doc is provided. Focus areas: newly added in-loop queries / N+1, full table scans, unbounded collections, synchronous blocking calls, cache breakdown, and batch sizes for bulk operations. When caches or proxies are involved, trace to the actual I/O boundary; without confirming database access, do not equate the number of method calls with the number of new SQL queries. If the design doc has performance targets, compare them against the relevant implementation, and judge whether targets are met only when corresponding measured data exists. When design targets or measured data are missing, record the verification limitation and do not fail it merely for missing material; when targets exist but no measured data does, note "performance targets unverified" and do not claim they are met based on code inspection alone. This probe does not run load tests automatically. When a risk is found, describe the specific code path, the applicable data scale or call conditions, and the possible consequences. ## 4. Architectural impact For modules and related call/dependency paths involved in this change, check against the project's existing architectural conventions: - Is unrelated responsibility crammed into the same module? - Are circular dependencies introduced, or the project's existing dependency direction broken? - After a public interface change, can existing implementations or callers still be replaced and used normally? - Do new abstractions and intermediate layers serve the current need? When a problem is found, cite the specific module, dependency, or call path, and describe the affected contract or constraint and the actual consequence. Do not rate it high risk merely for "not following some design principle", and do not force the project to adopt a fixed layering. Things that are only structural optimization suggestions without proven concrete breaking impact are recorded separately as non-blocking suggestions. ## 5. Anti-patterns - ❌ Reporting high just because a method name changed, without confirming whether there are callers — that is noise and drowns out real problems - ❌ Checking only in-repository callers and ignoring **external** consumers of external interfaces - ❌ Treating "someone could theoretically call it" as a finding (it must be **specific** about which caller) ``` ## File: skills/code-review/references/probes/repo-guard.md size: 2908 sha: 3a87458d299363fbd5927aed16684fbe428013f0 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/probes/repo-guard.md ```text # probe: repo-guard (repository standards) **required: true** | **requires: rules** | `read_only: true` ## 1. Select the rule pack 1. When `$CODE_REVIEW_RULES_DIR` is non-empty, use the specified path; if the path does not exist or is unreadable, record `error` and do not fall back. 2. When unspecified, use `/.code-review/rules/` if that directory exists; otherwise use the skill's built-in `references/rules/`. First read the selected rule pack's `index.md` to learn the rule topics. A project-provided rule pack **entirely replaces** the built-in one; they are not merged. ## 2. Rule scanning and candidate review The review scope and code version follow "Determine the review scope" in the main flow's `SKILL.md`. 1. **Select files**: choose `.java` / `.ts` / `.tsx` / `.vue` from the change list, including added files and excluding deleted files; when no applicable files exist, go to section 3. 2. **Run the scan**: pass the file paths one by one as independent arguments to `bash "$CODE_REVIEW_SKILL_DIR/scripts/rule-grep"`. The script scans the full file text, and hits may include old code. 3. **AI review**: with the corresponding rule, code context, and this round's diff, confirm whether a violation actually exists. Include only issues introduced or triggered by this change in the findings; record unrelated old issues separately and exclude them from this round's counts. Rules that cannot be decided by pattern go to section 3. When on-disk files differ from the review target version, read the target version's source and perform the AI semantic check per section 3. The scan only provides candidates; a hit is not a violation, and a miss is not compliance. ## 3. On-demand loading and AI semantic checks An AI semantic check means the AI reads the rule text, the diff, and the relevant code context to judge whether the rule applies and whether the code actually violates it. Load the corresponding rule files on demand based on the index, project language, change topic, and scan hits. Prose rules that cannot be machine-judged but are relevant to the change must also be loaded; do not check only the scan hits. Default check topics include: - SQL injection - XSS - Authentication and permission isolation - Sensitive data handling - Hardcoded credentials - Idempotency The specific requirements follow the rule files selected this round. Compare each selected rule against this change, and note in the report the rule files actually loaded and the checked scope. Produce findings only for HIGH-severity rules. When a standards violation is found, record the **applicable rule ID, file, line number, and violation description**. For general behavioral correctness checks, see [behavior-review](behavior-review.md). ## 4. Judgment and reporting Record `error` when the rule pack directory does not exist or not a single rule file can be read. ``` ## File: skills/code-review/references/probes/test-coverage.md size: 6005 sha: ae028e4f5be1e4ec0f353defdd20f580a099b540 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/probes/test-coverage.md ```text # probe: test-coverage (test quality and incremental coverage) **required: true** | **requires: []** Each round, the AI checks the quality of the relevant tests; coverage is an optional sub-item. Tests need not be run, nor coverage measured, every round. ## 1. Inputs and boundaries - Locate the relevant test source and configuration for the target version per "Determine the review scope" in `SKILL.md`. - Coverage reports and measurement commands are both optional. First confirm whether the available material is a complete project or only a diff / partial source / minimal snapshot. With only partial material, you can only say "no relevant tests found in the current material"; the missing part is recorded as unverified, and you must not assert that the original project has no tests. ## 2. Test quality checks (every round) First compare against expected behavior to distinguish implementation regressions from problems in the tests themselves. When existing tests correctly reveal this regression, record their coverage and hand the behavioral suspicion and evidence back to the main executor; do not conclude that the tests are defective merely because they were not modified. Using the relevant tests and the code under test, check the tests themselves: - **Assertions**: do they verify real results and distinguish a correct implementation from the relevant errors, rather than merely executing code or unconditionally asserting `true`? For key changes, choose a plausible incorrect behavior (such as a wrong return value, an incorrect boundary condition, or a missing state update) and identify the specific test and assertion that would detect it. If none is found, assess whether the tests actually inspected have a scenario or assertion gap. Against expected behavior, check whether mocks bypass the logic under test, exceptions are swallowed, and important side effects such as state changes or call counts are verified; using mocks alone does not make a test ineffective. - **Scenarios**: do they cover the important boundaries and failure paths of this change; check retries and concurrency according to actual applicability. - **Stability**: do they depend on execution order, unstable time, or an uncontrollable external environment? - **Test changes**: were relevant tests deleted, skipped, commented out, or weakened? Judge with the reason and any substitute verification whether valid checks were lost; do not mark it high based merely on a deletion or a skip / pending marker. These checks are static judgments: do not modify the implementation or automatically run mutation testing, and do not present them as executed verification. Meeting coverage targets does not establish test effectiveness. Produce findings only for proven defects in the tests themselves or for scenario / assertion gaps, stating the location, expected-behavior basis, and risk; describe the gap within the actually checked test scope. When expected behavior is still under contract dispute, record an unverified item and do not presuppose assertion outcomes. Grade per `references/output-schema.md`; a missing test is not automatically high. ## 3. Optional coverage check and reporting Use effective `line_target` / `branch_target` handed off by init; project policy takes precedence, defaults are 85 / 80. Compare against these effective targets, not a separate fixed 85% baseline. 1. Prefer reusing an existing coverage report that corresponds to the review's target version and modules. 2. When no corresponding report exists, run measurement only if the project already has a measurement method and this round's task allows it; otherwise record "not measured" and still complete the test quality check. 3. With a valid report, compute the executable-line coverage of production source lines added or modified by this diff: `diff line coverage = covered executable changed lines / total executable changed lines × 100%`. The report must state the numerator, denominator, and result; do not use whole-file coverage; when processing JaCoCo XML, read the details at the end. | Coverage situation | Record and handling | |---|---| | Measured diff line coverage ≥ line_target | Meets target | | Measured diff line coverage < line_target | high finding, blocks merge | | Branch coverage | Target branch_target (default 80%); falling short is advisory only and does not block or lower scores | | Confirmed no executable incremental lines | Record coverage as "not applicable", explain why, and do not fill in 0% or 100% | | A specified report is unreadable, unparsable, incomplete, or version-mismatched, or the measurement command fails | `error`; must not be recorded as "not measured" or "not applicable" | Return the report path, target version, numerator/denominator, and percentage for valid coverage; the main executor reports them per `SKILL.md`. Do not fill unmeasured or inapplicable metrics with 0. ## 4. Reporting Record test quality, coverage status, and unverified items separately. `passed` does not mean the tests were run or coverage was measured. ## 5. JaCoCo report parsing details (read when processing XML) 1. Identify production and test source per the project source sets, extract added or modified lines of the target version's production source, and exclude deleted lines. 2. Match `` / `` by module, source path, and changed line numbers. `mi + ci > 0` counts toward the denominator, and `ci > 0` counts as covered. 3. Count each "module + source path + line number" only once; compute the ratio after aggregating, and do not average per-file percentages. 4. Record `error` when required data is missing; do not exclude it as non-executable or record it as "not applicable". This is [JaCoCo's line coverage definition](https://www.jacoco.org/jacoco/trunk/doc/counters.html): a line counts as covered if at least one instruction in it executed, which does not mean all branches on that line were covered. ``` ## File: skills/code-review/references/rules/baseline-java.md size: 2515 sha: a4c8d79add4a790c1c5b0abc7d6f07ba06c9814f upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/baseline-java.md ```text # Common baseline (Java) A minimal usable rule set covering three high-frequency categories: security, concurrency, and exceptions. Add or remove rules according to the project's actual stack; replacing this file does not affect the orchestration logic in `SKILL.md`. ## Security ### SEC-001 [HIGH] SQL string concatenation SQL must use parameter binding; user input must never be concatenated into statements. **Detection pattern** (any hit is a violation): - `Statement[^;]*execute(Query|Update)?\s*\(\s*["'][^"']*["']\s*\+` - `create(Query|NativeQuery)\s*\(\s*["'][^"']*["']\s*\+` - `["'][^"']*(SELECT|INSERT|UPDATE|DELETE)[^"']*["']\s*\+` ### SEC-002 [HIGH] Hard-coded credentials Passwords, keys, and tokens must not be written in source code. **Detection pattern**: - `(password|passwd|secret|token|apiKey|accessKey|privateKey)\s*=\s*"[^"]{6,}"` ### SEC-003 [HIGH] Dangerous XSS rendering Untrusted content must not be passed directly to HTML rendering. **Detection pattern**: - `v-html|dangerouslySetInnerHTML|innerHTML\s*=` ### SEC-004 [HIGH] Command execution Confirm that arguments do not come from user input before invoking a system command. **Detection pattern**: - `Runtime\.getRuntime\(\)\.exec\s*\(|new\s+ProcessBuilder\s*\(` ### AUTH-001 [HIGH] Missing authorization on queries **Detection mode**: semantic When querying user data by business ID, include the current user or tenant condition as well. **Detection pattern**: none (manual check: whether the query condition contains only orderId / id, with no userId / tenantId) ## Logging and data ### LOG-001 [HIGH] Sensitive fields in logs ID card numbers, phone numbers, bank card numbers, passwords, and tokens must not appear in logs in plaintext. **Detection pattern**: - `log(ger)?\.(info|debug|warn|error)\([^)]*(idCard|idNo|phone|mobile|bankCard|password|token)` ## Concurrency and exceptions ### CONC-001 [HIGH] Shared SimpleDateFormat `SimpleDateFormat` is not thread-safe and must not be used as a static shared instance. **Detection pattern**: - `static\s+(final\s+)?SimpleDateFormat` ### CONC-002 [MEDIUM] Thread pool without explicit bounds `Executors` factory methods can easily create unbounded queues or unlimited threads. **Detection pattern**: - `Executors\.(newCachedThreadPool|newFixedThreadPool)\s*\(` ### ERR-001 [HIGH] Empty catch swallows exceptions An empty catch block fails silently; it must at least log the exception or rethrow it. **Detection pattern**: - `catch\s*\([^)]*\)\s*\{\s*\}` ``` ## File: skills/code-review/references/rules/concurrency.md size: 4466 sha: b7cbf09e6610b6403e3d90f0be057d11e3f3cf51 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/concurrency.md ````text # Concurrency guidelines ## Thread pool creation **Detection mode**: semantic **Do not create thread pools with Executors (MULT-CONC-001):** ```java // Wrong: may cause OOM ExecutorService executor = Executors.newFixedThreadPool(10); // Correct: set all parameters explicitly ThreadPoolExecutor executor = new ThreadPoolExecutor( 10, // corePoolSize 20, // maximumPoolSize 60L, // keepAliveTime TimeUnit.SECONDS, new LinkedBlockingQueue<>(1000), new ThreadFactoryBuilder().setNameFormat("worker-%d").build(), new ThreadPoolExecutor.CallerRunsPolicy() ); ``` ## SimpleDateFormat thread safety **Detection mode**: semantic ```java // Wrong: not thread-safe private static final SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd"); // Correct option 1: ThreadLocal private static final ThreadLocal sdf = ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd")); // Correct option 2: use Java 8 DateTimeFormatter (recommended) private static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd"); ``` ## Lock ordering **Detection mode**: semantic **Locking multiple resources must follow a consistent order to prevent deadlocks (MULT-CONC-002):** ```java // Wrong: may deadlock public void transfer(Account from, Account to, int amount) { synchronized (from) { synchronized (to) { from.debit(amount); to.credit(amount); } } } // Correct: lock in a fixed order public void transfer(Account from, Account to, int amount) { Account first = from.getId() < to.getId() ? from : to; Account second = from.getId() < to.getId() ? to : from; synchronized (first) { synchronized (second) { from.debit(amount); to.credit(amount); } } } ``` ## Concurrent modification **Detection mode**: semantic **Concurrent modification of the same record must use a lock or optimistic locking (MULT-CONC-003):** ```java // Optimistic locking approach @Update("UPDATE account SET balance = #{balance}, version = version + 1 WHERE id = #{id} AND version = #{version}") int updateWithVersion(Account account); // Retry logic (at least 3 times) int retry = 0; while (retry < 3) { Account account = accountDao.getById(id); int updated = accountDao.updateWithVersion(account); if (updated > 0) { break; } retry++; } ``` ## TOCTOU protection **Detection mode**: semantic **Check-Then-Act operations must be atomic (MULT-CONC-012):** ```java // Wrong: non-atomic operation if (account.getBalance() >= amount) { // Check account.debit(amount); // Act - the balance may have changed! } // Correct: use a database optimistic lock or distributed lock @Update("UPDATE account SET balance = balance - #{amount} WHERE id = #{id} AND balance >= #{amount}") int debitWithCheck(@Param("id") Long id, @Param("amount") Long amount); ``` ## High-concurrency optimization **Detection mode**: semantic **In high-concurrency code, do not use equality checks as interruption/exit conditions (MULT-CONC-004):** ```java // Wrong: may loop forever while (count == 0) { // may never exit } // Correct: use a range check while (count > 0) { // explicit exit condition } ``` ## Singleton thread safety **Detection mode**: semantic **Singleton objects must be thread-safe (MULT-CONC-005):** ```java // Double-checked locking (requires volatile) public class Singleton { private static volatile Singleton instance; private Singleton() { } public static Singleton getInstance() { if (instance == null) { synchronized (Singleton.class) { if (instance == null) { instance = new Singleton(); } } } return instance; } } ``` ## Thread resources **Detection mode**: semantic **Thread resources must be provided through thread pools; do not create threads explicitly (MULT-CONC-007):** ```java // Wrong new Thread(new Runnable() { @Override public void run() { } }).start(); // Correct executor.execute(() -> { // ... }); ``` ## Random in multithreaded code **Detection mode**: semantic **Avoid sharing Random across threads; on JDK 7+ use ThreadLocalRandom (MULT-CONC-011):** ```java // Wrong Random random = new Random(); int value = random.nextInt(100); // Correct int value = ThreadLocalRandom.current().nextInt(100); ``` ```` ## File: skills/code-review/references/rules/exception-logging.md size: 5060 sha: 217ba0b68841dea68ff10c8bd3954a466e1359a5 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/exception-logging.md ````text # Exception handling and logging guidelines ## Exception handling ### Exception catching **Detection mode**: semantic **Empty catch blocks are forbidden (MULT-EXC-001):** ```java // Wrong: swallows the exception try { doSomething(); } catch (Exception e) { // empty block } // Correct: at least log it try { doSomething(); } catch (Exception e) { log.error("Failed to do something", e); throw new BusinessException("operation failed", e); } ``` ### Transaction rollback **Detection mode**: semantic **Catching an exception in transactional code requires a manual rollback (MULT-EXC-002):** ```java @Transactional(rollbackFor = Exception.class) public void updateUser(User user) { try { userDao.update(user); userPhoneDao.update(user.getPhone()); } catch (Exception e) { log.error("failed to update user", e); TransactionAspectSupport.currentTransactionStatus().setRollbackOnly(); throw new BusinessException("update failed"); } } ``` ### finally rules **Detection mode**: semantic **Do not return from finally (MULT-EXC-003):** ```java // Wrong try { return 1; } finally { return 2; // Forbidden! Overrides the return in try } // Correct try { return 1; } finally { // Cleanup is allowed here, but no return } ``` ### Resource cleanup **Detection mode**: semantic **The finally block must close resources; on JDK 7+ use try-with-resources (MULT-EXC-004):** ```java // Correct try (InputStream is = new FileInputStream(file); OutputStream os = new FileOutputStream(output)) { // use the resource } catch (IOException e) { log.error("IO error", e); } ``` ### Pre-check principle **Detection mode**: semantic **Pre-checks are better than exception catching (MULT-EXC-005):** ```java // Wrong try { return user.getName().toUpperCase(); } catch (NullPointerException e) { return null; } // Correct if (user != null && user.getName() != null) { return user.getName().toUpperCase(); } return null; ``` ### Exception classification **Detection mode**: semantic **Distinguish stable code from unstable code (MULT-EXC-007):** ```java // Stable code: catch a specific exception try { userDao.insert(user); } catch (DuplicateKeyException e) { log.warn("Duplicate user: {}", user.getId()); throw new BusinessException("user already exists"); } // Unstable code: catch a broader exception try { thirdPartyService.call(); } catch (Exception e) { log.error("Third party call failed", e); throw new ServiceException("third-party service call failed", e); } ``` ### Cascading-call NPEs **Detection mode**: semantic **Cascading calls are prone to NPEs; use Optional (MULT-EXC-008):** ```java // Wrong String cityName = user.getAddress().getCity().getName(); // Correct String cityName = Optional.ofNullable(user) .map(User::getAddress) .map(Address::getCity) .map(City::getName) .orElse("unknown"); ``` ## Logging ### Logging framework **Detection mode**: semantic **Depend on the SLF4J facade; do not use Log4j/Logback APIs directly (MULT-LOG-140):** ```java // Wrong: use Log4j/Logback directly import org.apache.log4j.Logger; // Correct: use SLF4J import org.slf4j.Logger; import org.slf4j.LoggerFactory; private static final Logger log = LoggerFactory.getLogger(UserService.class); ``` ### Log content **Detection mode**: semantic **Exception logs must include context and the stack trace (MULT-LOG-141):** ```java // Correct log.error("params: {}, error: {}", userId, e.getMessage(), e); // Or use placeholders log.error("failed to update user, userId={}, error={}", userId, e.getMessage(), e); ``` ### Conditional output **Detection mode**: semantic **Use conditional output or placeholders for trace/debug/info (MULT-LOG-142):** ```java // Wrong: string concatenation log.debug("Processing user: " + userId); // Correct: placeholder log.debug("Processing user: {}", userId); // Or conditional output if (log.isDebugEnabled()) { log.debug("Processing user: {}", userId); } ``` ### Sensitive-data masking **Detection mode**: semantic **Logs must be masked (MULT-SEC-025/026):** ```java // Wrong: log sensitive information directly log.info("User login: phone={}, idCard={}", phone, idCard); // Correct: mask it log.info("User login: phone={}, idCard={}", SensitiveDataMasker.maskPhone(phone), SensitiveDataMasker.maskIdCard(idCard)); // Or use the masking helper log.info("User login: {}", SensitiveDataMasker.maskUserInfo(user)); ``` ### Log levels **Detection mode**: semantic **Disable debug in production; enable info selectively (MULT-LOG-146):** ```java // Wrong log.debug("Detailed info"); // Correct log.info("Operation info"); log.warn("Warning info"); log.error("Error info", e); ``` ### Duplicate logs **Detection mode**: semantic **Avoid duplicate log output; set additivity=false (MULT-LOG-143):** ```xml ``` ```` ## File: skills/code-review/references/rules/index.md size: 2053 sha: ae1f67f8fcba2e2d4ca8e4d342701991ec7e5a8a upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/index.md ````text # Rule pack This index lists the built-in rule files and their purposes; see the [repo-guard probe](../probes/repo-guard.md) for the loading and checking flow. ## Location priority 1. `$CODE_REVIEW_RULES_DIR` 2. Project root `.code-review/rules/` 3. Skill built-in: `references/rules/` A project-provided rule pack replaces the built-in pack entirely (it is not merged). `bash "$CODE_REVIEW_SKILL_DIR/scripts/rule-grep"` uses the same priority order. ## Built-in files | File | Type | Content | |---|---|---| | `baseline-java.md` | Machine-checkable / semantic check | 9 baseline rules | | `concurrency.md` | Prose | Thread pools, SimpleDateFormat, lock ordering | | `exception-logging.md` | Prose | Exception catching, transaction rollback, logging | | `naming-format.md` | Prose | Naming and formatting | | `oop-collection.md` | Prose | Immutability, defensive programming, collections | | `spring-performance.md` | Prose | Spring / JPA / transactions / performance | | `testing-engineering.md` | Prose | Unit-test conventions | | `security.md` | Prose | Security baseline | ## Rule file format Each rule has one heading, followed by an optional `Detection pattern` line: ```markdown ### SEC-001 [HIGH] SQL string concatenation Why this rule matters. **Detection pattern** (any hit is a violation): - `regex-one` - `regex-two` ``` - The heading must be `### [] `; the severity is one of `HIGH` / `MEDIUM` / `LOW` - Backtick fragments in `Detection pattern` are extracted by `bash "$CODE_REVIEW_SKILL_DIR/scripts/rule-grep"` and run as regexes - Rules without a detection pattern do not participate in regex scanning - **The rule ID is the finding's evidence**; a judgment without an ID does not count - Legacy localized rule packs may use their own detection-marker label; `rule-grep` accepts the supported aliases ## Constraints - Only write your team's own rules; do not copy internal standards of unknown origin - Keep regexes as narrow as possible; prefer missing a hit over producing many false positives ```` ## File: skills/code-review/references/rules/naming-format.md size: 5525 sha: e5679ae808611986b6cdcad1b0000b3e6c0b6dc7 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/naming-format.md ````text # Naming and formatting guidelines ## Naming conventions ### Basic naming rules **Detection mode**: semantic | Type | Rule ID | Convention | Example | Forbidden example | |------|--------|------|------|---------| | Class name | MULT-NAME-003 | UpperCamelCase | `UserDO`, `HtmlUtil`, `OrderService` | `userDo`, `htmlutil` | | Method name | MULT-NAME-004 | lowerCamelCase | `getUserById()`, `calculateTotal()` | `GetUserById()`, `calculate_total()` | | Constant | MULT-CONST-005 | UPPER_SNAKE_CASE | `MAX_RETRY_COUNT`, `DEFAULT_TIMEOUT` | `maxRetryCount`, `Max_Retry_Count` | | Package name | MULT-NAME-009 | all lowercase, singular | `com.example.user` | `com.example.Users`, `com.example.userMgmt` | | Variable | MULT-NAME-004 | lowerCamelCase | `userName`, `orderList` | `user_name`, `UserName` | | Array | MULT-NAME-007 | `String[] args` | `String[] args` | `String args[]` | ### Special naming rules **Detection mode**: semantic | Rule ID | Rule | Description | Example | |--------|------|------|------| | MULT-NAME-006 | Abstract class | Must start with Abstract or Base | `AbstractProcessor`, `BaseServiceImpl` | | MULT-NAME-006 | Exception class | Must end with Exception | `UserNotFoundException`, `InvalidParameterException` | | MULT-NAME-006 | Test class | Name of the class under test + Test | `UserServiceTest`, `OrderControllerTest` | | MULT-NAME-011 | Interface implementation | Impl suffix | `UserServiceImpl`, `OrderServiceImpl` | | MULT-NAME-017 | Domain model | DO/DTO/VO/BO suffix | `UserDO`, `UserDTO`, `UserVO`, `UserBO` | | MULT-NAME-013 | Enum class | Enum suffix | `StatusEnum`, `TypeEnum` | ### Boolean variable naming **Detection mode**: semantic **Boolean properties in POJOs must not use the is prefix (MULT-NAME-008):** ```java // Wrong: causes serialization problems public class User { private Boolean isSuccess; // Forbidden! private boolean isActive; // Forbidden! } // Correct public class User { private Boolean success; private boolean active; // getter methods may use the is prefix public boolean isSuccess() { return success; } } ``` ### Method naming conventions **Detection mode**: semantic | Operation | Rule ID | Naming prefix | Example | |---------|--------|---------|------| | Get one | MULT-NAME-014 | get | `getUserById()`, `getOrderByNo()` | | Get many | MULT-NAME-014 | list | `listUsersByStatus()`, `listOrdersByUser()` | | Get count | MULT-NAME-014 | count | `countActiveUsers()`, `countOrders()` | | Insert | MULT-NAME-014 | save/insert | `saveUser()`, `insertOrder()` | | Delete | MULT-NAME-014 | remove/delete | `removeUser()`, `deleteOrder()` | | Update | MULT-NAME-014 | update | `updateUserStatus()`, `updateOrderAmount()` | | Predicate | - | is/has/can | `isActive()`, `hasPermission()`, `canDelete()` | ### Constant conventions **Detection mode**: semantic | Rule ID | Rule | Description | |--------|------|------| | MULT-CONST-001 | No magic values | Undefined constants must not appear directly | | MULT-CONST-002 | Use uppercase L for long literals | `long amount = 2L` (lowercase l is forbidden) | | MULT-CONST-003 | Group constants by function | Do not keep all constants in one class | | MULT-CONST-004 | Use enum for fixed ranges | Use an enum when a variable's value changes only within a fixed range | ## Code formatting conventions ### Indentation and spacing **Detection mode**: semantic | Rule ID | Rule | Description | |--------|------|------| | MULT-FMT-001 | Indentation | 4 spaces; tab characters are forbidden | | MULT-FMT-002 | Line length | No more than 120 characters | | MULT-FMT-181 | Operator spacing | One space on each side: `a + b`, `a == b` | | MULT-FMT-183 | Comma spacing | One space after a comma: `func(a, b, c)` | | MULT-FMT-181 | Parenthesis spacing | One space between if/for/while and the parenthesis: `if (condition)` | | MULT-FMT-184 | File encoding | UTF-8, Unix line endings | ### Brace rules **Detection mode**: semantic ```java // Correct: opening brace on the same line, body on the next line if (condition) { doSomething(); } else { doOther(); } // Wrong: opening brace on a new line if (condition) { doSomething(); } ``` ### Control statements **Detection mode**: semantic **All control statements must use braces, even for a single line (MULT-CTRL-133):** ```java // Wrong if (condition) doSomething(); // Correct if (condition) { doSomething(); } ``` **switch must have a default branch (MULT-CTRL-132):** ```java switch (status) { case Status.ACTIVE: return "active"; case Status.INACTIVE: return "inactive"; default: return "unknown"; } ``` **if-else nesting must not exceed 3 levels; use guard clauses beyond that (MULT-CTRL-134):** ```java // Wrong: nesting too deep if (user != null) { if (user.isActive()) { if (user.hasPermission()) { // ... } } } // Correct: guard clauses if (user == null) { return; } if (!user.isActive()) { return; } if (!user.hasPermission()) { return; } // Normal logic ``` ### Method length limit **Detection mode**: semantic **A single method must not exceed 80 lines in total (MULT-FMT-186):** ```java // Correct: single responsibility, manageable length public UserDTO convertToDTO(UserDO userDO) { if (userDO == null) { return null; } UserDTO dto = new UserDTO(); dto.setId(userDO.getId()); dto.setName(userDO.getName()); dto.setStatus(userDO.getStatus()); return dto; } ``` ```` ## File: skills/code-review/references/rules/oop-collection.md size: 7153 sha: 9d63d58f8a69f20156c807f4360c1746b33ab6ed upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/oop-collection.md ````text # OOP and collection guidelines ## Core principles ### Immutability principle **Detection mode**: semantic **Prefer immutable data structures and side-effect-free operations:** ```java // Wrong: mutable operation public List<User> processUsers(List<User> users) { List<User> results = new ArrayList<>(); for (User user : users) { user.setVerified(true); // Modifies the original object! results.add(user); } return results; } // Correct: immutable operation (create new objects) public List<User> processUsers(List<User> users) { return users.stream() .map(u -> { User copy = new User(u); // Create a copy copy.setVerified(true); return copy; }) .collect(Collectors.toList()); } ``` ### Defensive programming principle **Detection mode**: semantic **Every place that may be null must be null-checked:** ```java // Wrong: may throw NPE public String getUserName(User user) { return user.getName().toUpperCase(); } // Correct: use Optional public String getUserName(User user) { return Optional.ofNullable(user) .map(User::getName) .map(String::toUpperCase) .orElse(""); } // Or use Objects.equals (recommended for comparisons) public boolean isEqual(User u1, User u2) { return Objects.equals(u1, u2); } ``` ## OOP conventions ### Overridden methods **Detection mode**: semantic **All overridden methods must have the @Override annotation (MULT-OOP-001):** ```java // Correct @Override public boolean equals(Object obj) { // ... } @Override public String toString() { // ... } ``` ### Object comparison **Detection mode**: semantic **Wrapper objects must be compared with equals (MULT-OOP-003):** ```java // Wrong: may compare references Integer a = 128; Integer b = 128; if (a == b) { } // false! // Correct if (a.equals(b)) { } // Safer: use Objects.equals (recommended) if (Objects.equals(a, b)) { } ``` **Call equals on a constant or an object known to be non-null (MULT-OOP-002):** ```java // Wrong: may throw NPE if (user.getName().equals("admin")) { } // Correct if ("admin".equals(user.getName())) { } // Or use Objects.equals if (Objects.equals("admin", user.getName())) { } ``` ### POJO conventions **Detection mode**: semantic ```java // Correct POJO class definition public class UserDTO { // Use wrapper types (MULT-OOP-004) private Long id; private String name; private Integer age; private Boolean active; // Do not use the is prefix (MULT-NAME-008) // Do not set default values (MULT-OOP-006) // private Integer status = 0; // Forbidden // Must have a toString method (MULT-OOP-011) @Override public String toString() { return "UserDTO{id=" + id + ", name='" + name + "'}"; } } ``` ### Access control **Detection mode**: semantic **Use the most restrictive access level: private > protected > default > public (MULT-OOP-121):** ```java // Utility classes must not have public constructors public class StringUtils { private StringUtils() { throw new UnsupportedOperationException("Utility class"); } public static boolean isEmpty(String str) { return str == null || str.isEmpty(); } } ``` ### Serialization conventions **Detection mode**: semantic **Do not change serialVersionUID when adding fields to a serializable class (MULT-OOP-007):** ```java // Correct private static final long serialVersionUID = 1L; ``` ### Constructor conventions **Detection mode**: semantic **Constructors must not contain business logic; put initialization in an init method (MULT-OOP-008):** ```java // Wrong: constructor contains business logic public UserService() { loadConfig(); // Forbidden initCache(); // Forbidden } // Correct public UserService() { // Initialize only necessary properties } @PostConstruct public void init() { loadConfig(); initCache(); } ``` ## Collection guidelines ### Collection initialization **Detection mode**: semantic **Specify the initial capacity to avoid resizing (MULT-COLL-130):** ```java // Correct List<String> list = new ArrayList<>(expectedSize); Map<String, Object> map = new HashMap<>(expectedSize); // Calculation formula int initialCapacity = (int) (expectedSize / 0.75) + 1; ``` ### equals/hashCode **Detection mode**: semantic **Overriding equals requires overriding hashCode (MULT-COLL-122):** ```java // Correct @Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; User user = (User) o; return Objects.equals(id, user.id); } @Override public int hashCode() { return Objects.hash(id); } ``` ### Collection iteration **Detection mode**: semantic **Do not remove/add inside foreach (MULT-COLL-123):** ```java // Wrong: ConcurrentModificationException for (String item : list) { if (condition) { list.remove(item); // Forbidden! } } // Correct: use Iterator Iterator<String> iter = list.iterator(); while (iter.hasNext()) { String item = iter.next(); if (condition) { iter.remove(); } } // Or use removeIf (Java 8+), recommended list.removeIf(item -> condition); ``` ### Arrays.asList **Detection mode**: semantic **The collection returned by Arrays.asList() must not be add/remove/clear'd (MULT-COLL-125):** ```java // Wrong List<String> list = Arrays.asList("a", "b"); list.add("c"); // throws UnsupportedOperationException // Correct: create a new list List<String> list = new ArrayList<>(Arrays.asList("a", "b")); list.add("c"); ``` ### subList caveats **Detection mode**: semantic **subList is a view of the original collection; modifying the original causes ConcurrentModificationException (MULT-COLL-126):** ```java // Wrong List<String> list = new ArrayList<>(Arrays.asList("a", "b", "c")); List<String> sub = list.subList(0, 2); list.add("d"); // causes sub iteration to fail // Correct: create an independent copy List<String> sub = new ArrayList<>(list.subList(0, 2)); ``` ### Collection to array **Detection mode**: semantic ```java // Wrong String[] arr = list.toArray(); // returns Object[] // Correct String[] arr = list.toArray(new String[0]); ``` ### Map iteration **Detection mode**: semantic ```java // Recommended: use entrySet (MULT-COLL-131) for (Map.Entry<String, Object> entry : map.entrySet()) { String key = entry.getKey(); Object value = entry.getValue(); } // Java 8+: use forEach map.forEach((key, value) -> { // ... }); ``` ### Empty collection checks **Detection mode**: semantic ```java // Wrong if (list.size() == 0) { } // Correct if (list.isEmpty()) { } // Safe empty check (recommended) if (CollectionUtils.isEmpty(list)) { } ``` ### ConcurrentHashMap **Detection mode**: semantic **ConcurrentHashMap does not allow null keys or values (MULT-COLL-129):** ```java // Wrong ConcurrentHashMap<String, Object> map = new ConcurrentHashMap<>(); map.put("key", null); // Forbidden // Correct: use HashMap (nulls allowed) Map<String, Object> map = new HashMap<>(); map.put("key", null); ``` ```` ## File: skills/code-review/references/rules/security.md size: 3514 sha: 0333ba2ad130cab625448cb18e722224b63947f8 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/security.md ```text # Security guidelines ## HIGH security rules **Detection mode**: semantic | Rule ID | Rule | Description | Wrong example | Correct example | |--------|------|------|---------|---------| | SEC-101 | SQL parameter binding | String-concatenated SQL is forbidden | `"SELECT * FROM users WHERE id = " + userId` | `"SELECT * FROM users WHERE id = ?"` + PreparedStatement | | SEC-102 | No hard-coded credentials | API keys / passwords / tokens must not be hard-coded | `String password = "123456"` | `System.getenv("DB_PASSWORD")` | | SEC-103 | Input validation | All user input must be validated | Use request parameters directly | Use `@Valid` / `@Validated` | | SEC-104 | Sensitive-data masking | Logs must not print sensitive information | `log.info("user: {}", user)` | `log.info("phone: {}", SensitiveDataMasker.maskPhone(phone))` | | SEC-105 | CSRF protection | State-changing endpoints must verify CSRF | Direct POST | Add CSRF token verification | | SEC-106 | Permission checks | All user-data endpoints must enforce authorization | Return data directly | Verify that userId matches | | SEC-107 | XSS protection | v-html / dangerouslySetInnerHTML are forbidden | `<div v-html="content">` | Use a safe rendering method | ## Authentication and authorization **Detection mode**: semantic | Rule ID | Rule | Description | |--------|------|------| | AUTH-101 | Permission checks | All user-data endpoints must enforce authorization and prevent horizontal privilege escalation | | AUTH-102 | Identity verification | External systems use unified authentication; internal systems use SSO | | AUTH-103 | Token storage | Must not be placed in URL parameters; use Cookie / POST body | | AUTH-104 | Cookie security | HttpOnly=true, Secure=true (HTTPS) | | AUTH-105 | Do not trust the client | Do not use client-supplied userId / roleId directly for authorization | ## Data security **Detection mode**: semantic | Rule ID | Rule | Description | |--------|------|------| | DATA-101 | Sensitive-data masking | Phone numbers, ID numbers, bank card numbers, etc. must be masked in displays | | DATA-102 | Data classification | Decide whether to mask / watermark according to the project's data-classification standard | | DATA-103 | Number protection | Scenarios involving the real numbers of both parties must use the number-protection service | | DATA-104 | Query authorization | Do not query by business ID alone; always join userId / tenantId | ## API security **Detection mode**: semantic | Rule ID | Rule | Description | |--------|------|------| | API-101 | Replay protection | Platform resources such as SMS and payments must be replay-protected | | API-102 | Rate limiting | Endpoints with enumerable parameters must be rate-limited to prevent abuse | | API-103 | Real client IP | Use trusted proxy headers; do not trust X-Forwarded-For directly | | API-104 | CORS | Access-Control-Allow-Origin must not be `*` | ## Frontend security **Detection mode**: semantic | Rule ID | Rule | Description | |--------|------|------| | WEB-101 | XSS protection | v-html and dangerouslySetInnerHTML are forbidden | | WEB-102 | CSRF protection | Forms / AJAX must verify CSRF | | WEB-103 | Request methods | Non-query operations must not use GET | ## Key management **Detection mode**: semantic | Rule ID | Rule | Description | |--------|------|------| | KEY-101 | Key configuration | No hard-coding; use KMS / a key-management service | | KEY-102 | Private-key protection | Hard-coding `PRIVATE_KEY = "..."` is forbidden | ``` ## File: skills/code-review/references/rules/spring-performance.md size: 3551 sha: 5e721a8433e3a349ec579dbefecba5245b9e7d1c upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/spring-performance.md Content is not inlined in the catalog. Read the upstream file: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/spring-performance.md ## File: skills/code-review/references/rules/testing-engineering.md size: 5666 sha: b1397a18fa11270357a059d9045d37e9bedfad08 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/testing-engineering.md Content is not inlined in the catalog. Read the upstream file: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/references/rules/testing-engineering.md ## File: skills/code-review/assets/code-review.md size: 2382 sha: 46cefb65a20662b4dd4ce38fc4eb5af872658e22 upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/assets/code-review.md Content is not inlined in the catalog. Read the upstream file: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/assets/code-review.md ## File: skills/code-review/assets/report-labels.json size: 2194 sha: 3b7135e90ae372184ed2980f4dd0f78b352afa5d upstream: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/assets/report-labels.json Content is not inlined in the catalog. Read the upstream file: https://github.com/kyy-logs/code-review-skill/blob/main/skills/code-review/assets/report-labels.json ## File: README.md upstream: https://github.com/kyy-logs/code-review-skill/blob/main/README.md ````text # code-review-skill English | [简体中文](README.zh-CN.md) **Evidence-backed code review. Static-first, probe-driven, and gated.** The skill sources live in [`skills/code-review/`](skills/code-review/) (English) and [`skills/code-review-zh/`](skills/code-review-zh/) (Chinese), independently of any host's installation directory. The current stable release is `v0.1.1`; see [VERSION](VERSION). [![Claude Code](https://img.shields.io/badge/Claude%20Code-skill-6B4FBB?style=flat-square)](https://claude.com/claude-code) [![Agent Skills](https://img.shields.io/badge/Agent%20Skills-standard-1f6feb?style=flat-square)](https://agentskills.io) [![Python 3.9+](https://img.shields.io/badge/Python-3.9%2B-3776AB?style=flat-square&logo=python&logoColor=white)](https://www.python.org/) [![Static-first](https://img.shields.io/badge/review-static--first-2ea44f?style=flat-square)](#what-this-is-not) [![GitHub stars](https://img.shields.io/github/stars/kyy-logs/code-review-skill?style=flat-square&logo=github)](https://github.com/kyy-logs/code-review-skill/stargazers) Most AI reviews stop at “this looks risky.” This skill produces a review you can audit: every finding points to a location, a contract or code path, and a severity; every gate decision is rendered from machine-readable JSON. It reads the diff, surrounding source, tests, and project rules — no build, test run, or coverage report required. It never modifies the code under review. ## Latest news <!-- NEWS:START --> ### v0.1.1 (2026-11-05) #### Added - code-review: surface unverified items and tighten behavior checks - release: publish stable versions after validated VERSION changes #### Fixed - code-review: return immediately after saving probe JSON [Full update history](NEWS.md) <!-- NEWS:END --> ## Release status `v0.1.1` is a stable release within the scope described below. Pre-1.0 contract or format changes will be documented in [NEWS](NEWS.md). - **Severity is three levels:** `high`, `medium`, `low`. Records written by the earlier four-level version, including any `critical` value, are **not compatible**; `collect-metrics` rejects them. Re-run the review or migrate the records before using them with this release. - **Scope:** static-first review with four probes and JSON-derived report/gate output. It does not replace tests, CI, or human review. - **Validation:** 87 offline pipeline, NEWS, installation, bilingual, and locale tests; clean-project end-to-end gate runs; additional probe smokes. These are regression checks, not a general recall claim. Concurrency-defect detection has not been validated. For the next stable release, update `VERSION` (for example, to `0.1.1`) and push it to `main`. After Validate skills succeeds, the release workflow generates NEWS from commits, updates the README release information, and publishes the version tag and GitHub Release. Existing releases are preserved; failed CI does not publish. ## Contents - [Latest news](#latest-news) - [Release status](#release-status) - [Why this skill](#why-this-skill) - [Quick start](#quick-start) - [How it works](#how-it-works) - [Evidence contract](#evidence-contract) - [Configuration](#configuration) - [What this is not](#what-this-is-not) - [Validation](#validation) - [Requirements](#requirements) - [Repository layout](#repository-layout) - [Contributing](#contributing) - [License](#license) ## Why this skill **No self-grading.** The implementation context does not sign off on its own work. Four probes run in fresh contexts, each files its own evidence, and the gate is derived from those records. *This is separation of duties, not independent human adjudication; all probes still use the same model.* | | | |---|---| | **Static-first** | Works on a fresh clone or a partial checkout. Coverage is optional, not a prerequisite. | | **Evidence contract** | A finding without a location, summary, and checkable evidence is not a finding. | | **Four probes** | Behavior, tests, rules, and impact are separate questions with separate reports. | | **Machine-checkable gate** | Counts and blockers come from JSON, not from prose the model retyped. | | **Auditable output** | The human report and the JSON records are rendered from the same source and verified against each other. | ## Quick start ### Install Run this command **inside your project**: ```bash # English (default) npx -y skills add kyy-logs/code-review-skill --agent codex --skill code-review --yes # Chinese npx -y skills add kyy-logs/code-review-skill --agent codex --skill code-review-zh --yes ``` Use `--agent claude-code` for Claude Code. Codex installs to `.agents/skills/<skill>/`; Claude Code installs to `.claude/skills/<skill>/`. Node.js 22.20+ is required. Install one language per project. The Python installers also accept `--lang en|zh-CN` and default to English. ### Run Ask in Claude Code: ```text Review these changes with code-review. ``` Or invoke the skill directly: ```text /code-review ``` For a fixed range: ```text Use code-review on commits abc123..def456 ``` The skill supports two review modes: - `worktree` — staged, unstaged, and untracked changes against the current `HEAD`. - `commits` — a fixed `base..head` range. ### Read the result ```text docs/work/active/{slug}/ ├── code-review.md # human report └── probes/ ├── behavior-review.json ├── test-coverage.json ├── repo-guard.json └── impact-analysis.json ``` ## How it works <p align="center"> <img src="assets/code-review-architecture.svg" width="1160" alt="Four fresh-context probes run in parallel and write their own JSON; the main executor syncs, reconciles high findings, renders the report, and applies the gate."> </p> The four probes use the same model in fresh contexts. This is separation of duties, not independent human review. A passing gate is not human approval, CI, or proof that no issues remain. The current skill has no separate reviewer or finding-verification stage; raw findings feed the report and gate. 1. **Scope** — determine `worktree` or `commits`; record base and head as commit IDs. 2. **Probes** — dispatch four independent agents with the same scope and diff, each writing its own probe JSON. Run in batches if host capacity is limited, or record a serial fallback if agents are unavailable. Cross-probe duplicates are retained and counted; actual speed and token savings remain unverified. 3. **Sync** — validate probe JSON and derive counts with `collect-metrics`. 4. **Reconcile** — high findings must be closed or marked false-positive by the probe. 5. **Report** — render the human report’s fact sections from JSON, then verify them. 6. **Gate** — required-probe failures and open high findings block the merge. ### The four probes | Probe | Required | Question it answers | |---|---|---| | `behavior-review` | yes | Does the changed behavior match its actual contract and implementation path? | | `test-coverage` | yes | Are the tests meaningful, and is incremental coverage measured when data exists? | | `repo-guard` | yes | Does the change violate repository or project rules? | | `impact-analysis` | no | Who calls this, what contracts change, and what is the performance/architecture blast radius? | Probes are intentionally narrow. `behavior-review` is not a linter, and `repo-guard` is not a general bug scanner. ### Language and stack coverage The default `repo-guard` workflow selects `.java`, `.ts`, `.tsx`, and `.vue` files for pattern scanning. The built-in rule pack includes Java- and Spring-specific guidance; selecting an extension does not establish complete framework or language coverage. AI semantic checks can inspect relevant code beyond those extensions, but their results depend on the model and available contracts, source, and tests. The historical cases below exercise particular stacks and skill snapshots; they do not establish equally validated support across all stacks. Use applicable project rules and record unverified depende ````