Skip to content

chore(clippy): bring cargo clippy to zero warnings and enforce -D warnings in CI#205

Merged
tylergraydev merged 10 commits into
tylergraydev:mainfrom
prefrontalsys:chore/clippy-cleanup
Apr 25, 2026
Merged

chore(clippy): bring cargo clippy to zero warnings and enforce -D warnings in CI#205
tylergraydev merged 10 commits into
tylergraydev:mainfrom
prefrontalsys:chore/clippy-cleanup

Conversation

@prefrontalsys

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #200 and #202. Brings cargo clippy --lib --tests --all-features -- -D warnings from 243 warnings to 0, then flips the CI Clippy job from continue-on-error: true to an enforcing gate, and adds --lib --tests so test-target lint debt can't accumulate invisibly.

No behavior change. All 2021 lib tests pass before and after.

Evidence

Baseline (upstream/main @ 63671e3, cargo clippy --lib --tests --all-features):

  • lib: 123 warnings
  • lib test: 120 warnings (70 duplicates)

Post-PR: 0 warnings, 0 errors.

Shape

9 commits, one category per commit, to make review bisectable:

  1. relocate cfg(test)-gated importsEnvPlaceholder and DEFAULT_MCP_SERVER_PORT were imported at module level but only used inside #[cfg(test)] blocks. Moving them into the test modules eliminates the warning without risking a cargo clippy --fix regression that would strip them and break test compile.
  2. prefix unused variables with _ — 10 sites across 6 files.
  3. replace useless vec! with arrays — 28 sites.
  4. resolve dead-code warnings via targeted allows — 66 warnings broken into four sub-patterns: 45 pub(crate) test-helpers (#[cfg_attr(not(test), allow(dead_code))]); 4 vestigial vendor enums + 8 variant structs (#[allow(dead_code)] + a comment noting they are reserved for a typed parse path that the Parsed*Mcp normalization currently supersedes); 4 serde-deser false positives + 3 fields; one genuine deletion (Database::from_connection, already #[cfg(test)]-gated with no callers).
  5. apply mechanical autofix suggestions — manual Option::map, assert_eq!(x, true), Iterator::lastnext_back(), map().flatten()and_then, redundant closures, useless format!, etc. Also replaces a tautological assert!(enabled == true || enabled == false) in debug_logger.rs with let _ = is_debug_enabled(); matching the adjacent test's pattern — the assertion was already always-true before the rewrite, and the newer nonminimal_bool lint now flags the explicit form.
  6. accept &Path instead of &PathBuf — 10 helper signatures; callers already pass &PathBuf which auto-derefs to &Path.
  7. allow type_complexity on DB-row-tuple helpers — 7 fns where the 7–10-field rusqlite row tuple IS the domain shape; a named alias would just duplicate the tuple. Matches how the existing McpTuple aliases in services/*_config.rs are scoped.
  8. long-tail cleanupmodule_inception on commands::commands, manual_async_fn on the rmcp ServerHandler impl (trait uses impl Future return), strip_prefix(\"---\") in parse_frontmatter, too_many_arguments on get_or_create_mcp(9), fm.contains_key(...) over fm.get(...).is_none(), non_snake_case allow on test_parse_skill_file_with_allowedTools_camelCase where the camelCase mirrors the schema field under test.
  9. ci: enforce -D warnings on lib + tests — drops continue-on-error: true, adds --lib --tests to the Clippy step.

Not in scope

  • clippy.toml / [lints.clippy] in Cargo.toml. The repo currently has no crate-level clippy config; adding one is a separate decision about baseline hardening.
  • Enabling lint groups beyond the default set (e.g. -W clippy::pedantic, -W clippy::nursery).
  • Matrix-ing Clippy across ubuntu/macos/windows; CI still runs it on ubuntu-only, which is sufficient for Rust-level lints.

Caveats

Relation to prior work

Test plan

  • cargo fmt --check — passes
  • cargo clippy --lib --tests --all-features -- -D warnings — exit 0
  • cargo test --lib --all-features — 2021 passed, 0 failed
  • cargo test --all-features — full suite, all green
  • Local diff review: changes confined to src-tauri/src/** and .github/workflows/rust-tests.yml

prefrontalsys and others added 10 commits April 22, 2026 19:59
EnvPlaceholder (mcp_registry.rs) and DEFAULT_MCP_SERVER_PORT (mcp_server.rs)
were imported at module level but only consumed inside #[cfg(test)] blocks.
This made them look unused to clippy's lib pass and invited a cargo-fix
regression that would strip them and break the test build. Moving the imports
into the test module where they are used eliminates the warning without
risking the test build.

Co-Authored-By: Claude <noreply@anthropic.com>
Resolves 'unused variable' warnings across 6 files by adding the _ prefix
idiom. In claude_settings.rs the mut binding was also dropped because the
variable was never mutated after the read. All sites are in test code or
Tauri command handlers where the variable captures a value kept for lifetime
purposes (app_handle.clone()) but not referenced directly.

Co-Authored-By: Claude <noreply@anthropic.com>
Sites in test code that collect JSONL lines or MCP configs into a temporary
collection only to call .join() or pass as a slice. Array literal is cheaper
(no heap allocation) and expresses intent more directly. 26 sites in
session_explorer.rs tests, 2 in config_writer.rs tests — all previously
flagged by 'clippy::useless_vec'.

Co-Authored-By: Claude <noreply@anthropic.com>
66 dead-code warnings broken into four sub-patterns, each with its
own appropriate fix:

1. Test-helper pub(crate) fns (45 sites across 9 files): #[cfg_attr(not(test),
   allow(dead_code))]. These are #[cfg(test)]-consumed helpers exposed as
   pub(crate) so the test module can bypass Tauri State<> wrappers. The attr
   suppresses the warning for non-test builds while preserving genuine
   dead-code detection in test builds.

2. Vestigial typed-parser vendor hierarchy: 4 enums (CodexMcp, CopilotMcp,
   CursorMcp, GeminiMcp) + 8 Stdio/Http variant structs. These were
   scaffolded for a typed intermediate-representation parse path that was
   never wired up; current code uses ParsedXxxMcp normalization directly.
   Annotated with #[allow(dead_code)] + a comment noting the design intent so
   they remain available for future typed-parse work.

3. Serde-deser false positives (DevcontainerConfig, ProfileItem,
   SpinnerVerbConfig, 3 fields on RawRecord/RawMessage/ToolManagerServer).
   Clippy's 'never constructed' lint does not see serde's
   Deserialize-generated constructors. Standard #[allow(dead_code)] idiom.

4. One genuine deletion: Database::from_connection in db/schema.rs. Already
   #[cfg(test)]-gated with zero callers; sibling Database::in_memory() is the
   actual entry point used throughout tests.

Dead-code warning count: 66 -> 0. Total clippy warnings: 134 -> 68.

Co-Authored-By: Claude <noreply@anthropic.com>
Broad pass applying clippy's machine-applicable suggestions now that the
cfg(test)-gated-import and unused-variable landmines have been resolved.
Covers:

  - manual Option::map rewrites (several sites in mcp_client.rs, skills.rs,
    session_explorer.rs)
  - assert_eq!(x, true/false) -> assert!(x) / assert!(!x)
  - useless format! dropped
  - redundant closures replaced with the function itself
  - Iterator::last on DoubleEndedIterator -> .next_back()
  - map_or simplification
  - Option::map().flatten() -> and_then
  - map().values() iteration cleanup
  - assorted rustfmt normalization introduced by the rewrites

Additionally: replaced a tautological bool assertion in debug_logger.rs
(assert!(enabled == true || enabled == false)) with 'let _ = is_debug_enabled();'
matching the adjacent test's 'let _ = path;' pattern — the assertion was
already always-true before clippy rewrote it, and the newer nonminimal_bool
lint now flags the explicit form as an error under -D warnings.

Clippy warning count: 68 -> 33.

Co-Authored-By: Claude <noreply@anthropic.com>
Clippy::ptr_arg prefers &Path over &PathBuf because the former is a slice
type and avoids a pointless heap indirection. 10 sites across 3 files:
  - src/services/debug_logger.rs (enable_debug_logging, get_logs_dir,
    get_debug_flag_path)
  - src/utils/paths.rs (project_mcp_file, project_settings_file)
  - src/utils/opencode_paths.rs (5 project_opencode_* helpers)
Call sites already pass &PathBuf, which auto-dereferences to &Path — no
caller changes required.

Co-Authored-By: Claude <noreply@anthropic.com>
7 command helpers use multi-field tuples (7-10 fields) for rusqlite row
extraction. These tuples ARE the domain shape — introducing type aliases
would just duplicate the tuple next to the fn without adding meaning,
and the existing file-local McpTuple aliases in services/*_config.rs
follow a different shape (writer tuples, not reader tuples).

Annotating with #[allow(clippy::type_complexity)] on the containing fn
matches the established pattern for 'this IS the data shape' cases.

Co-Authored-By: Claude <noreply@anthropic.com>
Final cleanup batch:

- opencode_paths.rs: #[cfg(test)] use std::path::Path; — Path is only needed
  by the cfg(test)-gated helpers that now take &Path.
- debug_logger.rs: &PathBuf -> &Path on 3 more helpers (persist_debug_enabled,
  is_debug_persisted, init_from_persisted). Missed in the first PathBuf pass.
- db/models.rs: #[cfg_attr(not(test), allow(dead_code))] on is_powerline and
  is_powerline_round — test-only consumers.
- docker/client.rs: #[allow(dead_code)] on connect_with_params (private
  helper used via ping_host) and ping_host (public API held for future use).
- docker/devcontainer.rs: #[allow(dead_code)] on the impl block — several
  methods are defined for the DevcontainerConfig API shape but not all are
  consumed today.
- commands/mod.rs: #[allow(clippy::module_inception)] on `pub mod commands`
  — the nested name mirrors the Claude Code commands domain concept;
  renaming would cascade through imports.
- mcp_gateway/tools.rs: #[allow(clippy::manual_async_fn)] on
  impl ServerHandler — rmcp's trait uses impl Future return shape; async fn
  in traits isn't usable here without widening the bound.
- commands/scanner.rs: removed tautological assert!(true) in the
  compilation-smoke-test (comment documents intent instead).
- services/scanner.rs:
  - #[allow(clippy::too_many_arguments)] on get_or_create_mcp (9/7)
  - strip_prefix("---") rewrite in parse_frontmatter
  - assert!(!fm.contains_key("empty_key")) replaces .get().is_none()
  - #[allow(non_snake_case)] on test_parse_skill_file_with_allowedTools_camelCase
    where the camelCase mirrors the schema field under test.
- services/mcp_registry.rs: filter_map always-Some -> map.

Clippy warning count: 17 -> 0. Tests: 2021 passing. Ready to flip CI gate.

Co-Authored-By: Claude <noreply@anthropic.com>
Flips the Clippy job to block CI on warnings. Two tightenings:

  - Drop 'continue-on-error: true'. The comment said 'initially' — eight
    commits of cleanup later, the baseline is zero warnings, so the gate
    becomes meaningful.
  - Add '--lib --tests' to the clippy invocation so the ~120 warnings that
    lived exclusively in the test target are covered too. Without this the
    prior gate (if enforced) would have allowed test-only lint debt to
    accumulate invisibly.

All other CI jobs untouched. No policy change beyond Clippy enforcement.

Co-Authored-By: Claude <noreply@anthropic.com>
@tylergraydev tylergraydev merged commit 74fe948 into tylergraydev:main Apr 25, 2026
10 checks passed
tylergraydev pushed a commit that referenced this pull request May 4, 2026
User-facing additions:
- Model picker now uses Claude Code aliases (opus/sonnet/haiku/opusplan/best)
  with a 1M context toggle and a free-form "Other (custom model ID)" field
  for Bedrock ARNs / Vertex / Foundry IDs (#213).
- Rules dropped into ~/.claude/rules/ or {project}/.claude/rules/ now show
  up in the UI via new global/project scanners (#200).

Fixes:
- Sync Config no longer wipes externally-managed configs (.mcp.json,
  claude.json, OpenCode/Copilot/Cursor/Gemini/Codex) — extends the #191
  guard to all 7 writers and imports .mcp.json on project add (#204).
- Skill/command/sub-agent writers emit `tags` as JSON arrays so they
  round-trip through the DB reader (#202).
- PR builds from forks/Dependabot no longer fail on missing
  TAURI_SIGNING_PRIVATE_KEY (#203).
- delete_rule_inner returns real DB errors instead of swallowing them as
  "row absent" (follow-up to #200).

Internal:
- Cargo clippy from 134 warnings to 0; CI now enforces -D warnings on
  --lib --tests (#205).

All 1565 frontend tests and 2021 Rust tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants