[ENH] Use writer injection in CLI

[itaismith]

Description of changes

We currently can't test the CLI properly because there is no easy way to capture stdout in Rust. This is a prototype for a refactor for the CLI, such that each command takes in a "Writer". For standard runs by users this is just Stdout, and for testing this could be any kind of buffer, which we can use to verify that the CLI output was correct.

The change only implemented for the "run" command, as this is a prototype for using this approach.

Test plan

• Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

N/A

[github-actions]

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

• Can you think of any use case in which the code does not behave as intended? Have they been tested?
• Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
• If appropriate, are there adequate property based tests?
• If appropriate, are there adequate unit tests?
• Should any logging, debugging, tracing information be added or removed?
• Are error messages user-friendly?
• Have all documentation changes needed been made?
• Have all non-obvious changes been commented?

System Compatibility

• Are there any potential impacts on other parts of the system or backward compatibility?
• Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

• Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

[propel-code-bot]

Writer Injection Pattern for CLI Commands

This PR refactors the CLI command system to use dependency injection for writing output, making the code more testable. It introduces a CommandHandler trait with an async run method and adapts the RunCommand to use this pattern with a configurable writer. This change improves testability by allowing output to be captured during tests while maintaining the same user-facing functionality.

Key Changes:
• Introduced a CommandHandler trait with an async run method
• Refactored RunCommand to use dependency injection for output writing
• Added writer injection to enable testable command output
• Created a cli_writeln! macro for consistent error handling of writes
• Added unit test for RunCommand with output verification

Affected Areas:
• CLI command execution flow
• Command output handling
• Test infrastructure
• Run command implementation

Potential Impact:

Functionality: No change to user-facing functionality, though internal command execution flow is significantly altered

Performance: Minimal impact, possibly slight overhead from trait-based dispatch

Security: No significant security implications

Scalability: Improved code extensibility for adding new commands that follow the same pattern

Review Focus:
• Error propagation in the CLI command execution flow
• Test coverage for the new command pattern
• Consistency of writer injection approach across commands
• Security of IO operations with externally supplied writers

Testing Needed

• Verify all CLI commands work correctly with the new pattern
• Ensure error handling propagates correctly to command line exit codes
• Test CLI with actual Chroma server operations
• Validate output formatting consistency across different commands

Code Quality Assessment

rust/cli/src/commands/run.rs: Major restructuring with good separation of concerns and testability improvements

rust/cli/src/lib.rs: Added CommandHandler trait and appropriate dispatch logic

rust/cli/src/ui_utils.rs: Added cli_writeln! macro for consistent error handling

Best Practices

Testing:
• Using dependency injection to make code more testable
• Capturing output for verification in tests

Error Handling:
• Consistent error propagation using Result pattern
• Standardized error handling for IO operations

Potential Issues

• Error propagation from commands to CLI exit codes isn't fully implemented
• ChromaClient in tests connects to default port rather than test port
• Some commands are still using the old pattern which may lead to inconsistency

This summary was automatically generated by @propel-code-bot

[propel-code-bot]

[CriticalError]

The chroma_cli function currently prints errors from command execution but then proceeds to return Ok(()) and prints a final newline regardless of the outcome. This prevents the main application from knowing if an error occurred (e.g., to exit with an appropriate status code) because the error is not propagated. The function should propagate the Err variant, and the final newline should ideally be conditional or consistently placed after any primary output.

Suggested change

	if result.is_err() {
	let error_message = result.err().unwrap().to_string();
	eprintln!("{}", error_message.red());
	}
	println!();
	Ok(())
	// Assuming 'result' holds the outcome of command execution as determined
	// by the preceding logic (synchronous calls or async handler outcome).
	match result {
	Ok(_) => {
	println!(); // Final newline on success.
	Ok(())
	}
	Err(e) => {
	eprintln!("{}", e.to_string().red());
	println!(); // Final newline after error message for console readability.
	Err(e) // Propagate the error.
	}
	}
	} // This closes the function body, ensure it replaces from the original `if result.is_err()...` to `Ok(())`

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

[propel-code-bot]

[BestPractice]

The main function should handle the Result returned by chroma_cli to ensure the program exits with a non-zero status code if an error occurs. Currently, let _ = chroma_cli(args); discards this result. Assuming chroma_cli is modified to correctly propagate errors, main should act on them.

Suggested change

	let args: Vec<String> = env::args().collect();
	chroma_cli(args)
	let _ = chroma_cli(args);
	}
	fn main() {
	let args: Vec<String> = env::args().collect();
	if let Err(_e) = chroma_cli::chroma_cli(args) {
	// Assuming chroma_cli now prints its own errors before returning Err.
	// Otherwise, you might want to print the error here: eprintln!("Error: {}", _e);
	std::process::exit(1);
	}
	}

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

[propel-code-bot]

[CriticalError]

The test test_run starts a server on a dynamic port (e.g., 8001 defined by the port variable) but then creates a ChromaClient using ChromaClient::local_default(). This default client likely connects to a standard port (e.g., 8000) and not the port the test server is running on. The client should be initialized with the URL of the test server.

Suggested change

	let chroma_client = ChromaClient::local_default();
	let url = format!("http://localhost:{}", port);
	let chroma_client = ChromaClient::new(url);

⚡ Committable suggestion

Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.