Review correctness sub-agent should detect test-infrastructure failures in shell-script test suites

[fullsend-ai-retro]

What happened

On PR #686, the human reviewer (waynesun09) identified three distinct test-infrastructure failures that caused GitLab tests to silently not execute, directly contradicting the PR's claim that "all 112 tests pass":

1. GNU-only sed -i without backup suffix (scripts/post-triage-test.sh): BSD sed requires a backup suffix argument with -i. Combined with set -euo pipefail, this killed the entire test run before any GitLab tests executed.

2. Mock pagination handler returns content for every page (scripts/post-triage-test.sh): The /notes? mock handler returned content regardless of page number (unlike the labels mock which gates on page=1). The sticky-comment code's pagination loop never terminated, hanging the test suite. Three sticky-comment tests never executed.

3. Test setup failure cascading under set -euo pipefail: The sed -i failure in test setup prevented all downstream GitLab test functions from running, but the test file's structure meant the exit code from earlier successful GitHub tests masked the failure.

The fullsend-ai-review[bot] ran 30 CHANGES_REQUESTED reviews without detecting any of these. Its correctness sub-agent analyzed the test file multiple times but only flagged surface-level issues (naming inconsistencies, missing shellcheck directives) — never questioning whether the tests actually executed to completion.

What could go better

The review agent's correctness sub-agent should include heuristics for detecting test-infrastructure failures — cases where tests appear to pass but are actually not executing. This is a distinct category from test-coverage analysis (agents#512, agents#397), which focuses on whether changed code has tests. Test-infrastructure failures are about whether existing tests actually run.

Confidence: High that this is a real gap. The review agent analyzed the test file in every review cycle and never questioned test executability. The patterns are heuristically detectable: GNU-only flags in cross-platform scripts, mock handlers that don't bound pagination, and test-setup failures that cascade silently.

Uncertainty: Medium about whether static heuristics alone are sufficient. The most reliable detection would involve actually running the test suite, which the review agent sandbox may not support for all repos. However, pattern-based detection would catch the most common cases.

Proposed change

Add test-infrastructure validation patterns to the review agent's correctness sub-agent prompt or as a dedicated review skill in fullsend-ai/agents. The patterns should cover:

1. Shell portability flags: When a PR modifies shell scripts that use sed -i, grep -P, readlink -f, or other GNU-only invocations, flag them if the test runner or CI environment may be BSD-based (macOS). Specifically, sed -i '' (BSD) vs sed -i (GNU) is a common portability trap.

2. Mock pagination termination: When test files define mock HTTP handlers that respond to paginated API endpoints (URLs containing page=, per_page=, or Link: headers), verify that the mock gates its response on the page parameter to terminate iteration. A mock that returns content for every page causes infinite loops in pagination code.

3. Test-setup failure propagation: When test files use set -euo pipefail and define setup functions called before test functions, verify that a failure in setup would visibly fail the test run rather than silently skipping downstream tests. Look for patterns where early exit codes are masked by the test harness structure.

4. Test execution claims vs infrastructure: When a PR description claims "all N tests pass," the correctness sub-agent should cross-reference the test count against the test file to verify that new test functions are reachable from the test runner entry point.

The implementation could be a checklist added to the correctness sub-agent's prompt in agents/review.md or the skills/code-review/SKILL.md skill, gated on PRs that modify *-test.sh or *_test.sh files.

Validation criteria

1. On the next 3 PRs to fullsend-ai/agents that modify shell-script test files, the review agent should flag at least one test-infrastructure concern if applicable (GNU-only flags, unbounded mock pagination, unreachable test functions).
2. If a PR claims "all tests pass" but modifies test infrastructure in a way that could prevent test execution, the review agent should question the claim with a specific finding.
3. No increase in false positives on PRs that correctly handle test infrastructure (the heuristics should be specific enough to avoid flagging correct code).

---

Generated by retro agent from #686

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 4:14 AM UTC · Completed 4:18 AM UTC

Commit: d761949 · View workflow run →

[fullsend-ai-triage]

Severity: medium — This is a genuine detection gap demonstrated by a concrete incident on PR #686, but the scope is narrow (shell-script test suites only).

Implementation target

The correctness dimension in skills/code-review/SKILL.md (under "#### Correctness") is the natural home. The four proposed patterns are well-scoped and can be added as a sub-section gated on PRs modifying shell test files.

Related issues

• Correctness sub-agent should verify test assertion effectiveness, not just coverage #681 — test assertion effectiveness (complementary, not overlapping: that issue is about assertions being meaningful; this one is about tests not running at all)
• Correctness sub-agent should verify external CLI tool flag behavior against documentation #778 — external CLI tool flag behavior (broader scope; sed -i portability is one instance of that class)
• Review correctness sub-agent should detect missing cleanup for test-shared-state mutations #640 — test-shared-state cleanup (different failure mode, same sub-agent)

Proposed test case

# Positive case: review eval with a test file containing:
# 1. `sed -i 's/foo/bar/' file.txt` (GNU-only, no backup suffix)
# 2. Mock handler: handle_notes() { echo '{"id":1}'; } (no page gating)
# Expected: at least one finding flagging test-infrastructure risk

# Negative case: test file with:
# 1. `sed -i '' 's/foo/bar/' file.txt` (BSD-compatible)
# 2. Mock handler: handle_notes() { [[ $page == 1 ]] && echo '{"id":1}'; }
# Expected: no test-infrastructure findings

---

Labels: Feature enhancement to the review agent's correctness sub-agent, filed by the retro agent from a real incident.

---

Next steps:

• /fs-code — agent creates a PR to implement this issue
• /fs-code <your instruction> — agent implements with your specific guidance

[fullsend-ai-prioritize]

RICE Priority Score: 1.2

Score breakdown

Dimension	Score	Reasoning
Reach	1	This affects the review agent's correctness sub-agent used across PRs in the fullsend-ai/agents repo. The scope is narrowed to PRs modifying shell-script test files, which limits the affected surface. It primarily impacts the internal development team relying on the review agent for automated code review quality.
Impact	1.5	The review agent ran 30 CHANGES_REQUESTED reviews on PR #686 without detecting any of three test-infrastructure failures that caused GitLab tests to silently not execute. This is a significant pain point: the agent provided false confidence that 'all 112 tests pass' when tests weren't actually running, undermining trust in automated review. Human reviewers can catch these issues as a workaround, but that defeats the review agent's purpose.
Confidence	0.8	High confidence. The issue is backed by a concrete, well-documented incident on PR #686 with three specific failure patterns. The triage agent confirmed medium severity. The proposed solution is well-scoped with clear validation criteria and test cases. The only uncertainty is whether static heuristics alone are sufficient versus actually running the test suite.
Effort	1	Medium effort. The implementation involves adding four specific detection patterns to the correctness sub-agent's prompt in skills/code-review/SKILL.md, gated on shell test file modifications. The patterns are well-defined (sed -i portability, mock pagination termination, setup failure propagation, test count verification). Requires eval test cases for both positive and negative scenarios, but the scope is clear and contained.

Formula: (1 x 1.5 x 0.8) / 1 = 1.2