From f57b42fd82d1d4c9b57764c8a36ddf5d8c94f0d7 Mon Sep 17 00:00:00 2001 From: zhanghongbin Date: Wed, 2 Sep 2026 20:02:10 +0800 Subject: [PATCH] =?UTF-8?q?fix(plan-review):=20=E5=AE=89=E5=85=A8=E9=98=80?= =?UTF-8?q?=E6=94=BE=E8=A1=8C=E5=90=8C=E6=AD=A5=E5=B8=83=E9=98=B2=20dispat?= =?UTF-8?q?ch-check=EF=BC=8C=E6=94=B9=E8=BF=87=E7=9A=84=20plan=20=E4=B8=8D?= =?UTF-8?q?=E5=86=8D=E5=85=8D=E5=AE=A1=20(v1.7.2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 问题 非 Critical 安全阀（CONCERNS 轮次耗尽后放行）有两处缺口： 1. 放行不布防。`.dispatch-.json` 只在引擎判 APPROVE 的分支写入， 安全阀分支直接 allow 并退出，dispatch-check 因此从不生效。本机 plan-review.log 显示 2026-09-02 当天 4 次放行全部走安全阀，即 dispatch-check 在实战中一次都没布防过。 2. 阀门那一轮不做任何检查。执行顺序是「读计数 → 安全阀 → 预检 → 引擎评 审」，第 3 轮之后对 plan 的任何改动在第 4 次 ExitPlanMode 时既不比对 hash、也不预检、也不评审，直接呈给用户，界面仍挂着 Red Team Review 标题。 ## 改动 - 把 APPROVE 分支里「parse manifest → 校验 → 落盘」抽成 arm_dispatch_state， 安全阀放行前同样调用。函数补两道落盘校验：目标已存在且不是普通文件直接判 失败（mv -f 对目录不会报错，会把临时文件移进去）；mv 之后再次 -f 与 dispatch_state_is_valid_v2。APPROVE 调用处保持 fail-silent；安全阀调用处 fail-closed：有 manifest 却布防失败 → deny，TOTAL_ROUNDS+1、ATTEMPT 不变。 - 新增 .review-hash-：CONCERNS 后原子写入本轮评审的 plan hash（同样带 「目标非普通文件」前置与「落成普通文件」后置校验，写失败只记日志不中断 hook），REJECT 与每个 cycle 结束点删除。安全阀只放行 hash 与之相同的修订； 不同或为空（含本版本之前遗留的计数状态、hash 写失败）一律落到预检与引擎 再评审一轮，不清计数。 - 超过上限后的补评审轮，CONCERNS 反馈标题改为「Round N, 补评审」并说明下一步， 不再显示「Round 4/3」「剩余轮次 -1」。 - README Consultation Flow / Dispatch Manifest v2 两段同步；版本 1.7.1 → 1.7.2。 ## 兼容 - .review-count- 仍是 ATTEMPT:TOTAL 两段，precompact-review.sh 的解析 不受影响；hash 另存文件。 - 升级前已积累 3 轮 CONCERNS 的会话，下一次 ExitPlanMode 会多评审一轮再放 行，而不是直接放行。 - dispatch-check.sh 未改动。 - 既有用例「counter: non-critical safety valve → allow JSON with ESCALATED」 的 setup 随语义改写：原本 set_counter_value 3 后一次调用即 ESCALATED，现在 先一轮 CONCERNS 产生 hash，第二次同一 plan 才 ESCALATED；断言目标未变。 ## 测试 - 新增 10 个 bats 用例：安全阀布防成功 / 无 manifest 不写状态 / 改过的 plan 再评审 / 旧状态先评审一轮 / CONCERNS 记录 hash / ack 放行清 hash / REJECT 重置不放行 / manifest 不合规 deny / 落盘目标为目录 deny / hash 路径不可写不破坏 hook 也不放行（并断言同名目录内不残留临时文件）。 其中「布防成功」「无 manifest」「manifest 不合规」「落盘目标为目录」四个 用例的 setup 预置了与 plan 匹配的 hash 文件：不预置时请求会先落到「再评审 一轮」分支，测不到安全阀自身的布防与失败分支。 - plan-review.bats 256 项：249 通过，7 个失败与 origin/main 基线 （246 项，239 通过）逐名相同；dispatch-check.bats 24/24。 - 黑盒：计数 3:3 + agent 行 manifest + 匹配 hash → allow，reason 含 ESCALATED，dispatch 状态落地 schema_version 2；hash 不匹配 → deny， 计数 4:4。 Claude-Session: https://claude.ai/code/session_01VFEQiWv8DrfKJj7XNHqEfL --- .claude-plugin/marketplace.json | 2 +- .../plan-review/.claude-plugin/plugin.json | 2 +- plugins/plan-review/README.md | 11 +- plugins/plan-review/scripts/lib/verdict.sh | 11 + plugins/plan-review/scripts/plan-review.sh | 207 ++++++++++++-- plugins/plan-review/tests/plan-review.bats | 260 ++++++++++++++++++ 6 files changed, 462 insertions(+), 31 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 96ba0c7..47be519 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -8,7 +8,7 @@ { "name": "plan-review", "description": "Adversarial plan review via cross-model consultation (agy/Claude/Codex)", - "version": "1.7.1", + "version": "1.7.2", "author": { "name": "WooDragon" }, diff --git a/plugins/plan-review/.claude-plugin/plugin.json b/plugins/plan-review/.claude-plugin/plugin.json index f1af6dd..5fbe82b 100644 --- a/plugins/plan-review/.claude-plugin/plugin.json +++ b/plugins/plan-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "plan-review", "description": "Adversarial plan review via cross-model consultation (agy/Claude/Codex)", - "version": "1.7.1", + "version": "1.7.2", "author": { "name": "woodragon" } diff --git a/plugins/plan-review/README.md b/plugins/plan-review/README.md index d531172..bdca856 100644 --- a/plugins/plan-review/README.md +++ b/plugins/plan-review/README.md @@ -129,8 +129,9 @@ is written, separate from `plan-review.log`. A plan with dispatch keywords must include a Dispatch Manifest with at least one `agent` row. A plan without dispatch keywords, including Tier0 work, remains -Manifest-free. `plan-review.sh` stores only the approved v2 signature set. The -fixed columns are `step | location | subagent_type | model_source | model | +Manifest-free. `plan-review.sh` stores only the approved v2 signature set — the +set is written on APPROVE and on safety-valve escalation alike. The fixed +columns are `step | location | subagent_type | model_source | model | depends_on | parallel_with`. - `main` rows use `-` for `subagent_type`, `model_source`, and `model`. @@ -151,7 +152,11 @@ fails open. ExitPlanMode → hook intercepts → engine reviews ├─ APPROVE → ack-deny: present review + re-call ExitPlanMode → allow (user's native go/no-go) ├─ CONCERNS/REJECT → deny + feedback → Claude revises → re-submit - └─ max rounds reached → allow through (user decides) + └─ max rounds reached and the plan is the exact revision the last CONCERNS round + reviewed → allow through (user decides). Dispatch Manifest signatures are armed + exactly as on APPROVE, and a Manifest that cannot be armed denies instead of + escalating. Any other revision — edited after the last round, or state predating + this mechanism — gets one more review before it can be escalated. ``` An engine APPROVE is **not** authorization to start work — it only clears the plan diff --git a/plugins/plan-review/scripts/lib/verdict.sh b/plugins/plan-review/scripts/lib/verdict.sh index 11da826..ff6ecf6 100644 --- a/plugins/plan-review/scripts/lib/verdict.sh +++ b/plugins/plan-review/scripts/lib/verdict.sh @@ -80,6 +80,17 @@ render_concerns_or_reject_feedback() { if [ "$verdict" = "REJECT" ]; then feedback_header="Red Team Review — ${review_engine} — REJECT (Round ${total_rounds}/${review_max_total_rounds})" phase_msg="审阅引擎发现 Critical 级别问题。非 Critical 磋商计数已重置，解决 Critical 项后可重新获得 ${review_max_rounds} 轮磋商机会。" + elif [ "$attempt" -gt "$review_max_rounds" ]; then + # attempt is already post-increment here (caller bumps ATTEMPT before + # calling this function — see plan-review.sh's CONCERNS branch), so + # attempt > review_max_rounds means the round that just ran was a + # post-cap supplementary review: the non-Critical valve (ATTEMPT >= + # REVIEW_MAX_ROUNDS) had already armed, but the plan hash changed since + # the last reviewed revision, so the valve fell through to one more real + # review instead of escalating unreviewed. "Round N/MAX" and a negative + # remaining-rounds count would misreport this as a normal in-budget round. + feedback_header="Red Team Review — ${review_engine} — CONCERNS (Round ${attempt}, 补评审)" + phase_msg="本轮是 plan 改动后的补评审，非 Critical 磋商已达上限。同一修订再次调用 ExitPlanMode 将直接呈现给用户做最终裁决；再改 plan 则会再评审一轮。" else remaining=$((review_max_rounds - attempt)) feedback_header="Red Team Review — ${review_engine} — CONCERNS (Round ${attempt}/${review_max_rounds})" diff --git a/plugins/plan-review/scripts/plan-review.sh b/plugins/plan-review/scripts/plan-review.sh index ee6a15e..996ea34 100755 --- a/plugins/plan-review/scripts/plan-review.sh +++ b/plugins/plan-review/scripts/plan-review.sh @@ -271,6 +271,14 @@ CONV_FILE="$COUNTER_DIR/.conversation-${SESSION_ID}" # clearing it (see that branch below) — that is the highest-value moment for # cross-round memory, not a cycle end. HISTORY_FILE="$COUNTER_DIR/.review-history-${SESSION_ID}" +# Plan hash of the most recent CONCERNS/REJECT-reviewed revision. The +# non-Critical safety valve (below) only escalates a plan whose hash matches +# this file's content — an edit made after the last review round must get +# one more review, never ride the valve on the strength of a stale hash. +# Written on CONCERNS (A.4), deleted on REJECT (Critical resets the whole +# non-Critical cycle) and at every cycle-ending cleanup point alongside +# CONV_FILE/HISTORY_FILE. +REVIEWED_HASH_FILE="$COUNTER_DIR/.review-hash-${SESSION_ID}" # Dispatch state is session-scoped, but stale files are global debris. Clean # them for every valid plan-review invocation, including Tier0 plans that have # no Manifest and therefore never enter the approval serializer branch. @@ -320,7 +328,7 @@ if [ -z "$PLAN" ] || [ "$PLAN" = "null" ]; then TRANSCRIPT_PATH=$(echo "$INPUT" | jq -r '.transcript_path // ""' 2>/dev/null || echo "") CWD_VAL=$(echo "$INPUT" | jq -r '.cwd // ""' 2>/dev/null || echo "") log_decision "decision=deny reason=no-plan-content-fail-closed resolve=${RESOLVE_REASON:-none} resolvePath=${RESOLVE_PATH:-empty} planFilePath=${PLAN_FILE_PATH:-empty} top_keys=${TOP_KEYS} tool_input_keys=${TI_KEYS} transcript_path=${TRANSCRIPT_PATH:-empty} cwd=${CWD_VAL:-empty}" - rm -f "$APPROVE_MARKER" "$COUNTER_FILE" "${CONV_FILE:-}" "${HISTORY_FILE:-}" + rm -f "$APPROVE_MARKER" "$COUNTER_FILE" "${CONV_FILE:-}" "${HISTORY_FILE:-}" "${REVIEWED_HASH_FILE:-}" # Error message routed by the resolver's RESOLVE_REASON (see # lib/plan-source.sh: plan_source_error_reason) — sets $REASON directly. plan_source_error_reason @@ -337,7 +345,7 @@ if [ -f "$APPROVE_MARKER" ]; then if [ -z "$APPROVED_HASH" ] || [ "$CURRENT_HASH" = "$APPROVED_HASH" ]; then # True ack-round: plan unchanged (empty marker = legacy format, unconditional allow) log_decision "decision=allow reason=ack-round-approved" - rm -f "$APPROVE_MARKER" "$COUNTER_FILE" "${CONV_FILE:-}" "${HISTORY_FILE:-}" + rm -f "$APPROVE_MARKER" "$COUNTER_FILE" "${CONV_FILE:-}" "${HISTORY_FILE:-}" "${REVIEWED_HASH_FILE:-}" allow_with_reason "Red Team 审阅已通过，plan 放行。" else # Plan was modified after approve: marker invalid, delete and fall through to re-review. @@ -360,13 +368,109 @@ if [ -f "$APPROVE_MARKER" ]; then fi fi +# --- Safety-valve helper functions (defined here, before both valves use them) --- + +# arm_dispatch_state — parse the plan's Dispatch Manifest into dispatch-check's +# state file so subsequent Agent/Task calls are enforced against it. Shared by +# the APPROVE branch (below) and the non-Critical safety valve's escalation +# branch (A.3): a Manifest that APPROVE would have armed is armed identically +# when the round instead terminates via the valve — enforcement must not +# silently lapse just because the cycle ended by escalation instead of a +# clean APPROVE. +# +# Two landing checks beyond the pre-refactor inline version: +# - Pre-mv guard: if DISPATCH_FILE already exists and is NOT a regular file +# (e.g. pre-created as a directory), fail immediately WITHOUT attempting +# mv — `mv -f srcfile dir` does not error, it silently moves srcfile INTO +# the directory, which would both misreport success and leave a stray +# temp file behind. +# - Post-mv guard: after mv, re-require `-f` on DISPATCH_FILE AND a second +# dispatch_state_is_valid_v2 pass on the file actually sitting at that +# path (not the one validated before the move) — catches any other +# landing failure the pre-check does not. +# +# Returns 0 on success (state armed and re-verified), 1 on any failure. The +# APPROVE call site ignores the return (fail-silent, unchanged contract); the +# valve call site treats 1 as fail-closed (deny, do not escalate). +arm_dispatch_state() { + mkdir -p "$DISPATCH_DIR" 2>/dev/null || true + DISPATCH_FILE="$DISPATCH_DIR/.dispatch-${SESSION_ID}.json" + if [ -e "$DISPATCH_FILE" ] && [ ! -f "$DISPATCH_FILE" ]; then + log_decision "manifest-write-skipped reason=target-not-regular" + return 1 + fi + local dispatch_temp + dispatch_temp=$(mktemp "$DISPATCH_DIR/.dispatch-${SESSION_ID}.json.XXXXXX" 2>/dev/null || true) + if [ -z "$dispatch_temp" ] \ + || ! parse_manifest_to_json "$PLAN" "$(plan_hash "$PLAN")" > "$dispatch_temp" 2>/dev/null \ + || ! dispatch_state_is_valid_v2 "$dispatch_temp"; then + rm -f "${dispatch_temp:-}" "$DISPATCH_FILE" + log_decision "manifest-write-skipped reason=invalid-json" + return 1 + fi + mv -f "$dispatch_temp" "$DISPATCH_FILE" + if [ -f "$DISPATCH_FILE" ] && dispatch_state_is_valid_v2 "$DISPATCH_FILE"; then + local dispatch_bytes + dispatch_bytes=$(wc -c < "$DISPATCH_FILE" | tr -d ' ') + log_decision "manifest-written file=$DISPATCH_FILE bytes=$dispatch_bytes" + return 0 + fi + rm -f "${dispatch_temp:-}" "$DISPATCH_FILE" + log_decision "manifest-write-skipped reason=landing-failed" + return 1 +} + +# record_reviewed_hash — persist the plan hash this round's CONCERNS +# verdict reviewed, so the non-Critical safety valve can distinguish "this +# exact revision was just reviewed" from "the author edited the plan after +# the last review round." Same region as arm_dispatch_state: both are +# escalation-path helpers the valve branch consults. +# +# mktemp+mv (not a direct `printf > file`) for the same reason DISPATCH_FILE +# uses a temp-then-rename: the valve's next-round `cat` on REVIEWED_HASH_FILE +# must never observe a partially-written hash. +# +# Returns 0 on success, 1 on any failure (temp-file creation or write) — +# callers must guard with `|| true` under set -euo pipefail. A failed write +# degrades to "no hash recorded," which the valve already treats safely +# (empty REVIEWED_HASH_FILE → review again, never a false escalation). +# Same two landing checks as arm_dispatch_state: a pre-mv guard (fail before +# attempting mv if REVIEWED_HASH_FILE exists and isn't a regular file — `mv -f +# srcfile dir` doesn't error, it silently moves srcfile INTO the directory, +# which would both misreport success and leave a stray temp file behind) and +# a post-mv guard (re-require `-f` on REVIEWED_HASH_FILE after the move). +record_reviewed_hash() { + local hash="$1" + if [ -e "$REVIEWED_HASH_FILE" ] && [ ! -f "$REVIEWED_HASH_FILE" ]; then + log_decision "reviewed-hash-write-failed reason=target-not-regular" + return 1 + fi + local hash_temp + hash_temp=$(mktemp "$COUNTER_DIR/.review-hash-${SESSION_ID}.XXXXXX" 2>/dev/null || true) + if [ -z "$hash_temp" ] || ! printf '%s' "$hash" > "$hash_temp" 2>/dev/null; then + rm -f "${hash_temp:-}" + log_decision "reviewed-hash-write-failed" + return 1 + fi + if ! mv -f "$hash_temp" "$REVIEWED_HASH_FILE" 2>/dev/null; then + rm -f "${hash_temp:-}" + log_decision "reviewed-hash-write-failed reason=mv-failed" + return 1 + fi + if [ ! -f "$REVIEWED_HASH_FILE" ]; then + log_decision "reviewed-hash-write-failed reason=landing-failed" + return 1 + fi + return 0 +} + # --- Global safety valve: total rounds exhausted → hard deny (tombstone counter) --- # Never delete counter — tombstone blocks subsequent calls until human intervenes if [ "$TOTAL_ROUNDS" -ge "$REVIEW_MAX_TOTAL_ROUNDS" ]; then log_decision "decision=deny reason=global-safety-valve total=$TOTAL_ROUNDS" # Keep the COUNTER_FILE tombstone, but the review cycle is over — no further # engine call will happen, so drop the session ref to avoid an orphan CONV_FILE. - rm -f "${CONV_FILE:-}" "${HISTORY_FILE:-}" + rm -f "${CONV_FILE:-}" "${HISTORY_FILE:-}" "${REVIEWED_HASH_FILE:-}" BLOCK_MSG="## Red Team Review — HARD STOP 审阅已达全局上限（${TOTAL_ROUNDS}/${REVIEW_MAX_TOTAL_ROUNDS}），仍存在未解决的审阅意见。 @@ -378,18 +482,67 @@ EOF exit 0 fi -# --- Non-Critical safety valve: CONCERNS rounds exhausted → allow (escalate to user) --- +# --- Non-Critical safety valve: CONCERNS rounds exhausted → allow, but ONLY +# if this exact plan revision is the one the last CONCERNS round reviewed +# (A.3). Any other revision — edited after the last round, or a legacy +# counter state predating this hash mechanism — gets one more real review +# instead of riding the valve unreviewed. --- if [ "$ATTEMPT" -ge "$REVIEW_MAX_ROUNDS" ]; then - log_decision "decision=allow reason=non-critical-safety-valve round=$ATTEMPT total=$TOTAL_ROUNDS" - rm -f "$COUNTER_FILE" "${CONV_FILE:-}" "${HISTORY_FILE:-}" - VALVE_MSG="## Red Team Review — ${REVIEW_ENGINE} — ESCALATED + CURRENT_HASH=$(plan_hash "$PLAN") + REVIEWED_HASH=$(cat "$REVIEWED_HASH_FILE" 2>/dev/null || true) + if [ -z "$REVIEWED_HASH" ] || [ "$CURRENT_HASH" != "$REVIEWED_HASH" ]; then + # Empty REVIEWED_HASH covers every "not reviewed at this revision" case + # uniformly: hash file absent, a directory, or unreadable. Do NOT allow, + # do NOT clear the counter — fall through to pre-flight and the engine + # review below. A CONCERNS verdict there increments the counter as usual + # and records this revision's hash (A.4), so the next identical-revision + # ExitPlanMode call clears the valve. + log_decision "decision=review-again reason=plan-not-reviewed-at-this-revision round=$ATTEMPT total=$TOTAL_ROUNDS" + else + if has_manifest "$PLAN"; then + if arm_dispatch_state; then + log_decision "decision=allow reason=non-critical-safety-valve round=$ATTEMPT total=$TOTAL_ROUNDS" + rm -f "$COUNTER_FILE" "${CONV_FILE:-}" "${HISTORY_FILE:-}" "$REVIEWED_HASH_FILE" + VALVE_MSG="## Red Team Review — ${REVIEW_ENGINE} — ESCALATED + +非 Critical 磋商已达上限（${ATTEMPT}/${REVIEW_MAX_ROUNDS}），未能达成一致。Plan 直接呈现给用户做最终裁决。 +已按 plan 的 Dispatch Manifest 写入 dispatch-check 状态，Agent 调用须与 Manifest 签名一致。" + VALVE_JSON=$(printf '%s' "$VALVE_MSG" | jq -Rs .) + cat << EOF +{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow","permissionDecisionReason":${VALVE_JSON}}} +EOF + exit 0 + else + # Fail-closed: a Manifest that cannot be armed must not let the plan + # through un-enforced. Same accounting as a pre-flight rejection — + # TOTAL_ROUNDS increments, ATTEMPT is frozen — so persistent failure + # still eventually trips the global valve. arm_dispatch_state already + # cleaned up any dispatch temp/target file it touched internally. + TOTAL_ROUNDS=$((TOTAL_ROUNDS + 1)) + echo "${ATTEMPT}:${TOTAL_ROUNDS}" > "$COUNTER_FILE" + log_decision "decision=deny reason=valve-arm-failed round=$ATTEMPT total=$TOTAL_ROUNDS" + ARM_FAIL_MSG="## Red Team Pre-flight — DISPATCH STATE NOT ARMED + +Manifest 无法序列化为 dispatch 状态，请按 v2 七列修正 Manifest 后重试。" + ARM_FAIL_JSON=$(printf '%s' "$ARM_FAIL_MSG" | jq -Rs .) + cat << EOF +{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"deny","permissionDecisionReason":${ARM_FAIL_JSON}}} +EOF + exit 0 + fi + else + log_decision "decision=allow reason=non-critical-safety-valve round=$ATTEMPT total=$TOTAL_ROUNDS" + rm -f "$COUNTER_FILE" "${CONV_FILE:-}" "${HISTORY_FILE:-}" "$REVIEWED_HASH_FILE" + VALVE_MSG="## Red Team Review — ${REVIEW_ENGINE} — ESCALATED 非 Critical 磋商已达上限（${ATTEMPT}/${REVIEW_MAX_ROUNDS}），未能达成一致。Plan 直接呈现给用户做最终裁决。" - VALVE_JSON=$(printf '%s' "$VALVE_MSG" | jq -Rs .) - cat << EOF + VALVE_JSON=$(printf '%s' "$VALVE_MSG" | jq -Rs .) + cat << EOF {"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow","permissionDecisionReason":${VALVE_JSON}}} EOF - exit 0 + exit 0 + fi + fi fi # --- Pre-flight manifest checks (AFTER non-critical valve, not before) --- @@ -725,21 +878,12 @@ if [ "$VERDICT" = "APPROVE" ]; then # Write plan hash to marker — ack-round guard compares hash to detect post-approve edits printf '%s' "$(plan_hash "$PLAN")" > "$APPROVE_MARKER" - # Parse manifest → dispatch JSON for Layer 2 enforcement (fail-silent: Layer 2 self-disables) + # Parse manifest → dispatch JSON for Layer 2 enforcement (fail-silent: Layer 2 + # self-disables). arm_dispatch_state (defined above, shared with the + # non-Critical safety valve's escalation branch) does the parse/validate/ + # write/re-verify sequence; this call site ignores its return value. if has_manifest "$PLAN"; then - mkdir -p "$DISPATCH_DIR" 2>/dev/null || true - DISPATCH_FILE="$DISPATCH_DIR/.dispatch-${SESSION_ID}.json" - DISPATCH_TEMP=$(mktemp "$DISPATCH_DIR/.dispatch-${SESSION_ID}.json.XXXXXX" 2>/dev/null || true) - if [ -n "$DISPATCH_TEMP" ] \ - && parse_manifest_to_json "$PLAN" "$(plan_hash "$PLAN")" > "$DISPATCH_TEMP" 2>/dev/null \ - && dispatch_state_is_valid_v2 "$DISPATCH_TEMP"; then - mv -f "$DISPATCH_TEMP" "$DISPATCH_FILE" - dispatch_bytes=$(wc -c < "$DISPATCH_FILE" | tr -d ' ') - log_decision "manifest-written file=$DISPATCH_FILE bytes=$dispatch_bytes" - else - rm -f "${DISPATCH_TEMP:-}" "$DISPATCH_FILE" - log_decision "manifest-write-skipped reason=invalid-json" - fi + arm_dispatch_state || true fi # Emit deny so Claude presents the approval to the user (allow reasons are invisible) @@ -750,11 +894,22 @@ fi TOTAL_ROUNDS=$((TOTAL_ROUNDS + 1)) if [ "$VERDICT" = "REJECT" ]; then - # REJECT (Critical): reset ATTEMPT so subsequent non-Critical rounds restart fresh + # REJECT (Critical): reset ATTEMPT so subsequent non-Critical rounds restart fresh. + # Also drop the reviewed-hash marker (A.4) — a Critical reset restarts the + # whole non-Critical cycle, so no revision should be treated as "already + # reviewed" by the safety valve after this point. ATTEMPT=0 + rm -f "$REVIEWED_HASH_FILE" else - # CONCERNS: non-Critical, increment ATTEMPT + # CONCERNS: non-Critical, increment ATTEMPT. ATTEMPT=$((ATTEMPT + 1)) + # Record which revision this CONCERNS round reviewed (A.4) — the safety + # valve only escalates a revision whose hash matches this file's content. + # `|| true`: under set -euo pipefail a failed hash write must never abort + # the hook mid-deny; record_reviewed_hash's own cleanup already makes a + # write failure equivalent to "no hash recorded," which the valve already + # treats safely (empty file → review again, never a false escalation). + record_reviewed_hash "$(plan_hash "$PLAN")" || true fi echo "${ATTEMPT}:${TOTAL_ROUNDS}" > "$COUNTER_FILE" diff --git a/plugins/plan-review/tests/plan-review.bats b/plugins/plan-review/tests/plan-review.bats index b8a8a66..ad60876 100644 --- a/plugins/plan-review/tests/plan-review.bats +++ b/plugins/plan-review/tests/plan-review.bats @@ -85,12 +85,24 @@ teardown() { # ============================================================================= # 6. Non-Critical safety valve → allow JSON with ESCALATED +# NOTE: setup changed by the plan-not-reviewed-at-this-revision safety valve +# fix — the valve now only escalates a plan revision the last CONCERNS round +# actually reviewed (see plan-review.sh's REVIEWED_HASH_FILE mechanism), so +# hitting ATTEMPT>=MAX_ROUNDS with no reviewed hash on file no longer +# escalates on its own; it falls through to one more real review round +# first, which is what seeds a matching hash for the second call below. @test "counter: non-critical safety valve → allow JSON with ESCALATED" { set_counter_value 3 test-session 5 export REVIEW_MAX_ROUNDS=3 + create_mock_engine "agy" "CONCERNS +[Major] one more round needed." INPUT=$(build_input) run_hook + assert_deny_json + [ "$(get_counter_value)" -eq 4 ] + [ "$(get_total_rounds)" -eq 6 ] + run_hook assert_approve_json local reason reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') @@ -99,6 +111,254 @@ teardown() { [ ! -f "${REVIEW_COUNTER_DIR}/.review-count-test-session" ] } +# write_reviewed_hash [session] +# Seeds REVIEWED_HASH_FILE with plan_hash(plan) so a single run_hook call +# lands the non-Critical safety valve on its "this exact revision was +# already reviewed" branch instead of the mismatch/review-again branch — +# isolates a test on the valve's manifest handling from the separately +# covered mismatch mechanic (see the "valve: plan changed..." / +# "valve: legacy counter..." tests below). Mirrors create_approve_marker's +# own hash computation above. +write_reviewed_hash() { + local plan_content="$1" session="${2:-test-session}" + local marker="${REVIEW_COUNTER_DIR}/.review-hash-${session}" + if command -v sha256sum >/dev/null 2>&1; then + printf '%s' "$plan_content" | sha256sum | awk '{print $1}' > "$marker" + elif command -v shasum >/dev/null 2>&1; then + printf '%s' "$plan_content" | shasum -a 256 | awk '{print $1}' > "$marker" + else + printf '%s' "$plan_content" | cksum | awk '{print $1}' > "$marker" + fi +} + +@test "valve: escalation with agent-row manifest writes dispatch state (v2)" { + export REVIEW_MAX_ROUNDS=3 + local plan + plan=$(manifest_v2_plan '| 1 | main | - | - | - | - | - | +| 2 | agent | dev-econ | preset | - | 1 | - | +| 3 | agent | Explore | runtime | haiku | 1 | 2 |') + write_reviewed_hash "$plan" + set_counter_value 3 test-session 3 + INPUT=$(build_input "plan=$plan") + run_hook + + assert_approve_json + local reason + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"ESCALATED"* ]] + [[ "$reason" == *"dispatch-check"* ]] + local dispatch_file="${REVIEW_COUNTER_DIR}/.dispatch-test-session.json" + [ -f "$dispatch_file" ] + jq -e '.schema_version == 2' "$dispatch_file" >/dev/null + jq -e '.allowed_signatures == [{"subagent_type":"dev-econ","model_source":"preset"},{"subagent_type":"Explore","model_source":"runtime","model":"haiku"}]' "$dispatch_file" >/dev/null + [ ! -f "${REVIEW_COUNTER_DIR}/.review-count-test-session" ] +} + +@test "valve: escalation without manifest writes no dispatch state (v2)" { + export REVIEW_MAX_ROUNDS=3 + write_reviewed_hash "Test plan content" + set_counter_value 3 test-session 3 + INPUT=$(build_input) + run_hook + + assert_approve_json + local reason + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"ESCALATED"* ]] + [ ! -f "${REVIEW_COUNTER_DIR}/.dispatch-test-session.json" ] +} + +@test "valve: plan changed since last reviewed round is reviewed again (v2)" { + export REVIEW_MAX_ROUNDS=3 + set_counter_value 3 test-session 3 + printf '%s' "0000" > "${REVIEW_COUNTER_DIR}/.review-hash-test-session" + create_mock_engine "agy" "CONCERNS +[Major] still needs work." + INPUT=$(build_input) + run_hook + + assert_deny_json + local reason + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"CONCERNS"* ]] + [[ "$reason" == *"补评审"* ]] + [[ "$reason" != *"-1"* ]] + [[ "$reason" != *"4/3"* ]] + [ "$(get_counter_value)" -eq 4 ] + [ "$(get_total_rounds)" -eq 4 ] + local hash_file="${REVIEW_COUNTER_DIR}/.review-hash-test-session" + [ -s "$hash_file" ] + [ "$(cat "$hash_file")" != "0000" ] + + run_hook + assert_approve_json + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"ESCALATED"* ]] + [ ! -f "$hash_file" ] + [ ! -f "${REVIEW_COUNTER_DIR}/.review-count-test-session" ] +} + +@test "valve: legacy counter without hash file is reviewed once before escalation (v2)" { + export REVIEW_MAX_ROUNDS=3 + set_counter_value 3 test-session 3 + create_mock_engine "agy" "CONCERNS +[Major] still needs work." + INPUT=$(build_input) + run_hook + + assert_deny_json + local reason + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"CONCERNS"* ]] + [[ "$reason" == *"补评审"* ]] + [[ "$reason" != *"-1"* ]] + [[ "$reason" != *"4/3"* ]] + [ "$(get_counter_value)" -eq 4 ] + [ "$(get_total_rounds)" -eq 4 ] + [ -s "${REVIEW_COUNTER_DIR}/.review-hash-test-session" ] + + run_hook + assert_approve_json + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"ESCALATED"* ]] +} + +@test "hash: CONCERNS round records reviewed plan hash (v2)" { + create_mock_engine "agy" "CONCERNS +[Major] needs work." + INPUT=$(build_input plan="Plan A") + run_hook + assert_deny_json + local hash_file="${REVIEW_COUNTER_DIR}/.review-hash-test-session" + [ -s "$hash_file" ] + local hash_a + hash_a=$(cat "$hash_file") + + INPUT=$(build_input plan="Plan B") + run_hook + assert_deny_json + [ -s "$hash_file" ] + local hash_b + hash_b=$(cat "$hash_file") + [ "$hash_a" != "$hash_b" ] +} + +@test "hash: ack-round approval clears reviewed hash file (v2)" { + create_mock_engine "agy" "CONCERNS +[Major] needs work." + INPUT=$(build_input) + run_hook + assert_deny_json + local hash_file="${REVIEW_COUNTER_DIR}/.review-hash-test-session" + [ -s "$hash_file" ] + + create_mock_engine "agy" "APPROVE +Looks good now." + run_hook + assert_ack_approve_json + + run_hook + assert_approve_json + [ ! -f "$hash_file" ] +} + +@test "valve: REJECT after threshold resets cycle and does not escalate (v2)" { + export REVIEW_MAX_ROUNDS=3 + set_counter_value 3 test-session 3 + printf '%s' "0000" > "${REVIEW_COUNTER_DIR}/.review-hash-test-session" + create_mock_engine "agy" "REJECT +[Critical] broken." + INPUT=$(build_input) + run_hook + + assert_deny_json + local reason + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"REJECT"* ]] + [ "$(get_counter_value)" -eq 0 ] + [ "$(get_total_rounds)" -eq 4 ] + [ ! -f "${REVIEW_COUNTER_DIR}/.review-hash-test-session" ] + + run_hook + assert_deny_json + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"REJECT"* ]] + [[ "$reason" != *"ESCALATED"* ]] + [ ! -f "${REVIEW_COUNTER_DIR}/.dispatch-test-session.json" ] +} + +@test "valve: manifest present but dispatch state cannot be written denies (v2)" { + export REVIEW_MAX_ROUNDS=3 + local plan='## Plan +Use Task( for isolation. + +## Dispatch Manifest +| step | location | subagent_type | model_source | model | depends_on | +|------|----------|---------------|--------------|-------|------------| +| 1 | main | - | - | - | - |' + write_reviewed_hash "$plan" + set_counter_value 3 test-session 3 + INPUT=$(build_input "plan=$plan") + run_hook + + assert_deny_json + local reason + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"dispatch 状态"* ]] + [ ! -f "${REVIEW_COUNTER_DIR}/.dispatch-test-session.json" ] + [ "$(get_counter_value)" -eq 3 ] + [ "$(get_total_rounds)" -eq 4 ] +} + +@test "valve: dispatch state landing failure denies (v2)" { + export REVIEW_MAX_ROUNDS=3 + local plan + plan=$(manifest_v2_plan '| 1 | main | - | - | - | - | - | +| 2 | agent | dev-econ | preset | - | 1 | - | +| 3 | agent | Explore | runtime | haiku | 1 | 2 |') + write_reviewed_hash "$plan" + set_counter_value 3 test-session 3 + mkdir "${REVIEW_COUNTER_DIR}/.dispatch-test-session.json" + INPUT=$(build_input "plan=$plan") + run_hook + + assert_deny_json + local reason + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"dispatch 状态"* ]] + [ -d "${REVIEW_COUNTER_DIR}/.dispatch-test-session.json" ] + [ -z "$(ls -A "${REVIEW_COUNTER_DIR}/.dispatch-test-session.json")" ] + ! find "$REVIEW_COUNTER_DIR" -maxdepth 1 -name '.dispatch-test-session.json.*' -print -quit | grep -q . + [ "$(get_counter_value)" -eq 3 ] + [ "$(get_total_rounds)" -eq 4 ] +} + +@test "hash: unwritable hash path never breaks the hook nor escalates (v2)" { + export REVIEW_MAX_ROUNDS=3 + mkdir "${REVIEW_COUNTER_DIR}/.review-hash-test-session" + create_mock_engine "agy" "CONCERNS +[Major] needs work." + set_counter_value 2 test-session 2 + INPUT=$(build_input) + run_hook + + assert_deny_json + local reason + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"CONCERNS"* ]] + [ "$(get_counter_value)" -eq 3 ] + [ "$(get_total_rounds)" -eq 3 ] + ! find "$REVIEW_COUNTER_DIR" -maxdepth 1 -name '.review-hash-test-session.*' -print -quit | grep -q . + [ -z "$(ls -A "${REVIEW_COUNTER_DIR}/.review-hash-test-session")" ] + + run_hook + assert_deny_json + reason=$(echo "$HOOK_STDOUT" | jq -r '.hookSpecificOutput.permissionDecisionReason') + [[ "$reason" == *"CONCERNS"* ]] + [[ "$reason" != *"ESCALATED"* ]] + [ -z "$(ls -A "${REVIEW_COUNTER_DIR}/.review-hash-test-session")" ] +} + # 7. Below max rounds → proceeds to review @test "counter: below max rounds → calls engine" { set_counter_value 2 test-session 3