feat(cli): Add comprehensive CLI command groups for API coverage - #190
Conversation
Implement complete CLI command structure with 12 command groups: Phase 1 (Core workflows): - auth: login, logout, register, whoami, status - projects: list, create, get, status, tasks, activity, start, pause, resume - blockers: list, resolve, skip, metrics - checkpoints: list, create, restore - discovery: start, progress, answer, restart, generate-prd Phase 2 (Agent & task management): - agents: list, assign, remove, status, role - tasks: list, create, get, update - quality-gates: get, run - metrics: tokens, costs, agent - session: get - context: get, stats, flash-save, checkpoints - review: status, stats, findings, list All commands follow TDD approach with 157 tests passing. Includes APIClient with retry logic, auth token management, and Rich-based terminal output formatting.
WalkthroughAdds a Typer-based CLI (top-level app + many subcommand groups), an HTTP API client with retries and auth/error mapping, JWT token file/env handling, a dashboard serve command with port checks and uvicorn start, numerous command modules (auth, projects, agents, blockers, checkpoints, discovery, tasks, quality-gates, metrics, session, context, review), shared CLI helpers, and comprehensive tests for the CLI surface. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as Typer CLI (app)
participant Auth as Local Auth (file / env)
participant API as Backend API
participant Server as Uvicorn Server
participant Browser as System Browser
rect rgb(230,245,255)
Note right of CLI: Login + token persistence
User->>CLI: login --email user --password pwd
CLI->>API: POST /auth/jwt/login
API-->>CLI: 200 {access_token: jwt}
CLI->>Auth: store_token(jwt)
Auth-->>CLI: token persisted
CLI-->>User: "Login successful"
end
rect rgb(245,255,230)
Note right of CLI: Authenticated API call flow
User->>CLI: projects list
CLI->>Auth: get_token() -> jwt
CLI->>API: GET /api/projects (Authorization: Bearer jwt)
API-->>CLI: 200 [projects]
CLI-->>User: formatted table/json
end
rect rgb(255,245,230)
Note right of CLI: Serve dashboard flow
User->>CLI: serve --port 8080 --open-browser
CLI->>CLI: validate port range & availability
CLI->>Server: start uvicorn host:port (reload?)
Server-->>CLI: running
alt open_browser true
CLI->>Browser: open http://host:port
end
CLI-->>User: "Serving on http://host:port"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review: CLI Comprehensive API CoverageI've reviewed this comprehensive CLI implementation. Overall, this is excellent work with strong test coverage (157 passing tests), clean architecture, and good adherence to project standards. Here are my findings: ✅ Strengths
🔍 Issues Found1. Security: Credentials File Permissions (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @codeframe/cli/__init__.py:
- Around line 168-174: The top-level stub command checkpoint in the CLI (the
function checkpoint decorated with @app.command()) conflicts with the full
checkpoints sub-app; remove the redundant stub or convert it into a deprecation
shim: delete or comment out the checkpoint function entirely, or modify it to
print a clear deprecation/warning message directing users to use the checkpoints
subcommand (e.g., "Use `codeframe checkpoints ...`") and return a non-error
exit, ensuring you reference the existing checkpoint function name and
@app.command() decorator when making the change.
🧹 Nitpick comments (27)
tests/cli/test_auth_commands.py (1)
1-223: Excellent test coverage for auth commands!The test suite is comprehensive and well-structured:
- Covers all auth commands (login, logout, register, whoami)
- Tests both success and error paths
- Validates prompting behavior
- Properly isolates tests with mocking and tmp_path
- Verifies token storage and retrieval side effects
Optional: Consider extracting common test fixtures.
The credential setup pattern is repeated across many tests. Consider creating a pytest fixture to reduce duplication:
💡 Optional refactor to reduce duplication
@pytest.fixture def mock_credentials(tmp_path): """Fixture providing mocked credentials path with valid token.""" creds_path = tmp_path / ".codeframe" / "credentials.json" creds_path.parent.mkdir(parents=True) with open(creds_path, "w") as f: json.dump({"access_token": "valid-token"}, f) with patch("codeframe.cli.auth.get_credentials_path", return_value=creds_path): yield creds_pathThen tests can use:
def test_whoami_authenticated(self, mock_credentials):tests/cli/test_metrics_commands.py (1)
1-105: LGTM! Well-structured metrics command tests.The test suite properly validates the metrics commands:
- Tests tokens, costs, and agent metrics endpoints
- Mocks API responses appropriately
- Verifies output contains expected data
- Properly isolates tests with credential mocking
Optional: Similar duplication pattern as auth tests.
The credential setup code is repeated across test methods. Consider using the same fixture pattern suggested in test_auth_commands.py to reduce duplication across the entire test suite.
tests/cli/test_agents_commands.py (1)
1-201: Excellent agent command test coverage!The test suite thoroughly validates agent management operations:
- Covers all CRUD operations for agents
- Tests interactive prompts (remove without --force)
- Validates request payloads (lines 109-110)
- Handles edge cases like empty agent lists
- Properly tests role assignment and updates
The verification of request payload at lines 109-110 is particularly good practice, ensuring the role parameter is properly sent to the API.
Optional: Extract credential setup fixture.
Same duplication pattern noted in previous files. A shared fixture would improve maintainability across the entire CLI test suite.
tests/cli/test_quality_gates_commands.py (1)
21-111: Consider extracting credentials setup to a pytest fixture.The credentials file creation pattern is repeated in all test methods. A fixture would reduce duplication and improve maintainability:
💡 Suggested refactor using pytest fixture
Add a fixture at the module level:
@pytest.fixture def mock_credentials(tmp_path): """Create mock credentials file.""" creds_path = tmp_path / ".codeframe" / "credentials.json" creds_path.parent.mkdir(parents=True) with open(creds_path, "w") as f: json.dump({"access_token": "valid-token"}, f) return creds_pathThen simplify each test:
- def test_get_quality_gates_success(self, tmp_path): + def test_get_quality_gates_success(self, mock_credentials): """Get should display quality gate status for task.""" - creds_path = tmp_path / ".codeframe" / "credentials.json" - creds_path.parent.mkdir(parents=True) - with open(creds_path, "w") as f: - json.dump({"access_token": "valid-token"}, f) mock_response = MagicMock() # ... rest of test ... - with patch("codeframe.cli.auth.get_credentials_path", return_value=creds_path): + with patch("codeframe.cli.auth.get_credentials_path", return_value=mock_credentials):Apply this pattern to all test methods.
tests/cli/test_context_commands.py (1)
21-128: Apply the same fixture refactoring as suggested for quality_gates tests.This file follows the same pattern of duplicating credentials setup across all test methods. The pytest fixture approach suggested for
test_quality_gates_commands.pyapplies here as well.tests/cli/test_api_client.py (1)
25-31: Remove redundantos.environ.pop()call.The
patch.dict(os.environ, {}, clear=True)already clears all environment variables, making the subsequentos.environ.pop("CODEFRAME_API_URL", None)redundant.🔎 Suggested simplification
def test_default_url(self): """Default URL should be localhost:8080.""" with patch.dict(os.environ, {}, clear=True): - # Remove any existing CODEFRAME_API_URL - os.environ.pop("CODEFRAME_API_URL", None) url = get_api_base_url() assert url == "http://localhost:8080"tests/cli/test_session_commands.py (1)
21-62: Apply credentials fixture pattern to reduce duplication.As with the other test files, the credentials setup is duplicated. Consider using the pytest fixture approach suggested in
test_quality_gates_commands.py.tests/cli/test_checkpoint_commands.py (1)
21-284: Apply credentials fixture pattern across all 11 test methods.This file has the most duplication of the credentials setup pattern. The pytest fixture approach would provide the greatest benefit here, reducing 11 repetitions to a single fixture definition.
codeframe/cli/metrics_commands.py (1)
33-38: Extract duplicatedrequire_authhelper to a shared module.The
require_authfunction is duplicated across at least 8 CLI command modules (metrics, session, review, context, project, agents, discovery, checkpoint). This violates the DRY principle and makes maintenance harder.🔎 Recommended refactor
Create a shared utility module (e.g.,
codeframe/cli/utils.pyorcodeframe/cli/common.py):# codeframe/cli/utils.py from rich.console import Console import typer from codeframe.cli.api_client import APIClient console = Console() def require_auth(client: APIClient): """Check if client is authenticated, exit with error if not.""" if not client.token: console.print("[yellow]Not logged in.[/yellow]") console.print("Please log in: codeframe auth login") raise typer.Exit(1)Then import it in each command module:
-def require_auth(client: APIClient): - """Check if client is authenticated, exit with error if not.""" - if not client.token: - console.print("[yellow]Not logged in.[/yellow]") - console.print("Please log in: codeframe auth login") - raise typer.Exit(1) +from codeframe.cli.utils import require_authBased on relevant code snippets showing identical implementations across multiple modules.
codeframe/cli/auth.py (1)
32-57: Minor: Consider atomic file creation with secure permissions.There's a brief window between file creation and
chmod(0o600)where the file has default permissions. For a CLI tool storing tokens in the user's home directory, this is acceptable, but for higher-security contexts, consider atomic creation.🔎 Optional: Atomic secure file creation
- # Write to file - with open(creds_path, "w") as f: - json.dump(credentials, f, indent=2) - - # Set secure permissions (owner read/write only) - creds_path.chmod(0o600) + import os + # Atomic creation with secure permissions + fd = os.open(creds_path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + try: + with os.fdopen(fd, "w") as f: + json.dump(credentials, f, indent=2) + except: + os.close(fd) + raisecodeframe/cli/quality_gates_commands.py (1)
33-39: Consider extractingrequire_authto a shared module.The
require_authfunction is duplicated across multiple command modules (tasks_commands.py,agents_commands.py, etc.). Consider extracting it to a shared utilities module to maintain DRY principles.Based on the relevant code snippets, this same function exists in
codeframe/cli/tasks_commands.pyat lines 35-40.🔎 Proposed refactor
Create a shared module (e.g.,
codeframe/cli/utils.py):# codeframe/cli/utils.py import typer from rich.console import Console from codeframe.cli.api_client import APIClient console = Console() def require_auth(client: APIClient): """Check if client is authenticated, exit with error if not.""" if not client.token: console.print("[yellow]Not logged in.[/yellow]") console.print("Please log in: codeframe auth login") raise typer.Exit(1)Then import in each command module:
from codeframe.cli.utils import require_authcodeframe/cli/discovery_commands.py (2)
23-23: Unused imports fromrich.progress.
Progress,SpinnerColumn, andTextColumnare imported but never used in this module. Consider removing them to keep the imports clean.🔎 Proposed fix
-from rich.progress import Progress, SpinnerColumn, TextColumn
38-43: Duplicaterequire_authhelper across CLI modules.This exact function is duplicated in at least 7 modules (agents_commands, metrics_commands, session_commands, context_commands, checkpoint_commands, blocker_commands, project_commands). Consider extracting it to a shared utility module (e.g.,
codeframe/cli/utils.py) to improve maintainability.🔎 Suggested shared utility
Create
codeframe/cli/utils.py:import typer from rich.console import Console from codeframe.cli.api_client import APIClient console = Console() def require_auth(client: APIClient): """Check if client is authenticated, exit with error if not.""" if not client.token: console.print("[yellow]Not logged in.[/yellow]") console.print("Please log in: codeframe auth login") raise typer.Exit(1)Then import in each command module:
from codeframe.cli.utils import require_authcodeframe/cli/checkpoint_commands.py (2)
38-43: Duplicaterequire_authhelper.Same duplication issue as noted in discovery_commands.py. Extract to a shared utility.
205-242: Delete command uses--forcebut restore uses--confirm- consider consistency.The
deletecommand uses--forceto skip confirmation, whilerestoreuses--confirmto perform the action. This inconsistency may confuse users. Consider aligning the flag semantics or documenting the difference clearly.codeframe/cli/__init__.py (2)
232-245: Fixed 1.5s delay before opening browser may be insufficient.The hardcoded
time.sleep(1.5)assumes the server starts within 1.5 seconds. On slower machines or under load, this may fail. Consider implementing a health-check loop with timeout instead.🔎 Suggested improvement
def open_in_browser(): """Open browser after server is ready.""" import urllib.request max_wait = 10 # seconds for _ in range(max_wait * 2): try: urllib.request.urlopen(f"http://localhost:{port}/health", timeout=0.5) webbrowser.open(f"http://localhost:{port}") return except Exception: time.sleep(0.5) # Fallback: try anyway webbrowser.open(f"http://localhost:{port}")
298-312: Imports after code violates PEP 8 convention.While this works, placing imports at the bottom of the file after function definitions is unconventional. Consider moving these imports to the top of the file with a comment explaining they're deferred to avoid circular imports (if that's the reason), or restructure to follow standard conventions.
tests/cli/test_discovery_commands.py (1)
21-42: Consider using a pytest fixture to reduce credential setup duplication.The credential file creation code is repeated in every test method. A shared fixture would improve maintainability.
🔎 Suggested fixture
@pytest.fixture def mock_credentials(tmp_path): """Create mock credentials file and return the path.""" creds_path = tmp_path / ".codeframe" / "credentials.json" creds_path.parent.mkdir(parents=True) with open(creds_path, "w") as f: json.dump({"access_token": "valid-token"}, f) return creds_pathThen use in tests:
def test_start_discovery_success(self, mock_credentials): mock_response = MagicMock() # ... with patch("codeframe.cli.auth.get_credentials_path", return_value=mock_credentials): # ...codeframe/cli/blocker_commands.py (1)
37-42: Duplicaterequire_authhelper.Same duplication issue as noted previously. Extract to a shared utility.
codeframe/cli/api_client.py (2)
162-220:last_exceptionis captured but never used.The variable
last_exceptionis assigned on lines 198 and 208 but is never used in the final error message. Consider including it for debugging purposes or remove the variable.🔎 Proposed fix
# All retries exhausted raise APIError( f"Connection error: Unable to connect to {self.base_url}. " - "Please check the server is running and try again.", + f"Please check the server is running and try again. Last error: {last_exception}", status_code=None, )
197-213: Consider adding jitter to exponential backoff to prevent thundering herd.When multiple CLI instances retry simultaneously (e.g., in CI), pure exponential backoff can cause synchronized retry storms. Adding random jitter is a common best practice.
🔎 Suggested improvement
import random # In the retry loop: if attempt < self.max_retries - 1: # Exponential backoff with jitter base_delay = 2 ** attempt jitter = random.uniform(0, base_delay * 0.1) time.sleep(base_delay + jitter)codeframe/cli/project_commands.py (2)
41-46: Duplicaterequire_authhelper.Same duplication issue as noted in other modules. Extract to a shared utility.
99-102: Redundant login hint after AuthenticationError.The
AuthenticationErrormessage already includes login guidance (per api_client.py line 129). The additionalconsole.printon line 101 is redundant.🔎 Proposed fix
except AuthenticationError as e: console.print(f"[red]Authentication error:[/red] {e}") - console.print("Please log in: codeframe auth login") raise typer.Exit(1)tests/cli/test_blocker_commands.py (4)
15-15: Consider using a pytest fixture for the runner.While the module-level
runnerworks, a pytest fixture would be more consistent with patterns used elsewhere (see test_cli_session.py) and better for test isolation.🔎 Suggested refactor
-runner = CliRunner() + +@pytest.fixture +def runner(): + """Create CLI test runner.""" + return CliRunner()Then update test methods to accept
runneras a parameter.
18-84: Consider a shared fixture for credential setup.The credential file setup pattern (lines 23-26, 49-52, 67-70) is repeated across all tests. Extract this into a pytest fixture to reduce duplication.
🔎 Suggested fixture
@pytest.fixture def mock_credentials(tmp_path): """Create mock credentials file.""" creds_path = tmp_path / ".codeframe" / "credentials.json" creds_path.parent.mkdir(parents=True) with open(creds_path, "w") as f: json.dump({"access_token": "valid-token"}, f) return creds_pathThen use it in tests:
def test_list_blockers_success(self, mock_credentials): # ... with patch("codeframe.cli.auth.get_credentials_path", return_value=mock_credentials): # ...
139-160: Consider verifying the request payload.The test confirms the command succeeds but doesn't verify that the answer "Use PostgreSQL" is included in the request body sent to the API. While not critical, this would make the test more thorough.
🔎 Example verification
with patch("codeframe.cli.auth.get_credentials_path", return_value=creds_path): with patch("requests.request", return_value=mock_response) as mock_request: result = runner.invoke(blockers_app, ["resolve", "1", "Use PostgreSQL"]) assert result.exit_code == 0 assert "resolved" in result.output.lower() + + # Verify the answer was sent to the API + call_kwargs = mock_request.call_args.kwargs + assert call_kwargs.get("json", {}).get("answer") == "Use PostgreSQL"
229-231: Consider more specific output assertions.The assertions check for "20" and "18" or "resolved" anywhere in the output, which are quite permissive. The test would pass even if the metrics aren't formatted correctly.
🔎 More specific assertions
assert result.exit_code == 0 - assert "20" in result.output # total blockers - assert "18" in result.output or "resolved" in result.output.lower() + # Check for specific metrics in output + assert "total" in result.output.lower() and "20" in result.output + assert "resolved" in result.output.lower() and "18" in result.output + assert "pending" in result.output.lower() and "1" in result.outputThis ensures the labels appear alongside the values, not just the values alone.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
codeframe/cli/__init__.pycodeframe/cli/agents_commands.pycodeframe/cli/api_client.pycodeframe/cli/auth.pycodeframe/cli/auth_commands.pycodeframe/cli/blocker_commands.pycodeframe/cli/checkpoint_commands.pycodeframe/cli/context_commands.pycodeframe/cli/discovery_commands.pycodeframe/cli/metrics_commands.pycodeframe/cli/project_commands.pycodeframe/cli/quality_gates_commands.pycodeframe/cli/review_commands.pycodeframe/cli/session_commands.pycodeframe/cli/tasks_commands.pytests/cli/__init__.pytests/cli/test_agents_commands.pytests/cli/test_api_client.pytests/cli/test_auth_commands.pytests/cli/test_auth_module.pytests/cli/test_blocker_commands.pytests/cli/test_checkpoint_commands.pytests/cli/test_context_commands.pytests/cli/test_discovery_commands.pytests/cli/test_metrics_commands.pytests/cli/test_project_commands.pytests/cli/test_quality_gates_commands.pytests/cli/test_review_commands.pytests/cli/test_session_commands.pytests/cli/test_tasks_commands.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/cli/metrics_commands.pycodeframe/cli/session_commands.pycodeframe/cli/auth_commands.pycodeframe/cli/api_client.pycodeframe/cli/__init__.pycodeframe/cli/auth.pycodeframe/cli/quality_gates_commands.pycodeframe/cli/blocker_commands.pycodeframe/cli/context_commands.pycodeframe/cli/tasks_commands.pycodeframe/cli/discovery_commands.pycodeframe/cli/checkpoint_commands.pycodeframe/cli/review_commands.pycodeframe/cli/project_commands.pycodeframe/cli/agents_commands.py
🧠 Learnings (4)
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/**/*.py : Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Applied to files:
tests/cli/test_quality_gates_commands.pytests/cli/test_review_commands.pycodeframe/cli/quality_gates_commands.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/**/*.py : Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Applied to files:
codeframe/cli/session_commands.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : Organize Python backend files with Auth module at codeframe/auth/ containing dependencies.py (get_current_user), manager.py (UserManager), models.py, router.py, and schemas.py
Applied to files:
codeframe/cli/auth_commands.pycodeframe/cli/api_client.pycodeframe/cli/auth.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : For authentication, use FastAPI Users with JWT tokens and mandatory authentication (no bypass mode)
Applied to files:
codeframe/cli/auth_commands.pycodeframe/cli/auth.py
🧬 Code graph analysis (22)
tests/cli/test_quality_gates_commands.py (1)
codeframe/cli/quality_gates_commands.py (1)
get(42-111)
tests/cli/test_context_commands.py (3)
codeframe/cli/api_client.py (1)
patch(258-268)tests/cli/test_cli_session.py (1)
runner(19-21)tests/cli/test_review_commands.py (1)
test_stats_success(71-93)
tests/cli/test_checkpoint_commands.py (2)
codeframe/cli/api_client.py (1)
patch(258-268)tests/cli/test_cli_session.py (1)
runner(19-21)
codeframe/cli/metrics_commands.py (2)
codeframe/cli/api_client.py (4)
APIClient(54-279)APIError(29-35)AuthenticationError(38-41)get(222-232)codeframe/cli/project_commands.py (2)
require_auth(41-46)get(171-212)
tests/cli/test_auth_module.py (1)
codeframe/cli/auth.py (5)
get_credentials_path(23-29)store_token(32-56)get_token(59-99)clear_token(102-113)is_authenticated(116-122)
tests/cli/test_agents_commands.py (3)
codeframe/cli/api_client.py (1)
patch(258-268)tests/cli/test_cli_session.py (1)
runner(19-21)tests/cli/test_project_commands.py (1)
test_status_success(212-239)
tests/cli/test_review_commands.py (1)
tests/cli/test_cli_session.py (1)
runner(19-21)
tests/cli/test_api_client.py (1)
codeframe/cli/api_client.py (8)
APIError(29-35)AuthenticationError(38-41)get_api_base_url(44-51)_get_headers(76-87)get(222-232)post(234-244)delete(270-279)put(246-256)
tests/cli/test_auth_commands.py (3)
codeframe/cli/api_client.py (1)
patch(258-268)tests/cli/test_cli_session.py (1)
runner(19-21)codeframe/core/config.py (1)
load(223-235)
codeframe/cli/auth.py (2)
codeframe/cli/api_client.py (1)
get(222-232)codeframe/cli/context_commands.py (1)
get(44-88)
tests/cli/test_project_commands.py (2)
codeframe/cli/api_client.py (1)
get(222-232)codeframe/cli/project_commands.py (1)
get(171-212)
codeframe/cli/quality_gates_commands.py (2)
codeframe/cli/api_client.py (4)
APIClient(54-279)APIError(29-35)AuthenticationError(38-41)get(222-232)codeframe/cli/tasks_commands.py (2)
require_auth(36-41)get(182-224)
tests/cli/test_blocker_commands.py (2)
tests/cli/test_cli_session.py (1)
runner(19-21)codeframe/cli/blocker_commands.py (1)
get(116-171)
codeframe/cli/blocker_commands.py (1)
codeframe/cli/api_client.py (5)
APIClient(54-279)APIError(29-35)AuthenticationError(38-41)get(222-232)post(234-244)
tests/cli/test_metrics_commands.py (2)
codeframe/cli/api_client.py (1)
patch(258-268)tests/cli/test_cli_session.py (1)
runner(19-21)
codeframe/cli/context_commands.py (5)
codeframe/cli/api_client.py (5)
APIClient(54-279)APIError(29-35)AuthenticationError(38-41)get(222-232)post(234-244)codeframe/cli/agents_commands.py (1)
require_auth(38-43)codeframe/cli/checkpoint_commands.py (2)
require_auth(38-43)get(153-202)codeframe/cli/discovery_commands.py (1)
require_auth(38-43)codeframe/cli/project_commands.py (2)
require_auth(41-46)get(171-212)
tests/cli/test_tasks_commands.py (2)
codeframe/cli/api_client.py (2)
patch(258-268)get(222-232)codeframe/cli/tasks_commands.py (1)
get(182-224)
codeframe/cli/tasks_commands.py (1)
codeframe/cli/api_client.py (6)
APIClient(54-279)APIError(29-35)AuthenticationError(38-41)get(222-232)post(234-244)patch(258-268)
codeframe/cli/discovery_commands.py (8)
codeframe/cli/api_client.py (5)
APIClient(54-279)APIError(29-35)AuthenticationError(38-41)post(234-244)get(222-232)codeframe/cli/agents_commands.py (1)
require_auth(38-43)codeframe/cli/blocker_commands.py (2)
require_auth(37-42)get(116-171)codeframe/cli/checkpoint_commands.py (2)
require_auth(38-43)get(153-202)codeframe/cli/context_commands.py (2)
require_auth(35-40)get(44-88)codeframe/cli/metrics_commands.py (1)
require_auth(33-38)codeframe/cli/project_commands.py (3)
require_auth(41-46)start(374-401)get(171-212)codeframe/cli/session_commands.py (1)
require_auth(34-39)
codeframe/cli/checkpoint_commands.py (2)
codeframe/cli/api_client.py (3)
APIClient(54-279)APIError(29-35)AuthenticationError(38-41)codeframe/cli/tasks_commands.py (1)
require_auth(36-41)
codeframe/cli/project_commands.py (1)
codeframe/cli/api_client.py (5)
APIClient(54-279)APIError(29-35)AuthenticationError(38-41)get(222-232)post(234-244)
codeframe/cli/agents_commands.py (1)
codeframe/cli/api_client.py (7)
APIClient(54-279)APIError(29-35)AuthenticationError(38-41)get(222-232)post(234-244)delete(270-279)put(246-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (74)
tests/cli/__init__.py (1)
1-1: LGTM! Clean package initializer.The package initializer correctly follows Python conventions with a descriptive docstring and no unnecessary code.
tests/cli/test_auth_module.py (1)
1-228: Excellent comprehensive unit tests for auth utilities!The test suite provides thorough coverage of all auth utility functions:
Security considerations properly tested:
- Lines 99-108: Validates file permissions are restricted to 0o600 (owner read/write only)
Environment variable precedence verified:
- Lines 162-174: Confirms CODEFRAME_TOKEN env var takes precedence over file-based credentials
Edge cases well covered:
- Invalid JSON handling
- Missing keys in JSON
- Non-existent files
- Parent directory creation
- Token overwriting
The test structure is excellent for TDD, validating each function's behavior comprehensively without unnecessary duplication.
tests/cli/test_quality_gates_commands.py (1)
94-111: Good practice validating the gate filter in the request payload.The test correctly verifies that the
--gateoption is passed through to the API request body, ensuring the CLI properly forwards user options.tests/cli/test_api_client.py (1)
278-293: Excellent retry logic testing.The use of
side_effectwith a list to simulate a failed then successful request is a clean way to test retry behavior.tests/cli/test_checkpoint_commands.py (2)
192-208: Good use ofinputparameter to test interactive prompts.The test correctly simulates user confirmation by passing
input="y\n"to the CLI runner, ensuring the interactive prompt path is tested without requiring actual user interaction.
93-113: Thorough validation of optional parameter propagation.The test correctly verifies that the
--descriptionoption is passed through to the API by inspecting the call arguments. This ensures CLI options are properly mapped to API requests.codeframe/cli/metrics_commands.py (4)
41-43: LGTM!The
format_numberhelper correctly formats numbers with thousand separators using Python's built-in formatting. This improves readability of large token counts and costs.
46-106: LGTM!The
tokenscommand is well-implemented with:
- Clear authentication enforcement
- Proper error handling for 404 and other API errors
- Support for both text and JSON output formats
- Clean, user-friendly text formatting with token breakdown by agent
108-167: LGTM!The
costscommand properly implements cost metrics display with:
- Appropriate USD formatting (2 decimal places)
- Safe slicing of the last 7 days (handles lists shorter than 7)
- Consistent error handling pattern
- Clear daily breakdown table
170-215: LGTM!The
agentcommand correctly implements agent-specific metrics with comprehensive statistics display (total tokens, cost, tasks completed, average tokens per task) and consistent error handling.codeframe/cli/session_commands.py (1)
42-85: LGTM!The
get_sessioncommand is well-implemented with:
- Clean session state display with color-coded status (green for active, yellow otherwise)
- Friendly handling of 404 (no active session) vs other errors
- Support for both text and JSON output formats
- Consistent error handling pattern
codeframe/cli/review_commands.py (5)
43-52: LGTM!The
severity_emojihelper provides good visual indicators for severity levels with appropriate color coding (🔴 critical, 🟠 high, 🟡 medium, 🔵 low, ⚪ info) and a fallback for unknown values.
55-109: LGTM!The
statuscommand correctly implements review status display with:
- Appropriate color coding for different review states (approved, changes_requested, rejected)
- Graceful handling of tasks without reviews
- Clean output showing status, score, and findings count
112-160: LGTM!The
statscommand provides a clear overview of project-wide review statistics with appropriate visual indicators (✅ approved, 🔄 changes requested, ❌ rejected) and aggregate metrics.
163-243: LGTM!The
findingscommand is well-designed with:
- Optional severity filtering via query parameter
- Comprehensive summary (total findings, blocking status, severity breakdown)
- Appropriate truncation of file paths (last 25 chars) and messages (40 chars) for readable table display
- Clear visual indicators using severity emojis
246-328: LGTM!The
list_reviewscommand appropriately extends the findings display to project-wide scope with:
- Task ID column for cross-referencing
- Adjusted column widths to fit additional information
- Consistent filtering and display patterns
- Clear summary and breakdown of findings
codeframe/cli/context_commands.py (4)
43-88: LGTM! Aligns with tiered memory architecture.The
getcommand correctly displays agent context with tier distribution (HOT/WARM/COLD) using appropriate visual indicators (🔴 🟠 🔵). This aligns with the tiered memory system specified in the coding guidelines.As per coding guidelines: "Use tiered memory system (HOT/WARM/COLD) with importance scoring."
91-146: LGTM!The
statscommand provides detailed tier-level statistics with a clean table format showing item counts and token usage for each tier (hot, warm, cold), supporting effective context management monitoring.
149-180: LGTM! Supports session checkpoint management.The
flash_savecommand correctly creates context checkpoints, aligning with the session lifecycle management guidelines. Clear success feedback includes checkpoint ID and timestamp for reference.Based on learnings: "Implement session lifecycle management with auto-save/restore using file-based storage."
183-237: LGTM!The
checkpointscommand correctly lists saved context checkpoints with:
- Clear table display showing checkpoint ID, timestamp, and item count
- Helpful guidance when no checkpoints exist
- Clean timestamp formatting (ISO → human-readable)
tests/cli/test_tasks_commands.py (4)
18-65: LGTM!The list command tests provide good coverage:
- Success case verifies task titles appear in output
- Status filter test confirms query parameters are correctly passed to the API
- Proper mocking of credentials and API responses
67-116: LGTM!The create command tests appropriately verify:
- Basic task creation with 201 response handling
- Optional parameters (priority, description) are correctly included in the JSON payload
- Success messages appear in output
118-165: LGTM!The get command tests properly cover:
- Success case with task details displayed
- 404 error case with appropriate error message and non-zero exit code
167-189: LGTM!The update command test verifies the status update operation works correctly with appropriate success messaging.
tests/cli/test_project_commands.py (4)
1-16: LGTM! Well-structured test setup.The test module follows a clean TDD approach with proper imports and a shared
CliRunnerinstance. The pattern of usingtmp_pathfor credential isolation is appropriate for testing authentication-dependent commands.
18-85: LGTM! Comprehensive list command tests.Good coverage of success, empty state, and JSON format scenarios. The assertions appropriately verify both exit codes and output content.
87-158: LGTM! Solid create command test coverage.The tests properly verify success, options passing to API, and conflict handling. The API payload verification in
test_create_project_with_optionsis particularly valuable for ensuring correct data transmission.
160-373: LGTM! Consistent test coverage across all project commands.The remaining test classes maintain the established patterns. Notable positives:
- 404 error handling is verified
- Status filter is tested with API parameter validation
- Lifecycle commands (start/pause/resume) verify expected output messages
tests/cli/test_review_commands.py (2)
1-16: LGTM! Review command tests follow established patterns.Consistent with the project commands test module structure, using the same credential mocking and response simulation approach.
18-207: LGTM! Comprehensive review command test coverage.Good test coverage including:
- Both existing and missing review states
- Empty findings scenario with appropriate messaging
- Security/quality finding categories in the mock data
codeframe/cli/auth.py (2)
1-29: LGTM! Clear module documentation and credential path handling.The docstring clearly explains the module's responsibilities and security considerations. The
get_credentials_pathfunction correctly usesPath.home()for cross-platform compatibility.
59-122: LGTM! Robust token retrieval with proper error handling.The
get_tokenfunction correctly:
- Prioritizes environment variable for CI/CD flexibility
- Handles missing files, invalid JSON, and read errors gracefully
- Uses appropriate logging levels (debug for normal flow, warning for errors)
The
is_authenticatedfunction provides a clean abstraction.codeframe/cli/quality_gates_commands.py (2)
41-111: LGTM! Well-implemented quality gates display.The command provides:
- Clear status visualization with color coding and emoji
- Proper handling of empty gates configuration
- Consistent error handling patterns
This aligns with the coding guidelines for implementing quality gates with multi-stage pre-completion checks.
114-154: LGTM! Clean quality gate trigger implementation.Good UX with the follow-up command hint (
codeframe quality-gates get {task_id}) to check status after triggering.codeframe/cli/tasks_commands.py (4)
1-43: LGTM! Solid module setup with clear documentation.The module docstring provides helpful usage examples, and the Typer app is configured correctly with
no_args_is_help=True.Note:
require_authduplication was addressed in the quality_gates_commands.py review.
44-128: LGTM! Well-implemented list command with filtering.Good attention to detail:
- Correctly uses
priority is not None(line 71) to allow priority 0 filtering- Priority emoji labels provide good visual hierarchy
- Title truncation prevents table overflow
130-179: LGTM! Create command with sensible defaults.Good defaults for priority (3 = Normal) and status ("pending").
181-280: LGTM! Get and update commands are well-implemented.The update command correctly:
- Uses PATCH for partial updates
- Validates that at least one field is specified before making the API call
- Uses
priority is not None(line 252) to allow setting priority to 0codeframe/cli/agents_commands.py (4)
1-44: LGTM! Agent management module setup.Clear documentation with usage examples for all commands. The Typer app is properly configured.
46-148: LGTM! List and assign commands are well-implemented.Good handling of edge cases:
- Empty agent list shows helpful guidance
- 409 conflict provides clear feedback for duplicate assignments
- Date display is cleanly truncated to YYYY-MM-DD format
150-191: LGTM! Safe remove command with confirmation.The confirmation prompt provides a safety net, and
--forceallows scripting. TheExit(0)on cancellation correctly indicates user-initiated abort.
193-287: LGTM! Status and role commands are well-structured.The
statuscommand provides a useful view of agent assignments across projects, and therolecommand correctly uses PUT for the role update endpoint.codeframe/cli/auth_commands.py (5)
1-34: LGTM! Well-documented auth module with proper imports.The module correctly imports from both
auth.py(token storage) andapi_client.py(API communication).
36-111: LGTM! Login command correctly implements FastAPI Users authentication.Key implementation details are correct:
- Uses form-encoded POST with
usernamefield (FastAPI Users convention)- Password input is hidden via
hide_input=True- Comprehensive error handling for different HTTP status codes
113-125: LGTM! Simple and effective logout.
127-222: LGTM! Comprehensive register command with auto-login.Good UX decisions:
- Password confirmation only when prompting interactively (allows scripting)
- Auto-login after registration reduces friction
- Detailed validation error display helps users fix issues
The
ALREADY_EXISTScheck using string matching (line 193) is reasonable given FastAPI Users' error response format.
224-257: LGTM! Whoami command with good session handling.The two-step approach (local auth check, then API call) provides better UX:
- Fails fast if no token exists locally
- Detects expired/invalid tokens via API response
codeframe/cli/discovery_commands.py (5)
83-165: Progress command implementation looks good.The progress command properly handles multiple states (idle, discovering, completed), renders a visual progress bar, shows the current question with a panel, and provides helpful next-step hints. Error handling for 404 and authentication is consistent with other commands.
168-226: Answer command implementation is solid.Properly submits the answer, displays progress updates, handles completion state with PRD generation hint, and provides appropriate error handling for "not active" discovery state. The error parsing on line 217 correctly extracts the detail message.
229-281: Restart command correctly implements confirmation flow.The
--forceflag bypasses confirmation appropriately, and the interactive prompt usingtyper.confirmis well-implemented. Error handling distinguishes between "already completed" (400) and "not found" (404) cases.
284-324: Generate PRD command implementation is correct.Handles the success case with appropriate messaging and provides clear guidance for error states (discovery not complete, project not found).
46-80: Bothcodeframe discovery startandcodeframe projects startcommands do use the same/api/projects/{project_id}/startendpoint (implemented inagents.py). This is intentional per cf-10.2 specification—the single endpoint manages the unified project and discovery lifecycle, checking discovery state to determine whether to initiate discovery or return an already-in-progress status. No separate/api/projects/{project_id}/discovery/startendpoint exists in the discovery router, which only exposes/answer,/progress,/restart, and/generate-prd. The current design is working as specified and does not require changes.codeframe/cli/checkpoint_commands.py (5)
46-102: List checkpoints implementation is correct.Properly handles empty list case with helpful guidance, renders table with truncated git commit and date, and uses consistent error handling patterns.
105-149: Create checkpoint correctly builds payload and displays result.The optional description is conditionally added, and the response is properly formatted with truncated git commit display. The restore hint at the end is helpful.
152-202: Get checkpoint implementation handles metadata well.The conditional display of cost (line 190-191) and tasks metadata is nicely formatted. Error handling for 404 is appropriate.
245-296: Restore command with preview mode is well-designed.The dual-mode behavior (preview without
--confirm, actual restore with--confirm) is a good UX pattern for destructive operations. The diff preview using Rich Syntax highlighting is appropriate.
299-345: Diff command implementation is correct.Statistics display is clear with color-coded insertions/deletions, and the diff content is rendered with syntax highlighting.
codeframe/cli/__init__.py (3)
38-165: Top-level commands (init, start, pause, resume, status, chat, config) look correct.These commands operate on local projects via the
Projectclass and have consistent error handling patterns. Theconfigcommand properly validates the action argument.
180-259: Serve command is well-implemented with port validation and availability check.Good use of
validate_port_rangeandcheck_port_availabilitybefore starting. The error messages for common issues (port in use, uvicorn not found) are helpful.
314-328: Sub-app registration is complete and consistent.All 12 command groups are properly registered with descriptive help text.
tests/cli/test_discovery_commands.py (1)
1-341: Test coverage is comprehensive.The tests cover:
- Success paths for all 5 commands
- Error states (409 conflict, 400 bad request, 404 not found)
- Interactive confirmation flow (restart without --force)
- State transitions (idle → discovering → completed)
Good TDD approach as documented.
codeframe/cli/blocker_commands.py (4)
45-112: List blockers implementation is correct with helpful guidance.The pending blockers hint at line 104 is a nice UX touch. Table formatting with status colors is well done.
115-171: Get blocker with panel display is well-implemented.Good use of Rich Panel for the question display, and the conditional resolve hint for PENDING status is helpful.
174-210: Resolve command handles edge cases well.The 409 (already resolved) case provides a clear non-error message with yellow styling, appropriately distinguishing it from true errors while still exiting with code 1.
213-271: Metrics command with time formatting logic is well-designed.The tiered time formatting (seconds → minutes → hours) on lines 252-257 and the color-coded expiration rate are good UX decisions.
codeframe/cli/api_client.py (4)
29-41: Exception classes are well-designed.
APIErrorwithstatus_codeanddetailattributes, andAuthenticationErroras a subclass for 401/403, provides good structure for error handling in calling code.
54-74: APIClient initialization is clean.Good use of defaults with environment variable override for base URL, and automatic token loading from storage when not provided.
102-160: Response handling is comprehensive.Properly handles 2xx (with JSON parsing fallback), 401/403 with AuthenticationError, 4xx with detail extraction, and 5xx errors. The fallback to
response.textwhen JSON parsing fails is appropriate.
222-279: HTTP method wrappers are clean and consistent.All methods properly delegate to
_request_with_retrywith appropriate kwargs.codeframe/cli/project_commands.py (5)
170-212: Get project implementation is correct.Properly handles text and JSON formats, displays optional workspace path, and has appropriate 404 handling.
215-260: Status command with progress bar is well-implemented.The visual progress bar rendering matches the pattern used in discovery_commands.py, maintaining consistency.
263-326: Tasks command with pagination is correct.Properly passes limit/offset params, displays count information in table title, and handles empty results.
329-370: Activity command implementation is clean.Timestamp formatting (removing T, truncating to 19 chars) creates readable output.
373-463: Start, pause, resume commands are consistent.All three follow the same pattern with proper error handling. The optional message display is a nice touch.
- Remove unused imports (pytest, typing.Any, tempfile, MagicMock) - Remove f-string prefixes where no placeholders used - Remove unused last_exception variable in api_client.py - Add noqa: E402 comments for intentional late imports in __init__.py All 157 CLI tests continue to pass.
Comprehensive PR Review: CLI Command ImplementationThis is an excellent PR that implements a comprehensive CLI interface with 12 command groups covering all major API endpoints. The implementation follows TDD principles with 157 tests and demonstrates strong software engineering practices. Below are my detailed findings. ✅ Strengths1. Excellent Architecture & Design
2. Robust Error Handling
3. Security Best Practices
4. Testing Quality
🔧 Issues Requiring Fixes1. CRITICAL: Conflicting checkpoint commands (codeframe/cli/init.py:168-174)Severity: HIGH The stub @app.command()
def checkpoint(
action: str = typer.Argument("create", help="Action: create or list"),
message: Optional[str] = typer.Option(None, help="Checkpoint message"),
):
"""Manage checkpoints."""
console.print(f"Checkpoint {action} - [yellow]Not implemented yet[/yellow]")Problem: Users might invoke Recommendation: Remove the stub entirely, or convert it to a deprecation notice: @app.command()
def checkpoint():
"""Deprecated: Use 'codeframe checkpoints' instead."""
console.print("[yellow]This command is deprecated.[/yellow]")
console.print("Use: codeframe checkpoints [command]")
console.print("Example: codeframe checkpoints create --message 'My checkpoint'")
raise typer.Exit(0)2. Code Duplication:
|
The top-level 'checkpoint' command conflicted with the full 'checkpoints' sub-app. Convert it to a hidden, deprecated shim that directs users to the proper checkpoints subcommand.
Code Review SummaryThis is an excellent PR that implements a comprehensive CLI interface for CodeFRAME! The implementation demonstrates strong software engineering practices with TDD, extensive test coverage (157 tests), and well-organized code structure. Here's my detailed feedback: ✅ Strengths
🔍 Issues to Address1. Code Duplication -
|
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In @codeframe/cli/agents_commands.py:
- Around line 37-42: Extract the duplicated require_auth function into a single
shared helper module (e.g., create codeframe.cli.helpers or codeframe.cli.utils)
containing the Console instance and the require_auth(client: APIClient) function
signature that checks client.token and raises typer.Exit with the same messages;
then replace the local require_auth definitions in discovery_commands.py,
session_commands.py, context_commands.py, tasks_commands.py, blocker_commands.py
and any other command modules with a single import from the new helper module
(import require_auth), removing the duplicated code so all CLI modules use the
same implementation.
- Line 88: The slice agent.get("assigned_at", "")[:10] in agents_commands.py can
fail or produce wrong output for short or malformed strings; add a defensive
check or helper to extract the date safely: implement a small function (e.g.
format_date(date_str: str | None) -> str) that returns "" if date_str is falsy
or shorter than 10, otherwise returns date_str[:10], and replace the inline
slice with a call to format_date(agent.get("assigned_at")). Ensure all uses of
assigned_at in this file use the helper to avoid repeated slicing logic.
- Line 233: The slice of proj.get("assigned_at") is unsafe; before slicing check
its type and length or use the shared format_date helper used elsewhere: replace
the direct slice of proj.get("assigned_at") in agents_commands.py with a call to
format_date(proj.get("assigned_at")) (or ensure proj.get("assigned_at") is a
non-empty string of at least 10 chars before doing proj.get("assigned_at")[:10])
to avoid exceptions when the value is missing or shorter than 10 characters.
In @codeframe/cli/discovery_commands.py:
- Around line 37-42: The require_auth function is duplicated; extract it into a
single shared helper (e.g., a new CLI helper module) and update callers to
import that one function. Move the implementation that checks client.token and
uses console.print + raise typer.Exit into a shared function named
require_auth(client: APIClient), keep the same behavior and signatures, remove
the duplicate implementations from discovery_commands.py and other CLI modules,
and update their imports to use the new shared helper (ensure console, APIClient
and typer are available/imported in the helper).
In @codeframe/cli/review_commands.py:
- Around line 35-41: Extract the duplicated require_auth function into a single
shared helper module (e.g., create codeframe/cli/auth_utils.py) that defines
require_auth(client: APIClient) and the shared Console instance, then update all
command modules that currently define their own require_auth (e.g.,
review_commands.py, tasks_commands.py, discovery_commands.py) to remove the
local function and import require_auth from codeframe.cli.auth_utils; ensure the
signature and behavior remain identical (checking client.token, printing the two
messages, and raising typer.Exit(1)) and that imports reference the APIClient
and typer as needed in the shared module.
In @codeframe/cli/tasks_commands.py:
- Around line 36-41: The require_auth function is duplicated across CLI modules;
extract it into a single shared helper (e.g., a new cli-level module named auth
or utils exposing require_auth) preserving the signature def
require_auth(client: APIClient): and its behavior (console prints and raise
typer.Exit(1)), then update callers (e.g., the require_auth usages in
tasks_commands.py and review_commands.py) to import the shared require_auth
instead of duplicating it; ensure the new module imports console, typer, and
APIClient so callers only need to import the function.
♻️ Duplicate comments (2)
codeframe/cli/project_commands.py (1)
154-156: Misleading message: discovery does not auto-start.As noted in a previous review, this message incorrectly states "Discovery started automatically" but the backend has no mechanism to auto-start discovery after project creation. The user would need to run
codeframe discovery start {project_id}separately.Either update the message to guide users to start discovery manually, or implement actual auto-start by calling the discovery API after project creation.
codeframe/cli/__init__.py (1)
168-175: Stubcheckpointcommand conflicts withcheckpointssub-app.The top-level stub command (
codeframe checkpoint) coexists with the fullcheckpointssub-app (codeframe checkpoints list, etc.). Consider removing this stub or converting it to a deprecation shim that directs users to usecodeframe checkpointsinstead.
🧹 Nitpick comments (12)
codeframe/cli/auth_commands.py (2)
147-153: Consider password confirmation for CLI-provided passwords.When users provide passwords via the
--passwordflag, they skip the confirmation prompt. This could lead to typos being persisted without user awareness. Interactive prompts request confirmation (lines 149-153), but CLI arguments don't.While this may be intentional for scripting scenarios, consider documenting this behavior or adding an optional
--password-confirmflag for safety. Alternatively, always prompt for confirmation when registering, regardless of how the password was provided:# Prompt for password if not provided if not password: password = typer.prompt("Password", hide_input=True) # Confirm password password_confirm = typer.prompt("Confirm password", hide_input=True) if password != password_confirm: console.print("[red]Error:[/red] Passwords don't match") raise typer.Exit(1) else: # Password provided via CLI - consider confirming in interactive mode if sys.stdin.isatty(): # Only prompt if in interactive terminal password_confirm = typer.prompt("Confirm password", hide_input=True) if password != password_confirm: console.print("[red]Error:[/red] Passwords don't match") raise typer.Exit(1)
255-257: Prefer specific exception handling over broad catch.Catching
Exceptionon Line 255 may hide unexpected errors and make debugging harder. Consider catching specific exceptions likeAPIErrororrequests.RequestException.- except Exception as e: - console.print(f"[red]Error:[/red] {e}") - raise typer.Exit(1) + except APIError as e: + console.print(f"[red]Error:[/red] {e}") + raise typer.Exit(1) + except Exception as e: + # Unexpected error - log for debugging + logger.exception("Unexpected error in whoami command") + console.print(f"[red]Unexpected error:[/red] {e}") + raise typer.Exit(1)tests/cli/test_quality_gates_commands.py (1)
109-110: Add defensive assertion before accessing mock call arguments.The test accesses
mock_request.call_args.kwargswithout first verifying the mock was called. If the command execution fails before making the request, this could raise anAttributeError.🔎 Suggested improvement
assert result.exit_code == 0 + # Verify the API request was made + mock_request.assert_called_once() call_kwargs = mock_request.call_args.kwargs assert call_kwargs["json"].get("gate") == "tests"This makes the test more explicit about expectations and provides clearer failure messages if the mock wasn't called.
tests/cli/test_agents_commands.py (1)
14-14: Consider using a pytest fixture for the test runner.While the module-level
runnerworks, using a pytest fixture (as seen intests/cli/test_cli_session.pylines 18-20) provides better test isolation and is more idiomatic.🔎 Optional refactor to use fixture
-runner = CliRunner() +@pytest.fixture +def runner(): + """Create CLI test runner.""" + return CliRunner()Then update test method signatures to accept
runneras a parameter.tests/cli/test_context_commands.py (2)
14-14: Consider using a pytest fixture for the test runner.Same as in the agents test file, using a pytest fixture would provide better test isolation.
73-74: Consider more specific assertions for stats output.The
orcondition allows the test to pass if only one metric is shown. For a stats command, you may want to assert that both metrics are present in the output.🔎 More specific assertion
- assert result.exit_code == 0 - assert "50" in result.output or "75000" in result.output + assert result.exit_code == 0 + assert "50" in result.output # total_items + assert "75000" in result.output # total_tokenstests/cli/test_checkpoint_commands.py (1)
14-14: Consider using a pytest fixture for the test runner.Consistent with other test files, using a pytest fixture would improve test isolation.
codeframe/cli/blocker_commands.py (1)
37-42: Consolidate duplicatedrequire_authacross CLI modules.The
require_authfunction is duplicated across multiple CLI command modules (blocker_commands, metrics_commands, session_commands, context_commands, tasks_commands). This violates the DRY principle and makes maintenance harder.Consider extracting this to a shared utility module like
codeframe/cli/auth_utils.pyor including it in the existingcodeframe/cli/auth.pymodule, then importing it where needed.🔎 Example consolidation
In
codeframe/cli/auth.py, add:def require_auth(client: APIClient): """Check if client is authenticated, exit with error if not.""" if not client.token: console.print("[yellow]Not logged in.[/yellow]") console.print("Please log in: codeframe auth login") raise typer.Exit(1)Then in command modules:
-def require_auth(client: APIClient): - """Check if client is authenticated, exit with error if not.""" - if not client.token: - console.print("[yellow]Not logged in.[/yellow]") - console.print("Please log in: codeframe auth login") - raise typer.Exit(1) +from codeframe.cli.auth import require_authcodeframe/cli/metrics_commands.py (1)
33-38: Consolidate duplicatedrequire_authacross CLI modules.Same duplication issue as in
blocker_commands.py. Therequire_authfunction appears in multiple CLI modules and should be consolidated into a shared utility.Refer to the consolidation approach suggested in the blocker_commands review.
codeframe/cli/review_commands.py (1)
58-58: Consider standardizing the default output format across commands.Within this module,
statusandstatscommands default to"text"format, whilefindingsandlist_reviewsdefault to"table"format. This inconsistency may confuse users about what to expect from different commands.Consider either:
- Defaulting all to
"table"for consistency (since findings naturally fit tables)- Or document why certain commands use different defaults
Also applies to: 115-115, 167-167, 250-250
codeframe/cli/tasks_commands.py (1)
135-135: Add validation for priority values.The help text indicates priority should be 0-4 (0=critical, 4=low), but there's no validation to enforce this range. Users could pass invalid values like -1 or 10, which would be sent to the API.
🔎 Proposed validation
Add validation in both
createandupdatecommands:@tasks_app.command() def create( project_id: int = typer.Argument(..., help="Project ID"), title: str = typer.Argument(..., help="Task title"), description: Optional[str] = typer.Option(None, "--description", "-d", help="Task description"), priority: int = typer.Option(3, "--priority", "-p", help="Priority (0=critical, 4=low)"), status: str = typer.Option("pending", "--status", "-s", help="Initial status"), ): """Create a new task.""" try: client = APIClient() require_auth(client) + + if not 0 <= priority <= 4: + console.print("[red]Error:[/red] Priority must be between 0 and 4") + raise typer.Exit(1) data = { "project_id": project_id,Similar validation in
updatecommand around line 252.Also applies to: 231-231
codeframe/cli/context_commands.py (1)
35-41: Consider extractingrequire_authto a shared utility.This helper is duplicated across multiple CLI modules (
context_commands,checkpoint_commands,project_commands, etc.). Extracting it to a common location (e.g.,api_client.pyor a newcli_utils.py) would reduce duplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
codeframe/cli/__init__.pycodeframe/cli/agents_commands.pycodeframe/cli/api_client.pycodeframe/cli/auth_commands.pycodeframe/cli/blocker_commands.pycodeframe/cli/checkpoint_commands.pycodeframe/cli/context_commands.pycodeframe/cli/discovery_commands.pycodeframe/cli/metrics_commands.pycodeframe/cli/project_commands.pycodeframe/cli/quality_gates_commands.pycodeframe/cli/review_commands.pycodeframe/cli/tasks_commands.pytests/cli/test_agents_commands.pytests/cli/test_api_client.pytests/cli/test_auth_commands.pytests/cli/test_auth_module.pytests/cli/test_blocker_commands.pytests/cli/test_checkpoint_commands.pytests/cli/test_context_commands.pytests/cli/test_discovery_commands.pytests/cli/test_metrics_commands.pytests/cli/test_project_commands.pytests/cli/test_quality_gates_commands.pytests/cli/test_review_commands.pytests/cli/test_session_commands.pytests/cli/test_tasks_commands.py
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/cli/test_review_commands.py
- tests/cli/test_discovery_commands.py
- tests/cli/test_api_client.py
- tests/cli/test_session_commands.py
- tests/cli/test_tasks_commands.py
- tests/cli/test_metrics_commands.py
- codeframe/cli/quality_gates_commands.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/cli/api_client.pycodeframe/cli/checkpoint_commands.pycodeframe/cli/review_commands.pycodeframe/cli/tasks_commands.pycodeframe/cli/blocker_commands.pycodeframe/cli/context_commands.pycodeframe/cli/discovery_commands.pycodeframe/cli/metrics_commands.pycodeframe/cli/agents_commands.pycodeframe/cli/__init__.pycodeframe/cli/auth_commands.pycodeframe/cli/project_commands.py
🧠 Learnings (4)
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/**/*.py : Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Applied to files:
tests/cli/test_quality_gates_commands.pytests/cli/test_project_commands.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : Organize Python backend files with Auth module at codeframe/auth/ containing dependencies.py (get_current_user), manager.py (UserManager), models.py, router.py, and schemas.py
Applied to files:
tests/cli/test_auth_module.pycodeframe/cli/auth_commands.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : For authentication, use FastAPI Users with JWT tokens and mandatory authentication (no bypass mode)
Applied to files:
codeframe/cli/auth_commands.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
codeframe/cli/project_commands.py
🧬 Code graph analysis (14)
tests/cli/test_quality_gates_commands.py (3)
codeframe/cli/api_client.py (1)
get(218-228)tests/cli/test_cli_session.py (1)
runner(19-21)codeframe/cli/quality_gates_commands.py (1)
get(42-111)
codeframe/cli/tasks_commands.py (1)
codeframe/cli/api_client.py (5)
APIError(29-35)AuthenticationError(38-41)get(218-228)post(230-240)patch(254-264)
codeframe/cli/blocker_commands.py (5)
codeframe/cli/api_client.py (3)
APIClient(54-275)APIError(29-35)AuthenticationError(38-41)codeframe/cli/context_commands.py (1)
require_auth(35-40)codeframe/cli/metrics_commands.py (1)
require_auth(33-38)codeframe/cli/tasks_commands.py (1)
require_auth(36-41)codeframe/cli/session_commands.py (1)
require_auth(34-39)
codeframe/cli/context_commands.py (1)
codeframe/cli/api_client.py (5)
APIClient(54-275)APIError(29-35)AuthenticationError(38-41)get(218-228)post(230-240)
codeframe/cli/discovery_commands.py (1)
codeframe/cli/api_client.py (4)
APIClient(54-275)APIError(29-35)AuthenticationError(38-41)get(218-228)
tests/cli/test_project_commands.py (2)
codeframe/cli/api_client.py (2)
patch(254-264)get(218-228)codeframe/cli/project_commands.py (1)
get(171-212)
tests/cli/test_checkpoint_commands.py (2)
codeframe/cli/api_client.py (1)
patch(254-264)tests/cli/test_cli_session.py (1)
runner(19-21)
codeframe/cli/metrics_commands.py (1)
codeframe/cli/api_client.py (3)
APIError(29-35)AuthenticationError(38-41)get(218-228)
tests/cli/test_auth_module.py (1)
codeframe/cli/auth.py (5)
get_credentials_path(23-29)store_token(32-56)get_token(59-99)clear_token(102-113)is_authenticated(116-122)
codeframe/cli/agents_commands.py (7)
tests/api/conftest.py (1)
api_client(67-177)codeframe/cli/api_client.py (7)
APIClient(54-275)APIError(29-35)AuthenticationError(38-41)get(218-228)post(230-240)delete(266-275)put(242-252)codeframe/cli/blocker_commands.py (2)
require_auth(37-42)get(116-171)codeframe/cli/context_commands.py (2)
require_auth(35-40)get(44-88)codeframe/cli/discovery_commands.py (1)
require_auth(37-42)codeframe/cli/tasks_commands.py (2)
require_auth(36-41)get(182-224)codeframe/cli/session_commands.py (1)
require_auth(34-39)
tests/cli/test_blocker_commands.py (2)
codeframe/cli/api_client.py (2)
patch(254-264)get(218-228)codeframe/cli/blocker_commands.py (1)
get(116-171)
codeframe/cli/auth_commands.py (2)
codeframe/cli/auth.py (3)
store_token(32-56)clear_token(102-113)is_authenticated(116-122)codeframe/cli/api_client.py (5)
APIClient(54-275)AuthenticationError(38-41)get_api_base_url(44-51)post(230-240)get(218-228)
tests/cli/test_auth_commands.py (3)
codeframe/cli/api_client.py (1)
patch(254-264)tests/cli/test_cli_session.py (1)
runner(19-21)codeframe/core/config.py (1)
load(223-235)
tests/cli/test_agents_commands.py (3)
codeframe/cli/api_client.py (1)
patch(254-264)tests/cli/test_cli_session.py (1)
runner(19-21)tests/cli/test_project_commands.py (1)
test_status_success(211-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (46)
tests/cli/test_auth_commands.py (1)
1-222: LGTM! Comprehensive test coverage.The test suite is well-structured with:
- Clear test organization into logical classes
- Good coverage of success and error paths
- Proper mocking of external dependencies (requests, filesystem)
- Validation of both CLI output and side effects (token storage)
- Edge case testing (prompts, expired tokens, non-existent users)
The TDD approach is evident and the tests provide good confidence in the auth commands implementation.
tests/cli/test_auth_module.py (1)
1-226: LGTM! Excellent test coverage for auth module.This test suite thoroughly validates the auth module's core functionality:
- ✅ Credential path resolution and structure
- ✅ Token storage with secure permissions (600)
- ✅ Token retrieval with proper precedence (env var → file)
- ✅ Robust error handling (invalid JSON, missing keys, I/O errors)
- ✅ Token clearing operations
- ✅ Authentication state detection
The tests are well-organized, use appropriate mocking/patching, and cover edge cases comprehensively. This provides strong confidence in the authentication infrastructure.
codeframe/cli/blocker_commands.py (3)
45-113: LGTM - Well-structured list command.The
list_blockerscommand has good error handling, supports filtering and multiple output formats, and provides helpful guidance when pending blockers are found.
115-211: LGTM - Robust blocker resolution flow.Both
getandresolvecommands handle errors appropriately (404, 409 conflict) and provide clear user feedback with colored status indicators and guidance messages.
213-271: LGTM - Well-designed metrics display.The metrics command provides clear visualization with smart time formatting and threshold-based color coding for the expiration rate.
codeframe/cli/metrics_commands.py (3)
46-106: LGTM - Clear token metrics display.The
tokenscommand provides comprehensive token usage information with proper formatting and per-agent breakdown.
108-167: LGTM - Well-formatted cost metrics.The
costscommand displays cost information clearly with proper currency formatting and a reasonable 7-day breakdown.
170-215: LGTM - Comprehensive agent metrics.The
agentcommand provides useful per-agent statistics with consistent formatting and error handling.codeframe/cli/review_commands.py (2)
228-228: Path truncation approach is acceptable but could be misleading.Using string slicing (e.g.,
[-25:]) to truncate file paths takes the last N characters, which could be confusing when multiple files share similar endings (e.g.,src/foo/utils.pyandlib/bar/utils.pyboth display as.../bar/utils.py).This is acceptable for display purposes in a terminal table with space constraints, but be aware that identical truncated paths might appear for different files.
Also applies to: 313-313
43-52: Well-structured CLI commands with comprehensive error handling.The command implementations follow a consistent pattern with:
- Clear authentication checks
- Proper error handling for different HTTP status codes
- Support for both JSON and formatted text output
- User-friendly messaging and next-step guidance
The severity emoji mapping and Rich-based formatting enhance the user experience.
Also applies to: 55-328
tests/cli/test_blocker_commands.py (1)
1-230: Comprehensive test coverage for blocker CLI commands.The test suite follows solid testing practices:
- Proper isolation using
tmp_pathfor credentials- Appropriate mocking of HTTP layer via
requests.request- Coverage of both success and error scenarios (404, 409, etc.)
- Verification of API parameter passing (e.g., status filter on line 82)
- Exit code and output assertions
The TDD approach aligns with the PR objectives.
tests/cli/test_project_commands.py (1)
1-372: Excellent test coverage for project CLI commands.The test suite is comprehensive and well-structured:
- Tests all lifecycle commands (create, get, list, status, tasks, activity, start, pause, resume)
- Validates API payload structure (lines 134-137) to ensure correct data is sent
- Tests query parameter passing for filters (lines 282, 287)
- Covers error scenarios with appropriate HTTP status codes
- Verifies both table and JSON output formats
- Follows consistent mocking patterns for isolation
codeframe/cli/tasks_commands.py (1)
44-127: Well-implemented task management commands.The commands provide comprehensive task management functionality:
- Flexible filtering by status and priority
- Clear priority visualization with emoji indicators (lines 105-107)
- Support for both table and JSON output formats
- Helpful error messages and next-step guidance
- Proper handling of optional fields in create/update operations
- Validation that at least one field is specified for updates (lines 257-260)
Also applies to: 130-280
codeframe/cli/discovery_commands.py (1)
45-323: Excellent discovery workflow implementation with great UX.The discovery commands provide a well-thought-out workflow:
- Clear state management (idle, discovering, completed)
- Visual progress bar rendering (lines 132-135) enhances user experience
- Contextual error messages that guide users to correct actions
- Confirmation prompt for destructive operations (restart, lines 249-255)
- Helpful next-step suggestions throughout the workflow
- Proper handling of edge cases (already in progress, not started, completed)
The implementation provides good guardrails for the multi-step discovery process.
codeframe/cli/context_commands.py (5)
1-33: LGTM!Module setup follows consistent patterns with other CLI modules. The Typer app configuration and Rich console initialization are correct.
43-88: LGTM!The
getcommand is well-structured with proper error handling, user-friendly output formatting with emoji tier indicators, and support for both text and JSON output formats.
91-146: LGTM!The
statscommand properly displays context statistics with a well-formatted Rich table for tier breakdown.
149-180: LGTM!The
flash_savecommand correctly creates context checkpoints with appropriate success messaging and error handling.
183-237: LGTM!The
checkpointscommand properly lists context checkpoints with a nicely formatted table. The timestamp formatting is handled well for display purposes.codeframe/cli/api_client.py (7)
1-26: LGTM!Module documentation clearly explains the purpose and usage. Imports are appropriate for the HTTP client functionality.
29-41: LGTM!Exception hierarchy is well-designed.
AuthenticationErroras a subclass ofAPIErrorallows callers to catch authentication failures specifically or handle all API errors generically.
44-51: LGTM!The
get_api_base_urlfunction correctly handles environment variable lookup with a sensible default and proper URL normalization.
54-87: LGTM!The
APIClientinitialization properly handles optional parameters with sensible defaults. The header construction correctly adds the Authorization header only when a token is present.
89-160: LGTM!URL construction and response handling are comprehensive. The response handler correctly distinguishes between success, authentication errors, client errors, and server errors with appropriate exception types and user-friendly messages.
162-216: LGTM!The retry logic with exponential backoff is correctly implemented. The handling of
ConnectionErrorandTimeoutwith appropriate retry behavior and final error reporting is well done.
218-275: LGTM!HTTP method wrappers provide a clean, consistent API. The use of
json=dataparameter correctly handles JSON serialization for request bodies.codeframe/cli/checkpoint_commands.py (7)
1-44: LGTM!Module setup follows established patterns. The Syntax import from Rich is appropriate for the diff display functionality.
46-103: LGTM!The
list_checkpointscommand properly handles both populated and empty checkpoint lists with appropriate formatting and user guidance.
105-149: LGTM!The
createcommand is well-implemented with helpful post-creation guidance showing how to restore to the new checkpoint.
152-202: LGTM!The
getcommand properly displays checkpoint details with well-formatted metadata including task completion and cost information.
205-242: LGTM!The
deletecommand implements proper safety measures with confirmation prompts and a--forceflag for scripted usage.
245-296: LGTM!The
restorecommand implements an excellent two-phase workflow: preview changes first, then confirm to execute. The diff preview with syntax highlighting provides a clear view of what will change.
299-345: LGTM!The
diffcommand provides clear statistics and well-formatted diff output with appropriate color coding for additions and deletions.codeframe/cli/project_commands.py (7)
1-47: LGTM!Module setup follows consistent patterns with other CLI modules.
49-106: LGTM!The
list_projectscommand properly handles both populated and empty project lists with clear formatting.
170-212: LGTM!The
getcommand properly displays project details with appropriate formatting and error handling.
215-260: LGTM!The
statuscommand provides a clear visual representation of project progress with a text-based progress bar.
263-326: LGTM!The
taskscommand properly implements filtering and pagination with a well-formatted table output.
329-370: LGTM!The
activitycommand properly displays recent project activities with clear timestamp formatting.
373-463: LGTM!The lifecycle commands (
start,pause,resume) are consistently implemented with appropriate success messaging and error handling.codeframe/cli/__init__.py (6)
1-31: LGTM!Module setup correctly configures the main Typer application with appropriate imports for the CLI functionality.
38-115: LGTM!Legacy commands for local project operations are correctly implemented. The coexistence with API-based project commands under
codeframe projectsprovides backward compatibility while adding new API functionality.
180-259: LGTM!The
servecommand is well-implemented with proper port validation, browser auto-launch, and comprehensive error handling. The background thread for browser opening prevents blocking the server startup.
262-289: LGTM!The
clear-sessionandversioncommands are simple and correctly implemented.
292-329: LGTM!Command group imports and registrations are correctly organized. The Phase 1/Phase 2 separation provides clear documentation of the CLI evolution. All 12 command groups are properly registered with descriptive help text.
332-333: LGTM!Standard CLI entry point pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @codeframe/cli/__init__.py:
- Around line 98-116: The status command is using wrong dict keys from
Project.get_status(); update the status() function to access status['name']
instead of status['project_name'] and status['progress_pct'] instead of
status['completion_percentage'] (i.e., change the two console.print calls that
reference project_name and completion_percentage to use name and progress_pct
respectively) so the printed values match the keys returned by
Project.get_status().
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codeframe/cli/__init__.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/cli/__init__.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Use feature branches from main with Conventional Commits format (feat/fix/docs scope): description
🧬 Code graph analysis (1)
codeframe/cli/__init__.py (9)
codeframe/core/port_utils.py (2)
check_port_availability(26-65)validate_port_range(68-88)codeframe/cli/project_commands.py (6)
create(110-167)start(374-401)pause(405-432)resume(436-463)status(216-260)get(171-212)codeframe/cli/discovery_commands.py (1)
start(46-79)codeframe/cli/agents_commands.py (1)
status(193-247)codeframe/cli/review_commands.py (1)
status(56-109)codeframe/core/project.py (1)
get_status(445-613)codeframe/cli/blocker_commands.py (1)
get(116-171)codeframe/cli/context_commands.py (1)
get(44-88)codeframe/core/session_manager.py (1)
SessionManager(9-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Backend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (9)
codeframe/cli/__init__.py (9)
1-30: LGTM! Clean module setup and imports.The module docstring clearly describes the CLI structure, imports are well-organized, and the Typer app is properly configured.
38-54: LGTM! Well-structured init command with helpful user guidance.The command properly creates a project and provides clear next steps for users.
56-96: LGTM! Consistent command implementations.The start, pause, and resume commands follow a clear, consistent pattern with appropriate error handling.
118-166: LGTM! Chat and config commands are well-implemented.Both commands have clear logic, appropriate input validation, and helpful error messages.
168-180: Excellent resolution of the checkpoint conflict!The deprecated checkpoint command is now properly marked as
hidden=Trueand provides clear guidance to users about the newcheckpointssubcommand. This effectively addresses the previous review comment about the naming conflict.
185-265: LGTM! Robust serve command with comprehensive validation.The command properly validates the port range and availability before starting the server, handles errors gracefully with actionable messages, and safely opens the browser in a background thread. The pre-flight checks and error handling follow best practices.
267-287: LGTM! Clear and straightforward session clearing.The command properly uses the SessionManager to clear session state with appropriate error handling.
289-295: LGTM! Simple version display.The version command is concise and properly formatted.
297-335: LGTM! Well-organized command group registration.The imports and registrations are cleanly structured with appropriate E402 suppression (since imports must follow app definition). All 12 command groups are properly registered with descriptive help text.
- Create helpers.py with shared console, require_auth(), and format_date() - Remove duplicated require_auth from all 12 command modules - Replace unsafe date slicing with format_date() helper - Fix status command to use correct dict keys (name, progress_pct) - All 157 tests continue to pass, ruff checks clean
Code Review - PR #190: feat(cli): Add comprehensive CLI command groups for API coverageI've reviewed this PR and overall it's well-structured with good test coverage (157 tests). Here's my detailed feedback: ✅ Strengths
🔍 Code Quality Observations1. APIClient Retry Logic (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @codeframe/cli/checkpoint_commands.py:
- Line 129: The console.print call uses result.get('git_commit', '')[:7] which
can raise TypeError when git_commit is None; change to use the safe pattern
(e.g., (result.get('git_commit') or '')[:7]) where this print occurs so None
falls back to empty string—update the expression in the checkpoint display code
that constructs the Commit string (the console.print line referencing result and
'git_commit').
♻️ Duplicate comments (3)
codeframe/cli/review_commands.py (1)
1-319: LGTM! Successfully uses shared helpers.The module now correctly imports
require_authandconsolefrom the sharedhelpersmodule, eliminating the duplication that was previously flagged in review comments. All commands consistently userequire_auth(client)and have comprehensive error handling.Based on learnings, this resolves the code duplication concern raised in the past review comment for this file.
codeframe/cli/agents_commands.py (1)
1-277: LGTM! Successfully addresses unsafe date slicing with shared helper.The module now correctly uses
format_date()from the shared helpers module (lines 79, 224), replacing the previously unsafe[:10]slicing that was flagged in past review comments. This defensive approach handles None values and short strings gracefully.All commands demonstrate good practices:
- Consistent authentication enforcement
- Interactive confirmations where appropriate (remove command)
- Multiple output formats (table/json)
- Specific error handling for different scenarios (404, 409)
Based on learnings, this resolves the unsafe date slicing concerns raised in past review comments for this file at lines 88 and 233.
codeframe/cli/project_commands.py (1)
145-148: The--no-discoverymessage is misleading.The message at lines 145-147 states "Discovery started automatically" when
--no-discoveryis False. However, based on the past review analysis, discovery does not actually auto-start after project creation—it requires a separatecodeframe discovery startcommand. This misleads users into thinking discovery has begun.Either implement auto-start by making an additional API call, or correct the message:
Proposed fix if auto-start is not intended
if not no_discovery: - console.print("\n[cyan]Discovery started automatically.[/cyan]") - console.print(f"Check progress: codeframe discovery progress {result.get('id')}") + console.print(f"\n[cyan]Start discovery:[/cyan] codeframe discovery start {result.get('id')}")
🧹 Nitpick comments (2)
codeframe/cli/auth_commands.py (1)
172-188: Consider logging auto-login failures for better diagnostics.The auto-login after registration silently swallows errors and only suggests manual login. While functional, logging the failure reason could help users troubleshoot issues.
🔎 Optional enhancement
if login_response.status_code == 200: token = login_response.json().get("access_token") if token: store_token(token) console.print("[green]✓ Logged in automatically[/green]") else: - console.print("[yellow]Note:[/yellow] Please log in manually: codeframe auth login") + console.print( + f"[yellow]Note:[/yellow] Auto-login failed (HTTP {login_response.status_code}). " + "Please log in manually: codeframe auth login" + )codeframe/cli/project_commands.py (1)
346-354: Consider usingformat_datefrom helpers for consistent formatting.The commit message mentions
format_date()was added to helpers.py to replace unsafe date slicing. While this slicing is safe (it handles empty strings), using the shared helper would ensure consistency across the CLI.Proposed refactor
+from codeframe.cli.helpers import console, require_auth, format_date ... for item in activities: - timestamp = item.get("timestamp", "")[:19].replace("T", " ") + timestamp = format_date(item.get("timestamp", "")) action = item.get("action", "unknown")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
codeframe/cli/__init__.pycodeframe/cli/agents_commands.pycodeframe/cli/auth_commands.pycodeframe/cli/blocker_commands.pycodeframe/cli/checkpoint_commands.pycodeframe/cli/context_commands.pycodeframe/cli/discovery_commands.pycodeframe/cli/helpers.pycodeframe/cli/metrics_commands.pycodeframe/cli/project_commands.pycodeframe/cli/quality_gates_commands.pycodeframe/cli/review_commands.pycodeframe/cli/session_commands.pycodeframe/cli/tasks_commands.py
🚧 Files skipped from review as they are similar to previous changes (5)
- codeframe/cli/metrics_commands.py
- codeframe/cli/context_commands.py
- codeframe/cli/blocker_commands.py
- codeframe/cli/tasks_commands.py
- codeframe/cli/session_commands.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/cli/__init__.pycodeframe/cli/review_commands.pycodeframe/cli/agents_commands.pycodeframe/cli/checkpoint_commands.pycodeframe/cli/auth_commands.pycodeframe/cli/helpers.pycodeframe/cli/discovery_commands.pycodeframe/cli/project_commands.pycodeframe/cli/quality_gates_commands.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Use feature branches from main with Conventional Commits format (feat/fix/docs scope): description
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : Organize Python backend files with Auth module at codeframe/auth/ containing dependencies.py (get_current_user), manager.py (UserManager), models.py, router.py, and schemas.py
Applied to files:
codeframe/cli/auth_commands.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : For authentication, use FastAPI Users with JWT tokens and mandatory authentication (no bypass mode)
Applied to files:
codeframe/cli/auth_commands.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
codeframe/cli/project_commands.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/**/*.py : Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Applied to files:
codeframe/cli/quality_gates_commands.py
🧬 Code graph analysis (4)
codeframe/cli/agents_commands.py (2)
codeframe/cli/api_client.py (7)
APIClient(54-275)APIError(29-35)AuthenticationError(38-41)get(218-228)post(230-240)delete(266-275)put(242-252)codeframe/cli/helpers.py (2)
require_auth(21-33)format_date(36-56)
codeframe/cli/checkpoint_commands.py (2)
codeframe/cli/api_client.py (6)
APIClient(54-275)APIError(29-35)AuthenticationError(38-41)get(218-228)post(230-240)delete(266-275)codeframe/cli/helpers.py (1)
require_auth(21-33)
codeframe/cli/auth_commands.py (2)
codeframe/cli/auth.py (3)
store_token(32-56)clear_token(102-113)is_authenticated(116-122)codeframe/cli/api_client.py (5)
APIClient(54-275)AuthenticationError(38-41)get_api_base_url(44-51)post(230-240)get(218-228)
codeframe/cli/helpers.py (2)
tests/api/conftest.py (1)
api_client(67-177)codeframe/cli/api_client.py (1)
APIClient(54-275)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (27)
codeframe/cli/auth_commands.py (2)
35-110: LGTM! Robust authentication flow.The login command correctly uses form-encoded data with the "username" field (FastAPI Users convention) and provides comprehensive error handling for all expected scenarios.
223-256: LGTM! Proper authentication check and error handling.The whoami command correctly pre-checks authentication status and uses the APIClient for the authenticated endpoint with appropriate error handling.
codeframe/cli/helpers.py (1)
1-56: Excellent refactoring! Successfully eliminates code duplication.This new module effectively consolidates:
- The
require_authhelper that was previously duplicated across 12 command modules- The
format_datehelper that replaces unsafe date string slicing- A shared
consoleinstance for consistent outputThe implementations are clean, well-documented, and defensive (handling None and short strings gracefully).
Based on learnings, this addresses the specific duplication concerns raised in past review comments for review_commands.py and agents_commands.py.
codeframe/cli/quality_gates_commands.py (1)
1-145: LGTM! Consistent use of shared helpers and clean implementation.The module correctly leverages the shared
require_authandconsolefrom helpers, maintains consistent error handling patterns, and provides clear user guidance (e.g., suggesting the status check command after triggering a run).codeframe/cli/discovery_commands.py (6)
1-34: LGTM! Good structure and imports.The module is well-organized with clear docstrings and proper imports. The refactoring to use centralized
require_authandconsolefromhelpers.pyaddresses the previous review comment about code duplication.
36-71: LGTM!The
startcommand correctly handles the 409 conflict case with helpful guidance and follows consistent error handling patterns.
73-156: LGTM!The
progresscommand has comprehensive state handling with appropriate visual feedback. The progress bar calculation and state color mapping are correct.
158-217: LGTM!The
answercommand correctly handles the workflow transition, showing either the next question or completion status with appropriate guidance.
219-272: LGTM!The
restartcommand properly implements the confirmation flow and provides clear error messages with guidance.
274-314: LGTM!The
generate_prdcommand correctly handles prerequisites and provides clear guidance when discovery isn't complete.codeframe/cli/project_commands.py (6)
1-38: LGTM! Good module structure.The module is well-organized with proper imports from the centralized helpers module.
40-98: LGTM!The
list_projectscommand handles empty projects gracefully and the date slicing pattern is safe with the conditional check.
161-204: LGTM!The
getcommand correctly handles the 404 case with a clear error message.
206-252: LGTM!The
statuscommand correctly usesresult.get('name')for the project name and implements a clear progress bar visualization.
254-318: LGTM!The
taskscommand provides useful filtering and pagination options with clean table output.
364-454: LGTM!The lifecycle commands (
start,pause,resume) follow consistent patterns with proper error handling.codeframe/cli/__init__.py (5)
1-31: LGTM! Good module organization.The docstring clearly explains the CLI structure and the main app is properly configured.
98-116: LGTM!The
statuscommand now correctly usesstatus['name']andstatus['progress_pct'], addressing the previous review comment about incorrect dictionary keys.
168-180: LGTM!The deprecated
checkpointcommand now provides clear migration guidance to thecheckpointssub-app, addressing the previous review concern.
185-265: LGTM!The
servecommand has comprehensive validation and error handling. The daemon thread for browser opening is appropriate, and the error messages provide actionable guidance.
297-335: LGTM!All 12 command groups are properly imported and registered with clear help text. The late imports are correctly annotated with
noqa: E402.codeframe/cli/checkpoint_commands.py (6)
1-35: LGTM! Good module structure.The module is well-organized with clear docstrings, proper imports, and follows the established patterns from other CLI modules.
37-94: LGTM!The
list_checkpointscommand has proper null handling for optional fields and provides helpful guidance when no checkpoints exist.
143-194: LGTM!The
getcommand provides comprehensive checkpoint details with good metadata display and proper 404 handling.
196-234: LGTM!The
deletecommand follows the established confirmation pattern with a clear warning about irreversibility.
236-288: LGTM!The
restorecommand has excellent UX with preview mode by default and clear diff visualization using Rich Syntax.
290-336: LGTM!The
diffcommand provides a clear summary of changes with file counts and colored insertions/deletions before showing the full diff.
| console.print("[green]✓ Checkpoint created successfully[/green]") | ||
| console.print(f"\n[bold]ID:[/bold] {result.get('id')}") | ||
| console.print(f"[bold]Name:[/bold] {result.get('name')}") | ||
| console.print(f"[bold]Commit:[/bold] {result.get('git_commit', '')[:7]}") |
There was a problem hiding this comment.
Potential TypeError if git_commit is None.
If the API returns git_commit: null, result.get('git_commit', '') returns None (not the default ''), and None[:7] raises TypeError. Use the safe pattern from other commands.
Proposed fix
- console.print(f"[bold]Commit:[/bold] {result.get('git_commit', '')[:7]}")
+ console.print(f"[bold]Commit:[/bold] {result.get('git_commit', '')[:7] if result.get('git_commit') else 'N/A'}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.print(f"[bold]Commit:[/bold] {result.get('git_commit', '')[:7]}") | |
| console.print(f"[bold]Commit:[/bold] {result.get('git_commit', '')[:7] if result.get('git_commit') else 'N/A'}") |
🤖 Prompt for AI Agents
In @codeframe/cli/checkpoint_commands.py around line 129, The console.print call
uses result.get('git_commit', '')[:7] which can raise TypeError when git_commit
is None; change to use the safe pattern (e.g., (result.get('git_commit') or
'')[:7]) where this print occurs so None falls back to empty string—update the
expression in the checkpoint display code that constructs the Commit string (the
console.print line referencing result and 'git_commit').
1 participant
Summary
Features
authprojectsblockerscheckpointsdiscoveryagentstasksquality-gatesmetricssessioncontextreviewTechnical Details
~/.codeframe/credentials.jsonTest plan
uv run pytest tests/cli/ -v)from codeframe.cli import app)Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.