safety: Enforce 100% branch coverage #2944
Draft
Conversation
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
`safety_rx_hook` in safety.h only calls the model-specific `rx` hook for messages that are already whitelisted. Since the whitelist configurations in subaru.h, subaru_preglobal.h, mazda.h, psa.h, toyota.h, volkswagen_mqb.h, body.h, chrysler.h, and ford.h already enforce the correct bus, the additional bus checks are redundant and unreachable (dead code). Removing them cleans up the logic and improves branch coverage.
This brings volkswagen_common.h coverage to 100% and improves psa.h coverage.
`controls_allowed` is always true on a body (for now). Until that changes, we should avoid a redundant check of that variable.
Remove unreachable logic branches to improve code clarity and coverage: 1. Remove `hyundai_longitudinal` check in hyundai_common_cruise_state_check. This function is only called when an SCC cruise state message is received. In longitudinal mode, these messages are not whitelisted (openpilot replaces them), so this function is never entered. 2. Remove unused fallback logic in hyundai_common_canfd_compute_checksum. All checksummed messages in the hyundai_canfd configuration are verified to be either 24 or 32 bytes long, making the fallback case for other lengths unreachable.
- Remove redundant explicit addr check in psa_tx_hook, relying on the safety whitelist. - Flatten logic for better readability and improved coverage.
- Remove redundant `0x100` address check (implicit on Bus 2) to fix coverage miss.
- Remove redundant address checks in rivian_get_counter and rivian_get_checksum as these functions are securely guarded by the safety whitelist. - Simplify rivian_compute_checksum and rivian_get_quality_flag_valid to remove unreachable branches.
- Remove the check for `MSG_ACC_05` inside the `bus == 2` block. - This check is redundant because `MSG_ACC_05` is the only whitelisted message for Bus 2 in volkswagen_mlb_init. - Removing this dead code improves branch coverage validation.
- Remove redundant `msg->bus == 0` check in rx_hook. - Refactor `get_counter` to eliminate unreachable branches.
- Remove redundant address check in `get_quality_flag_valid` (implicit via whitelist in TOYOTA_COMMON_RX_CHECKS).
- Remove redundant bus checks for msg 0x184 (handled by whitelist) - Simplify init logic to remove dead code path - Remove redundant cruise state check in TX hook
17 tasks
This was referenced Dec 19, 2025
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.
Description
This PR aims to close and complete #2557 (which shows as locked in the openpilot bounties, but I am not able to find a relevant, open PR). It is still a work in progress. So far, coverage for all files in the modes/ directory has been brought up to at least 90%, and the only remaining modes that need coverage updated are ford, honda, hyundai, hyundai_canfd, subaru, and tesla. Coverage inside safety/tests is currently
97%, I plan to bring coverage up to the full 100%, but I am opening the PR now just to get some eyes on it in case I may be going about it in the wrong way. It was also a bit unclear to me if the bounty is for only the files in modes/ or for the entire repo to be covered. I am working under the assumption it was the entire repo since that is what the bounty seemed to imply to me.Related PRs:
else#2948Verification
./test.sh --reporton master:./test.sh --reporton this branch: