Correctness sub-agent should check for null/missing-field handling in jq filters processing API responses

[fullsend-ai-retro]

What happened

On PR #284, the review agent's first successful review (run 29678947790, commit c744f89, ~12 min) examined a new 183-line shell library (scripts/lib/pr-assignee.lib.sh) containing jq filters that process GitHub API responses. It found only [protected-path] — zero correctness findings. The second review (run 29721525543, commit 2d0b158, ~18 min) examined byte-identical code in that file and found a medium-severity [null-body-crash]: the jq first_word function crashed when processing a comment with body: null because null | gsub("\r$"; "") errors, and the outer error suppression (2>/dev/null || true) silently caused valid /fs-code comments later in the array to be skipped. The author confirmed and fixed the bug in commit 395f0f1.

What could go better

The null-body-crash is a jq-specific data-processing pitfall: API response fields (like comment.body) can be null, and jq string operations on null silently error or produce unexpected results. This class of bug is distinct from the bash-specific pitfalls covered by agents#131 (trailing newline stripping, set -u interactions, mktemp cross-filesystem, pipe-subshell scoping), which focuses on bash language semantics rather than jq filter robustness. The correctness sub-agent's current guidance includes general 'edge case' and 'runtime mechanism verification' checks but has no jq-specific patterns, making detection dependent on LLM attention rather than systematic checking. Confidence: high that the gap exists (the first review missed it, the code was identical); medium that adding explicit guidance will consistently improve detection (LLM non-determinism is a factor, but explicit patterns improve prompt reliability for well-defined bug classes).

Proposed change

Add a 'jq / data-processing filter robustness' subsection to the correctness sub-agent's guidance (in skills/code-review/SKILL.md or skills/pr-review/SKILL.md, whichever defines the correctness dimension's checklist). The section should cover: (1) Null-field handling — when jq filters process API response fields (.body, .login, .name, etc.), verify that null values are handled with // "" or explicit null checks before string operations (gsub, split, test, ascii_downcase); (2) Array indexing on empty arrays — [][0] returns null in jq; verify downstream operations handle null from empty-array access; (3) Error suppression masking real failures — when jq pipelines are wrapped in 2>/dev/null || true or try-catch, verify that error suppression does not silently skip valid data (as happened with the null-body-crash where error suppression caused the entire comment scan to abort early); (4) Optional object traversal — use ? operator or // empty for fields that may not exist in all response variants. This complements agents#131's bash pitfalls with jq-specific patterns for the same ecosystem (shell scripts processing API data).

Validation criteria

On the next 5 review agent runs examining PRs that add or modify jq filters processing GitHub API responses (or similar JSON APIs), the correctness sub-agent should flag null-field handling gaps on the first review pass. A specific test: a jq filter like .body | split("\n")[0] | gsub("\r$"; "") without null guarding should be flagged. Can also be validated with a synthetic review eval case containing a jq filter that processes API comments without null checks.

---

Generated by retro agent from #284

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 8:55 PM UTC · Completed 8:59 PM UTC
Commit: 17eed93 · View workflow run →

[fullsend-ai-triage]

Triage Summary

Category: Feature — add jq data-processing robustness checklist to correctness sub-agent
Severity: Medium

Problem

The correctness sub-agent has no jq-specific guidance for reviewing shell scripts that process JSON API responses. On PR #284, a null-body crash in a jq filter was missed on the first review pass — detection depended on LLM non-determinism rather than systematic checking. The current checklist covers general nil/null handling but lacks patterns specific to jq's behavior (where string operations on null silently error).

Root Cause

skills/pr-review/sub-agents/correctness.md has no jq-specific section. This is a distinct gap from #131 (bash language pitfalls) — jq filter robustness is a separate domain requiring its own patterns.

Recommended Fix

Add a jq / data-processing filter robustness subsection to skills/pr-review/sub-agents/correctness.md covering:

1. Null-field handling — verify API response fields (.body, .login, .name) are guarded with // "" or null checks before string operations (gsub, split, test, ascii_downcase)
2. Empty-array indexing — [][0] returns null; verify downstream operations handle it
3. Error suppression masking failures — verify 2>/dev/null || true or try-catch does not silently skip valid data
4. Optional object traversal — use ? operator or // empty for fields absent in some response variants

Proposed Test Case

# Should be flagged: no null guard on .body before string ops
.[] | .body | gsub("\\r$"; "") | split("\\n")[0]

# Should NOT be flagged: null-guarded
.[] | (.body // "") | gsub("\\r$"; "") | split("\\n")[0]

Validation: on the next 5 review runs examining PRs with jq filters processing API responses, the correctness sub-agent should flag null-field handling gaps on the first pass.

---

Labels: This is a feature enhancement to the review agent's correctness sub-agent. Applying 'enhancement' and 'review-agent' labels to reflect scope. Removing 'ready-for-triage' as triage is complete, and adding 'ready-to-code' since the issue is fully specified with clear implementation guidance.