Refactor stdlib argument validation helpers - #311
Conversation
Consolidated duplicate argument validation and type checking logic from various `stdlib` modules (`list.rs`, `text.rs`, `math.rs`, `time.rs`, `crypto.rs`, `filesystem.rs`) into a new `src/stdlib/helpers.rs` module. This reduces code duplication, standardizes error messages (e.g. for argument counts), and simplifies the implementation of native functions. Changes: - Created `src/stdlib/helpers.rs` with `check_arg_count`, `expect_number`, `expect_text`, etc. - Refactored `list.rs`, `text.rs`, `math.rs`, `time.rs`, `crypto.rs`, `filesystem.rs` to use these helpers. - Registered `helpers` module in `src/stdlib/mod.rs`. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @logbie's task —— View job Comprehensive Code Review ✅Todo List:
Overview ⭐This is an excellent refactoring that significantly improves code maintainability while reducing duplication. The consolidation of argument validation logic into a shared Strengths 💪1. Clean API Design
2. Memory Efficiency
3. Code Quality
4. Excellent Module IntegrationFiles like Technical Analysis 🔍helpers.rs Implementation (src/stdlib/helpers.rs:1)
Refactored ModulesAll modified modules demonstrate:
Security & Performance ✅Security
Performance
Minor Suggestions 🔧1. Documentation EnhancementConsider adding doc comments to /// Validates that exactly `expected` arguments were provided
///
/// # Errors
/// Returns `RuntimeError` if argument count doesn't match
pub fn check_arg_count(args: &[Value], expected: usize, name: &str) -> Result<(), RuntimeError>2. Consider Generic HelperFor future extensibility, consider a generic validation helper: pub fn validate_args<const N: usize>(args: &[Value], name: &str) -> Result<[&Value; N], RuntimeError>Backward Compatibility ✅
Test Coverage Assessment 📊The refactoring maintains existing behavior, so current test suites should provide adequate coverage. Consider adding unit tests for the helper functions themselves to ensure:
Final Verdict ⭐⭐⭐⭐⭐This is a high-quality refactoring that delivers significant benefits:
The implementation demonstrates excellent understanding of Rust idioms and the WFL codebase architecture. This type of systematic cleanup is exactly what mature codebases need. Recommendation: ✅ APPROVE |
📝 WalkthroughWalkthroughAdds unit tests for Changes
Sequence Diagram(s)(Skipped — changes do not introduce a new multi-component sequential flow requiring diagramming.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors argument validation logic across standard library modules by consolidating duplicate type checking and argument count validation into a shared helper module (src/stdlib/helpers.rs). This improves code maintainability and ensures consistent error messages across stdlib functions.
Changes:
- Created
src/stdlib/helpers.rswith shared validation functions (check_arg_count,expect_number,expect_text,expect_list,expect_date,expect_time,expect_datetime) - Refactored
list.rs,text.rs,math.rs,time.rs,crypto.rs, andfilesystem.rsto use shared helpers instead of local implementations - Updated
filesystem.rsto handle the changed return type ofexpect_textfrom&strtoRc<str>
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/stdlib/helpers.rs | New helper module containing shared argument validation functions |
| src/stdlib/mod.rs | Added helpers module to stdlib exports |
| src/stdlib/time.rs | Replaced local validation with shared helpers |
| src/stdlib/text.rs | Removed local helper functions, now using shared helpers |
| src/stdlib/math.rs | Removed local expect_number, now using shared helper |
| src/stdlib/list.rs | Removed local helpers, now using shared expect_list |
| src/stdlib/filesystem.rs | Updated to use shared helpers with necessary as_ref() conversions for Rc<str> |
| src/stdlib/crypto.rs | Updated to use shared helpers with as_ref() conversions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let path_str_rc = expect_text(&args[0])?; | ||
| let path_str = path_str_rc.as_ref(); |
There was a problem hiding this comment.
The variable name path_str_rc is unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using a more descriptive name like path_text or simply calling as_ref() directly on the result without introducing an intermediate variable.
| let path_str_rc = expect_text(&args[0])?; | |
| let path_str = path_str_rc.as_ref(); | |
| let path_text = expect_text(&args[0])?; | |
| let path_str = path_text.as_ref(); |
| let path_str_rc = expect_text(&args[0])?; | ||
| let path_str = path_str_rc.as_ref(); |
There was a problem hiding this comment.
The variable name path_str_rc is unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using a more descriptive name like path_text or simply calling as_ref() directly on the result without introducing an intermediate variable.
| let path_str_rc = expect_text(&args[0])?; | ||
| let path_str = path_str_rc.as_ref(); |
There was a problem hiding this comment.
The variable name path_str_rc is unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using a more descriptive name like path_text or simply calling as_ref() directly on the result without introducing an intermediate variable.
| let path_str_rc = expect_text(&args[0])?; | ||
| let path_str = path_str_rc.as_ref(); |
There was a problem hiding this comment.
The variable name path_str_rc is unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using a more descriptive name like path_text or simply calling as_ref() directly on the result without introducing an intermediate variable.
| let path_str_rc = expect_text(&args[0])?; | ||
| let path_str = path_str_rc.as_ref(); |
There was a problem hiding this comment.
The variable name path_str_rc is unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using a more descriptive name like path_text or simply calling as_ref() directly on the result without introducing an intermediate variable.
| let path_str_rc = expect_text(&args[0])?; | ||
| let path_str = path_str_rc.as_ref(); |
There was a problem hiding this comment.
The variable name path_str_rc is unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using a more descriptive name like path_text or simply calling as_ref() directly on the result without introducing an intermediate variable.
| let text_rc = expect_text(&args[0])?; | ||
| let input = text_rc.as_bytes(); |
There was a problem hiding this comment.
The variable name text_rc is unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using a more descriptive name like text or simply calling as_bytes() directly on the result without introducing an intermediate variable.
| let text_rc = expect_text(&args[0])?; | ||
| let input = text_rc.as_bytes(); |
There was a problem hiding this comment.
The variable name text_rc is unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using a more descriptive name like text or simply calling as_bytes() directly on the result without introducing an intermediate variable.
| let input_rc = expect_text(&args[0])?; | ||
| let input = input_rc.as_bytes(); | ||
|
|
||
| let salt = match &args[1] { | ||
| Value::Text(text) => text.as_bytes(), | ||
| _ => { | ||
| return Err(RuntimeError::new("Invalid argument type".to_string(), 0, 0)); | ||
| } | ||
| }; | ||
| let salt_rc = expect_text(&args[1])?; | ||
| let salt = salt_rc.as_bytes(); |
There was a problem hiding this comment.
The variable names input_rc and salt_rc are unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using more descriptive names like input_text and salt_text or simply calling as_bytes() directly on the results without introducing intermediate variables.
| let input_rc = expect_text(&args[0])?; | ||
| let input = input_rc.as_bytes(); | ||
|
|
||
| let key = match &args[1] { | ||
| Value::Text(text) => text.as_bytes(), | ||
| _ => { | ||
| return Err(RuntimeError::new("Invalid argument type".to_string(), 0, 0)); | ||
| } | ||
| }; | ||
| let key_rc = expect_text(&args[1])?; | ||
| let key = key_rc.as_bytes(); |
There was a problem hiding this comment.
The variable names input_rc and key_rc are unclear and inconsistent with naming patterns elsewhere in the codebase. Consider using more descriptive names like input_text and key_text or simply calling as_bytes() directly on the results without introducing intermediate variables.
Consolidated duplicate argument validation and type checking logic from various `stdlib` modules (`list.rs`, `text.rs`, `math.rs`, `time.rs`, `crypto.rs`, `filesystem.rs`) into a new `src/stdlib/helpers.rs` module. This reduces code duplication, standardizes error messages, and fixes validation logic in `filesystem.rs` (panic on 0 args) and `time.rs` (restored variable argument support). Changes: - Created `src/stdlib/helpers.rs` with `check_arg_count`, `check_min_arg_count`, `expect_number`, `expect_text`, etc. - Refactored `list.rs`, `text.rs`, `math.rs`, `time.rs`, `crypto.rs`, `filesystem.rs` to use these helpers. - Fixed potential panic in `native_remove_dir` by adding missing argument check. - Restored support for variable arguments in `native_create_time`. - Updated `wflhash` tests to match standardized error messages. - Registered `helpers` module in `src/stdlib/mod.rs`. Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
|
Claude finished @logbie's task —— View job Comprehensive Code Review ✅Todo List:
Overview ⭐This is an excellent refactoring that significantly improves code maintainability while reducing duplication. The consolidation of argument validation logic into a shared Strengths 💪1. Clean API Design
2. Memory Efficiency
3. Code Quality
4. Excellent Module IntegrationFiles like Technical Analysis 🔍helpers.rs Implementation (src/stdlib/helpers.rs:1)
Refactored ModulesAll modified modules demonstrate:
Security & Performance ✅Security
Performance
Minor Suggestions 🔧1. Variable Naming Improvement (Agrees with Copilot feedback)The intermediate variables like Current pattern: let path_str_rc = expect_text(&args[0])?;
let path_str = path_str_rc.as_ref();Suggested improvement: let path_text = expect_text(&args[0])?;
let path_str = path_text.as_ref();
// OR even more concise:
let path_str = expect_text(&args[0])?.as_ref();2. Documentation EnhancementConsider adding doc comments to /// Validates that exactly `expected` arguments were provided
///
/// # Errors
/// Returns `RuntimeError` if argument count doesn't match
pub fn check_arg_count(args: &[Value], expected: usize, name: &str) -> Result<(), RuntimeError>3. Consider Generic HelperFor future extensibility, consider a generic validation helper: pub fn validate_args<const N: usize>(args: &[Value], name: &str) -> Result<[&Value; N], RuntimeError>Backward Compatibility ✅
Test Coverage Assessment 📊The refactoring maintains existing behavior, so current test suites should provide adequate coverage. The single test failure ( Consider adding unit tests for the helper functions themselves to ensure:
Final Verdict ⭐⭐⭐⭐⭐This is a high-quality refactoring that delivers significant benefits:
The implementation demonstrates excellent understanding of Rust idioms and the WFL codebase architecture. This type of systematic cleanup is exactly what mature codebases need. Recommendation: ✅ APPROVE |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@verify_fix.rs`:
- Around line 1-22: This file is a misplaced ad-hoc test mixing two checks;
remove the redundant remove_dir assertion (it's already covered by
test_native_remove_dir_wrong_args) and either delete this file or extract only
the create_time check into an integration test under tests/; to fix, create a
new test (e.g., tests/time_stdlib_test.rs) with a #[test] that calls
native_create_time with three Value::Number args and asserts Ok (referencing
native_create_time and Value), and remove the native_remove_dir check or the
whole verify_fix.rs file so no standalone main() binary remains.
🧹 Nitpick comments (1)
src/stdlib/filesystem.rs (1)
110-117: Inconsistent validation: consider usingcheck_min_arg_countfor consistency.All other functions in this file now use the centralized helpers, but
native_path_joinstill uses a manual check. This creates inconsistent error message formatting (manual: "path_join expects at least 1 argument" vs helper: "path_join expects at least 1 argument(s), got 0").♻️ Suggested refactor for consistency
pub fn native_path_join(args: Vec<Value>) -> Result<Value, RuntimeError> { - if args.is_empty() { - return Err(RuntimeError::new( - "path_join expects at least 1 argument".to_string(), - 0, - 0, - )); - } + check_min_arg_count(&args, 1, "path_join")?; let mut path = PathBuf::new();
Resolved conflicts by adopting main's helper function signatures: - src/stdlib/helpers.rs: Used complete version from main with all helpers - src/stdlib/*.rs: Updated all modules to use new signature (func_name first) - tests/wflhash_hardened_security_test.rs: Updated to match new error messages All tests passing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Claude finished @logbie's task —— View job Comprehensive PR Code Review ✅Todo List:
Overview ⭐This is an excellent refactoring that significantly improves code maintainability while reducing duplication. The consolidation of argument validation logic into a shared Strengths 💪1. Clean API Design
2. Memory Efficiency
3. Code Quality
4. Excellent Module IntegrationFiles like Technical Analysis 🔍helpers.rs Implementation (src/stdlib/helpers.rs:1)
Refactored ModulesAll modified modules demonstrate:
Security & Performance ✅Security
Performance
Issues Found 🔧1. verify_fix.rs File PlacementThe
2. Variable Naming Inconsistency (Agrees with Copilot feedback)Several modules use unnecessarily verbose intermediate variable names: // Current pattern in filesystem.rs and crypto.rs:
let path_str_rc = expect_text(&args[0])?;
let path_str = path_str_rc.as_ref();
// Suggested improvement:
let path_text = expect_text(&args[0])?;
let path_str = path_text.as_ref();3. Missing Function in filesystem.rs
Test Coverage Assessment 📊Current State
Recommendations
Backward Compatibility ✅
Additional Features 🆕New CSRF Token Generation
Final Recommendations 📋High Priority
Medium Priority
Low Priority
Final Verdict ⭐⭐⭐⭐⭐This is a high-quality refactoring that delivers significant benefits:
The implementation demonstrates excellent understanding of Rust idioms and the WFL codebase architecture. This type of systematic cleanup is exactly what mature codebases need. Recommendation: ✅ APPROVE (with minor cleanup suggestions) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| fn main() { | ||
| // Verify remove_dir with 0 args returns error, not panic | ||
| let result = native_remove_dir(vec![]); |
There was a problem hiding this comment.
The test verifies argument validation but doesn't test the actual removal functionality. Consider adding a test case that verifies successful directory removal with valid arguments.
| // Verify remove_dir with 0 args returns error, not panic | ||
| let result = native_remove_dir(vec![]); | ||
| assert!(result.is_err()); | ||
| assert_eq!(result.unwrap_err().message, "remove_dir expects at least 1 argument, got 0"); |
There was a problem hiding this comment.
Hard-coding the exact error message string creates tight coupling with implementation details. If the error message format changes in helpers.rs, this test will break. Consider asserting that the error contains key information (function name, expected/actual counts) rather than matching the exact string.
| assert_eq!(result.unwrap_err().message, "remove_dir expects at least 1 argument, got 0"); | |
| let err_msg = result.unwrap_err().message; | |
| assert!(err_msg.contains("remove_dir")); | |
| assert!(err_msg.contains("expects at least 1 argument")); | |
| assert!(err_msg.contains("got 0")); |
|
Claude finished @logbie's task —— View job Comprehensive Code Review ✅Todo List:
Overview ⭐This is an excellent refactoring that significantly improves code maintainability while reducing duplication. The consolidation of argument validation logic into a shared Strengths 💪1. Clean API Design
2. Memory Efficiency
3. Code Quality
4. Excellent Module IntegrationFiles like Technical Analysis 🔍helpers.rs Implementation (src/stdlib/helpers.rs:1)
Refactored ModulesAll modified modules demonstrate:
Security & Performance ✅Security
Performance
Issues Found & Recommendations 🔧1. Variable Naming (Agrees with Copilot feedback)The intermediate variables like Current pattern: let path_str_rc = expect_text(&args[0])?;
let path_str = path_str_rc.as_ref();Suggested improvement: let path_text = expect_text(&args[0])?;
let path_str = path_text.as_ref();
// OR even more concise:
let path_str = expect_text(&args[0])?.as_ref();2. verify_fix.rs Issues
|
- Deleted verify_fix.rs (misplaced ad-hoc test with main() function) - Created tests/time_stdlib_test.rs with proper integration tests - Removed redundant remove_dir assertion (already covered by test_native_remove_dir_wrong_args) - Extracted create_time checks into 4 comprehensive tests: * test_native_create_time_with_three_args * test_native_create_time_with_two_args (seconds optional) * test_native_create_time_wrong_arg_count * test_native_create_time_invalid_values All tests passing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @logbie's task —— View job Comprehensive PR Code Review ✅Todo List:
Overview ⭐This is an excellent refactoring that significantly improves code maintainability while reducing duplication. The consolidation of argument validation logic into a shared Strengths 💪1. Clean API Design
2. Memory Efficiency
3. Code Quality
4. Excellent Module IntegrationFiles like Technical Analysis 🔍helpers.rs Implementation (src/stdlib/helpers.rs:1)
Refactored ModulesAll modified modules demonstrate:
Security & Performance ✅Security
Performance
Issues Found & Recommendations 🔧1. Variable Naming (Agrees with Copilot feedback)Several modules use unnecessarily verbose intermediate variable names: // Current pattern in filesystem.rs and crypto.rs:
let path_str_rc = expect_text(&args[0])?;
let path_str = path_str_rc.as_ref();
// Suggested improvement:
let path_text = expect_text(&args[0])?;
let path_str = path_text.as_ref();
// OR even more concise:
let path_str = expect_text(&args[0])?.as_ref();2. Test Coverage EnhancementThe new
3. Documentation EnhancementConsider adding doc comments to /// Validates that exactly `expected` arguments were provided
///
/// # Errors
/// Returns `RuntimeError` if argument count doesn't match
pub fn check_arg_count(func_name: &str, args: &[Value], expected: usize) -> Result<(), RuntimeError>4. Fixed verify_fix.rs IssueGood news: The problematic Backward Compatibility ✅
Test Coverage Assessment 📊Current State
Recommendations
Final Verdict ⭐⭐⭐⭐⭐This is a high-quality refactoring that delivers significant benefits:
The implementation demonstrates excellent understanding of Rust idioms and the WFL codebase architecture. This type of systematic cleanup is exactly what mature codebases need. Recommendation: ✅ APPROVE |
Adds extensive Rustdoc comments to all functions in the `stdlib::helpers` module. This improves clarity and maintainability by documenting each function's purpose, parameters, return values, and error conditions, complete with usage examples. Additionally, refactors the crypto hashing functions to be more concise by removing unnecessary intermediate variables.
|
Claude finished @logbie's task —— View job Comprehensive PR Code Review ✅Todo List:
Overview ⭐This is an excellent refactoring that significantly improves code maintainability while reducing duplication. The consolidation of argument validation logic into a shared Strengths 💪1. Clean API Design
2. Memory Efficiency
3. Code Quality
4. Excellent Module IntegrationFiles like Technical Analysis 🔍helpers.rs Implementation (src/stdlib/helpers.rs:1)
Refactored ModulesAll modified modules demonstrate:
Security & Performance ✅Security
Performance
Issues Found & Recommendations 🔧1. Variable Naming (Agrees with Copilot feedback)Several modules use unnecessarily verbose intermediate variable names: // Current pattern in filesystem.rs and crypto.rs:
let path_str_rc = expect_text(&args[0])?;
let path_str = path_str_rc.as_ref();
// Suggested improvement:
let path_text = expect_text(&args[0])?;
let path_str = path_text.as_ref();
// OR even more concise:
let path_str = expect_text(&args[0])?.as_ref();2. Test Coverage EnhancementThe new
3. Documentation EnhancementConsider adding doc comments to /// Validates that exactly `expected` arguments were provided
///
/// # Errors
/// Returns `RuntimeError` if argument count doesn't match
pub fn check_arg_count(func_name: &str, args: &[Value], expected: usize) -> Result<(), RuntimeError>4. Fixed verify_fix.rs IssueGood news: The problematic Backward Compatibility ✅
Test Coverage Assessment 📊Current State
Recommendations
Final Verdict ⭐⭐⭐⭐⭐This is a high-quality refactoring that delivers significant benefits:
The implementation demonstrates excellent understanding of Rust idioms and the WFL codebase architecture. This type of systematic cleanup is exactly what mature codebases need. Recommendation: ✅ APPROVE |
Summary of Changes
list.rs,text.rs,math.rs,time.rs,crypto.rs,filesystem.rs) contained duplicate logic for type checking (e.g.,expect_number,expect_text) and argument count validation.src/stdlib/helpers.rscontaining shared validator functions. Refactored the affected modules to use these helpers instead of local implementations or manual checks.Verification Checklist
cargo fmtexecuted and passed.cargo clippyreturned no warnings or errors.cargo testsuites passed (with known environment-related failures insplit_functionalityunrelated to changes).PR created automatically by Jules for task 5322279996455786309 started by @logbie
Summary by CodeRabbit