Implement if/elif/else clause support

[AlexandreYang]

Summary

• Adds POSIX if/elif/else conditional construct support to the shell interpreter
• Removes the validation block on *syntax.IfClause while keeping child node validation (blocked features like subshells in conditions are still rejected)
• Adds case *syntax.IfClause in cmd() dispatcher following mvdan/sh's reference pattern
• Fixes stmts() to check break/continue/exit between statements, ensuring break inside if bodies within loops propagates correctly
• Clamps excess break/continue levels at the outermost loop to match bash behavior (e.g., break 99 with 1 loop)

Test plan

• 33 YAML scenario tests covering basic if/else, elif chains, nested ifs, conditions (pipes, &&/||, test builtin, negation), exit codes, and loop interaction
• 30 Go unit tests in if_clause_test.go
• 13 pentest tests for deeply nested ifs, context cancellation, blocked features in conditions, and break/continue edge cases
• All tests pass: go test ./interp/... ./tests/...
• Bash comparison tests pass: RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash

🤖 Generated with Claude Code

[AlexandreYang]

@DataDog @codex review

[datadog-datadog-prod-us1]

View session in Datadog

Bits Dev status: ✅ Done

CI Auto-fix: Disabled | Enable

Comment @DataDog to request changes

[AlexandreYang]

Implementation Results

Core Implementation (12 lines)

• Removed IfClause validation block in validate.go (child node validation still rejects blocked features like subshells in conditions)
• Added case *syntax.IfClause in runner_exec.go:cmd() following mvdan/sh's reference pattern: evaluate Cond stmts → if exit 0, run Then → else reset exit and recurse into Else
• Guards for break/continue/exit between condition and body execution

Bug Fix Discovered During Pentest

stmts() was not checking breakEnclosing/contnEnclosing between statements, so break inside an if body within a for loop didn't stop subsequent statements in the same body. Fixed by adding the check to stmts(). Also added clamping of excess break/continue levels at the outermost loop to match bash behavior (e.g. break 99 with 1 loop depth).

Test Coverage (45 new files, 844 lines)

Category	Count	Examples
Basic if/elif/else	9	true/false conditions, elif chains, nested
Conditions	11	pipelines, &&/`
Exit codes	4	then/else/elif branch codes, no-branch-exits-zero
Loop interaction	4	break/continue in if/else within for loops, for-in-if-body
Edge cases	5	commands before/after, assignments in body, if-as-condition, exit-in-then, blocked-in-condition
Go unit tests	30	end-to-end through shell runner
Pentest tests	13	100-deep nesting, 50 elif branches, context cancellation, break/continue edge cases, blocked features

Verification

• ✅ go test ./interp/... ./tests/... — all pass
• ✅ RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash — all pass (byte-for-byte match with GNU bash on debian:bookworm-slim)

[datadog-prod-us1-6]

Something went wrong! If you want me to try again, please let me know.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. More of your lovely PRs please.

ℹ️ 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".

[AlexandreYang]

Review Summary

Reviewed the implementation of if/elif/else clause support across 3 commits (validation unblock, execution logic, stmts fix, 46 YAML scenarios, 13 pentest tests).

Overall assessment: safe to merge ✅

Security

• Removing *syntax.IfClause from validate.go is safe — the AST walker still recurses into child nodes, so blocked features (subshells, command substitution, function declarations, background &) inside if conditions/bodies are still rejected before execution. Verified by blocked_in_condition.yaml and pentest tests.
• No new filesystem access, imports, or command execution paths introduced.
• The stmts() break/continue propagation fix is a security improvement — without it, statements after break inside an if body within a loop could still execute.

Bash Compatibility

• POSIX if semantics correctly implemented: execute Cond → if exit 0 then Then, else reset exit to 0 and recurse into Else.
• No-branch-taken → exit 0 matches bash.
• Excess break/continue level clamping (break 99 with 1 loop) matches bash behavior.
• All YAML scenarios (except intentionally skipped ones) verified against bash.

Correctness

• IfClause correctly checks exit.exiting, breakEnclosing, contnEnclosing after evaluating Cond before branching — consistent with the BinaryCmd pattern.
• stmts() now stops iterating when break/continue/exit is pending — fixes a pre-existing bug exposed by this feature.
• Excess level clamping at outermost for loop prevents post-loop commands from being skipped.

Test Coverage

• 46 YAML scenario files organized by concern (basic, conditions, exit_code, loop_interaction, edge_cases)
• 13 pentest Go tests covering deeply nested ifs (100 levels), 50 elif branches, context cancellation, break/continue edge cases, exit in various positions, blocked features in conditions/body, sequential ifs, if in pipeline
• Good coverage of test/[ builtin combinations with if/elif

#	Priority	File	Finding
1		tests/scenarios/shell/if_clause/edge_cases/if_in_pipeline.yaml	Misleading description says "blocked" but test expects success
2		tests/scenarios/shell/if_clause/edge_cases/exit_in_then.yaml	skip_assert_against_bash: true may be unnecessary — bash produces the same output

Positive Observations

• Clean, minimal implementation (13 lines for IfClause case) following existing patterns
• The stmts() fix is well-motivated and well-tested — a genuine pre-existing bug fix
• Excellent test organization with YAML scenarios grouped by concern
• Pentest tests cover important edge cases (deeply nested, context cancellation, break/continue propagation)

[AlexandreYang]

Code Review Results

Assessment: Safe to merge ✅ — no P0/P1/P2 findings.

Two P3 (nice-to-have) suggestions:

1. if_in_pipeline.yaml — description says "blocked - subshell needed for pipe" but the test expects success; should fix the description
2. exit_in_then.yaml — skip_assert_against_bash: true may be unnecessary since bash produces the same output

See full review for details.

[AlexandreYang]

@DataDog @codex review

[datadog-datadog-prod-us1-2]

Reviewed. I don’t see any blocking issues in this PR.

✅ What I checked

• if/elif/else execution path in interp/runner_exec.go
• Removal of IfClause rejection in interp/validate.go while keeping child-node safety validation
• Control-flow propagation (break/continue/exit) through stmts() and loop handling
• Bash-compat behavior for excessive loop levels (break 99, continue 99 at outermost loop)

✅ Validation run

• go test ./interp -run 'TestIf' (pass)
• go test ./tests -run TestShellScenarios -timeout 300s (pass)
• RSHELL_BASH_TEST=1 go test ./tests -run TestShellScenariosAgainstBash -timeout 240s (pass)

ℹ️ Note

A full go test ./interp/... run showed failures in:

• TestHeadPermissionDenied
• TestTailPermissionDenied

These appear unrelated to the if-clause changes in this PR.

If you want, I can do a second pass focused specifically on CI-hardening and propose extra edge-case tests likely to catch regressions early.

[AlexandreYang]

Review Summary (2nd pass — post-fix)

Re-reviewed the full implementation of if/elif/else clause support after the fix commit (f6117f0): 4 commits, 3 modified Go files, 1 new test file, 46 YAML scenarios.

Overall assessment: safe to merge ✅ (would APPROVE but can't approve own PR)

No P0, P1, or P2 findings. Previous P3 findings have been addressed.

Security

• Sandbox integrity preserved: Removing *syntax.IfClause from validate.go is safe — the AST walker returns true for unhandled nodes, so it still recurses into child nodes. Blocked features (subshells, command substitution, function declarations, background &) inside if conditions/bodies remain rejected before execution.
• No new filesystem access: The IfClause handler only calls stmts() and cmd() — no direct os.Open, no new imports.
• Break/continue propagation fix: The stmts() change prevents executing statements after break/continue in if bodies within loops — a pre-existing bug fix with security implications.

Bash Compatibility

• POSIX if semantics correctly implemented. All 46 YAML scenarios now assert against bash (no unnecessary skip_assert_against_bash remaining).
• Excess break/continue clamping matches bash behavior.

Correctness

• Context cancellation: fatal() sets exiting = true, so context cancellation propagates correctly through the IfClause r.exit.exiting check.
• stmts() callers: Block, IfClause, and api.go top-level — break/continue check is correct for all. At top level, counters are always 0 so early return never fires.
• loopStmtsBroken: Doesn't explicitly check exit.exiting but relies on stop() inside stmt() — correct and consistent.

Positive Observations

• Clean, minimal 13-line IfClause implementation following existing patterns
• The stmts() fix addresses a genuine pre-existing bug (break in brace group within loop)
• Excellent test coverage: 46 YAML scenarios + 13 pentest Go tests
• Previous P3 findings (misleading description, unnecessary skip_assert_against_bash) addressed in fix commit

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b5417e24c

ℹ️ 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".

[AlexandreYang]

Code Review Summary

Verdict: APPROVE (submitted as COMMENT since GitHub disallows self-approval)

This is a clean, well-implemented PR that adds POSIX if/elif/else support. The core implementation in runner_exec.go correctly handles all branches, follows the mvdan/sh AST structure (where Else is an *IfClause for both elif and bare else), and properly propagates exit/break/continue signals.

Key observations

Correctness (all good):

• The condition-check logic (r.exit.ok() after r.stmts(ctx, cm.Cond)) correctly uses the last command's exit code, matching bash behavior for multi-statement conditions (if false; true; then ...).
• The r.exit = exitStatus{} reset before checking cm.Else ensures the "no branch taken, no else" case returns exit code 0, matching bash.
• The recursive r.cmd(ctx, cm.Else) correctly handles both elif (re-enters IfClause case) and bare else (empty Cond, body in Then).
• Break/continue/exit propagation from conditions is properly short-circuited.

The stmts() fix is broader than if-clause (P3):
The added early-return check in stmts() for break/continue/exit also fixes a pre-existing latent bug in brace groups (Block): before this change, { break; echo hello; } inside a loop would incorrectly execute echo hello after break, since stop() only checks exiting and context errors, not breakEnclosing/contnEnclosing. This is a good fix worth noting.

Break/continue clamping (correct):
The clamping of excess breakEnclosing/contnEnclosing at the outermost loop matches bash behavior (break 99 with 1 loop just exits that loop).

Test coverage is excellent:
33 YAML scenario tests + 13 Go pentest tests covering deep nesting, context cancellation, blocked features, and edge cases. Good use of Go tests for the things scenarios cannot express (deep nesting, timeouts).

Minor suggestions (P3)

• A few Go pentest tests (TestIfPentestSequentialIfs, TestIfPentestBreakInCondition, TestIfPentestContinueInElif) could be scenario tests per project preference in AGENTS.md, though having them as Go tests is not wrong.
• Consider mentioning in the PR description that the stmts() fix also improves brace group behavior, since it's a broader correctness improvement.

Overall, excellent work. No P0/P1/P2 findings.

[AlexandreYang]

@DataDog @codex make a comprehensive code and security reviews

[datadog-prod-us1-3]

I can only run on private repositories.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. More of your lovely PRs please.

ℹ️ 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".

[AlexandreYang]

Code Review — PR #45: Implement if/elif/else

Overall assessment: safe to merge ✅

(Note: Cannot self-approve own PR, submitting as COMMENT)

This is a clean, well-structured implementation of if/elif/else support. The change is minimal, correct, and thoroughly tested.

Review Summary

#	Priority	File	Finding
—	—	—	No issues found

Security

• Sandbox integrity: No new command execution paths introduced. The if clause handler only controls flow using existing r.stmts and r.cmd calls. All commands inside if conditions/bodies are still validated by validateNode before execution. ✓
• Break/continue propagation: Correctly stops executing remaining statements in if bodies when break/continue/exit fires. ✓
• No new imports or external calls: The implementation uses only existing interpreter primitives. ✓

Bash Compatibility

• Condition evaluation: Uses exit code of last statement in condition list — matches bash. ✓
• elif chains: Recursive IfClause.Else handling correctly mirrors bash's elif/else semantics. ✓
• Exit code when no branch taken (if false; then ...; fi): Correctly resets to 0, matching bash. ✓
• Break/continue through if inside loops: Correctly propagates, matching bash. ✓

Correctness

• stmts early-return fix (line 207): The new exit.exiting || breakEnclosing > 0 || contnEnclosing > 0 check is essential — without it, statements after break/continue/exit in if bodies (and brace groups) would still execute. This is a broader correctness fix that also benefits Block handling. ✓
• Excess continue clamping (line 183-185): Correctly handles continue 99 at the outermost loop by clamping and continuing iteration, matching bash behavior. ✓
• Post-loop clamping (line 192-195): Safety net that clears excess break/continue levels at the outermost loop boundary. ✓
• r.exit = exitStatus{} in else branch (line 162): Correctly resets exit before processing elif/else — the break guard at line 156-158 ensures fatal exits are not cleared. ✓

Test Coverage

Code path	Scenario test	Go test	Status
Basic if true/false	basic_if.yaml, basic_if_false.yaml	—	Covered
if-else	if_else.yaml	—	Covered
if-elif-else chain	if_elif.yaml, if_elif_else.yaml, if_elif_first_true.yaml, if_elif_second_true.yaml	—	Covered
Conditions (&&, pipes, !, test)	14 condition scenarios	—	Covered
Exit codes (then, else, elif, no-branch)	4 exit_code scenarios	—	Covered
Loop interaction (break, continue)	break_in_if.yaml, continue_in_if.yaml, break_in_else.yaml, for_in_if_body.yaml	TestIfPentestBreakInCondition, TestIfPentestContinueInElif	Covered
Nested if, if-as-condition	nested_if.yaml, nested_if_else.yaml, if_condition_as_nested_if.yaml	—	Covered
Edge cases (pipeline, assignment, blocked features)	8 edge case scenarios	—	Covered
Deep nesting (100 levels)	—	TestIfPentestDeeplyNestedIf, TestIfPentestDeeplyNestedElif	Covered
Context cancellation	—	TestIfPentestContextCancellation	Covered
Exit in various positions	—	TestIfPentestExitInCondition/Else/Elif	Covered
Blocked features in if	—	TestIfPentestSubshellInCondition, TestIfPentestFuncDeclInBody, TestIfPentestBackgroundInBody	Covered
continue 99 multi-iteration	continue_exceeds_depth_multi_iter.yaml	—	Covered

Coverage: Adequate — all new/changed code paths are covered by scenario and/or Go tests.

Positive Observations

1. Elegant recursive design: Using IfClause.Else recursion for elif/else mirrors the AST structure and keeps the handler minimal (~12 lines).
2. Defensive stmts fix: The early-return in stmts is a broader correctness improvement that benefits brace groups too.
3. Comprehensive test battery: 40+ scenario tests plus pentest Go tests covering deep nesting, context cancellation, blocked features, and break/continue edge cases.
4. Validation consistency: The blocked-command tests were cleanly updated to use while instead of if, maintaining test coverage for the validation path.
5. Documentation updated: SHELL_FEATURES.md correctly reflects the new support.

[AlexandreYang]

@DataDog @codex make a comprehensive code and security reviews

[datadog-official]

I can only run on private repositories.

[AlexandreYang]

Review-Fix Loop Summary

• PR: Implement if/elif/else clause support #45
• Iterations completed: 1
• Final status: ✅ CLEAN

Iteration log

#	Review result	Findings	Fixes applied	CI status
1	APPROVE	0	—	Passing (8/8)

Final state

• Self-review: APPROVE (no findings)
• Unresolved external comments: 0
• CI: Passing (CodeQL, compliance, Test ubuntu/macos/windows, Test against Bash, Analyze, devflow/mergegate)

Remaining issues

None. PR is clean and ready to merge.

[AlexandreYang]

Code Review (iteration 2) — PR #45: Implement if/elif/else

Overall assessment: safe to merge ✅ — No findings.

Re-reviewed at the same HEAD (aa2a6d7). All previous analysis holds:

• Security: No sandbox escapes. If clause uses existing r.stmts/r.cmd primitives; validateNode still walks the full AST.
• Bash compatibility: Condition evaluation, elif chains, exit codes, break/continue propagation all match bash.
• Correctness: stmts early-return fix is essential and correct. Excess break/continue clamping at outermost loop is correct.
• Test coverage: Adequate — 40+ scenario tests + pentest Go tests.
• No P0–P3 findings.

[AlexandreYang]

@DataDog @codex make a comprehensive code and security reviews

[datadog-prod-us1-3]

I can only run on private repositories.

[AlexandreYang]

Review-Fix Loop Summary (Run 2)

• PR: Implement if/elif/else clause support #45
• Iterations completed: 1
• Final status: ✅ CLEAN

Iteration log

#	Review result	Findings	Fixes applied	CI status
1	APPROVE	0	—	Passing (8/8)

Final state

• Self-review: APPROVE (no findings)
• Unresolved external comments: 0
• CI: Passing (CodeQL, compliance, Test ubuntu/macos/windows, Test against Bash, Analyze, devflow/mergegate)

Remaining issues

None. PR is clean and ready to merge.

[AlexandreYang]

/merge

[gh-worker-devflow-routing-ef8351]

View all feedbacks in Devflow UI.

2026-03-11 21:39:15 UTC ℹ️ Start processing command /merge

---

2026-03-11 21:39:20 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 1m (p90).

---

2026-03-11 21:39:59 UTC ℹ️ MergeQueue: This merge request was merged

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Nice work!

ℹ️ 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".