Nmt/conv #130
Nmt/conv #130
Conversation
Implement interactive conversation CLI following existing aegis TUI patterns: - Add 'conversation' command to main CLI with login requirement - Create Rich-based TUI with conversation loop and message history - Support conversation commands: help, clear, new, quit - Format AI responses with panels and JSON syntax highlighting - Auto-generate conversation IDs and maintain session state - Integration with Chariot conversation API endpoint Usage: praetorian-cli conversation Features: - Clean command-driven interface matching aegis style - Formatted display of query results with JSON highlighting - Session management with conversation persistence - Help system with query examples - Keyboard interrupt handling and graceful exit
- Fix decorator import: use cli_handler instead of with_chariot_login_required - Fix API call: use sdk._make_request and sdk.url() methods - Add test script for debugging conversation API integration Ready for testing with corrected Bedrock model ID.
Test debugging complete - CLI implementation verified working.
- Add debug logging to see actual tool input parameter keys - Handle both nested query format and direct parameter format - Add getKeys helper for debugging tool input structure This will help identify how the AI is structuring tool calls and enable flexible parameter parsing.
Implement smart tool output display: 1. Truncate JSON tool output to first 5 lines for readability 2. Show truncation indicator with line count 3. Display AI summary prominently above tool output 4. Use color coding: green for complete, yellow for truncated 5. Maintain full JSON formatting for short outputs This provides clean, digestible output while indicating when full tool results are available but truncated for readability.
Add CONVERSATION_TEST.md with: - Manual testing instructions for interactive CLI - Expected output examples with summarization + truncation - Alternative testing methods and troubleshooting - Command examples for validation Ready for manual testing of complete conversation flow.
Move conversation from top-level to agent subcommand following MCP pattern: - Add conversation as @agent.command() in handlers/agent.py - Remove standalone conversation handler - Update main.py imports and command registration - Update test documentation for new command path New usage: praetorian-cli chariot agent conversation This follows the established pattern where AI features are organized under the agent subcommand alongside MCP.
- Add special display formatting for scan completion messages - Support **Scan Complete** and **Scan Failed** message prefixes - Green/red color coding for scan result status - Enhanced UI feedback for asynchronous job completion events
- Remove client-side conversation ID generation - Send first message without conversationId field - Extract conversation UUID from server response - Use server-provided ID for subsequent messages in conversation - Implement secure conversation creation flow (server controls IDs)
- Accept both 200 and 201 status codes as success - Handle conversation creation response (201) properly - Extract conversation UUID from creation response - Show success message for conversation creation instead of error
- Only handle 200 status code since backend now returns 200 for all success - Extract conversation ID from first message response - Simplify response parsing for unified conversation creation flow
- Add string response type handling for direct AI responses - Handle both string and object response formats - Fix 'str' object has no attribute 'get' error - Support unified response format from conversation API
- Add startup menu to choose new conversation or resume existing - Implement conversation polling for real-time AI responses - Add conversation listing with message counts and creation dates - Replace immediate response with polling until AI responds - Remove legacy response handling and queued message display - Add conversation resume functionality with table selection - Implement proper message polling and display pipeline
- Replace non-existent endpoints with sdk.my() calls
- Use #conversation# prefix for listing conversations
- Use #message#{conversation_id} prefix for message polling
- Fix conversation resume functionality with proper SDK integration
- Sort messages by timestamp for correct display order
- Replace incorrect my() usage with search.by_key_prefix() - Use proper SDK search interface for conversations and messages - Fix conversation resume and message polling functionality - Follow established SDK patterns from existing codebase
- Poll conversation messages continuously until AI (chariot role) responds - Keep thinking spinner active while most recent message is from user - Stop spinner only when AI provides response - Handle asynchronous processing properly with real-time polling
Backend: - Replace AmazonDynamoDBFullAccess with DatabaseRW policy for PlannerEventsFunction - Add Topic field to Conversation model for first message preview - Set conversation topic from first message (256 chars) on creation CLI: - Display conversation topic in resume conversation table - Add topic column to conversation selection interface - Truncate topics to 40 chars for table display
- Show tool call messages as '🔧 Executing {tool_name} tool...'
- Show tool response completion as '✅ Tool execution completed'
- Extract tool name from toolUseContent JSON for better UX
- Provide real-time feedback during tool chaining workflows
- Add debug output to see actual toolUseContent structure - Try multiple possible field names for tool name - Help identify why tool names show as 'unknown'
- Change to simple '🔧 Executing tool...' message - Remove complex tool name extraction logic - Clean up tool execution display for better simplicity
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 472-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
|
Caution Review failedThe pull request is closed. Walkthrough
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 4
🧹 Nitpick comments (11)
docs/webpage-linking.md (1)
28-29: Repository key example appears inconsistent (.git duplicated in “name”)Examples show “#repo.git”/“#app.git” as the final name component. If “name” is the repo name, recommend dropping “.git”.
Apply this diff to correct the examples:
- praetorian chariot link webpage-source "#webpage#https://example.com/login" "#repository#https://github.com/org/repo.git#repo.git" + praetorian chariot link webpage-source "#webpage#https://example.com/login" "#repository#https://github.com/org/repo.git#repo"- entity_key='#repository#https://github.com/org/app.git#app.git' + entity_key='#repository#https://github.com/org/app.git#app'-- Example: `#repository#https://github.com/org/repo.git#repo.git` +- Example: `#repository#https://github.com/org/repo.git#repo`Also applies to: 65-69, 99-101
CONVERSATION_TEST.md (2)
31-31: Add language identifiers to fenced code blocksFixes markdownlint MD040 by specifying a language.
Apply this diff:
-``` +```text-``` +```textAlso applies to: 38-38
7-9: Avoid user-specific paths; prefer CLI invocationMake steps portable and simpler to run.
Apply this diff:
-cd /Users/ntutt/chariot-dev/chariot-development-platform/modules/praetorian-cli -python3 -m praetorian_cli.main chariot agent conversation +# From repo root (installed CLI) +praetorian chariot agent conversation +# Or, if running from source +python3 -m praetorian_cli.main chariot agent conversationtest_tool_execution.py (2)
1-1: Minor fixes: remove shebang, use default Keychain, add request timeout, drop empty f-stringImproves portability and robustness for manual runs.
Apply this diff:
-#!/usr/bin/env python3- keychain = Keychain("United States", None) + # Use default profile and environment/keychain configuration + keychain = Keychain()- response = chariot._make_request("POST", chariot.url("/conversations"), json={ + response = chariot._make_request("POST", chariot.url("/conversations"), timeout=30, json={- print(f"Full AI Response:") + print("Full AI Response:")Also applies to: 10-11, 16-16, 25-25
4-4: Narrow exception handling for network errorsCatch requests errors explicitly; keep generic fallback after.
Apply this diff:
-import uuid +import uuid +import requests- except Exception as e: + except requests.RequestException as e: + print(f"Network error: {e}") + except Exception as e: print(f"Test failed: {e}") import traceback traceback.print_exc()Also applies to: 70-74
praetorian_cli/ui/conversation/menu.py (6)
69-72: Use Rich’s clear() instead of spawning a shellSafer and cross-platform without shell invocation.
Apply this diff:
def clear_screen(self) -> None: """Clear the screen""" - os.system('clear' if os.name == 'posix' else 'cls') + self.console.clear()
227-231: Remove unusedstatusvariableSilences linter warning; variable isn’t used.
Apply this diff:
- ) as status: + ):
75-75: Drop extraneous f-stringsThese strings have no placeholders.
Apply this diff:
- self.console.print(f"\n[bold blue]Chariot AI Assistant[/bold blue]") + self.console.print("\n[bold blue]Chariot AI Assistant[/bold blue]")- self.console.print(f"\n[bold blue]Chariot AI Assistant[/bold blue]") + self.console.print("\n[bold blue]Chariot AI Assistant[/bold blue]")- self.console.print(f"[dim]🔧 Executing tool...[/dim]") + self.console.print("[dim]🔧 Executing tool...[/dim]") - elif role == 'tool response': - self.console.print(f"[dim]✅ Tool execution completed[/dim]") + elif role == 'tool response': + self.console.print("[dim]✅ Tool execution completed[/dim]")Also applies to: 105-105, 278-281
98-99: Avoid silent exception swallowing; print a dim hintKeeps UX friendly while not hiding issues entirely.
Apply this diff:
- except Exception: - pass + except Exception: + self.console.print("[dim]Job summary unavailable[/dim]")- except Exception: - pass + except Exception: + self.console.print("[dim]Failed to load recent conversations[/dim]") + return []- except Exception: - pass + except Exception: + self.console.print("[dim]Failed to get message count[/dim]") return 0Also applies to: 174-176, 185-187
26-27: Make user lookup resilientGuard against token/claims parsing errors.
Apply this diff:
- self.user_email, self.username = self.sdk.get_current_user() + try: + self.user_email, self.username = self.sdk.get_current_user() + except Exception: + self.user_email, self.username = ("", "unknown")
304-304: Add HTTP timeout to conversation API callPrevents hangs on network issues.
Apply this diff:
- response = self.sdk._make_request("POST", url, json=payload) + response = self.sdk._make_request("POST", url, json=payload, timeout=30)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
CONVERSATION_TEST.md(1 hunks)docs/webpage-linking.md(1 hunks)praetorian_cli/handlers/agent.py(1 hunks)praetorian_cli/ui/conversation/menu.py(1 hunks)test_tool_execution.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
praetorian_cli/handlers/agent.py (3)
praetorian_cli/sdk/chariot.py (1)
agent(232-236)praetorian_cli/handlers/cli_decorators.py (1)
cli_handler(48-52)praetorian_cli/ui/conversation/menu.py (1)
run_conversation_menu(469-472)
test_tool_execution.py (2)
praetorian_cli/sdk/chariot.py (3)
Chariot(31-282)_make_request(67-79)url(238-239)praetorian_cli/sdk/keychain.py (1)
Keychain(23-186)
praetorian_cli/ui/conversation/menu.py (4)
praetorian_cli/handlers/chariot.py (1)
chariot(6-17)praetorian_cli/sdk/chariot.py (4)
Chariot(31-282)get_current_user(258-282)url(238-239)_make_request(67-79)praetorian_cli/sdk/keychain.py (1)
username(116-118)praetorian_cli/sdk/entities/search.py (1)
by_key_prefix(20-33)
🪛 markdownlint-cli2 (0.18.1)
CONVERSATION_TEST.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.13.1)
test_tool_execution.py
1-1: Shebang is present but file is not executable
(EXE001)
25-25: f-string without any placeholders
Remove extraneous f prefix
(F541)
70-70: Do not catch blind exception: Exception
(BLE001)
praetorian_cli/ui/conversation/menu.py
1-1: Shebang is present but file is not executable
(EXE001)
71-71: Starting a process with a shell, possible injection detected
(S605)
75-75: f-string without any placeholders
Remove extraneous f prefix
(F541)
98-99: try-except-pass detected, consider logging the exception
(S110)
98-98: Do not catch blind exception: Exception
(BLE001)
105-105: f-string without any placeholders
Remove extraneous f prefix
(F541)
164-164: Do not catch blind exception: Exception
(BLE001)
174-175: try-except-pass detected, consider logging the exception
(S110)
174-174: Do not catch blind exception: Exception
(BLE001)
185-186: try-except-pass detected, consider logging the exception
(S110)
185-185: Do not catch blind exception: Exception
(BLE001)
231-231: Local variable status is assigned to but never used
Remove assignment to unused variable status
(F841)
255-255: Do not catch blind exception: Exception
(BLE001)
278-278: f-string without any placeholders
Remove extraneous f prefix
(F541)
280-280: f-string without any placeholders
Remove extraneous f prefix
(F541)
284-284: Do not catch blind exception: Exception
(BLE001)
446-446: Do not catch blind exception: Exception
(BLE001)
453-453: Consider moving this statement to an else block
(TRY300)
454-454: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
praetorian_cli/handlers/agent.py (1)
73-101: Nice addition: clean CLI entrypoint for conversationsCommand is simple, uses cli_handler, and lazy-imports the UI runner. LGTM.
Please confirm this new command appears in help output (praetorian chariot agent --help) and that CONVERSATION_TEST.md reflects the final invocation string.
docs/webpage-linking.md
Outdated
| # Get webpage details (including linked artifacts) | ||
| webpage = chariot.webpage.get('#webpage#https://example.com') | ||
| if 'artifacts' in webpage: | ||
| for artifact in webpage['artifacts']: | ||
| print(f"Linked artifact: {artifact['key']} (Secret: {artifact.get('secret', 'N/A')})") | ||
|
|
There was a problem hiding this comment.
Do not print secrets/tokens in examples
The SDK example prints artifact secrets to stdout. Avoid leaking sensitive data.
Apply this diff to mask/remove secret output:
-# Get webpage details (including linked artifacts)
-webpage = chariot.webpage.get('#webpage#https://example.com')
-if 'artifacts' in webpage:
- for artifact in webpage['artifacts']:
- print(f"Linked artifact: {artifact['key']} (Secret: {artifact.get('secret', 'N/A')})")
+# Get webpage details (including linked artifacts)
+webpage = chariot.webpage.get('#webpage#https://example.com')
+if 'artifacts' in webpage:
+ for artifact in webpage['artifacts']:
+ # Avoid printing secrets/tokens
+ secret_present = 'secret' in artifact and artifact.get('secret')
+ print(f"Linked artifact: {artifact['key']}" + (" (secret present)" if secret_present else ""))📝 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.
| # Get webpage details (including linked artifacts) | |
| webpage = chariot.webpage.get('#webpage#https://example.com') | |
| if 'artifacts' in webpage: | |
| for artifact in webpage['artifacts']: | |
| print(f"Linked artifact: {artifact['key']} (Secret: {artifact.get('secret', 'N/A')})") | |
| # Get webpage details (including linked artifacts) | |
| webpage = chariot.webpage.get('#webpage#https://example.com') | |
| if 'artifacts' in webpage: | |
| for artifact in webpage['artifacts']: | |
| # Avoid printing secrets/tokens | |
| secret_present = 'secret' in artifact and artifact.get('secret') | |
| print(f"Linked artifact: {artifact['key']}" + (" (secret present)" if secret_present else "")) |
🤖 Prompt for AI Agents
In docs/webpage-linking.md around lines 76 to 81, the example prints artifact
secrets to stdout which can leak sensitive tokens; modify the example to stop
printing the secret value directly — either remove the secret field from the
output or replace it with a masked placeholder (e.g., "REDACTED" or "****"), or
print only a boolean/presence indicator (e.g., "has_secret: true") so examples
demonstrate the field without exposing secrets.
| # Poll until AI responds (most recent message is from AI) | ||
| while True: | ||
| if self.conversation_id: | ||
| messages, _ = self.sdk.search.by_key_prefix(f"#message#{self.conversation_id}", pages=1) | ||
| if messages: | ||
| # Sort by timestamp to get most recent | ||
| messages = sorted(messages, key=lambda x: x.get('timestamp', '')) | ||
| most_recent = messages[-1] | ||
|
|
||
| # Stop thinking when AI responds | ||
| if most_recent.get('role') == 'chariot': | ||
| break | ||
|
|
||
| time.sleep(1) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid indefinite wait when polling for AI response
While loop can hang if no AI message arrives. Add a max wait and user feedback.
Apply this diff:
- # Poll until AI responds (most recent message is from AI)
- while True:
+ # Poll until AI responds (most recent message is from AI) or timeout
+ start_time = time.time()
+ max_wait_seconds = 180
+ while True:
if self.conversation_id:
messages, _ = self.sdk.search.by_key_prefix(f"#message#{self.conversation_id}", pages=1)
if messages:
# Sort by timestamp to get most recent
messages = sorted(messages, key=lambda x: x.get('timestamp', ''))
most_recent = messages[-1]
# Stop thinking when AI responds
if most_recent.get('role') == 'chariot':
break
time.sleep(1)
+ if time.time() - start_time > max_wait_seconds:
+ self.console.print("\n[dim]Timed out waiting for AI response. Showing recent messages...[/dim]")
+ break📝 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.
| # Poll until AI responds (most recent message is from AI) | |
| while True: | |
| if self.conversation_id: | |
| messages, _ = self.sdk.search.by_key_prefix(f"#message#{self.conversation_id}", pages=1) | |
| if messages: | |
| # Sort by timestamp to get most recent | |
| messages = sorted(messages, key=lambda x: x.get('timestamp', '')) | |
| most_recent = messages[-1] | |
| # Stop thinking when AI responds | |
| if most_recent.get('role') == 'chariot': | |
| break | |
| time.sleep(1) | |
| # Poll until AI responds (most recent message is from AI) or timeout | |
| start_time = time.time() | |
| max_wait_seconds = 180 | |
| while True: | |
| if self.conversation_id: | |
| messages, _ = self.sdk.search.by_key_prefix(f"#message#{self.conversation_id}", pages=1) | |
| if messages: | |
| # Sort by timestamp to get most recent | |
| messages = sorted(messages, key=lambda x: x.get('timestamp', '')) | |
| most_recent = messages[-1] | |
| # Stop thinking when AI responds | |
| if most_recent.get('role') == 'chariot': | |
| break | |
| time.sleep(1) | |
| if time.time() - start_time > max_wait_seconds: | |
| self.console.print("\n[dim]Timed out waiting for AI response. Showing recent messages...[/dim]") | |
| break |
- Remove CONVERSATION_TEST.md (testing documentation) - Remove docs/webpage-linking.md (feature documentation) - Remove test_tool_execution.py (test script) - Keep only essential conversation UI implementation
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review Analysis
|
- Add USER_FLAG to SDK globals following GLOBAL_FLAG pattern - Add user parameter to by_term and by_key_prefix methods - Update conversation and message queries in CLI to use user=True - Enable user-scoped queries for conversations and messages in TUI - Follow existing global parameter implementation pattern
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 472-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
praetorian_cli/ui/conversation/menu.py (8)
227-231: Remove unused variable in status context manager.Avoid assigning to an unused name to satisfy linters.
Apply this diff:
- ) as status: + ):
69-72: Use Console.clear() instead of os.system for clearing the screen.Safer and avoids shell invocation (S605).
Apply this diff:
-import os @@ def clear_screen(self) -> None: """Clear the screen""" - os.system('clear' if os.name == 'posix' else 'cls') + self.console.clear()Also applies to: 3-3
1-1: Remove shebang from a non-executable module.Shebang triggers EXE001; this is an importable module, not a script.
Apply this diff:
-#!/usr/bin/env python3
75-75: Drop unnecessary f-string prefixes.These strings have no placeholders; remove the f for F541 compliance.
Apply this diff:
- self.console.print(f"\n[bold blue]Chariot AI Assistant[/bold blue]") + self.console.print("\n[bold blue]Chariot AI Assistant[/bold blue]") @@ - self.console.print(f"\n[bold blue]Chariot AI Assistant[/bold blue]") + self.console.print("\n[bold blue]Chariot AI Assistant[/bold blue]") @@ - self.console.print(f"[dim]🔧 Executing tool...[/dim]") + self.console.print("[dim]🔧 Executing tool...[/dim]") - self.console.print(f"[dim]✅ Tool execution completed[/dim]") + self.console.print("[dim]✅ Tool execution completed[/dim]")Also applies to: 105-105, 278-280
26-27: Harden user lookup; avoid crashing if identity resolution fails.get_current_user can raise; fall back to placeholders.
Apply this diff:
- self.user_email, self.username = self.sdk.get_current_user() + try: + self.user_email, self.username = self.sdk.get_current_user() + except Exception: + self.user_email, self.username = ("unknown@local", "unknown")
264-283: Pagination-aware message tracking to avoid missed messages.Using last_message_count with pages=1 can drop messages if server returns a paged window. Prefer timestamp-based filtering or paging via offset.
Option: track last_seen_ts and filter messages by timestamp; request additional pages until no newer items. Want a patch for this?
319-326: Ensure response is string; handle dict/list tool outputs gracefully.Avoid type errors if content is non-string.
Apply this diff:
def display_ai_response(self, response: str) -> None: """Display AI response with proper formatting""" self.console.print() + # Normalize response to string + if not isinstance(response, str): + try: + response = json.dumps(response, indent=2) + except Exception: + response = str(response)
98-99: Avoid bare excepts; log or narrow exceptions.Catching Exception silences real errors and hampers debugging.
- Narrow to specific exceptions where known (e.g., requests.RequestException).
- Otherwise, log at debug level or print dimmed errors.
Want me to sweep and propose concrete exception types per block?Also applies to: 174-175, 185-186, 284-284, 446-446, 454-454
praetorian_cli/sdk/entities/search.py (3)
20-34: Propagate and document new user parameter in by_key_prefix.Signature change is good; docstring should include user flag.
Apply this diff to extend the docstring:
""" - Search for entities by key prefix. <mcp>If the response is too large, make your query more specific.<mcp> + Search for entities by key prefix. <mcp>If the response is too large, make your query more specific.<mcp> @@ - :return: A tuple containing (list of matching entities, next page offset) + :param user: When True, restrict results to the current user scope + :type user: bool + :return: A tuple containing (list of matching entities, next page offset)
121-156: Document user flag in by_term docstring.Behavior change should be reflected in params.
Apply this diff:
- :param global_: Use the global data set + :param global_: Use the global data set :type global_: bool + :param user: Restrict results to the current user scope + :type user: bool
35-52: Optional: expose user scoping consistently across helpers.Consider adding user=False to by_source/by_status/by_name/by_dns for API parity.
If you want, I can provide a small patch to thread user through these helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
praetorian_cli/sdk/entities/search.py(5 hunks)praetorian_cli/sdk/model/globals.py(1 hunks)praetorian_cli/ui/conversation/menu.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
praetorian_cli/sdk/entities/search.py (2)
praetorian_cli/sdk/model/globals.py (1)
Kind(101-112)praetorian_cli/sdk/model/query.py (1)
params(147-148)
praetorian_cli/ui/conversation/menu.py (3)
praetorian_cli/sdk/chariot.py (4)
Chariot(31-282)get_current_user(258-282)url(238-239)_make_request(67-79)praetorian_cli/sdk/keychain.py (1)
username(116-118)praetorian_cli/sdk/entities/search.py (1)
by_key_prefix(20-33)
🪛 Ruff (0.13.1)
praetorian_cli/ui/conversation/menu.py
1-1: Shebang is present but file is not executable
(EXE001)
71-71: Starting a process with a shell, possible injection detected
(S605)
75-75: f-string without any placeholders
Remove extraneous f prefix
(F541)
98-99: try-except-pass detected, consider logging the exception
(S110)
98-98: Do not catch blind exception: Exception
(BLE001)
105-105: f-string without any placeholders
Remove extraneous f prefix
(F541)
164-164: Do not catch blind exception: Exception
(BLE001)
174-175: try-except-pass detected, consider logging the exception
(S110)
174-174: Do not catch blind exception: Exception
(BLE001)
185-186: try-except-pass detected, consider logging the exception
(S110)
185-185: Do not catch blind exception: Exception
(BLE001)
231-231: Local variable status is assigned to but never used
Remove assignment to unused variable status
(F841)
255-255: Do not catch blind exception: Exception
(BLE001)
278-278: f-string without any placeholders
Remove extraneous f prefix
(F541)
280-280: f-string without any placeholders
Remove extraneous f prefix
(F541)
284-284: Do not catch blind exception: Exception
(BLE001)
446-446: Do not catch blind exception: Exception
(BLE001)
453-453: Consider moving this statement to an else block
(TRY300)
454-454: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
praetorian_cli/sdk/model/globals.py (1)
117-118: LGTM: USER_FLAG added.Consistent with existing flags; enables user-scoped searches.
praetorian_cli/sdk/entities/search.py (2)
3-3: LGTM: import USER_FLAG.Import set matches use below.
154-156: LGTM: USER_FLAG application.Correctly applied when user=True.
praetorian_cli/ui/conversation/menu.py (2)
298-298: Fix API endpoint mismatch: use /conversations, not /planner.Tests and other modules target /conversations; posting to /planner will break the flow.
Apply this diff:
- url = self.sdk.url("/planner") + url = self.sdk.url("/conversations")Run to check endpoints used across the repo:
#!/bin/bash rg -nP -C2 --type=py '\.url\("/planner"\)|\.url\("/conversations"\)'
238-252: Avoid indefinite wait when polling; add timeout and feedback.The loop can hang if no AI message arrives.
Apply this diff:
- # Poll until AI responds (most recent message is from AI) - while True: + # Poll until AI responds (most recent message is from AI) or timeout + start_time = time.time() + max_wait_seconds = 180 + while True: if self.conversation_id: messages, _ = self.sdk.search.by_key_prefix(f"#message#{self.conversation_id}", pages=1, user=True) if messages: # Sort by timestamp to get most recent messages = sorted(messages, key=lambda x: x.get('timestamp', '')) most_recent = messages[-1] # Stop thinking when AI responds if most_recent.get('role') == 'chariot': break time.sleep(1) + if time.time() - start_time > max_wait_seconds: + self.console.print("\n[dim]Timed out waiting for AI response. Showing recent messages...[/dim]") + break
- Add background polling thread that runs continuously - Poll every 3 seconds for new messages even when not actively waiting - Display job completion summaries immediately when they arrive - Handle async scan results with special formatting and job update notifications - Enable real-time job completion feedback without user interaction - Clean thread shutdown on exit
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 472-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
- Fix nested try/except structure that caused syntax error - Properly structure exception handling for main conversation loop - Ensure background polling cleanup happens on exit
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 472-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
praetorian_cli/ui/conversation/menu.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
praetorian_cli/ui/conversation/menu.py (4)
praetorian_cli/sdk/chariot.py (4)
Chariot(31-282)get_current_user(258-282)url(238-239)_make_request(67-79)praetorian_cli/sdk/keychain.py (1)
username(116-118)praetorian_cli/sdk/entities/search.py (1)
by_key_prefix(20-33)praetorian_cli/handlers/agent.py (1)
conversation(75-100)
🪛 Ruff (0.13.1)
praetorian_cli/ui/conversation/menu.py
1-1: Shebang is present but file is not executable
(EXE001)
50-50: Expected except or finally after try block
(invalid-syntax)
50-50: Invalid annotated assignment target
(invalid-syntax)
50-51: Expected an expression
(invalid-syntax)
51-51: Unexpected indentation
(invalid-syntax)
54-54: Expected a statement
(invalid-syntax)
54-54: Invalid annotated assignment target
(invalid-syntax)
54-55: Expected an expression
(invalid-syntax)
55-55: Unexpected indentation
(invalid-syntax)
57-57: Expected except or finally after try block
(invalid-syntax)
57-57: Invalid annotated assignment target
(invalid-syntax)
57-58: Expected an expression
(invalid-syntax)
58-58: Unexpected indentation
(invalid-syntax)
60-60: Expected a statement
(invalid-syntax)
60-60: Invalid annotated assignment target
(invalid-syntax)
60-61: Expected an expression
(invalid-syntax)
61-61: Unexpected indentation
(invalid-syntax)
63-63: Expected a statement
(invalid-syntax)
63-63: Invalid annotated assignment target
(invalid-syntax)
63-64: Expected an expression
(invalid-syntax)
64-64: Unexpected indentation
(invalid-syntax)
67-67: Expected a statement
(invalid-syntax)
70-70: Expected a statement
(invalid-syntax)
70-71: Expected an expression
(invalid-syntax)
71-71: Unexpected indentation
(invalid-syntax)
73-73: Expected a statement
(invalid-syntax)
73-73: Expected a statement
(invalid-syntax)
73-74: Expected a statement
(invalid-syntax)
74-74: Unexpected indentation
(invalid-syntax)
76-76: Expected a statement
(invalid-syntax)
534-534: Expected a statement
(invalid-syntax)
⏰ 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). (1)
- GitHub Check: claude-code-action / claude-code-action
🔇 Additional comments (2)
praetorian_cli/ui/conversation/menu.py (2)
252-275: Add a polling timeout.This loop still blocks forever when the service never sends a
chariotmessage (network hiccup, tool stall, etc.), leaving the CLI hung. Please cap the wait and surface a timeout message.- while True: + start_time = time.time() + max_wait_seconds = 180 + while True: if self.conversation_id: messages, _ = self.sdk.search.by_key_prefix(f"#message#{self.conversation_id}", pages=1, user=True) if messages: messages = sorted(messages, key=lambda x: x.get('timestamp', '')) most_recent = messages[-1] if most_recent.get('role') == 'chariot': break time.sleep(1) + if time.time() - start_time > max_wait_seconds: + self.console.print("\n[dim]Timed out waiting for AI response. Showing recent messages...[/dim]") + break
363-378: Call the/conversationsendpoint.
call_conversation_apistill posts to/planner, but the conversation tooling (e.g.,tests/test_tool_execution.py) targets/conversations. Leaving this mismatch means the CLI never reaches the conversation API.- url = self.sdk.url("/planner") + url = self.sdk.url("/conversations")
- Replace jobs.list(prefix_filter) with search.by_term for conversation job queries - Use conversation:<uuid> search term with user=True parameter - Fix job listing for proper conversation-scoped job filtering - Enable correct job status display in conversation UI
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 472-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
Job Display Improvements: - Separate queued (JQ) from running (JR) jobs in status counts - Extract readable target names from asset keys (#asset#domain#hostname) - Map job status codes to human text (JQ->Queued, JR->Running, etc.) - Use proper blue color for queued jobs vs yellow for running Conversation Resume: - Load and display previous messages when resuming conversation - Show complete conversation history with proper role formatting - Include tool execution history and job completion summaries - Set proper message count for background polling continuation Enhanced UX with accurate job status reporting and full conversation context.
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 607-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
- Add debug output to understand actual job target structure - Try multiple extraction approaches for target display - Debug both target field and job key field contents - Enable proper target name extraction once structure is understood
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 607-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
- Extract target from job key format: #job#target#capability - Parse nested asset keys within job target for readable display - Use DNS field as fallback for target display - Handle both direct targets and nested asset key targets - Should now show proper hostnames instead of 'unknown'
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 607-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
- Remove Messages column from conversation selection table - Remove get_conversation_message_count method to eliminate API calls - Speed up conversation resume menu loading - Simplify conversation table to ID, Topic, and Created columns only
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 598-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
…an queries - Consolidate to single background polling thread for all message loading - Use efficient key:> search queries to load only new messages since last poll - Replace message count tracking with message key tracking for precision - Apply same efficient logic for both conversation history and real-time updates - Improve performance by avoiding full conversation scans on each poll - Enable true real-time updates with minimal API overhead
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 598-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
praetorian_cli/ui/conversation/menu.py (11)
337-337: Add HTTP timeout to avoid indefinite network hangsRequests without timeout can block the UI.
- response = self.sdk._make_request("POST", url, json=payload) + response = self.sdk._make_request("POST", url, json=payload, timeout=30)
76-79: Use Rich to clear the screen (avoid spawning a shell)Avoid os.system; use Console.clear() which is cross‑platform and safer.
- def clear_screen(self) -> None: - """Clear the screen""" - os.system('clear' if os.name == 'posix' else 'cls') + def clear_screen(self) -> None: + """Clear the screen""" + self.console.clear()
82-82: Remove unnecessary f-stringsThese literals have no placeholders; drop the f prefix.
- self.console.print(f"\n[bold blue]Chariot AI Assistant[/bold blue]") + self.console.print("\n[bold blue]Chariot AI Assistant[/bold blue]")- self.console.print(f"\n[bold blue]Chariot AI Assistant[/bold blue]") + self.console.print("\n[bold blue]Chariot AI Assistant[/bold blue]")- self.console.print(f"\n[bold green]🎯 Job Update:[/bold green]") + self.console.print("\n[bold green]🎯 Job Update:[/bold green]")- self.console.print(f"[dim]🔧 Executing tool...[/dim]") + self.console.print("[dim]🔧 Executing tool...[/dim]")- self.console.print(f"[dim]✅ Tool execution completed[/dim]") + self.console.print("[dim]✅ Tool execution completed[/dim]")- self.console.print(f"[dim]🎯 Processing job completion...[/dim]") + self.console.print("[dim]🎯 Processing job completion...[/dim]")- self.console.print(f"[bold green]AI:[/bold green]") + self.console.print("[bold green]AI:[/bold green]")Also applies to: 115-115, 302-302, 307-307, 309-309, 311-311, 557-557
242-243: Limit polling queries to a single pageDefault pages=100000 is heavy in tight loops. Use pages=1.
- messages, _ = self.sdk.search.by_term(f"key:>{search_key}", user=True) + messages, _ = self.sdk.search.by_term(f"key:>{search_key}", pages=1, user=True)- messages, _ = self.sdk.search.by_term(f"key:>{search_key}", user=True) + messages, _ = self.sdk.search.by_term(f"key:>{search_key}", pages=1, user=True)Also applies to: 289-290
223-231: Echo the user’s message for better UXShow “You: …” immediately so users see their input reflected.
def send_message_with_polling(self, message: str) -> None: """Send message and poll for AI response""" try: + self.console.print(f"[bold blue]You:[/bold blue] {message}") with self.console.status( "[dim]Thinking...[/dim]", spinner="dots", spinner_style="blue" ) as status: response = self.call_conversation_api(message)
108-109: Don’t swallow exceptions silentlyAt least log a dim message for visibility.
- except Exception: - pass + except Exception as e: + self.console.print(f"[dim]Failed to load job summary: {e}[/dim]")
183-185: Avoid bare except/pass in get_recent_conversationsReturn [] is fine, but emit a dim log so issues are diagnosable.
- except Exception: - pass + except Exception as e: + self.console.print(f"[dim]Failed to fetch recent conversations: {e}[/dim]") return []
317-318: Avoid bare except in background pollerLog and continue; otherwise errors are invisible.
- except Exception: - pass + except Exception as e: + self.console.print(f"[dim]Background poll error: {e}[/dim]")
507-509: Scope job search to the user (if required by backend)Most searches here use user=True; consider consistency or confirm necessity for jobs.
- jobs, _ = self.sdk.search.by_term(f"conversation:{self.conversation_id}") + jobs, _ = self.sdk.search.by_term(f"conversation:{self.conversation_id}", pages=1, user=True)
29-31: Use threading.Event for stop signal (cleaner cross-thread signaling)Boolean flags work but Events are the idiomatic, thread-safe choice.
def __init__(self, sdk: Chariot): self.sdk: Chariot = sdk self.console = Console() self.conversation_id: Optional[str] = None self.messages: List[Dict] = [] self.user_email, self.username = self.sdk.get_current_user() self.last_message_key = "" self.polling_thread = None - self.stop_polling = False + self._stop_event = threading.Event()def start_background_polling(self): """Start continuous background polling for new messages""" if self.polling_thread and self.polling_thread.is_alive(): - self.stop_polling = True + self._stop_event.set() self.polling_thread.join() - self.stop_polling = False + self._stop_event.clear() self.polling_thread = threading.Thread(target=self._background_poll, daemon=True) self.polling_thread.start()def stop_background_polling(self): """Stop background polling""" - self.stop_polling = True + self._stop_event.set() if self.polling_thread: self.polling_thread.join()- while not self.stop_polling: + while not self._stop_event.is_set():Also applies to: 266-281, 282-286, 278-281, 316-316
1-1: Remove shebang (module, not script) or make file executableThis is a library module; drop the shebang to satisfy linters.
-#!/usr/bin/env python3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
praetorian_cli/ui/conversation/menu.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
praetorian_cli/ui/conversation/menu.py (3)
praetorian_cli/sdk/chariot.py (3)
get_current_user(258-282)url(238-239)_make_request(67-79)praetorian_cli/sdk/keychain.py (1)
username(116-118)praetorian_cli/sdk/entities/search.py (2)
by_key_prefix(20-33)by_term(121-165)
🪛 Ruff (0.13.1)
praetorian_cli/ui/conversation/menu.py
1-1: Shebang is present but file is not executable
(EXE001)
78-78: Starting a process with a shell, possible injection detected
(S605)
82-82: f-string without any placeholders
Remove extraneous f prefix
(F541)
108-109: try-except-pass detected, consider logging the exception
(S110)
108-108: Do not catch blind exception: Exception
(BLE001)
115-115: f-string without any placeholders
Remove extraneous f prefix
(F541)
173-173: Do not catch blind exception: Exception
(BLE001)
183-184: try-except-pass detected, consider logging the exception
(S110)
183-183: Do not catch blind exception: Exception
(BLE001)
262-262: Do not catch blind exception: Exception
(BLE001)
302-302: f-string without any placeholders
Remove extraneous f prefix
(F541)
307-307: f-string without any placeholders
Remove extraneous f prefix
(F541)
309-309: f-string without any placeholders
Remove extraneous f prefix
(F541)
311-311: f-string without any placeholders
Remove extraneous f prefix
(F541)
317-318: try-except-pass detected, consider logging the exception
(S110)
317-317: Do not catch blind exception: Exception
(BLE001)
501-501: Do not catch blind exception: Exception
(BLE001)
508-508: Consider moving this statement to an else block
(TRY300)
509-509: Do not catch blind exception: Exception
(BLE001)
557-557: f-string without any placeholders
Remove extraneous f prefix
(F541)
572-572: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
praetorian_cli/ui/conversation/menu.py (2)
329-345: Align endpoint with conversation API used elsewhereCode posts to “/planner”; tests and other code paths use “/conversations”. Align to avoid breakage.
- url = self.sdk.url("/planner") + url = self.sdk.url("/conversations")Please confirm the expected payload/response shape and update tests accordingly if needed.
237-261: Prevent indefinite wait when polling for AI responseAdd a timeout to avoid hanging if no AI message arrives.
- # Wait for AI to respond (background polling handles display) - while True: + # Wait for AI to respond (background polling handles display) with timeout + start_time = time.time() + max_wait_seconds = 180 + while True: if self.conversation_id: # Use efficient greater-than search search_key = f"#message#{self.conversation_id}#{self.last_message_key}" if self.last_message_key else f"#message#{self.conversation_id}#" - messages, _ = self.sdk.search.by_term(f"key:>{search_key}", user=True) + messages, _ = self.sdk.search.by_term(f"key:>{search_key}", pages=1, user=True) if messages: messages = sorted(messages, key=lambda x: x.get('key', '')) most_recent = messages[-1] # Update spinner for tool execution for msg in messages: role = msg.get('role') if role == 'tool call': status.update("[dim]🔧 Executing tool...[/dim]") elif role == 'tool response': status.update("[dim]✅ Tool completed, thinking...[/dim]") # Stop when AI responds if most_recent.get('role') == 'chariot': break time.sleep(1) + if time.time() - start_time > max_wait_seconds: + self.console.print("\n[dim]Timed out waiting for AI response. Showing recent messages...[/dim]") + break
- Default to query mode for safe data discovery - Add mode display in header with color coding (blue=query, green=agent) - Add mode switching commands: 'query' and 'agent' - Include mode parameter in API requests - Update help documentation with mode descriptions - Enable real-time mode switching within conversations
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 598-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 622-line interactive conversation CLI interface ( Recommendations• Add comprehensive unit tests for |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
praetorian_cli/ui/conversation/textual_chat.py (9)
1-1: Remove non-executable shebang or make file executable.Ruff EXE001: either drop the shebang (preferred for library modules) or mark the file as executable in git.
Apply:
-#!/usr/bin/env python3
3-8: Drop unused imports; add logging.json and time are unused. Add logging for error reporting (used below).
Apply:
import asyncio -import json -import time from datetime import datetime from typing import Optional, List, Dict, Any +import loggingAnd add a module logger near other top-level imports:
from textual import on from praetorian_cli.sdk.chariot import Chariot +logger = logging.getLogger(__name__)
258-266: Don’t swallow exceptions; log them.Blind except+pass hides failures and complicates diagnosis.
Apply:
async def background_poll(self) -> None: """Background polling for job completion events""" while True: try: if self.conversation_id: await self.check_for_new_messages() await asyncio.sleep(3) # Poll every 3 seconds - except Exception: - pass + except Exception: + logger.exception("Background poll failed")
329-335: Render system messages as Markdown (headings, lists, code blocks).Static won’t format Markdown. Use Markdown widget for help/tables to render properly.
Apply:
def add_system_message(self, content: str) -> None: """Add system message to chat log""" chat_log = self.query_one("#chat-log") - message_widget = Static(content, classes="system-message") + message_widget = Markdown(content, classes="system-message") chat_log.mount(message_widget) chat_log.scroll_end()
151-161: Cancel background tasks on exit to prevent leaks.Ensure background_poll and ad-hoc tasks are cancelled when the app closes.
Apply:
def on_mount(self) -> None: """Called when app starts""" # Start background polling for job completion events self.polling_task = asyncio.create_task(self.background_poll()) @@ self.add_system_message("Welcome to Chariot AI Assistant! Type 'help' for commands or ask about your security data.") + + async def on_unmount(self) -> None: + """Cleanup background tasks on shutdown""" + if self.polling_task: + self.polling_task.cancel() + with contextlib.suppress(Exception): + await self.polling_task + for task in list(self._background_tasks): + task.cancel() + self._background_tasks.clear()And add the missing import at the top:
+import contextlib
416-476: Resume flow: avoid blocking and limit pages; log parse errors.
- Wrap search in to_thread.
- Limit pages to 1 (or a small number).
- Replace bare except with Exception to satisfy linters and log it.
Apply:
- conversations, _ = self.sdk.search.by_key_prefix("#conversation#", user=True) + conversations, _ = await asyncio.to_thread( + self.sdk.search.by_key_prefix, + "#conversation#", + None, + 1, # fetch a page of recent conversations + True # user=True + ) @@ - except: - created = created[:16] + except Exception: + created = created[:16] @@ - except Exception as e: - self.add_system_message(f"Error loading conversations: {e}") + except Exception as e: + logger.exception("Error loading conversations") + self.add_system_message(f"Error loading conversations: {e}") await self.start_new_conversation()
508-543: History load: avoid blocking call and large pulls; log errors.Use to_thread; optionally page results if conversations can be large.
Apply:
- messages, _ = self.sdk.search.by_key_prefix(f"#message#{self.conversation_id}#", user=True) + messages, _ = await asyncio.to_thread( + self.sdk.search.by_key_prefix, + f"#message#{self.conversation_id}#", + None, + 5, # fetch a few pages for history; adjust as needed + True # user=True + ) @@ - except Exception as e: - self.add_system_message(f"Error loading conversation history: {e}") + except Exception as e: + logger.exception("Error loading conversation history") + self.add_system_message(f"Error loading conversation history: {e}")
549-617: Job status: avoid blocking; limit pages; add user filter if appropriate.Wrap in to_thread and reduce pages to avoid large scans every time. Consider user=True if jobs are user-scoped in your API.
Apply:
- jobs, _ = self.sdk.search.by_term(f"conversation:{self.conversation_id}") + jobs, _ = await asyncio.to_thread( + self.sdk.search.by_term, + f"conversation:{self.conversation_id}", + None, # kind + None, # offset + 1 # pages + ) @@ - except Exception as e: - self.add_system_message(f"Failed to get job status: {e}") + except Exception as e: + logger.exception("Failed to get job status") + self.add_system_message(f"Failed to get job status: {e}")Please confirm whether search.by_term supports user=True for jobs and whether results need descending ordering for “latest-first.” Adjust parameters accordingly.
19-38: Consolidate rendering via ChatMessage or remove it.ChatMessage isn’t used; either wire it up for consistency or remove the class to reduce dead code.
Also applies to: 307-321
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
praetorian_cli/handlers/agent.py(1 hunks)praetorian_cli/ui/conversation/__init__.py(1 hunks)praetorian_cli/ui/conversation/textual_chat.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- praetorian_cli/ui/conversation/init.py
🧰 Additional context used
🧬 Code graph analysis (2)
praetorian_cli/handlers/agent.py (2)
praetorian_cli/handlers/cli_decorators.py (1)
cli_handler(48-52)praetorian_cli/ui/conversation/textual_chat.py (1)
run_textual_conversation(619-622)
praetorian_cli/ui/conversation/textual_chat.py (4)
praetorian_cli/handlers/chariot.py (1)
chariot(6-17)praetorian_cli/sdk/chariot.py (4)
Chariot(31-282)get_current_user(258-282)url(238-239)_make_request(67-79)praetorian_cli/sdk/keychain.py (1)
username(116-118)praetorian_cli/sdk/entities/search.py (2)
by_key_prefix(20-33)by_term(121-165)
🪛 Ruff (0.13.1)
praetorian_cli/ui/conversation/textual_chat.py
1-1: Shebang is present but file is not executable
(EXE001)
231-231: Do not catch blind exception: Exception
(BLE001)
236-236: Store a reference to the return value of asyncio.create_task
(RUF006)
238-238: Do not catch blind exception: Exception
(BLE001)
265-266: try-except-pass detected, consider logging the exception
(S110)
265-265: Do not catch blind exception: Exception
(BLE001)
304-305: try-except-pass detected, consider logging the exception
(S110)
304-304: Do not catch blind exception: Exception
(BLE001)
304-304: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
456-456: Do not use bare except
(E722)
473-473: Do not catch blind exception: Exception
(BLE001)
541-541: Do not catch blind exception: Exception
(BLE001)
615-615: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (1)
- GitHub Check: claude-code-action / claude-code-action
🔇 Additional comments (1)
praetorian_cli/handlers/agent.py (1)
73-101: LGTM: clean CLI entrypoint for the conversation UI.Command wiring and import-on-use look good. No blocking issues spotted.
Confirm that praetorian_cli/ui/conversation/init.py exports run_textual_conversation; otherwise the import will fail at runtime.
| def __init__(self, sdk: Chariot): | ||
| super().__init__() | ||
| self.sdk = sdk | ||
| self.user_email, self.username = self.sdk.get_current_user() | ||
| self.polling_task: Optional[asyncio.Task] = None | ||
| self._selecting_conversation = False | ||
| self._available_conversations = [] | ||
|
|
There was a problem hiding this comment.
Harden user lookup; avoid crashing UI if identity lookup fails.
Wrap get_current_user to fall back gracefully.
Apply:
def __init__(self, sdk: Chariot):
super().__init__()
self.sdk = sdk
- self.user_email, self.username = self.sdk.get_current_user()
+ try:
+ self.user_email, self.username = self.sdk.get_current_user()
+ except Exception as e:
+ logger.exception("Failed to load current user")
+ self.user_email, self.username = ("unknown@unknown", "unknown")
self.polling_task: Optional[asyncio.Task] = None
self._selecting_conversation = False
self._available_conversations = []
+ self._background_tasks: set[asyncio.Task] = set()📝 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.
| def __init__(self, sdk: Chariot): | |
| super().__init__() | |
| self.sdk = sdk | |
| self.user_email, self.username = self.sdk.get_current_user() | |
| self.polling_task: Optional[asyncio.Task] = None | |
| self._selecting_conversation = False | |
| self._available_conversations = [] | |
| def __init__(self, sdk: Chariot): | |
| super().__init__() | |
| self.sdk = sdk | |
| try: | |
| self.user_email, self.username = self.sdk.get_current_user() | |
| except Exception as e: | |
| logger.exception("Failed to load current user") | |
| self.user_email, self.username = ("unknown@unknown", "unknown") | |
| self.polling_task: Optional[asyncio.Task] = None | |
| self._selecting_conversation = False | |
| self._available_conversations = [] | |
| self._background_tasks: set[asyncio.Task] = set() |
🤖 Prompt for AI Agents
In praetorian_cli/ui/conversation/textual_chat.py around lines 126 to 133, the
call to self.sdk.get_current_user() can raise or return unexpected results and
will crash the UI; wrap that call in a try/except (or handle None/tuple shapes)
and fall back to safe defaults (e.g., user_email = None and username = "Unknown"
or similar) when lookup fails; assign those defaults to self.user_email and
self.username, log or warn the failure via the module logger (don’t re-raise),
and ensure the attributes remain defined so downstream code doesn’t crash.
| # Create async task for API call to avoid blocking UI | ||
| async def send_api_request(): | ||
| try: | ||
| # Call API in background | ||
| response = self.call_conversation_api(message) | ||
|
|
||
| if response.get('error'): | ||
| self.add_system_message(f"Error: {response.get('error')}") | ||
| self.update_status("Error - Ready for next message") | ||
| return | ||
|
|
||
| # Update status and wait for AI response | ||
| self.update_status("Waiting for AI response...") | ||
|
|
||
| # Poll for AI response | ||
| await self.wait_for_ai_response() | ||
|
|
||
| except Exception as e: | ||
| self.add_system_message(f"Failed to send message: {e}") | ||
| self.update_status("Error - Ready for next message") | ||
|
|
||
| # Start the API request as a background task | ||
| asyncio.create_task(send_api_request()) |
There was a problem hiding this comment.
Blocking HTTP on the event loop freezes the UI; make API calls non-blocking.
call_conversation_api uses requests synchronously and is awaited indirectly from async code. Offload to a thread and await it. Also keep a reference to the created task (RUF006).
Apply:
- async def send_api_request():
+ async def send_api_request():
try:
# Call API in background
- response = self.call_conversation_api(message)
+ response = await self.call_conversation_api(message)
if response.get('error'):
self.add_system_message(f"Error: {response.get('error')}")
self.update_status("Error - Ready for next message")
return
@@
- # Start the API request as a background task
- asyncio.create_task(send_api_request())
+ # Start the API request as a background task and keep a ref
+ task = asyncio.create_task(send_api_request())
+ self._background_tasks.add(task)
+ task.add_done_callback(self._background_tasks.discard)- def call_conversation_api(self, message: str) -> Dict:
+ async def call_conversation_api(self, message: str) -> Dict:
"""Call the Chariot conversation API"""
url = self.sdk.url("/planner")
payload = {"message": message, "mode": self.mode}
if self.conversation_id:
payload["conversationId"] = self.conversation_id
- response = self.sdk._make_request("POST", url, json=payload)
+ response = await asyncio.to_thread(self.sdk._make_request, "POST", url, json=payload)
if response.status_code == 200:
result = response.json()
if not self.conversation_id and 'conversation' in result:
self.conversation_id = result['conversation'].get('uuid')
self.update_status("Ready")
return {'success': True}
else:
return {
'success': False,
'error': f"API error: {response.status_code} - {response.text}"
}Also applies to: 342-365
🧰 Tools
🪛 Ruff (0.13.1)
231-231: Do not catch blind exception: Exception
(BLE001)
236-236: Store a reference to the return value of asyncio.create_task
(RUF006)
🤖 Prompt for AI Agents
In praetorian_cli/ui/conversation/textual_chat.py around lines 214-236 (and
similarly 342-365), call_conversation_api is synchronous and being invoked
inside async code which blocks the event loop; change the async helper to
offload the blocking HTTP call to a thread (use asyncio.to_thread or
loop.run_in_executor) and await that result, handle exceptions from the awaited
thread call as currently done, and keep a reference to the created background
task on the instance (e.g., self._api_task = asyncio.create_task(...)) so it can
be inspected/cancelled later and avoid orphaned tasks (RUF006).
| async def check_for_new_messages(self) -> None: | ||
| """Check for new messages and display them""" | ||
| if not self.conversation_id: | ||
| return | ||
|
|
||
| try: | ||
| # Load all messages for this conversation | ||
| all_messages, _ = self.sdk.search.by_key_prefix(f"#message#{self.conversation_id}#", user=True) | ||
|
|
||
| # Filter to only new messages | ||
| if self.last_message_key: | ||
| messages = [msg for msg in all_messages if msg.get('key', '') > self.last_message_key] | ||
| else: | ||
| messages = all_messages | ||
|
|
||
| if messages: | ||
| messages = sorted(messages, key=lambda x: x.get('key', '')) | ||
|
|
||
| for msg in messages: | ||
| role = msg.get('role') | ||
| content = msg.get('content', '') | ||
|
|
||
| if role == 'chariot': | ||
| self.add_ai_message(content) | ||
| elif role == 'tool call': | ||
| self.add_tool_message("🔧 Executing tool...") | ||
| self.update_status("Executing tool...") | ||
| elif role == 'tool response': | ||
| self.add_tool_message("✅ Tool execution completed") | ||
| self.update_status("Tool completed, thinking...") | ||
| elif role == 'planner-output': | ||
| self.add_system_message("🎯 Processing job completion...") | ||
|
|
||
| # Update last message key | ||
| self.last_message_key = messages[-1].get('key', '') | ||
|
|
||
| except Exception as e: | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid blocking search calls on the event loop; limit result size.
Use asyncio.to_thread for SDK calls and reduce pages to avoid pulling large histories on every poll. Also log errors instead of pass.
Apply:
async def check_for_new_messages(self) -> None:
"""Check for new messages and display them"""
if not self.conversation_id:
return
try:
# Load all messages for this conversation
- all_messages, _ = self.sdk.search.by_key_prefix(f"#message#{self.conversation_id}#", user=True)
+ all_messages, _ = await asyncio.to_thread(
+ self.sdk.search.by_key_prefix,
+ f"#message#{self.conversation_id}#",
+ None,
+ 1, # one page; adjust if needed
+ True, # user=True
+ )
@@
- except Exception as e:
- pass
+ except Exception:
+ logger.exception("Failed to check for new messages")Note: With pages=1 you may miss bursts >1 page between polls; consider descending=True and paging until last_message_key is seen. Please verify expected API ordering.
🏁 Script executed:
#!/bin/bash
rg -n "def by_key_prefix" -C3 .Length of output: 582
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' praetorian_cli/sdk/entities/search.pyLength of output: 8494
Offload SDK call, cap page size, and log errors
- Wrap the blocking
self.sdk.search.by_key_prefixin a thread:all_messages, _ = await asyncio.to_thread( self.sdk.search.by_key_prefix, f"#message#{self.conversation_id}#", None, 1, # limit to one page True, # user=True )
- Replace the silent
except Exception: passwith:except Exception: logger.exception("Failed to check for new messages")
(Optional) Use descending=True and loop pages until last_message_key to avoid missing high-volume bursts.
🧰 Tools
🪛 Ruff (0.13.1)
304-305: try-except-pass detected, consider logging the exception
(S110)
304-304: Do not catch blind exception: Exception
(BLE001)
304-304: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🤖 Prompt for AI Agents
In praetorian_cli/ui/conversation/textual_chat.py around lines 268-305, the
blocking SDK call and silent exception handling need fixing: move the
self.sdk.search.by_key_prefix call into asyncio.to_thread, passing the
conversation key, a page token of None, a page size limit (e.g., 1) and
user=True to avoid blocking the event loop and cap page size; then handle
exceptions by replacing the bare except/pass with logger.exception("Failed to
check for new messages") so errors are logged; optionally consider using
descending=True and paginating until last_message_key to avoid missing messages
during high-volume bursts.
| async def clear_chat(self) -> None: | ||
| """Clear the chat log""" | ||
| chat_log = self.query_one("#chat-log") | ||
| await chat_log.remove_children() | ||
|
|
There was a problem hiding this comment.
Fix await on non-awaitable remove_children (runtime error).
remove_children isn’t async; awaiting it will raise a TypeError.
Apply:
async def clear_chat(self) -> None:
"""Clear the chat log"""
chat_log = self.query_one("#chat-log")
- await chat_log.remove_children()
+ chat_log.remove_children()📝 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.
| async def clear_chat(self) -> None: | |
| """Clear the chat log""" | |
| chat_log = self.query_one("#chat-log") | |
| await chat_log.remove_children() | |
| async def clear_chat(self) -> None: | |
| """Clear the chat log""" | |
| chat_log = self.query_one("#chat-log") | |
| chat_log.remove_children() |
🤖 Prompt for AI Agents
In praetorian_cli/ui/conversation/textual_chat.py around lines 544 to 548, the
clear_chat method awaits remove_children even though remove_children is not
async; this causes a TypeError at runtime. Fix by removing the await and calling
chat_log.remove_children() directly (keeping the method async if other awaits
remain), or if synchronous behavior is undesired, replace with the correct async
API that returns an awaitable; ensure no await is used on a non-awaitable.
|
Claude finished @noah-tutt-praetorian's task —— View job PR Review AnalysisTodo List:
Has Go, TypeScript, or Python Changes: Yes Findings• Major new feature added: 622-line interactive conversation CLI interface ( Recommendations• Fix blocking HTTP calls: Wrap |
1 participant
Summary (required)
Briefly describe the changes and the issue it addresses. It helps the reviewers understand the context.
JIRA (required)