Testing

[cdprice02]

This pull request introduces significant improvements to the codebase, focusing on enhanced testing, CI/CD integration, and code quality. The main changes include adding comprehensive unit tests to core modules, setting up a multi-platform CI workflow, improving documentation, and making minor API and formatting adjustments for clarity and maintainability.

Testing and Quality Improvements:

• Added extensive unit tests for key modules, including arg, command/builtin, command/executable, env, and error, ensuring better coverage and reliability. [1] [2] [3] [4] [5]
• Implemented a custom PartialEq for ShellError to facilitate error comparison in tests.
• Added tempfile as a dev-dependency to support isolated testing.

Continuous Integration and Coverage:

• Introduced a GitHub Actions workflow (ci.yml) for linting, building, testing, and code coverage on Linux, Windows, and macOS.
• Added build, test, lint, and coverage badges to README.md for visibility of CI status.

Documentation and Guidance:

• Added CLAUDE.md to provide architecture, testing strategy, and conventions for contributors and code assistants.
• Improved internal documentation and comments for clarity, including doc comments for public types like Arg and Command. [1] [2]
• Updated project references and fixed relative links in documentation.

API and Codebase Adjustments:

• Refactored the executor to use the ShellIo trait for I/O abstraction, improving testability and modularity. [1] [2]
• Made environment functions in env.rs public for broader access.
• Minor error message rewording for consistency and clarity in ShellError.

Project Structure and Conventions:

• Clarified file-based module organization and architectural flow in documentation. [1] [2]

These changes collectively enhance code reliability, maintainability, and developer experience, while laying a strong foundation for future development.

---

Testing and Quality:

• Added comprehensive unit tests for Arg, BuiltInName, BuiltInCommand, ExecutableCommand, env functions, and ShellError, including equality and display checks. [1] [2] [3] [4] [5]
• Implemented PartialEq for ShellError to support robust test assertions.
• Added tempfile as a dev-dependency for isolated integration tests.

CI/CD and Coverage:

• Introduced a cross-platform GitHub Actions workflow for linting, building, testing, and code coverage, including artifact uploads.
• Added CI and coverage badges to README.md.

Documentation and Guidance:

• Added CLAUDE.md for contributor and AI assistant guidance, including architecture and testing strategy.
• Improved doc comments and fixed documentation links. [1] [2] [3]

API and Codebase Improvements:

• Refactored executor to use the ShellIo trait for output, improving testability. [1] [2]
• Made environment helper functions public.
• Standardized error messages in ShellError.

Project Structure and Conventions:

• Clarified file-based module structure and architectural flow in documentation. [1] [2]

[Copilot]

Pull request overview

This PR focuses on adding test coverage and CI automation while refactoring the shell’s I/O abstraction to make the core execution path more testable.

Changes:

• Added unit + integration tests across core modules and REPL behavior, plus a test harness using MockIo.
• Refactored execution to use a ShellIo trait (src/io.rs) and updated Shell/executor to write via trait-provided writers.
• Introduced a multi-platform GitHub Actions workflow with clippy and coverage, and added badges + contributor guidance docs.

Reviewed changes

Copilot reviewed 24 out of 26 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
tests/repl.rs	Adds REPL/prompt + error recovery integration tests via harness
tests/harness.rs	New integration harness running Shell with MockIo and optional HOME isolation
tests/executable.rs	Adds executable/command-not-found integration tests
tests/env_handling.rs	Adds basic env/path-related integration tests
tests/builtin.rs	Adds integration tests for builtins and command sequencing
src/shell.rs	Switches prompt + error output to writer-based ShellIo and adds unit tests
src/parser.rs	Makes parse_command/parse_arg public and adds unit tests
src/lib.rs	Exposes io module publicly and tweaks doc examples
src/io_internal.rs	Removes old internal I/O abstraction
src/io.rs	Adds new ShellIo trait + StandardIo/MockIo implementations and tests
src/fs.rs	Adds unit tests around path resolution/canonicalization
src/exit.rs	Adds basic unit tests for ExitCode
src/executor.rs	Refactors executor to use ShellIo and adds unit tests
src/error.rs	Tweaks error messages, adds PartialEq, and adds unit tests
src/env.rs	Makes env helpers public and adds unit tests
src/command/executable.rs	Adds unit tests for ExecutableCommand
src/command/builtin.rs	Adds unit tests for builtin parsing/display
src/command.rs	Removes helper constructors and adds doc comment
src/arg.rs	Adds From<&str> and unit tests
README.md	Adds CI/coverage/lint badges
Cargo.toml	Adds tempfile dev-dependency
Cargo.lock	Locks new dependency graph
CLAUDE.md	Adds contributor/assistant guidance and testing strategy notes
.gitignore	Adds tmp/ ignore
.github/workflows/ci.yml	Adds multi-platform build/test, clippy, and coverage jobs
.github/copilot-instructions.md	Fixes README link and formatting

Comments suppressed due to low confidence (1)

src/shell.rs:56

• When read_line returns 0 (EOF), this loop continues, which will repeatedly print prompts and spin forever. Typically EOF should terminate the REPL (break/return). Also, after writing the prompt you likely need to flush out_writer() so it appears immediately in interactive use.

    pub fn run(&mut self) -> anyhow::Result<()> {
        loop {
            self.io
                .borrow_mut()
                .out_writer()
                .write_all(Shell::prefix().as_bytes())?;

            let mut buffer = Vec::<u8>::new();
            let bytes = self.io.borrow_mut().read_line(&mut buffer)?;
            if bytes == 0 {
                continue;

[Copilot]

Pull request overview

Copilot reviewed 24 out of 26 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

src/shell.rs:57

• When read_line returns 0 bytes (EOF), the loop continues, which will repeatedly print prompts and spin forever on EOF. The usual REPL behavior is to exit on EOF; consider breaking the loop (or returning Ok(())) when bytes == 0.

            let mut buffer = Vec::<u8>::new();
            let bytes = self.io.borrow_mut().read_line(&mut buffer)?;
            if bytes == 0 {
                continue;
            }

[Copilot]

This test claims to verify an external/system command, but echo is a ferrish builtin (BuiltInName::Echo). As written, it doesn’t exercise executable detection. Consider switching to a command that is not implemented as a builtin on each platform (or renaming the test/comment to reflect that it’s testing the builtin path).

Suggested change

	let result = ShellTest::new()
	.with_isolated_home()
	.script("echo executable_test")
	.run();
	// System echo should work (running as external command)
	#[cfg(unix)]
	let script = "sh -c 'echo executable_test'";
	#[cfg(windows)]
	let script = "cmd /C echo executable_test";
	let result = ShellTest::new()
	.with_isolated_home()
	.script(script)
	.run();
	// External shell (sh/cmd) should be detected and run, producing the expected output

[Copilot]

test_home_isolation relies on heuristics like "/tmp", "ferrish", and output length, which are platform- and environment-dependent and can be flaky (especially on Windows/macOS CI). Since the harness sets CWD to the isolated temp home, consider asserting that pwd parses as a valid directory path and equals the isolated home path (e.g., by having the harness return the temp dir path for comparison).

[Copilot]

test_canonicalize_path_absolute uses PathBuf::from("/"), which is Unix-specific and may not represent a valid absolute path on Windows (and can cause cross-platform CI failures). Consider deriving an existing absolute path in a platform-agnostic way (e.g., using env::current_dir() or its root ancestor), or gating the "/" assertion with #[cfg(unix)].

Suggested change

	let path = PathBuf::from("/");
	let path = env::current_dir().unwrap();

[Copilot]

The REPL prompt is written to out_writer() but never flushed. In interactive use, the prompt may not appear until after the next write. Consider flushing stdout after writing the prefix (or reintroducing a ShellIo::flush API and using it here).

[Copilot]

execute_executable takes io: &mut impl ShellIo but does not use it and currently inherits the parent process stdio. This makes executable commands untestable with MockIo and inconsistent with builtins (which write via ShellIo). Consider piping the child stdout/stderr and forwarding them into io.out_writer() / io.err_writer() (or otherwise integrating executables with the ShellIo abstraction).

[Copilot]

The assertion here is too weak: if both stdout and stderr are empty, it still passes because !error.contains("not found") is true for an empty string. That can mask failures where the command didn’t execute or output wasn’t captured. Consider asserting on a positive signal of success (e.g., non-empty output, or expected path-like output) and failing when both outputs are empty.

Suggested change

	!output.is_empty() || !error.contains("not found"),
	(!output.is_empty() || !error.is_empty())
	&& !output.contains("not found")
	&& !error.contains("not found"),

[Copilot]

This integration test hard-codes type sh, which is not reliably present on Windows (and may be absent on some minimal macOS/Linux environments). Since CI runs the test suite on all three OSes, this is likely to be flaky/fail. Consider making the script OS-specific (e.g., sh on unix, cmd/where on Windows) or selecting an executable that’s guaranteed in your CI images.

[Copilot]

ShellIo::read_line currently reads into a String via BufRead::read_line, which requires UTF-8 and will error on non‑UTF8 input. Since the shell parser operates on raw bytes, this is a behavioral regression vs reading bytes directly. Consider implementing the default read_line using BufRead::read_until(b'\n', buffer) (or an equivalent byte-based approach) to accept arbitrary bytes and avoid the extra allocation/copy.

Suggested change

	let mut str = String::new();
	self.reader().read_line(&mut str).inspect(|_| {
	buffer.extend_from_slice(str.as_bytes());
	})
	self.reader().read_until(b'\n', buffer)