feat(review): add REVIEW_FINDING_SEVERITY_THRESHOLD config var - #2341
Conversation
Site previewPreview: https://6e3e555e-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 Finished Review · ✅ Success · Started 4:04 PM UTC · Completed 4:18 PM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR modifies review agent scripts, prompts, env config, and agent docs Previous run (2)ReviewFindingsLow
Previous run (3)ReviewFindingsHigh
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
|
||
| threshold_rank=$(severity_rank "$REVIEW_FINDING_SEVERITY_THRESHOLD") | ||
|
|
||
| if jq -e '.findings' "${RESULT_FILE}" >/dev/null 2>&1; then |
There was a problem hiding this comment.
[high] logic-error
The post-script filters findings below the severity threshold but never downgrades the action when all findings are removed. If filtering removes all findings from a request-changes result, the result will contain {action: request-changes, findings: []} — an empty array that violates the schema's minItems: 1 constraint. The agent prompt instructs downgrade to 'approve' but the post-script does not implement this, and 'approve' is a weaker safety posture than 'comment'.
Suggested fix: After filtering, check if findings array is empty and action is request-changes. If so, rewrite action to 'comment' (not 'approve') and remove the empty findings key. Align the agent prompt to also use 'comment' instead of 'approve'.
There was a problem hiding this comment.
Fixed in 2174998. Post-script now downgrades to comment (not approve) when filtering empties the findings array, and deletes the findings key so we don't violate minItems: 1.
| echo " actual count: '${actual_count}'" | ||
| FAILURES=$((FAILURES + 1)) | ||
| return | ||
| fi |
There was a problem hiding this comment.
[medium] test-inadequate
The severity-filter tests verify finding counts but do not test the verdict-downgrade scenario: when filtering removes all findings from a request-changes action. The filter function is duplicated in the test rather than sourced from post-review.sh.
There was a problem hiding this comment.
Added three test cases in 2174998 — request-changes with all findings filtered, reject with all filtered, and partial filtering (no downgrade).
| 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. | ||
|
|
There was a problem hiding this comment.
[low] documentation-accuracy
The docs state the post-script 'filters the findings array as defense-in-depth' but do not mention the post-script is missing verdict-downgrade logic.
There was a problem hiding this comment.
Updated the docs in 2174998 — now describes the downgrade behavior directly.
| info) echo 0 ;; | ||
| low) echo 1 ;; | ||
| medium) echo 2 ;; | ||
| high) echo 3 ;; |
There was a problem hiding this comment.
[low] edge-case
severity_rank maps unrecognized severity values to rank 1 (same as low). Misspelled severity strings silently treated as low rather than flagged.
There was a problem hiding this comment.
The input validation on the threshold value (added in 2174998) covers this — unrecognized strings now get a ::warning:: and fall back to low before severity_rank is ever called.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad Report — 6 agents (2x claude-coder, 2x claude-researcher, 1x gemini-code-review, 1x cursor-code-review) · Models: Claude, Gemini, Codex
5 new inline comments posted (MEDIUM+). 2 findings already covered by the existing fullsend-ai-review comments (verdict downgrade at post-review.sh:90, test coverage at post-review-test.sh:155) — skipped to avoid duplicates.
Summary: The severity filtering implementation is clean for the happy path, but the post-script is missing the verdict-downgrade logic when filtering empties all findings (H1, already flagged), and the downgrade target should be comment not approve (H2, new). Input validation on the threshold value (M1) is the most broadly agreed-upon fix across all 6 agents.
|
|
||
| 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`. |
There was a problem hiding this comment.
[high] safety — downgrade target should be comment, not approve
The prompt instructs the agent to downgrade to approve when filtering removes all findings. But approve triggers the ready-for-merge label (post-review.sh:242) and signals the PR is safe to merge. A PR that originally had request-changes findings — but all fell below the severity threshold — should not get an explicit approval.
The safer target is comment, which yields the requires-manual-review label: "the review ran, no blocking findings remain, but a human should still look."
Suggestion: Change to: "downgrade the verdict to comment"
Flagged by 2/6 review agents (Claude) — consensus
There was a problem hiding this comment.
Agreed — comment is the right target. Changed in 2174998.
| # 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}" |
There was a problem hiding this comment.
[medium] input-validation — no validation on REVIEW_FINDING_SEVERITY_THRESHOLD
The variable defaults to low if unset but is never validated. A typo like hgih or uppercase HIGH silently maps to rank 1 (low) via the severity_rank wildcard case, giving different behavior than intended with no warning.
Suggestion:
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" ;;
esacFlagged by 5/6 review agents (Claude, Gemini, Codex) — strong consensus
There was a problem hiding this comment.
Added the validation with a ::warning:: in 2174998. Falls back to low on unrecognized values.
| # --------------------------------------------------------------------------- | ||
| # 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 |
There was a problem hiding this comment.
[medium] documentation — defense-in-depth limitation not documented
The post-script filters the structured findings array but cannot filter the narrative body text (free-form markdown). If the agent's prompt-based filtering fails — the exact scenario defense-in-depth covers — below-threshold findings will still appear in the review body while their inline comments are suppressed.
Suggestion: Add to the comment block:
# Note: this only filters the structured findings array. Narrative body
# filtering relies solely on the agent prompt.
Flagged by 1/6 review agents (Claude) — single-agent finding
There was a problem hiding this comment.
I'm going to leave this one as-is. The comment already says "defense-in-depth" which communicates that it's a partial backstop. Documenting every thing it doesn't do feels like it'd grow without bound.
| else | ||
| echo " $doc_basename: OK" | ||
| fi | ||
| done |
There was a problem hiding this comment.
[medium] lint-gap — ### Variables check is not positional
The awk command checks that ### Variables exists anywhere in the file outside fenced code blocks. It does not verify it appears between ## Configuration and extension and the next ## heading. A doc with ### Variables under the wrong section would pass.
Suggestion: Combine into a single awk pass that tracks section context:
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"Flagged by 4/6 review agents (Claude, Gemini) — consensus
There was a problem hiding this comment.
Good catch — made the awk check positional in 2174998. It now tracks whether we're inside ## Configuration and extension before looking for ### Variables.
| 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 |
There was a problem hiding this comment.
[medium] documentation — docs imply two-place configuration unnecessarily
The docs say "Set the same value in both places." In practice, the user only needs to set it once in the workflow env: block — review.env passes it through to the sandbox via export REVIEW_FINDING_SEVERITY_THRESHOLD="${REVIEW_FINDING_SEVERITY_THRESHOLD}", and the post-script reads it from the runner environment directly. The current wording creates unnecessary confusion.
Suggestion: Simplify to:
Set
REVIEW_FINDING_SEVERITY_THRESHOLDin the CI workflowenv:block. The env file passes it to the sandbox automatically, and the post-script reads it from the runner environment. No separate configuration is needed.
Also mention the verdict-downgrade behavior: when filtering removes all findings from a request-changes verdict, the review is downgraded to comment (requires manual review).
Flagged by 2/6 review agents (Claude) — consensus
There was a problem hiding this comment.
Simplified in 2174998. Dropped the two-places wording and added the downgrade behavior.
…lback Per human feedback on PR #2415: all findings whose line is outside a diff hunk now fall back to file-level comments, not just medium+. Removed isMediumPlusSeverity() helper and info-severity filtering — severity-based filtering will be handled by a separate configuration variable introduced in #2341. Addresses review feedback on #2415
f33d467 to
2174998
Compare
ralphbean
changed the title
|
🤖 Finished Review · ✅ Success · Started 2:37 PM UTC · Completed 2:53 PM UTC |
| *) echo "::warning::Invalid REVIEW_FINDING_SEVERITY_THRESHOLD='${REVIEW_FINDING_SEVERITY_THRESHOLD}', defaulting to 'low'" | ||
| REVIEW_FINDING_SEVERITY_THRESHOLD="low" ;; | ||
| esac | ||
|
|
There was a problem hiding this comment.
[low] edge-case
severity_rank maps unrecognized severity values to rank 1 (same as low). A misspelled severity string would be silently treated as low-severity. Risk is minimal since findings come from schema-validated JSON.
There was a problem hiding this comment.
Covered by the input validation at lines 79–83 (added in 2174998). Unrecognized values get a ::warning:: and fall back to low before severity_rank is called.
fullsend-ai-review
Bot
added
the
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — Round 2
Agents: 4 (Claude ×2, Gemini, Codex)
Findings posted: 7 (1 critical, 1 high, 4 medium, 1 low)
Skipped (already posted from round 1): input-validation, lint-gap, docs two-place config, defense-in-depth limitation
New findings this round
| # | Sev | Finding | Consensus |
|---|---|---|---|
| 1 | critical | Branch conflicts with main — missing label_actions + CLEANUP_FILES |
1/4 (verified) |
| 2 | high | Empty findings: [] on approve/comment violates schema minItems: 1 |
3/4 |
| 3 | medium | Agent prompt omits reject from downgrade instruction |
2/4 |
| 4 | medium | No test coverage for approve/comment with all findings filtered | 2/4 |
| 5 | medium | Duplicated severity-ranking logic across 3 locations | 3/4 |
| 6 | medium | Agent prompt "if set" ambiguous with empty string from env | 1/4 |
| 7 | low | DOWNGRADE_RESULT temp file not in trap cleanup |
2/4 |
Prior round status
Author responded to 9/10 threads from round 1 claiming fixes in 2174998, but 0/10 threads are resolved on GitHub. Thread 7 (defense-in-depth docs) was intentionally declined. Thread 10 (new fullsend-ai-review finding) has no response yet.
Assisted-by: Claude (review), Gemini (review), Codex (review)
| set -euo pipefail | ||
|
|
||
| FILTERED_RESULT="" MODIFIED_RESULT="" | ||
| trap 'rm -f "${FILTERED_RESULT:-}" "${MODIFIED_RESULT:-}"' EXIT |
There was a problem hiding this comment.
[critical] rebase-required — branch conflicts with main
The PR merge state is CONFLICTING. Main has been refactored to use a CLEANUP_FILES=() array pattern with trap 'rm -f "${CLEANUP_FILES[@]}"' EXIT and CLEANUP_FILES+=() accumulation. Main also added an entire label_actions validation section (~65 lines for sanitizing label names, validating control labels, checking repo label existence). Both are missing from this branch.
After rebasing, the severity-filtering temp files (FILTERED_RESULT, DOWNGRADE_RESULT) should integrate into the CLEANUP_FILES+=() pattern instead of the separate trap here.
Flagged by 1/4 review agents (Claude) — verified via git show origin/main and gh pr view --json mergeable
There was a problem hiding this comment.
Rebased onto main in 7c24049. CLEANUP_FILES pattern integrated, label_actions processing picked up cleanly.
| if [ "${filtered_count}" -eq 0 ]; then | ||
| original_action=$(jq -r '.action' "${FILTERED_RESULT}") | ||
| if [ "${original_action}" = "request-changes" ] || [ "${original_action}" = "reject" ]; then | ||
| echo "All findings removed by severity filter — downgrading '${original_action}' to 'comment'" | ||
| DOWNGRADE_RESULT=$(mktemp) | ||
| jq 'del(.findings) | .action = "comment"' "${FILTERED_RESULT}" > "${DOWNGRADE_RESULT}" | ||
| RESULT_FILE="${DOWNGRADE_RESULT}" | ||
| rm -f "${FILTERED_RESULT}" | ||
| FILTERED_RESULT="${DOWNGRADE_RESULT}" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
[high] schema-violation — empty findings: [] left for approve/comment actions
The downgrade logic only handles request-changes and reject. If an approve or comment result has findings (e.g., info-level actionable findings) and filtering removes all of them, the result retains "findings": []. The schema specifies "minItems": 1 for findings (schemas/review-result.schema.json), so this empty array fails schema validation and triggers a harness retry.
Suggested fix: After filtering, unconditionally del(.findings) when count is 0 for all actions, not just request-changes/reject. Only downgrade the action for request-changes/reject:
if [ "${filtered_count}" -eq 0 ]; then
original_action=$(jq -r '.action' "${FILTERED_RESULT}")
DOWNGRADE_RESULT=$(mktemp)
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}"
rm -f "${FILTERED_RESULT}"
FILTERED_RESULT="${DOWNGRADE_RESULT}"
fiFlagged by 3/4 review agents (Claude, Gemini) — strong consensus
There was a problem hiding this comment.
Fixed in 7c24049. The post-script now unconditionally del(.findings) when filtered_count is 0, regardless of action. Added test cases for comment and approve with all findings filtered.
|
|
||
| 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`. |
There was a problem hiding this comment.
[medium] prompt-gap — downgrade instruction omits reject verdict
The prompt says "If filtering removes all findings from a request-changes verdict, downgrade the verdict to comment." but the post-script also downgrades reject (line 121). Without this, the agent could produce a reject verdict with zero findings.
Suggestion: Update to: "If filtering removes all findings from a request-changes or reject verdict, downgrade the verdict to comment."
Flagged by 2/4 review agents (Claude, Codex) — consensus
There was a problem hiding this comment.
Fixed in 7c24049. Prompt now says "request-changes or reject."
| 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" |
There was a problem hiding this comment.
[medium] test-gap — no coverage for approve/comment with all findings filtered
Tests cover request-changes and reject downgrade but not approve or comment actions with findings that get completely filtered. This is the path that triggers the empty findings: [] schema violation (see companion finding on post-review.sh).
Suggestion: Add test cases:
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_ALL_INFO='{"action":"approve","body":"LGTM","head_sha":"abc123","findings":[
{"severity":"info","category":"style","file":"a.go","description":"x","actionable":true}
]}'
run_downgrade_test "approve-all-filtered-removes-findings" \
"$APPROVE_ALL_INFO" "low" "approve" "false"Flagged by 2/4 review agents (Claude, Gemini) — consensus
There was a problem hiding this comment.
Added in 7c24049 — comment-all-filtered-removes-findings and approve-all-filtered-removes-findings.
|
|
||
| severity_rank() { | ||
| case "$1" in | ||
| info) echo 0 ;; | ||
| low) echo 1 ;; | ||
| medium) echo 2 ;; | ||
| high) echo 3 ;; | ||
| critical) echo 4 ;; | ||
| *) echo 1 ;; | ||
| esac |
There was a problem hiding this comment.
[medium] maintenance — duplicated severity-ranking logic
severity_rank() and the jq filter are re-implemented here rather than sourced from post-review.sh. If the production logic changes (e.g., new severity levels, rank order), the test copy must be manually kept in sync. This matches the existing pattern in this test file (header says "reimplements... so we can test it without network access"), so it's a known tradeoff.
Suggestion: Add a cross-reference comment: # Mirrors severity_rank() in post-review.sh — keep in sync
Flagged by 3/4 review agents (Claude, Codex) — strong consensus
There was a problem hiding this comment.
Added cross-ref comment during the rebase: "Mirrors severity_rank() in post-review.sh — keep in sync".
There was a problem hiding this comment.
Already there — line 104 has # Mirrors severity_rank() in post-review.sh — keep in sync, added in 7c24049.
| 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`. |
There was a problem hiding this comment.
[medium] prompt-ambiguity — "If set" vs empty string
The prompt says "If $REVIEW_FINDING_SEVERITY_THRESHOLD is set" but review.env always exports it — potentially as an empty string when the user doesn't configure it. The agent sees an empty string, not an unset variable, and the phrasing "If set" could confuse the model into applying filtering with an empty threshold.
Suggestion: Change to: "If $REVIEW_FINDING_SEVERITY_THRESHOLD is set to a non-empty value, use that as the minimum severity. When unset or empty, treat the threshold as low."
Flagged by 1/4 review agents (Claude) — single-agent finding
There was a problem hiding this comment.
Fixed in 7c24049. Changed to "If set to a non-empty value" with explicit "When unset or empty, treat the threshold as low."
There was a problem hiding this comment.
Fixed in 7c24049. Prompt now reads "set to a non-empty value... When unset or empty, treat the threshold as low."
| FILTERED_RESULT="" MODIFIED_RESULT="" | ||
| trap 'rm -f "${FILTERED_RESULT:-}" "${MODIFIED_RESULT:-}"' EXIT |
There was a problem hiding this comment.
[low] cleanup-gap — DOWNGRADE_RESULT temp file not in trap
DOWNGRADE_RESULT is created at line 123 via mktemp but not declared in the trap here. It's cleaned up indirectly via FILTERED_RESULT="${DOWNGRADE_RESULT}" reassignment at line 127, which is subtle — a future maintainer may not realize the cleanup depends on that reassignment.
Suggestion: Moot after rebase (should use CLEANUP_FILES+=() from main), but if the current pattern is kept: add DOWNGRADE_RESULT="" to line 21 and include it in the trap.
Flagged by 2/4 review agents (Claude, Codex) — consensus
There was a problem hiding this comment.
Moot after rebase — DOWNGRADE_RESULT now uses CLEANUP_FILES+=() like everything else.
There was a problem hiding this comment.
Moot after the rebase — DOWNGRADE_RESULT now uses CLEANUP_FILES+=() at line 126.
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
…filter 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 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
- 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 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
2174998 to
7c24049
Compare
|
🤖 Finished Review · ✅ Success · Started 6:01 PM UTC · Completed 6:14 PM UTC |
| # --------------------------------------------------------------------------- | ||
| REVIEW_FINDING_SEVERITY_THRESHOLD="${REVIEW_FINDING_SEVERITY_THRESHOLD:-low}" | ||
|
|
||
| case "$REVIEW_FINDING_SEVERITY_THRESHOLD" in |
There was a problem hiding this comment.
[medium] injection
The REVIEW_FINDING_SEVERITY_THRESHOLD value is interpolated unsanitized into a GHA ::warning:: workflow command in the invalid-value catch-all branch. If the environment variable contains embedded newlines, the echo output could inject additional GHA workflow commands. The same script already sanitizes label values before interpolation into workflow commands, establishing the expected pattern.
Suggested fix: Sanitize the value before interpolating it into the ::warning:: message — strip newlines, carriage returns, and :: delimiters, matching the sanitization already applied to label values later in the same script.
| 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" ;; |
There was a problem hiding this comment.
[low] edge-case
severity_rank maps unrecognized severity values to rank 1 (same as low). The jq filter else 1 branch has the same behavior. In practice, findings come from a schema-validated JSON file where severity is constrained, so the risk is minimal.
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad — Round 3
Agents: 5 (Claude ×3, Gemini, Codex)
New findings posted: 1 (medium)
Already posted (skipped): injection in ::warning:: (posted by fullsend-ai-review bot, awaiting response)
False positives removed: 2 (empty array set -u from Codex — pre-existing code from main; request_changes vs request-changes mismatch from Gemini — also pre-existing)
Code is in good shape after two rounds of fixes. The one new medium finding is the lack of integration test coverage for the severity filtering path through the real post-review.sh.
Assisted-by: Claude (review), Gemini (review), Codex (review)
| info) echo 0 ;; | ||
| low) echo 1 ;; | ||
| medium) echo 2 ;; | ||
| high) echo 3 ;; |
There was a problem hiding this comment.
[medium] test-gap — severity filter tests exercise mirrored logic, not production code
The severity_rank(), filter_findings_json(), and filter_and_downgrade() functions here are reimplementations of the production logic in post-review.sh. These tests pass — but they're testing their own copy, not the real script. If the production jq filter or severity_rank() diverges (e.g., a new severity level added to production but not here), these tests still pass while production behaves differently.
The integration tests at the bottom of this file (via run_label_test) do invoke the real post-review.sh, but none of them set REVIEW_FINDING_SEVERITY_THRESHOLD to a non-default value with below-threshold findings. A regression in the interaction between severity filtering and subsequent logic (protected-path checks, label-actions, fullsend post-review invocation) would not be caught.
Suggestion: Add at least one integration test that exports REVIEW_FINDING_SEVERITY_THRESHOLD=medium with a request-changes result containing only low-severity findings, then asserts: exit 0, action downgraded to comment, and requires-manual-review label applied.
Flagged by 3/5 review agents (Claude ×3) — strong consensus
There was a problem hiding this comment.
Added integration tests in 890e31d. Two new tests run the real post-review.sh with REVIEW_FINDING_SEVERITY_THRESHOLD=medium and a request-changes result with only low findings — asserts the downgrade to comment and the requires-manual-review label.
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 <rbean@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 9:00 PM UTC · Completed 9:14 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
LGTM — all findings from 4 rounds of multi-agent review have been addressed.
The severity filtering implementation is correct: threshold validation with safe fallback, proper jq filtering pipeline, verdict downgrade to comment for request-changes/reject, CLEANUP_FILES integration, and comprehensive test coverage including integration tests against the real post-review.sh.
One minor open item from the prior round (unsanitized env var in ::warning:: at post-review.sh:81) — low practical risk since the variable is set by repo owners, not PR authors.
Assisted-by: Claude (review), Gemini (review), Codex (review)
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Finished Retro · ✅ Success · Started 9:35 PM UTC · Completed 9:44 PM UTC |
Retro Analysis: PR #2341 — Review severity thresholdWorkflow went well overall. The review agent and human reviewer caught real bugs across 5 review rounds, including a safety-critical issue (verdict downgrade to Timeline
Final review agent run (dispatch Improvement opportunities (all covered by existing issues)
No new proposals filed — all identified patterns are already tracked by existing open issues. |
2 participants
Summary
REVIEW_FINDING_SEVERITY_THRESHOLDconfig var for the review agent, letting repo owners suppress low-severity findings (default:low, which dropsinfo-level findings)findingsarray as defense-in-depth before posting inline commentsrequest-changes/rejecttocommentwhen filtering removes all findings### Variablessubsection to alldocs/agents/*.mdfileshack/lint-agent-docsto require the### Variablessubsection under## Configuration and extensionTest plan
./hack/lint-agent-docspasses with new### Variablescheckmake script-testpasses (18 post-review tests including 6 severity filter + 3 verdict-downgrade tests)make lint— clean🤖 Generated with Claude Code