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

[fullsend-ai-coder]

Summary

• Verdict logic: Findings with actionable: true and a non-empty remediation now produce request-changes regardless of severity level, so the fix agent triggers automatically on findings with concrete remediations.
• Severity threshold is absolute: The REVIEW_FINDING_SEVERITY_THRESHOLD setting is respected throughout — findings filtered by the threshold are excluded even if actionable, composing correctly with the new verdict logic.
• Cross-file consistency: Updated agents/review.md, skills/pr-review/SKILL.md, skills/code-review/SKILL.md, docs/review.md, and schemas/review-result.schema.json to decouple actionable from severity.

Testing

• Updated unit tests in filter_and_downgrade() to verify actionable findings below threshold are filtered and the verdict is downgraded.
• All existing post-review-test.sh tests continue to pass.

---

Closes #1046

Post-script verification

• Branch is not main/master (agent/1046-actionable-verdict-logic)
• Secret scan passed (gitleaks)
• PR body secret scan passed (gitleaks — no-git)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:36 PM UTC · Completed 5:57 PM UTC

Commit: 7f6744e · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.12

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Moderate risk confirmed via re-review anchoring. Prior score (2) preserved: PROTECTED_PATH_COUNT decreased from 5 to 3 and PR shrank from 188 to 104 lines, while high file churn and multi-author contention are offset by trusted bot author, strong issue alignment, and easy rollback safety.

Previous run

Risk Assessment: moderate (2/5)

Details

Moderate risk confirmed via re-review anchoring. PROTECTED_PATH_COUNT=5 elevates metadata risk, but narrow scope (188 lines, 6 files), trusted bot author, high issue alignment, and well-maintained codebase offset this. Prior assessment (score: 2) remains valid.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Small, focused bug fix (165 lines, 6 files) by a trusted bot addressing review verdict logic; high protected path count (5) is offset by narrow scope, trusted author, and clear issue alignment with test coverage.

Previous run (3)

Risk Assessment: moderate (2/5)

Details

Small, focused bug fix (122 lines, 5 files) by a trusted bot with clear issue alignment and test coverage; high file churn and protected paths are offset by narrow scope and matching acceptance criteria.

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] agents/review.md — This PR modifies 3 files under protected paths: agents/review.md (agents/), scripts/post-review-test.sh (scripts/), skills/pr-review/SKILL.md (skills/). The PR links to issue Review agent should submit CHANGES_REQUESTED when any finding has a concrete remediation #1046 and provides a clear rationale for the changes. Human approval is always required for protected-path changes, regardless of justification.

• [cross-file-inconsistency] schemas/review-result.schema.json:118 — The JSON schema description for the actionable field reads: "True when this non-blocking finding should be tracked as a follow-up issue if the review approves." This contradicts the new semantics where actionable: true with a non-empty remediation routes to request-changes.
  Remediation: Update the actionable field description in schemas/review-result.schema.json.

• [logic-error] skills/pr-review/SKILL.md:1200 — In section 6f, the actionable+remediation → request-changes rule appears after the medium-stylistic → comment-only rule. If a PR has both a medium stylistic finding AND a low actionable+remediation finding, the ordering ambiguity could cause the agent to produce comment-only instead of request-changes, shadowing the actionable finding.
  Remediation: Move the actionable+remediation bullet above the medium-stylistic bullet, or clarify that the most restrictive matching verdict applies.

• [internal-inconsistency] agents/review.md:204 — The approve outcome bullet reads "no medium+ findings; the change is safe" without mentioning the new exclusion for actionable+remediation findings. Lines 223–225 correctly state the exclusion, creating an internal contradiction within the same file.
  Remediation: Update the approve bullet to include the actionable+remediation exclusion.

• [incomplete-issue-resolution] scripts/post-review.src.sh — Issue Review agent should submit CHANGES_REQUESTED when any finding has a concrete remediation #1046 identifies two gates: (1) orchestrator verdict logic and (2) post-script severity filter. Gate harness/triage.yaml has stale /tmp/workspace/ dest paths; env vars never reach sandbox #1 is addressed. Gate chore: update fullsend shim workflow #2 (post-script) was not changed — the PR documents the threshold as intentionally absolute. The PR summary claims gate chore: update fullsend shim workflow #2 was fixed ("the downgrade from request-changes to comment is now skipped if any removed findings had actionable: true"), but post-review.src.sh is not in the diff and contains no actionability logic.
  Remediation: Correct the PR summary to reflect that gate chore: update fullsend shim workflow #2 was not changed, or explicitly document in issue Review agent should submit CHANGES_REQUESTED when any finding has a concrete remediation #1046 why gate chore: update fullsend shim workflow #2 is deferred.

Low

• [cross-file-inconsistency] skills/code-review/SKILL.md:297 — The code-review skill's verdict rules (lines 297–301) and actionable field description (lines 234–238) still reflect the old semantics where low/info actionable findings produce approve. The deferral clause ("the agent definition wins") mitigates runtime inconsistency in orchestrator mode, but the divergence could confuse the agent in local/pre-push mode.

• [scope-contradiction] agents/review.md:71 — The gate harness/triage.yaml has stale /tmp/workspace/ dest paths; env vars never reach sandbox #1 fix (actionable → request-changes) can be nullified by gate chore: update fullsend shim workflow #2 (absolute severity threshold) when the threshold is set above the finding's severity. With the default threshold ("low"), this interaction does not manifest — actionable low findings pass the filter. The tension is by design but worth noting.

• [test-coverage-gap] scripts/post-review-test.sh:310 — The new tests verify that actionable findings below the threshold are filtered and the verdict is downgraded, confirming the absolute threshold behavior. These tests codify the gate chore: update fullsend shim workflow #2 design decision.

Previous run

Review

Findings

Medium

• [doc-implementation-mismatch] agents/review.md:70 — The severity-filter downgrade rule (lines 70–71) says the verdict is preserved when any removed finding had actionable: true, without requiring a non-empty remediation. The post-script (post-review.src.sh, post-review.sh) enforces the stricter condition .actionable == true and .remediation != null and .remediation != "". The same file’s verdict rule (line 210) and approve paragraph (line 224) both correctly require actionable: true with a non-empty remediation. The same looser language also appears in docs/review.md. This internal inconsistency means the agent may follow the looser filtering rule and preserve a blocking verdict for a finding with actionable: true but no remediation, while the post-script would then downgrade it anyway.
  Remediation: Add the remediation condition to both the filtering rule in agents/review.md and the corresponding paragraph in docs/review.md. Change ‘unless any of the removed findings had actionable: true’ to ‘unless any of the removed findings had actionable: true with a non-empty remediation’.

• [protected-path] scripts/post-review.src.sh — This PR modifies 5 files under protected paths: agents/review.md (agents/), scripts/post-review-test.sh (scripts/), scripts/post-review.sh (scripts/), scripts/post-review.src.sh (scripts/), skills/pr-review/SKILL.md (skills/). The PR links to issue Review agent should submit CHANGES_REQUESTED when any finding has a concrete remediation #1046 and provides a clear rationale for the changes. Human approval is always required for protected-path changes, regardless of justification.

Previous run (2)

Review

Findings

Medium

• [doc-implementation-mismatch] scripts/post-review.src.sh — The documentation (agents/review.md, skills/pr-review/SKILL.md, docs/review.md) specifies the condition for preserving the verdict as findings having actionable: true with a non-empty remediation. The shell implementation only checks .actionable == true — it does not verify that remediation exists or is non-empty. A finding with {"actionable": true} and no remediation field would prevent the downgrade, contradicting the documented two-part condition. The same gap exists in the test helper filter_and_downgrade in post-review-test.sh.
  Remediation: Update the jq filter in both post-review.src.sh and post-review-test.sh to also require a non-empty remediation: jq '[.findings[] | select(.actionable == true and .remediation != null and .remediation != "")] | length'. Regenerate post-review.sh via make script-build.

• [schema-violation] scripts/post-review.src.sh — When actionable findings preserve the verdict, the code deletes the entire findings array (jq 'del(.findings)') but keeps action: "request-changes". While schema validation occurs before the post-script runs (ADR 0022), the result file becomes structurally inconsistent — the fix agent receives a request-changes verdict with no structured findings to act on. Keeping the actionable findings in the array would give the fix agent concrete data. See also: [doc-implementation-mismatch] finding.
  Remediation: Instead of jq 'del(.findings)', retain only the actionable findings with remediations: jq '.findings |= [.[] | select(.actionable == true and .remediation != null and .remediation != "")]'. This resolves both this finding and the doc-implementation-mismatch.

• [protected-path] scripts/post-review.src.sh — This PR modifies 5 files under protected paths: agents/review.md (agents/), scripts/post-review-test.sh (scripts/), scripts/post-review.sh (scripts/), scripts/post-review.src.sh (scripts/), skills/pr-review/SKILL.md (skills/). The PR links to issue Review agent should submit CHANGES_REQUESTED when any finding has a concrete remediation #1046 and provides a clear rationale for the changes. Human approval is always required for protected-path changes, regardless of justification.

Low

• [potentially-stale-verdict-rules] skills/code-review/SKILL.md:297 — The code-review skill's verdict rules still say low/info findings always produce approve. The new actionability-keyed verdict rule routes actionable low/info findings to request-changes. The code-review skill's deferral clause ("the agent definition wins") mitigates runtime inconsistency, but the explicit divergence could confuse the agent in local/pre-push mode where the orchestrator is not involved.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

Previous run (3)

Review

Findings

High

• [internal-consistency] skills/pr-review/SKILL.md:1290 — Section 6f (line 1200) now routes actionable low/info findings to request-changes, but the outcome mapping table (line 1290) was not updated — it still says the approve action should "include findings[] when low/info findings are actionable follow-up work." The agent receives contradictory instructions within the same document.
  Remediation: Update the outcome mapping table row for approve to remove the clause about including actionable findings. Adjust the request-changes row to indicate that low/info actionable findings with non-empty remediation also use this action.

• [cross-file-consistency] agents/review.md — The authoritative agent definition was not updated to reflect the new verdict logic. Multiple sections are now stale: (1) severity filtering section (lines 68–71) unconditionally says the verdict is downgraded to comment when filtering removes all findings, not mentioning the new actionable-count exception; (2) the Outcome section (line 204) does not mention the actionability-keyed request-changes path; (3) line 224 instructs "approve the PR" for low/info actionable findings; (4) the actionable field description (line 274) says it applies "on low/info findings in an approve result"; (5) the jq example (line 293) is titled "For approve with actionable low/info findings." Because agents/review.md is declared authoritative over SKILL.md, these stale instructions override the new behavior.
  Remediation: Update agents/review.md: (1) lines 68–71 — note that verdict downgrade is conditional on no actionable findings remaining; (2) line 204 area — add the actionability-keyed request-changes path to the Outcome section; (3) line 224 — change from "approve" to "request changes" for actionable low/info findings; (4) line 274 — update the actionable field description; (5) lines 293–305 — update the jq example from approve to request-changes.

Medium

• [naming-convention] scripts/post-review.src.sh:153 — Variable name PREFILTER_RESULT_FILE concatenates "PRE" and "FILTER" without an underscore. All other UPPER_SNAKE_CASE variables in the file separate each word with underscores (e.g., RESULT_FILE, FILTERED_RESULT, DOWNGRADE_RESULT).
  Remediation: Rename to PRE_FILTER_RESULT_FILE in both post-review.src.sh and post-review.sh.

• [protected-path] scripts/post-review.src.sh — This PR modifies 4 files under protected paths (scripts/post-review-test.sh, scripts/post-review.sh, scripts/post-review.src.sh, skills/pr-review/SKILL.md). The PR links to issue Review agent should submit CHANGES_REQUESTED when any finding has a concrete remediation #1046 and provides a clear rationale for the changes. Human approval is always required for protected-path changes, regardless of justification.

Low

• [scope-creep] skills/pr-review/SKILL.md:1200 — The SKILL.md change gates on both actionable: true AND non-empty remediation. The conjunction is a defensible design choice, but the remediation requirement is an additional constraint beyond what the issue title explicitly authorizes.

• [potentially-stale-verdict-rules] skills/code-review/SKILL.md:297 — The code-review skill's verdict rules still say low/info findings always produce approve. If the new actionability-keyed verdict rule is intended to apply consistently across both the PR orchestrator and local/pre-push paths, these rules are now inconsistent.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:59 PM UTC · Completed 6:06 PM UTC

Commit: 7f6744e · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.57

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (bot-triggered)

Addressed 3 of 6 review findings with code fixes: updated SKILL.md outcome mapping table, updated 5 stale sections in agents/review.md, and renamed PREFILTER_RESULT_FILE for naming consistency. Disagreed with 3 findings: protected-path observation (informational), scope-creep observation (reviewer acknowledged as defensible), and code-review SKILL.md staleness (intentionally different verdict rules for local vs PR context).

Fixed (3):

1. SKILL.md outcome mapping table inconsistent with new verdict logic (skills/pr-review/SKILL.md): Updated the outcome mapping table: removed the stale clause about including findings[] for actionable follow-up work from the approve row, and added a note to the request-changes row indicating it also covers actionable low/info findings with non-empty remediation.
2. agents/review.md stale sections not updated for actionable verdict logic (agents/review.md): Updated 5 stale sections: (1) severity filtering now notes the actionable-count exception to verdict downgrade; (2) Outcome section adds actionability-keyed request-changes path; (3) low/info guidance changed from approve to request-changes for actionable findings; (4) actionable field description updated from approve to request-changes context; (5) removed the separate approve-with-actionable-findings jq example and consolidated into request-changes example.
3. PREFILTER_RESULT_FILE naming convention violation (scripts/post-review.src.sh): Renamed PREFILTER_RESULT_FILE to PRE_FILTER_RESULT_FILE in both post-review.src.sh and post-review.sh to match the UPPER_SNAKE_CASE word-separation convention used by all other variables in the file.

Disagreed (3):

1. Protected-path files modified require human approval: This is an informational observation, not a code change request. The reviewer correctly notes that human approval is required for protected-path changes. No code action needed.
2. Scope creep: remediation requirement is beyond issue title scope: The conjunction of actionable: true AND non-empty remediation is a defensible design choice made in this PR. The reviewer acknowledges this. The remediation gate ensures the fix agent only triggers when there is a concrete fix to apply. No change needed.
3. code-review SKILL.md verdict rules potentially stale: The code-review skill serves local/pre-push review where there is no fix agent to trigger. In that context, low/info findings correctly produce approve. The actionability-keyed request-changes path is specific to the PR orchestrator flow. The two skills serve different contexts and their verdict rules are intentionally different.

Tests: passed

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:09 PM UTC · Completed 6:31 PM UTC

Commit: 361e055 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.60

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 6:33 PM UTC · Completed 6:40 PM UTC

Commit: 361e055 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.21

[fullsend-ai-coder]

🔧 Fix agent — iteration 2 (bot-triggered)

Addressed 2 of 4 review findings with code fixes: (1) updated jq filter to require both actionable==true and non-empty remediation, matching docs; (2) retained actionable findings in the result instead of deleting them, so the fix agent has structured data. Disagreed with 2 findings: protected-path (informational) and stale verdict rules (intentional design difference between local and PR review contexts).

Fixed (2):

1. doc-implementation-mismatch: jq filter only checks actionable==true, not also requiring non-empty remediation (scripts/post-review.src.sh): Updated the jq filter in both post-review.src.sh and post-review-test.sh to check for both actionable==true AND remediation!=null AND remediation!="", matching the documented two-part condition. Regenerated post-review.sh via make script-build. Added two new test cases covering actionable-without-remediation and actionable-with-empty-remediation edge cases.
2. schema-violation: actionable findings deleted instead of retained when preserving verdict (scripts/post-review.src.sh): Changed from jq 'del(.findings)' to jq '.findings = $af' where $af contains only the actionable findings with non-empty remediations extracted from the pre-filter result. The fix agent now receives concrete structured findings to act on. Updated the test helper filter_and_downgrade in post-review-test.sh to match, and changed three test expectations from has_findings=false to has_findings=true.

Disagreed (2):

1. protected-path: PR modifies files under protected paths requiring human approval: This is an informational observation, not a code change request. The reviewer correctly notes that human approval is required for protected-path changes. No code action needed.
2. potentially-stale-verdict-rules: code-review SKILL.md verdict rules diverge from new actionability logic: The code-review skill serves local/pre-push review where there is no fix agent to trigger. In that context, low/info findings correctly produce approve. The actionability-keyed request-changes path is specific to the PR orchestrator flow. The two skills serve different contexts and their verdict rules are intentionally different. The deferral clause in code-review SKILL.md mitigates any ambiguity.

Tests: passed

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:42 PM UTC · Completed 6:59 PM UTC

Commit: 84ac7a3 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.44

Superseded by updated review

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 10:18 PM UTC · Completed 11:00 PM UTC

Commit: f1b8cc7 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:09 AM UTC · Completed 12:51 AM UTC

Commit: f1b8cc7 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high

[fullsend-ai-review]

Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:

• agents/review.md (file-level): Line 204 · [medium] internal-inconsistency

The approve outcome bullet reads 'no medium+ findings; the change is safe' without mentioning the new exclusion for actionable+remediation findings. Lines 223-225 correctly state the exclusion, creating an internal contradiction.

Suggested fix: Update the approve bullet to include the actionable+remediation exclusion.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:25 AM UTC · Ended 1:26 AM UTC

Commit: 6ba8355 · View workflow run →

[fullsend-ai-review]

🤖 Review · Started 1:27 AM UTC

Commit: 486e155 · View workflow run →