From 485507ab35859870564857c2b9ca36fbaa74a106 Mon Sep 17 00:00:00 2001 From: screenleon Date: Fri, 31 Jul 2026 09:53:16 +0900 Subject: [PATCH] fix(gate): harden reviewer contract diagnostics --- runtime/bin/pr-gate.sh | 42 ++++++++++- runtime/lib/gate-result-verify.sh | 35 ++++++++- tests/lib/test-pr-gate-fixture.sh | 10 +++ tests/shell/test-pr-gate.sh | 116 ++++++++++++++++++++++++++++++ 4 files changed, 199 insertions(+), 4 deletions(-) diff --git a/runtime/bin/pr-gate.sh b/runtime/bin/pr-gate.sh index d67c288a..f1f25f48 100755 --- a/runtime/bin/pr-gate.sh +++ b/runtime/bin/pr-gate.sh @@ -2224,9 +2224,30 @@ else def finding_contract: .findings | type == "array" and all(.[]; finding) and ([.[].id] | length) == ([.[].id] | unique | length); + def findings_array: + if (.findings | type) == "array" then .findings else [] end; + def blocking_severity_violation: + [findings_array[] | + select( + (.hard_gate_class | IN("soft_block","hard_block")) and + ((.severity | IN("critical","high")) | not) + ) + ] | first; + def blocking_origin_violation: + [findings_array[] | + select( + (.hard_gate_class | IN("soft_block","hard_block")) and + ((.origin | IN("diff_caused","uncertain")) | not) + ) + ] | first; + def display: + if . == null + then "" + else (tostring | tojson | .[1:-1]) + end; def verdict_contract: (.verdict | IN("approve","advise","block-soft","block")) and - (.rationale | nonempty) and + (.rationale | nonempty) and (if .verdict == "block-soft" then any(.findings[]; .hard_gate_class == "soft_block") elif .verdict == "block" @@ -2245,6 +2266,18 @@ else then "invalid top-level or binding contract" elif (coverage_contract | not) then "invalid coverage contract" + elif (blocking_severity_violation != null) + then (blocking_severity_violation as $invalid | + "invalid finding contract: " + ($invalid.id | display) + + " hard_gate_class=" + ($invalid.hard_gate_class | display) + + " requires severity=critical|high (got " + + ($invalid.severity | display) + ")") + elif (blocking_origin_violation != null) + then (blocking_origin_violation as $invalid | + "invalid finding contract: " + ($invalid.id | display) + + " hard_gate_class=" + ($invalid.hard_gate_class | display) + + " requires origin=diff_caused|uncertain (got " + + ($invalid.origin | display) + ")") elif (finding_contract | not) then "invalid finding contract" elif $references != null and (evidence_reference_contract | not) @@ -5285,8 +5318,11 @@ printf -v REVIEWER_PROTOCOL_INSTRUCTIONS \ ' origin=diff_caused|pre_existing|uncertain|caution, source={path,line,symbol},' \ ' affected_behavior, why_it_matters, failure_mode, minimum_fix_boundary, and' \ ' verification_expectation. source needs path plus line or symbol.' \ - ' - pre_existing/caution findings are non-blocking. block-soft needs a soft_block' \ - ' finding; block needs a hard_block finding; approve/advise may contain only none.' \ + ' - soft_block/hard_block findings require severity=critical|high and' \ + ' origin=diff_caused|uncertain. medium/low and pre_existing/caution findings' \ + ' must use hard_gate_class=none.' \ + ' - block-soft needs a soft_block finding; block needs a hard_block finding;' \ + ' approve/advise may contain only hard_gate_class=none.' \ ' - verdict is exactly approve|advise|block-soft|block. Map legacy pass and' \ ' pass-not-applicable to approve; map needs-tests to block. Never put pass,' \ ' pass-not-applicable, needs-tests, or prose in verdict; prose belongs in rationale.' \ diff --git a/runtime/lib/gate-result-verify.sh b/runtime/lib/gate-result-verify.sh index a3599e69..9da9d8d8 100644 --- a/runtime/lib/gate-result-verify.sh +++ b/runtime/lib/gate-result-verify.sh @@ -190,9 +190,30 @@ _gate_reviewer_protocol_document_verify() { def finding_contract: .findings | type == "array" and all(.[]; finding) and ([.[].id] | length) == ([.[].id] | unique | length); + def findings_array: + if (.findings | type) == "array" then .findings else [] end; + def blocking_severity_violation: + [findings_array[] | + select( + (.hard_gate_class | IN("soft_block","hard_block")) and + ((.severity | IN("critical","high")) | not) + ) + ] | first; + def blocking_origin_violation: + [findings_array[] | + select( + (.hard_gate_class | IN("soft_block","hard_block")) and + ((.origin | IN("diff_caused","uncertain")) | not) + ) + ] | first; + def display: + if . == null + then "" + else (tostring | tojson | .[1:-1]) + end; def verdict_contract: (.verdict | IN("approve","advise","block-soft","block")) and - (.rationale | nonempty) and + (.rationale | nonempty) and (if .verdict == "block-soft" then any(.findings[]; .hard_gate_class == "soft_block") elif .verdict == "block" @@ -211,6 +232,18 @@ _gate_reviewer_protocol_document_verify() { then "invalid top-level or binding contract" elif (coverage_contract | not) then "invalid coverage contract" + elif (blocking_severity_violation != null) + then (blocking_severity_violation as $invalid | + "invalid finding contract: " + ($invalid.id | display) + + " hard_gate_class=" + ($invalid.hard_gate_class | display) + + " requires severity=critical|high (got " + + ($invalid.severity | display) + ")") + elif (blocking_origin_violation != null) + then (blocking_origin_violation as $invalid | + "invalid finding contract: " + ($invalid.id | display) + + " hard_gate_class=" + ($invalid.hard_gate_class | display) + + " requires origin=diff_caused|uncertain (got " + + ($invalid.origin | display) + ")") elif (finding_contract | not) then "invalid finding contract" elif $references != null and (evidence_reference_contract | not) diff --git a/tests/lib/test-pr-gate-fixture.sh b/tests/lib/test-pr-gate-fixture.sh index 1827afc9..9a5074f3 100644 --- a/tests/lib/test-pr-gate-fixture.sh +++ b/tests/lib/test-pr-gate-fixture.sh @@ -109,6 +109,16 @@ pr_gate_fixture_write_reviewer_protocol() { minimum_fix_boundary:"Use the complete reviewer role as the prefix.", verification_expectation:"Validate the corrected finding ID." }] + elif $mutation == "blocking-medium-severity" and + $reviewer == "architecture-reviewer" + then .findings[0].severity = "medium" + elif $mutation == "blocking-pre-existing-origin" and + $reviewer == "architecture-reviewer" + then .findings[0].origin = "pre_existing" + elif $mutation == "blocking-terminal-escape" and + $reviewer == "architecture-reviewer" + then .findings[0].id = "architecture-reviewer-F001\u001b[31m" + | .findings[0].severity = "medium" else . end ' | { diff --git a/tests/shell/test-pr-gate.sh b/tests/shell/test-pr-gate.sh index 2f335f5c..41d230df 100755 --- a/tests/shell/test-pr-gate.sh +++ b/tests/shell/test-pr-gate.sh @@ -8696,6 +8696,10 @@ test_parallel_reviewer_protocol_preserves_session_topology() { "reference_index.entries[]" || return assert_file_contains "$name" "$reviewer_brief" \ "out-of-scope repository paths make the protocol INCOMPLETE" || return + assert_file_contains "$name" "$reviewer_brief" \ + "soft_block/hard_block findings require severity=critical|high" || return + assert_file_contains "$name" "$reviewer_brief" \ + "medium/low and pre_existing/caution findings" || return pass "$name" } @@ -8879,6 +8883,115 @@ test_reviewer_protocol_abbreviated_finding_id_is_incomplete() { pass "$name" } +# Behavior: a blocking finding with medium severity is rejected with the exact +# field-level contract diagnostic that the reviewer brief now documents. +# Steps: emit the observed architecture-reviewer combination, run the parallel +# gate, then assert the finding ID, blocking class, and required severity. +test_reviewer_protocol_blocking_medium_severity_is_diagnosed() { + local name="reviewer-protocol/blocking-medium-severity-diagnostic" + should_run "$name" || return 0 + local dir="$TMP_ROOT/$name" home="$TMP_ROOT/$name/home" + local repo="$TMP_ROOT/$name/repo" runner="$TMP_ROOT/$name/runner" + local out="$TMP_ROOT/$name/out" err="$TMP_ROOT/$name/err" code + mkdir -p "$dir" + create_runner "$runner" + create_agents "$home" critic qa-tester architecture-reviewer + create_repo "$repo" docs + set +e + CODEX_GATE_STUB_VERDICT=block-soft \ + CODEX_GATE_STUB_PROTOCOL_MUTATION=blocking-medium-severity \ + run_gate "$home" "$runner" "$repo" "$out" "$err" \ + --base main --reviewers critic,qa-tester,architecture-reviewer --mode parallel + code=$? + set -e + [[ "$code" -ne 0 ]] || { + fail "$name" "medium-severity blocker unexpectedly passed" + return + } + if ! grep -Fq -- \ + "architecture-reviewer-F001 hard_gate_class=soft_block requires severity=critical|high (got medium)" \ + "$err"; then + fail "$name" "precise blocking-severity diagnostic missing: $(cat "$err" 2>/dev/null)" + return + fi + assert_not_contains "$name" "$out" "[synthesis]" || return + pass "$name" +} + +# Behavior: a blocking finding with a pre-existing origin is rejected before +# synthesis with a precise origin-contract diagnostic. +# Steps: mutate the architecture-reviewer origin, run the parallel gate, then +# assert the offending value, permitted origins, and absence of synthesis. +test_reviewer_protocol_blocking_pre_existing_origin_is_diagnosed() { + local name="reviewer-protocol/blocking-pre-existing-origin-diagnostic" + should_run "$name" || return 0 + local dir="$TMP_ROOT/$name" home="$TMP_ROOT/$name/home" + local repo="$TMP_ROOT/$name/repo" runner="$TMP_ROOT/$name/runner" + local out="$TMP_ROOT/$name/out" err="$TMP_ROOT/$name/err" code + mkdir -p "$dir" + create_runner "$runner" + create_agents "$home" critic qa-tester architecture-reviewer + create_repo "$repo" docs + set +e + CODEX_GATE_STUB_VERDICT=block-soft \ + CODEX_GATE_STUB_PROTOCOL_MUTATION=blocking-pre-existing-origin \ + run_gate "$home" "$runner" "$repo" "$out" "$err" \ + --base main --reviewers critic,qa-tester,architecture-reviewer --mode parallel + code=$? + set -e + [[ "$code" -ne 0 ]] || { + fail "$name" "pre-existing-origin blocker unexpectedly passed" + return + } + if ! grep -Fq -- \ + "architecture-reviewer-F001 hard_gate_class=soft_block requires origin=diff_caused|uncertain (got pre_existing)" \ + "$err"; then + fail "$name" "precise blocking-origin diagnostic missing: $(cat "$err" 2>/dev/null)" + return + fi + assert_not_contains "$name" "$out" "[synthesis]" || return + pass "$name" +} + +# Behavior: reviewer-controlled control characters are JSON-escaped before a +# field-level protocol diagnostic reaches the terminal. +# Steps: inject ESC into a rejected finding ID, run the parallel gate, then +# assert literal JSON escaping, no raw ESC byte, and no synthesis. +test_reviewer_protocol_diagnostic_terminal_escapes_control_characters() { + local name="reviewer-protocol/diagnostic-terminal-control-escape" + should_run "$name" || return 0 + local dir="$TMP_ROOT/$name" home="$TMP_ROOT/$name/home" + local repo="$TMP_ROOT/$name/repo" runner="$TMP_ROOT/$name/runner" + local out="$TMP_ROOT/$name/out" err="$TMP_ROOT/$name/err" code + mkdir -p "$dir" + create_runner "$runner" + create_agents "$home" critic qa-tester architecture-reviewer + create_repo "$repo" docs + set +e + CODEX_GATE_STUB_VERDICT=block-soft \ + CODEX_GATE_STUB_PROTOCOL_MUTATION=blocking-terminal-escape \ + run_gate "$home" "$runner" "$repo" "$out" "$err" \ + --base main --reviewers critic,qa-tester,architecture-reviewer --mode parallel + code=$? + set -e + [[ "$code" -ne 0 ]] || { + fail "$name" "control-character finding unexpectedly passed" + return + } + if ! grep -Fq -- \ + 'architecture-reviewer-F001\u001b[31m hard_gate_class=soft_block requires severity=critical|high (got medium)' \ + "$err"; then + fail "$name" "JSON-escaped diagnostic missing: $(cat "$err" 2>/dev/null)" + return + fi + if grep -q $'\033' "$err"; then + fail "$name" "raw terminal ESC byte leaked into diagnostic" + return + fi + assert_not_contains "$name" "$out" "[synthesis]" || return + pass "$name" +} + # Behavior: a parallel reviewer cannot cite a syntactically valid repository # path that is absent from the scope manifest reference index. # Steps: mutate one coverage reference to an out-of-scope path, run the @@ -9039,6 +9152,9 @@ run_test test_reviewer_protocol_evidence_less_blocker_is_incomplete run_test test_reviewer_protocol_legacy_pass_reports_verdict_contract run_test test_reviewer_protocol_extra_role_field_reports_top_level_contract run_test test_reviewer_protocol_abbreviated_finding_id_is_incomplete +run_test test_reviewer_protocol_blocking_medium_severity_is_diagnosed +run_test test_reviewer_protocol_blocking_pre_existing_origin_is_diagnosed +run_test test_reviewer_protocol_diagnostic_terminal_escapes_control_characters run_test test_parallel_reviewer_protocol_out_of_scope_reference_is_incomplete run_test test_sequential_reviewer_protocol_out_of_range_line_is_incomplete run_test test_reviewer_protocol_duplicate_heading_uses_json_verdict