docs(skills): rework /improve-test-coverage and apply it to the test suite#236
Merged
Conversation
…e to improve-test-coverage
Tracking file for /improve-test-coverage. This file is committed during the run for resumability and removed in the finalization commit. Co-Authored-By: Claude Opus 4.6 <[email protected]>
uname requires a fake /proc filesystem, which the scenario test framework cannot configure. Comprehensive Go-level tests already cover the command (28 tests, including all flag combinations, help, error paths, custom proc path, partial proc tree, whitespace handling, pipe integration, and cancellation). Update COVERAGE_PROGRESS.md. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
| Candidate gap | Reason |
|---|---|
Default uname (no flags) → kernel name |
Scenario framework cannot mock /proc/sys/kernel/; bash on Docker reads real kernel info, our shell errors on non-Linux hosts → bash divergence is fundamental, not testable |
Individual flags -s, -n, -r, -v, -m, -a |
Same proc-mock limitation |
uname -h short help variant |
Cosmetic variant of --help already covered |
Coverage before/after
- Before: 3 scenario tests
- After: 3 scenario tests (+0 new)
- Evaluated: 7; added: 0; skipped: 7
Cleanup
- Duplicate tests removed: 0
skip_assert_against_bashflags removed: 0 (existing two flags are correct: help format intentionally differs from GNU; pflag error format intentionally differs)- Unnecessary Windows-specific assertions removed: 0
Findings
- The scenario test framework lacks a
proc_pathconfiguration option. Adding one would unlock real scenario coverage foruname,ip,ss,ps, andfind -mount. This is out of scope for the coverage run but worth a follow-up issue.
🤖 Generated with Claude Code
Add 6 scenario tests covering rshell-specific sandbox behaviour: - blocked_with_help_hint: help in allowlist activates "Run 'help' to see allowed commands." hint - blocked_in_pipeline: blocked stage in pipeline yields exit 127 - blocked_in_subshell: subshell continues after a blocked command - blocked_in_command_substitution: $() with a blocked command substitutes empty and parent continues - allowed_in_pipeline: positive case for pipelines (both stages allowed) - special_builtin_blocked: POSIX special builtin `:` is gated like any other command Add missing "# skip:" comments on 6 existing tests that use skip_assert_against_bash to document why bash divergence is intentional. Update COVERAGE_PROGRESS.md. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
| File | Priority | Why a regression here would matter |
|---|---|---|
blocked_with_help_hint.yaml |
P2 | The "Run 'help' to see allowed commands." hint is part of the documented UX; regressing it confuses operators when commands are blocked |
blocked_in_pipeline.yaml |
P2 | Pipelines are extremely common; sandbox enforcement must apply across pipeline stages |
blocked_in_subshell.yaml |
P2 | ( ... ) is a common scoping pattern; sandbox must enforce inside subshells without halting the parent |
blocked_in_command_substitution.yaml |
P2 | $(...) is the canonical capture-output pattern; substituting empty + continuing is load-bearing |
allowed_in_pipeline.yaml |
P2 | Positive case — confirms multi-allow-list pipelines actually work |
special_builtin_blocked.yaml |
P2 | Documents that POSIX special builtins (:, true, etc.) are gated, not silently allowed |
Candidates skipped
| Candidate gap | Reason |
|---|---|
true, false, exit blocked variants |
Covered by special_builtin_blocked.yaml (: is the canonical special builtin) |
| Multi-colon namespace rejection | API-level validation, not script-level — covered by Go tests in interp/allowed_commands_test.go |
if blocked-cmd; then ... exit-code semantics |
Behavioural overlap with blocked_in_pipeline and blocked_in_subshell; would not catch a regression that those don't catch |
Coverage before/after
- Before: 6 scenario tests
- After: 12 scenario tests (+6 new)
- Evaluated: 9; added: 6; skipped: 3
Cleanup
- Duplicate tests removed: 0
skip_assert_against_bashflags removed: 0 (all 12 tests intentionally skip bash assertion —allowed_commandsis rshell-specific)- Unnecessary Windows-specific assertions removed: 0
- Missing skip-comments added: 6 (existing tests had
skip_assert_against_bash: truewithout an explanatory comment)
Findings
- No shell bugs discovered.
- All
skip_assert_against_bash: trueinstances are intentional and now documented with# skip: ...comments per project convention.
🤖 Generated with Claude Code
…n scenarios can't reach the gap
Tracking file for /improve-test-coverage. This file is committed during the run for resumability and removed in the finalization commit. Co-Authored-By: Claude Opus 4.6 <[email protected]>
69 break-related scenarios under for_clause/while_clause/until_clause already cover nested loops, exit status, error cases, conditionals, pipelines, subshells, logical operators, and help. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
62 continue-related scenarios under for_clause/while_clause/until_clause already cover nested loops, exit status, error cases, conditionals, logical operators, and help. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
8 scenarios cover all syntactic forms of readonly (rejected at parse validation) and 3 Go tests cover the runtime API used by host code to mark variables readonly. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
Add 4 P2 scenarios filling gaps from yash POSIX grouping tests: - multiline subshell (newlines instead of semicolons) - subshell as pipe destination consuming stdin - triple-nested variable isolation (each scope independent) - input redirection on subshell construct Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
| File | Layer | Priority | Why a regression here would matter |
|---|---|---|---|
tests/scenarios/shell/subshell/multiline.yaml |
scenario | P2 | Multi-line (...) is the canonical POSIX form; broken parsing of newlines inside subshells would silently break long, readable script blocks |
tests/scenarios/shell/subshell/as_pipe_target.yaml |
scenario | P2 | Common pattern: piping into a grouped command. A regression would silently drop stdin into the subshell's first command |
tests/scenarios/shell/subshell/triple_nested_var_isolation.yaml |
scenario | P2 | Existing test only verifies one level of isolation; triple nesting catches scope-leak bugs that don't surface at one level |
tests/scenarios/shell/subshell/input_redirect.yaml |
scenario | P2 | Verifies the subshell construct accepts a file redirection — distinct from the redirection feature itself; a parser regression around (...)<file would slip past redirection-only tests |
Candidates skipped
| Candidate gap | Reason |
|---|---|
Output redirect on subshell (...) > file |
Sandbox blocks > redirection — exercising the same parser path as input-redirect would be redundant |
Subshell ending with semicolon (echo foo;) |
Cosmetic POSIX variant; same parser path as multiple_commands |
Subshell with cd doesn't change parent cwd |
cd is not implemented as a builtin |
Subshell with async & |
Background execution not supported |
Chained subshells with && |
Already covered in brace_group/nested_subshell_blocked.yaml |
Coverage before/after
- Scenario tests: 8 → 12 (+4 new)
- Unit tests: N/A (no Go unit tests directly for subshell construct)
- Evaluated: 9; added: 4; skipped: 5
Cleanup
- Duplicate tests removed: 0
- Low-value tests removed: 0
skip_assert_against_bashflags removed: 0- Unnecessary Windows-specific assertions removed: 0
Findings
None. Behavior matches bash for all tested cases.
🤖 Generated with Claude Code
Add 2 P2 scenarios for Windows-saved scripts: - CRLF-only empty script - CRLF-style comment-only script Catches regressions where CRLF parsing breaks scripts copied from Windows editors. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
| File | Layer | Priority | Why a regression here would matter |
|---|---|---|---|
tests/scenarios/shell/empty_script/crlf_only.yaml |
scenario | P2 | Windows-saved scripts use \r\n; a CRLF-only script must exit cleanly. A regression would silently fail every Windows-edited shell script |
tests/scenarios/shell/empty_script/crlf_comments.yaml |
scenario | P2 | Combines CRLF with comment parsing — common real-world case for Windows-saved scripts |
Candidates skipped
| Candidate gap | Reason |
|---|---|
| BOM-only script | bash itself rejects BOM as unknown command (verified locally); behavior is consistent and not actually "empty" semantically |
| CR-only line endings | Old Mac line endings; vanishingly rare in modern usage |
| Trailing comment without newline | Cosmetic variant; same parser path as comment_only |
Coverage before/after
- Scenario tests: 9 → 11 (+2 new)
- Evaluated: 5; added: 2; skipped: 3
Cleanup
- Duplicate tests removed: 0
- Low-value tests removed: 0
skip_assert_against_bashflags removed: 0- Unnecessary Windows-specific assertions removed: 0
Findings
None. Both new tests pass against our shell.
🤖 Generated with Claude Code
10 scenarios cover every syntactic form of case (simple, default, character class, glob, multiple patterns, paren, wildcard, single-line, variable subject, empty body). All produce identical parse rejection. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
Replace syntax_error_kills_shell.yaml (misnamed duplicate of valid_after_error) with a real coverage gap: command-not-found inside command substitution emits an error to stderr but the outer command runs with empty substitution. Note: scenarios cannot test true parse-time syntax errors because the test framework parses the script before running it; that's a Go-test gap if needed. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
| File | Layer | Priority | Why a regression here would matter |
|---|---|---|---|
tests/scenarios/shell/errors/cmd_subst_unknown_cmd.yaml |
scenario | P2 | Verifies command substitution captures the unknown-command failure inline (empty substitution) without aborting the outer command — common script pattern, regression would silently produce malformed output |
Candidates skipped
| Candidate gap | Reason |
|---|---|
| Real syntax error aborts script | The scenario test framework parses scripts via syntax.NewParser().Parse(...) before running them, so parse errors fail-test-setup rather than producing observable stderr/exit_code. Would require a Go unit test or end-to-end binary test |
| Unclosed quote/brace | Same framework limitation as above |
Cleanup
- Duplicate tests removed: 1 (
syntax_error_kills_shell.yaml— misnamed file whose content is identical in semantics tovalid_after_error.yaml; both test "valid commands run after an unknown command") - Low-value tests removed: 0
skip_assert_against_bashflags removed: 0- Unnecessary Windows-specific assertions removed: 0
Coverage before/after
- Scenario tests: 10 → 10 (+1 new, -1 duplicate)
Findings
- Framework limitation: scenario test framework parses scripts before invoking the runner. This means syntax-error behavior (parse-time abort, exit code 2) is untestable via scenarios. If high-value, it should be a Go unit test against
interp.Runor an end-to-end test against thershellbinary.
🤖 Generated with Claude Code
10 scenarios cover every syntactic form of function declaration (name(), function keyword, multiline, nested, redefine, for body, if body, portable name, etc.). All produce identical parse rejection. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
12 scenarios cover runtime allowlist behavior (allow-all, blocked contexts: pipeline/subshell/command-substitution, help-hint, special builtin gating). 8 Go tests in interp/allowed_commands_test.go cover CLI validation (namespace, whitespace, duplicates, etc.). Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Coverage Improvement Summary —
|
13 scenarios cover blank lines, comments (block + inline), whitespace handling, tab separators, mixed tabs/spaces, long lines, multi-command sequences, leading whitespace, trailing-newline absence. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans 7 categories: basic short-circuit, chained &&, chained ||, mixed && / ||, with grouping (subshell/brace), with negation, exit status propagation, with cmd-subst, with pipelines, with redirs. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: -n max-args, -I replace-string, -0 null-separated, -d delimiter, -r no-run-if-empty, -t verbose, -p prompt, -L lines, -s size, -E end-of-input, exec-only-allowed-builtins (sandbox), with pipes, with cmd-subst, multi-arg, single-arg. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: -b bytes, -c chars, -f fields, -d delimiter, --complement, --output-delimiter, range forms (1, 1-3, 3-, -3), -s suppress-no-delim, -n no-split-multi-byte, multi-file, stdin, errors. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: -E/-F/-G regex modes, -i case-insensitive, -v invert, -c count, -l files-with-matches, -L files-without, -n line-num, -H/-h with-filename, -r recursive, --include/--exclude, -A/-B/-C context, -e multiple-patterns, -f patterns-from-file, -w word, -x line, -q quiet, multi-file, stdin, ReDoS protection (RE2 engine). No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
AlexandreYang
changed the title
Coverage spans: -n numeric, -r reverse, -u unique, -k key, -t separator, -f case-fold, -b ignore-blanks, -h human-numeric, -V version, -M month, -R random, -g general-numeric, multiple keys, key ranges, multi-file, stable -s, -c check, errors. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: -l long, -a all (incl. dotfiles), -A almost-all, -d directory, -r reverse, -t time-sort, -S size-sort, -1 one-per-line, -R recursive, -F classify, -h human-readable, -i inode, --color, with multiple paths, with symlinks, with errors, with special-char names. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The echo builtin is intentionally simple: all args are emitted as literals separated by single spaces, with a trailing newline. -n / -e / -E are NOT supported — they are emitted as literal text. Coverage spans: no args, single arg, multiple args, args with spaces, escaped chars (literal), $var expansion in args, glob expansion in args, -n / -e / -E literal-emission, special chars, very long output. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: -n N lines (default 10), -c N bytes, -n -N (all but last N), -c -N (all but last N), -q quiet, -v verbose, -n +N (skip offsets), multi-file with header, stdin/pipe, single line, no-trailing- newline, large file, suffix multipliers (k/M/G/b), errors. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: -n N (default 10), -c N bytes, -n +N (start at line N), -c +N (start at byte N), -q quiet, -v verbose, -f follow rejection (sandbox forbids), multi-file with header, stdin/pipe, suffix multipliers, single line, no-trailing-newline, large file, errors. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: explicit word list, $@ / $* iteration, glob expansion, cmd-subst expansion, empty list (no iters), single iter, multi-iter, nested for, in pipeline, in subshell, with break/continue at all levels, exit status, in if/then, with redirs, var-scope post-loop. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: %s/%d/%i/%u/%o/%x/%X/%e/%E/%f/%g/%G/%c/%b/%q, %% literal, width and precision (incl. .* / *N), padding (-/0/space/+), left-align, multi-arg cycling, escape sequences (\n/\t/\\/\xNN), \c truncation, integer overflow, octal/hex literal args, errors. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: s/// command (with all flags g/p/i/N), address forms (N, /regex/, N,M, /a/,/b/, $, addr!), commands d/p/n/N/y/=, character classes, anchors, backrefs, escape sequences, multi-script -e, multi- file, stdin, separators (s|||), errors. No -i in-place (sandbox forbids writes). No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: $VAR / ${VAR} basic, special parameters
($? / $# / $@ / $* / $0 / $$ / $!), defaults (${V:-x},
${V:=x}, ${V:?msg}, ${V:+x}), unset vs empty distinction, blocked
forms (${V//pat/repl}, ${V%pat}, ${V#pat}, indirect ${!V},
arithmetic $((expr)) — sandbox-forbidden), nested expansion, in
quoted contexts, special chars in names, status_nested_for_break.
No high-value gaps.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Coverage spans: -name / -iname / -path / -ipath patterns, -type (f/d/l/p/s/c/b), -mtime / -atime / -ctime / -newer, -size, -empty, -readable / -writable / -executable, -depth / -maxdepth / -mindepth, -prune, -print / -print0, expressions (and / -o / !), parens for grouping, -exec rejection (sandbox), errors, special-char names, deeply-nested trees. No high-value gaps. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…ytes The crlf_only scenario tests that our shell treats a CRLF-only script (Windows-saved empty file) as empty. Bash 5 (Debian bookworm) treats the stray \r bytes as command names and emits 3 error messages. This is intentional divergence — our shell tolerates Windows line endings; bash does not. This was a pre-existing CI failure on the branch (added in 82d40a8) that surfaced once /improve-test-coverage's Phase B finished without modifying any scenarios. Fixed during Phase C Step 12 (/fix-ci-tests). Co-Authored-By: Claude Opus 4.6 <[email protected]>
Member
Author
Final Coverage Improvement ReportThis run of This is the expected steady-state outcome for this branch: the prior CI status
Findings
Tracker
Summary
🤖 Generated with Claude Code |
Final coverage report has been posted as a PR comment. The tracker file is no longer needed on the branch. Co-Authored-By: Claude Opus 4.6 <[email protected]>
AlexandreYang
marked this pull request as ready for review
May 7, 2026 12:34
AlexandreYang
requested review from
astuyve,
julesmcrt,
matt-dz,
thieman and
val06
as code owners
May 7, 2026 12:34
julesmcrt
approved these changes
May 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two strands of work on this branch:
/improve-test-coverageskill rework (.claude/skills/improve-test-coverage/SKILL.md)TaskCreate.COVERAGE_PROGRESS.mdtracker file at the repo root. The tracker is initialized in Phase A, updated per-target in Phase B, and removed in Phase C so it never lands in the merged change set.skip_assert_against_bashaudit reworked to diff our shell against bash side-by-side, mirroring the scenario YAML's sandbox config on the CLI (--allow-all-commands/--allowed-commands/--allowed-paths) so the diff reflects engine behaviour rather than CLI defaults. Real shell bugs surfaced by the diff feed into a per-target Findings section.stdout_windows/stderr_windowsfields whose values are identical to the non-Windows fallback./fix-ci-testsbefore posting the final report, so the PR is left green rather than relying solely on local Docker bash-comparison runs.Test-suite coverage pass driven by the reworked skill
cat,head,tail,grep,sed,printf,find,xargs,cut,tr,sort,uniq,wc,test,read,ls,du,pwd,ps,ss,ip,ping,uname,echo,help,strings,exit,true,false) and every shell feature directory undertests/scenarios/shell/(control flow, expansion, quoting, redirections, sandbox surface, etc.).test: improve coverage for allowed_redirects,test: improve coverage for errors,test: improve coverage for empty_script,test: improve coverage for subshell,test: improve coverage for allowed_commands.pipe,blocked_commands,exit,brace_group,comments,true,false.tests/scenarios/(excluding the skill file).Test plan
RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBashpasses for the new and modified scenariosmake fmtclean