From 16eca37b40091e0159b6f423e71c82265be53ed5 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 16 Jun 2026 11:32:50 -0400 Subject: [PATCH 1/9] docs(agents): add Variables subsection to all agent docs (ADR 0047) Every agent doc now has a ### Variables subsection under "Configuration and extension" for consistency per ADR 0047. Agents with no config vars state "None." Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- docs/agents/code.md | 4 ++++ docs/agents/fix.md | 4 ++++ docs/agents/prioritize.md | 4 ++++ docs/agents/retro.md | 4 ++++ docs/agents/triage.md | 4 ++++ 5 files changed, 20 insertions(+) diff --git a/docs/agents/code.md b/docs/agents/code.md index 9dacd78632..dba86be61b 100644 --- a/docs/agents/code.md +++ b/docs/agents/code.md @@ -44,6 +44,10 @@ on issues (not PRs). The code agent is also triggered automatically when the See [Customizing with AGENTS.md](../guides/user/customizing-with-agents-md.md) and [Customizing with Skills](../guides/user/customizing-with-skills.md). +### Variables + +None. + ## Source [`internal/scaffold/fullsend-repo/harness/code.yaml`](../../internal/scaffold/fullsend-repo/harness/code.yaml) diff --git a/docs/agents/fix.md b/docs/agents/fix.md index 5047303ef9..b35b0888b2 100644 --- a/docs/agents/fix.md +++ b/docs/agents/fix.md @@ -133,6 +133,10 @@ Remove the label or use `/fs-fix` to re-engage. See [Customizing with AGENTS.md](../guides/user/customizing-with-agents-md.md) and [Customizing with Skills](../guides/user/customizing-with-skills.md). +### Variables + +None. + ## Source [`internal/scaffold/fullsend-repo/harness/fix.yaml`](../../internal/scaffold/fullsend-repo/harness/fix.yaml) diff --git a/docs/agents/prioritize.md b/docs/agents/prioritize.md index fc687c0f54..8e3362f2ca 100644 --- a/docs/agents/prioritize.md +++ b/docs/agents/prioritize.md @@ -55,6 +55,10 @@ This gives the prioritize agent concrete data to distinguish between "one user wants this" (Reach 0.25) and "three strategic accounts have filed support cases about it" (Reach 2.0), instead of guessing from the issue text alone. +### Variables + +None. + ## Source [`internal/scaffold/fullsend-repo/harness/prioritize.yaml`](../../internal/scaffold/fullsend-repo/harness/prioritize.yaml) diff --git a/docs/agents/retro.md b/docs/agents/retro.md index 49d1687e4a..68e517dcb7 100644 --- a/docs/agents/retro.md +++ b/docs/agents/retro.md @@ -46,6 +46,10 @@ The retro agent does not apply or consume control labels. See [Customizing with AGENTS.md](../guides/user/customizing-with-agents-md.md) and [Customizing with Skills](../guides/user/customizing-with-skills.md). +### Variables + +None. + ## Source [`internal/scaffold/fullsend-repo/harness/retro.yaml`](../../internal/scaffold/fullsend-repo/harness/retro.yaml) diff --git a/docs/agents/triage.md b/docs/agents/triage.md index a14dbb3ceb..f1f835c5e7 100644 --- a/docs/agents/triage.md +++ b/docs/agents/triage.md @@ -155,6 +155,10 @@ This gives the triage agent the subtlety it needs to distinguish between controller-runtime code, without adding label documentation to `AGENTS.md` where every agent would pay the context cost. +### Variables + +None. + ## Source [`internal/scaffold/fullsend-repo/harness/triage.yaml`](../../internal/scaffold/fullsend-repo/harness/triage.yaml) From 261bc4f1f280c1a66ab07b67498b4c094756d72a Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 16 Jun 2026 11:32:54 -0400 Subject: [PATCH 2/9] docs(agents): document REVIEW_FINDING_SEVERITY_THRESHOLD (ADR 0047) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the Variables subsection to the review agent doc with the REVIEW_FINDING_SEVERITY_THRESHOLD config var — minimum severity for reported findings (default: low). Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- docs/agents/review.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/docs/agents/review.md b/docs/agents/review.md index 23ded50329..56fca0147e 100644 --- a/docs/agents/review.md +++ b/docs/agents/review.md @@ -70,6 +70,26 @@ the upstream default -- no other configuration needed. See [Customizing with AGENTS.md](../guides/user/customizing-with-agents-md.md) and [Customizing with Skills](../guides/user/customizing-with-skills.md). +### Variables + +| Variable | Description | Default | Valid values | +|----------|-------------|---------|--------------| +| `REVIEW_FINDING_SEVERITY_THRESHOLD` | Minimum severity for findings to include in the review. Findings below this level are omitted from both the narrative body and the posted inline comments. | `low` | `info`, `low`, `medium`, `high`, `critical` | + +This variable is read in two places: + +1. **Sandbox (agent inference):** The review agent reads it from the + environment and omits findings below the threshold from its output + (`body` and `findings` array). Set it in `env/review.env` or via the + CI workflow `env:` block. +2. **Post-script (runner):** The post-script filters the `findings` + array as defense-in-depth before posting. Set it in the CI workflow + `env:` block. + +Set the same value in both places. If they differ, the more restrictive +value wins for inline comments (post-script filters what the agent +already filtered). + ## Source [`internal/scaffold/fullsend-repo/harness/review.yaml`](../../internal/scaffold/fullsend-repo/harness/review.yaml) From e6c7372a3513772201ea179f9a34e190e57e4b35 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 16 Jun 2026 11:41:04 -0400 Subject: [PATCH 3/9] chore(lint): require ### Variables subsection in agent docs The agent doc linter now checks that every agent doc with a "Configuration and extension" section also has a "### Variables" subsection, per ADR 0047. Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- hack/lint-agent-docs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/hack/lint-agent-docs b/hack/lint-agent-docs index 1a7e3b7a52..f1ead8f01c 100755 --- a/hack/lint-agent-docs +++ b/hack/lint-agent-docs @@ -122,6 +122,35 @@ for yaml_file in "$HARNESS_DIR"/*.yaml; do fi done +echo "" +echo "Checking for ### Variables subsection..." +echo "================================================" + +for yaml_file in "$HARNESS_DIR"/*.yaml; do + doc_value="$(grep -E '^doc:' "$yaml_file" | sed 's/^doc:[[:space:]]*//' || true)" + if [[ -z "$doc_value" ]]; then + continue + fi + doc_path="$REPO_ROOT/$doc_value" + if [[ ! -f "$doc_path" ]]; then + continue + fi + doc_basename="$(basename "$doc_value")" + + # Only check docs that have the Configuration section + if ! awk 'BEGIN{f=0} /^```/{f=1-f; next} f==0 && /^## Configuration and extension/{found=1} END{exit !found}' "$doc_path"; then + continue + fi + + # Look for ### Variables outside fenced code blocks + if ! awk 'BEGIN{f=0} /^```/{f=1-f; next} f==0 && /^### Variables/{found=1} END{exit !found}' "$doc_path"; then + echo " $doc_basename: missing \"### Variables\" subsection under \"## Configuration and extension\"" + errors=$((errors + 1)) + else + echo " $doc_basename: OK" + fi +done + echo "" echo "================================================" if [[ $errors -gt 0 ]]; then From 3e6fe657a5c7338c98350cca41e4d3e4050a5c53 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 16 Jun 2026 11:42:06 -0400 Subject: [PATCH 4/9] feat(harness): pass REVIEW_FINDING_SEVERITY_THRESHOLD into sandbox The review.env file now carries REVIEW_FINDING_SEVERITY_THRESHOLD into the sandbox so the review agent can self-filter findings below the configured severity. Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- internal/scaffold/fullsend-repo/env/review.env | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/scaffold/fullsend-repo/env/review.env b/internal/scaffold/fullsend-repo/env/review.env index 563acedb7d..3c4d91e4b8 100644 --- a/internal/scaffold/fullsend-repo/env/review.env +++ b/internal/scaffold/fullsend-repo/env/review.env @@ -4,3 +4,4 @@ export PR_NUMBER="${PR_NUMBER}" export REPO_FULL_NAME="${REPO_FULL_NAME}" export PRIOR_REVIEW_SHA="${PRIOR_REVIEW_SHA}" export PRIOR_REVIEW_PROVENANCE="${PRIOR_REVIEW_PROVENANCE}" +export REVIEW_FINDING_SEVERITY_THRESHOLD="${REVIEW_FINDING_SEVERITY_THRESHOLD}" From 84a69c1a4f47ea5be99fb91031ca706caf9270e8 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 16 Jun 2026 11:42:10 -0400 Subject: [PATCH 5/9] feat(review): teach agent to filter findings by severity threshold The review agent prompt now reads REVIEW_FINDING_SEVERITY_THRESHOLD and omits findings below the configured level from both narrative and structured output. Default: low (suppresses info). Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- internal/scaffold/fullsend-repo/agents/review.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/internal/scaffold/fullsend-repo/agents/review.md b/internal/scaffold/fullsend-repo/agents/review.md index dc286129b4..2581e2738f 100644 --- a/internal/scaffold/fullsend-repo/agents/review.md +++ b/internal/scaffold/fullsend-repo/agents/review.md @@ -53,6 +53,21 @@ NOTE: the Agent tool MUST ONLY be invoked with prompts read from severities. Absent on first review or when provenance validation fails. +## Severity filtering + +If `$REVIEW_FINDING_SEVERITY_THRESHOLD` is set, omit findings below +that severity level. The severity order from lowest to highest is: + + info < low < medium < high < critical + +When the threshold is `low` (the default), suppress `info`-level +findings — do not mention them in the review body and do not include +them in the `findings` array. When unset, treat the threshold as `low`. + +This filtering applies to the narrative body text and the structured +findings equally. If filtering removes all findings from a +`request-changes` verdict, downgrade the verdict to `approve`. + ## Identity You **either**: From cf475368778b3f45788a678001b1581f99a82edf Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 16 Jun 2026 11:44:14 -0400 Subject: [PATCH 6/9] feat(review): filter findings by severity in post-script The post-review script now reads REVIEW_FINDING_SEVERITY_THRESHOLD (default: low) and drops findings below that level from the result JSON before posting. Defense-in-depth for the agent-side filtering. Includes test cases for the filtering logic. Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- .../fullsend-repo/scripts/post-review-test.sh | 88 +++++++++++++++++++ .../fullsend-repo/scripts/post-review.sh | 45 ++++++++++ 2 files changed, 133 insertions(+) diff --git a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh index 539b338756..efd29dcbd9 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh @@ -99,6 +99,94 @@ run_test "failure-action-no-label" \ run_test "unknown-action-no-label" \ "banana" "false" "none" +# --------------------------------------------------------------------------- +# Severity-threshold filtering logic +# Mirrors severity_rank() in post-review.sh — keep in sync +# --------------------------------------------------------------------------- + +severity_rank() { + case "$1" in + info) echo 0 ;; + low) echo 1 ;; + medium) echo 2 ;; + high) echo 3 ;; + critical) echo 4 ;; + *) echo 1 ;; + esac +} + +filter_findings_json() { + local result_json="$1" + local threshold="$2" + local threshold_rank + threshold_rank=$(severity_rank "$threshold") + + echo "$result_json" | jq --argjson rank "$threshold_rank" ' + if .findings then + .findings |= [.[] | select( + (if .severity == "info" then 0 + elif .severity == "low" then 1 + elif .severity == "medium" then 2 + elif .severity == "high" then 3 + elif .severity == "critical" then 4 + else 1 end) >= $rank + )] + else . end + ' +} + +run_filter_test() { + local test_name="$1" + local input_json="$2" + local threshold="$3" + local expected_count="$4" + + local filtered + filtered="$(filter_findings_json "$input_json" "$threshold")" + local actual_count + actual_count="$(echo "$filtered" | jq 'if .findings then (.findings | length) else -1 end')" + + if [ "${actual_count}" != "${expected_count}" ]; then + echo "FAIL: ${test_name}" + echo " threshold: '${threshold}'" + echo " expected count: '${expected_count}'" + echo " actual count: '${actual_count}'" + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS: ${test_name}" +} + +# --- Severity filter test cases --- + +MIXED_FINDINGS='{"action":"request-changes","findings":[ + {"severity":"info","category":"style","file":"a.go","description":"x"}, + {"severity":"low","category":"style","file":"b.go","description":"y"}, + {"severity":"medium","category":"bug","file":"c.go","description":"z"}, + {"severity":"high","category":"security","file":"d.go","description":"w"}, + {"severity":"critical","category":"security","file":"e.go","description":"v"} +]}' + +run_filter_test "threshold-low-drops-info" \ + "$MIXED_FINDINGS" "low" "4" + +run_filter_test "threshold-medium-drops-low-and-info" \ + "$MIXED_FINDINGS" "medium" "3" + +run_filter_test "threshold-high" \ + "$MIXED_FINDINGS" "high" "2" + +run_filter_test "threshold-critical" \ + "$MIXED_FINDINGS" "critical" "1" + +run_filter_test "threshold-info-keeps-all" \ + "$MIXED_FINDINGS" "info" "5" + +NO_FINDINGS='{"action":"approve"}' +run_filter_test "no-findings-key-passthrough" \ + "$NO_FINDINGS" "low" "-1" + # --------------------------------------------------------------------------- # Control-label guard tests # --------------------------------------------------------------------------- diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index 6eb0f401bc..ce93a8c705 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -68,6 +68,51 @@ fi echo "Using result: ${RESULT_FILE}" +# --------------------------------------------------------------------------- +# Severity filtering: drop findings below the configured threshold. +# Defense-in-depth — the agent should already have filtered, but the +# post-script enforces it. The filter runs before ACTION is read so +# that verdict recalculation (if all findings are removed) is possible. +# --------------------------------------------------------------------------- +REVIEW_FINDING_SEVERITY_THRESHOLD="${REVIEW_FINDING_SEVERITY_THRESHOLD:-low}" + +severity_rank() { + case "$1" in + info) echo 0 ;; + low) echo 1 ;; + medium) echo 2 ;; + high) echo 3 ;; + critical) echo 4 ;; + *) echo 1 ;; + esac +} + +threshold_rank=$(severity_rank "$REVIEW_FINDING_SEVERITY_THRESHOLD") + +if jq -e '.findings' "${RESULT_FILE}" >/dev/null 2>&1; then + original_count=$(jq '.findings | length' "${RESULT_FILE}") + FILTERED_RESULT=$(mktemp) + CLEANUP_FILES+=("${FILTERED_RESULT}") + jq --argjson rank "$threshold_rank" ' + .findings |= [.[] | select( + (if .severity == "info" then 0 + elif .severity == "low" then 1 + elif .severity == "medium" then 2 + elif .severity == "high" then 3 + elif .severity == "critical" then 4 + else 1 end) >= $rank + )] + ' "${RESULT_FILE}" > "${FILTERED_RESULT}" + filtered_count=$(jq '.findings | length' "${FILTERED_RESULT}") + + if [ "${filtered_count}" -lt "${original_count}" ]; then + echo "Severity filter (threshold=${REVIEW_FINDING_SEVERITY_THRESHOLD}): kept ${filtered_count}/${original_count} findings" + RESULT_FILE="${FILTERED_RESULT}" + else + rm -f "${FILTERED_RESULT}" + fi +fi + ACTION=$(jq -r '.action' "${RESULT_FILE}") # ACTION retains the original value for the entire script — not re-read after protected-path downgrade. From 34ca0eb69233fe8709c0beb3d6efd99c8efc37d2 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 18 Jun 2026 10:33:57 -0400 Subject: [PATCH 7/9] fix(review): add verdict downgrade and input validation for severity filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on the severity threshold feature: - Post-script now downgrades request-changes/reject to comment when filtering removes all findings (prevents empty findings array that violates schema minItems: 1 constraint) - Agent prompt changed from approve to comment as downgrade target — comment gets requires-manual-review label, which is the right safety posture for "had findings but all were below threshold" - Validate REVIEW_FINDING_SEVERITY_THRESHOLD: warn and default to low on unrecognized values instead of silently mapping to low - Lint check for ### Variables now verifies positional placement under ## Configuration and extension, not just existence anywhere - Simplified docs/agents/review.md config instructions (removed confusing "set in two places" wording, added downgrade behavior) - Three new test cases covering verdict-downgrade scenarios Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/agents/review.md | 22 ++--- hack/lint-agent-docs | 4 +- .../scaffold/fullsend-repo/agents/review.md | 2 +- .../fullsend-repo/scripts/post-review-test.sh | 92 +++++++++++++++++++ .../fullsend-repo/scripts/post-review.sh | 24 +++++ 5 files changed, 128 insertions(+), 16 deletions(-) diff --git a/docs/agents/review.md b/docs/agents/review.md index 56fca0147e..4804010adc 100644 --- a/docs/agents/review.md +++ b/docs/agents/review.md @@ -76,19 +76,15 @@ See [Customizing with AGENTS.md](../guides/user/customizing-with-agents-md.md) a |----------|-------------|---------|--------------| | `REVIEW_FINDING_SEVERITY_THRESHOLD` | Minimum severity for findings to include in the review. Findings below this level are omitted from both the narrative body and the posted inline comments. | `low` | `info`, `low`, `medium`, `high`, `critical` | -This variable is read in two places: - -1. **Sandbox (agent inference):** The review agent reads it from the - environment and omits findings below the threshold from its output - (`body` and `findings` array). Set it in `env/review.env` or via the - CI workflow `env:` block. -2. **Post-script (runner):** The post-script filters the `findings` - array as defense-in-depth before posting. Set it in the CI workflow - `env:` block. - -Set the same value in both places. If they differ, the more restrictive -value wins for inline comments (post-script filters what the agent -already filtered). +Set this in the CI workflow `env:` block. The env file passes it to the +sandbox automatically, and the post-script reads it from the runner +environment directly — no separate configuration is needed. + +The review agent omits findings below the threshold from its output. The +post-script also filters the structured `findings` array as +defense-in-depth. When filtering removes all findings from a +`request-changes` verdict, the post-script downgrades the verdict to +`comment` (applying the `requires-manual-review` label). ## Source diff --git a/hack/lint-agent-docs b/hack/lint-agent-docs index f1ead8f01c..640ef70ac6 100755 --- a/hack/lint-agent-docs +++ b/hack/lint-agent-docs @@ -142,8 +142,8 @@ for yaml_file in "$HARNESS_DIR"/*.yaml; do continue fi - # Look for ### Variables outside fenced code blocks - if ! awk 'BEGIN{f=0} /^```/{f=1-f; next} f==0 && /^### Variables/{found=1} END{exit !found}' "$doc_path"; then + # Look for ### Variables under ## Configuration and extension (not just anywhere) + if ! awk 'BEGIN{f=0;c=0} /^```/{f=1-f;next} f{next} /^## Configuration and extension/{c=1;next} /^## /{c=0} c && /^### Variables/{found=1} END{exit !found}' "$doc_path"; then echo " $doc_basename: missing \"### Variables\" subsection under \"## Configuration and extension\"" errors=$((errors + 1)) else diff --git a/internal/scaffold/fullsend-repo/agents/review.md b/internal/scaffold/fullsend-repo/agents/review.md index 2581e2738f..552c556f83 100644 --- a/internal/scaffold/fullsend-repo/agents/review.md +++ b/internal/scaffold/fullsend-repo/agents/review.md @@ -66,7 +66,7 @@ them in the `findings` array. When unset, treat the threshold as `low`. This filtering applies to the narrative body text and the structured findings equally. If filtering removes all findings from a -`request-changes` verdict, downgrade the verdict to `approve`. +`request-changes` verdict, downgrade the verdict to `comment`. ## Identity diff --git a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh index efd29dcbd9..b371d253bc 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh @@ -187,6 +187,98 @@ NO_FINDINGS='{"action":"approve"}' run_filter_test "no-findings-key-passthrough" \ "$NO_FINDINGS" "low" "-1" +# --------------------------------------------------------------------------- +# Verdict-downgrade tests: when filtering empties all findings, the action +# must be downgraded from request-changes/reject to comment with findings +# key removed. +# Mirrors filter + downgrade logic in post-review.sh — keep in sync +# --------------------------------------------------------------------------- + +filter_and_downgrade() { + local result_json="$1" + local threshold="$2" + + local filtered + filtered="$(filter_findings_json "$result_json" "$threshold")" + local count + count="$(echo "$filtered" | jq 'if .findings then (.findings | length) else -1 end')" + + if [ "$count" -eq 0 ]; then + local action + action="$(echo "$filtered" | jq -r '.action')" + if [ "$action" = "request-changes" ] || [ "$action" = "reject" ]; then + echo "$filtered" | jq 'del(.findings) | .action = "comment"' + return + fi + # For approve/comment, just remove the empty findings array + echo "$filtered" | jq 'del(.findings)' + return + fi + echo "$filtered" +} + +run_downgrade_test() { + local test_name="$1" + local input_json="$2" + local threshold="$3" + local expected_action="$4" + local expected_has_findings="$5" + + local result + result="$(filter_and_downgrade "$input_json" "$threshold")" + local actual_action + actual_action="$(echo "$result" | jq -r '.action')" + local has_findings + has_findings="$(echo "$result" | jq 'has("findings")')" + + if [ "$actual_action" != "$expected_action" ] || [ "$has_findings" != "$expected_has_findings" ]; then + echo "FAIL: ${test_name}" + echo " expected action: '${expected_action}'" + echo " actual action: '${actual_action}'" + echo " expected has_findings: '${expected_has_findings}'" + echo " actual has_findings: '${has_findings}'" + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS: ${test_name}" +} + +# All findings are info-level; threshold=low removes them all → downgrade +ALL_INFO='{"action":"request-changes","findings":[ + {"severity":"info","category":"style","file":"a.go","description":"x"}, + {"severity":"info","category":"style","file":"b.go","description":"y"} +]}' + +run_downgrade_test "request-changes-all-filtered-downgrade" \ + "$ALL_INFO" "low" "comment" "false" + +# Same scenario with reject action +ALL_INFO_REJECT='{"action":"reject","findings":[ + {"severity":"info","category":"style","file":"a.go","description":"x"} +]}' + +run_downgrade_test "reject-all-filtered-downgrade" \ + "$ALL_INFO_REJECT" "low" "comment" "false" + +# Partial filtering: some findings remain → no downgrade +run_downgrade_test "request-changes-partial-filter-no-downgrade" \ + "$MIXED_FINDINGS" "medium" "request-changes" "true" + +# comment with all findings filtered → action stays comment, findings removed +COMMENT_ALL_INFO='{"action":"comment","body":"text","head_sha":"abc123","findings":[ + {"severity":"info","category":"style","file":"a.go","description":"x"} +]}' +run_downgrade_test "comment-all-filtered-removes-findings" \ + "$COMMENT_ALL_INFO" "low" "comment" "false" + +# approve with all findings filtered → action stays approve, findings removed +APPROVE_ALL_INFO='{"action":"approve","body":"LGTM","head_sha":"abc123","findings":[ + {"severity":"info","category":"style","file":"a.go","description":"x"} +]}' +run_downgrade_test "approve-all-filtered-removes-findings" \ + "$APPROVE_ALL_INFO" "low" "approve" "false" + # --------------------------------------------------------------------------- # Control-label guard tests # --------------------------------------------------------------------------- diff --git a/internal/scaffold/fullsend-repo/scripts/post-review.sh b/internal/scaffold/fullsend-repo/scripts/post-review.sh index ce93a8c705..31ec127a65 100755 --- a/internal/scaffold/fullsend-repo/scripts/post-review.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review.sh @@ -76,6 +76,12 @@ echo "Using result: ${RESULT_FILE}" # --------------------------------------------------------------------------- REVIEW_FINDING_SEVERITY_THRESHOLD="${REVIEW_FINDING_SEVERITY_THRESHOLD:-low}" +case "$REVIEW_FINDING_SEVERITY_THRESHOLD" in + info|low|medium|high|critical) ;; + *) echo "::warning::Invalid REVIEW_FINDING_SEVERITY_THRESHOLD='${REVIEW_FINDING_SEVERITY_THRESHOLD}', defaulting to 'low'" + REVIEW_FINDING_SEVERITY_THRESHOLD="low" ;; +esac + severity_rank() { case "$1" in info) echo 0 ;; @@ -108,6 +114,24 @@ if jq -e '.findings' "${RESULT_FILE}" >/dev/null 2>&1; then if [ "${filtered_count}" -lt "${original_count}" ]; then echo "Severity filter (threshold=${REVIEW_FINDING_SEVERITY_THRESHOLD}): kept ${filtered_count}/${original_count} findings" RESULT_FILE="${FILTERED_RESULT}" + + # If filtering removed all findings, delete the empty findings array + # (minItems: 1 in the schema). For request-changes/reject, also + # downgrade to comment — zero findings with a blocking verdict is + # semantically wrong. Use "comment" (not "approve") so the PR gets + # requires-manual-review, not ready-for-merge. + if [ "${filtered_count}" -eq 0 ]; then + original_action=$(jq -r '.action' "${FILTERED_RESULT}") + DOWNGRADE_RESULT=$(mktemp) + CLEANUP_FILES+=("${DOWNGRADE_RESULT}") + if [ "${original_action}" = "request-changes" ] || [ "${original_action}" = "reject" ]; then + echo "All findings removed by severity filter — downgrading '${original_action}' to 'comment'" + jq 'del(.findings) | .action = "comment"' "${FILTERED_RESULT}" > "${DOWNGRADE_RESULT}" + else + jq 'del(.findings)' "${FILTERED_RESULT}" > "${DOWNGRADE_RESULT}" + fi + RESULT_FILE="${DOWNGRADE_RESULT}" + fi else rm -f "${FILTERED_RESULT}" fi From 7c24049622eabb79640fab074ef6cabb1b976cbb Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 18 Jun 2026 13:53:00 -0400 Subject: [PATCH 8/9] fix(review): address round 2 review feedback on severity filter - Agent prompt: mention reject alongside request-changes in downgrade instruction - Agent prompt: clarify "if set to a non-empty value" instead of ambiguous "if set" - docs/agents/review.md: mention reject in downgrade behavior Rebase onto main already handled: CLEANUP_FILES pattern, label_actions integration, empty-findings del for approve/comment, and approve/comment test cases. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- docs/agents/review.md | 4 ++-- internal/scaffold/fullsend-repo/agents/review.md | 11 +++++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/docs/agents/review.md b/docs/agents/review.md index 4804010adc..2462750108 100644 --- a/docs/agents/review.md +++ b/docs/agents/review.md @@ -83,8 +83,8 @@ environment directly — no separate configuration is needed. The review agent omits findings below the threshold from its output. The post-script also filters the structured `findings` array as defense-in-depth. When filtering removes all findings from a -`request-changes` verdict, the post-script downgrades the verdict to -`comment` (applying the `requires-manual-review` label). +`request-changes` or `reject` verdict, the post-script downgrades the +verdict to `comment` (applying the `requires-manual-review` label). ## Source diff --git a/internal/scaffold/fullsend-repo/agents/review.md b/internal/scaffold/fullsend-repo/agents/review.md index 552c556f83..c30e683e07 100644 --- a/internal/scaffold/fullsend-repo/agents/review.md +++ b/internal/scaffold/fullsend-repo/agents/review.md @@ -55,18 +55,21 @@ NOTE: the Agent tool MUST ONLY be invoked with prompts read from ## Severity filtering -If `$REVIEW_FINDING_SEVERITY_THRESHOLD` is set, omit findings below -that severity level. The severity order from lowest to highest is: +If `$REVIEW_FINDING_SEVERITY_THRESHOLD` is set to a non-empty value, +use it as the minimum severity for findings to include. When unset or +empty, treat the threshold as `low`. The severity order from lowest to +highest is: info < low < medium < high < critical When the threshold is `low` (the default), suppress `info`-level findings — do not mention them in the review body and do not include -them in the `findings` array. When unset, treat the threshold as `low`. +them in the `findings` array. This filtering applies to the narrative body text and the structured findings equally. If filtering removes all findings from a -`request-changes` verdict, downgrade the verdict to `comment`. +`request-changes` or `reject` verdict, downgrade the verdict to +`comment`. ## Identity From 890e31dd45ff10d46abc4d618eaa0be5384fcb6d Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Thu, 18 Jun 2026 16:56:52 -0400 Subject: [PATCH 9/9] test: add severity filter integration tests through real post-review.sh Add integration tests that exercise the production severity filtering code path by running the real post-review.sh with REVIEW_FINDING_SEVERITY_THRESHOLD=medium and request-changes results containing only low-severity findings. Asserts the action is downgraded to comment and the requires-manual-review label is applied. Addresses round 3 review feedback on PR #2341. Signed-off-by: Ralph Bean Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- .../fullsend-repo/scripts/post-review-test.sh | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh index b371d253bc..b37279dfad 100644 --- a/internal/scaffold/fullsend-repo/scripts/post-review-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/post-review-test.sh @@ -573,6 +573,106 @@ run_label_test_stdout "label-actions-gha-delimiter-sanitized" \ '{"action":"approve","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"LGTM","label_actions":{"reason":"Injection.","actions":[{"action":"add","label":"::warning::injected"}]}}' \ "::warning::Skipping label ':warning:injected'" +# --- Severity filtering integration tests --- +# These invoke the real post-review.sh with REVIEW_FINDING_SEVERITY_THRESHOLD +# set to a non-default value, exercising the production severity_rank() and jq +# filter rather than the mirrored copies above. + +run_label_test_with_env() { + local test_name="$1" + local json_content="$2" + local expected_pattern="$3" + local env_var="$4" + local env_val="$5" + + local run_dir="${TMPDIR}/run-${test_name}" + mkdir -p "${run_dir}/iteration-1/output" + echo "${json_content}" > "${run_dir}/iteration-1/output/agent-result.json" + : > "${GH_LOG}" + + local exit_code=0 + # shellcheck disable=SC2030,SC2031 + ( + cd "${run_dir}" + export PATH="${MOCK_BIN}:${PATH}" + export REVIEW_TOKEN="fake-token" + export PR_NUMBER="99" + export REPO_FULL_NAME="test-org/test-repo" + export "${env_var}=${env_val}" + bash "${POST_SCRIPT}" + ) > "${TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$? + + if [[ ${exit_code} -ne 0 ]]; then + echo "FAIL: ${test_name} — exit code ${exit_code}" + cat "${TMPDIR}/stdout-${test_name}.log" + FAILURES=$((FAILURES + 1)) + return + fi + + if ! grep -qF "${expected_pattern}" "${GH_LOG}"; then + echo "FAIL: ${test_name} — expected pattern '${expected_pattern}' not found in gh calls" + echo "Actual calls:" + cat "${GH_LOG}" + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS: ${test_name}" +} + +run_label_test_with_env "severity-filter-downgrade-integration" \ + '{"action":"request-changes","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"Issues found","findings":[{"severity":"low","category":"style","file":"a.go","description":"minor"}]}' \ + "requires-manual-review" \ + "REVIEW_FINDING_SEVERITY_THRESHOLD" "medium" + +# Verify stdout mentions the downgrade +run_label_test_with_env_stdout() { + local test_name="$1" + local json_content="$2" + local expected_stdout="$3" + local env_var="$4" + local env_val="$5" + + local run_dir="${TMPDIR}/run-${test_name}" + mkdir -p "${run_dir}/iteration-1/output" + echo "${json_content}" > "${run_dir}/iteration-1/output/agent-result.json" + : > "${GH_LOG}" + + local exit_code=0 + # shellcheck disable=SC2030,SC2031 + ( + cd "${run_dir}" + export PATH="${MOCK_BIN}:${PATH}" + export REVIEW_TOKEN="fake-token" + export PR_NUMBER="99" + export REPO_FULL_NAME="test-org/test-repo" + export "${env_var}=${env_val}" + bash "${POST_SCRIPT}" + ) > "${TMPDIR}/stdout-${test_name}.log" 2>&1 || exit_code=$? + + if [[ ${exit_code} -ne 0 ]]; then + echo "FAIL: ${test_name} — exit code ${exit_code}" + cat "${TMPDIR}/stdout-${test_name}.log" + FAILURES=$((FAILURES + 1)) + return + fi + + if ! grep -qF "${expected_stdout}" "${TMPDIR}/stdout-${test_name}.log"; then + echo "FAIL: ${test_name} — expected stdout '${expected_stdout}' not found" + echo "Actual stdout:" + cat "${TMPDIR}/stdout-${test_name}.log" + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS: ${test_name}" +} + +run_label_test_with_env_stdout "severity-filter-downgrade-log-message" \ + '{"action":"request-changes","pr_number":99,"repo":"test-org/test-repo","head_sha":"abc123","body":"Issues found","findings":[{"severity":"low","category":"style","file":"a.go","description":"minor"}]}' \ + "All findings removed by severity filter" \ + "REVIEW_FINDING_SEVERITY_THRESHOLD" "medium" + # --- Summary --- echo ""