fix(#1046): key review verdict on finding actionability, not just severity

agents/review.md

@@ -68,7 +68,8 @@ review body and do not include 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` or `reject` verdict, downgrade the verdict to
-`comment`.
+`comment`. The severity threshold is absolute — it applies to all
+findings regardless of the `actionable` flag.
 
 ## Identity
 
@@ -201,15 +202,18 @@ mutations on the runner.
 
 ### Outcome
 
-- `approve` — no medium+ findings; the change is safe (low/info
-  findings may be attached as comments)
+- `approve` — no medium+ findings and no findings with `actionable: true`
+  and a non-empty `remediation`; the change is safe (low/info findings
+  may be attached as comments)
 - `request-changes` — findings *requiring* resolution: one or more critical or
   high findings; one or more medium-severity findings identifying a
   functional bug (incorrect behavior, permission error, schema violation,
-  or silent failure). If the summary text states findings should be
-  addressed, fixed, or resolved before merge, the verdict must be
-  `request-changes`, not `comment` — the summary language and the
-  verdict action must be consistent.
+  or silent failure); or any finding (regardless of severity) with
+  `actionable: true` and a non-empty `remediation` (the fix agent can
+  address these automatically). If the summary text states findings
+  should be addressed, fixed, or resolved before merge, the verdict
+  must be `request-changes`, not `comment` — the summary language and
+  the verdict action must be consistent.
 - `comment-only` — medium-severity findings worth noting but none
   that should block. Use only when medium findings are stylistic,
   advisory, or process-related — not when any medium finding identifies
@@ -220,9 +224,9 @@ mutations on the runner.
 - `failure` — review could not be completed (tool failure, missing
   context, ambiguous findings)
 
-When the change is safe and the only findings are low or info severity,
-approve the PR and mark concrete follow-up work as `actionable: true`
-in the structured result so the post-script can create tracking issues.
+When the change is safe and no findings have `actionable: true` with a
+non-empty `remediation`, approve the PR. Observations, confirmations,
+and analysis notes at any severity level do not block.
 
 The `code-review` skill defines the finding structure. The `pr-review`
 skill defines the review comment format and procedure.
@@ -271,7 +275,7 @@ fields such as `outcome`, `summary`, `prior_review_sha`, or
 | `line`        | integer | no       | Line number (minimum 1)                       |
 | `description` | string  | yes      | Finding description (min 1 char)              |
 | `remediation` | string  | no       | Suggested fix                                 |
-| `actionable`  | boolean | no       | When true on low/info findings in an `approve` result, marks the finding for future follow-up issue creation (temporarily disabled; see #1137) |
+| `actionable`  | boolean | no       | When true with a non-empty `remediation`, routes the verdict to `request-changes` so the fix agent can address the finding automatically (follow-up issue creation is temporarily disabled; see #1137) |
 
 Schema validation failures trigger a harness retry iteration. The jq
 examples below show the exact JSON shape for each action.
@@ -290,22 +294,7 @@ jq -n \
   > "$FULLSEND_OUTPUT_DIR/agent-result.json"
 ```
 
-For `approve` with actionable low/info findings:
-
-```bash
-jq -n \
-  --arg action "approve" \
-  --argjson pr_number <number> \
-  --arg repo "<owner/repo>" \
-  --arg head_sha "<sha>" \
-  --arg body "<markdown review comment>" \
-  --argjson findings '<findings array>' \
-  '{action: $action, pr_number: $pr_number, repo: $repo,
-    head_sha: $head_sha, body: $body, findings: $findings}' \
-  > "$FULLSEND_OUTPUT_DIR/agent-result.json"
-```
-
-For `request-changes` or `reject`:
+For `request-changes` (including actionable low/info findings) or `reject`:
 
 ```bash
 jq -n \

docs/review.md

@@ -111,6 +111,8 @@ Override any variable by extending the harness file via a `base` reference and s
 
 When severity filtering removes all findings from a negative review verdict, the
 verdict is downgraded to a comment (applying the `requires-manual-review` label).
+The severity threshold is absolute — it applies to all findings regardless of
+the `actionable` flag, respecting the user's configured threshold throughout.
 
 ### GitLab host validation
 

schemas/review-result.schema.json

@@ -115,7 +115,7 @@
         "remediation": { "type": "string" },
         "actionable": {
           "type": "boolean",
-          "description": "True when this non-blocking finding should be tracked as a follow-up issue if the review approves."
+          "description": "When true with a non-empty remediation, routes the verdict to request-changes so the fix agent can address the finding automatically."
         }
       },
       "additionalProperties": false

scripts/post-review-test.sh

@@ -299,6 +299,45 @@ APPROVE_ALL_INFO='{"action":"approve","body":"LGTM","head_sha":"abcdef0123456789
 run_downgrade_test "approve-all-filtered-removes-findings" \
   "$APPROVE_ALL_INFO" "low" "approve" "false"
 
+# ---------------------------------------------------------------------------
+# Severity-threshold downgrade tests with actionable findings: the severity
+# threshold is absolute — actionable findings below the threshold are
+# filtered out and the verdict is downgraded, respecting the user's
+# configured threshold.
+# ---------------------------------------------------------------------------
+
+# request-changes with actionable low findings filtered → downgraded
+# (severity threshold is respected even for actionable findings)
+ACTIONABLE_LOW='{"action":"request-changes","findings":[
+  {"severity":"low","category":"naming-convention","file":"a.go","description":"rename type","remediation":"rename FooBar to fooBar","actionable":true}
+]}'
+run_downgrade_test "request-changes-actionable-filtered-downgraded" \
+  "$ACTIONABLE_LOW" "medium" "comment" "false"
+
+# request-changes with mixed actionable/non-actionable info findings
+# filtered → downgraded (severity threshold applies to all findings)
+MIXED_ACTIONABLE='{"action":"request-changes","findings":[
+  {"severity":"info","category":"style","file":"a.go","description":"security: no SSRF bypass","actionable":false},
+  {"severity":"low","category":"naming-convention","file":"b.go","description":"rename type","remediation":"rename FooBar to fooBar","actionable":true}
+]}'
+run_downgrade_test "request-changes-mixed-actionable-filtered-downgraded" \
+  "$MIXED_ACTIONABLE" "medium" "comment" "false"
+
+# request-changes with all non-actionable low findings filtered → downgraded
+NON_ACTIONABLE_LOW='{"action":"request-changes","findings":[
+  {"severity":"low","category":"style","file":"a.go","description":"observation","actionable":false},
+  {"severity":"info","category":"style","file":"b.go","description":"note","actionable":false}
+]}'
+run_downgrade_test "request-changes-non-actionable-downgraded" \
+  "$NON_ACTIONABLE_LOW" "medium" "comment" "false"
+
+# reject with actionable findings filtered → downgraded
+ACTIONABLE_REJECT='{"action":"reject","findings":[
+  {"severity":"info","category":"style","file":"a.go","description":"rename","remediation":"fix it","actionable":true}
+]}'
+run_downgrade_test "reject-actionable-filtered-downgraded" \
+  "$ACTIONABLE_REJECT" "low" "comment" "false"
+
 # ---------------------------------------------------------------------------
 # Control-label guard tests
 # ---------------------------------------------------------------------------
@@ -916,6 +955,18 @@ run_label_test_with_env_stdout "severity-filter-downgrade-log-message" \
   "All findings removed by severity filter" \
   "REVIEW_FINDING_SEVERITY_THRESHOLD" "medium"
 
+# Actionable low findings below threshold → downgraded (threshold is absolute)
+run_label_test_with_env_stdout "severity-filter-actionable-still-downgrades" \
+  '{"action":"request-changes","pr_number":99,"repo":"test-org/test-repo","head_sha":"abcdef0123456789abcdef0123456789abcdef01","body":"Issues found","findings":[{"severity":"low","category":"naming-convention","file":"a.go","description":"rename type","remediation":"rename FooBar to fooBar","actionable":true}]}' \
+  "All findings removed by severity filter" \
+  "REVIEW_FINDING_SEVERITY_THRESHOLD" "medium"
+
+# Non-actionable low findings below threshold → downgraded
+run_label_test_with_env_stdout "severity-filter-non-actionable-downgrades" \
+  '{"action":"request-changes","pr_number":99,"repo":"test-org/test-repo","head_sha":"abcdef0123456789abcdef0123456789abcdef01","body":"Issues found","findings":[{"severity":"low","category":"style","file":"a.go","description":"minor","actionable":false}]}' \
+  "All findings removed by severity filter" \
+  "REVIEW_FINDING_SEVERITY_THRESHOLD" "medium"
+
 # --- Severity-threshold sanitization tests ---
 # Invalid REVIEW_FINDING_SEVERITY_THRESHOLD values are echoed into a GHA
 # `::error::` workflow command. Verify the sanitizer neutralizes both

scripts/post-review.sh

@@ -568,8 +568,9 @@ if jq -e '.findings' "${RESULT_FILE}" >/dev/null 2>&1; then
     # 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.
+    # semantically wrong. The threshold is absolute: even actionable
+    # findings are filtered (#1046). 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)

scripts/post-review.src.sh

@@ -158,8 +158,9 @@ if jq -e '.findings' "${RESULT_FILE}" >/dev/null 2>&1; then
     # 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.
+    # semantically wrong. The threshold is absolute: even actionable
+    # findings are filtered (#1046). 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)

skills/code-review/SKILL.md

@@ -231,11 +231,11 @@ For each issue identified, record:
   the match. If the content does not match, find the correct line. If
   you cannot determine the correct line, omit it rather than guessing.
 - **Remediation:** suggested fix or action (required for critical/high)
-- **Actionable:** whether the finding should become tracked follow-up
-  work if the PR is approved. Use `true` only for concrete low/info
-  items that can be fixed independently after merge. Use `false` for
-  observations, praise, broad suggestions, and anything already handled
-  by the PR.
+- **Actionable:** whether the finding has a concrete remediation the fix
+  agent can address automatically. When `true` with a non-empty
+  `remediation`, routes the verdict to `request-changes`. Use `false`
+  for observations, praise, broad suggestions, and anything already
+  handled by the PR.
 
 **Cross-file finding self-check:** Before recording any finding that
 asserts what a specific file contains, verify that you read that file
@@ -290,15 +290,17 @@ Then determine the overall outcome:
 - One or more **medium** findings identifying a functional bug
   (incorrect behavior, permission error, schema violation, or silent
   failure) -> `request-changes`
+- Any finding (regardless of severity) with `actionable: true` and a
+  non-empty `remediation` -> `request-changes` (these have concrete
+  remediations the fix agent can address automatically)
 - One or more **medium** findings that are all
   stylistic/advisory/process-related (no functional bugs) ->
   `comment-only` (attach findings as comments in the review body so the
   author sees them, but do not block the PR)
-- **Low** or **info** findings only (no medium+) -> `approve` (attach
+- **Low** or **info** findings only (no medium+), none with
+  `actionable: true` and a non-empty `remediation` -> `approve` (attach
   findings as comments in the review body so the author sees them, but
-  do not block the PR). Preserve concrete follow-up work in the structured
-  output with `actionable: true` (follow-up issue creation is temporarily
-  disabled pending #1137, but the field is retained for when it is re-enabled).
+  do not block the PR)
 - No findings -> `approve`
 - The approach is fundamentally wrong — wrong design, unauthorized
   change, or the PR should be closed/completely rethought -> `reject`.

skills/pr-review/SKILL.md

@@ -1193,13 +1193,16 @@ challenger-adjudicated finding set and evaluate:
 - One or more **medium** findings identifying a functional bug
   (incorrect behavior, permission error, schema violation, or silent
   failure) → `request-changes`
+- Any finding (regardless of severity) with `actionable: true` and a
+  non-empty `remediation` → `request-changes` (these have concrete
+  remediations the fix agent can address automatically)
 - One or more **medium** findings that are all
   stylistic/advisory/process-related (no functional bugs) →
   `comment-only` (attach findings as comments so the author sees them,
   but do not block the PR)
-- **Low** or **info** findings only (no medium+) → `approve` (attach
-  findings as comments; preserve concrete follow-up work with
-  `actionable: true` so the post-script can create follow-up issues)
+- **Low** or **info** findings only, none with `actionable: true` and
+  a non-empty `remediation` → `approve` (observations, confirmations,
+  and analysis notes at any severity level)
 - No findings → `approve`
 - The approach is fundamentally wrong — wrong design, unauthorized
   change, or the PR should be closed/completely rethought → `reject`.
@@ -1287,8 +1290,8 @@ The table below lists the **additional** required fields per action:
 
 | Outcome         | Action            | Required fields                                                                               |
 |-----------------|-------------------|-----------------------------------------------------------------------------------------------|
-| approve         | `approve`         | `body`, `head_sha`; set `body` to "Looks good to me" (preceded by the hidden SHA comment) when there are no findings; include `findings[]` when low/info findings are actionable follow-up work |
-| request-changes | `request-changes` | `body`, `head_sha`, `findings[]`                                                              |
+| approve         | `approve`         | `body`, `head_sha`; set `body` to "Looks good to me" (preceded by the hidden SHA comment) when there are no findings |
+| request-changes | `request-changes` | `body`, `head_sha`, `findings[]` (also used for actionable findings with non-empty `remediation`) |
 | comment-only    | `comment`         | `body`, `head_sha`                                                                            |
 | failure         | `failure`         | `reason` (body optional)                                                                      |
 | reject          | `reject`          | `body`, `head_sha`, `findings[]`                                                              |