feat: unify CLI entry point - eliminate binary/wheel split#179
Merged
Conversation
BREAKING CHANGE: The separate command is removed. Wheel installation is now available through the unified command. New subcommand-based CLI: clang-tools install <version> # auto-detect binary/wheel clang-tools install <version> --binary # force binary clang-tools install <version> --wheel # force wheel clang-tools install <tool-name> --wheel # wheel install by tool name clang-tools uninstall <version> # remove installed tools Auto-detection logic: - Version-like targets in supported range → binary first, fallback wheel - Version-like targets out of range → wheel - Tool name targets → wheel Legacy / flags are preserved for backward compatibility.
Drop --install/--uninstall top-level flags. Only subcommands remain: clang-tools install <target> [options] clang-tools uninstall <version> [options] Also fix Python 3.9 type annotation compatibility (str | None -> Optional[str]).
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors the clang-tools CLI from top-level ChangesUnified clang-tools CLI subcommand refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
shenxianpeng
force-pushed
the
feature/unify-cli-entry
branch
from
9ee22bf to
736cc4f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/gen_cli_docs.py (1)
37-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude subparser options when generating CLI docs.
_write_cli_doc()only walks top-levelparser._actions, so unified subcommand options are omitted from generated docs. With the new CLI design, this makescli_args.mdincomplete.Also applies to: 73-73
🧹 Nitpick comments (2)
tests/test_main.py (1)
154-181: ⚡ Quick winStrengthen uninstall tests to assert actual removal.
These tests currently pass on log text + exit code only. Add assertions that the expected binary/symlink no longer exists so API-order regressions are caught.
✅ Suggested assertion additions
def test_main_legacy_uninstall( @@ assert "Uninstalling" in result.out assert exit_code == 0 + assert not dummy_bin.exists() def test_main_uninstall_subcommand( @@ assert exit_code == 0 assert "Uninstalling" in result.out + assert not dummy_bin.exists()Also applies to: 513-540
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_main.py` around lines 154 - 181, The test test_main_legacy_uninstall currently only asserts log output and exit code but doesn't verify that the installed binary/symlink was actually removed; update this test (and the similar tests around lines 513-540) to assert the filesystem state after calling main(): check that the path represented by dummy_bin (and any expected symlink in install_dir) does not exist (use install_dir, tool_name, version and suffix to compute the expected path) so the uninstall behavior of main() is validated end-to-end.clang_tools/main.py (1)
183-324: 🏗️ Heavy liftRefactor
main()into smaller handlers before this grows further.Current cognitive complexity is high and already hides cross-path contract mistakes. Please split into focused handlers (e.g., legacy flow, install flow, uninstall flow, and install mode selection).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clang_tools/main.py` around lines 183 - 324, The main() function has grown too complex—split it into focused handler functions to reduce cognitive complexity and avoid hidden contract bugs: extract the legacy flow into a handler (e.g., handle_legacy_flags(args) calling uninstall_clang_tools and install_clang_tools), move uninstall branch into handle_uninstall(args) that calls uninstall_clang_tools, and move the install branch into handle_install(args) that encapsulates mode selection (wheel vs binary vs auto-detect) using helpers that call _wheel_install, install_clang_tools, and use _is_version_like and WHEEL_TOOLS for validation; update main() to only parse args and dispatch to these handlers, returning their exit codes.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clang_tools/main.py`:
- Around line 30-31: WHEEL_TOOLS currently includes "clang-query" and
"clang-apply-replacements" but the docs/state that wheel support is only for
"clang-format" and "clang-tidy", causing _wheel_install() to later fail; update
the WHEEL_TOOLS constant to only {"clang-format", "clang-tidy"} and remove any
other occurrences or references to "clang-query" and "clang-apply-replacements"
(including the duplicate definition/uses around the later block that mirrors
314-322) so accepted targets align with what _wheel_install() supports.
- Around line 194-196: The uninstall_clang_tools call arguments are in the wrong
order: uninstall_clang_tools expects (tools, version, directory) but the diff
passes (version, tools, directory); update both call sites (the one using
args._legacy_uninstall at the uninstall dispatch and the other at line 228) to
call uninstall_clang_tools(args.tool, args._legacy_uninstall, args.directory)
(or the equivalent ordering using the function's parameter names) so tools is
passed first, then version, then directory.
---
Nitpick comments:
In `@clang_tools/main.py`:
- Around line 183-324: The main() function has grown too complex—split it into
focused handler functions to reduce cognitive complexity and avoid hidden
contract bugs: extract the legacy flow into a handler (e.g.,
handle_legacy_flags(args) calling uninstall_clang_tools and
install_clang_tools), move uninstall branch into handle_uninstall(args) that
calls uninstall_clang_tools, and move the install branch into
handle_install(args) that encapsulates mode selection (wheel vs binary vs
auto-detect) using helpers that call _wheel_install, install_clang_tools, and
use _is_version_like and WHEEL_TOOLS for validation; update main() to only parse
args and dispatch to these handlers, returning their exit codes.
In `@tests/test_main.py`:
- Around line 154-181: The test test_main_legacy_uninstall currently only
asserts log output and exit code but doesn't verify that the installed
binary/symlink was actually removed; update this test (and the similar tests
around lines 513-540) to assert the filesystem state after calling main(): check
that the path represented by dummy_bin (and any expected symlink in install_dir)
does not exist (use install_dir, tool_name, version and suffix to compute the
expected path) so the uninstall behavior of main() is validated end-to-end.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c580859-64b5-4814-a44b-a5f8a0a714bc
📒 Files selected for processing (8)
README.mdclang_tools/main.pydocs/api.mddocs/gen_cli_docs.pydocs/index.mdmkdocs.ymlpyproject.tomltests/test_main.py
💤 Files with no reviewable changes (2)
- mkdocs.yml
- pyproject.toml
- Version.__init__() in clang_tools/util.py - main() in clang_tools/wheel.py
shenxianpeng
force-pushed
the
feature/unify-cli-entry
branch
from
7eb59f9 to
d023586
Compare
clang-query and clang-apply-replacements are binary-only and not available as wheels via cpp-linter-hooks. Add validation in --wheel path and new test for unsupported tool error.
shenxianpeng
force-pushed
the
feature/unify-cli-entry
branch
from
2302f07 to
29569e2
Compare
- Split main() into _handle_install/_handle_wheel/_handle_binary/ _handle_auto_detect handlers (fixes C901 complexity) - Replace blind except Exception with specific OSError/ValueError/ SystemExit (fixes BLE001) - Remove unnecessary else after return (fixes RET505) - Remove unused MIN_VERSION/MAX_VERSION imports - Update gen_cli_docs.py to handle subparsers - Extract _validate_wheel_tool helper
shenxianpeng
force-pushed
the
feature/unify-cli-entry
branch
from
72a2885 to
d4b5186
Compare
shenxianpeng
force-pushed
the
feature/unify-cli-entry
branch
from
0d0e80c to
19798a9
Compare
for more information, see https://pre-commit.ci
github-actions
Bot
added
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 99.64% 99.87% +0.23%
==========================================
Files 9 9
Lines 558 824 +266
==========================================
+ Hits 556 823 +267
+ Misses 2 1 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Fix uninstall_clang_tools argument order (version/tools were swapped) - Update CI workflow to use new subcommand syntax (clang-tools install, not --install) - Fix type annotations: tools: str → list[str] in install.py - Strengthen uninstall test to assert actual filesystem removal
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clang_tools/install.py (1)
250-260: 💤 Low valueDocstring missing
:param tools:description.The function signature now accepts
tools: list[str], but the docstring doesn't document this parameter.📝 Suggested docstring update
def uninstall_clang_tools(tools: list[str], version: str, directory: str): """Uninstall a clang tool of a given version. + :param tools: The list of tool names to uninstall. :param version: The version of the clang-tools to remove. :param directory: The directory from which to remove the installed clang-tools. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clang_tools/install.py` around lines 250 - 260, The docstring for uninstall_clang_tools is missing documentation for the tools parameter; update the docstring to add a :param tools: entry describing that it is a list of clang tool names to uninstall (e.g., "list of tool names to remove"), and mention the expected element type (str) and how it's used (each tool is passed to uninstall_tool with the given version and install_dir) so the parameters section documents tools, version, and directory consistently for the uninstall_clang_tools function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clang_tools/main.py`:
- Around line 137-144: The except block in install_clang_tools currently catches
SystemExit (masking permission errors from move_and_chmod_bin) and then falls
back to _wheel_install; change the except to only catch OSError and ValueError
so SystemExit is allowed to propagate; specifically, update the except line in
install_clang_tools from "except (OSError, ValueError, SystemExit) as exc:" to
"except (OSError, ValueError) as exc:" (no other changes
needed—move_and_chmod_bin should still raise SystemExit and _wheel_install
remains the fallback for the caught errors).
---
Nitpick comments:
In `@clang_tools/install.py`:
- Around line 250-260: The docstring for uninstall_clang_tools is missing
documentation for the tools parameter; update the docstring to add a :param
tools: entry describing that it is a list of clang tool names to uninstall
(e.g., "list of tool names to remove"), and mention the expected element type
(str) and how it's used (each tool is passed to uninstall_tool with the given
version and install_dir) so the parameters section documents tools, version, and
directory consistently for the uninstall_clang_tools function.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f1b5e20-ba52-4f1e-8e51-1a4b3fece6e5
📒 Files selected for processing (8)
.github/workflows/test.ymlCONTRIBUTING.mdclang_tools/install.pyclang_tools/main.pyclang_tools/util.pyclang_tools/wheel.pydocs/gen_cli_docs.pytests/test_main.py
✅ Files skipped from review due to trivial changes (3)
- CONTRIBUTING.md
- clang_tools/wheel.py
- clang_tools/util.py
shenxianpeng
added a commit
that referenced
this pull request
Jun 13, 2026
Bandit B101 ('Use of assert detected') is a well-known false positive
for pytest test files where assert is the standard assertion mechanism.
Exclude the tests/ directory from Bandit analysis to prevent 32 false
positives on PR #179.
- Add Bandit to pre-commit hooks - Add Bandit as dev dependency - Add security CI job running Bandit on production code - Exclude tests/ from Bandit (assert is standard in pytest) - Codacy webhook deleted via GitHub API
shenxianpeng
force-pushed
the
feature/unify-cli-entry
branch
from
fb767c0 to
946113c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Around line 33-35: The CI step named "Run Bandit security scan" currently has
continue-on-error: true which allows security findings to be ignored; change
that so the Bandit step fails the job by removing the continue-on-error key or
setting it to false, or implement a conditional gating strategy (e.g., set
continue-on-error based on an input/secret or a workflow variable like
allow_known_findings) and ensure any conditional logic (used to bypass failures)
is explicit and reviewed; update the step that runs bandit (the "Run Bandit
security scan" step) accordingly so security scan failures are blocking unless
intentionally and explicitly allowed.
- Around line 20-21: The new GitHub Actions job named "security" lacks an
explicit permissions block; add one to grant the minimal read-only scope
required for a Bandit scan by adding a permissions section under the "security"
job with at least "contents: read" (e.g., permissions: { contents: read }) so
the job uses least-privilege for GITHUB_TOKEN.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24e37547-d782-4c63-858b-29857a3f67f6
📒 Files selected for processing (3)
.github/workflows/test.yml.pre-commit-config.yamlpyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
Comment on lines
+33
to
+35
| - name: Run Bandit security scan | ||
| continue-on-error: true | ||
| run: bandit -c pyproject.toml -r clang_tools/ |
Contributor
There was a problem hiding this comment.
Make the security scan blocking (or gate failures conditionally).
Line 34 sets continue-on-error: true, which allows known security findings to pass CI silently. That weakens the intended security gate for this workflow.
Suggested fix
- name: Run Bandit security scan
- continue-on-error: true
run: bandit -c pyproject.toml -r clang_tools/🧰 Tools
🪛 zizmor (1.25.2)
[warning] 20-35: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/test.yml around lines 33 - 35, The CI step named "Run
Bandit security scan" currently has continue-on-error: true which allows
security findings to be ignored; change that so the Bandit step fails the job by
removing the continue-on-error key or setting it to false, or implement a
conditional gating strategy (e.g., set continue-on-error based on an
input/secret or a workflow variable like allow_known_findings) and ensure any
conditional logic (used to bypass failures) is explicit and reviewed; update the
step that runs bandit (the "Run Bandit security scan" step) accordingly so
security scan failures are blocking unless intentionally and explicitly allowed.
- Remove Bandit from pre-commit hooks (kept in CI security job only) - Add test for check_install_arch() amd64 branch - Add test for clang_tools_binary_url() non-macOS platform branch - Add test for install_dir_name() Linux default directory - Add test for create_sym_link() mkdir when directory doesn't exist - Add tests for _wheel_install() success and failure paths - Add test for _handle_binary with zeroed-out version (0.0.0) Production code coverage: clang_tools/*.py -> 100%
This reverts commit 946113c.
shenxianpeng
force-pushed
the
feature/unify-cli-entry
branch
from
e6e3a80 to
30488b2
Compare
- Remove SystemExit from except clause in _handle_auto_detect to avoid masking permission and unsupported-OS errors (SonarCloud python:S5754) - Fix type mismatch in test_install.py: pass list[str] instead of str to install_clang_tools (SonarCloud python:S5655 x3) - Add missing :param tools: docstring to uninstall_clang_tools - Wrap wheel install examples in code fences inside admonition in docs/index.md - Update REQUIRED_VERSIONS in gen_cli_docs.py with new CLI arg dests - Add tests for _wheel_install with version=None (latest), multi-tool mixed results, and _handle_auto_detect bad-semver path
for more information, see https://pre-commit.ci
shenxianpeng
changed the title
|
shenxianpeng
removed
the
documentation
This was referenced Jun 14, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Eliminates the binary/wheel CLI split by providing a single unified
clang-toolscommand.Before (confusing for users)
After (unified)
Auto-detection logic
Backward compatibility
Legacy
--install/--uninstalltop-level flags are preserved:Changes
clang_tools/main.py: Rewritten with subcommand-based CLI, auto-detection, and fallback logicpyproject.toml: Removedclang-tools-wheelentry pointBreaking change
The separate
clang-tools-wheelcommand is removed. Users should migrate:clang-tools-wheel --tool clang-formatclang-tools install clang-format --wheelclang-tools-wheel --tool clang-tidy --version 21clang-tools install clang-tidy --wheel --version 21Summary by CodeRabbit
New Features
clang-tools install <target>andclang-tools uninstall <version>; explicit--binary/--wheeloptions with auto-detect fallback.clang-tools install --wheelwith optional--version; wheel install support limited to supported tools.Documentation
Tests
Chores
clang-tools-wheelcommand; CI updated to use unified CLI.