Repository navigation
refactor(cli): split cli.py into one module per command - #725
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The package omits previously exposed CLI constants, and the verify.py exception contract needs updating.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Refactors the monolithic CLI into command-specific modules while preserving entry points and behavior.
Changes:
- Splits CLI commands and shared helpers into
src/vip/cli/. - Updates tests, documentation, packaging references, and CI path filters.
- Preserves console-script and
python -m vip.cliexecution.
| File | Reviewed changes |
|---|---|
src/vip/plugin.py |
Updated CLI path documentation. |
src/vip/cli/version.py |
Extracted version command. |
src/vip/cli/verify.py |
Extracted verify command and helpers. Finding (nit, 1 vote; line 103): the docstring promises SystemExit, but _normalize_categories raises ConfigError; update the exception contract. |
src/vip/cli/status.py |
Extracted status command. |
src/vip/cli/scaffold.py |
Extracted scaffold command and source resolution. |
src/vip/cli/report.py |
Extracted report command and template helpers. |
src/vip/cli/install.py |
Extracted install and uninstall commands. |
src/vip/cli/cleanup.py |
Extracted cleanup operations and session handling. |
src/vip/cli/auth.py |
Extracted connect-key minting. |
src/vip/cli/app.py |
Added parser construction and command dispatch. |
src/vip/cli/_common.py |
Centralized CA-bundle resolution. |
src/vip/cli/__main__.py |
Preserved module execution support. |
src/vip/cli/__init__.py |
Re-exports CLI APIs. Finding (critical, 3 votes; line 21): VALID_CATEGORIES and VALID_FORMATS are no longer re-exported, breaking existing imports; re-export them or document the breaking change. |
src/vip/cli.py |
Removed the monolithic CLI implementation. |
src/vip/auth/scheme.py |
Updated CLI helper references. |
src/vip/auth/flows.py |
Updated CLI path references. |
src/vip/auth/cache.py |
Updated cleanup module references. |
selftests/test_errors.py |
Retargeted dispatch patches. |
selftests/test_cli_verify.py |
Retargeted verify patches. |
selftests/test_cli_url_scheme.py |
Updated CLI references. |
selftests/test_cli_report.py |
Retargeted report patches. |
selftests/test_cli_cleanup.py |
Retargeted cleanup patches. |
selftests/test_auth_scheme.py |
Updated helper references. |
selftests/test_auth_cache.py |
Updated the module-path invariant. |
selftests/install/test_cli_uninstall.py |
Retargeted uninstall patches. |
pyproject.toml |
Updated CLI path documentation. |
AGENTS.md |
Documented the new CLI layout. |
.github/workflows/workbench-smoke.yml |
Updated CLI path filters. |
.github/workflows/packagemanager-smoke.yml |
Updated CLI path filters. |
.github/workflows/mock-idp-e2e.yml |
Updated CLI path filters. |
.github/workflows/install-flow-smoke.yml |
Updated CLI path filters. |
.github/workflows/copilot-setup-steps.yml |
Updated CLI references. |
.github/workflows/connect-smoke.yml |
Updated CLI path filters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+21
to
+29
| from vip.cli.verify import ( | ||
| _OPT_IN_CATEGORIES, | ||
| DEFAULT_TEST_TIMEOUT_SECONDS, | ||
| _default_marker_expr, | ||
| _extra_keep_from_args, | ||
| _generate_temp_config, | ||
| _normalize_categories, | ||
| run_verify, | ||
| ) |
Contributor
|
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
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.
Turns
src/vip/cli.py(2137 lines) intosrc/vip/cli/with one module per subcommand actually registered inmain():verify.py(run_verifyand its helpers: credential checks, temp-config generation, xdist and marker handling; lines 34-658),report.py(run_report, Quarto render, report-template resolution; lines 666-870),status.py(run_status,_collect_status; lines 873-971),install.py(run_install,run_uninstall; lines 974-1172),scaffold.py(run_scaffoldand scaffold-source resolution; lines 1178-1306),cleanup.py(run_cleanup, the Workbench session sweep, cleanup config loading; lines 1309-1546),auth.py(mint_connect_key; lines 138-157), andversion.py(run_version, version details).app.pyholds theargparse.ArgumentParserconstruction,_reorder_help_args, andmain(), including the singleexcept VipErrorhandlererrors-cli-single-handler(#717) added aroundargs.func(args)atcli.py:2129-2133._resolve_effective_ca_bundleis used byverify,install(run_uninstall) andcleanup, so it moves to a shared_common.py; the report-template helpers (_REPORT_TEMPLATE_FILES,_copy_report_templates,_ensure_report_templates) are used only byrun_reporttoday, andrun_scaffold's_resolve_scaffold_sourcereuses only theirExitStackpattern, not the helpers, so they stay inreport.py. Thevip = "vip.cli:main"console-script entry point is unchanged becausevip/cli/__init__.pyre-exportsmainand everyrun_*function. The CI path filters in five workflows move fromsrc/vip/cli.pytosrc/vip/cli/**, and thesrc/vip/cli.pyrow in AGENTS.md's module table and the comment atpyproject.toml:163point at the new locations.__main__.pykeepspython -m vip.cliworking (four selftests invoke it). The source-checkout fallbacks in_ensure_report_templatesand_resolve_scaffold_sourcegain one.parentbecause the modules sit one directory deeper; without it they resolve tosrc/instead of the repo root and 27 selftests fail. An AST check confirms the other 42 top-level definitions are byte-identical tomain'scli.pyand these two differ only by that.parentand its comment. Stalecli.pyreferences in comments and docs now point at the new modules: AGENTS.md (module table row, theauth_cache_path()note, the proxy paragraph, the install-flow-smoke path list),copilot-setup-steps.yml,plugin.py,auth/cache.py,auth/flows.py,auth/scheme.pyandtest_auth_scheme.py(the latter two citedcli.py:391, already stale, nowvip.cli._common._resolve_effective_ca_bundle),test_cli_url_scheme.py, and atest_cli_verify.pydocstring.Patch targets retargeted (the only selftest edits):
main()bindsfunc=run_verifyetc. from its own module's namespace, sopatch("vip.cli.run_verify")(test_cli_verify.py:722,:1606),patch("vip.cli.run_cleanup")(test_cli_cleanup.py:462),setattr(vip.cli, "run_version", ...)(3 intest_errors.py),setattr(cli, "run_uninstall", ...)(install/test_cli_uninstall.py:330) andsetattr(vip.cli, "_cleanup_workbench_sessions", ...)(4 intest_cli_cleanup.py::129,:149,:179,:228) move tovip.cli.app/vip.cli.cleanup.patch("vip.cli.subprocess.run")/patch("vip.cli.sys.exit")(3 intest_cli_verify.py) andsetattr(cli.subprocess, "run", ...)(8 intest_cli_report.py) patch the shared stdlib modules through an attribute path; they move tovip.cli.verify.subprocess/vip.cli.report.subprocessso they do not depend on the package__init__happening to importsubprocess. 22 sites across 5 files. One non-patch selftest edit:test_auth_cache.py's inline-cache-path invariant readvip.cli.__file__, which is now__init__.pyand would pass vacuously, so it readsvip.cli.cleanup(whereauth_cache_path()is called) instead.Module-level state: none (
cli.pyhas noglobalstatements).#627 exposure: high. #627 adds 334 lines to
cli.py(a newvip tracesubcommand,run_trace, and--controlshandling inrun_report) and already has two conflict hunks againstmainincli.py, both where #717 replacedprint(...); sys.exit(...)with a raisedVipError(run_verify's--formatvalidation andrun_report's missing-results check); #627 adds newsys.exitsites in the old style there. After this split, every one of those lines has to be re-applied by hand into the new modules.Verified with
uvx ruff@0.15.0 check .,uvx ruff@0.15.0 format --check .,uv run --all-extras mypy src/vip src/vip_tests selftests,uv run pytest selftests/,uv run pytest src/vip_tests/ --collect-only -q, andvip --helpplus each subcommand's--helpcompared byte-for-byte againstmain.Live gate
The gate ran as workflow_dispatch runs on this branch (9d9440e) instead of local
just test-local-fulland the local mock-IdP lanes.24 passed, 5 skipped13 passed, 1 skipped, 4 deselected12 passed, 14 skipped2 passed, 1 skipped, 10 warnings; SAML diagnostic1 passed, 1 skipped, 6 warningsand2 passed, 1 skipped, 10 warningsubuntu-24.04 (uv tool, root)andmacos-latest (uv tool)both succeeded