Review agent should flag bug-fix PRs that lack tests for the fixed behavior

[fullsend-ai-retro]

What happened

PR #64 fixed two bugs: (1) get_setting() returning None instead of falling back to DEFAULT_SETTINGS for missing DB rows, and (2) get_existing_session_ids() treating empty UUID directories as valid sessions. The review agent approved with no findings at 08:18 UTC. However, neither behavioral change has a corresponding test. The existing test test_default_settings_seeded only verifies init_schema() seeds values — it does not test the fallback path when a row is missing. The existing test test_get_existing_session_ids only tests the .jsonl file path, not the directory-based path that was changed. The test fixtures were updated to use set_setting() instead of raw SQL, but this is a refactor, not a test of the fix.

What could go better

The review agent should detect when a PR is fixing a bug (identifiable from the linked issue labels, branch name pattern fix/*, or PR title containing "fix") and check whether the behavioral changes have corresponding test coverage. In this case, the triage agent had already proposed a test case in its summary on issue #63, which the review agent could have cross-referenced.

Confidence: High. The missing test coverage is unambiguous — both changes modify runtime behavior with no new test assertions. This is not a style preference; untested bug fixes are a regression risk. The prior retro (issue #53) also identified a pattern of partial application of changes, and missing tests are another manifestation of the same root cause: changes that should come in pairs (fix + test, behavior + docs) arrive incomplete.

Uncertainty: It's possible the maintainer intentionally deferred tests, or considers the changes too simple to warrant tests. But the review agent should at least flag the gap so the human can make an informed decision.

Proposed change

In the review agent's prompt or skill definition (likely agents/review.md or the review skill in fullsend-ai/fullsend), add guidance for bug-fix PRs:

1. When the PR is linked to an issue labeled bug, or the branch matches fix/*, or the title contains "fix", the review agent should check whether each behavioral change introduced by the diff has a corresponding test assertion (new or modified).
2. If no test covers the changed behavior, the agent should emit a finding (severity: medium) suggesting a test, ideally referencing the specific function and scenario. For example: "The get_setting() fallback to DEFAULT_SETTINGS when the row is missing is untested. Consider adding a test that deletes the row and asserts the default is returned."
3. The agent should cross-reference the triage agent's output (if available in the issue comments) for any proposed test cases that were not implemented.

This should be a soft suggestion (not a blocking change request) to avoid false positives on trivial fixes.

Validation criteria

Over the next 10 bug-fix PRs reviewed by the review agent across fullsend-managed repos: (1) the agent should flag missing test coverage in at least 50% of cases where a behavioral change lacks a corresponding test, and (2) the false-positive rate (flagging missing tests when adequate coverage exists) should remain below 20%. Track by comparing review agent findings against manual assessment of test coverage gaps.

---

Generated by retro agent from agentshed/seshi#64

[fullsend-ai-triage]

Triage Summary

Severity: Medium | Category: Feature

Problem

The review agent currently approves bug-fix PRs without checking whether behavioral changes have corresponding test coverage. This was observed on agentshed/seshi PR #64, where two runtime behavior changes were approved with no new test assertions.

Root Cause

The code-review skill's evaluation dimensions do not include a bug-fix-specific heuristic. There is no mechanism to detect that a PR is a bug fix or to verify that changed behavior is tested.

Recommended Approach

1. Add a bug-fix detection heuristic to the code-review skill (internal/scaffold/fullsend-repo/skills/code-review/SKILL.md) — detect via linked issue labels (bug), branch name (fix/*), or PR title containing "fix"
2. When a bug-fix PR is detected, check whether each behavioral change in the diff has a corresponding test assertion (new or modified)
3. Emit a medium-severity finding (soft suggestion, not blocking) when test coverage is missing
4. Optionally cross-reference triage agent output for proposed test cases (complementary to Review agent should cross-reference triage findings when reviewing PRs linked to triaged issues #1351)

Related Issues

• Review agent should cross-reference triage findings when reviewing PRs linked to triaged issues #1351 — Review agent should cross-reference triage findings (complementary, not duplicate)
• code-review skill: mandatory finding when diff modifies review agent behavior files #1309 — Mandatory finding when diff modifies review agent behavior files (similar pattern of adding conditional review checks)

Proposed Test Case

Scenario 1 (should flag):
- Synthetic PR diff modifying a function's runtime behavior (e.g., adding fallback)
- No new/modified test assertions
- Linked to issue labeled 'bug'
- Expected: medium-severity finding about missing test coverage

Scenario 2 (should not flag):
- Same behavioral change + corresponding test assertion added
- Expected: no missing-test finding

Edge cases:
- PR refactoring test fixtures without new assertions → should flag
- fix/* branch with no linked issue → should still detect as bug-fix
- Trivial string typo fix → should ideally not flag (false-positive check)

Validation Criteria (from issue)

Over the next 10 bug-fix PRs: flag missing test coverage ≥50% of the time when warranted, with false-positive rate <20%.

---

Labels: Retro-generated feature request targeting the review agent's code-review skill, at medium priority given the regression risk but non-blocking nature.