Refactor standard library argument validation logic - #314
Conversation
- Create `src/stdlib/helpers.rs` with reusable `check_arg_count`, `expect_text`, etc. - Refactor `core`, `crypto`, `filesystem`, `json`, `list`, `math`, `text`, `time` modules to use helpers. - Standardize error messages for argument validation. - Remove redundant code and improve maintainability. - Fix unused imports in `list.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 Code Review CompleteI've completed a thorough review of this pull request that refactors standard library argument validation logic. Here's my analysis: Todo List:
🎯 Overall Assessment: Excellent Refactor ✅This is a high-quality refactoring that successfully eliminates code duplication while improving consistency and maintainability. 🔍 Code Quality✅ Strengths:
🔧 Minor Improvements:
🐛 Bug Analysis✅ No Critical Issues Found
🚀 Performance Considerations✅ Performance Impact: Positive
🔒 Security Assessment✅ Security: Well-Maintained
🧪 Test Coverage✅ Test Strategy: Sound
📋 Specific Technical Review
Module Integration: Clean imports and usage
Error Consistency: The standardization from generic errors like 🎯 Recommendations
This refactoring exemplifies best practices: eliminating duplication, improving consistency, and maintaining backward compatibility while reducing overall codebase complexity. |
📝 WalkthroughWalkthroughThis pull request introduces a new centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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 standard library modules to consolidate duplicated argument validation and type checking logic into a shared helpers.rs module, improving code maintainability and consistency across the codebase.
Changes:
- Created
src/stdlib/helpers.rswith reusable validation functions (check_arg_count,check_arg_range,expect_*type extractors) - Refactored 8 standard library modules to use the shared helpers, eliminating local duplicates
- Updated test assertions to reflect new standardized error messages
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/stdlib/helpers.rs | New module containing shared argument validation and type extraction functions |
| src/stdlib/mod.rs | Added helpers module to stdlib exports |
| src/stdlib/time.rs | Replaced local validation with helper functions |
| src/stdlib/text.rs | Removed duplicate expect_text and expect_number, using shared helpers |
| src/stdlib/math.rs | Removed duplicate expect_number, using shared helper |
| src/stdlib/list.rs | Removed duplicate expect_list and unused expect_number |
| src/stdlib/json.rs | Removed duplicate expect_text, using shared helper |
| src/stdlib/filesystem.rs | Updated to use helpers and fixed expect_text return type handling with .as_ref() |
| src/stdlib/crypto.rs | Replaced inline validation with helper functions and added arg count check to native_generate_csrf_token |
| tests/wflhash_hardened_security_test.rs | Updated test assertions to match new standardized error message format |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,32 +1,16 @@ | |||
| use super::helpers::{check_arg_count, check_arg_range, expect_text}; | |||
There was a problem hiding this comment.
The import of check_arg_range is used only once (in native_remove_dir). Consider whether this helper provides sufficient value given its single usage, or if the inline validation might be clearer in this specific case.
| match value { | ||
| Value::Number(n) => Ok(*n), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected a number, got {}", value.type_name()), |
There was a problem hiding this comment.
Inconsistent capitalization in error messages: 'Expected a number' vs 'Expected text' vs 'Expected a Date'. Consider standardizing to either always include or always omit the article 'a/an' for consistency.
| match value { | ||
| Value::Text(s) => Ok(Rc::clone(s)), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected text, got {}", value.type_name()), |
There was a problem hiding this comment.
Inconsistent capitalization in error messages: 'Expected a number' vs 'Expected text' vs 'Expected a Date'. Consider standardizing to either always include or always omit the article 'a/an' for consistency.
| format!("Expected text, got {}", value.type_name()), | |
| format!("Expected a text, got {}", value.type_name()), |
| match value { | ||
| Value::List(list) => Ok(Rc::clone(list)), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected a list, got {}", value.type_name()), |
There was a problem hiding this comment.
Inconsistent capitalization in error messages: 'Expected a number' vs 'Expected text' vs 'Expected a Date'. Consider standardizing to either always include or always omit the article 'a/an' for consistency.
| match value { | ||
| Value::Bool(b) => Ok(*b), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected a boolean, got {}", value.type_name()), |
There was a problem hiding this comment.
Inconsistent capitalization in error messages: 'Expected a number' vs 'Expected text' vs 'Expected a Date'. Consider standardizing to either always include or always omit the article 'a/an' for consistency.
| match value { | ||
| Value::Date(d) => Ok(Rc::clone(d)), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected a Date, got {}", value.type_name()), |
There was a problem hiding this comment.
Inconsistent capitalization in error messages: 'Expected a number' vs 'Expected text' vs 'Expected a Date'. Consider standardizing to either always include or always omit the article 'a/an' for consistency.
| match value { | ||
| Value::Time(t) => Ok(Rc::clone(t)), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected a Time, got {}", value.type_name()), |
There was a problem hiding this comment.
Inconsistent capitalization in error messages: 'Expected a number' vs 'Expected text' vs 'Expected a Date'. Consider standardizing to either always include or always omit the article 'a/an' for consistency.
| match value { | ||
| Value::DateTime(dt) => Ok(Rc::clone(dt)), | ||
| _ => Err(RuntimeError::new( | ||
| format!("Expected a DateTime, got {}", value.type_name()), |
There was a problem hiding this comment.
Inconsistent capitalization in error messages: 'Expected a number' vs 'Expected text' vs 'Expected a Date'. Consider standardizing to either always include or always omit the article 'a/an' for consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/stdlib/text.rs (2)
121-134:⚠️ Potential issue | 🟠 MajorConfirm intended behavior change for empty delimiters in
string_split.This now errors on an empty delimiter, which is a breaking change if callers previously relied on char-wise splitting. If this is intended, please update docs/tests; otherwise consider restoring the prior behavior.
🔧 Possible compatibility fix
- if delimiter.is_empty() { - return Err(RuntimeError::new( - "Empty delimiter not allowed in string split".to_string(), - 0, - 0, - )); - } + if delimiter.is_empty() { + let parts: Vec<Value> = text + .chars() + .map(|ch| Value::Text(Rc::from(ch.to_string()))) + .collect(); + return Ok(Value::List(Rc::new(RefCell::new(parts)))); + }
184-205:⚠️ Potential issue | 🟠 MajorCookie values with literal
+characters will be corrupted by currentpercent_decodeimplementation.RFC 6265 specifies that cookie values treat
+literally (no special meaning), unlike URL form encoding (application/x-www-form-urlencoded) which uses+for space. Thepercent_decodefunction currently applies form-encoding rules to all contexts, causing cookie values containing+to be incorrectly converted to spaces.This affects
native_parse_cookiesat lines 202–203. The fix requires makingpercent_decodecontext-aware: add aplus_as_spaceparameter (defaultfalse), passingtrueonly for query string and form data parsing viaparse_key_value_pairs, andfalsefor cookies.src/stdlib/time.rs (1)
102-183:⚠️ Potential issue | 🟠 MajorValidate numeric inputs before integer casts in
create_time/create_date.Rust
ascasting truncates and silently coerces floats (e.g.,-1.5 as u32wraps,1.7 as u32truncates to1,NaN as u32→0). Current validation only checks ranges after casting, allowing invalid inputs to slip through. Enforce integer + non-negative checks before casting:- let hours = expect_number(&args[0])? as u32; - let minutes = expect_number(&args[1])? as u32; + let hours_f = expect_number(&args[0])?; + let minutes_f = expect_number(&args[1])?; + if !hours_f.is_finite() || hours_f.fract() != 0.0 || hours_f < 0.0 { + return Err(RuntimeError::new( + format!("Hours must be a non-negative integer, got {hours_f}"), + 0, + 0, + )); + } + if !minutes_f.is_finite() || minutes_f.fract() != 0.0 || minutes_f < 0.0 { + return Err(RuntimeError::new( + format!("Minutes must be a non-negative integer, got {minutes_f}"), + 0, + 0, + )); + } + let hours = u32::try_from(hours_f as i64) + .map_err(|_| RuntimeError::new(format!("Hours out of range: {hours_f}"), 0, 0))?; + let minutes = u32::try_from(minutes_f as i64) + .map_err(|_| RuntimeError::new(format!("Minutes out of range: {minutes_f}"), 0, 0))?;- let year = expect_number(&args[0])? as i32; - let month = expect_number(&args[1])? as u32; - let day = expect_number(&args[2])? as u32; + let year_f = expect_number(&args[0])?; + let month_f = expect_number(&args[1])?; + let day_f = expect_number(&args[2])?; + if !year_f.is_finite() || year_f.fract() != 0.0 { + return Err(RuntimeError::new( + format!("Year must be an integer, got {year_f}"), + 0, + 0, + )); + } + if !month_f.is_finite() || month_f.fract() != 0.0 || month_f < 1.0 { + return Err(RuntimeError::new( + format!("Month must be a positive integer, got {month_f}"), + 0, + 0, + )); + } + if !day_f.is_finite() || day_f.fract() != 0.0 || day_f < 1.0 { + return Err(RuntimeError::new( + format!("Day must be a positive integer, got {day_f}"), + 0, + 0, + )); + } + let year = i32::try_from(year_f as i64) + .map_err(|_| RuntimeError::new(format!("Year out of range: {year_f}"), 0, 0))?; + let month = u32::try_from(month_f as i64) + .map_err(|_| RuntimeError::new(format!("Month out of range: {month_f}"), 0, 0))?; + let day = u32::try_from(day_f as i64) + .map_err(|_| RuntimeError::new(format!("Day out of range: {day_f}"), 0, 0))?;
|
Claude finished @logbie's task —— View job Code Review CompleteI've completed a thorough review of this pull request that refactors standard library argument validation logic. Here's my analysis: Todo List:
🎯 Overall Assessment: Excellent Refactor ✅This is a high-quality refactoring that successfully eliminates code duplication while improving consistency and maintainability. 🔍 Code Quality✅ Strengths:
🔧 Minor Improvements:
🐛 Bug Analysis✅ No Critical Issues Found
🚀 Performance Considerations✅ Performance Impact: Positive
🔒 Security Assessment✅ Security: Well-Maintained
🧪 Test Coverage✅ Test Strategy: Sound
📋 Specific Technical Review
Module Integration: Clean imports and usage
Error Consistency: The standardization from generic errors like 🎯 Recommendations
This refactoring exemplifies best practices: eliminating duplication, improving consistency, and maintaining backward compatibility while reducing overall codebase complexity. |
|
Claude finished @logbie's task —— View job Code Review CompleteI've completed a thorough review of this pull request that refactors standard library argument validation logic. Here's my analysis: Todo List:
🎯 Overall Assessment: Excellent Refactor ✅This is a high-quality refactoring that successfully eliminates code duplication while improving consistency and maintainability. 🔍 Code Quality✅ Strengths:
🔧 Minor Improvements:
🐛 Bug Analysis✅ No Critical Issues Found
🚀 Performance Considerations✅ Performance Impact: Positive
🔒 Security Assessment✅ Security: Well-Maintained
🧪 Test Coverage✅ Test Strategy: Sound
📋 Specific Technical Review
Module Integration: Clean imports and usage
Error Consistency: The standardization from generic errors like 🎯 Recommendations
This refactoring exemplifies best practices: eliminating duplication, improving consistency, and maintaining backward compatibility while reducing overall codebase complexity. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,3 +1,7 @@ | |||
| use super::helpers::{ | |||
| check_arg_count, check_arg_range, expect_date, expect_datetime, expect_number, expect_text, | |||
There was a problem hiding this comment.
The import list spans multiple lines but could be more readable with consistent formatting. Consider alphabetizing the imports or grouping them logically (e.g., check_* functions together, then expect_* functions).
| check_arg_count, check_arg_range, expect_date, expect_datetime, expect_number, expect_text, | |
| check_arg_count, | |
| check_arg_range, | |
| expect_date, | |
| expect_datetime, | |
| expect_number, | |
| expect_text, |
| // check_min_arg_count is needed here because it says "expects at least 1 argument" | ||
| // But helper implementation uses check_min_arg_count |
There was a problem hiding this comment.
The comment is confusing and seems redundant. Line 110 repeats what line 111 says, and line 111 is circular ('helper implementation uses check_min_arg_count'). Consider replacing with a clearer explanation like '// Accept variable number of path components (minimum 1)' or removing the comment entirely.
| // check_min_arg_count is needed here because it says "expects at least 1 argument" | |
| // But helper implementation uses check_min_arg_count | |
| // Accept variable number of path components (minimum 1) |
| } | ||
| // check_min_arg_count is needed here because it says "expects at least 1 argument" | ||
| // But helper implementation uses check_min_arg_count | ||
| super::helpers::check_min_arg_count("path_join", &args, 1)?; |
There was a problem hiding this comment.
This is the only place using a fully qualified path super::helpers::check_min_arg_count instead of importing it at the top like other helpers. For consistency with the rest of the file, add check_min_arg_count to the imports on line 1.
Resolved conflicts in: - src/stdlib/helpers.rs: Kept all helper functions from PR branch - src/stdlib/list.rs: Removed duplicate import - src/stdlib/math.rs: Removed duplicate import Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove duplicate imports that were added during merge with main: - src/stdlib/filesystem.rs - src/stdlib/json.rs - src/stdlib/text.rs All tests passing. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Claude finished @logbie's task —— View job Code Review CompleteI've completed a thorough review of this pull request that refactors standard library argument validation logic. Here's my analysis: Todo List:
🎯 Overall Assessment: Excellent Refactor ✅This is a high-quality refactoring that successfully eliminates code duplication while improving consistency and maintainability. 🔍 Code Quality✅ Strengths:
🔧 Minor Improvements:
🐛 Bug Analysis✅ No Critical Issues Found
🚀 Performance Considerations✅ Performance Impact: Positive
🔒 Security Assessment✅ Security: Well-Maintained
🧪 Test Coverage✅ Test Strategy: Sound
📋 Specific Technical Review
Module Integration: Clean imports and usage
Error Consistency: The standardization from generic errors like 🎯 Recommendations
This refactoring exemplifies best practices: eliminating duplication, improving consistency, and maintaining backward compatibility while reducing overall codebase complexity. |
|
Claude finished @logbie's task —— View job Code Review CompleteI've completed a thorough review of this pull request that refactors standard library argument validation logic. Here's my analysis: Todo List:
🎯 Overall Assessment: Excellent Refactor ✅This is a high-quality refactoring that successfully eliminates code duplication while improving consistency and maintainability. 🔍 Code Quality✅ Strengths:
🔧 Minor Improvements:
🐛 Bug Analysis✅ No Critical Issues Found
🚀 Performance Considerations✅ Performance Impact: Positive
🔒 Security Assessment✅ Security: Well-Maintained
🧪 Test Coverage✅ Test Strategy: Sound
📋 Specific Technical Review
Module Integration: Clean imports and usage
Error Consistency: The standardization from generic errors like 🎯 Recommendations
This refactoring exemplifies best practices: eliminating duplication, improving consistency, and maintaining backward compatibility while reducing overall codebase complexity. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn native_generate_csrf_token(args: Vec<Value>) -> Result<Value, RuntimeError> { | ||
| check_arg_count("generate_csrf_token", &args, 0)?; |
There was a problem hiding this comment.
The function signature changed from using an unused parameter prefix (_args) to a regular parameter (args), which is good. However, the validation is being added for a function that takes 0 arguments. While this makes the validation explicit, consider whether this check is necessary for zero-argument functions since it adds minimal value—calling with arguments would be caught at the call site in most cases.
This PR refactors the standard library modules to use a shared set of helper functions for argument validation and type checking.
The Issue:
Multiple standard library modules (
list.rs,text.rs,math.rs, etc.) contained duplicated logic for checking argument counts and extracting values fromValueenums (e.g.,expect_text,expect_number). This redundancy violated DRY and led to inconsistent error handling.The Solution:
src/stdlib/helpers.rsto house common validation logic:check_arg_countcheck_min_arg_countcheck_arg_rangeexpect_number,expect_text,expect_list,expect_boolexpect_date,expect_time,expect_datetimetests/wflhash_hardened_security_test.rsto align with the new, standardized error messages.Verification:
cargo fmtpassed.cargo clippypassed (fixed unused import warnings).cargo testpassed (all suites, including integration tests requiring release binary).PR created automatically by Jules for task 1559504527358475602 started by @logbie
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements