infragpt agentic loop - #102
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughReplaces LangChain with direct OpenAI/Anthropic SDKs; adds a provider-agnostic LLM layer (models, base, providers, router, exceptions), a new SDK-based LLMAdapter and ModernShellAgent with streaming/tool-call flow, PTY CommandExecutor and decorator-based tool system, credential-resolution and CLI changes, history sanitization, and deletes legacy docs and LangChain modules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (main)
participant Router as LLMRouter
participant Agent as ModernShellAgent
participant Provider as LLM Provider
participant Tools as Tool Runtime
participant Shell as CommandExecutor
participant History as History DB
User->>CLI: run infragpt --model provider:model
CLI->>Router: parse_model_string & create_provider
CLI->>Agent: run_shell_agent(model, api_key)
Agent->>Provider: validate_api_key()
Agent->>User: prompt for input
User-->>Agent: "execute command ..."
Agent->>Provider: stream(messages, tools)
alt streaming content
Provider-->>Agent: StreamChunk(content)
Agent->>User: stream assistant text
else tool invocation
Provider-->>Agent: StreamChunk(tool_calls)
Agent->>Tools: execute_tool_call(name, args)
Tools->>Shell: execute_command(cmd)
Shell-->>Tools: (exit_code, output, cancelled)
Tools-->>Agent: tool result
Agent->>Provider: continue stream with tool result appended
Provider-->>Agent: StreamChunk(content / finish)
Agent->>User: final assistant output
end
Agent->>History: log_interaction(sanitized_data)
sequenceDiagram
autonumber
participant Router as LLMRouter
participant OpenAI as OpenAIProvider
participant Anthropic as AnthropicProvider
Router->>Router: parse_model_string("openai:gpt-4o")
Router->>OpenAI: instantiate(api_key, model, params)
note over OpenAI: _initialize_client using OpenAI SDK
Router-->>Caller: OpenAIProvider instance
Router->>Router: parse_model_string("anthropic:claude-3")
Router->>Anthropic: instantiate(api_key, model, params)
note over Anthropic: _initialize_client using Anthropic SDK
Router-->>Caller: AnthropicProvider instance
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a major architectural update to implement an agentic loop with direct SDK integration, removing LangChain dependencies and transitioning from simple command generation to an interactive shell agent with tool calling capabilities.
Key changes:
- Replaces LangChain with direct OpenAI and Anthropic SDKs for better control and performance
- Implements comprehensive tool system with shell command execution
- Adds interactive agent loop with streaming responses and conversation context
- Introduces unified LLM provider abstraction with proper error handling
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/src/infragpt/tools.py | New tool system implementing shell command execution with user confirmation |
| cli/src/infragpt/shell.py | Advanced shell command executor with real-time streaming and timeout handling |
| cli/src/infragpt/prompts.py | Simplified to only provide system prompt for the shell agent |
| cli/src/infragpt/main.py | Redesigned CLI entry point to use new credential system and agent architecture |
| cli/src/infragpt/llm_adapter.py | New LLM adapter replacing LangChain with direct SDK integration |
| cli/src/infragpt/llm/router.py | Provider routing system for model selection and configuration |
| cli/src/infragpt/llm/providers/openai_provider.py | OpenAI provider implementation with streaming and tool calling |
| cli/src/infragpt/llm/providers/anthropic_provider.py | Anthropic provider implementation with streaming and tool calling |
| cli/src/infragpt/llm/providers/init.py | Provider module exports |
| cli/src/infragpt/llm/prompts.py | Removed LangChain-based prompt templates |
| cli/src/infragpt/llm/models.py | New data models for unified LLM interface |
| cli/src/infragpt/llm/exceptions.py | Unified exception hierarchy for LLM providers |
| cli/src/infragpt/llm/errors.py | Removed old error definitions |
| cli/src/infragpt/llm/client.py | Removed LangChain-based client implementation |
| cli/src/infragpt/llm/base.py | Abstract base class for LLM providers |
| cli/src/infragpt/llm/auth.py | Removed LangChain-based authentication |
| cli/src/infragpt/llm/init.py | Updated exports for new architecture |
| cli/src/infragpt/llm/README.md | Removed outdated documentation |
| cli/src/infragpt/history.py | Added support for agent conversation logging |
| cli/src/infragpt/agent.py | New interactive shell agent with conversation context management |
| cli/pyproject.toml | Updated dependencies to remove LangChain and add direct SDKs |
| cli/CLAUDE.md | Removed outdated documentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| tool_calls = None | ||
| if choice.delta.tool_calls: | ||
| # Update last_call_id if we see a new ID | ||
| for tc in choice.delta.tool_calls: |
There was a problem hiding this comment.
The code updates last_call_id inside the loop but only uses the last ID encountered. If multiple tool calls are present in the same delta, earlier tool calls without IDs might be assigned the wrong ID. Consider tracking call IDs per tool call index instead.
| yield StreamChunk(tool_calls=[tool_call]) | ||
|
|
||
| except json.JSONDecodeError as e: | ||
| print(f"Warning: Failed to parse JSON for {tool_block['name']}: {json_str} - Error: {e}") |
There was a problem hiding this comment.
Using print() instead of a proper logging mechanism. This should use the console object or a logger to maintain consistent output formatting with the rest of the application.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Nitpick comments (30)
cli/pyproject.toml (1)
28-29: Add upper bounds to provider SDKs to avoid surprise breakages.Pin major versions to reduce accidental API breaks.
- "openai>=1.98.0", - "anthropic>=0.60.0", + "openai>=1.98.0,<2.0.0", + "anthropic>=0.60.0,<1.0.0",cli/src/infragpt/history.py (1)
101-120: Make agent_conversation rendering robust to non-string responses.assistant_response may be dict/list (SDK outputs). Normalize before truncation; keep safe JSON fallback.
- # Truncate long responses for display - if len(assistant_response) > 200: - response_preview = assistant_response[:200] + "..." - else: - response_preview = assistant_response + # Normalize to string and truncate long responses for display + if not isinstance(assistant_response, str): + try: + assistant_response = json.dumps(assistant_response, ensure_ascii=False) + except Exception: + assistant_response = str(assistant_response) + response_preview = (assistant_response[:200] + "...") if len(assistant_response) > 200 else assistant_responsecli/src/infragpt/llm/exceptions.py (1)
6-12: Use Optional[...] for nullable parameters and export symbols.Type hints currently use str/int with None defaults; prefer Optional. Also consider all for clarity.
+from typing import Optional @@ -class LLMError(Exception): +class LLMError(Exception): @@ - def __init__(self, message: str, provider: str = None, model: str = None): + def __init__(self, message: str, provider: Optional[str] = None, model: Optional[str] = None): self.provider = provider self.model = model super().__init__(message)class APIError(LLMError): @@ - def __init__(self, message: str, status_code: int = None, provider: str = None, model: str = None): - self.status_code = status_code - super().__init__(message, provider, model) + def __init__(self, message: str, status_code: Optional[int] = None, provider: Optional[str] = None, model: Optional[str] = None): + self.status_code = status_code + super().__init__(message, provider, model)Optionally add explicit exports at the bottom:
__all__ = [ "LLMError", "AuthenticationError", "RateLimitError", "APIError", "ToolCallError", "ValidationError", "ContextWindowError", ]cli/src/infragpt/shell.py (2)
19-25: Remove unused imports flagged by static analysis.-from typing import Optional, Dict, Any, Tuple +from typing import Optional, Dict, Tuple @@ -from rich.live import Live -from rich.text import Text
79-83: Avoid bare except; use explicit suppression.- if hasattr(os, 'setsid'): - try: - popen_args['preexec_fn'] = os.setsid - except: - pass # Skip if not supported + if hasattr(os, 'setsid'): + from contextlib import suppress + with suppress(Exception): + popen_args['preexec_fn'] = os.setsidcli/src/infragpt/llm/models.py (3)
64-66: Hide provider-native payloads from repr to reduce noise/PII risk.Avoid dumping provider internals in logs.
- _provider_native: Dict[str, Any] = field(default_factory=dict) + _provider_native: Dict[str, Any] = field(default_factory=dict, repr=False)
11-13: Consider dataclass slots for lower memory and faster attribute access.Non-breaking for typical usage; opt-in per class.
-@dataclass +@dataclass(slots=True) class Parameter: @@ -@dataclass +@dataclass(slots=True) class InputSchema: @@ -@dataclass +@dataclass(slots=True) class Tool: @@ -@dataclass +@dataclass(slots=True) class ToolCall: @@ -@dataclass +@dataclass(slots=True) class StreamChunk: @@ -@dataclass +@dataclass(slots=True) class Message:Also applies to: 20-23, 29-35, 37-43, 45-51, 53-71
14-17: Constrain Parameter.type via Literal for better static checking.Improves IDE help and validation.
-from typing import Dict, Any, Optional, List, Union +from typing import Dict, Any, Optional, List, Union, Literal @@ - type: str # "string", "integer", "number", "boolean", "array", "object" + type: Literal["string", "integer", "number", "boolean", "array", "object"]cli/src/infragpt/llm/router.py (2)
5-5: Remove unused Optional import.Cleans lint (F401).
-from typing import Dict, Any, Optional, Tuple +from typing import Dict, Any, Tuple
62-63: Use exception chaining for clarity (ruff B904).Preserves original traceback.
- except Exception as e: - raise ValidationError(f"Failed to create {provider_name} provider: {e}") + except Exception as e: + raise ValidationError(f"Failed to create {provider_name} provider: {e}") from ecli/src/infragpt/llm/base.py (1)
6-8: Remove unused Message import (ruff F401).Also add TYPE_CHECKING import for forward-ref Tool to silence F821 in some linters.
-from typing import List, Dict, Any, Iterator, Optional -from .models import StreamChunk, Message +from typing import List, Dict, Any, Iterator, Optional, TYPE_CHECKING +from .models import StreamChunk +if TYPE_CHECKING: + from .models import Toolcli/src/infragpt/tools.py (5)
6-6: Remove unused import (ruff F401).-import json
118-121: Remove f-prefix from strings without placeholders (ruff F541).- console.print(f"\n[bold cyan]Tool Call: execute_shell_command[/bold cyan]") + console.print("\n[bold cyan]Tool Call: execute_shell_command[/bold cyan]") @@ - console.print(f"\n[yellow]Execute this command? (Y/n):[/yellow] ", end="") + console.print("\n[yellow]Execute this command? (y/N):[/yellow] ", end="") @@ - console.print(f"\n[bold blue]Executing command...[/bold blue]") + console.print("\n[bold blue]Executing command...[/bold blue]")Also applies to: 125-127, 147-149
187-192: Return structured errors for unknown tools or raise.Improves DX and allows caller to branch.
- else: - return f"Unknown tool: {tool_name}" + else: + return json.dumps({"error": "unknown_tool", "tool": tool_name})
168-175: Registry is hard-coded; expose simple registration API.Preps for more tools/MCP.
I can add a register_tool(func) API and build the registry dynamically. Want a patch?
106-117: Docstring style: convert to Google style per repo guidelines.Short, Args, Returns.
I can push a docstring-only patch if desired.
cli/src/infragpt/main.py (6)
10-12: Remove unused imports (keep CI green).
CONFIG_FILEandload_configare unused.Apply:
-from infragpt.config import ( - CONFIG_FILE, load_config, init_config, console -) +from infragpt.config import ( + init_config, console +)
65-65: Remove unnecessary f-string.- raise ValidationError(f"Invalid model format. Use 'provider:model' format.") + raise ValidationError("Invalid model format. Use 'provider:model' format.")
67-71: Drop unusedmodel_name.- provider_name, model_name = LLMRouter.parse_model_string(model_string) + provider_name, _ = LLMRouter.parse_model_string(model_string) @@ - provider_name = None - model_name = None + provider_name = None
22-31: Add missing type annotations for public functions (per repo guidelines).-@click.option('--model', '-m', +@click.option('--model', '-m', help='Model in provider:model format (e.g., openai:gpt-4o, anthropic:claude-3-5-sonnet-20241022)') @click.option('--api-key', '-k', help='API key for the selected provider') @click.option('--verbose', '-v', is_flag=True, help='Enable verbose output') -def cli(ctx, model, api_key, verbose): +def cli(ctx: click.Context, model: Optional[str], api_key: Optional[str], verbose: bool) -> None: @@ -@cli.command(name='providers') -def providers_cli(): +@cli.command(name='providers') +def providers_cli() -> None: @@ -from typing import Optional +from typing import Optional, Tuple @@ -def get_credentials_v2(model_string: Optional[str] = None, api_key: Optional[str] = None, verbose: bool = False): +def get_credentials_v2(model_string: Optional[str] = None, api_key: Optional[str] = None, verbose: bool = False) -> Tuple[str, str]: @@ -def main(model, api_key, verbose): +def main(model: Optional[str], api_key: Optional[str], verbose: bool) -> None:Also applies to: 42-59, 60-61, 118-118
60-116: Minor UX: echo supported examples only once during prompt loop.Current loop repeats examples on each invalid attempt; consider moving examples above loop and only re-print the error.
1-156: Ensure Google-style docstrings across public CLI functions.Some docstrings are terse; align with the repository’s guideline for Google-style docstrings for
cli,history_cli,providers_cli,get_credentials_v2,main.cli/src/infragpt/llm_adapter.py (2)
6-12: Trim unused imports.-from typing import Iterator, List, Dict, Any, Optional +from typing import Iterator, List, Dict, Any @@ -from .llm.exceptions import AuthenticationError, ValidationError, LLMError +from .llm.exceptions import ValidationError, LLMError
199-200: Remove unnecessary f-string.- if self.verbose: - console.print(f"[dim]Continuing conversation after tool execution...[/dim]") + if self.verbose: + console.print("[dim]Continuing conversation after tool execution...[/dim]")cli/src/infragpt/agent.py (3)
5-11: Remove unused imports.-import sys -import signal -from typing import List, Dict, Any, Optional -from dataclasses import dataclass -from datetime import datetime -from collections import deque +from typing import List, Dict, Any, Optional +from datetime import datetime @@ -from .llm.models import Message @@ -from prompt_toolkit import prompt, PromptSession -from prompt_toolkit.shortcuts import confirm +from prompt_toolkit import PromptSessionAlso applies to: 15-15, 22-25
155-159: Remove unnecessary f-string.- console.print(Panel.fit( - f"InfraGPT Shell Agent V2 - Direct SDK Integration", + console.print(Panel.fit( + "InfraGPT Shell Agent V2 - Direct SDK Integration", border_style="blue", title="[bold green]Shell Agent V2[/bold green]" ))
256-259: Combine nestedif(simpler and lint-friendly).- if chunk.finish_reason: - if self.verbose: - console.print(f"\n[dim]Finish reason: {chunk.finish_reason}[/dim]") + if chunk.finish_reason and self.verbose: + console.print(f"\n[dim]Finish reason: {chunk.finish_reason}[/dim]")cli/src/infragpt/llm/providers/openai_provider.py (2)
140-146: Fix type-hint lints and remove unused import inside function.- def _convert_tools(self, tools: List['Tool']) -> List[Dict]: + def _convert_tools(self, tools: List["Tool"]) -> List[Dict]: """Convert Tool objects to OpenAI format.""" - from ..models import Tool - openai_tools = []
178-179: Rename unused loop index.- for i, delta_call in enumerate(delta_tool_calls): + for _i, delta_call in enumerate(delta_tool_calls):cli/src/infragpt/llm/providers/anthropic_provider.py (1)
1-3: Consider adding module-level constants and version information.Following the pattern from the OpenAI provider and Python best practices, consider adding module-level constants for configuration values like default max_tokens. Also, consider documenting the minimum supported Anthropic SDK version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
cli/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
cli/CLAUDE.md(0 hunks)cli/pyproject.toml(1 hunks)cli/src/infragpt/agent.py(1 hunks)cli/src/infragpt/history.py(1 hunks)cli/src/infragpt/llm/README.md(0 hunks)cli/src/infragpt/llm/__init__.py(1 hunks)cli/src/infragpt/llm/auth.py(0 hunks)cli/src/infragpt/llm/base.py(1 hunks)cli/src/infragpt/llm/client.py(0 hunks)cli/src/infragpt/llm/errors.py(0 hunks)cli/src/infragpt/llm/exceptions.py(1 hunks)cli/src/infragpt/llm/models.py(1 hunks)cli/src/infragpt/llm/prompts.py(0 hunks)cli/src/infragpt/llm/providers/__init__.py(1 hunks)cli/src/infragpt/llm/providers/anthropic_provider.py(1 hunks)cli/src/infragpt/llm/providers/openai_provider.py(1 hunks)cli/src/infragpt/llm/router.py(1 hunks)cli/src/infragpt/llm_adapter.py(1 hunks)cli/src/infragpt/main.py(1 hunks)cli/src/infragpt/prompts.py(1 hunks)cli/src/infragpt/shell.py(1 hunks)cli/src/infragpt/tools.py(1 hunks)
💤 Files with no reviewable changes (6)
- cli/src/infragpt/llm/README.md
- cli/CLAUDE.md
- cli/src/infragpt/llm/auth.py
- cli/src/infragpt/llm/client.py
- cli/src/infragpt/llm/prompts.py
- cli/src/infragpt/llm/errors.py
🧰 Additional context used
📓 Path-based instructions (3)
cli/**/pyproject.toml
📄 CodeRabbit inference engine (cli/CLAUDE.md)
The CLI must be configured as a console script in pyproject.toml with infragpt = "cli.cli:cli"
Files:
cli/pyproject.toml
cli/**/*.py
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/**/*.py: Follow PEP 8 Python style guidelines
Use type annotations for all function signatures
Use Google-style docstrings for all public functions
Explicit exception handling with user-friendly messages
Organize imports in the following order: standard library imports, third-party imports, local imports
Files:
cli/src/infragpt/shell.pycli/src/infragpt/llm/providers/__init__.pycli/src/infragpt/llm/router.pycli/src/infragpt/history.pycli/src/infragpt/llm/exceptions.pycli/src/infragpt/llm/providers/anthropic_provider.pycli/src/infragpt/agent.pycli/src/infragpt/llm/__init__.pycli/src/infragpt/tools.pycli/src/infragpt/llm/models.pycli/src/infragpt/llm/base.pycli/src/infragpt/main.pycli/src/infragpt/llm/providers/openai_provider.pycli/src/infragpt/llm_adapter.pycli/src/infragpt/prompts.py
cli/**/__init__.py
📄 CodeRabbit inference engine (cli/CLAUDE.md)
All Python modules must include an init.py file for package initialization
Files:
cli/src/infragpt/llm/providers/__init__.pycli/src/infragpt/llm/__init__.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
PR: priyanshujain/infragpt#0
File: cli/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:53:49.372Z
Learning: Applies to cli/llm.py : Use LangChain through the shared LLM module for prompt templates
📚 Learning: 2025-07-30T07:54:31.378Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: services/agent/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:54:31.378Z
Learning: Applies to services/agent/src/agents/**/*.py : The Conversation Agent should provide natural language understanding, contextual dialogue management, and professional interaction patterns for infrastructure topics
Applied to files:
cli/src/infragpt/agent.py
📚 Learning: 2025-07-30T07:54:31.378Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: services/agent/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:54:31.378Z
Learning: Applies to services/agent/src/llm/**/*.py : Integrate LiteLLM client for LLM operations in the llm module
Applied to files:
cli/src/infragpt/llm/__init__.pycli/src/infragpt/llm_adapter.py
📚 Learning: 2025-07-30T07:53:49.372Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: cli/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:53:49.372Z
Learning: Applies to cli/llm.py : Use LangChain through the shared LLM module for prompt templates
Applied to files:
cli/src/infragpt/llm/__init__.py
📚 Learning: 2025-07-30T07:53:49.372Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: cli/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:53:49.372Z
Learning: Applies to cli/llm.py : Implement the Adapter pattern in cli/llm.py to interface with the shared LLM module
Applied to files:
cli/src/infragpt/llm/__init__.pycli/src/infragpt/llm_adapter.py
📚 Learning: 2025-07-30T07:54:31.378Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: services/agent/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:54:31.378Z
Learning: Applies to services/agent/src/tools/**/*.py : Implement base tool classes with an execution framework in the tools module
Applied to files:
cli/src/infragpt/tools.py
📚 Learning: 2025-07-30T07:54:31.378Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: services/agent/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:54:31.378Z
Learning: Applies to services/agent/src/tools/**/*.py : Implement tool registry and base tool classes in the tools module
Applied to files:
cli/src/infragpt/tools.py
📚 Learning: 2025-07-30T07:54:31.378Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: services/agent/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:54:31.378Z
Learning: Applies to services/agent/src/tools/**/*.py : The tool framework should support registry-based tool discovery and be ready for MCP (Model Context Protocol) integration
Applied to files:
cli/src/infragpt/tools.py
📚 Learning: 2025-07-30T07:53:49.372Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: cli/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:53:49.372Z
Learning: Applies to cli/tests/test_prompts.py : Test parameter extraction, command parsing, and multi-command handling in test_prompts.py
Applied to files:
cli/src/infragpt/prompts.py
🧬 Code graph analysis (11)
cli/src/infragpt/llm/providers/__init__.py (2)
cli/src/infragpt/llm/providers/openai_provider.py (1)
OpenAIProvider(22-241)cli/src/infragpt/llm/providers/anthropic_provider.py (1)
AnthropicProvider(22-211)
cli/src/infragpt/llm/router.py (4)
cli/src/infragpt/llm/providers/openai_provider.py (1)
OpenAIProvider(22-241)cli/src/infragpt/llm/providers/anthropic_provider.py (1)
AnthropicProvider(22-211)cli/src/infragpt/llm/base.py (1)
BaseLLMProvider(10-52)cli/src/infragpt/llm/exceptions.py (1)
ValidationError(36-38)
cli/src/infragpt/llm/exceptions.py (1)
cli/src/infragpt/llm/errors.py (4)
LLMError(6-8)ConfigurationError(31-33)GenerationError(16-18)ParsingError(21-23)
cli/src/infragpt/llm/providers/anthropic_provider.py (4)
cli/src/infragpt/llm/base.py (8)
BaseLLMProvider(10-52)_initialize_client(20-22)validate_api_key(30-32)_map_error(50-52)stream(25-27)_convert_tools(40-42)_convert_messages(35-37)_normalize_chunk(45-47)cli/src/infragpt/llm/models.py (3)
StreamChunk(46-50)ToolCall(38-42)Tool(30-34)cli/src/infragpt/llm/exceptions.py (6)
AuthenticationError(14-16)RateLimitError(19-21)APIError(24-28)ToolCallError(31-33)ContextWindowError(41-43)ValidationError(36-38)cli/src/infragpt/llm/providers/openai_provider.py (8)
_initialize_client(25-27)validate_api_key(29-47)_map_error(228-241)stream(49-105)_build_request(107-133)_convert_tools(140-171)_convert_messages(135-138)_normalize_chunk(222-226)
cli/src/infragpt/agent.py (5)
cli/src/infragpt/llm/models.py (1)
Message(54-71)cli/src/infragpt/llm_adapter.py (3)
get_llm_adapter(211-223)validate_api_key(38-45)stream_with_tools(47-100)cli/src/infragpt/history.py (1)
log_interaction(20-41)cli/src/infragpt/tools.py (1)
ToolExecutionCancelled(14-16)cli/src/infragpt/llm/providers/openai_provider.py (1)
validate_api_key(29-47)
cli/src/infragpt/llm/__init__.py (6)
cli/src/infragpt/llm/models.py (3)
Message(54-71)StreamChunk(46-50)ToolCall(38-42)cli/src/infragpt/llm/base.py (1)
BaseLLMProvider(10-52)cli/src/infragpt/llm/providers/openai_provider.py (1)
OpenAIProvider(22-241)cli/src/infragpt/llm/providers/anthropic_provider.py (1)
AnthropicProvider(22-211)cli/src/infragpt/llm/router.py (1)
LLMRouter(11-91)cli/src/infragpt/llm/exceptions.py (5)
LLMError(6-11)AuthenticationError(14-16)RateLimitError(19-21)APIError(24-28)ToolCallError(31-33)
cli/src/infragpt/tools.py (2)
cli/src/infragpt/shell.py (2)
CommandExecutor(31-233)execute_command(48-122)cli/src/infragpt/llm/models.py (3)
Tool(30-34)InputSchema(21-26)Parameter(12-17)
cli/src/infragpt/llm/base.py (5)
cli/src/infragpt/llm/models.py (2)
StreamChunk(46-50)Message(54-71)cli/src/infragpt/llm/providers/openai_provider.py (7)
_initialize_client(25-27)stream(49-105)validate_api_key(29-47)_convert_messages(135-138)_convert_tools(140-171)_normalize_chunk(222-226)_map_error(228-241)cli/src/infragpt/llm/providers/anthropic_provider.py (7)
_initialize_client(25-27)stream(41-115)validate_api_key(29-39)_convert_messages(145-160)_convert_tools(162-190)_normalize_chunk(192-196)_map_error(198-211)cli/src/infragpt/llm_adapter.py (1)
validate_api_key(38-45)cli/src/infragpt/llm/errors.py (2)
LLMError(6-8)AuthenticationError(11-13)
cli/src/infragpt/main.py (5)
cli/src/infragpt/config.py (2)
load_config(23-33)init_config(46-85)cli/src/infragpt/llm/router.py (5)
LLMRouter(11-91)get_supported_providers(75-83)get_provider_examples(86-91)validate_model_string(66-72)parse_model_string(27-45)cli/src/infragpt/llm/exceptions.py (2)
ValidationError(36-38)AuthenticationError(14-16)cli/src/infragpt/history.py (1)
history_command(125-162)cli/src/infragpt/agent.py (1)
run_shell_agent(302-305)
cli/src/infragpt/llm/providers/openai_provider.py (4)
cli/src/infragpt/llm/base.py (8)
BaseLLMProvider(10-52)_initialize_client(20-22)validate_api_key(30-32)_map_error(50-52)stream(25-27)_convert_tools(40-42)_convert_messages(35-37)_normalize_chunk(45-47)cli/src/infragpt/llm/models.py (3)
StreamChunk(46-50)ToolCall(38-42)Tool(30-34)cli/src/infragpt/llm/exceptions.py (6)
AuthenticationError(14-16)RateLimitError(19-21)APIError(24-28)ToolCallError(31-33)ContextWindowError(41-43)ValidationError(36-38)cli/src/infragpt/llm/providers/anthropic_provider.py (8)
_initialize_client(25-27)validate_api_key(29-39)_map_error(198-211)stream(41-115)_build_request(117-143)_convert_tools(162-190)_convert_messages(145-160)_normalize_chunk(192-196)
cli/src/infragpt/llm_adapter.py (4)
cli/src/infragpt/llm/router.py (3)
LLMRouter(11-91)create_provider(48-63)parse_model_string(27-45)cli/src/infragpt/llm/models.py (2)
StreamChunk(46-50)ToolCall(38-42)cli/src/infragpt/llm/exceptions.py (3)
AuthenticationError(14-16)ValidationError(36-38)LLMError(6-11)cli/src/infragpt/tools.py (3)
get_available_tools(168-175)execute_tool_call(187-192)ToolExecutionCancelled(14-16)
🪛 Ruff (0.12.2)
cli/src/infragpt/shell.py
19-19: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
24-24: rich.live.Live imported but unused
Remove unused import: rich.live.Live
(F401)
25-25: rich.text.Text imported but unused
Remove unused import: rich.text.Text
(F401)
80-83: Use contextlib.suppress(BaseException) instead of try-except-pass
(SIM105)
82-82: Do not use bare except
(E722)
cli/src/infragpt/llm/router.py
5-5: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
63-63: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
cli/src/infragpt/llm/providers/anthropic_provider.py
16-16: ..exceptions.ToolCallError imported but unused
Remove unused import: ..exceptions.ToolCallError
(F401)
32-32: Local variable response is assigned to but never used
Remove assignment to unused variable response
(F841)
39-39: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
105-106: Use a single if statement instead of nested if statements
(SIM102)
115-115: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
162-162: Undefined name Tool
(F821)
164-164: ..models.Tool imported but unused
Remove unused import: ..models.Tool
(F401)
cli/src/infragpt/agent.py
5-5: sys imported but unused
Remove unused import: sys
(F401)
6-6: signal imported but unused
Remove unused import: signal
(F401)
8-8: dataclasses.dataclass imported but unused
Remove unused import: dataclasses.dataclass
(F401)
10-10: collections.deque imported but unused
Remove unused import: collections.deque
(F401)
15-15: .llm.models.Message imported but unused
Remove unused import: .llm.models.Message
(F401)
22-22: prompt_toolkit.prompt imported but unused
Remove unused import: prompt_toolkit.prompt
(F401)
23-23: prompt_toolkit.shortcuts.confirm imported but unused
Remove unused import: prompt_toolkit.shortcuts.confirm
(F401)
156-156: f-string without any placeholders
Remove extraneous f prefix
(F541)
256-257: Use a single if statement instead of nested if statements
(SIM102)
cli/src/infragpt/tools.py
6-6: json imported but unused
Remove unused import: json
(F401)
46-46: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
48-48: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
50-50: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
52-52: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
58-58: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
59-59: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
61-61: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
63-63: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
65-65: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
118-118: f-string without any placeholders
Remove extraneous f prefix
(F541)
125-125: f-string without any placeholders
Remove extraneous f prefix
(F541)
132-132: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
147-147: f-string without any placeholders
Remove extraneous f prefix
(F541)
cli/src/infragpt/llm/base.py
7-7: .models.Message imported but unused
Remove unused import: .models.Message
(F401)
40-40: Undefined name Tool
(F821)
cli/src/infragpt/main.py
11-11: infragpt.config.CONFIG_FILE imported but unused
Remove unused import
(F401)
11-11: infragpt.config.load_config imported but unused
Remove unused import
(F401)
65-65: f-string without any placeholders
Remove extraneous f prefix
(F541)
70-70: Local variable model_name is assigned to but never used
Remove assignment to unused variable model_name
(F841)
127-127: Do not use bare except
(E722)
cli/src/infragpt/llm/providers/openai_provider.py
16-16: ..exceptions.ToolCallError imported but unused
Remove unused import: ..exceptions.ToolCallError
(F401)
44-44: Local variable response is assigned to but never used
Remove assignment to unused variable response
(F841)
47-47: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
105-105: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
140-140: Undefined name Tool
(F821)
142-142: ..models.Tool imported but unused
Remove unused import: ..models.Tool
(F401)
178-178: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
cli/src/infragpt/llm_adapter.py
6-6: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
10-10: .llm.exceptions.AuthenticationError imported but unused
Remove unused import: .llm.exceptions.AuthenticationError
(F401)
36-36: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
100-100: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
199-199: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (10)
cli/pyproject.toml (2)
11-11: Confirm Python 3.13 floor is intentional.3.13-only may unnecessarily exclude 3.10–3.12 users unless needed by the new agentic loop.
If not required, consider broadening:
-requires-python = ">=3.13" +requires-python = ">=3.10"
36-36: Keep current entry point; no change needed. The console script infragpt = "infragpt.main:cli" correctly references the cli() function in cli/src/infragpt/main.py, and there is no cli/cli.py or cli/main.py to target.Likely an incorrect or invalid review comment.
cli/src/infragpt/llm/exceptions.py (1)
24-29: No action needed: ValidationError is only imported viainfragpt.llm.exceptionsand no imports frominfragpt.llmwere found, so__init__.pyneed not re-export it.cli/src/infragpt/llm/providers/__init__.py (1)
1-8: Package initializer looks good.Public API and all set correctly; matches router usage.
cli/src/infragpt/llm/router.py (1)
89-91: Verify example model identifiers are current
Runopenai api models.listandanthropic models list(or check your SDK docs) to confirm thatopenai:gpt-4oandanthropic:claude-3-5-sonnet-20241022are still valid defaults; update them if newer model identifiers should be used.cli/src/infragpt/llm/base.py (1)
40-42: Keep forward ref but ensure type checkers resolve Tool.With TYPE_CHECKING guard above, this signature is fine.
cli/src/infragpt/main.py (1)
1-156: No missing init.py files detected. All package directories under cli/src/infragpt (including llm and llm/providers) contain an init.py file.cli/src/infragpt/llm/__init__.py (1)
2-31: LGTM: cohesive public surface for SDK-based LLMs.Imports/exports are consistent; no LangChain leakage.
cli/src/infragpt/llm/providers/anthropic_provider.py (2)
145-160: Remove the truly unused_convert_messagesoverride inanthropic_provider.pyThe class-level
_convert_messagesincli/src/infragpt/llm/providers/anthropic_provider.pyis never called directly within that file (all message conversion in_build_requestreimplements the same logic) and only shadows the base‐class implementation incli/src/infragpt/llm/base.py. Remove this override and update_build_requestto callsuper()._convert_messages(...).Likely an incorrect or invalid review comment.
162-190: Fix type annotation issue in_convert_toolsmethod.The type hint
List['Tool']referencesToolbefore importing it, which causes a forward reference issue. The import statement on line 164 is redundant.Apply this diff to fix the type annotation:
-def _convert_tools(self, tools: List['Tool']) -> List[Dict]: +def _convert_tools(self, tools: List[Dict]) -> List[Dict]: """Convert Tool objects to Anthropic format.""" - from ..models import Tool anthropic_tools = [] for tool in tools:Likely an incorrect or invalid review comment.
| def _terminate_command(self): | ||
| """Terminate the current command.""" | ||
| if self.current_process: | ||
| try: | ||
| # Send SIGTERM to the process group | ||
| os.killpg(os.getpgid(self.current_process.pid), signal.SIGTERM) | ||
|
|
||
| # Wait a bit for graceful shutdown | ||
| time.sleep(1) | ||
|
|
||
| # Force kill if still running | ||
| if self.current_process.poll() is None: | ||
| os.killpg(os.getpgid(self.current_process.pid), signal.SIGKILL) | ||
| except (OSError, ProcessLookupError): | ||
| # Process already terminated | ||
| pass | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Termination is POSIX-only; add Windows-safe fallback.
Use killpg on POSIX, terminate/kill on Windows.
def _terminate_command(self):
"""Terminate the current command."""
if self.current_process:
try:
- # Send SIGTERM to the process group
- os.killpg(os.getpgid(self.current_process.pid), signal.SIGTERM)
-
- # Wait a bit for graceful shutdown
- time.sleep(1)
-
- # Force kill if still running
- if self.current_process.poll() is None:
- os.killpg(os.getpgid(self.current_process.pid), signal.SIGKILL)
+ if os.name == "posix":
+ # Send SIGTERM to the process group
+ os.killpg(os.getpgid(self.current_process.pid), signal.SIGTERM)
+ time.sleep(1)
+ if self.current_process.poll() is None:
+ os.killpg(os.getpgid(self.current_process.pid), signal.SIGKILL)
+ else:
+ # Windows fallback
+ self.current_process.terminate()
+ time.sleep(1)
+ if self.current_process.poll() is None:
+ self.current_process.kill()
except (OSError, ProcessLookupError):
# Process already terminated
pass📝 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 _terminate_command(self): | |
| """Terminate the current command.""" | |
| if self.current_process: | |
| try: | |
| # Send SIGTERM to the process group | |
| os.killpg(os.getpgid(self.current_process.pid), signal.SIGTERM) | |
| # Wait a bit for graceful shutdown | |
| time.sleep(1) | |
| # Force kill if still running | |
| if self.current_process.poll() is None: | |
| os.killpg(os.getpgid(self.current_process.pid), signal.SIGKILL) | |
| except (OSError, ProcessLookupError): | |
| # Process already terminated | |
| pass | |
| def _terminate_command(self): | |
| """Terminate the current command.""" | |
| if self.current_process: | |
| try: | |
| if os.name == "posix": | |
| # Send SIGTERM to the process group | |
| os.killpg(os.getpgid(self.current_process.pid), signal.SIGTERM) | |
| time.sleep(1) | |
| if self.current_process.poll() is None: | |
| os.killpg(os.getpgid(self.current_process.pid), signal.SIGKILL) | |
| else: | |
| # Windows fallback | |
| self.current_process.terminate() | |
| time.sleep(1) | |
| if self.current_process.poll() is None: | |
| self.current_process.kill() | |
| except (OSError, ProcessLookupError): | |
| # Process already terminated | |
| pass |
🤖 Prompt for AI Agents
In cli/src/infragpt/shell.py around lines 179 to 195, the current
_terminate_command implementation uses os.killpg which is POSIX-only; update it
to detect Windows and use the subprocess API instead: if running on Windows
(os.name == "nt" or platform.system() == "Windows"), call
self.current_process.terminate(), wait briefly and then
self.current_process.kill() if still running; otherwise keep the existing
os.killpg(SIGTERM)/sleep/kill flow for POSIX. Wrap both flows in try/except to
ignore already-terminated processes and check self.current_process.poll() before
force-killing.
- Add sanitize_sensitive_data function to mask API keys, passwords, and tokens - Automatically redact sensitive information before logging to history - Set restrictive file permissions (0o600) on history file - Add patterns to detect and mask common sensitive data formats: - API keys starting with 'sk-' - Bearer tokens - Long alphanumeric strings (32+ chars) - Recursively sanitize nested data structures - Add security comments for CodeQL scanning This prevents sensitive information from being stored in plain text in the history file.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
cli/src/infragpt/tools.py (2)
7-7: Robust type inference for annotations; fix E721 "type comparisons".Use get_origin/get_args to support PEP 604 unions and use
is/is notfor type comparisons. Matches previous feedback.-from typing import Callable, Dict, Any, Optional, List +from typing import Callable, Dict, Any, Optional, List, get_origin, get_args @@ - if param.annotation != inspect.Parameter.empty: - if param.annotation == str: + if param.annotation is not inspect.Parameter.empty: + ann = param.annotation + origin = get_origin(ann) + args = get_args(ann) + # Handle Optional[T] / Union[..., None] + if origin is Optional or (origin is Union and type(None) in args): # type: ignore[name-defined] + non_none = [t for t in args if t is not type(None)] + ann = non_none[0] if non_none else str + param_description = f"Optional parameter {param_name}" + if ann is str: param_type = "string" - elif param.annotation == int: + elif ann is int: param_type = "integer" - elif param.annotation == float: + elif ann is float: param_type = "number" - elif param.annotation == bool: + elif ann is bool: param_type = "boolean" - - # Check if it's Optional (has default None) - if hasattr(param.annotation, '__args__') and type(None) in param.annotation.__args__: - # It's Optional, extract the actual type - actual_type = next(t for t in param.annotation.__args__ if t != type(None)) - if actual_type == str: - param_type = "string" - elif actual_type == int: - param_type = "integer" - elif actual_type == float: - param_type = "number" - elif actual_type == bool: - param_type = "boolean" - param_description = f"Optional parameter {param_name}"Also applies to: 45-67
128-137: Default to safe “No” and chain cancellation exceptions.Avoid accidental execution; treat empty input as “no” and chain exceptions. Mirrors prior guidance.
- try: - user_input = input().strip().lower() - except (KeyboardInterrupt, EOFError): - console.print("\n[yellow]Command execution cancelled.[/yellow]") - raise ToolExecutionCancelled("User cancelled command execution") + try: + user_input = input().strip().lower() + except (KeyboardInterrupt, EOFError) as e: + console.print("\n[yellow]Command execution cancelled.[/yellow]") + raise ToolExecutionCancelled("User cancelled command execution") from None @@ - # Only execute if user explicitly confirms with 'y' or 'yes' - if user_input not in ['y', 'yes', '']: # Empty input defaults to yes for backward compatibility + # Only execute if user explicitly confirms with 'y' or 'yes' + if user_input not in ('y', 'yes'): console.print("\n[yellow]Command execution cancelled.[/yellow]") raise ToolExecutionCancelled("User cancelled command execution")cli/src/infragpt/shell.py (2)
189-194: Mark cancellation on timeout.Expose timeout as
was_cancelled=Trueto callers. Matches prior feedback.def _timeout_handler(self): """Handle command timeout.""" if self.current_process and self.current_process.poll() is None: console.print(f"\n[bold yellow]Command timed out after {self.timeout} seconds[/bold yellow]") - self._terminate_command() + self.cancelled = True + self._terminate_command()
195-214: POSIX-only termination; add Windows-safe fallback.Support Windows with terminate/kill; keep pg kill on POSIX. Mirrors earlier suggestion.
def _terminate_command(self): """Terminate the current command.""" if self.current_process: - try: - # Check if process group exists - pgid = os.getpgid(self.current_process.pid) - except (OSError, ProcessLookupError): - # Process group does not exist, nothing to terminate - return - try: - # Send SIGTERM to the process group - os.killpg(pgid, signal.SIGTERM) - # Wait a bit for graceful shutdown - time.sleep(1) - # Force kill if still running - if self.current_process.poll() is None: - os.killpg(pgid, signal.SIGKILL) - except (OSError, ProcessLookupError): - # Process group may have terminated between checks - pass + if os.name == "posix": + try: + pgid = os.getpgid(self.current_process.pid) + except (OSError, ProcessLookupError): + return + try: + os.killpg(pgid, signal.SIGTERM) + time.sleep(1) + if self.current_process.poll() is None: + os.killpg(pgid, signal.SIGKILL) + except (OSError, ProcessLookupError): + pass + else: + try: + self.current_process.terminate() + time.sleep(1) + if self.current_process.poll() is None: + self.current_process.kill() + except Exception: + passcli/src/infragpt/llm/providers/openai_provider.py (2)
44-47: Remove unused variable and chain mapped exception.Avoid assigning unused
response; preserve traceback withfrom e. This repeats earlier guidance.- response = self._client.chat.completions.create(**params) + self._client.chat.completions.create(**params) return True except Exception as e: - raise self._map_error(e) + raise self._map_error(e) from e
104-105: Chain mapped exception in stream wrapper.Preserve original traceback. As noted previously.
- except Exception as e: - raise self._map_error(e) + except Exception as e: + raise self._map_error(e) from ecli/src/infragpt/llm/providers/anthropic_provider.py (1)
21-21: Good: switched from prints to logger and added exception chaining.This addresses prior review feedback and aligns with the project’s logging and error-handling practices.
🧹 Nitpick comments (14)
cli/src/infragpt/tools.py (4)
6-6: Remove unused import.
jsonis not used.-from typing import Callable, Dict, Any, Optional, List +from typing import Callable, Dict, Any, Optional, List
22-27: Add precise return type for decorator.Annotate decorator to satisfy typing guidelines.
-def tool(name: Optional[str] = None, description: Optional[str] = None): +def tool( + name: Optional[str] = None, + description: Optional[str] = None, +) -> Callable[[Callable[..., Any]], Callable[..., Any]]:
118-118: Remove extraneous f-strings (F541).These strings have no interpolations.
-console.print(f"\n[bold cyan]Tool Call: execute_shell_command[/bold cyan]") +console.print("\n[bold cyan]Tool Call: execute_shell_command[/bold cyan]") @@ -console.print(f"\n[yellow]Execute this command? (Y/n):[/yellow] ", end="") +console.print("\n[yellow]Execute this command? (Y/n):[/yellow] ", end="") @@ -console.print(f"\n[bold blue]Executing command...[/bold blue]") +console.print("\n[bold blue]Executing command...[/bold blue]")Also applies to: 125-125, 146-146
167-175: Auto-register decorated tools; avoid hardcoding registry.Let the decorator append to a registry so get_available_tools stays in sync.
- func._tool = tool_obj + func._tool = tool_obj + TOOL_REGISTRY.append(tool_obj)-def get_available_tools() -> List[Tool]: +def get_available_tools() -> List[Tool]: @@ - tools = [] - - # Add the shell command tool - tools.append(execute_shell_command._tool) - - return tools + return TOOL_REGISTRY.copy()Add near the console declaration:
TOOL_REGISTRY: List[Tool] = []Also applies to: 95-96
cli/src/infragpt/llm_adapter.py (2)
5-11: Tighten imports; remove unused symbols.Drop Optional and AuthenticationError; keep only what’s used.
-import json -from typing import Iterator, List, Dict, Any, Optional +import json +from typing import Iterator, List, Dict, Any @@ -from .llm.exceptions import AuthenticationError, ValidationError, LLMError +from .llm.exceptions import ValidationError, LLMError
199-199: Remove extraneous f-string (F541).- console.print(f"[dim]Continuing conversation after tool execution...[/dim]") + console.print("[dim]Continuing conversation after tool execution...[/dim]")cli/src/infragpt/shell.py (3)
19-19: Remove unused imports.
Any,Live, andTextare unused.-from typing import Optional, Dict, Any, Tuple +from typing import Optional, Dict, Tuple @@ -from rich.live import Live -from rich.text import Text +Also applies to: 24-25
61-63: Only show ESC hint where supported.Avoid misleading prompt on platforms without termios.
-console.print("[dim]Press ESC to cancel command...[/dim]\n") +if os.name == "posix": + console.print("[dim]Press ESC to cancel command...[/dim]\n")
79-83: Avoid bare except; use suppress or explicit exception.Cleaner and linter-friendly.
- if hasattr(os, 'setsid'): - try: - popen_args['preexec_fn'] = os.setsid - except: - pass # Skip if not supported + if hasattr(os, 'setsid'): + from contextlib import suppress + with suppress(Exception): + popen_args['preexec_fn'] = os.setsidcli/src/infragpt/llm/providers/openai_provider.py (3)
16-19: Remove unused exception import.
ToolCallErroris not referenced.- APIError, - ToolCallError, + APIError,
140-144: Fix type/import issues for Tool; align signature with usage.Avoid F821/F401 by not importing
Toolat runtime and typing asList[Dict](matches BaseLLMProvider contract).- def _convert_tools(self, tools: List['Tool']) -> List[Dict]: + def _convert_tools(self, tools: List[Dict]) -> List[Dict]: - from ..models import Tool - - openai_tools = [] + openai_tools = []
177-177: Rename unused loop variable.Silence B007.
- for i, delta_call in enumerate(delta_tool_calls): + for _i, delta_call in enumerate(delta_tool_calls):cli/src/infragpt/llm/providers/anthropic_provider.py (2)
79-84: Harden streaming: log out-of-order JSON deltas and free buffers after tool completion.Prevents silent drops on unknown indices and avoids unbounded growth when many tool calls occur.
@@ - elif event.delta.type == "input_json_delta": + elif event.delta.type == "input_json_delta": # Tool call arguments (partial JSON) - use index tracking index = event.index if index in tool_use_inputs: tool_use_inputs[index] += event.delta.partial_json + else: + logger.debug("Received input_json_delta for unknown index %s; ignoring fragment", index) @@ - yield StreamChunk(tool_calls=[tool_call]) + yield StreamChunk(tool_calls=[tool_call]) + # Cleanup buffers for this tool index + try: + del tool_use_inputs[index] + del tool_blocks[index] + except KeyError: + passAlso applies to: 85-104
129-135: Reconsider defaultmax_tokens=4096to mitigate cost/spillover risk.Large defaults can cause unexpected token usage or context overflow. Prefer a conservative default (e.g., 1024) or require callers to pass it explicitly.
- "max_tokens": kwargs.get("max_tokens", 4096), # Required for Anthropic + "max_tokens": kwargs.get("max_tokens", 1024), # Safer default; override as needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
cli/src/infragpt/llm/models.py(1 hunks)cli/src/infragpt/llm/providers/anthropic_provider.py(1 hunks)cli/src/infragpt/llm/providers/openai_provider.py(1 hunks)cli/src/infragpt/llm_adapter.py(1 hunks)cli/src/infragpt/shell.py(1 hunks)cli/src/infragpt/tools.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.py
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/**/*.py: Follow PEP 8 Python style guidelines
Use type annotations for all function signatures
Use Google-style docstrings for all public functions
Explicit exception handling with user-friendly messages
Organize imports in the following order: standard library imports, third-party imports, local imports
Files:
cli/src/infragpt/llm_adapter.pycli/src/infragpt/llm/models.pycli/src/infragpt/llm/providers/anthropic_provider.pycli/src/infragpt/llm/providers/openai_provider.pycli/src/infragpt/shell.pycli/src/infragpt/tools.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
PR: priyanshujain/infragpt#0
File: cli/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:53:49.372Z
Learning: Applies to cli/llm.py : Use LangChain through the shared LLM module for prompt templates
📚 Learning: 2025-07-30T07:53:49.372Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: cli/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:53:49.372Z
Learning: Applies to cli/llm.py : Implement the Adapter pattern in cli/llm.py to interface with the shared LLM module
Applied to files:
cli/src/infragpt/llm_adapter.py
📚 Learning: 2025-07-30T07:54:31.378Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: services/agent/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:54:31.378Z
Learning: Applies to services/agent/src/llm/**/*.py : Integrate LiteLLM client for LLM operations in the llm module
Applied to files:
cli/src/infragpt/llm_adapter.py
📚 Learning: 2025-07-30T07:53:49.372Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: cli/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:53:49.372Z
Learning: Applies to cli/llm.py : Use LangChain through the shared LLM module for prompt templates
Applied to files:
cli/src/infragpt/llm_adapter.py
📚 Learning: 2025-07-30T07:54:31.378Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: services/agent/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:54:31.378Z
Learning: Applies to services/agent/src/tools/**/*.py : Implement base tool classes with an execution framework in the tools module
Applied to files:
cli/src/infragpt/tools.py
📚 Learning: 2025-07-30T07:54:31.378Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: services/agent/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:54:31.378Z
Learning: Applies to services/agent/src/tools/**/*.py : Implement tool registry and base tool classes in the tools module
Applied to files:
cli/src/infragpt/tools.py
📚 Learning: 2025-07-30T07:54:31.378Z
Learnt from: CR
PR: priyanshujain/infragpt#0
File: services/agent/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:54:31.378Z
Learning: Applies to services/agent/src/tools/**/*.py : The tool framework should support registry-based tool discovery and be ready for MCP (Model Context Protocol) integration
Applied to files:
cli/src/infragpt/tools.py
🧬 Code graph analysis (4)
cli/src/infragpt/llm_adapter.py (6)
cli/src/infragpt/llm/router.py (3)
LLMRouter(11-91)create_provider(48-63)parse_model_string(27-45)cli/src/infragpt/llm/models.py (2)
StreamChunk(46-50)ToolCall(38-42)cli/src/infragpt/llm/exceptions.py (3)
AuthenticationError(14-16)ValidationError(36-38)LLMError(6-11)cli/src/infragpt/tools.py (3)
get_available_tools(167-174)execute_tool_call(186-191)ToolExecutionCancelled(14-16)cli/src/infragpt/llm/providers/anthropic_provider.py (2)
validate_api_key(37-47)stream(49-115)cli/src/infragpt/llm/providers/openai_provider.py (2)
validate_api_key(29-47)stream(49-105)
cli/src/infragpt/llm/providers/anthropic_provider.py (5)
cli/src/infragpt/llm/base.py (8)
BaseLLMProvider(10-52)_initialize_client(20-22)validate_api_key(30-32)_map_error(50-52)stream(25-27)_convert_tools(40-42)_convert_messages(35-37)_normalize_chunk(45-47)cli/src/infragpt/llm/models.py (3)
StreamChunk(46-50)ToolCall(38-42)Tool(30-34)cli/src/infragpt/llm/exceptions.py (5)
AuthenticationError(14-16)RateLimitError(19-21)APIError(24-28)ContextWindowError(41-43)ValidationError(36-38)cli/src/infragpt/llm/providers/openai_provider.py (8)
_initialize_client(25-27)validate_api_key(29-47)_map_error(229-242)stream(49-105)_build_request(107-133)_convert_tools(140-171)_convert_messages(135-138)_normalize_chunk(223-227)cli/src/infragpt/tools.py (1)
tool(22-103)
cli/src/infragpt/llm/providers/openai_provider.py (4)
cli/src/infragpt/llm/base.py (8)
BaseLLMProvider(10-52)_initialize_client(20-22)validate_api_key(30-32)_map_error(50-52)stream(25-27)_convert_tools(40-42)_convert_messages(35-37)_normalize_chunk(45-47)cli/src/infragpt/llm/models.py (3)
StreamChunk(46-50)ToolCall(38-42)Tool(30-34)cli/src/infragpt/llm/exceptions.py (6)
AuthenticationError(14-16)RateLimitError(19-21)APIError(24-28)ToolCallError(31-33)ContextWindowError(41-43)ValidationError(36-38)cli/src/infragpt/llm/providers/anthropic_provider.py (8)
_initialize_client(33-35)validate_api_key(37-47)_map_error(198-211)stream(49-115)_build_request(117-143)_convert_tools(162-190)_convert_messages(145-160)_normalize_chunk(192-196)
cli/src/infragpt/tools.py (2)
cli/src/infragpt/shell.py (2)
CommandExecutor(31-253)execute_command(48-138)cli/src/infragpt/llm/models.py (3)
Tool(30-34)InputSchema(21-26)Parameter(12-17)
🪛 Ruff (0.12.2)
cli/src/infragpt/llm_adapter.py
6-6: typing.Optional imported but unused
Remove unused import: typing.Optional
(F401)
10-10: .llm.exceptions.AuthenticationError imported but unused
Remove unused import: .llm.exceptions.AuthenticationError
(F401)
199-199: f-string without any placeholders
Remove extraneous f prefix
(F541)
cli/src/infragpt/llm/providers/anthropic_provider.py
162-162: Undefined name Tool
(F821)
164-164: ..models.Tool imported but unused
Remove unused import: ..models.Tool
(F401)
cli/src/infragpt/llm/providers/openai_provider.py
16-16: ..exceptions.ToolCallError imported but unused
Remove unused import: ..exceptions.ToolCallError
(F401)
44-44: Local variable response is assigned to but never used
Remove assignment to unused variable response
(F841)
47-47: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
105-105: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
140-140: Undefined name Tool
(F821)
142-142: ..models.Tool imported but unused
Remove unused import: ..models.Tool
(F401)
177-177: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
cli/src/infragpt/shell.py
19-19: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
24-24: rich.live.Live imported but unused
Remove unused import: rich.live.Live
(F401)
25-25: rich.text.Text imported but unused
Remove unused import: rich.text.Text
(F401)
80-83: Use contextlib.suppress(BaseException) instead of try-except-pass
(SIM105)
82-82: Do not use bare except
(E722)
cli/src/infragpt/tools.py
6-6: json imported but unused
Remove unused import: json
(F401)
46-46: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
48-48: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
50-50: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
52-52: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
58-58: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
59-59: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
61-61: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
63-63: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
65-65: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
118-118: f-string without any placeholders
Remove extraneous f prefix
(F541)
125-125: f-string without any placeholders
Remove extraneous f prefix
(F541)
132-132: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
146-146: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ 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: Analyze (go)
🔇 Additional comments (1)
cli/src/infragpt/llm/models.py (1)
1-71: LGTM.Dataclasses, UTC-aware timestamps, and schema modeling look solid.
| """ | ||
| Anthropic provider implementation using direct SDK. | ||
| """ | ||
|
|
||
| import json | ||
| import logging | ||
| from typing import List, Dict, Any, Iterator, Optional | ||
| from anthropic import Anthropic | ||
| import anthropic | ||
|
|
There was a problem hiding this comment.
Fix undefined name 'Tool' and remove unused import; add safe forward-ref import.
Ruff flags F821 (undefined name Tool) on the annotation and F401 (unused import) for the local import inside _convert_tools. Use a TYPE_CHECKING gate and drop the local import to satisfy lint and keep annotations precise.
Apply this diff:
@@
-"""
-Anthropic provider implementation using direct SDK.
-"""
+"""
+Anthropic provider implementation using direct SDK.
+"""
+
+from __future__ import annotations
@@
-from typing import List, Dict, Any, Iterator, Optional
+from typing import List, Dict, Any, Iterator, Optional, TYPE_CHECKING
@@
logger = logging.getLogger(__name__)
+if TYPE_CHECKING:
+ from ..models import Tool
@@
-def _convert_tools(self, tools: List['Tool']) -> List[Dict]:
+def _convert_tools(self, tools: List['Tool']) -> List[Dict]:
@@
- from ..models import Tool
-
anthropic_tools = []Also applies to: 162-170, 164-166
🤖 Prompt for AI Agents
In cli/src/infragpt/llm/providers/anthropic_provider.py around lines 1 to 10,
the code uses the name Tool in type annotations but causes a lint F821 and also
has an unused local import inside _convert_tools; to fix it add "from typing
import TYPE_CHECKING" to the top imports, wrap a type-only import of the real
Tool under "if TYPE_CHECKING: from <correct_tool_module> import Tool" (replace
<correct_tool_module> with the module that defines Tool), change any runtime
annotations to use the forward-ref string "Tool" (e.g. def fn(..., tools:
List['Tool']) -> ...), and remove the local import inside _convert_tools so
there are no unused runtime imports.
| def _initialize_client(self, **kwargs): | ||
| """Initialize Anthropic client.""" | ||
| return Anthropic(api_key=self.api_key) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add return type to _initialize_client to meet typing guideline.
All function signatures should be annotated. Return the concrete SDK client type.
-def _initialize_client(self, **kwargs):
+def _initialize_client(self, **kwargs) -> Anthropic:
"""Initialize Anthropic client."""
return Anthropic(api_key=self.api_key)📝 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 _initialize_client(self, **kwargs): | |
| """Initialize Anthropic client.""" | |
| return Anthropic(api_key=self.api_key) | |
| def _initialize_client(self, **kwargs) -> Anthropic: | |
| """Initialize Anthropic client.""" | |
| return Anthropic(api_key=self.api_key) |
🤖 Prompt for AI Agents
In cli/src/infragpt/llm/providers/anthropic_provider.py around lines 33 to 36,
the _initialize_client method lacks a return type annotation; update the
signature to return the concrete SDK client type (Anthropic) and ensure that
type is imported (e.g. add from anthropic import Anthropic or a
TYPE_CHECKING-only import if desired) so the signature becomes def
_initialize_client(self, **kwargs) -> Anthropic: and the implementation still
returns Anthropic(api_key=self.api_key).
| def stream(self, messages: List[Dict[str, Any]], tools: Optional[List[Dict]] = None, **kwargs) -> Iterator[StreamChunk]: | ||
| """Stream response with unified tool calling support following Anthropic best practices.""" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bring docstrings in line with Google style for public methods.
Add Args/Returns/Yields/Raises sections to match the repo’s guidelines.
@@
- def stream(self, messages: List[Dict[str, Any]], tools: Optional[List[Dict]] = None, **kwargs) -> Iterator[StreamChunk]:
- """Stream response with unified tool calling support following Anthropic best practices."""
+ def stream(self, messages: List[Dict[str, Any]], tools: Optional[List[Dict]] = None, **kwargs) -> Iterator[StreamChunk]:
+ """Stream response with unified tool-calling.
+
+ Args:
+ messages: Conversation in unified format (list of role/content dicts).
+ tools: Optional list of tool definitions.
+ **kwargs: Provider-specific options (e.g., max_tokens, temperature).
+ Yields:
+ StreamChunk: Text deltas, tool calls, and finish signals as they arrive.
+ Raises:
+ AuthenticationError, RateLimitError, ContextWindowError, ValidationError, APIError:
+ On mapped provider errors.
+ """
@@
- def _build_request(self, messages: List[Dict[str, Any]], tools: Optional[List[Dict]] = None, **kwargs: Any) -> Dict[str, Any]:
- """Build Anthropic API request."""
+ def _build_request(self, messages: List[Dict[str, Any]], tools: Optional[List[Dict]] = None, **kwargs: Any) -> Dict[str, Any]:
+ """Build Anthropic API request.
+
+ Args:
+ messages: Unified messages (system extracted automatically).
+ tools: Optional tool definitions.
+ **kwargs: Additional provider params (max_tokens, temperature).
+ Returns:
+ Dict[str, Any]: Parameters for `Anthropic.messages.create`.
+ """
@@
- def _normalize_chunk(self, raw_chunk) -> StreamChunk:
- """Convert Anthropic chunk to unified format."""
- # This method is not used in the current implementation
- # as we handle normalization in the stream method
- raise NotImplementedError("This method is not used in the current implementation")
+ def _normalize_chunk(self, raw_chunk) -> StreamChunk:
+ """Convert Anthropic chunk to unified format.
+
+ Note:
+ Not used—normalization is performed directly in `stream`.
+ Args:
+ raw_chunk: Provider-specific chunk/event.
+ Returns:
+ StreamChunk: Normalized chunk.
+ Raises:
+ NotImplementedError: Always; see note above.
+ """
+ raise NotImplementedError("Chunk normalization is handled in the stream method")Also applies to: 117-119, 192-196
🤖 Prompt for AI Agents
In cli/src/infragpt/llm/providers/anthropic_provider.py around lines 49-50 (and
also update the public methods at 117-119 and 192-196), the docstrings are
missing Google-style sections; update each public method docstring to follow
Google style by adding an Args section describing messages: List[Dict[str,
Any]], tools: Optional[List[Dict]] and **kwargs with brief types and purpose, a
Returns or Yields section indicating the method yields Iterator[StreamChunk]
(explain chunk contents), and a Raises section listing possible exceptions
raised (e.g., network/API errors or validation errors); keep descriptions
concise and consistent with the repo guidelines.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cli/src/infragpt/history.py (1)
64-66: Fix CodeQL finding: enforce 0600 at creation, remove misapplied nosec, and make JSON writing robustWriting first then chmod can expose contents momentarily; also the Bandit
# nosec B108is unrelated and won’t affect CodeQL. Create the file with 0o600, use utf-8, and handle non-JSON-serializable types.- # Append to history file - # CodeQL: Data is sanitized before storage to prevent leaking sensitive information - with open(HISTORY_DB_FILE, "a") as f: - f.write(json.dumps(entry) + "\n") # nosec B108 - Data sanitized above - - # Set restrictive permissions (user read/write only) - os.chmod(HISTORY_DB_FILE, 0o600) + # Append to history file with restrictive permissions from creation time + fd = os.open(HISTORY_DB_FILE, os.O_CREAT | os.O_APPEND | os.O_WRONLY, 0o600) + try: + with os.fdopen(fd, "a", encoding="utf-8") as f: + f.write(json.dumps(entry, ensure_ascii=False, default=str) + "\n") + finally: + try: + os.chmod(HISTORY_DB_FILE, 0o600) # enforce on existing files too + except Exception: + passAlso, per repo guidelines, consider adding explicit return annotations (
-> None) tolog_interaction,init_history_dir,display_history_entry, andhistory_command.Run this to ensure no other code paths write unsanitized history:
#!/bin/bash # Find all writes to history.jsonl and verify they go through log_interaction rg -nP -C2 'open\([^)]*history\.jsonl|HISTORY_DB_FILE' || true rg -nP 'log_interaction\s*\(' -C2Also applies to: 72-72, 76-76, 78-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cli/src/infragpt/history.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
cli/**/*.py
📄 CodeRabbit inference engine (cli/CLAUDE.md)
cli/**/*.py: Follow PEP 8 Python style guidelines
Use type annotations for all function signatures
Use Google-style docstrings for all public functions
Explicit exception handling with user-friendly messages
Organize imports in the following order: standard library imports, third-party imports, local imports
Files:
cli/src/infragpt/history.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: priyanshujain/infragpt#0
File: cli/CLAUDE.md:0-0
Timestamp: 2025-07-30T07:53:49.372Z
Learning: Applies to cli/llm.py : Use LangChain through the shared LLM module for prompt templates
🪛 GitHub Check: CodeQL
cli/src/infragpt/history.py
[failure] 78-78: Clear-text storage of sensitive information
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
This expression stores sensitive data (password) as clear text.
🔇 Additional comments (1)
cli/src/infragpt/history.py (1)
8-8: Import is appropriate
reis needed for the new sanitization logic.
…nsitive information Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation