Review agent: add shell correctness rule for top-level `return` in executed scripts

[guyoron1]

What happened

On PR #3182, the CSMA library was inlined from a sourced file (lib/github-api-csma.sh) into directly-executed scripts (post-prioritize.sh and post-prioritize-test.sh). The inlined code retained the library-style load guard [[ -n "${GITHUB_API_CSMA_SH_LOADED:-}" ]] && return 0 at script top level (line 32 of post-prioritize.sh).

The Qodo code review bot correctly identified this as a bug: return at the top level of an executed script is invalid, and under set -euo pipefail it causes a hard abort with exit code 2.

The fullsend review agent analyzed the same guard code but dismissed it: "The GITHUB_API_CSMA_SH_LOADED guard with return 0 is dead code in the inlined context (the variable is always unset at script start). It's harmless." This reasoning is incorrect — return at script top level is invalid regardless of reachability, and environment contamination can trigger the crash.

The human reviewer also approved without catching the issue. The PR was merged on July 7 with the bug still present.

What could go better

The review agent's AGENTS.md already contains shell-specific review guidance (the gh api --paginate and jq section with explicit instructions to flag aggregation inside --jq). A similar rule about return vs exit context would have caught this bug. The correctness sub-agent's "Runtime mechanism checklist" instructs to "trace the full path from producer to consumer and verify the mechanism will function at runtime," but the agent performed reachability analysis ("the variable is always unset") rather than construct validity analysis ("is return valid here at all?").

Confidence: high. The bug is confirmed by reproduction — running the script with GITHUB_API_CSMA_SH_LOADED=1 causes return: can only 'return' from a function or sourced script and exit code 2.

Proposed change

Add a new subsection to the 'Shell scripting' section of AGENTS.md, after the existing gh api --paginate guidance:

### `return` vs `exit` in executed scripts

`return` is valid only inside functions or in scripts that are `source`d. At the top level of an executed script (one with a shebang or run via `bash script.sh`), `return` causes a hard error — and under `set -euo pipefail`, this aborts the script.

This commonly occurs when inlining library code that was previously `source`d. Library-style load guards like `[[ -n "${LOADED:-}" ]] && return 0` must be removed or replaced when the code is inlined into an executed script.

**When reviewing shell scripts:** Flag any top-level `return` (outside a function body) in a script that is executed rather than sourced as a medium-severity correctness finding. Check for this especially when a PR inlines or moves code from a library/sourced file into an executable script.

Validation criteria

The next time the review agent encounters a PR that inlines shell library code containing a top-level return into an executable script, the correctness sub-agent flags it as a medium-severity finding rather than dismissing it based on reachability. Can be tested by submitting a PR with a similar pattern and verifying the review output.

---

Generated by retro agent from fullsend-ai#3182

[github-actions]

Triage Summary

This issue is a well-specified request to add shell correctness guidance to AGENTS.md to prevent the review agent from dismissing invalid top-level return statements in executed scripts.

Problem: When PR fullsend-ai#3182 inlined CSMA library code into an executed script, the review agent dismissed the retained return 0 guard as "harmless dead code" based on reachability analysis. However, return at script top level is invalid in bash and causes a hard abort under set -euo pipefail, regardless of whether the branch is reachable.

Root cause: AGENTS.md lacks a rule for construct validity of return in shell scripts. The review agent performed reachability analysis instead of checking whether return is valid in the execution context.

Implementation note: The current AGENTS.md in this repo does not have a "Shell scripting" section or the referenced gh api --paginate guidance. The developer should check the upstream repo for context on section placement, or create a new section. The issue provides exact proposed text ready to insert.

Relationship to #174: Issue #174 tracks the concrete bug fix (removing the invalid guard from post-prioritize scripts). This issue (#181) tracks the process improvement — teaching the review agent to catch this pattern in future PRs. Both can proceed independently.

Proposed test case:

Test: Review agent catches top-level return in executed scripts

Setup:
  Submit a PR that inlines shell library code containing a top-level
  load guard ([[ -n "${LOADED:-}" ]] && return 0) into an executable
  script (one with a shebang, not sourced).

Expected:
  The review agent flags the top-level return as a medium-severity
  correctness finding.

Negative case:
  return statements inside function bodies in the same script should
  NOT be flagged.

Previous run

Triage Summary

This issue is a well-specified request to add shell correctness guidance to AGENTS.md to prevent the review agent from dismissing invalid top-level return statements in executed scripts.

Problem: When PR fullsend-ai#3182 inlined CSMA library code into an executed script, the review agent dismissed the retained return 0 guard as "harmless dead code" based on reachability analysis. However, return at script top level is invalid in bash and causes a hard abort under set -euo pipefail, regardless of whether the branch is reachable.

Root cause: AGENTS.md lacks a rule for construct validity of return in shell scripts. The review agent performed reachability analysis instead of checking whether return is valid in the execution context.

Relationship to #174: Issue #174 tracks the concrete bug fix (removing the invalid guard from post-prioritize scripts). This issue (#181) tracks the process improvement — teaching the review agent to catch this pattern in future PRs. Both should proceed independently.

Proposed fix: Add a subsection to the 'Shell scripting' section of AGENTS.md with explicit guidance about return vs exit context, instructing the review agent to flag top-level return in executed scripts as a medium-severity correctness finding.

Proposed test case:

Test: Review agent catches top-level return in executed scripts

Setup:
  Submit a PR that inlines shell library code containing a top-level
  load guard ([[ -n "${LOADED:-}" ]] && return 0) into an executable
  script (one with a shebang, not sourced).

Expected:
  The review agent flags the top-level return as a medium-severity
  correctness finding.

Negative case:
  return statements inside function bodies in the same script should
  NOT be flagged.

The issue includes exact proposed text for the AGENTS.md addition and clear validation criteria. Ready for implementation.

---

Labels: This is a documentation enhancement adding review guidance to AGENTS.md. It is well-specified with proposed text and validation criteria, making it ready to implement.

[guyoron1]

/fs-triage

[guyoron1]

/fs-code

[guyoron1]

/fs-triage

[fullsend-ai-triage]

🤖 Finished Triage · ✅ Success · Started 8:50 AM UTC · Completed 8:53 AM UTC
Commit: a2afa8b · View workflow run →