feat(cli): final CLI polish and UX consistency pass #1018
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
cleveragents/cleveragents-core!1018
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m8-cli-polish"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Final CLI polish and UX consistency pass: shared constants, centralized error formatting, shell completion, and standardized help text.
New Modules
cli/constants.py(70 lines): Exit codes (EXIT_SUCCESS=0 throughEXIT_CONFLICT=4), format defaults (FORMAT_TEXT,FORMAT_JSON,FORMAT_TABLE)cli/errors.py(105 lines):cli_error()with hint support,cli_warning(),cli_not_found()with resource-type-aware hintCLI Changes
completioncommand generating scripts for bash/zsh/fish/powershellcli/__init__.pyTests
Quality Gates
nox -s lintnox -s typechecknox -s unit_testsnox -s coverage_reportCloses #861
cafee8b2b21ccea894f6PM Status — Day 37 — Rebase Required
This PR has merge conflicts and cannot be merged in its current state. 42% of all open PRs (21 of 50) have conflicts — this is a project-wide issue that must be resolved.
@brent.edwards — Please rebase this PR onto
masterby Day 39 EOD (2026-03-19). If you cannot rebase by then, please post a comment explaining the blocker.PM rebase request — Day 37
1ccea894f626648e5815Code Review Report: PR #1018 —
feat(cli): final CLI polish and UX consistency passBranch:
feature/m8-cli-polish| Issue: #861 | Reviewer: @CoreRasurae (automated review)Review scope: Code changes in
feature/m8-cli-polishplus close connections to surrounding code.Methodology: 3 global review cycles across all categories (bugs, security, test coverage, performance, spec compliance).
Summary
This PR introduces shared CLI constants (
constants.py), centralized error formatting utilities (errors.py), acompletioncommand for shell tab-completion, and standardized help text. It also performs large-scale cleanup of TUI/persona/REPL-input-mode code. While the new infrastructure modules are well-designed, the PR regresses critical functionality in the plan lifecycle service and leaves the acceptance criteria partially unmet. 15 findings are reported below, organized by severity and category.CRITICAL
C1.
list_plans()DB fallback removed — cross-process plan discovery brokenFile:
src/cleveragents/application/services/plan_lifecycle_service.py(lines 800-803)Category: Bug / Regression
The previous code queried
ctx.lifecycle_plans.list_all()from the database to populate the in-memory cache. This was a deliberate fix (commit22580752by Luis Mendes on Mar 13: "list_plans() only read from the in-memory self._plans dict, but the DI container creates PlanLifecycleService via providers.Factory, so every CLI invocation started with an empty dict"). This PR reverted that fix.Since
plan_lifecycle_serviceis registered as aproviders.Factoryincontainer.py:433, every CLI invocation creates a fresh instance with_plans = {}. The following commands now return empty results when the plan was created in a different process:agents plan execute(no plan_id) — "No plans ready for execution"agents plan lifecycle-apply(no plan_id) — "No plans ready for apply"agents plan status(no plan_id) — "No v3 lifecycle plans found"agents plan lifecycle-list— always empty_resolve_active_plan_id()— breaksplan correctwithout explicit--planFix: Restore the DB query fallback in
list_plans()as implemented in commit22580752.C2. CLI overrides from
plan useare no longer persistedFile:
src/cleveragents/cli/commands/plan.py(use_action command, lines ~1559-1602)Category: Bug / Regression
The PR removed both
service.save_plan(plan)andservice._commit_plan(plan)calls at the end of theuse_actioncommand. It also removed thesave_plan()method fromPlanLifecycleService. As a result, CLI overrides applied via flags (--strategy-actor,--execution-actor,--estimation-actor,--invariant-actor,--automation-profile,--execution-environment) are set on the in-memory plan object afteruse_action()returns, but never persisted to the database.When
plan executeis called (even in the same terminal), a newPlanLifecycleServiceis created (Factory pattern), and the overrides are lost.Fix: Re-introduce a persistence call after applying CLI overrides. Either restore
save_plan()/_commit_plan()or fold the overrides into theuse_action()service call.HIGH
H1.
ValueError/Exceptionhandlers removed fromexecute_plan— provider errors now crashFile:
src/cleveragents/cli/commands/plan.py(execute_plan, lines ~1792-1800)Category: Bug / Error handling regression
The PR removed:
ValueErroris commonly raised by provider resolution failures (e.g., missing API keys, invalid model configs). Without these handlers, such errors now produce an unformatted traceback instead of a clean CLI error message. The same removal also applies to_lifecycle_apply_with_id.Fix: Restore the
except ValueErrorhandler at minimum. Consider addingexcept Exceptionas a final catch-all with formatted output.H2. Null safety in
_lifecycle_apply_with_id— potentialAttributeErrorcrashFile:
src/cleveragents/cli/commands/plan.py(lines 892-900)Category: Bug
After
service.apply_plan(plan_id)at line 890, the code callscurrent = service.get_plan(plan_id)at line 892 and immediately accessescurrent.phaseat line 893 without a null check. Ifget_planreturnsNone(e.g., DB error, plan somehow removed), this crashes withAttributeError: 'NoneType' object has no attribute 'phase'. Same issue at line 896.Fix: Add null guards:
if current is None: raise typer.Abort().H3.
execution_environmentoverride inexecute_planis not persistedFile:
src/cleveragents/cli/commands/plan.py(lines 1713-1715)Category: Bug
When
--execution-environmentis passed toplan execute, the code setspre.execution_environment = execution_environment.lower()on the in-memory object but theservice._commit_plan(pre)call was removed. The override is ephemeral.Fix: Re-add a persistence call after setting the execution environment.
H4.
list_actions()has the same missing DB fallback aslist_plans()File:
src/cleveragents/application/services/plan_lifecycle_service.py(lines 540-565)Category: Bug / Regression
list_actions()only readsself._actions.values()with no database fallback. Due to the Factory DI pattern,action listin a separate CLI process fromaction createwill always show empty results.Fix: Apply the same DB fallback pattern as needed for
list_plans().MEDIUM
M1. Zero adoption of standardized exit codes in command modules
File: All files under
src/cleveragents/cli/commands/Category: Spec compliance / Issue acceptance criteria
The new
EXIT_SUCCESS,EXIT_ERROR,EXIT_USAGE,EXIT_NOT_FOUND,EXIT_CONFLICTconstants are defined but no command module imports or uses them. There are approximately 68 hardcodedtyper.Exit(<number>)calls across all command files. The issue's acceptance criteria states "Exit codes standardized" — this is only achieved at the definition level, not at the adoption level.Additionally,
actor.pyandactor_run.pyusecode=2andcode=3with semantics that conflict with the standardized definitions (EXIT_USAGE=2,EXIT_NOT_FOUND=3).M2. Zero adoption of
cli_error/cli_not_found/cli_warningin command modulesFile: All files under
src/cleveragents/cli/commands/Category: Spec compliance / Issue acceptance criteria
The centralized error formatting functions are defined and well-documented but never called from any command handler. All commands still use inline
console.print(f"[red]Error:[/red] ...")patterns. The issue's acceptance criteria states "Error messages follow consistent pattern" — this remains unmet.M3.
FORMAT_TEXT = "text"is not inVALID_FORMATSFile:
src/cleveragents/cli/constants.py(lines 40, 49)Category: Bug / Inconsistency
VALID_FORMATS = ("json", "yaml", "plain", "table", "rich", "color")does not include"text", yetFORMAT_TEXT = "text"is defined as a named constant. UsingFORMAT_TEXTas a format value will fail validation againstVALID_FORMATS. The spec (line 208) also does not list "text" as a valid format.Fix: Either remove
FORMAT_TEXTor add"text"toVALID_FORMATS(if it should be an alias for"plain").M4.
FORMAT_HELPomits "color" formatFile:
src/cleveragents/cli/constants.py(lines 34, 40)Category: Inconsistency
FORMAT_HELP = "Output format: json, yaml, plain, table, or rich (default: rich)"does not mention"color", butVALID_FORMATSincludes it, and the spec (line 208) listsrich|color|table|plain|json|yaml.Fix: Add "color" to
FORMAT_HELP.M5.
cli_not_foundgenerates incorrect hint for multi-word resource typesFile:
src/cleveragents/cli/errors.py(lines 94-95)Category: Bug
For resource types like
"Automation Profile", this generatesagents automation profile listinstead of the correctagents automation-profile list. The pluralization{resource_type.lower()}salso producesautomation profilesinstead ofautomation-profiles.Fix: Accept an optional
cli_commandparameter, or convert spaces/underscores to hyphens.LOW
L1.
execute_planhas duplicated output blocksFile:
src/cleveragents/cli/commands/plan.py(lines 1731-1790)Category: Code quality
The refactored
execute_planhas 3 near-identical blocks that each print the plan details and a next-step hint. This should be extracted into a helper function.L2. Module-level
err_console = get_err_console()eagerly creates consoleFile:
src/cleveragents/cli/main.py(line 54)Category: Performance
err_console = get_err_console()at module scope forces immediate creation of a Rich Console during import, undermining the lazy initialization strategy used byget_console()andget_err_console(). This adds unnecessary import-time overhead for lightweight commands like--version.L3. No tests for
completioncommand actual outputFile:
features/cli_consistency.feature,robot/cli_consistency.robotCategory: Test coverage gap
The
completiontests only verify the--helpoutput and unsupported-shell rejection. No test verifies that runningcompletion bashactually produces valid completion script content (even a basic non-empty check).L4. REPL
dispatch_commandsilently swallows exception detailsFile:
src/cleveragents/cli/commands/repl.py(lines 161-168)Category: Code quality / Debuggability
The
except Exception as excblock only printsstr(exc), discarding the traceback. This makes debugging REPL errors unnecessarily difficult.Positive Observations
m4_003_plan_env_columns.py) was the chain's leaf node — no migration chain breakage.constants.pyanderrors.pymodules are well-documented with proper__all__exports.vulture_whitelist.pyproperly updated to suppress false positives for new symbols.Review conducted over 3 global cycles covering: bugs, security, test coverage/flaws, performance, and spec compliance.
26648e58158654368dc2Review Fixes Applied
Addressed the review findings by force-pushing commit
8654368d.Step 1: Reverted unrelated regressions
Restored the following 6 files to their
masterversions — these were bundled into the squashed commit but are unrelated to CLI polish:CHANGELOG.mdbenchmarks/bench_uko_layer3.pydocs/timeline.mdfeatures/steps/plan_lifecycle_commands_coverage_steps.pysrc/cleveragents/application/services/plan_lifecycle_service.pylist_plans(),save_plan(), andlist_actions()src/cleveragents/cli/commands/plan.pysave_plan/_commit_planpersistence,ValueError/Exceptionhandlers inexecute_plan, and execution_environment persistenceThis resolves C1, C2, H1, H3, H4 by restoring master's code that the squash had regressed.
Step 2: CLI polish-specific fixes
FORMAT_TEXTnow equals"plain"(was"text"which isn't inVALID_FORMATS)FORMAT_HELPstring now includes"color"to matchVALID_FORMATScli_not_foundaccepts an optionalcli_commandkeyword parameter; when omitted, it derives the CLI command by lowercasing and replacing spaces/underscores with hyphens (e.g."Automation Profile"->"automation-profile")H2 (null guard in
_lifecycle_apply_with_id)After reverting
plan.pyto master, this is a pre-existing issue on master rather than a regression introduced by this PR. It should be addressed in a separate fix.Quality gates
nox -s lint— PASS (0 errors)nox -s typecheck— 10 errors, 1 warning, all inplan.py/plan_lifecycle_service.pydue to API mismatches between master's file versions and the older branch merge-base (e.g.ExecutionEnvPriority,save_plan). These will resolve once the branch is rebased onto master per the PM's Day 37 request. No typecheck errors in CLI polish files (constants.py,errors.py,main.py,__init__.py).Files in final commit
The commit now only modifies files in scope:
src/cleveragents/cli/constants.py(NEW)src/cleveragents/cli/errors.py(NEW)src/cleveragents/cli/main.py(completion command + help text + valid_cmds)src/cleveragents/cli/__init__.py(re-exports)vulture_whitelist.py(new symbols)features/cli_consistency.feature(NEW)features/steps/cli_consistency_steps.py(NEW)robot/cli_consistency.robot(NEW)robot/helper_cli_consistency.py(NEW)Plus
CHANGELOG.md,plan.py,plan_lifecycle_service.py,benchmarks/bench_uko_layer3.py,docs/timeline.md, andfeatures/steps/plan_lifecycle_commands_coverage_steps.py— all matching master exactly (carried forward from the merge-base difference, content identical to master).8654368dc244350731ce44350731ce3687225e1aReview Fixes Applied — Commit
3687225eRebase onto master (resolves C1, C2, H1, H3, H4)
The branch has been rebased onto current
origin/master(ad98d41d). This automatically resolved all critical and high regression findings (C1, C2, H1, H3, H4) because the regressions were caused by stale versions ofplan.pyandplan_lifecycle_service.pybundled into the commit. After rebase, these files match master exactly and are no longer modified by this PR.The branch now modifies only CLI polish files:
src/cleveragents/cli/constants.py(new)src/cleveragents/cli/errors.py(new)src/cleveragents/cli/main.py(completion command + help text)src/cleveragents/cli/__init__.py(re-exports)features/cli_consistency.feature+features/steps/cli_consistency_steps.pyrobot/cli_consistency.robot+robot/helper_cli_consistency.pyvulture_whitelist.py,CHANGELOG.mdCLI-specific fixes
plan_lifecycle_service.pyno longer modifiedplan.pyno longer modifiedlist_actions()DB fallback intactFORMAT_TEXT = "plain"+ updated 2 test assertionsFORMAT_HELPalready includes "color"automation profiles)err_consolewith_LazyErrConsoleproxy## UnreleasedQuality Gates
nox -s lint— PASSnox -s typecheck— PASS (0 errors)ad98d41dCode Review — PR #1018
feat(cli): final CLI polish and UX consistency passThe core concept is sound — standardized exit codes, error formatting utilities, and shell completion are all valuable additions. However, there are several issues that need resolution.
Issues Requiring Changes
Merge conflicts — The PR is not mergeable. The PM has already requested rebase by Day 39 EOD.
Out-of-scope production code changes — Despite the author's force-push to revert unrelated files, the diff still shows substantial changes that are not "CLI polish":
plan_lifecycle_service.py: Addssave_plan(),_resolve_actor_registry_entry(), and enhanced actor resolution in_prepare_and_run_preflight(). These are service-layer features, not CLI polish.plan.py: Adds--execution-env-priorityflag andExecutionEnvPriorityhandling — this is for issue #886, not #861.CHANGELOG.md: Adds entries for #886, #650, #695 which are not part of this PR's scope (#861).These need to be cleanly separated. After rebase, only changes related to issue #861 should remain in this PR.
Test expectation / implementation mismatch —
constants.pydefinesFORMAT_TEXT: str = "plain", but the Behave feature file at line ~80 assertsFORMAT_TEXT should equal "text". This test will fail. Either the constant value or the test expectation needs to be corrected.Minor Notes
cli/constants.pyandcli/errors.pyare clean, well-structured additions.completioncommand shells out to thetyperCLI viasubprocess.runto generate completions rather than using Typer's built-in completion utilities — this works but is more fragile.vulture_whitelist.pyentries is a lot for a polish PR — verify all are genuinely needed.What to Fix
FORMAT_TEXTtest assertion to match the implementation, or vice versa.3687225e1a2a3349df58Rebased onto
origin/master(79b0a2c5). CHANGELOG conflict resolved (kept master, re-added PR entry).nox -s lintPASS,nox -s typecheckPASS (0 errors). Commit2a3349df.2a3349df58bb9ba5898cbb9ba5898c369f97be89Fixed
ruff formatfailure.src/cleveragents/cli/constants.pyhad a multi-line string concatenation forFORMAT_HELPthat ruff wanted as a single line. Appliedruff formatand amended.