Skip to content

Conversation

@jerrysxie
Copy link
Contributor

@jerrysxie jerrysxie commented Dec 18, 2025

This pull request introduces several improvements to code safety, error handling, and CI workflows, with a particular focus on enforcing linting standards, making byte conversions more robust, and updating how Clippy is run in CI. The most significant changes are grouped below:

Linting and CI workflow improvements:

  • Enforced strict Rust and Clippy lint rules in Cargo.toml, forbidding unsafe code and denying a wide range of Clippy lints to improve code quality.
  • Removed the separate Clippy job from .github/workflows/check.yml and instead integrated Clippy checks directly into the main test matrix, ensuring Clippy runs with all feature combinations. [1] [2]

Byte conversion and error handling enhancements:

  • Refactored several From trait implementations for converting between protocol data structures and byte arrays to use TryFrom instead, adding comprehensive error handling for all byte conversions. This change affects types like GetFwVersionResponse, FwUpdateOffer, and FwUpdateContentCommand in src/protocol_definitions.rs. [1] [2] [3] [4] [5]
  • Updated usages and tests to handle the new fallible conversions, replacing .into() with .try_into().unwrap() where appropriate.

Code safety and clarity:

  • Improved safety and clarity in buffer handling by using get_mut and explicit error propagation instead of unchecked indexing in src/host.rs and src/protocol_definitions.rs. [1] [2] [3]

These changes collectively make the codebase more robust by enforcing stricter linting, preventing unsafe code, and ensuring that all byte-level conversions are safe and error-aware.

@jerrysxie jerrysxie self-assigned this Dec 18, 2025
@jerrysxie jerrysxie added the enhancement New feature or request label Dec 18, 2025
@jerrysxie jerrysxie marked this pull request as ready for review December 18, 2025 00:49
@jerrysxie jerrysxie requested a review from a team as a code owner December 18, 2025 00:49
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR eliminates panic code paths in protocol serialization/deserialization functions by replacing unwrap() calls with proper error handling and converting a From trait to TryFrom to enable error propagation.

  • Converts From<&GetFwVersionResponse> for [u8; 60] to TryFrom with error handling for out-of-bounds access
  • Replaces multiple try_into().unwrap() calls with try_into().map_err() for proper error propagation across serialization functions
  • Adds defensive bounds checking to prevent panics in slice operations

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
src/protocol_definitions.rs Converts GetFwVersionResponse serialization from From to TryFrom, adds bounds checking to array/slice accesses, replaces unwrap() with proper error handling in multiple TryFrom implementations
src/host.rs Adds defensive bounds check for chunk slice operation and explicit type annotation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

* Add lints to Cargo.toml to centralize in one location

* Add additional lints to check for panic paths

* Update workflow to run clippy with -Dwarnings and via cargo hack
Copy link

@dymk dymk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, but the code could be made a little more readable with some small helpers that do the ok_or and map_err conversions internally

@jerrysxie jerrysxie enabled auto-merge (squash) December 19, 2025 19:32
@jerrysxie jerrysxie merged commit e0d7760 into OpenDevicePartnership:main Dec 19, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants