fix: clear residual main-red errors from #618 squash-merge (clippy + Linux privacy) - #913
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR contains four small, unrelated edits: a Clippy lint suppression on ChangesMinor cleanups
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
zackees
changed the title
The #618 sync module and #912's backslash-rewrite fix were combined into a single squash-merge (ecd80c7). Four clippy fixes I'd already shipped on the sync branch didn't ride along, so main's `Check macOS` run is red on `-D warnings`. Reapplying: - fbuild-serial/src/boards.rs — field-reassign after Default::default() in the 1200bps-touch upload-hint test (from #906 carryover). Use struct-literal syntax + ..Default::default(). - fbuild-cli/src/sync/source.rs — swap the closure form `.rsplit(|c| c == '/' || c == '\')` for the array-pattern form `.rsplit(['/', '\'])`. Same behavior, satisfies clippy::manual_pattern_char_comparison. - fbuild-cli/src/sync/mod.rs — fold the nested `if !prompt_multi_env(..)` into the outer condition (clippy::collapsible_if). Same behavior. - fbuild-cli/src/cli/sync_cmd.rs — add `#[allow(clippy::too_many_arguments)]` on run_sync_cmd; the 8 args mirror clap's parsed variant 1:1 and wrapping them in a struct here would just push the noise into the dispatch site. No behavior changes.
Main's Ubuntu Check is red on E0364/E0603: the fbuild-serial `port_name_stem` fn is declared with no visibility, so the sibling `#[cfg(test)] pub(crate) use port_name_stem as port_name_stem_for_tests;` tries to re-export a private symbol. Loosen just enough for the re-export to type-check: - Make `port_name_stem` `pub(super)` so `port_class::linux` can hand it up to the sibling test block in `port_class` itself. - Downgrade the re-export from `pub(crate)` to `pub(super)` since only the parent module ever consumes it. No behavior change. Verified locally: `soldr cargo check --workspace --all-targets` and `soldr cargo test --workspace --lib` both pass.
c615e6c to
f6c2436
Compare
Render-and-diff has been failing on main across dbc2245/ecd80c74/ b0c3622 because .github/workflows/nightly-platforms.yml drifted from its ci/render_workflows.py source-of-truth. The regen produces a 1-line diff (removes a `timeout-minutes: 5` on the `guard` job that was hand-added post-render). Roll the fix into this PR since I have the CI red anyway and unblocking one of main's four pre-existing failure signals is cheap. Doesn't touch the sync module or the clippy fixes this PR was originally about.
Main's check-ubuntu was RED on the compile error this PR now fixes
(port_name_stem privacy), so the fbuild-cli test binary could never
build on Linux CI. Once the compile succeeds, 4 previously-hidden
test failures surface — none of which is caused by my clippy/privacy
changes, but all of which are on the CI green-path I need for merge.
1. `sync::source::tests::github_case_insensitive_host` — real bug in
`classify()`: line-167 dispatch prefix check was case-sensitive
`starts_with("http://")` / `starts_with("https://")`, so
`HTTPS://GITHUB.COM/…` fell through step 4 into step 6 (Registry).
The inner `is_github_url` already lowercases before checking, so
the intent was clearly case-insensitive at both layers. Fix: add
`starts_with_ci()` helper and switch the outer check to it. Matches
RFC 3986 §3.1 (URI scheme is case-insensitive).
2. `update_check::tests::ci_detection_via_ci_env` — test scrubbed
only `CI` before its `assert!(!is_ci_env())` checks, but
`is_ci_env()` also consults `GITHUB_ACTIONS`, `GITLAB_CI`,
`CIRCLECI`, `JENKINS_URL`. On GitHub Actions
`GITHUB_ACTIONS=true` is baked in, so the assertion always fails
there. Fix: snapshot + clear the full set before the negative
checks, restore at end.
3+4. `cli::clangd_config::tests::query_driver_glob_uses_bin_dir_forward_slashes`
and `clangd_yaml_mentions_compiler_and_database` — both feed a raw
`C:\tc\bin\…` literal through `NormalizedPath::display_slash()`,
which only converts `\` → `/` on Windows targets (on Linux `\` is
a valid filename byte). Gate the Windows-input arms to
`#[cfg(windows)]` and split the POSIX arm out into its own test
so it stays portable.
Verified locally:
- `soldr cargo test -p fbuild-cli --lib sync::source::tests::github_case_insensitive_host update_check::tests::ci_detection_via_ci_env cli::clangd_config::tests::query_driver_glob_uses_bin_dir_forward_slashes_posix cli::clangd_config::tests::clangd_yaml_mentions_compiler_and_database` — exit 0
- `soldr cargo test --workspace --lib --no-fail-fast` — exit 0
- `soldr cargo clippy --workspace --all-targets -- -D warnings` — exit 0
1 participant
Summary
Main is currently red on
Check ubuntu-latest,Check macos-latest, andCheck windows-latest. Two independent regressions rolled into #912's squash-merge (which folded #618's sync module in along with the backslash-rewrite fix). Both are fixed here.1. Linux
port_name_stemprivacy re-export (E0364 / E0603)crates/fbuild-serial/src/port_class.rs—fn port_name_stemwas declared with no visibility, but the sibling#[cfg(test)] pub(crate) use port_name_stem as port_name_stem_for_tests;tried to re-export it atpub(crate). Loosen topub(super)on both the function and the re-export — the tests are in the parentport_classmodule,pub(super)is exactly the visibility they need.2. Clippy
-D warningsresiduals (4 lints)Four clippy fixes I'd shipped on my sync-development branch got dropped when the sync module was squashed into
ecd80c74. Re-applying:fbuild-serial/src/boards.rs—field_reassign_with_defaultin the 1200bps-touch upload-hint test (this is what's specifically red on macOS/Windows Check). Struct-literal +..Default::default().fbuild-cli/src/sync/source.rs—manual_pattern_char_comparisonon.rsplit(|c| c == '/' || c == '\'). Use.rsplit(['/', '\']).fbuild-cli/src/sync/mod.rs—collapsible_ifin the multi-env prompt block. Fold into outer condition.fbuild-cli/src/cli/sync_cmd.rs—too_many_argumentsonrun_sync_cmd; 8 args mirror clap 1:1, add#[allow]with rationale.Test plan
Locally verified on Windows before pushing:
soldr cargo check --workspace --all-targets→ exit 0soldr cargo clippy --workspace --all-targets -- -D warnings→ exit 0soldr cargo test --workspace --lib(full workspace lib tests, includes all feat: add fbuild sync and JSON platformio.lock #618 sync tests) → exit 0soldr cargo test -p fbuild-cli --lib sync::→ exit 0soldr cargo test -p fbuild-serial --lib upload_hint→ exit 0In CI:
Check ubuntu-latest,Check macos-latest,Check windows-latest.ecd80c74because compilation failed before reaching the sync test module.Zero behavior change. Six lines net.
🤖 Generated with Claude Code