[JULES] Refactor: Utilize standard helpers in random module - #388
Conversation
This patch updates `src/stdlib/random.rs` to use the standard helper functions (`check_arg_count`, `expect_number`, `expect_list`) from `src/stdlib/helpers.rs`, significantly reducing duplicated validation logic across all random number generation functions. 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. |
📝 WalkthroughWalkthroughRefactors Changes
Sequence Diagram(s)(Skipped — changes are refactor/validation updates without new multi-component control flow.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
Refactors the random stdlib module to use shared argument-count and type-extraction helpers, aligning it with the patterns used across other stdlib modules and reducing duplicated validation logic.
Changes:
- Replaced per-function manual arg-count checks with
check_arg_count. - Replaced manual
Valuematching for numeric/list inputs withexpect_number/expect_list. - Added explicit
0-arg validation togenerate_uuid.
Comments suppressed due to low confidence (1)
src/stdlib/random.rs:136
native_generate_uuidbehavior was changed (arg validation added), but there are no tests asserting (a) it rejects unexpected arguments and (b) it returns a UUID-formatted string. Adding a small unit test alongside the existing module tests would lock in the intended contract.
pub fn native_generate_uuid(args: Vec<Value>) -> Result<Value, RuntimeError> {
check_arg_count("generate_uuid", &args, 0)?;
use uuid::Uuid;
let uuid = Uuid::new_v4();
Ok(Value::Text(Arc::from(uuid.to_string())))
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn native_random_from(args: Vec<Value>) -> Result<Value, RuntimeError> { | ||
| if args.len() != 1 { | ||
| check_arg_count("random_from", &args, 1)?; | ||
|
|
||
| let list_ref = expect_list(&args[0])?; | ||
| let list = list_ref.borrow(); | ||
|
|
||
| if list.is_empty() { | ||
| return Err(RuntimeError::new( | ||
| format!("random_from expects 1 argument, got {}", args.len()), | ||
| "random_from: cannot select from empty list".to_string(), | ||
| 0, | ||
| 0, | ||
| )); | ||
| } | ||
|
|
||
| match &args[0] { | ||
| Value::List(list_ref) => { | ||
| let list = list_ref.borrow(); | ||
| if list.is_empty() { | ||
| return Err(RuntimeError::new( | ||
| "random_from: cannot select from empty list".to_string(), | ||
| 0, | ||
| 0, | ||
| )); | ||
| } | ||
|
|
||
| RNG.with(|rng| { | ||
| let mut rng = rng.borrow_mut(); | ||
| let index = rng.random_range(0..list.len()); | ||
| Ok(list[index].clone()) | ||
| }) | ||
| } | ||
| _ => Err(RuntimeError::new( | ||
| format!("random_from expects a list, got {}", args[0].type_name()), | ||
| 0, | ||
| 0, | ||
| )), | ||
| } | ||
| RNG.with(|rng| { | ||
| let mut rng = rng.borrow_mut(); | ||
| let index = rng.random_range(0..list.len()); | ||
| Ok(list[index].clone()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
native_random_from was refactored but still has no local unit tests covering the key behaviors (returns an element from a non-empty list; errors on empty list; errors on non-list input). Since this module already has tests for other random functions, adding a couple focused tests here would help prevent regressions.
| pub fn native_generate_uuid(args: Vec<Value>) -> Result<Value, RuntimeError> { | ||
| check_arg_count("generate_uuid", &args, 0)?; | ||
|
|
There was a problem hiding this comment.
native_generate_uuid now enforces 0 arguments via check_arg_count, whereas it previously accepted (and ignored) any arguments. If this stricter validation is intentional, consider calling it out in any user-facing changelog/docs; otherwise, keep backward compatibility by allowing extra args (or add a deprecation path).
| pub fn native_generate_uuid(args: Vec<Value>) -> Result<Value, RuntimeError> { | |
| check_arg_count("generate_uuid", &args, 0)?; | |
| pub fn native_generate_uuid(_args: Vec<Value>) -> Result<Value, RuntimeError> { |
|
@google-labs-jules please address the remaining blocking items before merge:\n\n- [ ] Add/extend tests for to cover both: (1) it rejects unexpected arguments, and (2) it returns a valid UUID-formatted string.\n- [ ] Re-run CI after the test update and ensure all required checks stay green.\n- [ ] Confirm CodeRabbit review is fully complete and that there are no unresolved substantive review findings from bots/humans.\n\nOnce these are done, reply here and I’ll re-check for merge readiness. |
|
@google-labs-jules correction to item 1 above: please add/extend tests for native_generate_uuid / |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stdlib/random.rs (1)
35-52:⚠️ Potential issue | 🔴 CriticalGuard
random_betweenagainst non-finite bounds to prevent panic.
expect_numberonly validates type, not finiteness, allowingNaNand±infto reachrandom_range. Inrand0.9.x, the sampler (Uniform::new_inclusive) rejects invalid float bounds and returns an error, butrandom_rangeunwraps the result, causing a thread panic on non-finite inputs. Add an explicit finite-number check before the call:let min = expect_number(&args[0])?; let max = expect_number(&args[1])?; + if !min.is_finite() || !max.is_finite() { + return Err(RuntimeError::new( + "random_between: min and max must be finite numbers".to_string(), + 0, + 0, + )); + } + if min > max { return Err(RuntimeError::new( format!(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdlib/random.rs` around lines 35 - 52, The random_between implementation allows NaN/∞ to reach rng.random_range and panic; before calling RNG.with (i.e., after let min = expect_number(&args[0])?; let max = expect_number(&args[1])?;), add explicit finite checks for both min and max (use is_finite) and return a RuntimeError::new with a clear message (e.g., "random_between: bounds must be finite numbers") if either is not finite; this prevents passing invalid floats into random_range and avoids the unwrap/panic.
🧹 Nitpick comments (1)
src/stdlib/random.rs (1)
60-61: Consider explicit integer validation for clearer API semantics.The
as i64/as u64casts are well-defined in Rust (truncate toward zero, saturate on overflow), but accepting fractional inputs like1.9orNaNwithout validation means silent truncation to1or0. For a random library API, stricter input validation clarifies intent: either accept only integer-valued floats or reject fractional bounds explicitly.♻️ Suggested validation
pub fn native_random_int(args: Vec<Value>) -> Result<Value, RuntimeError> { check_arg_count("random_int", &args, 2)?; - let min = expect_number(&args[0])? as i64; - let max = expect_number(&args[1])? as i64; + let min_raw = expect_number(&args[0])?; + let max_raw = expect_number(&args[1])?; + if !min_raw.is_finite() || !max_raw.is_finite() || min_raw.fract() != 0.0 || max_raw.fract() != 0.0 { + return Err(RuntimeError::new( + "random_int expects finite integer bounds".to_string(), + 0, + 0, + )); + } + let min = min_raw as i64; + let max = max_raw as i64; @@ pub fn native_random_seed(args: Vec<Value>) -> Result<Value, RuntimeError> { check_arg_count("random_seed", &args, 1)?; - let seed = expect_number(&args[0])? as u64; + let seed_raw = expect_number(&args[0])?; + if !seed_raw.is_finite() || seed_raw.fract() != 0.0 || seed_raw < 0.0 { + return Err(RuntimeError::new( + "random_seed expects a non-negative integer seed".to_string(), + 0, + 0, + )); + } + let seed = seed_raw as u64;Also applies to: line 118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdlib/random.rs` around lines 60 - 61, The code is silently truncating floating inputs when casting the results of expect_number(&args[..]) to integers; update the validation in the places that read numeric args (the min/max extraction around expect_number(&args[0]) and expect_number(&args[1]) and the similar extraction at the other location near line 118) to explicitly reject non-integer or non-finite values: after calling expect_number(...) check that the returned f64 is finite (not NaN/inf) and that value.fract() == 0.0 (or otherwise within integer semantics you want), then only convert to i64/u64; on failure return a clear error mentioning the argument name and that an integer was required. Ensure you reference and update the same validation logic for both min/max and the other numeric parameter to keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/stdlib/random.rs`:
- Around line 35-52: The random_between implementation allows NaN/∞ to reach
rng.random_range and panic; before calling RNG.with (i.e., after let min =
expect_number(&args[0])?; let max = expect_number(&args[1])?;), add explicit
finite checks for both min and max (use is_finite) and return a
RuntimeError::new with a clear message (e.g., "random_between: bounds must be
finite numbers") if either is not finite; this prevents passing invalid floats
into random_range and avoids the unwrap/panic.
---
Nitpick comments:
In `@src/stdlib/random.rs`:
- Around line 60-61: The code is silently truncating floating inputs when
casting the results of expect_number(&args[..]) to integers; update the
validation in the places that read numeric args (the min/max extraction around
expect_number(&args[0]) and expect_number(&args[1]) and the similar extraction
at the other location near line 118) to explicitly reject non-integer or
non-finite values: after calling expect_number(...) check that the returned f64
is finite (not NaN/inf) and that value.fract() == 0.0 (or otherwise within
integer semantics you want), then only convert to i64/u64; on failure return a
clear error mentioning the argument name and that an integer was required.
Ensure you reference and update the same validation logic for both min/max and
the other numeric parameter to keep behavior consistent.
Added the requested tests |
Acknowledged. Both parts (a) and (b) of your request have been covered in the new tests for |
…t validation Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/stdlib/random.rs`:
- Around line 183-188: The test function test_generate_uuid_validates_args
contains a long chained expression (the assert on
result.unwrap_err().message.contains(...)) that is misformatted; reflow that
assertion into multiple lines or otherwise format the line to satisfy rustfmt
(or simply run cargo fmt --all to auto-fix), ensuring the block around
native_generate_uuid(vec![Value::Number(1.0)]) and both assert! calls are
formatted per rustfmt rules so the CI cargo fmt check passes.
|
@google-labs-jules follow-up triage after your latest commit — thank you for adding UUID tests. Remaining blockers before merge:
Notes:
Reply when the blockers are resolved and I will re-check merge readiness. |
Ran `cargo fmt` to resolve the CI check formatting failure in `src/stdlib/random.rs` after the previous commit. Co-authored-by: logbie <1138960+logbie@users.noreply.github.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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stdlib/random.rs (1)
35-52:⚠️ Potential issue | 🔴 CriticalAdd finite bounds validation to
native_random_betweento prevent panic.At line 51,
rng.random_range(min..=max)panics if eitherminormaxis NaN or infinite. The existingif min > maxcheck does not catch these cases (NaN comparisons always return false). Reject non-finite bounds before callingrandom_range.pub fn native_random_between(args: Vec<Value>) -> Result<Value, RuntimeError> { check_arg_count("random_between", &args, 2)?; let min = expect_number(&args[0])?; let max = expect_number(&args[1])?; + + if !min.is_finite() || !max.is_finite() { + return Err(RuntimeError::new( + "random_between: min and max must be finite numbers".to_string(), + 0, + 0, + )); + } if min > max { return Err(RuntimeError::new( format!( "random_between: min ({}) cannot be greater than max ({})", min, max ), 0, 0, )); } RNG.with(|rng| { let mut rng = rng.borrow_mut(); let random_value: f64 = rng.random_range(min..=max); Ok(Value::Number(random_value)) }) }Add a regression test covering non-finite cases:
#[test] fn test_random_between_rejects_non_finite_bounds() { let cases = [ (f64::NAN, 1.0), (1.0, f64::NAN), (f64::INFINITY, 1.0), (1.0, f64::NEG_INFINITY), ]; for (min, max) in cases { let result = native_random_between(vec![Value::Number(min), Value::Number(max)]); assert!(result.is_err()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdlib/random.rs` around lines 35 - 52, The native_random_between implementation must validate that both bounds are finite before calling RNG.random_range to avoid panics; in the function handling min and max (e.g., native_random_between / the block using expect_number, min, max and RNG.with), check min.is_finite() and max.is_finite() and return a RuntimeError with a clear message if either is not finite, then proceed to call rng.random_range(min..=max) only when both are finite and min <= max; also add regression tests exercising NaN, +inf and -inf for min and max to assert the function returns an error instead of panicking.
🧹 Nitpick comments (1)
src/stdlib/random.rs (1)
200-204: Strengthen UUID validation test by parsing the value.The current check can pass malformed strings with the right length/hyphen count. Parse it and assert v4 to make the test strict.
✅ Stronger test assertion
if let Ok(Value::Text(uuid_str)) = result { - // Very basic UUID format check: 36 chars, 4 hyphens - assert_eq!(uuid_str.len(), 36); - assert_eq!(uuid_str.chars().filter(|&c| c == '-').count(), 4); + let parsed = uuid::Uuid::parse_str(uuid_str.as_ref()) + .expect("generate_uuid should return a parseable UUID"); + assert_eq!(parsed.get_version_num(), 4); } else { panic!("Expected text from generate_uuid"); }Reference: https://docs.rs/uuid/latest/uuid/struct.Uuid.html
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdlib/random.rs` around lines 200 - 204, Replace the weak length/hyphen checks in the UUID test branch that matches Value::Text(uuid_str) by attempting to parse uuid_str with uuid::Uuid::parse_str and assert the parse succeeds and that the parsed UUID's version equals uuid::Version::Random (v4); update the match arm that currently contains assert_eq!(uuid_str.len(), 36) and the hyphen count to instead call Uuid::parse_str(uuid_str).expect(...) and assert_eq!(parsed.get_version(), Some(uuid::Version::Random)) so malformed-but-shaped strings no longer pass (refer to the Value::Text(uuid_str) match arm in src/stdlib/random.rs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/stdlib/random.rs`:
- Around line 35-52: The native_random_between implementation must validate that
both bounds are finite before calling RNG.random_range to avoid panics; in the
function handling min and max (e.g., native_random_between / the block using
expect_number, min, max and RNG.with), check min.is_finite() and max.is_finite()
and return a RuntimeError with a clear message if either is not finite, then
proceed to call rng.random_range(min..=max) only when both are finite and min <=
max; also add regression tests exercising NaN, +inf and -inf for min and max to
assert the function returns an error instead of panicking.
---
Nitpick comments:
In `@src/stdlib/random.rs`:
- Around line 200-204: Replace the weak length/hyphen checks in the UUID test
branch that matches Value::Text(uuid_str) by attempting to parse uuid_str with
uuid::Uuid::parse_str and assert the parse succeeds and that the parsed UUID's
version equals uuid::Version::Random (v4); update the match arm that currently
contains assert_eq!(uuid_str.len(), 36) and the hyphen count to instead call
Uuid::parse_str(uuid_str).expect(...) and assert_eq!(parsed.get_version(),
Some(uuid::Version::Random)) so malformed-but-shaped strings no longer pass
(refer to the Value::Text(uuid_str) match arm in src/stdlib/random.rs).
|
@google-labs-jules re-check complete. One substantive blocker remains before merge:\n\n- [ ] Implement non-finite bounds validation in |
Formatting has been fixed, non-finite bounds validation in |
Implemented non-finite bounds validation in |
Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/stdlib/random.rs (1)
206-218: Strengthen UUID validity assertion beyond shape-only checks.Current checks (length/hyphens) can pass non-UUID strings. Parse the returned value as a UUID for stricter regression coverage.
♻️ Suggested test refinement
#[test] fn test_generate_uuid_format() { let result = native_generate_uuid(vec![]); assert!(result.is_ok()); if let Ok(Value::Text(uuid_str)) = result { - // Very basic UUID format check: 36 chars, 4 hyphens - assert_eq!(uuid_str.len(), 36); - assert_eq!(uuid_str.chars().filter(|&c| c == '-').count(), 4); + let parsed = uuid::Uuid::parse_str(uuid_str.as_ref()); + assert!(parsed.is_ok(), "Expected valid UUID string"); } else { panic!("Expected text from generate_uuid"); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stdlib/random.rs` around lines 206 - 218, The test test_generate_uuid_format currently only checks shape (length and hyphens); instead call native_generate_uuid(vec![]) as before, extract the string from the Ok(Value::Text(uuid_str)) branch, and validate by parsing it with the UUID parser (e.g., Uuid::parse_str(&uuid_str)) and assert that parsing returns Ok; update references in the test to use native_generate_uuid and Value::Text to locate the correct value and replace the length/hyphen assertions with the parse assertion to ensure the string is a valid UUID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/stdlib/random.rs`:
- Around line 206-218: The test test_generate_uuid_format currently only checks
shape (length and hyphens); instead call native_generate_uuid(vec![]) as before,
extract the string from the Ok(Value::Text(uuid_str)) branch, and validate by
parsing it with the UUID parser (e.g., Uuid::parse_str(&uuid_str)) and assert
that parsing returns Ok; update references in the test to use
native_generate_uuid and Value::Text to locate the correct value and replace the
length/hyphen assertions with the parse assertion to ensure the string is a
valid UUID.
Summary of Changes
src/stdlib/random.rscontained redundant argument validation logic (e.g., checking.len()and manually matching onValue::Number/Value::Listinside multiple functions likenative_random_between,native_random_int, etc.). This violated the DRY principle compared to the rest of the stdlib.native_random,native_random_between,native_random_int,native_random_boolean,native_random_from,native_random_seed, andnative_generate_uuidto use the standardizedcheck_arg_count,expect_number, andexpect_listhelper methods.Verification Checklist
cargo fmtexecuted and passed.cargo clippyreturned no warnings or errors.cargo testsuites passed (100% success rate).PR created automatically by Jules for task 4541016097777918063 started by @logbie
Summary by CodeRabbit
Bug Fixes
Refactor
Tests