Lightweight Assurance 1: Define and enforce the Rust assurance baseline#2525
Lightweight Assurance 1: Define and enforce the Rust assurance baseline#2525yuvalkom-M wants to merge 35 commits intomainfrom
Conversation
…eta-11566/main
…eta-11566/main
…eta-11566/main
…eta-11566/main
There was a problem hiding this comment.
Pull request overview
This PR aims to define and start enforcing a stricter Rust “assurance baseline” across sequent-core and its downstream consumers (windmill, velvet) by tightening linting/documentation requirements and refactoring APIs (more borrowing, fewer clones, and removing unnecessary async in SQLite helpers).
Changes:
- Add strict Rust/clippy/rustdoc lint configuration for
packages/sequent-coreand adjust code/docs to move toward compliance. - Refactor multiple APIs to prefer borrowing/slices and make several SQLite helper functions synchronous; update call sites in
windmill/velvet. - Update Nix flake toolchain inputs (nightly date, OpenSSL/pkg-config) for building/testing.
Reviewed changes
Copilot reviewed 112 out of 118 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/yarn.lock | Updates local sequent-core tarball resolved hashes. |
| packages/windmill/src/tasks/send_template.rs | Updates get_auth_url call to pass &AuthAction. |
| packages/windmill/src/tasks/post_tally.rs | Updates from_pdf_options call to pass by reference. |
| packages/windmill/src/tasks/manage_election_init_report.rs | Adjusts allow-init handling for new payload shape. |
| packages/windmill/src/services/election_dates.rs | Passes scheduled events as slice to helper. |
| packages/windmill/src/services/ceremonies/velvet_tally.rs | Removes .await for now-sync SQLite creation helpers. |
| packages/windmill/src/services/ceremonies/result_documents.rs | Removes .await for now-sync SQLite update helpers. |
| packages/windmill/src/services/ballot_styles/ballot_style.rs | Uses imported create_ballot_style + borrows/slices. |
| packages/velvet/src/pipes/generate_db/generate_db.rs | Removes .await for now-sync SQLite helpers. |
| packages/velvet/src/pipes/decode_ballots/decode_mcballots.rs | Avoids cloning decoded ballots (borrow instead). |
| packages/velvet/src/cli/test_all.rs | Updates tests for new slice/ref voting-screen APIs. |
| packages/sequent-core/tests/mod.rs | Adds integration test module rustdoc. |
| packages/sequent-core/src/wasm/mod.rs | Adds clippy allow for module inception under wasmtest. |
| packages/sequent-core/src/wasm/areas.rs | Tightens imports + improves error messages/docs. |
| packages/sequent-core/src/util/voting_screen.rs | Refactors to borrow inputs + adds docs/must_use; introduces checked conversions. |
| packages/sequent-core/src/util/version.rs | Adds docs + minor parsing/formatting cleanups. |
| packages/sequent-core/src/util/temp_path.rs | Adds docs + changes byte inputs to slices. |
| packages/sequent-core/src/util/retry.rs | Adds checked arithmetic + docs for retry helper. |
| packages/sequent-core/src/util/path.rs | Makes filesystem helpers more iterator-based; adds must_use. |
| packages/sequent-core/src/util/normalize_vote.rs | Switches APIs to slices; simplifies option filtering. |
| packages/sequent-core/src/util/mod.rs | Adds module-level missing-docs allowance. |
| packages/sequent-core/src/util/mime.rs | Adds must_use + minor iteration style changes. |
| packages/sequent-core/src/util/integrity_check.rs | Adds docs + avoids extra string allocation. |
| packages/sequent-core/src/util/init_log.rs | Improves error messages on env/log initialization. |
| packages/sequent-core/src/util/float.rs | Converts comments to rustdoc + formatting tweak. |
| packages/sequent-core/src/util/external_config.rs | Adds docs for config loader. |
| packages/sequent-core/src/util/date.rs | Adds must_use + checked time arithmetic. |
| packages/sequent-core/src/util/date_time.rs | Adds docs + safer timezone offset conversions. |
| packages/sequent-core/src/util/convert_vec.rs | Generalizes hash map conversion + adds docs/must_use. |
| packages/sequent-core/src/util/console_log.rs | Exports console_log! macro for wasm/native. |
| packages/sequent-core/src/util/aws.rs | Adds error docs to AWS config helpers. |
| packages/sequent-core/src/types/to_map.rs | Adds docs; trims unused imports. |
| packages/sequent-core/src/types/templates.rs | Adjusts PDF options API to borrow + doc improvements. |
| packages/sequent-core/src/types/tally_sheets.rs | Adds docs + derives Default where appropriate. |
| packages/sequent-core/src/types/scheduled_event.rs | Changes allow-init payload to bool + refactors scheduled-date preparation API. |
| packages/sequent-core/src/types/results.rs | Adds extensive rustdoc across results types. |
| packages/sequent-core/src/types/permissions.rs | Adds missing-docs allows on large enums. |
| packages/sequent-core/src/types/mod.rs | Adds module-level rustdoc for exported type modules. |
| packages/sequent-core/src/types/keycloak.rs | Adds rustdoc for constants and Keycloak DTOs. |
| packages/sequent-core/src/types/hasura/mod.rs | Adds module rustdoc. |
| packages/sequent-core/src/types/hasura/extra.rs | Adds rustdoc + simplifies imports and enum attributes. |
| packages/sequent-core/src/types/hasura/core.rs | Adds rustdoc + small API cleanups (policy, is_default). |
| packages/sequent-core/src/types/error.rs | Adds rustdoc to unified error/result type. |
| packages/sequent-core/src/types/date_time.rs | Adds docs + derives defaults via attributes. |
| packages/sequent-core/src/temp_path.rs | Refactors temp path helpers to slices + docs. |
| packages/sequent-core/src/sqlite/utils.rs | Adds must_use + minor map simplification. |
| packages/sequent-core/src/sqlite/tally_session_resolution.rs | Makes helper synchronous + adds docs. |
| packages/sequent-core/src/sqlite/results_event.rs | Makes helper synchronous + adds docs + formatting improvements. |
| packages/sequent-core/src/sqlite/results_election.rs | Makes helper synchronous + adds docs + formatting improvements. |
| packages/sequent-core/src/sqlite/results_election_area.rs | Makes helper synchronous + adds docs + clippy allow. |
| packages/sequent-core/src/sqlite/results_contest.rs | Makes helper synchronous + adds docs + formatting improvements. |
| packages/sequent-core/src/sqlite/results_contest_candidate.rs | Makes helper synchronous + adds docs. |
| packages/sequent-core/src/sqlite/results_area_contest.rs | Makes helper synchronous + adds docs + clippy allow. |
| packages/sequent-core/src/sqlite/results_area_contest_candidate.rs | Makes helper synchronous + adds docs. |
| packages/sequent-core/src/sqlite/election.rs | Makes helper synchronous + avoids extra clones in to_string. |
| packages/sequent-core/src/sqlite/election_event.rs | Makes helper synchronous + adds docs. |
| packages/sequent-core/src/sqlite/contests.rs | Makes helper synchronous + avoids extra clones in to_string. |
| packages/sequent-core/src/sqlite/candidate.rs | Makes helper synchronous + refactors CSV import helper. |
| packages/sequent-core/src/sqlite/area.rs | Makes helper synchronous + avoids extra clones in to_string. |
| packages/sequent-core/src/sqlite/area_contest.rs | Makes helper synchronous + adds docs. |
| packages/sequent-core/src/signatures/shell.rs | Adds docs + improves error formatting. |
| packages/sequent-core/src/signatures/ecies_encrypt.rs | Refactors formatting + adds docs; minor string ops tweaks. |
| packages/sequent-core/src/services/uuid_validation.rs | Improves error formatting + adds docs. |
| packages/sequent-core/src/services/translations.rs | Refactors presentation parsing and i18n helper signatures. |
| packages/sequent-core/src/services/reports.rs | Adds docs + safer arithmetic in helpers + minor refactors. |
| packages/sequent-core/src/services/replace_uuids.rs | Refactors API to slice + adds docs; changes regex init style. |
| packages/sequent-core/src/services/probe.rs | Refactors probe handler types + adds docs. |
| packages/sequent-core/src/services/mod.rs | Adds module-level missing-docs allowance. |
| packages/sequent-core/src/services/keycloak/role.rs | Tightens imports + safer pagination slicing + docs. |
| packages/sequent-core/src/services/keycloak/permission.rs | Tightens imports + safer slicing + logging improvements. |
| packages/sequent-core/src/services/keycloak/mod.rs | Adds module rustdoc and submodule docs. |
| packages/sequent-core/src/services/keycloak/admin_client.rs | Adds docs + refactors config creation + safer token expiry math. |
| packages/sequent-core/src/services/jwt.rs | Adds docs + improves error formatting; minor iterator simplifications. |
| packages/sequent-core/src/services/generate_urls.rs | Changes get_auth_url to borrow AuthAction + adds docs. |
| packages/sequent-core/src/services/error_checker.rs | Adds checked arithmetic for per-type counts + docs. |
| packages/sequent-core/src/services/date.rs | Adds docs + safer timestamp parsing branches. |
| packages/sequent-core/src/services/connection.rs | Tightens imports + refactors header parsing; adds docs. |
| packages/sequent-core/src/services/authorization.rs | Adds docs + minor return/message cleanups. |
| packages/sequent-core/src/services/area_tree.rs | Refactors area tree APIs to slices/iterators + adds docs/iterator impl. |
| packages/sequent-core/src/serialization/mod.rs | Adds module-level missing-docs allowance. |
| packages/sequent-core/src/serialization/deserialize_with_path.rs | Adds docs + minor cleanup. |
| packages/sequent-core/src/serialization/base64.rs | Adds docs + simplifies decode path. |
| packages/sequent-core/src/plugins_wit/mod.rs | Adds module-level missing-docs allowance. |
| packages/sequent-core/src/plugins_wit/lib.rs | Adds rustdoc and allows missing docs inside generated bindings. |
| packages/sequent-core/src/plaintext.rs | Refactors imports + adds docs; changes decoded ballot mapping APIs to borrow. |
| packages/sequent-core/src/mixed_radix.rs | Changes APIs to slices + adds docs; introduces checked arithmetic. |
| packages/sequent-core/src/main.rs | Adds crate-level docs; changes main signature to const. |
| packages/sequent-core/src/lib.rs | Adds crate docs + enables crate-level allow(missing_docs). |
| packages/sequent-core/src/interpret_plaintext.rs | Refactors counting-alg match arms + adds docs + checked arithmetic. |
| packages/sequent-core/src/fixtures/mod.rs | Adds module-level missing-docs allowance. |
| packages/sequent-core/src/fixtures/encrypt.rs | Adds docs for fixtures. |
| packages/sequent-core/src/ballot_style.rs | Refactors create_ballot_style to borrow inputs and reduce clones + docs. |
| packages/sequent-core/src/ballot_codec/vec.rs | Changes APIs to slices + refactors copying logic + docs. |
| packages/sequent-core/src/ballot_codec/plaintext_contest.rs | Tightens imports + adds docs for codec trait. |
| packages/sequent-core/src/ballot_codec/mod.rs | Adds module-level missing-docs allowance. |
| packages/sequent-core/src/ballot_codec/checker.rs | Adds docs + minor refactors to avoid unnecessary returns/clones. |
| packages/sequent-core/src/ballot_codec/character_map.rs | Adds docs + tightens mapping logic/errors. |
| packages/sequent-core/src/ballot_codec/bigint.rs | Tightens imports + adds docs; improves robustness in helpers. |
| packages/sequent-core/src/ballot_codec/bases.rs | Adds docs + checked arithmetic for cumulative/max vote bases. |
| packages/sequent-core/flake.nix | Updates toolchain date and adds OpenSSL/pkg-config to build/shell. |
| packages/sequent-core/Cargo.toml | Adds strict lint baseline configuration. |
Comments suppressed due to low confidence (1)
packages/sequent-core/src/services/area_tree.rs:301
get_contest_matchesnow ignores itscontest_idsinput entirely (renamed to_contest_ids) and returns matches for every contest id present in the node data. This is a behavioral change that likely breaks callers expecting filtering by the provided set.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| unwrap_used = "deny" | ||
| panic = "deny" | ||
| shadow_unrelated = "deny" | ||
| print_stdout = "deny" |
There was a problem hiding this comment.
The crate enables very strict clippy lints (e.g., unwrap_used = "deny", panic = "deny", print_stdout = "deny"), but several changes in this PR introduce expect(...)/unwrap(...) and println! in sequent-core code (and tests). With these lint settings, clippy will fail unless those call sites are removed or locally #[allow(...)]-annotated with a clear rationale.
| print_stdout = "deny" | |
| print_stdout = "warn" |
| Err(err) if attempts < max_retries => { | ||
| // Failure, but we can try again after a backoff delay | ||
| attempts += 1; | ||
| attempts = attempts.checked_add(1).expect("Attempt overflow"); | ||
| info!( | ||
| "Failed attempt {attempts}, sleeping {:?} ms, error: {:?}", | ||
| backoff, err | ||
| ); | ||
| sleep(backoff).await; | ||
| // Exponential backoff: double the delay | ||
| backoff *= 2; | ||
| backoff = backoff.checked_mul(2).expect("Backoff overflow"); |
There was a problem hiding this comment.
retry_with_exponential_backoff uses expect(...) on checked_add/checked_mul, which will both (1) violate clippy::unwrap_used/clippy::panic (now set to deny) and (2) panic in production on overflow. Consider returning an error on overflow (as the docstring claims) or using a saturating strategy, rather than panicking.
| let choices_selected = decoded_contest | ||
| .choices | ||
| .iter() | ||
| .filter(|choice| choice.selected == 0) | ||
| .count(); | ||
|
|
||
| let choices_selected_i64 = i64::try_from(choices_selected).expect("error convert choices_selected to i64"); | ||
| let invalid_errors: &Vec<InvalidPlaintextError> = |
There was a problem hiding this comment.
i64::try_from(choices_selected).expect(...) will violate clippy::unwrap_used/clippy::panic (now deny) and can panic. This can be avoided entirely by comparing without a fallible cast (e.g., convert max to usize with try_from and handle negative/overflow cases) or by handling the Err branch explicitly.
| console_log!("choices_selected={choices_selected:?}, explicit_invalid={explicit_invalid:?}"); | ||
|
|
||
| let choices_selected_i64 = i64::try_from(choices_selected).expect("error convert choices_selected to i64"); | ||
| // Show Alert dialog if: |
There was a problem hiding this comment.
Same issue as above: i64::try_from(choices_selected).expect(...) will be rejected by the new clippy settings and may panic. Please refactor to avoid expect/panics here.
| .get_one("authorization") | ||
| .expect("Missing authorization header") | ||
| .to_string(), | ||
| }) |
There was a problem hiding this comment.
FromRequest for AuthHeaders uses .expect(...) after a headers.contains(...) check. This still violates clippy::unwrap_used = "deny". Prefer matching on get_one(...) and returning Outcome::Error if missing (even if it “shouldn’t happen”).
| println!("fixture: {}", fixture.title); | ||
|
|
There was a problem hiding this comment.
This test prints to stdout (println!), but clippy::print_stdout is now set to deny for this crate. Please remove the debug print (or gate it behind an allow that’s scoped to tests) so clippy passes.
| println!("fixture: {}", fixture.title); |
| // | ||
| // SPDX-License-Identifier: AGPL-3.0-only | ||
| //! Shared core types and services for Step crates. | ||
| #![allow(missing_docs)] |
There was a problem hiding this comment.
The crate sets missing_docs = "deny" in Cargo.toml, but #![allow(missing_docs)] at the crate root disables that enforcement for the entire crate. If the goal is to enforce a documentation baseline, consider removing this crate-level allow and instead adding targeted #[allow(missing_docs)] only where strictly necessary (or documenting remaining items).
| #![allow(missing_docs)] |
| encoded = encoded | ||
| .checked_add( | ||
| &acc_base | ||
| .checked_mul(&value_bigint) | ||
| .expect("Multiplication overflow when encoding value"), | ||
| ) |
There was a problem hiding this comment.
encode uses .expect(...) on checked_mul, which will violate clippy::unwrap_used/clippy::panic (deny) and panics on overflow. Since the function already returns Result<_, String>, please propagate an error instead of panicking on arithmetic overflow.
| let uuid_regex = | ||
| Regex::new(r"[0-9a-fA-F]{8}(-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12}") | ||
| .unwrap(); | ||
| .expect("Invalid UUID regex"); | ||
| let fixed_uuids_from_config = env::var("ELECTION_EVENT_FIXED_UUIDS") |
There was a problem hiding this comment.
replace_uuids uses .expect(...) for regex compilation and capture extraction, which conflicts with clippy::unwrap_used/clippy::panic set to deny. Either handle these failures without panicking (e.g., return the input unchanged / empty map on regex init failure) or add narrowly-scoped #[allow(...)] annotations with justification.
| #[derive(Serialize, Deserialize, Debug, Clone)] | ||
| pub struct ManageAllowInitPayload { | ||
| pub election_id: Option<String>, | ||
| #[serde(default = "default_allow_init")] | ||
| pub allow_init: Option<bool>, | ||
| pub allow_init: bool, | ||
| } |
There was a problem hiding this comment.
Changing ManageAllowInitPayload.allow_init from Option<bool> to bool is a breaking deserialization change: previously payloads with allow_init: null could deserialize (as None), but now they will fail deserialization. If older scheduled events can contain null, consider keeping Option<bool> (with #[serde(default)]) or adding a custom deserializer to treat null as the default.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 112 out of 118 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Increments an integer by 1 and writes it to output. | ||
| /// | ||
| /// # Errors | ||
| /// Returns an error if the parameter is missing, not a valid integer, or if writing to output fails. | ||
| /// | ||
| /// # Panics | ||
| /// Panics if the increment would overflow. | ||
| pub fn inc( |
There was a problem hiding this comment.
The docs claim this helper can panic on overflow, but the implementation uses checked_add and returns an error instead. Update the rustdoc to remove # Panics (or change the code to actually panic, but returning an error is preferable).
| /// Increments an integer by 2 and writes it to output. | ||
| /// | ||
| /// # Errors | ||
| /// Returns an error if the parameter is missing, not a valid integer, or if writing to output fails. | ||
| /// | ||
| /// # Panics | ||
| /// Panics if the increment would overflow. | ||
| pub fn inc2( |
There was a problem hiding this comment.
Same as inc: the docs claim a panic on overflow, but the function returns an error on overflow via checked_add. Update the rustdoc accordingly.
| let command = format!( | ||
| "java -jar {} encrypt {} {}", | ||
| ECIES_TOOL_PATH, temp_pem_file_string, password | ||
| "java -jar {ECIES_TOOL_PATH} encrypt {temp_pem_file_string} {password}" | ||
| ); | ||
| info!("command: '{}'", command); | ||
| info!("command: '{command}'"); | ||
|
|
||
| let result = run_shell_command(&command)?.replace("\n", ""); | ||
| let result = run_shell_command(&command)?.replace('\n', ""); | ||
|
|
||
| info!("ecies_encrypt_string: '{}'", result); | ||
| info!("ecies_encrypt_string: '{result}'"); |
There was a problem hiding this comment.
The constructed command includes the encryption password and is logged at info level, which will leak secrets. Avoid logging the full command (or redact the password) and consider passing secrets via stdin/env rather than command-line args to prevent exposure via process listings.
| Err(( | ||
| Status::Unauthorized, | ||
| format!("Unathorized: {perms_str:?} not in {permissions_set:?}"), | ||
| )) |
There was a problem hiding this comment.
Spelling in error message: "Unathorized" should be "Unauthorized".
| fn main() { | ||
| //! Main entry point for the sequent-core crate. | ||
|
|
||
| const fn main() { |
There was a problem hiding this comment.
main cannot be declared as const fn; Rust requires the entry point to be fn main(). Revert this to a normal fn main() and, if needed to satisfy clippy::missing_const_for_fn, add a targeted #[allow(clippy::missing_const_for_fn)] instead.
| const fn main() { | |
| fn main() { |
| //! Shared core types and services for Step crates. | ||
| #![allow(missing_docs)] |
There was a problem hiding this comment.
Crate-level #![allow(missing_docs)] effectively disables the new missing_docs = deny assurance lint for the entire crate. If the goal is to enforce the baseline, scope allow(missing_docs) to generated/legacy modules only (or remove it and document the remaining public API).
| /// Writes data into a named temp file. The temp file will have the | ||
| ///specificed prefix and suffix. | ||
| /// |
There was a problem hiding this comment.
Doc comment typo/formatting: "specificed" should be "specified" and there should be a space after /// for rustdoc formatting consistency.
There was a problem hiding this comment.
- lib.rs:5 adds a crate-wide allow for missing_docs while Cargo.toml:167 and Cargo.toml:175 introduce deny rules for missing docs. Remove the crate-wide allow, and instead of using missing_docs, add the docs (AI can help with it)
- Use cargo clippy (for crates where it should pass, in this case just sequent core for now) in the vs code task for linting
- Add documentation on how to use cargo clippy and what is the linting config
- Add a GH action for clippy linting (for specific crates).
- Cargo clippy should be used with
--all-features. - Modify the GH action for docs so that cargo docs (for all crates) are generated and pushed (copied), so that the preview and the "main" branch include the documentation generated by cargo doc.
- Related to the previous comment, I pushed a small commit to improve the generated cargo doc (so that all features are included).
|
parent issue: https://github.com/sequentech/meta/issues/11566