chore(bin): Refactor CLI

[refcell]

Description

Refactors the CLI for the rollup binary.

[refcell]

📚 $\text{Stack Overview}$

Pulls submitted in this stack:

• chore(bin): Refactor CLI #2762 👈
• main

This comment was automatically generated by st.

[claude]

Claude finished @refcell's task —— View job

---

PR Review: CLI Refactoring

I've completed my review of this CLI refactoring PR. Here's my comprehensive feedback:

Todo List:

• Analyze the overall refactoring scope and structure
• Review new CLI implementation in bin/rollup/
• Examine the utilities/cli crate refactoring
• Check dependency changes and build configuration
• Assess code quality and Rust best practices
• Look for potential bugs or issues
• Evaluate performance and security considerations
• Check test coverage implications
• Provide comprehensive feedback

Review Feedback

✅ Positive Aspects

1. Clean Code Organization

• Excellent modularization of CLI flags in crates/utilities/cli/src/flags/ - separating globals.rs, log.rs, metrics.rs, and overrides.rs makes the codebase much more maintainable
• Well-structured separation of concerns between the rollup binary and shared CLI utilities
• Good use of Rust's module system and re-exports in lib.rs

2. Strong Test Coverage

• Comprehensive test coverage in flags/globals.rs:58-125 with good edge case testing using rstest parameterized tests
• Tests cover various chain ID formats (numeric, string, env vars) and validation scenarios
• Test coverage for override args in flags/overrides.rs:76-168

3. Documentation and Ergonomics

• Well-documented CLI arguments with clear help text and environment variable support
• Good use of clap's derive API for clean CLI definition
• Updated README.md provides clear usage examples and architecture overview

4. Version Management

• Robust version handling in build.rs:1-92 following reth's proven patterns
• Comprehensive build-time information collection (git SHA, timestamp, features, etc.)

⚠️ Areas for Improvement

1. Unimplemented Functionality (Critical)

• bin/rollup/src/cli.rs:27: The run() method uses unimplemented!() - this makes the binary non-functional
• Recommendation: Either implement basic functionality or use todo!() with a clear plan for implementation

2. Environment Variable Inconsistencies

• bin/node/src/flags/globals.rs:22: Uses KONA_NODE_L2_CHAIN_ID
• crates/utilities/cli/src/flags/globals.rs:24: Uses KONA_L2_CHAIN_ID
• Issue: Same functionality, different env var names across binaries
• Recommendation: Standardize on one naming convention (suggest KONA_L2_CHAIN_ID)

3. Test Environment Safety

• crates/utilities/cli/src/flags/globals.rs:109-116: Uses unsafe blocks to manipulate environment variables in tests
• Issue: Could cause test isolation problems and data races
• Recommendation: Use test isolation techniques like serial_test crate or mock the environment

4. Error Handling Pattern

• flags/globals.rs:45-55: Uses anyhow::anyhow! for error creation which is less descriptive than custom error types
• Recommendation: Consider custom error types for better error categorization

5. Build Script Complexity

• build.rs:46-48: Uses brittle path manipulation with rsplit(MAIN_SEPARATOR).nth(3) to extract build profile
• Risk: Could break on different filesystem layouts
• Recommendation: Use more robust path handling or env!("PROFILE")

🔍 Security & Performance Considerations

Security: Good ✅

• No security issues identified
• Proper input validation through clap
• No hardcoded secrets or sensitive data exposure

Performance: Acceptable ✅

• CLI parsing overhead is minimal and appropriate for a CLI application
• Build script runs at compile-time, no runtime performance impact
• Efficient use of references in flag structures

📊 Test Coverage Assessment

Strengths:

• Good parameterized testing with rstest
• Tests cover both happy path and error scenarios
• Environment variable testing included

Gaps:

• No integration tests for the overall CLI workflow
• Missing tests for the actual run() implementation (though it's unimplemented)
• No tests for the build script functionality

🔧 Minor Issues

1. Version info naming: version.rs:1 comment says "kona-node" but this is for rollup binary
2. Dead code allowance: version.rs:3 has #![allow(dead_code)] - consider if all constants are actually used
3. Import organization: Some files could benefit from consistent import grouping per CLAUDE.md guidelines

📋 Recommendations Summary

1. High Priority: Implement the run() method in cli.rs or document the implementation plan
2. Medium Priority: Standardize environment variable naming across binaries
3. Low Priority: Improve test safety and add custom error types
4. Consider: Adding integration tests once core functionality is implemented

🎯 Overall Assessment

This is a solid refactoring that improves code organization and maintainability. The shared CLI utilities approach is excellent for consistency across the kona ecosystem. The main concern is the unimplemented run() method, but the foundation is well-architected for future implementation.

Status: ✅ Approve with minor changes recommended

---

[Copilot]

Pull Request Overview

This pull request refactors the CLI structure for the rollup binary by reorganizing logging and configuration modules. The refactoring improves modularity by splitting the CLI code into separate, focused modules for different types of arguments.

• Splits the single log.rs module into separate logs.rs and flags/log.rs modules
• Creates a new flags module structure with dedicated modules for different argument types
• Updates import paths across examples and utilities to reflect the new module organization

Reviewed Changes

Copilot reviewed 12 out of 26 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
examples/gossip/src/main.rs	Updates import statement to use new module path
examples/execution-fixture/src/main.rs	Updates import statement to use new module path
examples/discovery/src/main.rs	Updates import statement to use new module path
crates/utilities/cli/src/tracing.rs	Updates import to use shortened path from root module
crates/utilities/cli/src/logs.rs	New module containing logging configuration types
crates/utilities/cli/src/log.rs	Removes entire file (189 lines deleted)
crates/utilities/cli/src/lib.rs	Updates module structure and exports
crates/utilities/cli/src/flags/overrides.rs	New module for override CLI arguments
crates/utilities/cli/src/flags/mod.rs	New module for organizing CLI flags
crates/utilities/cli/src/flags/log.rs	New module containing LogArgs moved from old log.rs
crates/utilities/cli/src/flags/globals.rs	New module for global CLI arguments
crates/utilities/cli/Cargo.toml	Adds new workspace dependencies

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

The test is setting environment variable 'KONA_NODE_L2_CHAIN_ID' but the actual argument uses 'KONA_L2_CHAIN_ID' (line 24). This test will not work correctly.

Suggested change

	std::env::remove_var("KONA_NODE_L2_CHAIN_ID");
	std::env::set_var("KONA_L2_CHAIN_ID", env_value);
	}
	let args = GlobalArgs::try_parse_from(["test"]).unwrap();
	assert_eq!(args.l2_chain_id.id(), expected_id);
	unsafe {
	std::env::remove_var("KONA_L2_CHAIN_ID");

[Copilot]

The test is removing environment variable 'KONA_NODE_L2_CHAIN_ID' but should remove 'KONA_L2_CHAIN_ID' to match the actual argument configuration.

Suggested change

	std::env::remove_var("KONA_NODE_L2_CHAIN_ID");
	std::env::set_var("KONA_L2_CHAIN_ID", env_value);
	}
	let args = GlobalArgs::try_parse_from(["test"]).unwrap();
	assert_eq!(args.l2_chain_id.id(), expected_id);
	unsafe {
	std::env::remove_var("KONA_L2_CHAIN_ID");

[codecov]

Codecov Report

❌ Patch coverage is 79.47020% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.0%. Comparing base (2ba747f) to head (c1e11ba).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/utilities/cli/src/logs.rs	29.6%	19 Missing ⚠️
bin/rollup/src/main.rs	0.0%	6 Missing ⚠️
crates/utilities/cli/src/flags/globals.rs	86.3%	3 Missing ⚠️
bin/rollup/src/cli.rs	0.0%	2 Missing ⚠️
crates/utilities/cli/src/flags/log.rs	92.3%	1 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.