Add improve-loop skill for systematic shell feature review

[AlexandreYang]

Summary

• Adds new improve-loop skill that systematically reviews and improves every shell builtin command and shell feature
• Enumerates all builtins from interp/builtins/ and shell features from tests/scenarios/shell/ as individual review targets
• Iterates through each target with focused review (security, bash compat, correctness, test coverage, platform), fixes issues, runs tests, and re-reviews until clean

Test plan

• Invoke /improve-loop on a test PR and verify it enumerates all targets
• Verify the loop correctly cycles through targets and commits per-target fixes
• Verify the full sweep re-review catches cross-cutting regressions

🤖 Generated with Claude Code

[AlexandreYang]

Improve Loop — Review Plan

Starting systematic review of 49 targets in randomized order.

Review order

#	Target	Type
1	testcmd	command
2	printf	command
3	function	feature
4	field_splitting	feature
5	input_processing	feature
6	readonly	feature
7	wc	command
8	globbing	feature
9	exit	command
10	head	command
11	brace_group	feature
12	allowed_redirects	feature
13	errors	feature
14	cmd_separator	feature
15	echo	command
16	case_clause	feature
17	redirections	feature
18	ls	command
19	break	command
20	continue	command
21	tail	command
22	simple_command	feature
23	tr	command
24	sed	command
25	if_clause	feature
26	empty_script	feature
27	logic_ops	feature
28	strings_cmd	command
29	heredoc	feature
30	cut	command
31	line_continuation	feature
32	comments	feature
33	blocked_commands	feature
34	true	command
35	allowed_paths	feature
36	grep	command
37	var_expand	feature
38	cat	command
39	uniq	command
40	pipe	feature
41	inline_var	feature
42	blocked_redirects	feature
43	for_clause	feature
44	while_clause	feature
45	negation	feature
46	heredoc_dash	feature
47	environment	feature
48	until_clause	feature
49	false	command

Each target will be reviewed for security, bash compatibility, correctness, test coverage, and platform compatibility. Progress updates will be posted after each iteration.

[AlexandreYang]

Improve Loop — Iteration 1: testcmd

• Type: command
• Status: CLEAN (no issues found)
• Findings: 0
• Fixes applied: None — target was clean
• Tests: N/A (no changes)
• Progress: 1/49 targets reviewed

[AlexandreYang]

Improve Loop — Iteration 2: printf

• Type: command
• Status: CLEAN (no issues found)
• Findings: 0
• Fixes applied: None — target was clean
• Tests: N/A (no changes)
• Progress: 2/49 targets reviewed

[AlexandreYang]

Improve Loop — Iteration 3: function

• Type: feature
• Status: CLEAN (no issues found)
• Findings: 0
• Fixes applied: None — target was clean (functions intentionally blocked at AST validation)
• Tests: N/A (no changes)
• Progress: 3/49 targets reviewed

[AlexandreYang]

Improve Loop — Review Plan

Starting systematic review of 48 targets in randomized order.

Review order

#	Target	Type
1	continue	command
2	cut	command
3	inline_var	feature
4	simple_command	feature
5	comments	feature
6	sed	command
7	environment	feature
8	function	feature
9	empty_script	feature
10	heredoc_dash	feature
11	strings_cmd	command
12	readonly	feature
13	allowed_paths	feature
14	allowed_redirects	feature
15	var_expand	feature
16	blocked_commands	feature
17	blocked_redirects	feature
18	globbing	feature
19	until_clause	feature
20	echo	command
21	wc	command
22	head	command
23	pipe	feature
24	tr	command
25	true	command
26	redirections	feature
27	printf	command
28	field_splitting	feature
29	grep	command
30	for_clause	feature
31	tail	command
32	line_continuation	feature
33	while_clause	feature
34	input_processing	feature
35	testcmd	command
36	false	command
37	heredoc	feature
38	break	command
39	errors	feature
40	case_clause	feature
41	logic_ops	feature
42	ls	command
43	brace_group	feature
44	cmd_separator	feature
45	exit	command
46	cat	command
47	negation	feature
48	uniq	command
49	if_clause	feature

Each target will be reviewed for security, bash compatibility, correctness, test coverage, and platform compatibility. Progress updates will be posted after each iteration.

[AlexandreYang]

Improve Loop — Batch 1

Target	Type	Status	Findings	Fixes
continue	command	CLEAN	0 (false positives after verification)	—
cut	command	CLEAN	0	—
inline_var	feature	FIXED	1 (1xP1)	1 fixed
simple_command	feature	CLEAN	0	—
comments	feature	CLEAN	0	—

• Tests: PASS (scenario + bash comparison)
• Progress: 5/48 targets reviewed
• Commit: [inline_var] Use defer for variable restoration to protect against panics

[AlexandreYang]

Improve Loop — Batch 2

Target	Type	Status	Findings	Fixes
sed	command	CLEAN	0	—
environment	feature	CLEAN	0	—
function	feature	CLEAN	0 (blocked feature, well-tested)	—
empty_script	feature	CLEAN	0	—
heredoc_dash	feature	CLEAN	0	—

• Tests: N/A (no changes)
• Progress: 10/48 targets reviewed

[AlexandreYang]

Improve Loop — Batch 3

Target	Type	Status	Findings	Fixes
strings_cmd	command	CLEAN	0	—
readonly	feature	CLEAN	0 (dead code noted, blocked at parse time)	—
allowed_paths	feature	CLEAN	0 (excellent security design)	—
allowed_redirects	feature	CLEAN	0 (defense-in-depth, no bypasses)	—
var_expand	feature	CLEAN	0 (findings about dead readonly paths)	—

• Tests: N/A (no changes)
• Progress: 15/48 targets reviewed

[AlexandreYang]

Improve Loop — Batch 4

Target	Type	Status	Findings	Fixes
blocked_commands	feature	CLEAN	0	—
blocked_redirects	feature	CLEAN	0	—
globbing	feature	CLEAN	0	—
until_clause	feature	CLEAN	0 (blocked at parse time)	—
echo	command	CLEAN	0	—

• Tests: N/A (no changes)
• Progress: 20/48 targets reviewed

[AlexandreYang]

Improve Loop — Batch 5

Target	Type	Status	Findings	Fixes
wc	command	CLEAN	0	—
head	command	CLEAN	0	—
pipe	feature	CLEAN	0	—
tr	command	CLEAN	0	—
true	command	CLEAN	0	—

• Tests: N/A (no changes)
• Progress: 25/48 targets reviewed

[AlexandreYang]

Improve Loop — Batch 6

Target	Type	Status	Findings	Fixes
redirections	feature	CLEAN	0	—
printf	command	FIXED	1 (1xP1)	1 fixed
field_splitting	feature	CLEAN	0	—
grep	command	FIXED	1 (1xP3)	1 fixed
for_clause	feature	CLEAN	0 (inherent shell design concerns, not bugs)	—

• Tests: PASS
• Fixes:
  • printf: Removed 'q' from length modifiers list (was making %q error handler dead code)
  • grep: Added --help long flag per RULES.md
• Progress: 30/48 targets reviewed

[AlexandreYang]

Improve Loop — Batch 7

Target	Type	Status	Findings	Fixes
tail	command	CLEAN	0	—
line_continuation	feature	CLEAN	0	—
while_clause	feature	CLEAN	0 (blocked at parse time)	—
input_processing	feature	CLEAN	0	—
testcmd	command	CLEAN	0	—

• Tests: N/A (no changes)
• Progress: 35/48 targets reviewed

[AlexandreYang]

Improve Loop — Batch 8

Target	Type	Status	Findings	Fixes
false	command	CLEAN	0	—
heredoc	feature	CLEAN	0 (design choices, not bugs)	—
break	command	CLEAN	0 (verified correct in batch 1)	—
errors	feature	CLEAN	0	—
case_clause	feature	CLEAN	0 (blocked at parse time)	—

• Tests: N/A (no changes)
• Progress: 40/48 targets reviewed

[AlexandreYang]

Improve Loop — Batch 9

Target	Type	Status	Findings	Fixes
logic_ops	feature	CLEAN	0	—
ls	command	CLEAN	0	—
brace_group	feature	CLEAN	0	—
cmd_separator	feature	CLEAN	0	—
exit	command	CLEAN	0	—

• Tests: N/A (no changes)
• Progress: 45/48 targets reviewed

[AlexandreYang]

Improve Loop — Batch 10 (Final)

Target	Type	Status	Findings	Fixes
cat	command	CLEAN	0	—
negation	feature	CLEAN	0	—
uniq	command	CLEAN	0	—
if_clause	feature	CLEAN	0	—

• Tests: N/A (no changes)
• Progress: 48/48 targets reviewed — ALL TARGETS COMPLETE

[AlexandreYang]

Improve Loop Summary

• PR: Add improve-loop skill for systematic shell feature review #75 (Add improve-loop skill for systematic shell feature review #75)
• Targets reviewed: 48/48
• Final status: CLEAN

Target results

#	Target	Type	Findings	Fixes	Status
1	continue	command	0 (false positives after bash verification)	—	CLEAN
2	cut	command	0	—	CLEAN
3	inline_var	feature	1 (1xP1)	1 fixed	CLEAN
4	simple_command	feature	0	—	CLEAN
5	comments	feature	0	—	CLEAN
6	sed	command	0	—	CLEAN
7	environment	feature	0	—	CLEAN
8	function	feature	0	—	CLEAN
9	empty_script	feature	0	—	CLEAN
10	heredoc_dash	feature	0	—	CLEAN
11	strings_cmd	command	0	—	CLEAN
12	readonly	feature	0	—	CLEAN
13	allowed_paths	feature	0	—	CLEAN
14	allowed_redirects	feature	0	—	CLEAN
15	var_expand	feature	0	—	CLEAN
16	blocked_commands	feature	0	—	CLEAN
17	blocked_redirects	feature	0	—	CLEAN
18	globbing	feature	0	—	CLEAN
19	until_clause	feature	0	—	CLEAN
20	echo	command	0	—	CLEAN
21	wc	command	0	—	CLEAN
22	head	command	0	—	CLEAN
23	pipe	feature	0	—	CLEAN
24	tr	command	0	—	CLEAN
25	true	command	0	—	CLEAN
26	redirections	feature	0	—	CLEAN
27	printf	command	1 (1xP1)	1 fixed	CLEAN
28	field_splitting	feature	0	—	CLEAN
29	grep	command	1 (1xP3)	1 fixed	CLEAN
30	for_clause	feature	0	—	CLEAN
31	tail	command	0	—	CLEAN
32	line_continuation	feature	0	—	CLEAN
33	while_clause	feature	0	—	CLEAN
34	input_processing	feature	0	—	CLEAN
35	testcmd	command	0	—	CLEAN
36	false	command	0	—	CLEAN
37	heredoc	feature	0	—	CLEAN
38	break	command	0	—	CLEAN
39	errors	feature	0	—	CLEAN
40	case_clause	feature	0	—	CLEAN
41	logic_ops	feature	0	—	CLEAN
42	ls	command	0	—	CLEAN
43	brace_group	feature	0	—	CLEAN
44	cmd_separator	feature	0	—	CLEAN
45	exit	command	0	—	CLEAN
46	cat	command	0	—	CLEAN
47	negation	feature	0	—	CLEAN
48	uniq	command	0	—	CLEAN
49	if_clause	feature	0	—	CLEAN

Changes made

• Commits: 2 commits
• Files changed: interp/runner_exec.go, interp/builtins/printf/printf.go, interp/builtins/grep/grep.go, 2 new test scenarios
• Tests added: 2 new scenario tests (grep --help, printf %q rejection)

Fixes applied

1. [inline_var] Use defer for inline variable restoration to protect against panics corrupting shell state
2. [printf] Remove 'q' from C-style length modifiers — was making the %q: not supported error handler unreachable dead code
3. [grep] Add --help long-only flag per RULES.md (short -h is taken by --no-filename)

Key observations

• The codebase is in excellent shape. 45 out of 48 targets were completely clean on first review.
• Security-critical features (allowed_paths, blocked_commands, blocked_redirects, allowed_redirects) all passed thorough security review with no bypass possibilities found.
• All builtins properly use callCtx.OpenFile() for filesystem access, respecting the sandbox.
• Memory safety is well-enforced across all builtins with bounded buffers and context cancellation.
• Test coverage is comprehensive with scenario tests, pentest suites, and GNU compatibility tests.
• All tests pass including bash comparison tests (byte-for-byte parity).

[AlexandreYang]

Improve Loop (Run 2) — Review Plan

Starting systematic review of 48 targets in randomized order.

Review order

#	Target	Type
1	allowed_paths	feature
2	environment	feature
3	blocked_redirects	feature
4	comments	feature
5	ls	command
6	pipe	feature
7	redirections	feature
8	testcmd	command
9	if_clause	feature
10	continue	command
11	false	command
12	case_clause	feature
13	heredoc	feature
14	tr	command
15	brace_group	feature
16	function	feature
17	negation	feature
18	line_continuation	feature
19	field_splitting	feature
20	globbing	feature
21	blocked_commands	feature
22	errors	feature
23	exit	command
24	readonly	feature
25	printf	command
26	logic_ops	feature
27	grep	command
28	var_expand	feature
29	input_processing	feature
30	allowed_redirects	feature
31	for_clause	feature
32	uniq	command
33	while_clause	feature
34	simple_command	feature
35	cut	command
36	heredoc_dash	feature
37	until_clause	feature
38	tail	command
39	head	command
40	break	command
41	true	command
42	inline_var	feature
43	wc	command
44	sed	command
45	cmd_separator	feature
46	cat	command
47	strings_cmd	command
48	echo	command
49	empty_script	feature

Each target will be reviewed for security, bash compatibility, correctness, test coverage, and platform compatibility. Progress updates will be posted after each iteration.

[AlexandreYang]

Improve Loop (Run 2) — Batch 1

Target	Type	Status	Findings	Fixes
allowed_paths	feature	CLEAN	0	—
environment	feature	CLEAN	0	—
blocked_redirects	feature	CLEAN	0	—
comments	feature	CLEAN	0	—
ls	command	FIXED	1 (1xP1)	1 fixed (added --help flag)

• Tests: PASS
• Progress: 5/48 targets reviewed

[AlexandreYang]

Improve Loop — Review Plan

Starting systematic review of 49 targets in randomized order.

Review order

#	Target	Type
1	errors	feature
2	allowed_paths	feature
3	ls	command
4	input_processing	feature
5	readonly	feature
6	case_clause	feature
7	heredoc	feature
8	cat	command
9	cmd_separator	feature
10	line_continuation	feature
11	false	command
12	inline_var	feature
13	redirections	feature
14	echo	command
15	function	feature
16	uniq	command
17	heredoc_dash	feature
18	exit	command
19	while_clause	feature
20	comments	feature
21	sed	command
22	tr	command
23	pipe	feature
24	field_splitting	feature
25	printf	command
26	until_clause	feature
27	tail	command
28	blocked_redirects	feature
29	head	command
30	var_expand	feature
31	brace_group	feature
32	break	command
33	true	command
34	allowed_redirects	feature
35	testcmd	command
36	grep	command
37	negation	feature
38	for_clause	feature
39	strings_cmd	command
40	blocked_commands	feature
41	simple_command	feature
42	wc	command
43	continue	command
44	logic_ops	feature
45	cut	command
46	globbing	feature
47	empty_script	feature
48	if_clause	feature
49	environment	feature

Each target will be reviewed for security, bash compatibility, correctness, test coverage, and platform compatibility. Progress updates will be posted after each iteration.

[AlexandreYang]

@DataDog @codex review

[datadog-official]

I can only run on private repositories.

[AlexandreYang]

Code Review — Iteration 6

Result: COMMENT (no blocking issues)

Findings

P3 (nit) — 2 findings:

1. printf --help exit code inconsistency: printf --help returns exit code 2 while every other builtin (cat, tr, sed, grep, ls, strings) returns exit code 0 for --help. Since bash's printf doesn't have --help at all, exit code 2 is a defensible choice (it matches bash's response to unknown printf flags), but it's inconsistent with the project's own convention for --help across builtins. Consider aligning to exit 0 for consistency, or documenting the rationale.

2. Minor description mismatch: if_then_semicolons.yaml description says "if/then/else/fi" but the test has no else branch (it's if/then/fi). The companion if_else_semicolons.yaml covers the else case.

Previously addressed (not re-flagged)

• break/continue non-numeric arg exit code 128 — matches bash 5.2 reference
• Pipe reader explicit pr.Close() before wg.Wait() — prevents deadlock
• Defense-in-depth comment on vars.go readonly check — intentional
• All prior iteration findings addressed

Summary

The PR is in good shape after 5 iterations of fixes. The code changes are correct: readonly enforcement is solid with defense-in-depth guards, the exit builtin now matches bash behavior for all error cases, the expandErr handler properly blocks command substitution leaks, parse errors return exit code 2, the pipe fd handling avoids deadlocks, the defer for inline variable restoration protects against panics, and test coverage is thorough. The remaining findings are cosmetic nits only.

[AlexandreYang]

Iteration 6 self-review result: COMMENT

• P0: 0, P1: 0, P2: 0, P3: 2
• Findings:
  1. P3 — printf --help returns exit code 2 while other builtins return 0 for --help (inconsistent but defensible)
  2. P3 — Test description mismatch in if_then_semicolons.yaml (says "if/then/else/fi" but only tests "if/then/fi")

No blocking issues. Proceeding to address cosmetic nits.

[AlexandreYang]

@DataDog @codex review

[datadog-prod-us1-3]

I can only run on private repositories.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 35a9be4ec2

ℹ️ 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 — Iteration 7 (Final Verification)

Result: APPROVE (posted as COMMENT due to self-review restriction)

Summary

Reviewed the full diff (32 files changed, ~860 insertions, ~59 deletions) covering:

• Bash compatibility fixes: exit error messages, parse error exit code 2, loopctl too-many-args exits shell, break/continue non-numeric arg exit code 128
• New features: --help for grep, ls, strings builtins; empty --script "" support
• Bug fixes: printf %q length modifier conflict, pipe reader deadlock (pr.Close before wg.Wait), inline var restore via defer
• Defense-in-depth: readonly variable guards in vars.go, UnexpectedCommandError handling in expandErr
• Test coverage: 15 new/updated scenario YAML files, 4 new Go tests (readonly, expandErr, empty script, parse errors)

Findings by Severity

Severity	Count
P0 (Critical)	0
P1 (High)	0
P2 (Medium)	0
P3 (Nit)	0

All issues from iterations 1-6 have been verified as resolved. Code is clean, tests pass, gofmt is clean.

🤖 Reviewed with Claude Code

[AlexandreYang]

Iteration 7 self-review result: APPROVE

• P0: 0, P1: 0, P2: 0, P3: 0
• All previous findings confirmed resolved. PR is clean.

[AlexandreYang]

Review-Fix Loop Summary

• PR: Add improve-loop skill for systematic shell feature review #75 (Add improve-loop skill for systematic shell feature review #75)
• Iterations completed: 8
• Final status: CLEAN

Iteration log

#	Review result	Findings	Fixes applied	CI status
1	COMMENT	3 (3×P3)	2 fixed	Passing
2	COMMENT	3 (1×P1, 1×P2, 1×P3)	3 fixed	Passing
3	COMMENT	3 (2×P2, 1×P3)	3 fixed	Passing
4	COMMENT	1 (1×P1)	1 fixed + 1 CI fix (reverted to match bash 5.2)	Passing
5	COMMENT	1 (1×P1)	1 fixed (pipe reader deadlock)	Passing
6	COMMENT	2 (2×P3)	1 fixed, 1 dismissed	Passing
7	APPROVE	0	—	Passing
8	(external only)	1 (1×P3 from codex)	1 fixed	Passing

Final state

• Self-review: APPROVE
• Unresolved external comments: 0
• CI: Passing (all 9 checks)

Key fixes made during review loop

1. P1: exit with non-numeric arg now correctly exits the shell (bash compat)
2. P1: printf --help exit code corrected to 2 (bash compat)
3. P1: Pipe reader defer pr.Close() deadlock fixed — restored explicit close before wg.Wait()
4. P2: Exit error test descriptions corrected
5. P3: stderr_contains used for exit error tests, removing skip_assert_against_bash
6. P3: UnexpectedCommandError unit test added
7. P3: printf stderr assertion fixed to exact match
8. P3: Backslash path separator handling gated to Windows only (codex finding)

Remaining issues

None — PR is clean.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a2d60d4fd

ℹ️ 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]

@DataDog @codex review

[datadog-official]

I can only run on private repositories.

[AlexandreYang]

Code Review Summary — PR #75

Review result: APPROVE (posted as COMMENT since GitHub disallows self-approval)

This PR adds the improve-loop skill (a Claude Code automation tool) and a substantial batch of shell correctness fixes. After 8 prior review iterations, the code is in good shape. All tests pass.

Changes reviewed

New skill: .claude/skills/improve-loop/SKILL.md — automation tooling for systematic shell feature review. No runtime impact.

Correctness fixes (shell builtins):

• exit — bash-compatible error messages and exit behavior for non-numeric/multi args
• break/continue — bash-compatible exit codes (128 for non-numeric, shell exit for too-many-args)
• printf — removed q from length modifier list (was making %q error unreachable); --help exits 2 matching bash
• grep/ls/strings — added --help long flags
• ls — joinPath backslash handling gated to Windows via runtime.GOOS

Interpreter hardening:

• runner_exec.go — inline variable restoration via defer (panic safety)
• runner_expand.go — UnexpectedCommandError handled as fatal (defense-in-depth)
• vars.go — readonly guard added before KeepValue branch and unset path
• cmd/rshell/main.go — parse errors return exit code 2; empty scripts accepted

Test coverage:

• New Go tests for readonly enforcement, expandErr, parse error exit codes
• New scenario tests for cmd_separator, empty_script, field_splitting, line_continuation, printf %q, grep/ls/strings --help
• Exit error scenarios switched from skip_assert_against_bash to stderr_contains (better coverage)

Findings

P3 (nit):

1. printf.go doc comment (line 26) says --help exits 2, but this is consistent with the implementation. Unusual compared to other builtins in this codebase that return 0 for --help, but intentional bash-compat decision from a prior iteration.

All items flagged in prior iterations (break/continue exit code 128, printf --help exit code 2, vars.go defense-in-depth comment, pipe pr.Close() ordering, ls joinPath Windows gating) have been verified as correct and intentional.

Verdict

The code is clean, well-tested, and all bash-compatibility claims have been verified against the scenario test suite. Recommending approval.

---

Findings by severity:

• P0 (critical): 0
• P1 (major): 0
• P2 (moderate): 0
• P3 (nit): 1

[AlexandreYang]

Iteration 1 (run 2) self-review result: APPROVE

• P0: 0, P1: 0, P2: 0, P3: 1 (printf --help doc comment — already addressed, intentional)
• PR is clean. All previous findings confirmed resolved.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 849519d214

ℹ️ 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-Fix Loop Summary (Run 2)

• PR: Add improve-loop skill for systematic shell feature review #75 (Add improve-loop skill for systematic shell feature review #75)
• Iterations completed: 2
• Final status: CLEAN

Iteration log

#	Review result	Findings	Fixes applied	CI status
1	APPROVE	1 (1×P3 — already addressed)	0	Passing
2	(external only)	2 codex comments (1×P2 backslash already fixed, 1×P1 break/continue too-many-args — dismissed with bash verification)	0 (replied + resolved)	Passing

Final state

• Self-review: APPROVE
• Unresolved external comments: 0
• CI: Passing (all 9 checks)

Remaining issues

None — PR is clean.