chore: sync from monorepo @35cafa4#27
Conversation
📝 WalkthroughWalkthroughThis PR performs workspace-wide cleanup and refactoring: removes OSV-Scanner vulnerability ignore configurations and gitignore entries, fixes syntax errors in error enums, refactors instruction handlers to use single-line require macros, adds an explicit ChangesSecurity & Workspace Configuration Cleanup
resq-airspace Refactoring & Fixes
resq-delivery Refactoring & Fixes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several regressions and maintenance issues. Key concerns include the removal of the .gitignore and osv-scanner.toml files, and the downgrade of the rand crate which re-exposes a known security vulnerability (RUSTSEC-2026-0097). Furthermore, removing Clippy allow attributes for functions with many arguments will likely break CI builds, and the change from idiomatic range checks to explicit comparisons is a step backward in code quality.
I am having trouble creating individual review comments. Click here to see my feedback.
Cargo.lock (155)
Downgrading rand from 0.8.6 to 0.8.5 (and 0.9.3 to 0.9.2 elsewhere in this file) appears to be a security regression. The osv-scanner.toml file being removed in this PR explicitly identifies 0.8.6 and 0.9.3 as the minimum fixed versions for an unsoundness issue (RUSTSEC-2026-0097). Reverting to older versions may re-expose the project to this vulnerability.
.gitignore (1-6)
The removal of the .gitignore file is a regression in repository maintenance. Without this file, build artifacts (target/), dependency directories (node_modules/), and OS-specific files (.DS_Store) will be tracked by Git, leading to cluttered commits and potential inclusion of large binaries in the repository history.
osv-scanner.toml (1-58)
The removal of osv-scanner.toml deletes the documented security context and rationales for ignoring specific vulnerabilities. This reduces the visibility of the project's security posture and may cause automated security scans to fail or become unmonitored without clear justification for existing issues.
resq-airspace/src/instructions/initialize_property.rs (55)
The handler function takes 9 arguments, which exceeds the default Clippy limit of 7. Removing this allow attribute, especially alongside the removal of the crate-level allow in lib.rs, will cause Clippy linting to fail in CI.
#[allow(clippy::too_many_arguments)]
pub fn handler(
resq-airspace/src/lib.rs (1)
Removing crate-level allow attributes for clippy::too_many_arguments and clippy::diverging_sub_expression is likely to break the build during linting. The project contains functions with more than 7 arguments (e.g., initialize_property::handler) and uses unreachable!() (e.g., in record_crossing::handler), both of which will trigger these lints.
#![allow(
unexpected_cfgs,
clippy::too_many_arguments,
clippy::diverging_sub_expression
)]resq-airspace/src/instructions/initialize_property.rs (69)
The change from (1..=8).contains(&vertex_count) to explicit comparison is less idiomatic in Rust. Using range methods is generally preferred for readability and clarity.
(1..=8).contains(&vertex_count),
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
resq-delivery/tests/integration.rs (1)
266-330: ⚡ Quick winAdd exact-boundary cases for the inclusive GPS checks.
These tests only exercise out-of-range values. Since the handler change is specifically about inclusive bounds, add cases for
±900_000_000latitude and±1_800_000_000longitude so a future>/<regression gets caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resq-delivery/tests/integration.rs` around lines 266 - 330, Add exact-boundary tests for the inclusive GPS checks: create additional test cases (or extend test_rejects_latitude_out_of_range and test_rejects_longitude_out_of_range) that call create_record_ix with latitude = 900_000_000 and latitude = -900_000_000, and longitude = 1_800_000_000 and longitude = -1_800_000_000 (use the same delivery_pda/delivered_at, funding transfer, and tx signing flow as the existing tests), then call banks_client.process_transaction(tx).await and assert the transaction succeeds (i.e., no unwrap_err) to ensure the handler accepts the inclusive bounds rather than rejecting them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resq-airspace/src/instructions/record_crossing.rs`:
- Line 126: The permit validity check currently uses clock.unix_timestamp
(require!(permit.is_active(clock.unix_timestamp), ...)) which validates at
processing time and can reject delayed submissions; change the check to use the
recorded crossing time (crossed_at) instead (i.e., call
permit.is_active(crossed_at)) so permits are validated at crossing time; update
the require! invocation accordingly and ensure crossed_at is passed/available in
the same scope where permit.is_active is called.
In `@resq-delivery/tests/integration.rs`:
- Around line 382-384: The current assertion is too broad; tighten it to verify
the specific "account already in use" error by checking the error debug string
for the concrete variant name instead of the generic "InstructionError"
fallback. Replace the assert that checks for "InstructionError" with one that
asserts format!("{:?}", err) contains either "already in use" or the specific
variant string "AccountInUse" (or "AccountAlreadyInitialized" if that's the
variant used in this codebase) so the test proves the duplicate-delivery path
rather than any InstructionError.
---
Nitpick comments:
In `@resq-delivery/tests/integration.rs`:
- Around line 266-330: Add exact-boundary tests for the inclusive GPS checks:
create additional test cases (or extend test_rejects_latitude_out_of_range and
test_rejects_longitude_out_of_range) that call create_record_ix with latitude =
900_000_000 and latitude = -900_000_000, and longitude = 1_800_000_000 and
longitude = -1_800_000_000 (use the same delivery_pda/delivered_at, funding
transfer, and tx signing flow as the existing tests), then call
banks_client.process_transaction(tx).await and assert the transaction succeeds
(i.e., no unwrap_err) to ensure the handler accepts the inclusive bounds rather
than rejecting them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ecab411-7c28-4cac-9838-4dcc9c0349af
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.gitignoreosv-scanner.tomlresq-airspace/src/error.rsresq-airspace/src/instructions/grant_permit.rsresq-airspace/src/instructions/initialize_property.rsresq-airspace/src/instructions/record_crossing.rsresq-airspace/src/instructions/update_policy.rsresq-airspace/src/instructions/update_treasury.rsresq-airspace/src/lib.rsresq-airspace/src/state/airspace_account.rsresq-airspace/src/state/mod.rsresq-airspace/src/state/permit.rsresq-airspace/tests/host_init_regression.rsresq-airspace/tests/integration.rsresq-delivery/src/error.rsresq-delivery/src/instructions/mod.rsresq-delivery/src/instructions/record_delivery.rsresq-delivery/src/lib.rsresq-delivery/src/state/delivery_record.rsresq-delivery/src/state/mod.rsresq-delivery/tests/integration.rsvendor/solana-program-test/osv-scanner.toml
💤 Files with no reviewable changes (3)
- vendor/solana-program-test/osv-scanner.toml
- osv-scanner.toml
- .gitignore
| permit.is_active(clock.unix_timestamp), | ||
| AirspaceError::PermitExpired | ||
| ); | ||
| require!(permit.is_active(clock.unix_timestamp), AirspaceError::PermitExpired); |
There was a problem hiding this comment.
Validate permit at crossing time, not processing time.
Line 126 checks permit activity against clock.unix_timestamp, but Lines 88-93 explicitly allow delayed submissions using crossed_at. A permit that was valid at crossing time can be wrongly rejected if it expires before processing.
Suggested fix
- require!(permit.is_active(clock.unix_timestamp), AirspaceError::PermitExpired);
+ require!(permit.is_active(crossed_at), AirspaceError::PermitExpired);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require!(permit.is_active(clock.unix_timestamp), AirspaceError::PermitExpired); | |
| require!(permit.is_active(crossed_at), AirspaceError::PermitExpired); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resq-airspace/src/instructions/record_crossing.rs` at line 126, The permit
validity check currently uses clock.unix_timestamp
(require!(permit.is_active(clock.unix_timestamp), ...)) which validates at
processing time and can reject delayed submissions; change the check to use the
recorded crossing time (crossed_at) instead (i.e., call
permit.is_active(crossed_at)) so permits are validated at crossing time; update
the require! invocation accordingly and ensure crossed_at is passed/available in
the same scope where permit.is_active is called.
| let err = banks_client.process_transaction(tx2).await.unwrap_err(); | ||
| // Anchor initialization failure | ||
| assert!( | ||
| format!("{:?}", err).contains("already in use") | ||
| || format!("{:?}", err).contains("InstructionError") | ||
| ); | ||
| assert!(format!("{:?}", err).contains("already in use") || format!("{:?}", err).contains("InstructionError")); |
There was a problem hiding this comment.
Tighten the duplicate-delivery assertion.
The InstructionError fallback is broad enough to hide unrelated failures, so this test can pass without proving the account-already-in-use path.
Suggested tightening
- assert!(format!("{:?}", err).contains("already in use") || format!("{:?}", err).contains("InstructionError"));
+ let err_str = format!("{:?}", err);
+ assert!(
+ err_str.contains("already in use"),
+ "unexpected duplicate-delivery error: {err_str}",
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let err = banks_client.process_transaction(tx2).await.unwrap_err(); | |
| // Anchor initialization failure | |
| assert!( | |
| format!("{:?}", err).contains("already in use") | |
| || format!("{:?}", err).contains("InstructionError") | |
| ); | |
| assert!(format!("{:?}", err).contains("already in use") || format!("{:?}", err).contains("InstructionError")); | |
| let err = banks_client.process_transaction(tx2).await.unwrap_err(); | |
| // Anchor initialization failure | |
| let err_str = format!("{:?}", err); | |
| assert!( | |
| err_str.contains("already in use"), | |
| "unexpected duplicate-delivery error: {err_str}", | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resq-delivery/tests/integration.rs` around lines 382 - 384, The current
assertion is too broad; tighten it to verify the specific "account already in
use" error by checking the error debug string for the concrete variant name
instead of the generic "InstructionError" fallback. Replace the assert that
checks for "InstructionError" with one that asserts format!("{:?}", err)
contains either "already in use" or the specific variant string "AccountInUse"
(or "AccountAlreadyInitialized" if that's the variant used in this codebase) so
the test proves the duplicate-delivery path rather than any InstructionError.
1 participant
Automated sync from resq-software/resQ@
35cafa4.Review before merging — direct pushes to standalone repos are preserved.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores