docs(#251): add shell return-vs-exit correctness rule

[fullsend-ai-coder]

Add a Shell scripting section to AGENTS.md with guidance on the return-vs-exit distinction in executed vs sourced scripts. The review agent previously dismissed a top-level return in an executed script as harmless dead code (PR fullsend-ai#3182). In bash, return at script top level is invalid and causes a hard abort under set -euo pipefail, regardless of reachability.

---

Closes #251

Post-script verification

• Branch is not main/master (agent/251-shell-return-rule)
• Secret scan passed (gitleaks — d8e3df7c018996e396c79b8ecd59e4145a628a6f..HEAD)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:41 PM UTC · Completed 4:45 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

Review — comment

PR: docs(#251): add shell return-vs-exit correctness rule
Scope: AGENTS.md (+10 lines) — adds ## Shell scripting section with return vs exit review guidance

Authorization & scope ✅

Change traces to issue #251 (created by repo owner). The diff is a verbatim implementation of the issue's "Proposed change" section. No scope creep. CODEOWNERS wildcard rule ensures human approval.

Security ✅

No secrets, injection patterns, permission changes, or steganographic content. The review-guidance directive follows established AGENTS.md patterns.

Style & conventions ✅

Heading levels (## / ###), bold review callout (**When reviewing shell scripts:**), and document placement all follow existing AGENTS.md patterns. PR title follows COMMITS.md conventions.

Correctness — findings

The guidance is directionally correct and addresses a real, documented bug (PR fullsend-ai#3182). However, the technical explanation has a precision gap:

[medium · technical-accuracy · AGENTS.md:~51] The claim that top-level return "causes a hard error" overstates what happens without set -e. The actual behavior:

• Without set -e: return prints a diagnostic to stderr (bash: return: can only 'return' from a function or sourced script) and exits with code 2, but the script continues. The guard silently fails — the rest of the inlined code executes when it shouldn't.
• With set -e: The non-zero exit code from the failed return triggers set -e and the script aborts.

Both outcomes are bugs, but for different reasons. The current wording could lead a reviewer to skip flagging the pattern in scripts without set -e, since they might think "no set -euo pipefail means no problem." Consider rewording to describe both failure modes.

Note: the set -euo pipefail mention is accurate as a common idiom, but only the -e flag is relevant to this failure mode (-u and pipefail don't contribute).

[low · edge-case · AGENTS.md:~53] The example [[ -n "${LOADED:-}" ]] && return 0 only triggers the error when the guard variable is set. When unset (the common case), && short-circuits and return is never reached. This makes it a latent bug that passes basic testing but fails when the variable leaks from the environment or on "second load" scenarios. Adding a sentence about this would help reviewers understand why the pattern is insidious.

Verdict

The new rule is well-scoped, properly authorized, and adds genuine value — the review agent missed exactly this class of bug on PR fullsend-ai#3182. The medium finding is about precision of the technical explanation, not correctness of the review guidance itself ("flag top-level return" remains correct advice regardless). Worth addressing for accuracy, but not blocking.

---

Dimension	Result
Authorization	✅ Traced to #251
Scope	✅ No creep
Security	✅ Clean
Style	✅ Consistent
Correctness	⚠ 1 medium (precision), 1 low (edge case)
Docs currency	⏭ Skipped (this IS docs)
Cross-repo	⏭ Skipped (no APIs)

Previous run

Review — approve

PR: #282 · docs(#251): add shell return-vs-exit correctness rule
Commit: 43bf6058

Summary

Single-file documentation change adding a "Shell scripting" section to AGENTS.md with guidance on the return-vs-exit distinction in executed vs sourced scripts. The change directly addresses a missed review finding from PR fullsend-ai#3182, where a top-level return in an inlined library was incorrectly dismissed as harmless dead code.

Analysis

Correctness — The technical content is accurate. return at script top level in bash produces return: can only 'return' from a function or sourced script and exits non-zero, which under set -euo pipefail aborts the script. The example pattern ([[ -n "${LOADED:-}" ]] && return 0) correctly illustrates the most common source of this bug. The medium-severity guidance for review findings is appropriately calibrated.

Intent & coherence — The committed text matches the proposed change in issue #251 verbatim. The commit message uses the correct docs(#251): prefix per COMMITS.md conventions. The scope is tightly focused — one section, one rule, one file. The bot author is exempt from DCO per AGENTS.md policy.

Style & placement — The new ## Shell scripting section with its ### return vs exit in executed scripts subsection follows the existing heading hierarchy (parallel to ## Go code, ## Forge abstraction). Bold emphasis for review guidance ("When reviewing shell scripts:") is consistent with patterns in the Forge abstraction section. No formatting issues.

Security — Documentation-only change; no security implications.

Documentation currency — N/A (this is the documentation change).

Cross-repo contracts — No external interfaces affected.

No findings above the severity threshold.

---

Protected paths detected — this PR modifies files under one or more
protected paths. The review agent cannot approve PRs that touch these paths.
A human reviewer must approve this PR.

Protected files in this PR:

• AGENTS.md

---

Labels: PR adds documentation to AGENTS.md

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 5:38 PM UTC · Completed 5:44 PM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

No actionable review feedback was found. The review body was empty and the human instruction was 'none'. The PR modifies only AGENTS.md (a protected path). No code changes were made.

Disagreed (1):

1. No actionable review feedback found: The review body file was empty and HUMAN_INSTRUCTION was 'none'. No review findings to address. The only file changed by this PR (AGENTS.md) is a protected path that the fix agent should not modify without explicit instruction.

Tests: passed

_{Updated by fullsend fix agent}

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:50 AM UTC · Completed 4:57 AM UTC
Commit: d8e3df7 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] technical-accuracy

The claim that top-level return in an executed script 'causes a hard error' is imprecise. Without set -e, return at top level prints a diagnostic to stderr and exits with code 2, but the script continues execution — the guard silently fails. Only with set -e does the non-zero exit code cause an abort. Both outcomes are bugs but for different reasons, and the current wording could lead reviewers to skip flagging this pattern in scripts without set -e.

Suggested fix: Reword to describe both failure modes: 'At the top level of an executed script, return fails with exit code 2 and prints a diagnostic. Without set -e, the script continues past the failed return (the guard silently fails to skip execution). With set -e (commonly set -euo pipefail), the non-zero exit code aborts the script.'

[fullsend-ai-review]

[low] edge-case

The example pattern [[ -n "${LOADED:-}" ]] && return 0 only triggers the return error when the guard variable is set. When unset (common case), && short-circuits and return is never reached, making this a latent bug that passes basic testing but fails when the variable leaks from the environment.

Suggested fix: Consider adding: 'The error only triggers when the guard condition is true (variable is set); when false, && short-circuits and return is never reached, making this a latent bug that may not manifest in simple testing.'