refactor(api_caller): numeric-safety audit of pagination limit casts - #94
Merged
Conversation
…e lint policy Phase 1 of issue #1. Nearly every crate opened with `#![warn(clippy::restriction, clippy::pedantic)]` plus a long, drifted `#![allow(...)]` list — clippy's own docs say `restriction` isn't meant to be enabled wholesale, and three of the "commonly allowed" lints turned out to be default-on via `clippy::all` (match_ref_pats, needless_borrowed_reference, blanket_clippy_restriction_lints), meaning they were suppressing mainstream clippy output, not opting out of a restriction-tier lint. - Centralize the policy in root Cargo.toml's new [workspace.lints.clippy] table: pedantic stays fully on, each restriction-tier lint was reviewed individually and either enabled (ref_patterns, map_err_ignore, allow_attributes_without_reason) or explicitly allowed with a documented reason (implicit_return, question_mark_used, shadow_reuse/unrelated/same, single_call_fn, absolute_paths, mod_module_files, min_ident_chars, separated_literal_suffix, std_instead_of_core/alloc, arbitrary_source_item_ordering, doc_paragraphs_missing_punctuation). too_many_lines/match_ref_pats/needless_borrowed_reference are explicitly re-enabled since they were being suppressed despite not being restriction-tier. - Every hand-written crate (14 that had the old block, plus auth/oauth_flow and prototypes/workflow_engine which never opted into any lint policy) now just does `[lints] workspace = true`. - Removed service_loader's three `#[cfg(test)] mod test { #![allow(clippy::restriction, clippy::pedantic)] }` blocks, now meaningless since the crate root no longer blanket-enables either group. - Fixed the small, mechanical anti-patterns the old suppressions were hiding: `extern crate alloc; use alloc::sync::Arc` (etc.) in 13 ordinary std crates (zero crates in this workspace are #![no_std]) replaced with plain std paths; 4 `.map_err(|_| ...)` sites that discarded the original error now capture it in the resulting message. The ~86-site ref-pattern/match_ref_pats/needless_borrowed_reference rewrite this now surfaces as warnings, the too_many_lines fallout, and the api_caller numeric-safety audit (as_conversions/cast_possible_truncation, kept allowed here with a documented reason) are tracked as follow-ups in #85, #86, #87 rather than folded into this policy change. Fixes #1 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
Part of #85 (Phase 2 of #1). Converts every &Some(ref x)/match &value { &Variant(ref x) => ... } site in service_loader, execution_engine, and service_writer to plain match-ergonomics form (Some(x), Variant(x)) -- purely syntactic, binds the identical reference type as before. service_writer's clippy::ref_patterns/match_ref_pats/needless_borrowed_reference warnings are now fully zero; service_loader/execution_engine only retain pre-existing, out-of-scope warnings (too_many_lines, implicit_clone, etc.) tracked separately in #86. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
Part of #85 (Phase 2 of #1). Converts every &Some(ref x)/match &value { &Variant(ref x) => ... }/&mut Value::Object(ref mut x) site in api_caller, filtered_runner, and python_runner to plain match-ergonomics form -- purely syntactic, binds the identical (mutable or immutable) reference type as before. api_caller had two duplicated pagination-matching functions (find_results and the page-size calculator) each repeating the same 4-arm match, plus duplicated Number-coercion matches in two request-limit resolvers -- fixed all instances in each. filtered_runner and python_runner each had one &mut/ref mut site not caught by these clippy lints (which don't cover &mut patterns) but flagged by the same #85 inventory as the same anti-pattern. All three crates now have zero clippy::ref_patterns/match_ref_pats/ needless_borrowed_reference warnings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
Closes #85 (Phase 2 of #1). Converts every &Variant(ref x)-style site in apicli and apid to plain match-ergonomics form -- purely syntactic, binds the identical reference type as before, with one exception handled explicitly: apicli/template.rs's Integer(key) arm bound key: &i64 via ergonomics where the original &InputTokens::Integer(key) explicit-deref pattern bound an owned (Copy) i64 -- fixed by dereferencing at the use site instead of changing the match arm. apid/main.rs's provide_input handler matches Some((_, tx)) against a &mut-sourced get_mut() call; ergonomics binds tx: &mut Sender where the original ref tx bound &Sender, but Sender::send only needs &self so this is behaviorally inert. apicli's path.rs and stub.rs account for most of this PR's sites (30 of 33) and weren't flagged by clippy::ref_patterns/match_ref_pats/ needless_borrowed_reference at all -- those lints don't reliably fire on every &Some(Variant(_))-shaped match arm mixed with `ref`-bound arms in the same match. Found instead via the original manual site inventory from #1's scoping research; worth noting since a future clippy-only search of this codebase would miss them. apicli/engine.rs's merge() function keeps its outer `match &left`/ `match &right` (left/right are owned Schema values reused later in the same arms via `one_of.push(left)`/`vec![left, right]`, so the scrutinee itself can't drop its `&` without moving out from under later use) -- only the arm patterns lost their redundant `&`/`ref`. apicli/template.rs also fixes the impl ToString for PathKey match's ref-pattern shape only, not the ToString/Display issue itself (#13). Confirmed via a full `cargo clippy --workspace --all-features`: zero ref_patterns/match_ref_pats/needless_borrowed_reference warnings remain anywhere in the workspace, closing out issue #85. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
…es offenders Closes #86 (Phase 3 of #1). Phase 1 re-enabled clippy::too_many_lines workspace-wide; this was the entire remaining fallout -- exactly 2 functions over the 100-line threshold in the whole workspace. - execution_engine::Engine::run (123 lines): extracted each match arm (Swagger/Action/ApiWrapped/SimpleCode) into its own private dispatch_* helper method, mirroring the existing dispatch_code_runner/ resolve_data_connector/resolve_workflow methods already in this impl block. run() itself is now just the $input check, identifier parsing, the service/manifest lookup, a one-line-per-arm match delegating to the new helpers, and the final wrap_result call. dispatch_swagger and dispatch_action pick up clippy::too_many_arguments as a result -- allowed with the same #16 reasoning already used on dispatch_code_runner in this file, since each argument is a distinct pass-through dispatch input, not a case for a config struct. - service_loader's handle_schema (122 lines): the oneOf/anyOf/allOf composition handling repeated the identical fetch-map-collect shape three times. Extracted into a single resolve_schema_list helper (parameterized by field name), the read-side mirror of the exact dedup already done on the write side in #6 (service_writer's handle_composed_schema). Correction to Phase 1's PR descriptions: binary/apicli/src/engine.rs (issue #12's target) does NOT trip this lint -- no individual function there exceeds 100 lines, even though the file itself is oversized overall. too_many_lines is a per-function metric, so #86 and #12 don't actually overlap; #12 remains its own separate file-level refactor. Both changes are pure extract-method refactors with identical logic; existing tests (16 in execution_engine, 28 in service_loader) all pass unchanged. Confirmed via a full workspace clippy run: zero too_many_lines warnings remain anywhere in the repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018VzKriyNnMHmqQ6UxPhsv2
Closes #87 (Phase 4 of #1). Every `as`-cast site in api_caller turned out to be the same duplicated total_limit resolver (both APICaller::run_internal and AsyncAPICaller::run_internal): f64/i64/u64 -> i32 for options["limit"]. - i64/u64 -> i32 now use i32::try_from(...).ok(), falling back to DEFAULT_LIMIT on overflow instead of silently wrapping (e.g. a u64 limit of 2^32 previously wrapped to 0, which this code treats as "no cap enforced" - a real correctness bug, not just a lint nag). - f64 -> i32 keeps the `as` cast with a narrow, reasoned allow: float-to-int `as` casts saturate rather than wrap (defined behavior since Rust 1.45), so this one was already safe. - Extracted the duplicated resolver into resolve_total_limit(), which also fixed a too_many_lines regression the fix introduced and removed the duplication between the two run_internal methods. - Removed the crate-level #![allow(clippy::as_conversions, clippy::cast_possible_truncation)]; api_caller now lints like every other crate in the workspace. - Added regression tests confirming the old wraparound behavior for both the i64 and u64 arms (verified failing against the pre-fix casts).
…-too-many-lines # Conflicts: # usecases/execution_engine/src/lib.rs
…-1-phase4-numeric-safety
jhamill34
changed the base branch from
claude/issue-1-phase3-too-many-lines
to
main
August 27, 2026 00:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #87 (Phase 4 of #1). Stacked on #93 (Phase 3), which is stacked on #91/#90/#89/#88.
api_callerwas the only crate allowingclippy::as_conversions/clippy::cast_possible_truncation, pending this dedicated numeric-safety audit rather than a drive-by fix during the earlier lint-hygiene passes.Enumerating every cast site (with the blanket crate-level allow removed) found the entire fallout is one duplicated block:
APICaller::run_internalandAsyncAPICaller::run_internaleach independently resolveoptions["limit"](a JSON number that may arrive asf64/i64/u64) down to ani32pagination cap via 3as-casts apiece (6 sites total, all structurally identical).i64/u64→i32(4 sites): replacedn as i32withi32::try_from(n).ok(). This is a real correctness fix, not just a lint nag —astruncates via wraparound for int-to-int casts, so alimitof2^32previously wrapped to0, and this code treatstotal_limit == 0as "no cap enforced," silently discarding the caller's limit entirely instead of erroring or falling back sanely. It now falls back toDEFAULT_LIMITon overflow, same as any other unusable/non-numeric limit value.f64→i32(2 sites): kept theascast, with a narrow#[allow(clippy::cast_possible_truncation, reason = "...")]. Float-to-intascasts have been saturating (not wrapping) since Rust 1.45 — an out-of-range or NaN limit clamps toi32::MAX/i32::MIN/0, and a fractional limit truncates toward zero, both already the intended behavior for a pagination limit.resolve_total_limit(options: &serde_json::Value) -> i32free function, called from bothrun_internals. This both removes the literal duplication and avoids atoo_many_linesregression the inline fix would otherwise have introduced inAsyncAPICaller::run_internal(it was already at 100/100 lines).#[allow(clippy::as_conversions, clippy::cast_possible_truncation)]inlib.rs—api_callernow lints identically to every other crate in the workspace.No
as_conversionssites were found at all: it's restriction-tier and, per Phase 1's lint policy, deliberately not enabled anywhere in this workspace (clippy::restrictionisn't meant to be blanket-enabled), so the crate-level allow for it was already redundant before this PR removed it.Test plan
resolve_total_limit: in-rangef64/i64/u64pass through unchanged; absent/non-numeric falls back toDEFAULT_LIMIT; an out-of-rangei64(i32::MAX + 1) andu64(u32::MAX + 2) both fall back toDEFAULT_LIMITinstead of wrapping.n as i32casts (confirms they'd have caught this bug) and pass against the fix.cargo build -p api_caller --all-features— clean.cargo clippy -p api_caller --all-features— zerocast_possible_truncation/as_conversions/too_many_lineswarnings.cargo test -p api_caller --all-features— 10 tests, 0 failures.cargo clippy --workspace --all-features— zerocast_possible_truncation/as_conversions/too_many_lineswarnings anywhere in the repo.cargo build --workspace --all-features/cargo test --workspace --all-features— clean, zero failures.cargo fmt --all -- --check— clean.Generated by Claude Code