Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fullsend-ai-coder wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ All git forge operations (GitHub API calls, PR comments, issue creation, workflo

**When reviewing PRs:** Flag any direct `exec.Command("gh", ...)`, raw GitHub API calls, or other forge-specific operations outside `internal/forge/github/` as a medium-severity or higher finding. This is an architectural violation, not a style preference.

## Shell scripting

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

@fullsend-ai-review fullsend-ai-review Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] technical-accuracy

The phrase "hard error" is misleading. In bash, a top-level return in an executed script produces an error message with exit status 2, but execution continues past the return line unless set -e is active. Without set -e, the guard silently fails and execution falls through (silent wrong behavior), which is arguably worse than an abort. The documentation implies the script always halts.

Suggested fix: Reword to: "return at the top level of an executed script produces a non-fatal error in bash — it prints a diagnostic but execution continues past it. Under set -e (or set -euo pipefail), the non-zero exit status triggers an immediate abort. Without set -e, the guard silently fails and execution falls through."

@fullsend-ai-review fullsend-ai-review Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] technical-accuracy

The abort behavior is attributed to set -euo pipefail as a unit, but only -e (errexit) is relevant. The -u (nounset) and -o pipefail flags do not contribute to this mechanism.

Suggested fix: Say "under set -e" or add a parenthetical clarifying that -e is the specific flag that triggers the abort.

@fullsend-ai-review fullsend-ai-review Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] technical-accuracy

The claim about return behavior is bash-specific but stated as universal. In POSIX sh (dash), return at top level of an executed script silently behaves like exit — no error, no diagnostic. Adding "In bash," as a qualifier would improve precision.

Suggested fix: Add "In bash," at the start of the sentence, or note the shell-dependent behavior.


`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.

## Architecture Decision Records (ADRs)

These rules apply whenever you touch `docs/ADRs/` or review a PR that does. Full authoring guidance is in [`skills/writing-adrs/SKILL.md`](skills/writing-adrs/SKILL.md); invoke that skill when writing a new ADR.
Expand Down
Loading