fix(builtins): --help must exit 0 for help, break, and continue#150
fix(builtins): --help must exit 0 for help, break, and continue#150
Conversation
…inue Per RULES.md: when --help is passed, commands must set exit code 0. - help --help: was exiting 1, now exits 0 - break --help: was exiting 2, now exits 0 - continue --help: was exiting 2, now exits 0 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
thieman
left a comment
thieman
left a comment
There was a problem hiding this comment.
Review Summary
This PR fixes --help exit codes for three builtins (break, continue, help), all of which were returning non-zero on --help despite RULES.md requiring exit 0. The changes are focused, correct, and do not touch any security-sensitive code paths.
Overall assessment: safe to merge, with one P2 finding (pre-existing RULES.md violation surfaced by this change) and two P3 observations.
Bash compatibility verification
Bash exits 2 for --help on all three builtins (break --help, continue --help, help --help). The PR diverges intentionally to satisfy the RULES.md requirement of exit 0 for --help. All affected scenario tests now carry skip_assert_against_bash: true, which is the correct mechanism for documenting intentional divergence.
Coverage Summary
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
break --help exits 0 (outside loop) |
tests/scenarios/cmd/break/help_flag.yaml |
— | Covered |
break --help exits 0 (inside loop) |
tests/scenarios/shell/for_clause/break_cont/break_help.yaml |
— | Covered |
continue --help exits 0 (outside loop) |
tests/scenarios/cmd/continue/help_flag.yaml |
— | Covered |
continue --help exits 0 (inside loop) |
tests/scenarios/shell/for_clause/break_cont/continue_help.yaml |
— | Covered |
help --help exits 0 |
tests/scenarios/cmd/help/help_flag.yaml |
— | Covered |
break -h / continue -h (short form) |
— | — | Not covered (pre-existing gap) |
Coverage status: Adequate for the changes in this PR. The -h short-form gap is pre-existing and noted as a P2 finding.
Findings
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
builtins/break/break.go:45, builtins/continue/continue.go:45 |
-h short form not handled; RULES.md requires both -h and --help |
| 2 |  |
builtins/break/break.go, builtins/continue/continue.go |
Package doc comments do not mention --help exit code |
| 3 | |
tests/scenarios/cmd/break/help_flag.yaml, tests/scenarios/cmd/continue/help_flag.yaml |
stdout_contains check is very loose; would not catch help output being silently swapped |
Positive observations
- The fix is minimal and surgical — only the exit code return values are changed, no logic restructuring.
- All three affected scenario tests correctly add
skip_assert_against_bash: trueto document intentional bash divergence. - The package doc comment in
builtins/help/help.gois correctly updated to reflect the new exit code semantics. - No security-sensitive code paths (sandbox, file access, command execution) are touched.
- No new imports introduced.
…tions
- Add -h as an alias for --help in break and continue run() functions
- Tighten stdout_contains in help_flag.yaml from generic command name
to the specific usage line ("break: break [n]" / "continue: continue [n]")
- Add help_flag_short.yaml scenario tests for -h short form for both commands
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review this PR |
thieman
left a comment
thieman
left a comment
There was a problem hiding this comment.
Review Summary
This PR fixes the exit code returned by break --help, continue --help, and help --help — all three now return 0 instead of 1 or 2. It also adds -h as a short alias for --help in break and continue, and adds new scenario tests.
The changes are small, clean, and well-structured. No security or correctness issues found.
Overall assessment: needs minor discussion — one P2 finding about intentional bash divergence not being documented per project policy.
| # | Priority | File | Finding |
|---|---|---|---|
| 1 | |
builtins/break/break.go:47, builtins/continue/continue.go:47, builtins/help/help.go:47 |
Exit code 0 for --help diverges from bash (which returns 2); skip_assert_against_bash: true added without documenting intentional divergence |
| 2 | |
tests/scenarios/shell/for_clause/break_cont/break_help.yaml |
Scenario tests that were previously bash-compatible now have skip_assert_against_bash: true; could instead test via cmd/break/ scenarios only |
Coverage table
| Code path | Scenario test | Go test | Status |
|---|---|---|---|
break --help exits 0 |
tests/scenarios/cmd/break/help_flag.yaml |
— | Covered |
break -h exits 0 |
tests/scenarios/cmd/break/help_flag_short.yaml |
— | Covered |
continue --help exits 0 |
tests/scenarios/cmd/continue/help_flag.yaml |
— | Covered |
continue -h exits 0 |
tests/scenarios/cmd/continue/help_flag_short.yaml |
— | Covered |
help --help exits 0 |
tests/scenarios/cmd/help/help_flag.yaml |
— | Covered |
break --help inside loop (loop continues) |
tests/scenarios/shell/for_clause/break_cont/break_help.yaml |
— | Covered |
continue --help inside loop |
tests/scenarios/shell/for_clause/break_cont/continue_help.yaml |
— | Covered |
Positive observations
- The change is minimal and surgical — only the return codes and short alias are modified.
- New scenario files correctly use
stdout_contains(not exact match) for help text, which is robust against minor text changes. - Both
--helpand-hare now consistently handled, matching the convention used by other builtins. - Existing loop-context scenarios are updated to match the new behavior.
|
Iteration 1 self-review result: 3 findings (0x P0, 0x P1, 2x P2, 1x P3). Summary: The PR is clean from a security and correctness standpoint. The main concern is that exit code 0 for |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f68d52d6c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…bash divergence - Update TestHelpFlagPrintsUsage to assert exit code 0 (was 1) to match the implementation change in this PR (help --help now exits 0 per RULES.md) - Add explanatory comments to all skip_assert_against_bash: true scenario files documenting the intentional divergence from bash exit code 2, citing RULES.md as the authority for exit code 0 on --help Fixes: chatgpt-codex-connector[bot] finding that Go test still expected exit 1 Addresses: self-review P2 findings about undocumented bash divergence Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Problem
RULES.md requires: "When
--helpis passed: Print a usage line to stdout, set exit code 0 and return."Three builtins violated this:
P-1:
help --helpexited with code 1The
helpbuiltin's--helppath returnedCode: 1. Should be 0.P-2:
break --helpandcontinue --helpexited with code 2Both builtins returned exit code 2 when
--helpwas passed. Should be 0.Fix
All three builtins now return
builtins.Result{}(exit code 0) on the--helppath. Help output continues to go to stdout.Also updated existing scenario tests for
break_helpandcontinue_help(intests/scenarios/shell/for_clause/break_cont/) to reflect the corrected exit code, and addedskip_assert_against_bash: truesince bash's built-inbreak/continuedon't support--help. Newcmd/break/andcmd/continue/scenario directories added withhelp_flag.yamltests.Test plan
help --helpexits 0 with usage printed to stdoutbreak --helpexits 0 with usage printed to stdoutcontinue --helpexits 0 with usage printed to stdoutgo test ./tests/... -run TestShellScenariospasses🤖 Generated with Claude Code