docs(#181): add shell return-vs-exit correctness rule to AGENTS.md

[fullsend-ai-coder]

Add a new "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 statement 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. This new rule instructs reviewers to flag top-level return (outside a function body) in executed scripts as a medium-severity correctness finding, especially when code is being inlined from a sourced library.

Note: pre-commit could not run in-sandbox due to a network error installing shellcheck-py (exit 3). The post-script runs an authoritative pre-commit check on the runner.

---

Closes #181

Post-script verification

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

[fullsend-ai-review]

Review — Approve

PR: #204 — docs(#181): add shell return-vs-exit correctness rule to AGENTS.md
Scope: 1 file changed, +10/−0 (documentation only)
Linked issue: #181

Summary

This PR adds a new "Shell scripting" section to AGENTS.md with guidance on the return-vs-exit distinction in executed vs sourced scripts. The change directly addresses issue #181, which documented a real incident where the review agent failed to catch an invalid top-level return in an inlined script.

Dimensions reviewed

Dimension	Verdict	Notes
Correctness	✅ Pass	All technical claims verified as accurate: return at script top level produces exit status 2, set -e triggers errexit on the non-zero status, and the load-guard example correctly illustrates the problem. No internal inconsistencies.
Security	✅ Pass	No secrets, injection patterns, Unicode steganography, or permission changes. The agent-directed instruction follows the established pattern in AGENTS.md.
Intent & coherence	✅ Pass	Scope matches issue #181 exactly — no scope creep. The issue mentioned adding content "after existing gh api --paginate guidance" in a Shell scripting section, but no such section exists in this fork. The PR correctly creates a new ## Shell scripting section instead.
Style & conventions	✅ Pass	Heading hierarchy (##/###), backtick formatting, and section placement are consistent with the rest of AGENTS.md.
Docs currency	✅ Pass	No stale cross-references created. CLAUDE.md correctly links to AGENTS.md. The load-guard pattern exists in the codebase (scripts/lib/github-api-csma.sh), confirming the example is grounded in real usage.
Cross-repo contracts	⏭ Skipped	No exported interfaces, APIs, or schemas modified.

Minor observation (low)

Style — bold instruction prefix (AGENTS.md): The new section uses **When reviewing shell scripts:** while existing sections use the broader **When reviewing PRs:**. The more specific phrasing is arguably clearer in context (it scopes the guidance to shell scripts specifically), so this does not warrant a change — noting for consistency awareness.

Commit messages

• docs(#181): add shell return-vs-exit correctness rule to AGENTS.md — correct type, appropriate scope ✓
• fix(#181): clarify return behavior precision in shell scripting rule — addresses review feedback per COMMITS.md guidance ✓

---

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

Previous run

Review — approve

PR #204 adds a "Shell scripting" section to AGENTS.md documenting the return vs exit correctness rule for executed scripts, as requested by #181.

Summary

This is a focused, documentation-only change (10 lines added to AGENTS.md) that adds a new review rule based on a real bug found in PR fullsend-ai#3182. The added content is technically accurate, properly authorized by issue #181, follows existing formatting conventions, and is placed logically in the document structure. No security concerns were identified.

Findings

All findings are low severity and non-blocking.

1. Technical accuracy nuance (low) — AGENTS.md
The statement "return causes a hard error" is slightly imprecise. A top-level return in an executed script produces exit status 2 and an error message, but without set -e the script continues execution. The abort specifically requires set -e (which is part of set -euo pipefail). The practical guidance is correct for the common case (scripts using set -euo pipefail), but the phrasing could mislead a reviewer encountering a script without set -e.

2. Sub-agent awareness (low, downgraded from medium) — correctness.md
The correctness sub-agent template (internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/correctness.md) does not reference this new shell rule. However, AGENTS.md is the correct location for repo-specific review rules — the scaffold template is generic. Whether sub-agents automatically pick up AGENTS.md rules during dispatch is an orchestrator concern, not a gap in this PR. Worth verifying as a follow-up.

3. Section placement (low) — AGENTS.md
The new ## Shell scripting section sits between the ### Running e2e tests subsection (under ## Go code) and ## Forge abstraction. This is a reasonable placement — it groups language-specific guidance before architectural rules — though some might prefer it directly after the ## Go code section. Cosmetic only.

Verdict

Approve. The change is technically sound, properly scoped to issue #181, and follows the document's existing conventions. The low-severity findings are non-blocking observations for potential follow-up.

---

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

Previous run (2)

Review — PR #204

Verdict: comment · 1 medium, 2 low findings · non-blocking

Overview

This PR adds a "Shell scripting" section to AGENTS.md with a return vs exit correctness rule, implementing issue #181's request almost verbatim. The change is well-motivated by a documented real-world bug (upstream PR fullsend-ai#3182) where the review agent dismissed a top-level return in an executed script as "harmless dead code."

The core claims are technically correct: return at the top level of an executed bash script is invalid, the described load-guard pattern is real and common, and under set -euo pipefail the script does abort. The scope is tight (10 lines, 1 file) and traces directly to authorized work.

Findings

1. Imprecise failure mode description — return behavior without set -e differs from description

Severity	medium
Category	technical-accuracy
File	AGENTS.md

The text states: `return` causes a hard error — and under `set -euo pipefail`, this aborts the script.

The phrase "hard error" implies a fatal, unrecoverable condition. In practice:

• Without set -e: a top-level return produces bash: return: can only 'return' from a function or sourced script on stderr and sets exit status 2, but execution continues past the return. This is a control-flow bug (unintended fallthrough), not an abort.
• With set -e: the non-zero exit status from the failed return triggers set -e, which then aborts the script.

The abort is caused specifically by -e, not by -u or -o pipefail. Attributing the behavior to the full set -euo pipefail triad is slightly misleading. A reviewer encountering a script without set -e might misdiagnose the symptom (continuation vs. abort).

Suggested improvement: Clarify that without set -e, return produces a non-fatal error and execution falls through — a different but equally real bug. Under set -e (typically via set -euo pipefail), the non-zero exit status causes abort.

This doesn't change the review guidance outcome (flag it either way), but precise failure mode description helps with root cause analysis.

2. Edge case: dual-mode scripts not addressed

Severity	low
Category	edge-case-omission
File	AGENTS.md

Some scripts are designed to work both when sourced and when executed, typically using if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then .... In such dual-mode scripts, a top-level return is valid in the sourced code path. A reviewer following this rule literally might flag it incorrectly. This is a minor edge case and may not warrant inclusion, but is worth noting for completeness.

3. Minor style inconsistency in reviewer guidance label

Severity	low
Category	formatting-consistency
File	AGENTS.md

The new section uses **When reviewing shell scripts:** while the existing convention in AGENTS.md is **When reviewing PRs:** (used in Forge abstraction and ADR sections). The deviation is defensible — it scopes the guidance more precisely — but breaks the established formatting pattern.

What looks good

• Authorization: Change traces directly to issue [ARCHIVED] Original issue 181 #181 with matching scope — no scope creep.
• Technical accuracy: Core claims about return invalidity, load-guard patterns, and abort behavior under set -euo pipefail are verified correct.
• Security: No secrets, injection patterns, Unicode steganography, or permission changes. The review directive follows the existing instructional style in AGENTS.md.
• Documentation currency: No staleness introduced. README.md does not maintain a section index for AGENTS.md.
• Section placement: Creating ## Shell scripting as a peer to ## Go code is a reasonable structural choice for language-specific guidance.

Previous run (3)

Review — PR #204

Verdict: Approve

Summary

This PR adds a 10-line "Shell scripting" section to AGENTS.md with guidance on the return vs exit distinction in executed vs sourced shell scripts. The change directly implements the specification from issue #181, which documented a missed review finding where a top-level return in an executed script was dismissed as "harmless dead code."

Dimension results

Dimension	Result
Correctness	✅ Pass (1 low finding)
Security	✅ Pass
Intent & coherence	✅ Pass — change matches issue #181 exactly, scope is tight
Style & conventions	✅ Pass — follows all established AGENTS.md patterns
Documentation currency	✅ Pass
Cross-repo contracts	⏭ Skipped — no exported interfaces modified

Findings

[low] Technical precision — "hard error" characterization (AGENTS.md)

The phrase "return causes a hard error" is slightly imprecise. Without set -e, a top-level return in an executed script emits an error diagnostic and sets exit status 2, but the script continues executing subsequent commands. The abort only occurs when set -e (errexit) is active, because the non-zero exit status triggers errexit. The current wording could be read as implying return alone terminates the script.

The guidance is pragmatically correct for scripts following the set -euo pipefail convention (which is standard in this project), and the sentence does qualify with "under set -euo pipefail." This is a minor precision improvement, not a blocking issue.

Suggested rewording: "return produces an error (exit status 2) — and under set -e (commonly part of set -euo pipefail), this non-zero status triggers errexit and aborts the script."

Notes

• The change is properly authorized by issue [ARCHIVED] Original issue 181 #181 and matches the proposed text verbatim.
• Placement after the "Running e2e tests" section is reasonable; the issue referenced an existing "Shell scripting" section with gh api --paginate guidance that doesn't exist in this fork, so creating a new top-level section is the correct adaptation.
• The repo has 65+ shell scripts across multiple directories, all using return correctly (only in functions or sourced libraries), confirming the relevance of this guidance.

---

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 shell scripting documentation to AGENTS.md

[guyoron1]

/fs-review

[guyoron1]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:19 AM UTC · Completed 10:27 AM UTC
Commit: a2afa8b · View workflow run →

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 11:12 AM UTC · Completed 11:20 AM UTC
Commit: a2afa8b · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 1 of 3 review findings by fixing the technical accuracy issue with return behavior description. Disagreed with 2 cosmetic findings (section placement and formatting consistency) that the reviewer themselves rated as low/non-blocking.

Fixed (1):

1. Technical accuracy: return causes a hard error is imprecise (AGENTS.md): Reworded the return-vs-exit rule to precisely describe the failure mode: return produces exit status 2 (not a hard error), and the abort is specifically from set -e (errexit), not the full set -euo pipefail triad. New wording: 'return produces an error (exit status 2) — and under set -e (commonly part of set -euo pipefail), this non-zero status triggers errexit and aborts the script.'

Disagreed (2):

1. Section placement: Shell scripting section between e2e tests and Forge abstraction: The reviewer rated this as low severity and cosmetic only. The current placement groups language-specific guidance before architectural rules, which is a reasonable structural choice. No change needed.
2. Formatting consistency: When reviewing shell scripts vs When reviewing PRs: The reviewer themselves noted the deviation is defensible since it scopes the guidance more precisely to shell scripts rather than all PRs. The narrower scope improves clarity for reviewers.

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:27 AM UTC · Completed 11:34 AM UTC
Commit: a2afa8b · View workflow run →