chore(windows): make clippy -D warnings pass on a Windows host - #5762
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (9)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe changes add platform-specific compilation guards, remove unused imports and mutability, add ChangesPlatform and Rust Cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR makes platform-specific lint and compile-cleanliness fixes without changing runtime behavior; the reported checks and targeted tests pass, so no actionable merge-blocking risk remains beyond normal review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
How this change flows4 changed behaviours across 20 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 31 further behaviours left out to keep the diagram readable. flowchart LR
n0["claude_code_login_launch<br/>changed"]:::changed
n1["find_pid_on_port<br/>changed"]:::changed
n2["synthesize_piper<br/>changed"]:::changed
n3["check_workspace<br/>changed"]:::changed
n4["format"]:::impacted
n5["...ws_port_takeover_finds_and_kills_listener"]:::impacted
n6["Command"]:::impacted
n7["join"]:::impacted
n8["spawn_in_container"]:::impacted
n9["...cover_validation_and_mocked_runtime_paths"]:::impacted
n0 -->|calls| n4
n1 -->|calls| n4
n1 -->|uses| n6
n2 -->|calls| n4
n2 -->|uses| n6
n2 -->|calls| n7
n3 -->|calls| n4
n3 -->|calls| n7
n5 -->|calls| n1
n5 -->|tests| n1
n5 -->|calls| n4
n5 -->|tests| n4
n5 -->|uses| n6
n7 -->|calls| n4
n8 -->|calls| n4
n8 -->|uses| n6
n9 -->|calls| n2
n9 -->|tests| n2
n9 -->|calls| n7
n9 -->|tests| n7
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
The red The two failures Those tests resolve the process-global memory client and never call Why it fired here and not on
With Reproduced with no changes in the tree, clean All four — not just the two CI happened to schedule first. Which ones go red depends on ordering, so this would have bitten the next PR too. Why it cannot be this diff. The only composio file here is +#[cfg(not(windows))]
use fs2::FileExt;On Linux that predicate is true and the import is kept, so the Linux build of this branch is identical to #5769 adds the missing Once #5769 lands I will re-run this branch. |
|
#5769's own CI settles it: 19 checks green, and no So the two red tests here are not this diff, and #5769 fixes them without any change to production code. Once it lands I will re-run this branch. |
|
Maintainer review — no changes pushed, read-only assessment. State: On the change: it is the right kind of PR. Every edit is an import gate, a removed Rebasing it — one conflict, and it is not what it looks likeOnly #[cfg(test)]
#[path = "core_tests.rs"]
mod tests;
include!("core_part_01.rs");
include!("core_part_02.rs");Your one-line change moved with the body. Take #[cfg(target_os = "windows")]
{
- return available_disk_space_mb_windows(path);
+ available_disk_space_mb_windows(path)
}I confirmed the other eleven files rebase clean — they auto-merge with no conflict. Two things to check while you are in there, both consequences of the same split:
Current red checks are not diagnostic — with the branch this far behind, Not approving; a maintainer reviews and merges. |
`cargo clippy -p openhuman -- -D warnings` is what .husky/pre-push runs, and on Windows it fails with 11 errors before it reaches anything the developer changed. Linux CI never sees them: every one is in a code path guarded by #[cfg(windows)] or #[cfg(not(unix))], so the lint only fires where those arms are compiled. The practical effect is that no commit can be pushed from a Windows checkout. Imports that only one platform uses are now gated the way the imports beside them already were: - core/auth.rs, security/pairing.rs: `std::io::Write` is used solely by the #[cfg(unix)] arm of the token writer; the #[cfg(not(unix))] arm calls std::fs::write. Gated with #[cfg(unix)], matching the OpenOptionsExt import directly below it. - composio/trigger_history.rs: `fs2::FileExt` backs the #[cfg(not(windows))] lock/unlock path only - Windows serializes through the process-local Mutex above it. Two imports were redundant rather than platform-specific: - inference/local/process_util.rs, inference/voice/local_speech.rs: `creation_flags` is an inherent method on tokio::process::Command under Windows, so importing std's CommandExt adds nothing. The rest are ordinary lints in Windows-only code: - sandbox/cwd_jail/windows.rs: drop the unused HANDLE import, drop `mut` from `dacl_present` (only ever read), pass `&ea` to SetEntriesInAclW which takes *const, and add the Default impl clippy asks for next to AppContainerBackend::new(). - security/keyring/encrypted_store.rs: the #[cfg(windows)] arm shadows `msg` rather than mutating it, so `mut` was unnecessary on both platforms and the #[cfg_attr(not(windows), allow(unused_mut))] escape hatch can go with it. - platform/doctor/core.rs: on Windows the sibling #[cfg] block is stripped, so the remaining block is the function tail and the `return` is noise. No behaviour changes. cargo fmt clean, and clippy -D warnings now passes on Windows.
0227cf3 to
c616bff
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
The four composio_delete_connection_clear_memory_* tests take the clear_memory=true path, which resolves the process-global memory client through active_memory_client(). The embedding seam fails loudly when unwired, and none of these four call install_for_tests() — so they pass only while some earlier test in the same binary happened to install the seams first. ops_tests.rs already warns about exactly this, on notion_cleanup_targets_include_synced_page_sources. These four are the tests that omit it. Run alone against a clean main, all four fail with [composio] delete_connection cannot resolve the memory client: no EmbeddingHost installed CI has been green because Rust Core Coverage only runs the modules a PR changed: a filter that includes openhuman::memory picks up one of the 104 install_for_tests() callers for free, and one that does not gets red composio tests. That is what happened on tinyhumansai#5762, whose diff touches no composio code that Linux compiles.
…ppy-clean\n\nchore(windows): make clippy -D warnings pass on a Windows host\n
3 participants
No commit can be pushed from a Windows checkout
.husky/pre-pushrunscargo clippy -p openhuman -- -D warningsand then the same forapp/src-tauri. On a Windows host both fail before reaching anything the developer changed — 11 errors in the core crate, 4 in the Tauri crate.Linux CI never sees them: every one sits in a path guarded by
#[cfg(windows)]/#[cfg(not(unix))], so the lint only fires where those arms are compiled. The practical effect is that the hook is unpassable on Windows, and the only way to push is--no-verify— which disables the TypeScript, format and lint checks too.What changed
Imports only one platform uses, gated the way their neighbours already are:
core/auth.rs,security/pairing.rsstd::io::Write#[cfg(unix)]arm of the token writer — the#[cfg(not(unix))]arm callsstd::fs::writecomposio/trigger_history.rsfs2::FileExt#[cfg(not(windows))]lock/unlock path — Windows serializes through the process-localMutexabove itIn both files the gate matches the
#[cfg(unix)] use std::os::unix::fs::OpenOptionsExtline sitting directly below.Two imports were simply redundant:
inference/local/process_util.rsandinference/voice/local_speech.rsimportstd::os::windows::process::CommandExtto callcreation_flags, but that is an inherent method ontokio::process::Commandunder Windows, so the trait import adds nothing.Ordinary lints in Windows-only code:
sandbox/cwd_jail/windows.rs— drop the unusedHANDLEimport; dropmutfromdacl_present(only ever read, vialet _ =); pass&eatoSetEntriesInAclW, whose second parameter is*const EXPLICIT_ACCESS_W; add theDefaultimpl clippy asks for besideAppContainerBackend::new().security/keyring/encrypted_store.rs— the#[cfg(windows)]arm shadowsmsgrather than mutating it, somutwas unnecessary on both platforms; the#[cfg_attr(not(windows), allow(unused_mut))]escape hatch goes with it.platform/doctor/core.rs,src-tauri/claude_code.rs— on Windows the sibling#[cfg]blocks are stripped, so the remaining block is the function tail andreturnis noise. The macOS/Linux arms ofclaude_code_login_launchalready end in a bare expression; this makes the Windows arm match.src-tauri/core_process.rs—parse_lsof_pidparseslsofoutput and is reachable only from the#[cfg(unix)]find_pid_on_port, so it is dead on Windows. Gated with#[cfg(unix)], along with its import and test.src-tauri/deep_link_ipc_windows.rs— name the boxed callback (type LiveHandler) so the static and its accessor stop trippingclippy::type_complexity.Verification
No behaviour changes — every edit is an import gate, a removed
mut/return, a&mut→&, a type alias, or aDefaultimpl.Tests for every touched module:
This branch was pushed with the pre-push hook running and passing on Windows — which is the point of the change.
Summary by CodeRabbit
Refactor
Chores