Add pwd, clear, and help builtins to /bin/sh#907
Merged
Conversation
Implements the three new shell builtins requested in #901: - pwd: prints $PWD, falls back to std::env::current_dir() - clear: writes ANSI \x1b[2J\x1b[H to clear screen - help: lists all builtins with descriptions; help <name> shows detail Includes a static BUILTIN_HELP table with short and long descriptions for every builtin, and unit tests for all three new commands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds three shell builtins—pwd, clear, and help—by extending builtin recognition and dispatch in ChangesShell Builtin Additions
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
base/sh/src/builtins.rs (1)
2011-2023: ⚡ Quick winConsider removing duplicated builtin-name lists in tests.
The hardcoded
knownarray can drift from dispatch/help tables. Prefer deriving test inputs from a single source used by builtin metadata to keep coverage future-proof.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/sh/src/builtins.rs` around lines 2011 - 2023, The test help_each_builtin_has_entry duplicates the builtin name list; instead of the hardcoded known array, iterate the canonical builtin registry used by is_builtin (the dispatch/metadata table that defines available builtins) and call builtin_help(&args(&[name])) for each entry; update the test to pull names from that single source of truth (the same dispatch/metadata used by is_builtin) so the test always reflects the runtime builtin set while keeping help_each_builtin_has_entry, is_builtin, builtin_help and args as the used symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/sh/src/builtins.rs`:
- Around line 1941-1955: The tests pwd_prints_env_pwd and
pwd_fallback_when_unset only assert exit status; update them to also capture and
assert the command output: use the same test harness that runs builtin_pwd (via
builtin_pwd, env(), args()) but capture stdout/stderr and assert that
pwd_prints_env_pwd yields the exact PWD string ("/home/user") and that
pwd_fallback_when_unset yields the current directory string
(std::env::current_dir() formatted as string). Apply the same pattern to the
other failing tests referenced (the clear/escape-sequence and help/listing tests
at the other locations) by asserting their stdout contains the expected escape
sequence or help text content/format rather than only checking status. Ensure
you reference and use builtin_pwd, env, args and the help/clear builtin test
functions when adding the output assertions.
- Around line 1118-1121: The builtin_clear function writes ANSI sequences but
doesn't flush stdout; modify builtin_clear (fn builtin_clear) to call
std::io::stdout().flush() after print! to ensure the clear is sent
immediately—import std::io::Write if needed and handle the Result from flush()
(e.g., ignore error or map it to the return code) so the function still returns
an i32.
- Around line 1096-1109: The builtin_pwd function currently ignores any
operands; change it to reject unexpected args by using the passed-in args slice
(remove the leading underscore from _args or otherwise reference it) and if
args.len() > 0 print an error to stderr (consistent with existing style, e.g.
eprintln!("sh: pwd: too many operands" or "sh: pwd: too many arguments")) and
return a non-zero exit code (1) before proceeding to read PWD or current_dir;
keep the existing PWD/current_dir logic unchanged and only run it when no
operands are supplied.
---
Nitpick comments:
In `@base/sh/src/builtins.rs`:
- Around line 2011-2023: The test help_each_builtin_has_entry duplicates the
builtin name list; instead of the hardcoded known array, iterate the canonical
builtin registry used by is_builtin (the dispatch/metadata table that defines
available builtins) and call builtin_help(&args(&[name])) for each entry; update
the test to pull names from that single source of truth (the same
dispatch/metadata used by is_builtin) so the test always reflects the runtime
builtin set while keeping help_each_builtin_has_entry, is_builtin, builtin_help
and args as the used symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff70d307-da38-4a7c-9ac8-c272eef8760f
📒 Files selected for processing (1)
base/sh/src/builtins.rs
Comment on lines
+1941
to
+1955
| #[test] | ||
| fn pwd_prints_env_pwd() { | ||
| let mut e = env(); | ||
| e.set("PWD", "/home/user", None); | ||
| let status = builtin_pwd(&args(&[]), &mut e); | ||
| assert_eq!(status, 0); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pwd_fallback_when_unset() { | ||
| let mut e = env(); | ||
| // PWD is not set — should fall back to std::env::current_dir() | ||
| let status = builtin_pwd(&args(&[]), &mut e); | ||
| assert_eq!(status, 0); | ||
| } |
There was a problem hiding this comment.
New builtin tests miss output-contract assertions.
These tests validate status codes but not the required observable output (pwd text, clear escape sequence, help listing format/content). That leaves regressions undetected in the core behavior promised by this PR.
Also applies to: 1973-1976, 1993-1996
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@base/sh/src/builtins.rs` around lines 1941 - 1955, The tests
pwd_prints_env_pwd and pwd_fallback_when_unset only assert exit status; update
them to also capture and assert the command output: use the same test harness
that runs builtin_pwd (via builtin_pwd, env(), args()) but capture stdout/stderr
and assert that pwd_prints_env_pwd yields the exact PWD string ("/home/user")
and that pwd_fallback_when_unset yields the current directory string
(std::env::current_dir() formatted as string). Apply the same pattern to the
other failing tests referenced (the clear/escape-sequence and help/listing tests
at the other locations) by asserting their stdout contains the expected escape
sequence or help text content/format rather than only checking status. Ensure
you reference and use builtin_pwd, env, args and the help/clear builtin test
functions when adding the output assertions.
Owner
Author
There was a problem hiding this comment.
Deferred to future work -- the existing test patterns in this file (echo_basic, echo_no_args, etc.) also only assert status codes, not output content. Adding stdout capture infrastructure is out of scope for this PR.
There was a problem hiding this comment.
@dburkart, understood — that's a fair call. The existing echo_basic and similar tests confirm the same status-only pattern, so keeping the new builtin tests consistent makes sense. Happy to revisit output-capture assertions in a follow-up once the infrastructure is in place.
✏️ Learnings added
Learnt from: dburkart
Repo: dburkart/vibix PR: 907
File: base/sh/src/builtins.rs:1950-1964
Timestamp: 2026-05-06T05:16:34.614Z
Learning: In `base/sh/src/builtins.rs`, the test module does not yet have stdout/stderr capture infrastructure. All existing builtin tests (echo_basic, echo_no_args, pwd_prints_env_pwd, clear_returns_zero, help_no_args_lists_all, etc.) only assert exit status codes, not output content. Adding output-content assertions is deferred to future work.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- pwd now rejects unexpected operands with exit status 2 - clear now flushes stdout after writing escape sequences - Added test for pwd extra-args rejection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
base/sh/src/builtins.rs (1)
1950-1964:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTests still miss output-contract assertions for new builtins.
At Line 1954, Line 1991, and Line 2011 (and related tests), assertions only check exit status. This leaves regressions in visible behavior (
pwdtext,clearescape sequence,helpcontent/format) undetected, even though those behaviors are core to this PR objective.Based on learnings: tests currently only assert exit status codes; add stdout/stderr capture and assert actual output content for
pwd,clear, andhelp.Also applies to: 1990-1993, 2010-2025
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/sh/src/builtins.rs` around lines 1950 - 1964, The tests for pwd (pwd_prints_env_pwd, pwd_fallback_when_unset) and the related clear/help tests only assert exit status and must also capture and assert stdout/stderr content to prevent behavioral regressions; update these tests to run builtin_pwd / builtin_clear / builtin_help with the existing test helpers (e.g., env(), args()) while capturing output (use the project’s test capture helper or wrap the call to builtin_* and collect printed stdout/stderr), then assert the expected text: for pwd tests assert the printed path equals the PWD env value or the current_dir fallback, for clear assert the expected escape sequence is written to stdout, and for help assert the help string contains the expected sections/commands. Locate and modify the test functions named pwd_prints_env_pwd, pwd_fallback_when_unset and the tests around lines 1990–1993 and 2010–2025 to add these stdout/stderr assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@base/sh/src/builtins.rs`:
- Around line 1950-1964: The tests for pwd (pwd_prints_env_pwd,
pwd_fallback_when_unset) and the related clear/help tests only assert exit
status and must also capture and assert stdout/stderr content to prevent
behavioral regressions; update these tests to run builtin_pwd / builtin_clear /
builtin_help with the existing test helpers (e.g., env(), args()) while
capturing output (use the project’s test capture helper or wrap the call to
builtin_* and collect printed stdout/stderr), then assert the expected text: for
pwd tests assert the printed path equals the PWD env value or the current_dir
fallback, for clear assert the expected escape sequence is written to stdout,
and for help assert the help string contains the expected sections/commands.
Locate and modify the test functions named pwd_prints_env_pwd,
pwd_fallback_when_unset and the tests around lines 1990–1993 and 2010–2025 to
add these stdout/stderr assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ee66a39-d200-48bc-a81b-2060f932c9e9
📒 Files selected for processing (1)
base/sh/src/builtins.rs
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.
Closes #901
Summary
pwd,clear, andhelppwdprints$PWDfrom the environment, falling back tostd::env::current_dir()if unsetclearwrites ANSI escape sequences (\x1b[2J\x1b[H) to clear the screen and home the cursorhelpwith no args lists all builtins with one-line descriptions;help <name>shows detailed usageBUILTIN_HELPtable covering all 17 builtins with short and long descriptionsis_builtincoverageTest plan
cd base/sh && cargo test— all 412 tests pass (was 396 before, +16 new)cargo fmt --all -- --check— clean