feat: Add CLI for local Potpie usage#700
feat: Add CLI for local Potpie usage#700sofia-willow wants to merge 3 commits intopotpie-ai:mainfrom
Conversation
Implements the CLI requested in issue potpie-ai#224 with all required commands: Commands: - potpie start — Start Docker services + app server - potpie stop — Stop all Potpie services - potpie parse — Submit repo for parsing, poll until complete - potpie chat — Interactive console chat with streaming responses - potpie status — Check parsing status for a project - potpie projects — List all parsed projects - potpie agents — List available agents Features: - HTTP client wrapping the existing REST API (no new dependencies beyond httpx + rich) - Polling with progress dots for parse status - Streaming response display for chat - Clean error handling and input validation - Color-coded Rich console output - Configurable server URL via --url flag or POTPIE_URL env var - pyproject.toml entry point: potpie = potpie.cli.main:main Tests: - Client unit tests (health, polling, timeout) - Parser/dispatch tests for all commands Closes potpie-ai#224
WalkthroughAdds a new potpie CLI package: a synchronous httpx-based PotpieClient, a CLI entrypoint with start/stop/parse/status/projects/agents/chat commands, pyproject script + rich dependency, and unit tests for the client and CLI parsing/dispatch. Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as CLI (main)
participant Client as PotpieClient
participant Server as Potpie Server
User->>CLI: potpie parse <repo_path>
CLI->>Client: is_alive()
Client->>Server: GET /health
Server-->>Client: 200 OK
Client-->>CLI: True
CLI->>Client: parse(repo_path)
Client->>Server: POST /api/v1/parse
Server-->>Client: {"project_id":"..."}
Client-->>CLI: project data
CLI->>Client: poll_parsing(project_id)
loop Until terminal state
Client->>Server: GET /api/v1/parsing-status/{project_id}
Server-->>Client: {"status":"parsing"}
Client->>Client: wait(interval)
end
Server-->>Client: {"status":"ready"}
Client-->>CLI: {"status":"ready"}
CLI->>User: Parse completed
sequenceDiagram
participant User
participant CLI as CLI (main)
participant Client as PotpieClient
participant Server as Potpie Server
User->>CLI: potpie chat <project_id> --agent <agent>
CLI->>Client: list_projects()
Client->>Server: GET /api/v1/projects/list
Server-->>Client: [projects...]
Client-->>CLI: projects
CLI->>Client: create_conversation(project_id, agent_id)
Client->>Server: POST /api/v1/conversations/
Server-->>Client: {"conversation_id":"..."}
Client-->>CLI: conversation data
User->>CLI: Enter message
CLI->>Client: send_message(conversation_id, content)
Client->>Server: POST /api/v1/conversations/{id}/message/ (stream)
Server-->>Client: chunk1, chunk2...
Client-->>CLI: Generator<chunks>
CLI->>User: Display response chunks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/cli/test_main.py (1)
13-98: Add parser/dispatch tests for timeout and chat branch semantics.Given the new command surface, tests should explicitly cover parse timeout handling and (if intended by requirements)
chat --branchparsing/dispatch behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/test_main.py` around lines 13 - 98, Add tests that cover timeout parsing and chat branch semantics: extend tests in tests/cli/test_main.py to assert build_parser() parses a --timeout value into args.timeout (and that default timeout is used when omitted), and add a test that main([...]) with the chat command and a --branch flag (e.g., ["chat","project-123","--branch","develop"]) results in dispatch to cmd_chat with the branch preserved; reference build_parser, main, args.timeout, and cmd_chat/_get_client in the new tests to locate where to assert parsing and dispatch behavior.tests/cli/test_client.py (1)
12-64: Core HTTP contract paths are not covered by tests yet.Please add targeted tests for
parse,get_parsing_status,list_projects,list_agents,create_conversation, andsend_message(including endpoint prefix, headers, and payload schema). This would catch regressions in the most failure-prone parts of the new client.potpie/cli/main.py (1)
133-135: Add local validation forrepo_pathandbranchbefore API submission.This command currently accepts empty/invalid values without preflight checks, which misses the stated input-validation requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@potpie/cli/main.py` around lines 133 - 135, Validate repo_path and branch locally before the API call: ensure args.repo_path (repo_path) is non-empty, exists and is a directory (os.path.exists/os.path.isdir) and that it contains a git repo (e.g., check for .git or run a git command), and ensure args.branch (branch) is non-empty and corresponds to an existing branch in that repo (e.g., git rev-parse --verify or list branches). If any check fails, print a clear error and exit or raise an argparse error instead of proceeding to the API submission so bad inputs are rejected locally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@potpie/cli/client.py`:
- Around line 71-72: Several API calls in client.py are using
headers={"isDevelopmentMode": "enabled"} which fails authentication because
get_api_key_user expects x-api-key / x-user-id; update those calls (e.g., the
requests where headers={"isDevelopmentMode": "enabled"} at lines around 71-72,
87-88, 133-134, 146-147, 178-179, 201-202) to use the client's authentication
helper by replacing headers={"isDevelopmentMode": "enabled"} with
headers=self._auth_headers() so the required x-api-key and x-user-id are sent
(keep any other header keys if needed by merging with self._auth_headers()).
- Around line 66-67: The client is calling mounted endpoints under "/api/v1"
which no longer match the backend routes (they are under "/api/v2"); update
every request URL in potpie/cli/client.py that uses "/api/v1" (e.g. the JSON
POST to "/api/v1/parse" and the other occurrences at the noted locations) to use
the "/api/v2" base path instead so the requests match the server routes; search
for the string "/api/v1" in client.py and replace with "/api/v2" in the requests
(e.g., the requests.post calls constructing "/api/v1/parse", "/api/v1/..." etc.)
to ensure all endpoints target the v2 API.
- Around line 173-176: The conversation create payload is using the wrong key
"agent_id" which should be "agent_ids" (an array) to match the backend request
model; update the JSON payload where project_ids, agent_id, title are set so
that "agent_ids": [agent_id] is sent instead of "agent_id": agent_id, and verify
any code handling the response or constructing that dict (refer to the variables
project_id and agent_id and the conversation creation call) expects a list of
agent IDs.
In `@potpie/cli/main.py`:
- Around line 209-210: The status coloring compares the raw status string
case-sensitively; normalize the status before comparing by replacing the
assignment of state (from status.get("status", "unknown")) with a lower-cased
value (e.g., state = status.get("status", "unknown").lower()) and update the
style logic in the same place (the expression that sets style based on state) to
compare against lowercase literals ("ready", "completed", "parsed",
"processing"); repeat this normalization and comparison change for the second
occurrence of the same logic later in the file (the other style assignment
around the 250–251 area).
- Around line 413-416: The chat subparser (chat_cmd created for "chat") is
missing the --branch argument required by the CLI contract; update the parser
where chat_cmd is defined to add a branch option (e.g.,
chat_cmd.add_argument("--branch", help="Branch context for the chat",
default=... or required=...) so the chat command accepts branch context; ensure
the option name and help text match other commands that already use --branch to
maintain consistency with existing behavior.
- Around line 160-187: The polling loop in main.py never enforces a timeout so
the except TimeoutError branch is unreachable; update the loop that begins with
start_time = time.monotonic() / while True to compute elapsed each iteration and
compare it to a configurable timeout (e.g., PARSE_TIMEOUT_SECONDS or a CLI flag)
and when elapsed > timeout either raise TimeoutError or break/exit with the same
behavior as the current except block; make this change around the
get_parsing_status loop (references: start_time, while True,
client.get_parsing_status, TimeoutError) so a stuck parse will trigger the
timeout handling and show the timed-out message.
In `@pyproject.toml`:
- Around line 8-13: The TOML places the dependencies array under
[project.scripts], which breaks PEP 621; move the dependencies table so it is a
direct child of [project] (i.e., place the dependencies = [...] block before or
above the [project.scripts] section) and leave the potpie =
"potpie.cli.main:main" entry only inside [project.scripts]; ensure the final
structure has [project] with dependencies and then a separate [project.scripts]
containing the potpie entry.
---
Nitpick comments:
In `@potpie/cli/main.py`:
- Around line 133-135: Validate repo_path and branch locally before the API
call: ensure args.repo_path (repo_path) is non-empty, exists and is a directory
(os.path.exists/os.path.isdir) and that it contains a git repo (e.g., check for
.git or run a git command), and ensure args.branch (branch) is non-empty and
corresponds to an existing branch in that repo (e.g., git rev-parse --verify or
list branches). If any check fails, print a clear error and exit or raise an
argparse error instead of proceeding to the API submission so bad inputs are
rejected locally.
In `@tests/cli/test_main.py`:
- Around line 13-98: Add tests that cover timeout parsing and chat branch
semantics: extend tests in tests/cli/test_main.py to assert build_parser()
parses a --timeout value into args.timeout (and that default timeout is used
when omitted), and add a test that main([...]) with the chat command and a
--branch flag (e.g., ["chat","project-123","--branch","develop"]) results in
dispatch to cmd_chat with the branch preserved; reference build_parser, main,
args.timeout, and cmd_chat/_get_client in the new tests to locate where to
assert parsing and dispatch behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcd3540e-94ef-41c7-9a56-b7c13e5fce63
📒 Files selected for processing (7)
potpie/cli/__init__.pypotpie/cli/client.pypotpie/cli/main.pypyproject.tomltests/cli/__init__.pytests/cli/test_client.pytests/cli/test_main.py
| "/api/v1/parse", | ||
| json={ |
There was a problem hiding this comment.
Client endpoints are versioned to /api/v1, but backend routes are /api/v2.
These calls will fail with 404 against the current router contract.
🔧 Proposed fix
DEFAULT_BASE_URL = "http://localhost:8001"
DEFAULT_TIMEOUT = 30.0
+API_PREFIX = "/api/v2"
@@
- "/api/v1/parse",
+ f"{API_PREFIX}/parse",
@@
- f"/api/v1/parsing-status/{project_id}",
+ f"{API_PREFIX}/parsing-status/{project_id}",
@@
- "/api/v1/projects/list",
+ f"{API_PREFIX}/projects/list",
@@
- "/api/v1/list-available-agents",
+ f"{API_PREFIX}/list-available-agents",
@@
- "/api/v1/conversations/",
+ f"{API_PREFIX}/conversations/",
@@
- f"/api/v1/conversations/{conversation_id}/message/",
+ f"{API_PREFIX}/conversations/{conversation_id}/message/",Also applies to: 86-87, 132-133, 145-146, 172-173, 199-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@potpie/cli/client.py` around lines 66 - 67, The client is calling mounted
endpoints under "/api/v1" which no longer match the backend routes (they are
under "/api/v2"); update every request URL in potpie/cli/client.py that uses
"/api/v1" (e.g. the JSON POST to "/api/v1/parse" and the other occurrences at
the noted locations) to use the "/api/v2" base path instead so the requests
match the server routes; search for the string "/api/v1" in client.py and
replace with "/api/v2" in the requests (e.g., the requests.post calls
constructing "/api/v1/parse", "/api/v1/..." etc.) to ensure all endpoints target
the v2 API.
| state = status.get("status", "unknown") | ||
| style = "green" if state in ("ready", "completed", "parsed") else "yellow" if state == "processing" else "red" |
There was a problem hiding this comment.
Status coloring is case-sensitive and can mislabel valid success states.
Normalize status values before comparison so "READY"/"PARSED" render correctly.
🔧 Proposed fix
- state = status.get("status", "unknown")
- style = "green" if state in ("ready", "completed", "parsed") else "yellow" if state == "processing" else "red"
+ state = str(status.get("status", "unknown"))
+ normalized_state = state.lower()
+ style = "green" if normalized_state in ("ready", "completed", "parsed") else "yellow" if normalized_state == "processing" else "red"
@@
- status = p.get("status", "?")
- status_style = "green" if status in ("ready", "parsed") else "yellow"
+ status = str(p.get("status", "?"))
+ status_style = "green" if status.lower() in ("ready", "parsed") else "yellow"Also applies to: 250-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@potpie/cli/main.py` around lines 209 - 210, The status coloring compares the
raw status string case-sensitively; normalize the status before comparing by
replacing the assignment of state (from status.get("status", "unknown")) with a
lower-cased value (e.g., state = status.get("status", "unknown").lower()) and
update the style logic in the same place (the expression that sets style based
on state) to compare against lowercase literals ("ready", "completed", "parsed",
"processing"); repeat this normalization and comparison change for the second
occurrence of the same logic later in the file (the other style assignment
around the 250–251 area).
| [project.scripts] | ||
| potpie = "potpie.cli.main:main" | ||
|
|
||
| dependencies = [ | ||
| "aiohttp>=3.13.3", | ||
| "rich>=13.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import tomllib
from pathlib import Path
data = tomllib.loads(Path("pyproject.toml").read_text())
project = data.get("project", {})
scripts = project.get("scripts", {})
print("project.dependencies type:", type(project.get("dependencies")).__name__)
print("project.dependencies exists:", "dependencies" in project)
print("project.scripts keys:", sorted(scripts.keys()) if isinstance(scripts, dict) else scripts)
print("dependencies incorrectly under project.scripts:", isinstance(scripts, dict) and "dependencies" in scripts)
PYRepository: potpie-ai/potpie
Length of output: 232
dependencies is incorrectly nested under [project.scripts].
This violates PEP 621 metadata structure. The dependencies list should be a direct child of [project], not [project.scripts]. Move the [project.scripts] section after the dependencies list to fix the nesting.
Proposed fix
[project]
name = "potpie"
version = "0.1.0"
description = "Add your description here"
readme = "README.md"
requires-python = ">=3.10"
-
-[project.scripts]
-potpie = "potpie.cli.main:main"
dependencies = [
"aiohttp>=3.13.3",
"rich>=13.0.0", "greenlet>=3.3.0",
]
+
+[project.scripts]
+potpie = "potpie.cli.main:main"📝 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.
| [project.scripts] | |
| potpie = "potpie.cli.main:main" | |
| dependencies = [ | |
| "aiohttp>=3.13.3", | |
| "rich>=13.0.0", | |
| [project] | |
| name = "potpie" | |
| version = "0.1.0" | |
| description = "Add your description here" | |
| readme = "README.md" | |
| requires-python = ">=3.10" | |
| dependencies = [ | |
| "aiohttp>=3.13.3", | |
| "rich>=13.0.0", | |
| "greenlet>=3.3.0", | |
| ] | |
| [project.scripts] | |
| potpie = "potpie.cli.main:main" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 8 - 13, The TOML places the dependencies array
under [project.scripts], which breaks PEP 621; move the dependencies table so it
is a direct child of [project] (i.e., place the dependencies = [...] block
before or above the [project.scripts] section) and leave the potpie =
"potpie.cli.main:main" entry only inside [project.scripts]; ensure the final
structure has [project] with dependencies and then a separate [project.scripts]
containing the potpie entry.
- Fix auth: use x-api-key/x-user-id headers instead of isDevelopmentMode (reads from POTPIE_API_KEY or INTERNAL_ADMIN_SECRET env vars) - Fix conversation creation: use agent_ids (list) instead of agent_id - Fix parse polling: add actual timeout (600s) to prevent infinite hang - Fix status display: normalize status to lowercase before comparison - Add --branch flag to chat command per issue spec - Extract API_PREFIX constant for endpoint paths - Update tests to cover new auth headers and API prefix Addresses all critical and major findings from CodeRabbit review.
|
Thanks for the thorough review @coderabbitai! I've pushed fixes for all the findings:
Note on the API version: I verified that all routes in |
|
I'll kick off a fresh review to verify the updated code. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
potpie/cli/main.py (1)
251-253:⚠️ Potential issue | 🟡 MinorNormalize project status before color mapping.
Line 252 compares raw status values case-sensitively, so
"READY"/"PARSED"can be miscolored.🔧 Proposed fix
- status = p.get("status", "?") - status_style = "green" if status in ("ready", "parsed") else "yellow" - table.add_row(pid, name, branch, f"[{status_style}]{status}[/{status_style}]") + status = str(p.get("status", "?")) + status_norm = status.lower() + status_style = "green" if status_norm in ("ready", "completed", "parsed") else "yellow" + table.add_row(pid, name, branch, f"[{status_style}]{status}[/{status_style}]")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@potpie/cli/main.py` around lines 251 - 253, Normalize the project's status before deciding color: fetch the raw status with p.get("status", "?") into a variable (e.g., status_raw), compute a lowercased version (e.g., status_norm = status_raw.lower()), use status_norm when checking membership in ("ready", "parsed") to set status_style, but keep using the original status_raw (or a formatted display string) in table.add_row so the displayed label remains unchanged; update references around p.get, status_style, and table.add_row accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@potpie/cli/client.py`:
- Around line 36-47: Remove the placeholder "defaultuser" and fail fast when no
credentials are provided: in the constructor (where self._api_key and
self._user_id are set) stop defaulting self._user_id to "defaultuser" and only
assign it from the user_id argument or POTPIE_USER_ID env var (leave None/empty
if not present); validate that an API key exists (from api_key or POTPIE_API_KEY
or INTERNAL_ADMIN_SECRET) and raise a clear ValueError if missing; keep
_headers() as the single place that adds headers and ensure it only includes
"x-user-id" when self._user_id is explicitly present and "x-api-key" when
self._api_key is present so requests won’t send placeholder credentials.
In `@potpie/cli/main.py`:
- Around line 95-100: The readiness probe currently constructs PotpieClient()
with defaults and prints a hardcoded http://localhost:8001; update the start
logic to instantiate PotpieClient with the configured URL (use the CLI/options
variable or POTPIE_URL value passed into the command) so client.is_alive()
probes the correct endpoint, and replace the hardcoded URL in the console.print
with the same configured URL variable so the printed server address matches the
probe target.
- Around line 133-147: The CLI defers repo and branch validation to the server;
add preflight checks in main before calling client.parse: validate that
repo_path (from args.repo_path) is non-empty and exists on disk (use
os.path.exists/os.path.isdir) and that branch (args.branch) is non-empty; if
validation fails, print a clear console error and exit with sys.exit(1). Perform
these checks after assigning repo_path and branch and before the
client.is_alive() check or before calling client.parse to avoid unnecessary API
calls, and keep error messages consistent with existing console.print
formatting.
---
Duplicate comments:
In `@potpie/cli/main.py`:
- Around line 251-253: Normalize the project's status before deciding color:
fetch the raw status with p.get("status", "?") into a variable (e.g.,
status_raw), compute a lowercased version (e.g., status_norm =
status_raw.lower()), use status_norm when checking membership in ("ready",
"parsed") to set status_style, but keep using the original status_raw (or a
formatted display string) in table.add_row so the displayed label remains
unchanged; update references around p.get, status_style, and table.add_row
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e47b36b5-e47f-4213-ac40-3bd3987e7c55
📒 Files selected for processing (3)
potpie/cli/client.pypotpie/cli/main.pytests/cli/test_client.py
✅ Files skipped from review due to trivial changes (1)
- tests/cli/test_client.py
- Remove defaultuser fallback for user_id to prevent auth issues - Add ensure_auth() method for explicit credential validation - Fix start command to use _get_client(args) for correct URL handling - Show actual base_url instead of hardcoded localhost in start output - Add CLI-side input validation for parse: empty path/branch, local path existence - Only send x-user-id header when explicitly provided
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
potpie/cli/client.py (1)
40-51:⚠️ Potential issue | 🟠 MajorFail fast when using
INTERNAL_ADMIN_SECRETwithoutPOTPIE_USER_ID.Line 53 only validates API key presence. If the key is
INTERNAL_ADMIN_SECRET, Line 40-51 can still send nox-user-id, which the backend rejects as401 Invalid user_id(app/api/router.py:56-88). Please enforce this case inensure_auth()and callensure_auth()from_headers().🔧 Suggested fix
def _headers(self) -> dict[str, str]: """Build authentication headers for API requests. @@ - headers: dict[str, str] = {} - if self._api_key: - headers["x-api-key"] = self._api_key + self.ensure_auth() + headers: dict[str, str] = {"x-api-key": self._api_key} if self._user_id: headers["x-user-id"] = self._user_id return headers @@ def ensure_auth(self) -> None: @@ if not self._api_key: raise RuntimeError( "No API key configured. Set POTPIE_API_KEY or INTERNAL_ADMIN_SECRET " "environment variable, or pass --api-key to the CLI." ) + if ( + self._api_key == os.environ.get("INTERNAL_ADMIN_SECRET") + and not self._user_id + ): + raise RuntimeError( + "POTPIE_USER_ID is required when using INTERNAL_ADMIN_SECRET." + )Also applies to: 53-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@potpie/cli/client.py` around lines 40 - 51, The code must fail fast when the API key is the INTERNAL_ADMIN_SECRET but no user id is provided: update ensure_auth() to check if self._api_key equals INTERNAL_ADMIN_SECRET (or env POTPIE_INTERNAL_ADMIN_SECRET) and raise an authentication error if self._user_id (or env POTPIE_USER_ID) is missing; then call ensure_auth() at the start of _headers() so _headers() never returns headers with an admin key and no x-user-id. Reference: modify ensure_auth() and the _headers() method to perform this strict validation and raise/exit immediately when the admin-secret is used without a user id.potpie/cli/main.py (1)
267-269:⚠️ Potential issue | 🟡 MinorNormalize project status before styling.
Line 268 is case-sensitive and excludes
completed, so valid success states can be miscolored.🔧 Suggested fix
- status = p.get("status", "?") - status_style = "green" if status in ("ready", "parsed") else "yellow" + status = str(p.get("status", "?")) + normalized = status.lower() + status_style = "green" if normalized in ("ready", "completed", "parsed") else "yellow"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@potpie/cli/main.py` around lines 267 - 269, Normalize the project status string before styling: read status = p.get("status", "?") into a normalized variable (e.g., status_norm = (status or "?").lower()), use that to compute status_style (treat "ready", "parsed", and "completed" as success states, case-insensitively), and then use the original or normalized status when calling table.add_row so output still shows the original casing if desired; update the logic around status, status_style, and the table.add_row call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@potpie/cli/main.py`:
- Around line 145-150: The current check treats any string with a slash as a
local path, falsely rejecting remote repo forms like "owner/repo"; update the
logic around repo_path/local_path to first check if Path(repo_path).exists() and
only error when it does not exist and the input clearly looks like a filesystem
path (starts with "/" or "./" or "../" or contains a scheme like "://"); treat
inputs matching remote patterns (e.g., simple "owner/repo" or URLs) as remote
and skip the existence check. Locate the conditional using repo_path,
local_path, Path(repo_path), console.print and sys.exit and implement the
described ordering and pattern checks so remote forms are not rejected.
---
Duplicate comments:
In `@potpie/cli/client.py`:
- Around line 40-51: The code must fail fast when the API key is the
INTERNAL_ADMIN_SECRET but no user id is provided: update ensure_auth() to check
if self._api_key equals INTERNAL_ADMIN_SECRET (or env
POTPIE_INTERNAL_ADMIN_SECRET) and raise an authentication error if self._user_id
(or env POTPIE_USER_ID) is missing; then call ensure_auth() at the start of
_headers() so _headers() never returns headers with an admin key and no
x-user-id. Reference: modify ensure_auth() and the _headers() method to perform
this strict validation and raise/exit immediately when the admin-secret is used
without a user id.
In `@potpie/cli/main.py`:
- Around line 267-269: Normalize the project status string before styling: read
status = p.get("status", "?") into a normalized variable (e.g., status_norm =
(status or "?").lower()), use that to compute status_style (treat "ready",
"parsed", and "completed" as success states, case-insensitively), and then use
the original or normalized status when calling table.add_row so output still
shows the original casing if desired; update the logic around status,
status_style, and the table.add_row call accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e334982-4774-4d32-9210-b82d47a12c26
📒 Files selected for processing (2)
potpie/cli/client.pypotpie/cli/main.py
| # Check if local path exists (skip for remote repos like owner/repo) | ||
| if os.path.sep in repo_path or repo_path.startswith((".", "/")): | ||
| local_path = Path(repo_path) | ||
| if not local_path.exists(): | ||
| console.print(f"[red]Error:[/red] Local path '{repo_path}' does not exist.") | ||
| sys.exit(1) |
There was a problem hiding this comment.
Remote repo inputs are incorrectly rejected as missing local paths.
Line 146 treats any path containing / as local. That breaks documented inputs like owner/repo (Line 396) and URL forms, causing false “does not exist” errors.
🔧 Suggested fix
- # Check if local path exists (skip for remote repos like owner/repo)
- if os.path.sep in repo_path or repo_path.startswith((".", "/")):
- local_path = Path(repo_path)
- if not local_path.exists():
- console.print(f"[red]Error:[/red] Local path '{repo_path}' does not exist.")
- sys.exit(1)
+ # Check local path existence only when input is not a remote repo reference.
+ is_remote = repo_path.startswith(("http://", "https://", "git@", "ssh://")) or (
+ repo_path.count("/") == 1 and not repo_path.startswith((".", "/", "~"))
+ )
+ if not is_remote:
+ local_path = Path(repo_path).expanduser()
+ if not local_path.exists():
+ console.print(f"[red]Error:[/red] Local path '{repo_path}' does not exist.")
+ sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@potpie/cli/main.py` around lines 145 - 150, The current check treats any
string with a slash as a local path, falsely rejecting remote repo forms like
"owner/repo"; update the logic around repo_path/local_path to first check if
Path(repo_path).exists() and only error when it does not exist and the input
clearly looks like a filesystem path (starts with "/" or "./" or "../" or
contains a scheme like "://"); treat inputs matching remote patterns (e.g.,
simple "owner/repo" or URLs) as remote and skip the existence check. Locate the
conditional using repo_path, local_path, Path(repo_path), console.print and
sys.exit and implement the described ordering and pattern checks so remote forms
are not rejected.
PR SummaryThis PR adds a CLI tool for local Potpie usage, implementing commands to start/stop services, parse repositories, check status, list projects/agents, and chat interactively with streaming responses.
Estimated effort: moderate
|
| ) -> None: | ||
| self.base_url = base_url.rstrip("/") | ||
| self.timeout = timeout | ||
| self._api_key = api_key or os.environ.get("POTPIE_API_KEY") or os.environ.get("INTERNAL_ADMIN_SECRET") or "" |
There was a problem hiding this comment.
[warning][security] Empty string fallback for API key could mask missing auth configuration
Risk: May silently fail auth checks if neither env var is set
Suggested change: Remove empty string fallback and let ensure_auth() handle missing keys
Confidence: high
| sys.exit(e.returncode) | ||
|
|
||
|
|
||
| def cmd_parse(args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
[warning][security] Repository path validation should handle path traversal attempts
Risk: Malicious paths could access files outside intended directory
Suggested change: Add Path(repo_path).resolve().is_relative_to(Path.cwd()) check for local paths
Confidence: high
| console.print(" Parsing", end="") | ||
| max_wait = 600.0 | ||
| try: | ||
| start_time = time.monotonic() |
There was a problem hiding this comment.
[warning][correctness] Status check should handle HTTP errors explicitly
Risk: Network errors could be misinterpreted as parsing failures
Suggested change: Catch httpx.HTTPStatusError separately from other exceptions
Confidence: medium
| console.print("[bold blue]Potpie>[/bold blue] ", end="") | ||
| try: | ||
| full_response = "" | ||
| for chunk in client.send_message(conversation_id, user_input): |
There was a problem hiding this comment.
[warning][reliability] Streaming response should have timeout protection
Risk: Hanging connections could block CLI indefinitely
Suggested change: Add timeout parameter to client.send_message()
Confidence: medium
|
|
||
| # Create conversation | ||
| try: | ||
| conv = client.create_conversation(project_id, agent) |
There was a problem hiding this comment.
[warning][correctness] Agent validation should check against list_agents()
Risk: Invalid agent names could cause confusing errors
Suggested change: Pre-validate agent name against available agents
Confidence: high
PR-level observations
|
PR Review Agent summary
|
PR SummaryThis PR adds a CLI tool for local Potpie usage, implementing commands to start/stop services, parse repositories, check status, list projects/agents, and chat interactively with streaming responses. The CLI wraps the existing REST API with a thin HTTP client and provides rich terminal output.
Estimated effort: moderate
|
| ) -> None: | ||
| self.base_url = base_url.rstrip("/") | ||
| self.timeout = timeout | ||
| self._api_key = api_key or os.environ.get("POTPIE_API_KEY") or os.environ.get("INTERNAL_ADMIN_SECRET") or "" |
There was a problem hiding this comment.
[warning][security] Long line with multiple env var fallbacks makes secret handling hard to audit
Risk: Complex secret fallback chains can hide credential issues and make debugging harder
Suggested change: Break into multiple lines with clear comments for each fallback source
Confidence: medium
| return status | ||
| if time.monotonic() - start > max_wait: | ||
| raise TimeoutError( | ||
| f"Parsing did not complete within {max_wait}s (last status: {state})" |
There was a problem hiding this comment.
[warning][reliability] Timeout error message should include project_id for debugging
Risk: Without project_id in error, debugging failed parsing jobs is harder
Suggested change: Include project_id in timeout error message
Confidence: high
| console.print() | ||
|
|
||
| try: | ||
| process = subprocess.Popen( |
There was a problem hiding this comment.
[warning][security] Subprocess call uses untrusted input without validation
Risk: Potential command injection if start_script contains malicious content
Suggested change: Validate start_script path against allowlist of known scripts
Confidence: high
|
|
||
| try: | ||
| process = subprocess.Popen( | ||
| ["bash", str(start_script)], |
There was a problem hiding this comment.
[warning][reliability] Subprocess uses partial executable path ('bash')
Risk: Relies on PATH lookup which may resolve to unexpected binary
Suggested change: Use full path to bash executable (e.g. /bin/bash)
Confidence: medium
| sys.exit(e.returncode) | ||
|
|
||
|
|
||
| def cmd_parse(args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
[warning][maintainability] cmd_parse function complexity exceeds threshold (13 > 10)
Risk: High complexity makes code harder to maintain and test
Suggested change: Refactor into smaller helper methods
Confidence: high
| sys.exit(1) | ||
|
|
||
| if not client.is_alive(): | ||
| console.print("[red]Error:[/red] Potpie server is not running. Run [bold]potpie start[/bold] first.") |
There was a problem hiding this comment.
[warning][correctness] Error message assumes server is always started via CLI
Risk: Misleading message if server is started via other means
Suggested change: Generalize message to not assume CLI start method
Confidence: medium
PR Review Agent summary
|
PR SummaryThis PR adds a CLI tool for local Potpie usage, implementing commands to start/stop services, parse repositories, check status, list projects/agents, and chat interactively with streaming responses. The CLI wraps the existing REST API with a thin HTTP client and provides rich terminal output.
Estimated effort: moderate
|
| return status | ||
| if time.monotonic() - start > max_wait: | ||
| raise TimeoutError( | ||
| f"Parsing did not complete within {max_wait}s (last status: {state})" |
There was a problem hiding this comment.
[warning][reliability] Timeout error message could be more actionable
Risk: Users may not know how to proceed when parsing times out
Suggested change: Include troubleshooting steps in timeout message (e.g. check server logs, try smaller repo)
Confidence: medium
| """Walk up from cwd to find the Potpie project root (has compose.yaml).""" | ||
| current = Path.cwd() | ||
| for parent in [current, *current.parents]: | ||
| if (parent / "compose.yaml").exists() and (parent / "scripts" / "start.sh").exists(): |
There was a problem hiding this comment.
[warning][correctness] Path validation could incorrectly match GitHub owner/repo format
Risk: Remote repo identifiers containing slashes (owner/repo) will be incorrectly treated as local paths
Suggested change: First check for GitHub-style owner/repo format before testing as local path
Confidence: high
| console.print() | ||
|
|
||
| try: | ||
| process = subprocess.Popen( |
There was a problem hiding this comment.
[warning][reliability] Subprocess lacks signal handling for clean shutdown
Risk: If parent process is killed, child processes could become orphaned
Suggested change: Add signal handler to properly terminate subprocess on SIGTERM/SIGINT
Confidence: medium
| console.print(Panel.fit("🛑 Stopping Potpie", style="bold red")) | ||
|
|
||
| try: | ||
| subprocess.run( |
There was a problem hiding this comment.
[warning][reliability] Subprocess lacks signal handling for clean shutdown
Risk: If parent process is killed, child processes could become orphaned
Suggested change: Add signal handler to properly terminate subprocess on SIGTERM/SIGINT
Confidence: medium
| sys.exit(e.returncode) | ||
|
|
||
|
|
||
| def cmd_parse(args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
[warning][maintainability] cmd_parse function is too complex (C901 > 10)
Risk: High complexity makes the function harder to maintain and test
Suggested change: Break into smaller helper functions for parse submission, polling, and status handling
Confidence: high
PR Review Agent summary
|
PR SummaryThis PR adds a CLI tool for local Potpie usage, implementing commands to start/stop services, parse repositories, check status, list projects/agents, and chat interactively with streaming responses. The CLI wraps the existing REST API with a thin HTTP client and provides rich terminal output.
Estimated effort: moderate
|
| ) -> None: | ||
| self.base_url = base_url.rstrip("/") | ||
| self.timeout = timeout | ||
| self._api_key = api_key or os.environ.get("POTPIE_API_KEY") or os.environ.get("INTERNAL_ADMIN_SECRET") or "" |
There was a problem hiding this comment.
[warning][security] API key fallback to empty string may cause silent auth failures
Risk: Silent auth failures make debugging harder and could lead to unexpected behavior
Suggested change: Remove empty string fallback to force explicit auth configuration
Confidence: high
| ) -> None: | ||
| self.base_url = base_url.rstrip("/") | ||
| self.timeout = timeout | ||
| self._api_key = api_key or os.environ.get("POTPIE_API_KEY") or os.environ.get("INTERNAL_ADMIN_SECRET") or "" |
There was a problem hiding this comment.
[warning][maintainability] Long line exceeds PEP 8 maximum length
Risk: Reduces code readability and maintainability
Suggested change: Break line into multiple lines using parentheses
Confidence: high
| return status | ||
| if time.monotonic() - start > max_wait: | ||
| raise TimeoutError( | ||
| f"Parsing did not complete within {max_wait}s (last status: {state})" |
There was a problem hiding this comment.
[warning][maintainability] Long line exceeds PEP 8 maximum length
Risk: Reduces code readability and maintainability
Suggested change: Break line into multiple lines using parentheses
Confidence: high
| """Walk up from cwd to find the Potpie project root (has compose.yaml).""" | ||
| current = Path.cwd() | ||
| for parent in [current, *current.parents]: | ||
| if (parent / "compose.yaml").exists() and (parent / "scripts" / "start.sh").exists(): |
There was a problem hiding this comment.
[warning][correctness] Path validation could be more robust
Risk: Current validation may incorrectly match remote identifiers containing forward slashes
Suggested change: Use Path(x).exists() OR regex to detect owner/repo format first
Confidence: medium
| console.print() | ||
|
|
||
| try: | ||
| process = subprocess.Popen( |
There was a problem hiding this comment.
[warning][reliability] Subprocess call lacks signal handler
Risk: Process may become orphaned if parent is killed
Suggested change: Add signal handler to ensure clean process termination
Confidence: medium
|
|
||
| try: | ||
| process = subprocess.Popen( | ||
| ["bash", str(start_script)], |
There was a problem hiding this comment.
[warning][security] Subprocess call uses partial executable path
Risk: Partial paths may execute unexpected binaries if PATH is compromised
Suggested change: Use full path to bash executable
Confidence: medium
| console.print(Panel.fit("🛑 Stopping Potpie", style="bold red")) | ||
|
|
||
| try: | ||
| subprocess.run( |
There was a problem hiding this comment.
[warning][reliability] Subprocess call lacks signal handler
Risk: Process may become orphaned if parent is killed
Suggested change: Add signal handler to ensure clean process termination
Confidence: medium
|
|
||
| try: | ||
| subprocess.run( | ||
| ["bash", str(stop_script)], |
There was a problem hiding this comment.
[warning][security] Subprocess call uses partial executable path
Risk: Partial paths may execute unexpected binaries if PATH is compromised
Suggested change: Use full path to bash executable
Confidence: medium
| sys.exit(e.returncode) | ||
|
|
||
|
|
||
| def cmd_parse(args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
[warning][maintainability] cmd_parse function is too complex
Risk: High complexity makes code harder to maintain and test
Suggested change: Refactor into smaller, focused functions
Confidence: medium
| sys.exit(1) | ||
|
|
||
| if not client.is_alive(): | ||
| console.print("[red]Error:[/red] Potpie server is not running. Run [bold]potpie start[/bold] first.") |
There was a problem hiding this comment.
[warning][correctness] Error message could be more informative
Risk: Users may not understand how to fix the server not running issue
Suggested change: Include troubleshooting tips in error message
Confidence: medium
PR Review Agent summary
|
PR SummaryThis PR adds a CLI tool for local Potpie usage, implementing commands to start/stop services, parse repositories, check status, list projects/agents, and chat interactively with streaming responses. The CLI wraps the existing REST API with a thin HTTP client and provides rich terminal output.
Estimated effort: moderate
|
| return status | ||
| if time.monotonic() - start > max_wait: | ||
| raise TimeoutError( | ||
| f"Parsing did not complete within {max_wait}s (last status: {state})" |
There was a problem hiding this comment.
[warning][reliability] Timeout error message could be more actionable
Risk: Users may not know how to recover from parsing timeout
Suggested change: Add guidance on how to check parsing logs or retry
Confidence: medium
| """Walk up from cwd to find the Potpie project root (has compose.yaml).""" | ||
| current = Path.cwd() | ||
| for parent in [current, *current.parents]: | ||
| if (parent / "compose.yaml").exists() and (parent / "scripts" / "start.sh").exists(): |
There was a problem hiding this comment.
[warning][correctness] Path validation could incorrectly match GitHub owner/repo format
Risk: Remote repo identifiers containing forward slashes (like 'owner/repo') could be incorrectly treated as local paths
Suggested change: Add explicit check for GitHub-style owner/repo format before checking local paths
Confidence: medium
| console.print() | ||
|
|
||
| try: | ||
| process = subprocess.Popen( |
There was a problem hiding this comment.
[warning][reliability] Subprocess could become orphaned if parent is killed
Risk: If the CLI process is killed while start.sh is running, the child process may continue running
Suggested change: Add signal handler to clean up subprocess on SIGTERM/SIGINT
Confidence: medium
| console.print(Panel.fit("🛑 Stopping Potpie", style="bold red")) | ||
|
|
||
| try: | ||
| subprocess.run( |
There was a problem hiding this comment.
[warning][reliability] Subprocess could become orphaned if parent is killed
Risk: If the CLI process is killed while stop.sh is running, the child process may continue running
Suggested change: Add signal handler to clean up subprocess on SIGTERM/SIGINT
Confidence: medium
| sys.exit(e.returncode) | ||
|
|
||
|
|
||
| def cmd_parse(args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
[warning][maintainability] Function complexity exceeds recommended threshold (13 > 10)
Risk: High complexity makes the function harder to understand and maintain
Suggested change: Extract polling logic and status handling into separate helper functions
Confidence: high
| console.print("[bold blue]Potpie>[/bold blue] ", end="") | ||
| try: | ||
| full_response = "" | ||
| for chunk in client.send_message(conversation_id, user_input): |
There was a problem hiding this comment.
[warning][reliability] Streaming response lacks timeout
Risk: Hanging connections could block the CLI indefinitely
Suggested change: Add timeout parameter to streaming request
Confidence: medium
PR Review Agent summary
|
PR SummaryThis PR adds a CLI tool for local Potpie usage, implementing commands to start/stop services, parse repositories, check status, list projects/agents, and chat interactively with streaming responses. The CLI wraps the existing REST API with a thin HTTP client and provides rich terminal output.
Estimated effort: moderate
|
| sys.exit(e.returncode) | ||
|
|
||
|
|
||
| def cmd_parse(args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
[warning][correctness] Path validation could incorrectly match GitHub owner/repo format
Risk: Remote repo identifiers containing forward slashes (owner/repo) could be incorrectly treated as local paths
Suggested change: First check for GitHub-style owner/repo format before testing as local path
Confidence: medium
PR Review Agent summary
|
PR SummaryThis PR adds a CLI tool for local Potpie usage, implementing commands to start/stop services, parse repositories, check status, list projects/agents, and chat interactively with streaming responses. The CLI wraps the existing REST API with a thin HTTP client and provides rich terminal output.
Estimated effort: moderate
|
| description = "Add your description here" | ||
| readme = "README.md" | ||
| requires-python = ">=3.10" | ||
|
|
There was a problem hiding this comment.
[error][correctness] dependencies nested under [project.scripts] — breaks pip install
Risk: pip will ignore the dependencies list entirely. Package installs without its deps.
Suggested change: Move dependencies = [...] to be a direct child of [project], not [project.scripts].
Confidence: high
| sys.exit(1) | ||
|
|
||
| # Check if local path exists (skip for remote repos like owner/repo) | ||
| if os.path.sep in repo_path or repo_path.startswith((".", "/")): |
There was a problem hiding this comment.
[error][correctness] os.path.sep in x incorrectly matches owner/repo format on Unix
Risk: Remote repo identifiers like owner/repo contain '/' which is os.path.sep on Unix, causing them to be treated as local paths and rejected as missing.
Suggested change: Check Path(x).exists() for local paths, or match owner/repo with re.match(r'^[\w.-]+/[\w.-]+$', x) first.
Confidence: high
|
|
||
| DEFAULT_BASE_URL = "http://localhost:8001" | ||
| DEFAULT_TIMEOUT = 30.0 | ||
| API_PREFIX = "/api/v1" |
There was a problem hiding this comment.
[error][correctness] /api/v1 client calls but router registers /api/v2
Risk: Client and router API versions diverge (/api/v1 vs /api/v2). Calls may 404 or hit the wrong handlers at runtime.
Suggested change: Update the client hardcoded base URL/prefix to match the router prefix (/api/v2), or change the router prefix to /api/v1 intentionally.
Confidence: high
Cross-file refs: app/main.py
| """Test that API prefix is correctly configured.""" | ||
|
|
||
| def test_api_prefix_value(self): | ||
| assert API_PREFIX == "/api/v1" |
There was a problem hiding this comment.
[error][correctness] /api/v1 client calls but router registers /api/v2
Risk: Client and router API versions diverge (/api/v1 vs /api/v2). Calls may 404 or hit the wrong handlers at runtime.
Suggested change: Update the client hardcoded base URL/prefix to match the router prefix (/api/v2), or change the router prefix to /api/v1 intentionally.
Confidence: high
Cross-file refs: app/main.py
| return status | ||
| if time.monotonic() - start > max_wait: | ||
| raise TimeoutError( | ||
| f"Parsing did not complete within {max_wait}s (last status: {state})" |
There was a problem hiding this comment.
[warning][reliability] Timeout error message could leak internal state
Risk: Exposing internal status in error messages could aid attackers
Suggested change: Use a generic timeout message without internal status details
Confidence: medium
| console.print() | ||
|
|
||
| try: | ||
| process = subprocess.Popen( |
There was a problem hiding this comment.
[warning][reliability] subprocess.Popen without signal handler risks orphaned processes
Risk: If parent process is killed, child process may continue running indefinitely
Suggested change: Add signal handler to ensure child process is terminated on parent exit
Confidence: high
| console.print(Panel.fit("🛑 Stopping Potpie", style="bold red")) | ||
|
|
||
| try: | ||
| subprocess.run( |
There was a problem hiding this comment.
[warning][reliability] subprocess.run without signal handler risks orphaned processes
Risk: If parent process is killed, child process may continue running indefinitely
Suggested change: Add signal handler to ensure child process is terminated on parent exit
Confidence: high
| sys.exit(e.returncode) | ||
|
|
||
|
|
||
| def cmd_parse(args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
[warning][maintainability] cmd_parse function is too complex (13 > 10)
Risk: High complexity makes the function harder to maintain and test
Suggested change: Refactor into smaller helper functions
Confidence: high
| elapsed = time.monotonic() - start_time | ||
| console.print(f" [green]✓ Complete![/green] ({elapsed:.0f}s)") | ||
| console.print() | ||
| console.print(f" You can now chat with this project:") |
There was a problem hiding this comment.
[warning][maintainability] Unnecessary f-string prefix
Risk: Redundant syntax makes code harder to read
Suggested change: Remove f-string prefix since no variables are interpolated
Confidence: high
| return | ||
|
|
||
| if state in ("error", "failed"): | ||
| console.print(f" [red]✗ Failed![/red]") |
There was a problem hiding this comment.
[warning][maintainability] Unnecessary f-string prefix
Risk: Redundant syntax makes code harder to read
Suggested change: Remove f-string prefix since no variables are interpolated
Confidence: high
PR-level observations
|
PR Review Agent summary
|
2 participants
Summary
Implements the CLI tool requested in #224.
Commands
potpie startpotpie stoppotpie parse <path> [--branch <name>]potpie chat <project-id> --agent <name>potpie status <project-id>potpie projectspotpie agentsArchitecture
Design Decisions
parsecommand shows dots while waitinghttpxstreaming[project.scripts] potpie = potpie.cli.main:mainto pyproject.tomlTests
tests/cli/test_client.py— client unit tests (health, polling, timeout)tests/cli/test_main.py— argument parsing and command dispatch testsUsage
Closes #224
Summary by CodeRabbit
New Features
Tests